[Sound-open-firmware] [PATCH] sof: Add volume component state check
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called, but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
--- Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.git branch: topic/sof-v4.14 4881a4bd906f8b52bebd209b88ff920005550d53
firmware: git://git.alsa-project.org/sound-open-firmware.git branch: 1.1-stable 1f8c6c4d2018aac6994c75f60cee54765435d424
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff --- src/audio/volume.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/audio/volume.c b/src/audio/volume.c index df926c5..2652765 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -836,6 +836,9 @@ static int volume_copy(struct comp_dev *dev)
tracev_volume("cpy");
+ if (dev->state != COMP_STATE_ACTIVE) + return 0; + /* volume components will only ever have 1 source and 1 sink buffer */ source = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
On Tue, 2018-03-27 at 09:42 +0800, Wu Zhigang wrote:
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called, but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.gitt branch: topic/sof- v4.14 4881a4bd906f8b52bebd209b88ff920005550d53
firmware: git://git.alsa-project.org/sound-open-firmware.git branch: 1.1-stable 1f8c6c4d2018aac6994c75f60cee54765435d424
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff
src/audio/volume.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/audio/volume.c b/src/audio/volume.c index df926c5..2652765 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -836,6 +836,9 @@ static int volume_copy(struct comp_dev *dev)
tracev_volume("cpy");
- if (dev->state != COMP_STATE_ACTIVE)
return 0;
Zhigang, pipeline_copy_from_upstream() and pipeline_copy_to_downstream() both have the check to make sure we propogate copy only if the component is active. Will those not handle this situation as well?
- /* volume components will only ever have 1 source and 1 sink
buffer */ source = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
On 2018年03月27日 11:08, Ranjani Sridharan wrote:
On Tue, 2018-03-27 at 09:42 +0800, Wu Zhigang wrote:
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called, but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.gitt branch: topic/sof- v4.14 4881a4bd906f8b52bebd209b88ff920005550d53
firmware: git://git.alsa-project.org/sound-open-firmware.git branch: 1.1-stable 1f8c6c4d2018aac6994c75f60cee54765435d424
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff
src/audio/volume.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/audio/volume.c b/src/audio/volume.c index df926c5..2652765 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -836,6 +836,9 @@ static int volume_copy(struct comp_dev *dev)
tracev_volume("cpy");
- if (dev->state != COMP_STATE_ACTIVE)
return 0;Zhigang, pipeline_copy_from_upstream() and pipeline_copy_to_downstream() both have the check to make sure we propogate copy only if the component is active. Will those not handle this situation as well?
if the pipeline has more than one volume or other kinds of components, or the volume component is not the second component in the pipeline. this kind of check is not enough. so we have to add the state check in all of component's copy operation to avoid this issue in future.
thanks br --zhigang.wu
- /* volume components will only ever have 1 source and 1 sink
buffer */ source = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Tue, 2018-03-27 at 11:49 +0800, zhigang.wu wrote:
On 2018年03月27日 11:08, Ranjani Sridharan wrote:
On Tue, 2018-03-27 at 09:42 +0800, Wu Zhigang wrote:
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called, but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.gittt branch: topic/sof- v4.14 4881a4bd906f8b52bebd209b88ff920005550d53
firmware: git://git.alsa-project.org/sound-open- firmware.git branch: 1.1-stable 1f8c6c4d2018aac6994c75f60cee54765435d424
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff
src/audio/volume.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/audio/volume.c b/src/audio/volume.c index df926c5..2652765 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -836,6 +836,9 @@ static int volume_copy(struct comp_dev *dev)
tracev_volume("cpy");
- if (dev->state != COMP_STATE_ACTIVE)
return 0;Zhigang, pipeline_copy_from_upstream() and pipeline_copy_to_downstream() both have the check to make sure we propogate copy only if the component is active. Will those not handle this situation as well?
if the pipeline has more than one volume or other kinds of components, or the volume component is not the second component in the pipeline. this kind of check is not enough. so we have to add the state check in all of component's copy operation to avoid this issue in future.
Sorry Zhigang. I'm struggling to understand the problem. Could you please share your pipeline that you're seeing the issue with?
thanks br --zhigang.wu
- /* volume components will only ever have 1 source and 1
sink buffer */ source = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar e
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 2018年03月27日 12:14, Ranjani Sridharan wrote:
On Tue, 2018-03-27 at 11:49 +0800, zhigang.wu wrote:
On 2018年03月27日 11:08, Ranjani Sridharan wrote:
On Tue, 2018-03-27 at 09:42 +0800, Wu Zhigang wrote:
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called, but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.gittt branch: topic/sof- v4.14 4881a4bd906f8b52bebd209b88ff920005550d53
firmware: git://git.alsa-project.org/sound-open- firmware.git branch: 1.1-stable 1f8c6c4d2018aac6994c75f60cee54765435d424
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff
src/audio/volume.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/audio/volume.c b/src/audio/volume.c index df926c5..2652765 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -836,6 +836,9 @@ static int volume_copy(struct comp_dev *dev)
tracev_volume("cpy");
- if (dev->state != COMP_STATE_ACTIVE)
return 0;Zhigang, pipeline_copy_from_upstream() and pipeline_copy_to_downstream() both have the check to make sure we propogate copy only if the component is active. Will those not handle this situation as well?
if the pipeline has more than one volume or other kinds of components, or the volume component is not the second component in the pipeline. this kind of check is not enough. so we have to add the state check in all of component's copy operation to avoid this issue in future.
Sorry Zhigang. I'm struggling to understand the problem. Could you please share your pipeline that you're seeing the issue with?
This is not found out by special pipeline test. I just analysis it. But I think if we mask the IRQ in the pipeline cmd suggested by Liam. the state check is not needed in the component. thanks br --zhigang.wu
thanks br --zhigang.wu
- /* volume components will only ever have 1 source and 1
sink buffer */ source = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar e
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 27.03.2018 04:42, Wu Zhigang wrote:
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called,
Could this change be done to scheduler instead to avoid in every component need to implement this?
Thanks, Seppo
but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
Tested with apl-gpmrb board
kernel: https://github.com/plbossart/sound.git branch: topic/sof-v4.14 4881a4bd906f8b52bebd209b88ff920005550d53
firmware: git://git.alsa-project.org/sound-open-firmware.git branch: 1.1-stable 1f8c6c4d2018aac6994c75f60cee54765435d424
tools: branch: 1.1-stable cc91c73aa3e91eea35abdeb76d578b97f718feff
src/audio/volume.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/audio/volume.c b/src/audio/volume.c index df926c5..2652765 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -836,6 +836,9 @@ static int volume_copy(struct comp_dev *dev)
tracev_volume("cpy");
- if (dev->state != COMP_STATE_ACTIVE)
return 0;- /* volume components will only ever have 1 source and 1 sink buffer */ source = list_first_item(&dev->bsource_list, struct comp_buffer, sink_list); sink = list_first_item(&dev->bsink_list, struct comp_buffer, source_list);
On Tue, 2018-03-27 at 09:42 +0800, Wu Zhigang wrote:
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called, but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
This is no longer needed since we fixed the locking in your latest patch.
Liam
On 2018年03月27日 17:31, Liam Girdwood wrote:
On Tue, 2018-03-27 at 09:42 +0800, Wu Zhigang wrote:
Add the state check in volume_copy function, this can low down the xrun error possibility: For example: when firmware receive the PAUSE command, the HOST and VOLUME component are set to PAUSE state. at this time DMA IRQ happen, the pipeline is scheduled. host_copy() function will be called, but jump out immediately because of PAUSE state. but volume_copy() did not have this state check, the xrun will happen.
Signed-off-by: Wu Zhigang zhigang.wu@linux.intel.com Reviewed-by: Keyon Jie yang.jie@linux.intel.com
This is no longer needed since we fixed the locking in your latest patch.
Liam
yes, you are right!
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (5)
-
Liam Girdwood -
Ranjani Sridharan -
Seppo Ingalsuo -
Wu Zhigang -
zhigang.wu