[Sound-open-firmware] [PATCH] sof: Add volume component state check

[zhigang.wu]

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 at linux.intel.com>
>>>> Reviewed-by: Keyon Jie <yang.jie at 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 at alsa-project.org
>>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar
>>> e
>> _______________________________________________
>> Sound-open-firmware mailing list
>> Sound-open-firmware at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware