[Sound-open-firmware] [PATCH] component: don't report xrun in comp_copy().
Liam Girdwood
liam.r.girdwood at linux.intel.com
Wed Jan 17 12:40:47 CET 2018
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.
>
> >
> > > 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.
>
> 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
> >
More information about the Sound-open-firmware
mailing list