[PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Shine Liu]

There's no restriction on that the runtime->period_size should be the
multiplicator of the runtime->buffer_size in current kernel code.

So "hw_ptr_interrupt = runtime->hw_ptr_interrupt +
runtime->period_size;" is not always the trueth.

I've encounter the problem of this bug, the kernel told me "PCM:
Unexpected hw_pointer value (stream=0, pos=8, intr_ptr=16384)".

The period_size and buffer_size I used was 0x800 and 0x3e80 when the bug
occured.

Following patch will fix the bug.



Signed-off-by: Shine Liu <shinel@foxmail.com>
-----------------------------------------------------------------------

--- a/sound/core/pcm_lib.c	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/core/pcm_lib.c	2009-08-19 17:59:00.000000000 +0800
@@ -248,6 +248,12 @@
 	hw_base = runtime->hw_ptr_base;
 	new_hw_ptr = hw_base + pos;
 	hw_ptr_interrupt = runtime->hw_ptr_interrupt + runtime->period_size;
+	/* if the period_size is not the multiplicator of the 
+ 	 * buffer_size, the hw_ptr_interrupt calculated above 
+ 	 * may not be correct, should be fixed. 
+ 	 */
+	if(hw_ptr_interrupt > hw_base + runtime->buffer_size)
+		hw_ptr_interrupt = hw_base + runtime->buffer_size;
 	delta = new_hw_ptr - hw_ptr_interrupt;
 	if (hw_ptr_interrupt >= runtime->boundary) {
 		hw_ptr_interrupt -= runtime->boundary;

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Takashi Iwai]

At Wed, 19 Aug 2009 18:22:19 +0800,
Shine Liu wrote:
> 
> There's no restriction on that the runtime->period_size should be the
> multiplicator of the runtime->buffer_size in current kernel code.
> 
> So "hw_ptr_interrupt = runtime->hw_ptr_interrupt +
> runtime->period_size;" is not always the trueth.

The hw_ptr_interrupt is the expected position where the current irq is
issued.  So, the current code should be correct -- it's simply
increased in the size of period_size.

> I've encounter the problem of this bug, the kernel told me "PCM:
> Unexpected hw_pointer value (stream=0, pos=8, intr_ptr=16384)".
> 
> The period_size and buffer_size I used was 0x800 and 0x3e80 when the bug
> occured.
> 
> Following patch will fix the bug.

Does the problem happen with the latest driver code (as found in
sound git tree or alsa-driver-snapshot)?  If yes, we'd need more
further analysis, e.g. by enabling some xrun_debug flags.


thanks,

Takashi


> Signed-off-by: Shine Liu <shinel@foxmail.com>
> -----------------------------------------------------------------------
> 
> --- a/sound/core/pcm_lib.c	2009-08-14 06:43:34.000000000 +0800
> +++ b/sound/core/pcm_lib.c	2009-08-19 17:59:00.000000000 +0800
> @@ -248,6 +248,12 @@
>  	hw_base = runtime->hw_ptr_base;
>  	new_hw_ptr = hw_base + pos;
>  	hw_ptr_interrupt = runtime->hw_ptr_interrupt + runtime->period_size;
> +	/* if the period_size is not the multiplicator of the 
> + 	 * buffer_size, the hw_ptr_interrupt calculated above 
> + 	 * may not be correct, should be fixed. 
> + 	 */
> +	if(hw_ptr_interrupt > hw_base + runtime->buffer_size)
> +		hw_ptr_interrupt = hw_base + runtime->buffer_size;
>  	delta = new_hw_ptr - hw_ptr_interrupt;
>  	if (hw_ptr_interrupt >= runtime->boundary) {
>  		hw_ptr_interrupt -= runtime->boundary;
> 
> 
> 

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Shine Liu]

On Wed, 2009-08-19 at 12:34 +0200, Takashi Iwai wrote:
> At Wed, 19 Aug 2009 18:22:19 +0800,
> Shine Liu wrote:
> > 
> > There's no restriction on that the runtime->period_size should be the
> > multiplicator of the runtime->buffer_size in current kernel code.
> > 
> > So "hw_ptr_interrupt = runtime->hw_ptr_interrupt +
> > runtime->period_size;" is not always the trueth.
> 
> The hw_ptr_interrupt is the expected position where the current irq is
> issued.  So, the current code should be correct -- it's simply
> increased in the size of period_size.

Yes, the period_size and buffer_size I used was 0x800 and 0x3e80.

The log from the kernel is :

