[PATCH] ALSA: pcm - introduce soc_delay

[PATCH] ALSA: pcm - introduce soc_delay

[Vinod Koul]

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;
-- 
1.7.0.4

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1224 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Jassi Brar]

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 ?

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Takashi Iwai]

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;
> +		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);

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;
>  		} 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;
> -- 
> 1.7.0.4
> 

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Vinod Koul]

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 :-) ?


-- 
~Vinod

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Vinod Koul]

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;
> > +		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);
> 
> 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;
> >  		} 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;
> > -- 
> > 1.7.0.4
> > 


-- 
~Vinod

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1327 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Jassi Brar]

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 ?

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Vinod Koul]

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 :)


-- 
~Vinod

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Vinod Koul]

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 
-- 
~Vinod

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Vinod Koul]

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!

-- 
~Vinod

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 393 bytes --]

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().

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Jassi Brar]

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 ?

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1383 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Jassi Brar]

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 ?

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Pierre-Louis Bossart]

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.

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Mark Brown]

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.

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Pierre-Louis Bossart]

> 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

Re: [PATCH] ALSA: pcm - introduce soc_delay

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1868 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]