[Sound-open-firmware] [PATCH] component: don't report xrun in comp_copy().
Liam Girdwood
liam.r.girdwood at linux.intel.com
Wed Jan 17 21:27:13 CET 2018
On Wed, 2018-01-17 at 14:28 +0000, Jie, Yang wrote:
> > -----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).
ok, so you mean GW host DMA. This is fine as the Host GW copy() wont
actually control/initiate the copies. It will always return success.
>
> >
> > >
> > > >
> > > > > 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).
Ok, lets document it. If you think about a component in a pipeline it
must always have at least 1 period avail (for sink reader comp) and 1
free (for source writer comp) other wise it will XRUN.
>
> 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?
The rule is
1) Buffer with >= 2 periods must always at least have 1 free and 1
avail periods.
2) Buffer with 1 period (i.e. low latency) should always be free at
start of schedule. This then pushes the data period downstream from
source comp to sink comp (via other comps with 1 period buffers) in a
single schedule call.
>
> 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.
This is called at stream start i.e. trigger. so data will be ready at
host side (host buffers are actually ready at prepare()). i.e. you can
start your host GW in prepare() and have the data in DSP for trigger
(this would lower latency too).
>
> 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...
and it's adds 1 extra period of latency.
The best plan is to copy the 2 periods to host GW buffer during
prepare() so that they are in B0 for trigger(start) - we dont need to
support legacy HDA so you can rework the host sequence.
Liam
> ...
>
> #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
>
> _______________________________________________
> 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