Skip to content

Conversation

@bzhg
Copy link
Contributor

@bzhg bzhg commented Mar 25, 2022

Userspace may write the config control repeatedly in quick succession.
Accept the new config in cmd callback to avoid -EBUSY error and ensure
the last successful control write before prepare/copy is applied.

This patch also fixes an edge case during init where the first non-empty
control write happens after prepare. This patch ensures the config is
applied during copy.

Tested:

  1. Writes in quick succession
    for i in $(seq 1 100); do
    ./sof-ctl -n 92 -t 0 -b -r -s /root/test_blob0.txt &
    ./sof-ctl -n 92 -t 0 -b -r -s /root/test_blob1.txt &
    done

  2. Large config requiring multiple ipcs
    for i in $(seq 1 1000); do
    ./sof-ctl -n 92 -t 0 -b -r -s /root/large_blob.txt
    ./sof-ctl -n 92 -t 0 -b -r -s /root/test_blob1.txt
    done

  3. Reboot, write before and after arecord starts

  4. Suspend/resume

Signed-off-by: Ben Zhang benzh@chromium.org

Userspace may write the config control repeatedly in quick succession.
Accept the new config in cmd callback to avoid -EBUSY error and ensure
the last successful control write before prepare/copy is applied.

This patch also fixes an edge case during init where the first non-empty
control write happens after prepare. This patch ensures the config is
applied during copy.

Tested:
1) Writes in quick succession
for i in $(seq 1 100); do
    ./sof-ctl -n 92 -t 0 -b -r -s /root/test_blob0.txt &
    ./sof-ctl -n 92 -t 0 -b -r -s /root/test_blob1.txt &
done

2) Large config requiring multiple ipcs
for i in $(seq 1 1000); do
    ./sof-ctl -n 92 -t 0 -b -r -s /root/large_blob.txt
    ./sof-ctl -n 92 -t 0 -b -r -s /root/test_blob1.txt
done

3) Reboot, write before and after arecord starts
4) Suspend/resume

Signed-off-by: Ben Zhang <benzh@chromium.org>
@bzhg bzhg force-pushed the apmg3_fix_control branch from 2ba9686 to ec5775d Compare March 25, 2022 21:06
@bzhg bzhg requested a review from cujomalainey March 25, 2022 21:11
Copy link
Contributor

@lkoenig lkoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This look good to me.
I am still a bit itchy about the preemption but as it is possible at the moment, it should be fine.

@lkoenig
Copy link
Contributor

lkoenig commented Mar 28, 2022

I wonder if it would be possible in a follow-up PR to add locks around the config for the preemption case ?

@lgirdwood
Copy link
Member

I wonder if it would be possible in a follow-up PR to add locks around the config for the preemption case ?

Yes, if this is an atomic config change. Some processing component can keep the old config whilst the new one is being copied and then switch over by change the config pointer.

@cujomalainey cujomalainey merged commit 992b370 into thesofproject:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants