[alsa-devel] 2.6.30: xruns caused by "ALSA: pcm - Safer boundary checks"

[Takashi Iwai]

At Wed, 27 May 2009 09:25:57 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Wed, 27 May 2009, Takashi Iwai wrote:
> 
> > Thanks for testing!
> >
> > I have an impression that the jiffies check can behave pretty buggy in
> > cases the hardware doesn't give a smooth pointer update.  It's
> > supposed to filter out spontaneous invalid pointer values.  That'll
> > work indeed (although I don't think the check in
> > snd_pcm_period_elapsed() is worth).  But what if a hardware returns
> > the pointer value that is not always updated smoothly?
> >
> > Assume a hardware returning only spontaneously updated values, 0 at
> > t=0, 1000 at t=100, 2000 at t=200, and so on.  If
> > snd_pcm_update_hw_ptr() is called at t=99, it gets still position 0.
> > Then when it's called at t=100, it suddenly increases to position 1000
> > while jiffies delta is only 1.  The current code would filter out this
> > update because it appears as if too much.
> 
> That's good point. The hw_ptr_jiffies should be updated only when position 
> in the ring buffer moves:
> 
>  	if (runtime->status->hw_ptr != new_hw_ptr)
>  		runtime->hw_ptr_jiffies = jiffies;
> 
> Similar condition might be also used for runtime->status->tstamp variable.

Yep, that'll be needed.


> > The problem continues, if snd_pcm_update_hw_ptr() is constantly called
> > at each t=100, 101, ...  At each call, jiffies delta is 1, but the
> > position delta is still 1000 because it wasn't updated.  That is, as
> > long as the app keeps calling snd_pcm_update_hw_ptr(), the position
> > won't be updated.  This is pretty possible especially with an app like
> > pulseaudio.
> >
> >
> > I have a feeling that we should disable the jiffies check in that
> > part, at least for 2.6.30, until a better solution is found.  The
> > jiffies check is already done and works pretty well in the driver
> > side (hda-intel and intel8x0).  But this check in the PCM core can be
> > (not always, though) buggy, and actually gives problems sometimes.
> 
> As I already wrote, I have nothing against to make jiffies check 
> conditional on xrun_debug() for 2.6.30 for debugging purposes, but I'd 
> like to leave this code enabled in our development trees. It reveals some 
> buggy drivers which should be checked (a new one is Vortex - au88x0).

OK, I created a patch below and will included in the next pull
request today or tomorrow.


thanks,

Takashi

===
From c87d9732004b3f8fd82d729f12ccfb96c0df279e Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai at suse.de>
Date: Wed, 27 May 2009 10:53:33 +0200
Subject: [PATCH] ALSA: Enable PCM hw_ptr_jiffies check only in xrun_debug mode

The PCM hw_ptr jiffies check results sometimes in problems when a
hardware doesn't give smooth hw_ptr updates.  So far, au88x0 and some
other drivers appear not working due to this strict check.
However, this check is a nice debug tool, and the capability should be
still kept.

Hence, we disable this check now as default unless the user enables it
by setting the xrun_debug mode to the specific stream via a proc file.

Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
 Documentation/sound/alsa/Procfile.txt |    5 +++++
 sound/core/pcm_lib.c                  |    9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/Documentation/sound/alsa/Procfile.txt b/Documentation/sound/alsa/Procfile.txt
index bba2dbb..cfac20c 100644
--- a/Documentation/sound/alsa/Procfile.txt
+++ b/Documentation/sound/alsa/Procfile.txt
@@ -104,6 +104,11 @@ card*/pcm*/xrun_debug
 	When this value is greater than 1, the driver will show the
 	stack trace additionally.  This may help the debugging.
 
+	Since 2.6.30, this option also enables the hwptr check using
+	jiffies.  This detects spontaneous invalid pointer callback
+	values, but can be lead to too much corrections for a (mostly
+	buggy) hardware that doesn't give smooth pointer updates.
+
 card*/pcm*/sub*/info
 	The general information of this PCM sub-stream.
 
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 3eea98a..d659995 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -249,6 +249,11 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream)
 			new_hw_ptr = hw_base + pos;
 		}
 	}
+
+	/* Do jiffies check only in xrun_debug mode */
+	if (!xrun_debug(substream))
+		goto no_jiffies_check;
+
 	/* Skip the jiffies check for hardwares with BATCH flag.
 	 * Such hardware usually just increases the position at each IRQ,
 	 * thus it can't give any strange position.
@@ -336,7 +341,9 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream)
 			hw_base = 0;
 		new_hw_ptr = hw_base + pos;
 	}
-	if (((delta * HZ) / runtime->rate) > jdelta + HZ/100) {
+	/* Do jiffies check only in xrun_debug mode */
+	if (xrun_debug(substream) &&
+	    ((delta * HZ) / runtime->rate) > jdelta + HZ/100) {
 		hw_ptr_error(substream,
 			     "hw_ptr skipping! "
 			     "(pos=%ld, delta=%ld, period=%ld, jdelta=%lu/%lu)\n",
-- 
1.6.3