period_update: pcmC0D0p:0: pos=0x809/0x800/0x3e80, hwptr=0x1c, hw_base=0x0, hw_intr=0x0
period_update: pcmC0D0p:0: pos=0x1008/0x800/0x3e80, hwptr=0x809, hw_base=0x0, hw_intr=0x800
period_update: pcmC0D0p:0: pos=0x1808/0x800/0x3e80, hwptr=0x10e3, hw_base=0x0, hw_intr=0x1000
period_update: pcmC0D0p:0: pos=0x2006/0x800/0x3e80, hwptr=0x1808, hw_base=0x0, hw_intr=0x1800
period_update: pcmC0D0p:0: pos=0x2808/0x800/0x3e80, hwptr=0x203f, hw_base=0x0, hw_intr=0x2000
period_update: pcmC0D0p:0: pos=0x3008/0x800/0x3e80, hwptr=0x2808, hw_base=0x0, hw_intr=0x2800
period_update: pcmC0D0p:0: pos=0x3808/0x800/0x3e80, hwptr=0x30e9, hw_base=0x0, hw_intr=0x3000
period_update: pcmC0D0p:0: pos=0x8/0x800/0x3e80, hwptr=0x3808, hw_base=0x0, hw_intr=0x3800
PCM: Unexpected hw_pointer value (stream=0, pos=8, intr_ptr=16384)

It's true that irq was issued when hw_ptr_interrupt is
0x800,0x1000,0x1800,...,0x3800,0x3e80.

But the irq last issued should be at 0x3e80, but not 0x3800 + 0x800 =
0x4000. The DMA engine loaded 0x680 frame not 0x800 frame at the last
time.

In the snd_pcm_update_hw_ptr_interrupt function, hw_ptr_interrupt is
caculated simply by increased in the size of period_size, but the
runtime->period_size is not always consistent with the effective loaded
size since the last irq. For example, the DMA engine loaded 0x680 frame
when buffer reached the end at 0x3e80, and the irq issued.

> 
> > I've encounter the problem of this bug, the kernel told me "PCM:
> > Unexpected hw_pointer value (stream=0, pos=8, intr_ptr=16384)".
> > 
> > The period_size and buffer_size I used was 0x800 and 0x3e80 when the bug
> > occured.
> > 
> > Following patch will fix the bug.
> 
> Does the problem happen with the latest driver code (as found in
> sound git tree or alsa-driver-snapshot)?  If yes, we'd need more
> further analysis, e.g. by enabling some xrun_debug flags.
> 
> 
> thanks,
> 
> Takashi
> 
> 
> > Signed-off-by: Shine Liu <shinel@foxmail.com>
> > -----------------------------------------------------------------------
> > 
> > --- a/sound/core/pcm_lib.c	2009-08-14 06:43:34.000000000 +0800
> > +++ b/sound/core/pcm_lib.c	2009-08-19 17:59:00.000000000 +0800
> > @@ -248,6 +248,12 @@
> >  	hw_base = runtime->hw_ptr_base;
> >  	new_hw_ptr = hw_base + pos;
> >  	hw_ptr_interrupt = runtime->hw_ptr_interrupt + runtime->period_size;
> > +	/* if the period_size is not the multiplicator of the 
> > + 	 * buffer_size, the hw_ptr_interrupt calculated above 
> > + 	 * may not be correct, should be fixed. 
> > + 	 */
> > +	if(hw_ptr_interrupt > hw_base + runtime->buffer_size)
> > +		hw_ptr_interrupt = hw_base + runtime->buffer_size;
> >  	delta = new_hw_ptr - hw_ptr_interrupt;
> >  	if (hw_ptr_interrupt >= runtime->boundary) {
> >  		hw_ptr_interrupt -= runtime->boundary;
> > 
> > 
> > 
> 
> 
> 

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Takashi Iwai]

At Wed, 19 Aug 2009 18:59:31 +0800,
Shine Liu wrote:
> 
> On Wed, 2009-08-19 at 12:34 +0200, Takashi Iwai wrote:
> > At Wed, 19 Aug 2009 18:22:19 +0800,
> > Shine Liu wrote:
> > > 
> > > There's no restriction on that the runtime->period_size should be the
> > > multiplicator of the runtime->buffer_size in current kernel code.
> > > 
> > > So "hw_ptr_interrupt = runtime->hw_ptr_interrupt +
> > > runtime->period_size;" is not always the trueth.
> > 
> > The hw_ptr_interrupt is the expected position where the current irq is
> > issued.  So, the current code should be correct -- it's simply
> > increased in the size of period_size.
> 
> Yes, the period_size and buffer_size I used was 0x800 and 0x3e80.
> 
> The log from the kernel is :
> 
> period_update: pcmC0D0p:0: pos=0x809/0x800/0x3e80, hwptr=0x1c, hw_base=0x0, hw_intr=0x0
> period_update: pcmC0D0p:0: pos=0x1008/0x800/0x3e80, hwptr=0x809, hw_base=0x0, hw_intr=0x800
> period_update: pcmC0D0p:0: pos=0x1808/0x800/0x3e80, hwptr=0x10e3, hw_base=0x0, hw_intr=0x1000
> period_update: pcmC0D0p:0: pos=0x2006/0x800/0x3e80, hwptr=0x1808, hw_base=0x0, hw_intr=0x1800
> period_update: pcmC0D0p:0: pos=0x2808/0x800/0x3e80, hwptr=0x203f, hw_base=0x0, hw_intr=0x2000
> period_update: pcmC0D0p:0: pos=0x3008/0x800/0x3e80, hwptr=0x2808, hw_base=0x0, hw_intr=0x2800
> period_update: pcmC0D0p:0: pos=0x3808/0x800/0x3e80, hwptr=0x30e9, hw_base=0x0, hw_intr=0x3000
> period_update: pcmC0D0p:0: pos=0x8/0x800/0x3e80, hwptr=0x3808, hw_base=0x0, hw_intr=0x3800
> PCM: Unexpected hw_pointer value (stream=0, pos=8, intr_ptr=16384)
> 
> It's true that irq was issued when hw_ptr_interrupt is
> 0x800,0x1000,0x1800,...,0x3800,0x3e80.
> 
> But the irq last issued should be at 0x3e80, but not 0x3800 + 0x800 =
> 0x4000. The DMA engine loaded 0x680 frame not 0x800 frame at the last
> time.

Then it's a driver bug.  If unaligned period size is allowed, it means
that the irq is really generated in that period, not at the buffer
boundary.  Otherwise, it must have a proper hw-constraint to align the
period size to the buffer size.

What hardware is it?


thanks,

Takashi

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Shine Liu]

On Wed, 2009-08-19 at 14:05 +0200, Takashi Iwai wrote:
> > 
> > But the irq last issued should be at 0x3e80, but not 0x3800 + 0x800 =
> > 0x4000. The DMA engine loaded 0x680 frame not 0x800 frame at the last
> > time.
> 
> Then it's a driver bug.  If unaligned period size is allowed, it means
> that the irq is really generated in that period, not at the buffer
> boundary.  Otherwise, it must have a proper hw-constraint to align the
> period size to the buffer size.
> 
> What hardware is it?

Yes, but there's no constraint code currently to force to align the
period size to the buffer size. The bug occurs on linux-2.6.31-rc6 on
ASoC s3c24xx platform.

The constraint to force to align the period size to the buffer size
should not be hardware depended, it shoud be done in the generic layer,
is it?

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Takashi Iwai]

At Wed, 19 Aug 2009 12:48:26 +0000,
Shine Liu wrote:
> 
> On Wed, 2009-08-19 at 14:05 +0200, Takashi Iwai wrote:
> > > 
> > > But the irq last issued should be at 0x3e80, but not 0x3800 + 0x800 =
> > > 0x4000. The DMA engine loaded 0x680 frame not 0x800 frame at the last
> > > time.
> > 
> > Then it's a driver bug.  If unaligned period size is allowed, it means
> > that the irq is really generated in that period, not at the buffer
> > boundary.  Otherwise, it must have a proper hw-constraint to align the
> > period size to the buffer size.
> > 
> > What hardware is it?
> 
> Yes, but there's no constraint code currently to force to align the
> period size to the buffer size. The bug occurs on linux-2.6.31-rc6 on
> ASoC s3c24xx platform.
> 
> The constraint to force to align the period size to the buffer size
> should not be hardware depended, it shoud be done in the generic layer,
> is it?

It is hardware dependent, i.e. how the irq is generated.
If the irq is generated in a way like timer, the period size doesn't
have to be aligned with the buffer size.  But your case doesn't look
like that.


Takashi

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Shine Liu]

On Wed, 2009-08-19 at 14:53 +0200, Takashi Iwai wrote:
> 
> It is hardware dependent, i.e. how the irq is generated.
> If the irq is generated in a way like timer, the period size doesn't
> have to be aligned with the buffer size.  But your case doesn't look
> like that.
> 
> 
> Takashi
> 

Thanks, I'll find a way to fix this bug for the AsoC s3c24xx platform.

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Shine Liu]

