[Sound-open-firmware] [PATCH] component: don't report xrun in comp_copy().

[Keyon Jie]

On 2018年01月16日 23:15, Liam Girdwood wrote:
> On Tue, 2018-01-16 at 22:05 +0800, Keyon Jie wrote:
>> We don't need to report xrun every time it can't do real
>> copy(buffer avail/free not enough),
> 
> Why ? what problem is this solving ?

It is solving fake xrun reporting issue.

> 
>>   e.g. we actually don't
>> need copy if sink buffer is full, we should do empty copy
>> and return success to let pipeline schedule continue.
>>
>> The xrun will be checked in scheduling component (usually
>> is dai) only and reported there.
>>
>> Todo: remove xrun check in other components also, e.g.
>> mixer, src, etc.
>>
>> Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
>> ---
>> Sanity test passed on minnow turbot with rt5651.
>> SOF #master: commit 83fec1559716d5a06137b43848abc18c244bc9e6
>> SOF Tool #master: commit a6bb8de907acd642302a227f403bb9fb2c18d075
>> Kernel: git at github.com:plbossart/sound.git #topic/sof-v4.14:
>>          commit 772ab0da7a8298d08edd42ab9a4f4177ec37aec6
>>
>>   src/audio/host.c   |  6 +++---
>>   src/audio/volume.c | 12 ++++++------
>>   2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/audio/host.c b/src/audio/host.c
>> index 7cba6df..0a047b3 100644
>> --- a/src/audio/host.c
>> +++ b/src/audio/host.c
>> @@ -622,9 +622,9 @@ static int host_copy(struct comp_dev *dev)
>>   
>>   		if (dma_buffer->free < local_elem->size) {
>>   			/* make sure there is free bytes for next period */
>> -			trace_host_error("xro");
>> -			comp_overrun(dev, dma_buffer, local_elem->size, 0);
>> -			return -EIO;
>> +			/* dont be too nervous, just warning and return */
>> +			trace_host("xo?");
> 
> How will host userspace know about XRUN ?

Well, we still have pipeline_xrun(), which will xrun() for host 
component, and then send IPC to host, and let userspace know about it.

> 
> Also this means that the xrun will propagate through the pipeline (as
> each sink/source may underrun or overrun until xrun clears) causing a
> longer overall audio artefact.

It may propagate, but before people do hear it, it doesn't matter.
And mark it as xrun and reset pointer earlier will introduce 
earlier(actually more frequently also), the recover itself also 
introduce pop noise.

But it we postpone this reporting, it may also auto recover it, which 
will benefit to user space, let me give a example here:

Suppose we are using pipeline as below:
host -- B0 -- vol -- B1 -- dai
and both B0 and B1 with 2 periods size.

1, At time t0:
B0.avail = 2(periods, same below);
B1.avail = 2;
that means buffers are both full.

2, At time t1, dai dma interrupt arrives:
dai_dma_cb() will update B1:
B0.avail = 1;
B1.avail = 2;

pipeline_schedule_copy() will be triggered, and then

host.copy(): B0 full, no need copy (DONT report fake xrun here...), 
return 0 ==>
volume.copy(): copy one period, and update buffers:
B0.avail = 1; (buffer not full, but we can still play)
B1.avail = 2;

3, the next dai dma interrupt not arrived yet, but 
pipeline_schedule_copy() is triggered for some other reason, then

host.copy():
B0.avail = 2; (buffer full again :)) return 0 ==>
volume.copy(): both B0 and B1 full, no need copy (DONT report fake xrun 
here...), return 0 ==>

...

n. dai found xrun in dai dma interrupt handler, will call 
pipeline_xrun(), and then xrun() for host component, and then send IPC 
to host.

Then xrun_recover() will reset pointers and playback again.


Doesn't our pipeline looks more robust here?


So, in summary, my change here actually plan to change change the 
definition of pipeline_schedule_copy() to be like this:

To iterate all components in the pipeline, 'TRY' to copy periods if 
possible for each component(some components may real copy, some others 
may not -- zero copy only, it doesn't matter, the watermark for each 
buffer can be different, what we care is if there is xrun on dai/SSP).



Thanks,
~Keyon


> 
> Liam
> 
>> +			return 0;
>>   		}
>>   	} else {
>>   
>> diff --git a/src/audio/volume.c b/src/audio/volume.c
>> index 05459b8..9ea3c92 100644
>> --- a/src/audio/volume.c
>> +++ b/src/audio/volume.c
>> @@ -577,14 +577,14 @@ static int volume_copy(struct comp_dev *dev)
>>   	 * the sink component buffer has enough free bytes for copy. Also
>>   	 * check for XRUNs */
>>   	if (source->avail < cd->source_period_bytes) {
>> -		trace_volume_error("xru");
>> -		comp_underrun(dev, source, cd->source_period_bytes, 0);
>> -		return -EIO;	/* xrun */
>> +		/* dont be too nervous, just warning if can't copy this time */
>> +		trace_volume("xu?");
>> +		return 0; /* don't break the pipeline */
>>   	}
>>   	if (sink->free < cd->sink_period_bytes) {
>> -		trace_volume_error("xro");
>> -		comp_overrun(dev, sink, cd->sink_period_bytes, 0);
>> -		return -EIO;	/* xrun */
>> +		/* dont be too nervous, just warning if can't copy this time */
>> +		trace_volume_error("xo?");
>> +		return 0; /* don't break the pipeline */
>>   	}
>>   
>>   	/* copy and scale volume */
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>