From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758358AbZFOJsu (ORCPT ); Mon, 15 Jun 2009 05:48:50 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753298AbZFOJsn (ORCPT ); Mon, 15 Jun 2009 05:48:43 -0400 Received: from cantor2.suse.de ([195.135.220.15]:60030 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752941AbZFOJsm (ORCPT ); Mon, 15 Jun 2009 05:48:42 -0400 Date: Mon, 15 Jun 2009 11:48:43 +0200 Message-ID: From: Takashi Iwai To: David Miller Cc: bzolnier@gmail.com, sven.koehler@gmail.com, linux-kernel@vger.kernel.org Subject: Re: sound skipping regression introduced in 2.6.30-rc8 In-Reply-To: <20090615.022547.107684868.davem@davemloft.net> References: <200906121344.26139.bzolnier@gmail.com> <20090615.022547.107684868.davem@davemloft.net> User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka) FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 Emacs/22.3 (x86_64-suse-linux-gnu) MULE/5.0 (SAKAKI) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Mon, 15 Jun 2009 02:25:47 -0700 (PDT), David Miller wrote: > > From: Takashi Iwai > Date: Mon, 15 Jun 2009 10:39:50 +0200 > > > If that patch also doesn't work, we need to track the position update > > sequence in more details. > > The patch below will dump the each position update (CAUTION: it can be > > long logs!). Please apply it instead of the previous one, run with > > xrun_debug=5 on 2.6.31 (or xrun_debug=1 on 2.6.30), and give the > > outputs around a skip occurs. > > The patch doesn't work. > > I put up a debugging log at: > > http://vger.kernel.org/~davem/asound.log > > there is a "PCM: hw_ptr skipping! ..." message near the end. Thanks! I see the problem now. It's a race in the update of the current buffer position. The counter was reset to the full fragment size, but its index still points the previous position. So, the driver was confused and put the position back. The below is a revised patch. It has an additional sanity check. Please give it a try. Takashi --- diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c index 173bebf..474e40a 100644 --- a/sound/pci/intel8x0.c +++ b/sound/pci/intel8x0.c @@ -356,8 +356,6 @@ struct ichdev { unsigned int position; unsigned int pos_shift; unsigned int last_pos; - unsigned long last_pos_jiffies; - unsigned int jiffy_to_bytes; int frags; int lvi; int lvi_frag; @@ -844,7 +842,6 @@ static int snd_intel8x0_pcm_trigger(struct snd_pcm_substream *substream, int cmd case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: val = ICH_IOCE | ICH_STARTBM; ichdev->last_pos = ichdev->position; - ichdev->last_pos_jiffies = jiffies; break; case SNDRV_PCM_TRIGGER_SUSPEND: ichdev->suspended = 1; @@ -1048,7 +1045,6 @@ static int snd_intel8x0_pcm_prepare(struct snd_pcm_substream *substream) ichdev->pos_shift = (runtime->sample_bits > 16) ? 2 : 1; } snd_intel8x0_setup_periods(chip, ichdev); - ichdev->jiffy_to_bytes = (runtime->rate * 4 * ichdev->pos_shift) / HZ; return 0; } @@ -1073,19 +1069,19 @@ static snd_pcm_uframes_t snd_intel8x0_pcm_pointer(struct snd_pcm_substream *subs ptr1 == igetword(chip, ichdev->reg_offset + ichdev->roff_picb)) break; } while (timeout--); + ptr = ichdev->last_pos; if (ptr1 != 0) { ptr1 <<= ichdev->pos_shift; ptr = ichdev->fragsize1 - ptr1; ptr += position; - ichdev->last_pos = ptr; - ichdev->last_pos_jiffies = jiffies; - } else { - ptr1 = jiffies - ichdev->last_pos_jiffies; - if (ptr1) - ptr1 -= 1; - ptr = ichdev->last_pos + ptr1 * ichdev->jiffy_to_bytes; - ptr %= ichdev->size; + if (ptr < ichdev->last_pos) { + /* invalid position; likely ptr1 went back to + * fragsize1 while the base position wasn't updated yet + */ + ptr = ichdev->last_pos; + } } + ichdev->last_pos = ptr; spin_unlock(&chip->reg_lock); if (ptr >= ichdev->size) return 0;