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@xxxxxxx>
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@xxxxxxx>
---
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
