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@linux.intel.com
Sanity test passed on minnow turbot with rt5651. SOF #master: commit 83fec1559716d5a06137b43848abc18c244bc9e6 SOF Tool #master: commit a6bb8de907acd642302a227f403bb9fb2c18d075 Kernel: git@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 fornext period */
trace_host_error("xro");comp_overrun(dev, dma_buffer,local_elem->size, 0);
return -EIO;
/* dont be too nervous, just warning andreturn */
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;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'tcopy this time */
trace_volume("xu?");return 0; /* don't break the pipeline */
trace_volume_error("xro");comp_overrun(dev, sink, cd->sink_period_bytes,0);
return -EIO; /* xrun */
/* dont be too nervous, just warning if can'tcopy 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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar e