From ed3da3d9a0ef13c6fe1414ec73c9c1be12747b62 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 3 Mar 2009 17:00:15 +0100 Subject: [PATCH 1/7] ALSA: Rewrite hw_ptr updaters Clean up and improve snd_pcm_update_hw_ptr*() functions. snd_pcm_update_hw_ptr() tries to detect the unexpected hwptr jumps more strictly to avoid the position mess-up, which often results in the bad quality I/O with pulseaudio. The hw-ptr skip error messages are printed when xrun proc is set to non-zero. Signed-off-by: Takashi Iwai --- sound/core/pcm_lib.c | 126 ++++++++++++++++++++++++++++--------------- 1 file changed, 82 insertions(+), 44 deletions(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 921691080f35..86ac9ae9460e 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -125,19 +125,27 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram } } +#ifdef CONFIG_SND_PCM_XRUN_DEBUG +#define xrun_debug(substream) ((substream)->pstr->xrun_debug) +#else +#define xrun_debug(substream) 0 +#endif + +#define dump_stack_on_xrun(substream) do { \ + if (xrun_debug(substream) > 1) \ + dump_stack(); \ + } while (0) + static void xrun(struct snd_pcm_substream *substream) { snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); -#ifdef CONFIG_SND_PCM_XRUN_DEBUG - if (substream->pstr->xrun_debug) { + if (xrun_debug(substream)) { snd_printd(KERN_DEBUG "XRUN: pcmC%dD%d%c\n", substream->pcm->card->number, substream->pcm->device, substream->stream ? 'c' : 'p'); - if (substream->pstr->xrun_debug > 1) - dump_stack(); + dump_stack_on_xrun(substream); } -#endif } static inline snd_pcm_uframes_t snd_pcm_update_hw_ptr_pos(struct snd_pcm_substream *substream, @@ -182,11 +190,21 @@ static inline int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream return 0; } +#define hw_ptr_error(substream, fmt, args...) \ + do { \ + if (xrun_debug(substream)) { \ + if (printk_ratelimit()) { \ + snd_printd("hda_codec: " fmt, ##args); \ + } \ + dump_stack_on_xrun(substream); \ + } \ + } while (0) + static inline int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_uframes_t pos; - snd_pcm_uframes_t new_hw_ptr, hw_ptr_interrupt; + snd_pcm_uframes_t new_hw_ptr, hw_ptr_interrupt, hw_base; snd_pcm_sframes_t delta; pos = snd_pcm_update_hw_ptr_pos(substream, runtime); @@ -194,36 +212,47 @@ static inline int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *subs xrun(substream); return -EPIPE; } - if (runtime->period_size == runtime->buffer_size) - goto __next_buf; - new_hw_ptr = runtime->hw_ptr_base + pos; + hw_base = runtime->hw_ptr_base; + new_hw_ptr = hw_base + pos; hw_ptr_interrupt = runtime->hw_ptr_interrupt + runtime->period_size; - - delta = hw_ptr_interrupt - new_hw_ptr; - if (delta > 0) { - if ((snd_pcm_uframes_t)delta < runtime->buffer_size / 2) { -#ifdef CONFIG_SND_PCM_XRUN_DEBUG - if (runtime->periods > 1 && substream->pstr->xrun_debug) { - snd_printd(KERN_ERR "Unexpected hw_pointer value [1] (stream = %i, delta: -%ld, max jitter = %ld): wrong interrupt acknowledge?\n", substream->stream, (long) delta, runtime->buffer_size / 2); - if (substream->pstr->xrun_debug > 1) - dump_stack(); - } -#endif - return 0; + delta = new_hw_ptr - hw_ptr_interrupt; + if (hw_ptr_interrupt == runtime->boundary) + hw_ptr_interrupt = 0; + if (delta < 0) { + delta += runtime->buffer_size; + if (delta < 0) { + hw_ptr_error(substream, + "Unexpected hw_pointer value " + "(stream=%i, pos=%ld, intr_ptr=%ld)\n", + substream->stream, (long)pos, + (long)hw_ptr_interrupt); + /* rebase to interrupt position */ + hw_base = new_hw_ptr = hw_ptr_interrupt; + delta = 0; + } else { + hw_base += runtime->buffer_size; + if (hw_base == runtime->boundary) + hw_base = 0; + new_hw_ptr = hw_base + pos; } - __next_buf: - runtime->hw_ptr_base += runtime->buffer_size; - if (runtime->hw_ptr_base == runtime->boundary) - runtime->hw_ptr_base = 0; - new_hw_ptr = runtime->hw_ptr_base + pos; } - + if (delta > runtime->period_size) { + hw_ptr_error(substream, + "Lost interrupts? " + "(stream=%i, delta=%ld, intr_ptr=%ld)\n", + substream->stream, (long)delta, + (long)hw_ptr_interrupt); + /* rebase hw_ptr_interrupt */ + hw_ptr_interrupt = + new_hw_ptr - new_hw_ptr % runtime->period_size; + } if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) snd_pcm_playback_silence(substream, new_hw_ptr); + runtime->hw_ptr_base = hw_base; runtime->status->hw_ptr = new_hw_ptr; - runtime->hw_ptr_interrupt = new_hw_ptr - new_hw_ptr % runtime->period_size; + runtime->hw_ptr_interrupt = hw_ptr_interrupt; return snd_pcm_update_hw_ptr_post(substream, runtime); } @@ -233,7 +262,7 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_uframes_t pos; - snd_pcm_uframes_t old_hw_ptr, new_hw_ptr; + snd_pcm_uframes_t old_hw_ptr, new_hw_ptr, hw_base; snd_pcm_sframes_t delta; old_hw_ptr = runtime->status->hw_ptr; @@ -242,29 +271,38 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream) xrun(substream); return -EPIPE; } - new_hw_ptr = runtime->hw_ptr_base + pos; + hw_base = runtime->hw_ptr_base; + new_hw_ptr = hw_base + pos; - delta = old_hw_ptr - new_hw_ptr; - if (delta > 0) { - if ((snd_pcm_uframes_t)delta < runtime->buffer_size / 2) { -#ifdef CONFIG_SND_PCM_XRUN_DEBUG - if (runtime->periods > 2 && substream->pstr->xrun_debug) { - snd_printd(KERN_ERR "Unexpected hw_pointer value [2] (stream = %i, delta: -%ld, max jitter = %ld): wrong interrupt acknowledge?\n", substream->stream, (long) delta, runtime->buffer_size / 2); - if (substream->pstr->xrun_debug > 1) - dump_stack(); - } -#endif + delta = new_hw_ptr - old_hw_ptr; + if (delta < 0) { + delta += runtime->buffer_size; + if (delta < 0) { + hw_ptr_error(substream, + "Unexpected hw_pointer value [2] " + "(stream=%i, pos=%ld, old_ptr=%ld)\n", + substream->stream, (long)pos, + (long)old_hw_ptr); return 0; } - runtime->hw_ptr_base += runtime->buffer_size; - if (runtime->hw_ptr_base == runtime->boundary) - runtime->hw_ptr_base = 0; - new_hw_ptr = runtime->hw_ptr_base + pos; + hw_base += runtime->buffer_size; + if (hw_base == runtime->boundary) + hw_base = 0; + new_hw_ptr = hw_base + pos; + } + if (delta > runtime->period_size && runtime->periods > 1) { + hw_ptr_error(substream, + "hw_ptr skipping! " + "(pos=%ld, delta=%ld, period=%ld)\n", + (long)pos, (long)delta, + (long)runtime->period_size); + return 0; } if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) snd_pcm_playback_silence(substream, new_hw_ptr); + runtime->hw_ptr_base = hw_base; runtime->status->hw_ptr = new_hw_ptr; return snd_pcm_update_hw_ptr_post(substream, runtime); From 85122ea40c4fc82af5b66b8683f525c2c4a36d1a Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Fri, 6 Mar 2009 16:30:07 +0100 Subject: [PATCH 2/7] ALSA: Remove unneeded snd_pcm_substream.timer_lock The timer callbacks are called in the protected status by the lock of the timer instance, so there is no need for an extra lock in the PCM substream. Signed-off-by: Takashi Iwai --- include/sound/pcm.h | 1 - sound/core/pcm.c | 1 - sound/core/pcm_timer.c | 6 ------ 3 files changed, 8 deletions(-) diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 40c5a6fa6bcd..e4f60076e6c4 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -364,7 +364,6 @@ struct snd_pcm_substream { /* -- timer section -- */ struct snd_timer *timer; /* timer */ unsigned timer_running: 1; /* time is running */ - spinlock_t timer_lock; /* -- next substream -- */ struct snd_pcm_substream *next; /* -- linked substreams -- */ diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 192a433a2403..37f567a68ef2 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -667,7 +667,6 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) spin_lock_init(&substream->self_group.lock); INIT_LIST_HEAD(&substream->self_group.substreams); list_add_tail(&substream->link_list, &substream->self_group.substreams); - spin_lock_init(&substream->timer_lock); atomic_set(&substream->mmap_count, 0); prev = substream; } diff --git a/sound/core/pcm_timer.c b/sound/core/pcm_timer.c index 2c89c04f2916..ca8068b63d6c 100644 --- a/sound/core/pcm_timer.c +++ b/sound/core/pcm_timer.c @@ -85,25 +85,19 @@ static unsigned long snd_pcm_timer_resolution(struct snd_timer * timer) static int snd_pcm_timer_start(struct snd_timer * timer) { - unsigned long flags; struct snd_pcm_substream *substream; substream = snd_timer_chip(timer); - spin_lock_irqsave(&substream->timer_lock, flags); substream->timer_running = 1; - spin_unlock_irqrestore(&substream->timer_lock, flags); return 0; } static int snd_pcm_timer_stop(struct snd_timer * timer) { - unsigned long flags; struct snd_pcm_substream *substream; substream = snd_timer_chip(timer); - spin_lock_irqsave(&substream->timer_lock, flags); substream->timer_running = 0; - spin_unlock_irqrestore(&substream->timer_lock, flags); return 0; } From cad377acf3d6af6279622048e96680e79e352183 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 19 Mar 2009 09:55:15 +0100 Subject: [PATCH 3/7] ALSA: pcm - Fix a typo in error messages Fix a typo in error messages; forgotten after a copy&paste error. Signed-off-by: Takashi Iwai --- sound/core/pcm_lib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 86ac9ae9460e..2ff25ed4d834 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -194,7 +194,7 @@ static inline int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream do { \ if (xrun_debug(substream)) { \ if (printk_ratelimit()) { \ - snd_printd("hda_codec: " fmt, ##args); \ + snd_printd("PCM: " fmt, ##args); \ } \ dump_stack_on_xrun(substream); \ } \ From 98204646f2b15d368701265e4194b773a6f94600 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 19 Mar 2009 09:59:21 +0100 Subject: [PATCH 4/7] ALSA: pcm - avoid unnecessary inline Remove unnecessary explicit inlininig of internal functions. Let compiler optimize. Signed-off-by: Takashi Iwai --- sound/core/pcm_lib.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 2ff25ed4d834..302654769faf 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -148,8 +148,9 @@ static void xrun(struct snd_pcm_substream *substream) } } -static inline snd_pcm_uframes_t snd_pcm_update_hw_ptr_pos(struct snd_pcm_substream *substream, - struct snd_pcm_runtime *runtime) +static snd_pcm_uframes_t +snd_pcm_update_hw_ptr_pos(struct snd_pcm_substream *substream, + struct snd_pcm_runtime *runtime) { snd_pcm_uframes_t pos; @@ -167,8 +168,8 @@ static inline snd_pcm_uframes_t snd_pcm_update_hw_ptr_pos(struct snd_pcm_substre return pos; } -static inline int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, - struct snd_pcm_runtime *runtime) +static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, + struct snd_pcm_runtime *runtime) { snd_pcm_uframes_t avail; @@ -200,7 +201,7 @@ static inline int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream } \ } while (0) -static inline int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) +static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_uframes_t pos; From 5f513e1197f27e9a0bcfec0feaac59f976f4a37e Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 19 Mar 2009 10:01:47 +0100 Subject: [PATCH 5/7] ALSA: pcm - Reset invalid position even without debug option Always reset the invalind hw_ptr position returned by the pointer callback. The behavior should be consitent independently from the debug option. Also, add the printk_ratelimit() check to avoid flooding debug prints. Signed-off-by: Takashi Iwai --- sound/core/pcm_lib.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 302654769faf..92ed6d819225 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -159,11 +159,15 @@ snd_pcm_update_hw_ptr_pos(struct snd_pcm_substream *substream, pos = substream->ops->pointer(substream); if (pos == SNDRV_PCM_POS_XRUN) return pos; /* XRUN */ -#ifdef CONFIG_SND_DEBUG if (pos >= runtime->buffer_size) { - snd_printk(KERN_ERR "BUG: stream = %i, pos = 0x%lx, buffer size = 0x%lx, period size = 0x%lx\n", substream->stream, pos, runtime->buffer_size, runtime->period_size); + if (printk_ratelimit()) { + snd_printd(KERN_ERR "BUG: stream = %i, pos = 0x%lx, " + "buffer size = 0x%lx, period size = 0x%lx\n", + substream->stream, pos, runtime->buffer_size, + runtime->period_size); + } + pos = 0; } -#endif pos -= pos % runtime->min_align; return pos; } From ded652f7024bc2d7b6118b561a44187af30841b0 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 19 Mar 2009 10:08:49 +0100 Subject: [PATCH 6/7] ALSA: pcm - Fix delta calculation at boundary overlap When the hw_ptr_interrupt reaches the boundary, it must check whether the hw_base was already lapped and corret the delta value appropriately. Also, rebasing the hw_ptr needs a correction because buffer_size isn't always aligned to period_size. Signed-off-by: Takashi Iwai --- sound/core/pcm_lib.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 92ed6d819225..063c675177a9 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -221,8 +221,11 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) new_hw_ptr = hw_base + pos; hw_ptr_interrupt = runtime->hw_ptr_interrupt + runtime->period_size; delta = new_hw_ptr - hw_ptr_interrupt; - if (hw_ptr_interrupt == runtime->boundary) - hw_ptr_interrupt = 0; + if (hw_ptr_interrupt >= runtime->boundary) { + hw_ptr_interrupt %= runtime->boundary; + if (!hw_base) /* hw_base was already lapped; recalc delta */ + delta = new_hw_ptr - hw_ptr_interrupt; + } if (delta < 0) { delta += runtime->buffer_size; if (delta < 0) { @@ -233,6 +236,8 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) (long)hw_ptr_interrupt); /* rebase to interrupt position */ hw_base = new_hw_ptr = hw_ptr_interrupt; + /* align hw_base to buffer_size */ + hw_base -= hw_base % runtime->buffer_size; delta = 0; } else { hw_base += runtime->buffer_size; From 8b22d943c34b616eefbd6d2f8f197a53b1f29fd0 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Fri, 20 Mar 2009 16:26:15 +0100 Subject: [PATCH 7/7] ALSA: pcm - Safer boundary checks Make the boundary checks a bit safer. These caese are rare or theoretically won't happen, but nothing bad to keep the checks safer... Signed-off-by: Takashi Iwai --- sound/core/pcm_lib.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 063c675177a9..fbb2e391591e 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -222,8 +222,9 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) hw_ptr_interrupt = runtime->hw_ptr_interrupt + runtime->period_size; delta = new_hw_ptr - hw_ptr_interrupt; if (hw_ptr_interrupt >= runtime->boundary) { - hw_ptr_interrupt %= runtime->boundary; - if (!hw_base) /* hw_base was already lapped; recalc delta */ + hw_ptr_interrupt -= runtime->boundary; + if (hw_base < runtime->boundary / 2) + /* hw_base was already lapped; recalc delta */ delta = new_hw_ptr - hw_ptr_interrupt; } if (delta < 0) { @@ -241,7 +242,7 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) delta = 0; } else { hw_base += runtime->buffer_size; - if (hw_base == runtime->boundary) + if (hw_base >= runtime->boundary) hw_base = 0; new_hw_ptr = hw_base + pos; } @@ -296,7 +297,7 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream) return 0; } hw_base += runtime->buffer_size; - if (hw_base == runtime->boundary) + if (hw_base >= runtime->boundary) hw_base = 0; new_hw_ptr = hw_base + pos; }