[Sound-open-firmware] [PATCH] component: don't report xrun in comp_copy().
Jie, Yang
yang.jie at intel.com
Wed Jan 17 15:28:34 CET 2018
>-----Original Message-----
>From: sound-open-firmware-bounces at alsa-project.org [mailto:sound-open-
>firmware-bounces at alsa-project.org] On Behalf Of Liam Girdwood
>Sent: Wednesday, January 17, 2018 7:41 PM
>To: Keyon Jie <yang.jie at linux.intel.com>; sound-open-firmware at alsa-
>project.org
>Cc: Jie, Yang <yang.jie at intel.com>
>Subject: Re: [Sound-open-firmware] [PATCH] component: don't report xrun in
>comp_copy().
>
>On Wed, 2018-01-17 at 15:01 +0800, Keyon Jie wrote:
>>
>> 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.
>
>Sorry, do you mean XRUN notifications are being sent to the host when
>there is no XRUN ? If so, in what situation.
I am raising this as in host dma case, it can't copy only 1 period one time,
that means, we need copy the full buffer(e.g. 2 periods) at one time, otherwise,
otherwise the dma won't finish the copying(BF bit won't be set).
>
>>
>> >
>> > > 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.
>>
>
>We cant rely on XRUNs being sent by a non XRUN initiating component.
>The XRUN source must be the first component that notifies.
>
>> >
>> > 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.
>>
>
>Yes, we cant get away from introducing audio artefacts when an XRUN
>occurs but the sooner we detect and act on the XRUN the better. This
>way we can propagate the XRUN to other compoents and mute the output
>before the XRUN is rendered to the DAI.
>
>> 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.
>
>The current code state prior to trigger start is
>
>B0.avail = 1 (preloaded OR can be zeros)
>B1.avail = 1 (all zeros)
>
>This means each buffer (with >=2 periods in size) has at least 1 period
>avail and 1 period free.
>
>If this is not the case then we need to fix this.
Well, adding limitation to the avail/free of each buffer will do simplify
our logic(we may need to document it).
So, we should make sure:
1. there are at least 2 periods for each buffer(don't using buffer with
only 1 period?);
2. there are at least 1 period avail and 1 period free for each buffer,
before the pipeline_schedule() is called? Or its source component's
.copy() is called? Or its sink component's .copy() is called?
And, for the first pipeline_schdule_copy_idle() in cmd update for
start, it looks like hack, I suggest to do similar stuff(e.g. reset
pointers) in .prepare(), as pipeline_schdule_copy_idle() here is
unfriendly for host dma, we can't do real copy here as the start
bit in host side is not set yet.
Imagine the case I mentioned for host dma above, I prefer we
do it like this:
host -- B0 -- vol -- B1 -- dai
#0:
at .prepare(), reset buffer pointers:
B0.avail = 1 (zeros); -- can't be 0, as it may cause dai underrun
when the pipeline is pass-through (host - B0 -- dai);
B1.avail = 2 (all zeros), can't be 1, otherwise underrun
happens at step #3.
...
Don't need pipeline_schdule_copy_idle() anymore...
...
#1:
At trigger start,
(For Host: set COMP_STATE_ACTIVE, return;)
But For Host_gw: set ADSP side start bit, set COMP_STATE_ACTIVE,
return;
Volume: set COMP_STATE_ACTIVE, return;
Dai: dma config & start, set COMP_STATE_ACTIVE, return;
Return success to host side for trigger start.
#2, host side driver will set host side start bit
/* this will trigger the first full 2 periods buffer copy*/
But we can't update B0 pointer here ... :(
#3:
At dai dma handler, update B1 buffer pointers:
B0.avail = 1;
B1.avail = 1;
#4:
Then schedule pipeline_schdule_copy():
At host_copy(),
B0.avail = 1, we canNOT copy for the reason I mentioned
above, let me return 0;
At volume_copy(), update buffer pointers:
B0.avail = 0;
B1.avail = 2;
...
Dai dma interrupt, go to #3 again:
#3':
At dai dma handler, update B1 buffer pointers:
B0.avail = 0;
B1.avail = 1;
#4':
schedule pipeline_schdule_copy() again:
At host_copy(),
B0.avail = 0, we will copy 2 periods, and wait until it finish here,
then update B0:
B0.avail = 2, return 0;
At volume_copy(), update buffer pointers:
B0.avail = 1;
B1.avail = 2;
...
Dai dma interrupt, go to #3 the 3rd time:
#3'':
At dai dma handler, update B1 buffer pointers:
B0.avail = 1;
B1.avail = 1;
#4'':
Then schedule pipeline_schdule_copy():
At host_copy(),
B0.avail = 1, we canNOT copy for the reason I mentioned
above, let me return 0;
At volume_copy(), update buffer pointers:
B0.avail = 0;
B1.avail = 2;
So here #3''/#4'' are actually same with #3/#4, it will loop
#3 -> #4 -> #3' -> #4' -> #3 -> #4 -> ...
With this solution, the host dma can works fine.
So what change I need here is:
1. pipeline_schdule_copy_idle();
2. reset buffer pointers to proper value, suppose we have
N + 1 Buffers in total(host -> B0 -> ... BN --> dai), we should
reset them as:
B0.avail = 1;
B1.avail = 1;
...
B(N-1).avail = 1;
BN.avail = 2;
But for the case N = 0(host -> B0 -> dai), how can I handle it?
Maybe we should allocate at least 3 periods for host gateway?
Thanks,
~Keyon
>
>>
>> 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.
>>
>
>One thing that XRUN recover should do (that needs implemented) is to
>take a snapshot of the pipeline position at the last XRUN to try and
>determine if XRUN recovery is succeeding for this pipeline (i.e. a
>pipeline that is continually stuck at the same position is probably
>badly defined in topology).
>
>>
>> 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).
>>
>
>No, we need to be more deterministic. Introducing logic that *may* take
>a different course of action on every schedule or at stream start/stop
>adds unnecessary complexity.
>
>At stream start :-
>
>1) The pipeline source component (usually host) must have at least 1
>period of data free and 1 period avail (can be zeros or preloaded).
>
>2) The pipeline sink component (usually DAI) must have at least 1
>period of data free and 1 period avail (zeros).
>
>3) The pipeline can now be started AND scheduled immediately. It will
>play the avail sink (DAI) period AND concurrently process the preloaded
>source (host) avail period to the current sink free period.
>
>4) DAI IRQ occurs and pipeline is scheduled again. Pipeline source and
>sink buffer/period state is the same as 1 above.
>
>i.e. on *every* call to schedule (including start schedule)
>
>B0 - 1 avail, 1 free
>B1 - 1 avail, 1 free
>
>Liam
>
>>
>>
>> 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-firmwar
>> > e
>> >
>_______________________________________________
>Sound-open-firmware mailing list
>Sound-open-firmware at alsa-project.org
>http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
More information about the Sound-open-firmware
mailing list