On Wed, May 27, 2009 at 6:41 AM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> 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
>
>
>
Takashi:
I will test this patch. Also, I have some results from the test with
arecord that Jaroslav wanted.
Best Regards,
--
Rob Krakora
Senior Software Engineer
MessageNet Systems
101 East Carmel Dr. Suite 105
Carmel, IN 46032
(317)566-1677 Ext. 206
(317)663-0808 Fax
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
