[alsa-devel] [PATCH] ALSA: pcm - introduce soc_delay
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com
-- Once we are okay with this approach, I will send a follow up patch which adds this notion in ASoC and uses this to compute cpu_dai delay. The codec_dai delay along with FIFO delay from cpu_dai should be added and reresented by today's notion of delay. --- include/sound/pcm.h | 1 + sound/core/pcm_lib.c | 14 +++++++++++--- sound/core/pcm_native.c | 6 +++--- 3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index a55d5db..405deb7 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -281,6 +281,7 @@ struct snd_pcm_runtime { unsigned long hw_ptr_jiffies; /* Time when hw_ptr is updated */ unsigned long hw_ptr_buffer_jiffies; /* buffer time in jiffies */ snd_pcm_sframes_t delay; /* extra delay; typically FIFO size */ + snd_pcm_sframes_t soc_delay; /* extra delay; typically delay incurred in soc */
/* -- HW params -- */ snd_pcm_access_t access; /* access mode */ diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 8f312fa..4977012 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -292,7 +292,15 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, return -EPIPE; } } else { - if (avail >= runtime->stop_threshold) { + snd_pcm_uframes_t actual_avail; + if (avail < runtime->soc_delay) + actual_avail = avail; + else + actual_avail = avail - runtime->soc_delay; + if (actual_avail >= runtime->stop_threshold) { + snd_printd(KERN_ERR "avail > stop_threshold!!\n"); + snd_printd(KERN_ERR "actual_avail %ld, avail %ld, soc_delay %ld!!\n", + actual_avail, avail, runtime->soc_delay); xrun(substream); return -EPIPE; } @@ -440,9 +448,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, if (runtime->hw.info & SNDRV_PCM_INFO_BATCH) goto no_jiffies_check; hdelta = delta; - if (hdelta < runtime->delay) + if (hdelta < (runtime->delay + runtime->soc_delay)) goto no_jiffies_check; - hdelta -= runtime->delay; + hdelta -= (runtime->delay + runtime->soc_delay); jdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (((hdelta * HZ) / runtime->rate) > jdelta + HZ/100) { delta = jdelta / diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 53b5ada..fc2d664 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -606,13 +606,13 @@ int snd_pcm_status(struct snd_pcm_substream *substream, if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || runtime->status->state == SNDRV_PCM_STATE_DRAINING) { status->delay = runtime->buffer_size - status->avail; - status->delay += runtime->delay; + status->delay += runtime->delay + runtime->soc_delay; } else status->delay = 0; } else { status->avail = snd_pcm_capture_avail(runtime); if (runtime->status->state == SNDRV_PCM_STATE_RUNNING) - status->delay = status->avail + runtime->delay; + status->delay = status->avail + runtime->delay + runtime->soc_delay; else status->delay = 0; } @@ -2442,7 +2442,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream, n = snd_pcm_playback_hw_avail(runtime); else n = snd_pcm_capture_avail(runtime); - n += runtime->delay; + n += runtime->delay + runtime->soc_delay; break; case SNDRV_PCM_STATE_XRUN: err = -EPIPE;
On Mon, Jul 23, 2012 at 03:36:37PM +0530, Vinod Koul wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we
This isn't particularly new, we've had SRAM buffers in SoCs for ages.
have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
It's not immediately clear to me that we shouldn't be flagging an overrun here - obviously if there's just a pure delay introduced by the DSP then we will underrun if we catch up with the input, it's just that there will be a delay in the user hearing it. It seems like the thing we need to say here is more that the DSP or DMA knows where the application pointer is and can halt the DMA when it underruns, together with the fact that we have some buffering beyond the application visible DMA which means that an underrun isn't (yet) a user audible problem. We'd also want some other way of saying that that these further buffers did overrun.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
soc_delay seems like a very unclear name for this.
On Mon, 2012-07-23 at 11:18 +0100, Mark Brown wrote:
On Mon, Jul 23, 2012 at 03:36:37PM +0530, Vinod Koul wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we
This isn't particularly new, we've had SRAM buffers in SoCs for ages.
have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
It's not immediately clear to me that we shouldn't be flagging an overrun here - obviously if there's just a pure delay introduced by the DSP then we will underrun if we catch up with the input, it's just that there will be a delay in the user hearing it. It seems like the thing we need to say here is more that the DSP or DMA knows where the application pointer is and can halt the DMA when it underruns, together with the fact that we have some buffering beyond the application visible DMA which means that an underrun isn't (yet) a user audible problem. We'd also want some other way of saying that that these further buffers did overrun.
having DMA know where app_pointer helps in detection of overrun in dsp. But that's a separate topic.
Take the case of a DSP which employs DMA on input and output (of the DSP) At start, DSP would buffer some data, assuming a period. And assume app has filed one period only. So at start as we buffered a period, ALSA threw up wrong xrun, as it wasn't aware that this buffer is not yet rendered.
Using this notion, ALSA knows that there is some buffer in DSP and used that for xrun as well as delay calculations.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
soc_delay seems like a very unclear name for this.
I am not good with names, perhaps soc_buffer is good name :-) ?
On Mon, Jul 23, 2012 at 04:09:06PM +0530, Vinod Koul wrote:
On Mon, 2012-07-23 at 11:18 +0100, Mark Brown wrote:
It's not immediately clear to me that we shouldn't be flagging an overrun here - obviously if there's just a pure delay introduced by the
Take the case of a DSP which employs DMA on input and output (of the DSP) At start, DSP would buffer some data, assuming a period. And assume app has filed one period only. So at start as we buffered a period, ALSA threw up wrong xrun, as it wasn't aware that this buffer is not yet rendered.
Using this notion, ALSA knows that there is some buffer in DSP and used that for xrun as well as delay calculations.
I'm having a hard time relating this to what I was saying. The point here is that if the device keeps marching on consuming data (as most cyclic DMAs would) then there's still going to be an underrun even if there's a buffer that causes a delay in the user hearing it.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
soc_delay seems like a very unclear name for this.
I am not good with names, perhaps soc_buffer is good name :-) ?
The "soc" is the problem here - this shouldn't be specific to some sort of hardware.
On Mon, 2012-07-23 at 11:47 +0100, Mark Brown wrote:
On Mon, Jul 23, 2012 at 04:09:06PM +0530, Vinod Koul wrote:
On Mon, 2012-07-23 at 11:18 +0100, Mark Brown wrote:
It's not immediately clear to me that we shouldn't be flagging an overrun here - obviously if there's just a pure delay introduced by the
Take the case of a DSP which employs DMA on input and output (of the DSP) At start, DSP would buffer some data, assuming a period. And assume app has filed one period only. So at start as we buffered a period, ALSA threw up wrong xrun, as it wasn't aware that this buffer is not yet rendered.
Using this notion, ALSA knows that there is some buffer in DSP and used that for xrun as well as delay calculations.
I'm having a hard time relating this to what I was saying. The point here is that if the device keeps marching on consuming data (as most cyclic DMAs would) then there's still going to be an underrun even if there's a buffer that causes a delay in the user hearing it.
yes there would be overrun but that is correct one. At that point I can see that the DSP would have rendered everything it has (all buffers exhausted). At this point app_pointer = hw_pointer and soc_delay would be zero. I am assuming dsp driver reports the soc_delay dynamically as it would do for hw_pointer.
This doesn't stop all underruns, they would still be reported.
This only prevents false underrun when DSP still has something to process and give to output DMA.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
soc_delay seems like a very unclear name for this.
I am not good with names, perhaps soc_buffer is good name :-) ?
The "soc" is the problem here - this shouldn't be specific to some sort of hardware.
Hmmm, yes this can happen outside soc's as well. dsp_buffer is my next candidate :)
On Mon, Jul 23, 2012 at 04:36:18PM +0530, Vinod Koul wrote:
On Mon, 2012-07-23 at 11:47 +0100, Mark Brown wrote:
I'm having a hard time relating this to what I was saying. The point here is that if the device keeps marching on consuming data (as most cyclic DMAs would) then there's still going to be an underrun even if there's a buffer that causes a delay in the user hearing it.
yes there would be overrun but that is correct one. At that point I can see that the DSP would have rendered everything it has (all buffers exhausted). At this point app_pointer = hw_pointer and soc_delay would be zero.
I am assuming dsp driver reports the soc_delay dynamically as it would do for hw_pointer.
This is sounding exactly like the existing delay parameter and how it's handled in ASoC.
This doesn't stop all underruns, they would still be reported.
This only prevents false underrun when DSP still has something to process and give to output DMA.
I don't think you're quite getting my point here. The DSP still has something to process but unless whatever is feeding the data to the DSP stops sending data to the DSP when it catches up with where the application is writing then there will still be a glitch.
Reporting the delay is useful anyway but it's not the whole picture as far as recovering from and masking the underrun is concerned.
On 7/23/2012 5:47 AM, Mark Brown wrote:
I'm having a hard time relating this to what I was saying. The point here is that if the device keeps marching on consuming data (as most cyclic DMAs would) then there's still going to be an underrun even if there's a buffer that causes a delay in the user hearing it
Not necessarily. The DMA between system memory and DSP buffers need not work at a rate linked to the serial bit clock, they can be much faster, and actually they should to help put the system in a low power state rather than reading continuously from memory... what Vinod is trying to explain is that due to the bursty nature of data transfers inside the soc, we need to modify how the accounting is done.
On Mon, Jul 23, 2012 at 02:51:48PM -0500, Pierre-Louis Bossart wrote:
On 7/23/2012 5:47 AM, Mark Brown wrote:
Don't drop CCs from replies.
I'm having a hard time relating this to what I was saying. The point here is that if the device keeps marching on consuming data (as most cyclic DMAs would) then there's still going to be an underrun even if there's a buffer that causes a delay in the user hearing it
Not necessarily. The DMA between system memory and DSP buffers need not work at a rate linked to the serial bit clock, they can be much faster, and actually they should to help put the system in a low power state rather than reading continuously from memory... what
I'm aware of this, thanks.
Vinod is trying to explain is that due to the bursty nature of data transfers inside the soc, we need to modify how the accounting is done.
Right, but this depends on the ability of the device to pause reading data when it reads up to the point where the application has written. This is a separate capability to any latency that's been added by the buffering, and most of the systems that have the buffering don't have this capability but instead either don't report the buffer or rely on the application being a full buffer ahead of the hardware.
Don't drop CCs from replies.
Sorry.
Vinod is trying to explain is that due to the bursty nature of data transfers inside the soc, we need to modify how the accounting is done.
Right, but this depends on the ability of the device to pause reading data when it reads up to the point where the application has written. This is a separate capability to any latency that's been added by the buffering, and most of the systems that have the buffering don't have this capability but instead either don't report the buffer or rely on the application being a full buffer ahead of the hardware.
We don't have such fancy hardware (and I don't think anyone has). This can happen even with simple IP that has an embedded SRAM and bursty DMA, if the IP buffer amounts to the period size to avoid partial wakes or transfers, and the application cannot provide more than one period initially you get an underflow that isn't a true one. -Pierre
On Mon, Jul 23, 2012 at 03:16:28PM -0500, Pierre-Louis Bossart wrote:
Right, but this depends on the ability of the device to pause reading data when it reads up to the point where the application has written. This is a separate capability to any latency that's been added by the buffering, and most of the systems that have the buffering don't have this capability but instead either don't report the buffer or rely on the application being a full buffer ahead of the hardware.
We don't have such fancy hardware (and I don't think anyone has). This can happen even with simple IP that has an embedded SRAM and bursty DMA, if the IP buffer amounts to the period size to avoid
The usual way of representing this is to have the pointers in the buffer represent where the input to the hardware pipeline is at (ie, where the actual DMA is reading) and handle underflow on that normally.
partial wakes or transfers, and the application cannot provide more than one period initially you get an underflow that isn't a true one.
Normally this is handled by requiring that the application has at least one period ready for the hardware at all times, otherwise as you get down towards the end of the buffer things get racier and racier. It's not really about the delay, either - it's about when the DMA wants to read the memory which isn't quite the same thing. It could be that it does this with half the buffer remaining, it could be that it waits until it's got some much smaller amount of data left.
I think what's really needed here is to represent the buffer in the hardware and how it's filled up to the stack more directly. It's not the fact that there is data buffered that we need to tell the stack about (we can already do that), what we need to tell it is that the trigger level for refilling that may not be what's expected.
On 23 July 2012 16:09, Vinod Koul vinod.koul@linux.intel.com wrote:
Take the case of a DSP which employs DMA on input and output (of the DSP) At start, DSP would buffer some data, assuming a period. And assume app has filed one period only. So at start as we buffered a period, ALSA threw up wrong xrun, as it wasn't aware that this buffer is not yet rendered.
I thought the data was supposed to leave the ring-buffer only _after_ the TRIGGER_START call ?
On Mon, Jul 23, 2012 at 3:36 PM, Vinod Koul vinod.koul@linux.intel.com wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
Couldn't we employ snd_pcm_hw_params.fifo_size for the purpose ?
On Mon, 2012-07-23 at 15:49 +0530, Jassi Brar wrote:
On Mon, Jul 23, 2012 at 3:36 PM, Vinod Koul vinod.koul@linux.intel.com wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
Couldn't we employ snd_pcm_hw_params.fifo_size for the purpose ?
Nope, that should be used to represent the delay in samples being output not the sample which are buffered and processed in a DSP
On 23 July 2012 16:09, Vinod Koul vinod.koul@linux.intel.com wrote:
On Mon, 2012-07-23 at 15:49 +0530, Jassi Brar wrote:
On Mon, Jul 23, 2012 at 3:36 PM, Vinod Koul vinod.koul@linux.intel.com wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
Couldn't we employ snd_pcm_hw_params.fifo_size for the purpose ?
Nope, that should be used to represent the delay in samples being output not the sample which are buffered and processed in a DSP
After leaving alsa ring buffer, don't the samples incur extra delay of your internal buffer ?
On Mon, 2012-07-23 at 16:20 +0530, Jassi Brar wrote:
Couldn't we employ snd_pcm_hw_params.fifo_size for the purpose ?
Nope, that should be used to represent the delay in samples being
output
not the sample which are buffered and processed in a DSP
After leaving alsa ring buffer, don't the samples incur extra delay of your internal buffer ?
Yes they do.
And they are two types. a) constant delay in DMA FIFO, aptly represented by delay b) dynamic buffering on input DMA at DSP and during post-processing. This is before the circular DMA at output, and this patch attempts to represent this buffering.
I guess soc_delay is not a best of name and is causing quite a churn!
On Mon, Jul 23, 2012 at 04:47:07PM +0530, Vinod Koul wrote:
a) constant delay in DMA FIFO, aptly represented by delay b) dynamic buffering on input DMA at DSP and during post-processing. This is before the circular DMA at output, and this patch attempts to represent this buffering.
The delay in the runtime can vary too, in ASoC it's currently updated every time we call pointer().
On 23 July 2012 16:47, Vinod Koul vinod.koul@linux.intel.com wrote:
On Mon, 2012-07-23 at 16:20 +0530, Jassi Brar wrote:
Couldn't we employ snd_pcm_hw_params.fifo_size for the purpose ?
Nope, that should be used to represent the delay in samples being
output
not the sample which are buffered and processed in a DSP
After leaving alsa ring buffer, don't the samples incur extra delay of your internal buffer ?
Yes they do.
And they are two types. a) constant delay in DMA FIFO, aptly represented by delay b) dynamic buffering on input DMA at DSP and during post-processing. This is before the circular DMA at output, and this patch attempts to represent this buffering.
Correct, but I think (a) and (b) are not that different. I too might like to buy some time when the ring buffer is empty but my I2S controller's "deep fifo" still has a few millisecs of data 'buffered'. I don't see how your scenario is effectively different from mine. And I can't see how it matters if the the "DSP buffer" is before or after the ring-buffer. So perhaps we should abstract out any data outside of the ring-buffer to be in "fifo" and start taking fifo_size into account before warning XRUNs ?
At Mon, 23 Jul 2012 15:36:37 +0530, Vinod Koul wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com
-- Once we are okay with this approach, I will send a follow up patch which adds this notion in ASoC and uses this to compute cpu_dai delay. The codec_dai delay along with FIFO delay from cpu_dai should be added and reresented by today's notion of delay.
Hmm, it's confusing to have both delay and soc_delay fields.
And, if the XRUN detection is the only problem, we can provide a flag to correct avail with runtime->delay.
include/sound/pcm.h | 1 + sound/core/pcm_lib.c | 14 +++++++++++--- sound/core/pcm_native.c | 6 +++--- 3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index a55d5db..405deb7 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -281,6 +281,7 @@ struct snd_pcm_runtime { unsigned long hw_ptr_jiffies; /* Time when hw_ptr is updated */ unsigned long hw_ptr_buffer_jiffies; /* buffer time in jiffies */ snd_pcm_sframes_t delay; /* extra delay; typically FIFO size */
snd_pcm_sframes_t soc_delay; /* extra delay; typically delay incurred in soc */
/* -- HW params -- */ snd_pcm_access_t access; /* access mode */
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 8f312fa..4977012 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -292,7 +292,15 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, return -EPIPE; } } else {
if (avail >= runtime->stop_threshold) {
snd_pcm_uframes_t actual_avail;if (avail < runtime->soc_delay)actual_avail = avail;elseactual_avail = avail - runtime->soc_delay;if (actual_avail >= runtime->stop_threshold) {snd_printd(KERN_ERR "avail > stop_threshold!!\n");snd_printd(KERN_ERR "actual_avail %ld, avail %ld, soc_delay %ld!!\n",actual_avail, avail, runtime->soc_delay);
Don't add debug prints here. This is no kernel error, and XRUN is already informed in xrun() function.
thanks,
Takashi
xrun(substream); return -EPIPE; }@@ -440,9 +448,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, if (runtime->hw.info & SNDRV_PCM_INFO_BATCH) goto no_jiffies_check; hdelta = delta;
- if (hdelta < runtime->delay)
- if (hdelta < (runtime->delay + runtime->soc_delay)) goto no_jiffies_check;
- hdelta -= runtime->delay;
- hdelta -= (runtime->delay + runtime->soc_delay); jdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (((hdelta * HZ) / runtime->rate) > jdelta + HZ/100) { delta = jdelta /
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 53b5ada..fc2d664 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -606,13 +606,13 @@ int snd_pcm_status(struct snd_pcm_substream *substream, if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || runtime->status->state == SNDRV_PCM_STATE_DRAINING) { status->delay = runtime->buffer_size - status->avail;
status->delay += runtime->delay;
status->delay += runtime->delay + runtime->soc_delay;
status->delay = status->avail + runtime->delay;
status->delay = status->avail + runtime->delay + runtime->soc_delay;@@ -2442,7 +2442,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream, n = snd_pcm_playback_hw_avail(runtime); else n = snd_pcm_capture_avail(runtime);
n += runtime->delay;
n += runtime->delay + runtime->soc_delay;-- 1.7.0.4
On Mon, 2012-07-23 at 12:27 +0200, Takashi Iwai wrote:
At Mon, 23 Jul 2012 15:36:37 +0530, Vinod Koul wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com
-- Once we are okay with this approach, I will send a follow up patch which adds this notion in ASoC and uses this to compute cpu_dai delay. The codec_dai delay along with FIFO delay from cpu_dai should be added and reresented by today's notion of delay.
Hmm, it's confusing to have both delay and soc_delay fields.
Yes, I agree but delay doesn't represent this and we should still keep delay for knowing the delay in samples getting rendered. This should be sum of output DMA FIFO delay and codec delay
And, if the XRUN detection is the only problem, we can provide a flag to correct avail with runtime->delay.
For start yes, but from a correctness perspective we should tell ALSA how much data is buffered. It also helps when we would like to do rewind and forward (not handled in this patch yet)
include/sound/pcm.h | 1 + sound/core/pcm_lib.c | 14 +++++++++++--- sound/core/pcm_native.c | 6 +++--- 3 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index a55d5db..405deb7 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -281,6 +281,7 @@ struct snd_pcm_runtime { unsigned long hw_ptr_jiffies; /* Time when hw_ptr is updated */ unsigned long hw_ptr_buffer_jiffies; /* buffer time in jiffies */ snd_pcm_sframes_t delay; /* extra delay; typically FIFO size */
snd_pcm_sframes_t soc_delay; /* extra delay; typically delay incurred in soc */
/* -- HW params -- */ snd_pcm_access_t access; /* access mode */
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 8f312fa..4977012 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -292,7 +292,15 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, return -EPIPE; } } else {
if (avail >= runtime->stop_threshold) {
snd_pcm_uframes_t actual_avail;if (avail < runtime->soc_delay)actual_avail = avail;elseactual_avail = avail - runtime->soc_delay;if (actual_avail >= runtime->stop_threshold) {snd_printd(KERN_ERR "avail > stop_threshold!!\n");snd_printd(KERN_ERR "actual_avail %ld, avail %ld, soc_delay %ld!!\n",actual_avail, avail, runtime->soc_delay);Don't add debug prints here. This is no kernel error, and XRUN is already informed in xrun() function.
thanks,
Takashi
xrun(substream); return -EPIPE; }@@ -440,9 +448,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, if (runtime->hw.info & SNDRV_PCM_INFO_BATCH) goto no_jiffies_check; hdelta = delta;
- if (hdelta < runtime->delay)
- if (hdelta < (runtime->delay + runtime->soc_delay)) goto no_jiffies_check;
- hdelta -= runtime->delay;
- hdelta -= (runtime->delay + runtime->soc_delay); jdelta = curr_jiffies - runtime->hw_ptr_jiffies; if (((hdelta * HZ) / runtime->rate) > jdelta + HZ/100) { delta = jdelta /
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 53b5ada..fc2d664 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -606,13 +606,13 @@ int snd_pcm_status(struct snd_pcm_substream *substream, if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || runtime->status->state == SNDRV_PCM_STATE_DRAINING) { status->delay = runtime->buffer_size - status->avail;
status->delay += runtime->delay;
status->delay += runtime->delay + runtime->soc_delay;
status->delay = status->avail + runtime->delay;
status->delay = status->avail + runtime->delay + runtime->soc_delay;@@ -2442,7 +2442,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream, n = snd_pcm_playback_hw_avail(runtime); else n = snd_pcm_capture_avail(runtime);
n += runtime->delay;
n += runtime->delay + runtime->soc_delay;-- 1.7.0.4
Date 23.7.2012 12:27, Takashi Iwai wrote:
At Mon, 23 Jul 2012 15:36:37 +0530, Vinod Koul wrote:
In many modern SoCs the audio DSP can buffer the PCM ring buffer data. Today we have no means to represent this buffering and ALSA wrongly detects an overrun when hw_ptr reaches app_ptr value, though DSP may still have some buffered data.
This patch tries to add a new field "soc_delay" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA.
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com
-- Once we are okay with this approach, I will send a follow up patch which adds this notion in ASoC and uses this to compute cpu_dai delay. The codec_dai delay along with FIFO delay from cpu_dai should be added and reresented by today's notion of delay.
Hmm, it's confusing to have both delay and soc_delay fields.
And, if the XRUN detection is the only problem, we can provide a flag to correct avail with runtime->delay.
I agree. The runtime->delay variable should handle all situations for queued samples.
But the point is - we have data in some FIFO, but the DMA ring buffer is empty. If you use the delay value in the snd_pcm_update_state(), you'll probably allow to transfer invalid samples to the FIFO (probably previous samples from the ring buffer, because application has not provided new ones). That was the purpose of the check.
Of course, if you stop DMA according the appl_ptr and wait on application to provide new data in the "FIFO time window", then the check should be updated for this type of hw.
Jaroslav
On Mon, 2012-07-23 at 12:47 +0200, Jaroslav Kysela wrote:
mm, it's confusing to have both delay and soc_delay fields.
And, if the XRUN detection is the only problem, we can provide a flag to correct avail with runtime->delay.
I agree. The runtime->delay variable should handle all situations for queued samples. But the point is - we have data in some FIFO, but the DMA ring buffer is empty.
In a DSP you can have input buffer (from the ring buffer), which is post processed and sent to output DMA buffer. This buffering is dynamic in nature and since input buffer is memory copy, it would buffer ahead of time unlike the output buffer which will run at PCM rate. The samples at input DMA and various modules doing DSP processing are still before output circular DMA and should be considered when we do xrun calculation.
If you use the delay value in the snd_pcm_update_state(), you'll probably allow to transfer invalid samples to the FIFO (probably previous samples from the ring buffer, because application has not provided new ones). That was the purpose of the check.
Yes and the right one. But what about samples still there before DMA, this patch just attempts to represent those. not the ones at and after DMA, they are correctly represented today in runtime->delay.
Of course, if you stop DMA according the appl_ptr and wait on application to provide new data in the "FIFO time window", then the check should be updated for this type of hw.
Jaroslav
participants (7)
-
Jaroslav Kysela -
Jassi Brar -
Jassi Brar -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Vinod Koul