On Wed, 2009-08-19 at 14:53 +0200, Takashi Iwai wrote:
> At Wed, 19 Aug 2009 12:48:26 +0000,
> Shine Liu wrote:
> > 
> > On Wed, 2009-08-19 at 14:05 +0200, Takashi Iwai wrote:
> > > > 
> > > > But the irq last issued should be at 0x3e80, but not 0x3800 + 0x800 =
> > > > 0x4000. The DMA engine loaded 0x680 frame not 0x800 frame at the last
> > > > time.
> > > 
> > > Then it's a driver bug.  If unaligned period size is allowed, it means
> > > that the irq is really generated in that period, not at the buffer
> > > boundary.  Otherwise, it must have a proper hw-constraint to align the
> > > period size to the buffer size.

Hi, Takashi

    I carefully read the pcm_lib.c and pcm_native.c but I didn't found
any alignmnet constraint API. Althought there's a function called
snd_pcm_hw_constraint_step, but this function can't be used in the
situation referred in the last mail which one hw parameter should be
aligned to the other hw parameter.

    I've implement a snd_pcm_hw_constraint_aligned function based on
snd_interval_step.

    The patch is based on linux-2.6.31-rc6, and I've tested.


Signed-off-by: Shine Liu <shinel@foxmail.com>
-------------------------------------------------------------------

--- a/include/sound/pcm.h	2009-08-14 06:43:34.000000000 +0800
+++ b/include/sound/pcm.h	2009-08-20 14:29:13.000000000 +0800
@@ -790,6 +790,10 @@
 			       unsigned int cond,
 			       snd_pcm_hw_param_t var,
 			       unsigned long step);
+int snd_pcm_hw_constraint_aligned(struct snd_pcm_runtime *runtime,
+			       unsigned int cond,
+			       snd_pcm_hw_param_t var,
+			       snd_pcm_hw_param_t unit);
 int snd_pcm_hw_constraint_pow2(struct snd_pcm_runtime *runtime,
 			       unsigned int cond,
 			       snd_pcm_hw_param_t var);
--- a/sound/core/pcm_lib.c	2009-08-19 13:12:47.000000000 +0800
+++ b/sound/core/pcm_lib.c	2009-08-20 14:25:53.000000000 +0800
@@ -1319,6 +1319,33 @@
 
 EXPORT_SYMBOL(snd_pcm_hw_constraint_step);
 
