[Sound-open-firmware] [PATCH] component: don't report xrun in comp_copy().
We don't need to report xrun every time it can't do real copy(buffer avail/free not enough), 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 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?"); + 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 */
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 ?
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 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 ?
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.
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't copy 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't copy this time */trace_volume_error("xo?");return 0; /* don't break the pipeline */}
/* copy and scale volume */
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@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 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;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 */
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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
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
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Liam Girdwood Sent: Wednesday, January 17, 2018 7:41 PM To: Keyon Jie yang.jie@linux.intel.com; sound-open-firmware@alsa- project.org Cc: Jie, Yang yang.jie@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@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.
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 :-
- The pipeline source component (usually host) must have at least 1
period of data free and 1 period avail (can be zeros or preloaded).
- The pipeline sink component (usually DAI) must have at least 1
period of data free and 1 period avail (zeros).
- 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.
- 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
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Wed, 2018-01-17 at 14:28 +0000, Jie, Yang wrote:
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Liam Girdwood Sent: Wednesday, January 17, 2018 7:41 PM To: Keyon Jie yang.jie@linux.intel.com; sound-open-firmware@alsa- project.org Cc: Jie, Yang yang.jie@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@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.
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:
- 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:
- pipeline_schdule_copy_idle();
- 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 :-
- The pipeline source component (usually host) must have at least 1
period of data free and 1 period avail (can be zeros or preloaded).
- The pipeline sink component (usually DAI) must have at least 1
period of data free and 1 period avail (zeros).
- 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.
- 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
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (3)
-
Jie, Yang -
Keyon Jie -
Liam Girdwood