+static int snd_pcm_hw_rule_aligned(struct snd_pcm_hw_params *params,
+				struct snd_pcm_hw_rule *rule)
+{
+	struct snd_interval * i = hw_param_interval(params, rule->deps[0]);
+	unsigned int unit = snd_interval_value(i);
+	return snd_interval_step(hw_param_interval(params, rule->var), 0, unit);
+}
+
+/**
+ * snd_pcm_hw_constraint_aligned - add a hw constraint alignment rule
+ * @runtime: PCM runtime instance
+ * @cond: condition bits
+ * @var: hw_params variable to apply the alignment constraint
+ * @unit: hw_params variable to provid the alignment unit
+ */
+int snd_pcm_hw_constraint_aligned(struct snd_pcm_runtime *runtime,
+			       unsigned int cond,
+			       snd_pcm_hw_param_t var,
+			       snd_pcm_hw_param_t unit)
+{
+	return snd_pcm_hw_rule_add(runtime, cond, var, 
+				   snd_pcm_hw_rule_aligned, (void *) 0,
+				   unit, -1);
+}
+
+EXPORT_SYMBOL(snd_pcm_hw_constraint_aligned);
+
 static int snd_pcm_hw_rule_pow2(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule)
 {
 	static unsigned int pow2_sizes[] = {


> > > 
> > > What hardware is it?
> > 
> > Yes, but there's no constraint code currently to force to align the
> > period size to the buffer size. The bug occurs on linux-2.6.31-rc6 on
> > ASoC s3c24xx platform.
> > 
> > The constraint to force to align the period size to the buffer size
> > should not be hardware depended, it shoud be done in the generic layer,
> > is it?
> 
> It is hardware dependent, i.e. how the irq is generated.
> If the irq is generated in a way like timer, the period size doesn't
> have to be aligned with the buffer size.  But your case doesn't look
> like that.
> 
> 
> Takashi
> 
> 
> 

Re: [PATCH] pcm_lib.c: Fixed inaccurate calculation of hw_ptr_interrupt in snd_pcm_update_hw_ptr_interrupt function

[Clemens Ladisch]

Shine Liu wrote:
>     I carefully read the pcm_lib.c and pcm_native.c but I didn't found
> any alignmnet constraint API. Althought there's a function called
> snd_pcm_hw_constraint_step, but this function can't be used in the
> situation referred in the last mail which one hw parameter should be
> aligned to the other hw parameter.

The period size, the buffer size, and the number of periods are all
related, so you can align the period size to the buffer size by
restricting the number of periods to be an integer:

  snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);


HTH
Clemens

[PATCH] ASoC: S3C24XX : Align the peroid size to the buffer size

[Shine Liu]

On Wed, 2009-08-19 at 14:05 +0200, Takashi Iwai wrote:
> At Wed, 19 Aug 2009 18:59:31 +0800,
> Shine Liu wrote:
> > 
> > On Wed, 2009-08-19 at 12:34 +0200, Takashi Iwai wrote:
> > > At Wed, 19 Aug 2009 18:22:19 +0800,
> > > Shine Liu wrote:
> > > > 
> > > > There's no restriction on that the runtime->period_size should be the
> > > > multiplicator of the runtime->buffer_size in current kernel code.
> > > > 
> > > > So "hw_ptr_interrupt = runtime->hw_ptr_interrupt +
> > > > runtime->period_size;" is not always the trueth.
> > > 
> > > The hw_ptr_interrupt is the expected position where the current irq is
> > > issued.  So, the current code should be correct -- it's simply
> > > increased in the size of period_size.
> > 
> > Yes, the period_size and buffer_size I used was 0x800 and 0x3e80.
> > 
> > The log from the kernel is :
> > 
> > period_update: pcmC0D0p:0: pos=0x809/0x800/0x3e80, hwptr=0x1c, hw_base=0x0, hw_intr=0x0
> > period_update: pcmC0D0p:0: pos=0x1008/0x800/0x3e80, hwptr=0x809, hw_base=0x0, hw_intr=0x800
> > period_update: pcmC0D0p:0: pos=0x1808/0x800/0x3e80, hwptr=0x10e3, hw_base=0x0, hw_intr=0x1000
> > period_update: pcmC0D0p:0: pos=0x2006/0x800/0x3e80, hwptr=0x1808, hw_base=0x0, hw_intr=0x1800
> > period_update: pcmC0D0p:0: pos=0x2808/0x800/0x3e80, hwptr=0x203f, hw_base=0x0, hw_intr=0x2000
> > period_update: pcmC0D0p:0: pos=0x3008/0x800/0x3e80, hwptr=0x2808, hw_base=0x0, hw_intr=0x2800
> > period_update: pcmC0D0p:0: pos=0x3808/0x800/0x3e80, hwptr=0x30e9, hw_base=0x0, hw_intr=0x3000
> > period_update: pcmC0D0p:0: pos=0x8/0x800/0x3e80, hwptr=0x3808, hw_base=0x0, hw_intr=0x3800
> > PCM: Unexpected hw_pointer value (stream=0, pos=8, intr_ptr=16384)
> > 
> > It's true that irq was issued when hw_ptr_interrupt is
> > 0x800,0x1000,0x1800,...,0x3800,0x3e80.
> > 
> > But the irq last issued should be at 0x3e80, but not 0x3800 + 0x800 =
> > 0x4000. The DMA engine loaded 0x680 frame not 0x800 frame at the last
> > time.
> 
> Then it's a driver bug.  If unaligned period size is allowed, it means
> that the irq is really generated in that period, not at the buffer
> boundary.  Otherwise, it must have a proper hw-constraint to align the
> period size to the buffer size.
> 
> What hardware is it?
> 
> 
> thanks,
> 
> Takashi
> 

This patch will fix the bug metioned in the above mail. Force the peroid
size to be aligned with the buffer size.

Based and tested on linux-2.6.31-rc6.

Signed-off-by: Shine Liu <shinel@foxmail.com>
---------------------------------------------------------------------------

--- a/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-14 06:43:34.000000000 +0800
+++ b/sound/soc/s3c24xx/s3c24xx-pcm.c	2009-08-20 22:33:58.000000000 +0800
@@ -318,6 +318,7 @@
 
 	pr_debug("Entered %s\n", __func__);
 
+	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
 	snd_soc_set_runtime_hwparams(substream, &s3c24xx_pcm_hardware);
 
 	prtd = kzalloc(sizeof(struct s3c24xx_runtime_data), GFP_KERNEL);

Re: [PATCH] ASoC: S3C24XX : Align the peroid size to the buffer size

[Mark Brown]

On Thu, Aug 20, 2009 at 11:02:23PM +0800, Shine Liu wrote:

> This patch will fix the bug metioned in the above mail. Force the peroid
> size to be aligned with the buffer size.

Applied (with a slightly updated commit message) - thanks!