Patchworkβ sound skipping regression introduced in 2.6.30-rc8

login
register
about
Submitter Takashi Iwai
Date 2009-06-15 09:58:13
Message ID <s5htz2hes3u.wl%tiwai@suse.de>
Download mbox | patch
Permalink /patch/30291/
State New
Headers show

Comments

Takashi Iwai - 2009-06-15 09:58:13
At Mon, 15 Jun 2009 11:48:43 +0200,
I wrote:
> 
> At Mon, 15 Jun 2009 02:25:47 -0700 (PDT),
> David Miller wrote:
> > 
> > From: Takashi Iwai <tiwai@suse.de>
> > 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.

Damn, that was buggy again.  The fixed version is below.


Takashi

---
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
David Miller - 2009-06-15 10:11:18
From: Takashi Iwai <tiwai@suse.de>
Date: Mon, 15 Jun 2009 11:58:13 +0200

> Damn, that was buggy again.  The fixed version is below.

This seems to fix the bug and work fine.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Takashi Iwai - 2009-06-15 10:26:50
At Mon, 15 Jun 2009 03:11:18 -0700 (PDT),
David Miller wrote:
> 
> From: Takashi Iwai <tiwai@suse.de>
> Date: Mon, 15 Jun 2009 11:58:13 +0200
> 
> > Damn, that was buggy again.  The fixed version is below.
> 
> This seems to fix the bug and work fine.

Thanks for quick testing!
I'll put this to the next 2.6.31 pull request and mark it for 2.6.30
stable kernel.

Bartlomiej, Sven, if you have still a problem with this patch, please
let me know.


Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Sven Köhler - 2009-06-15 18:25:58
Takashi Iwai schrieb:
> Damn, that was buggy again.  The fixed version is below.

Seems to work fine.
I have been listening music fir quite a few minutes - skip free.


Thanks!

  Sven
Bartlomiej Zolnierkiewicz - 2009-06-15 19:15:36
On Monday 15 June 2009 20:25:58 Sven Köhler wrote:
> Takashi Iwai schrieb:
> > Damn, that was buggy again.  The fixed version is below.
> 
> Seems to work fine.
> I have been listening music fir quite a few minutes - skip free.

Same here. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 173bebf..8aa5687 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,23 @@  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) {
+			unsigned int pos_base, last_base;
+			pos_base = position / ichdev->fragsize1;
+			last_base = ichdev->last_pos / ichdev->fragsize1;
+			/* another sanity check; ptr1 can go back to full
+			 * before the base position is updated
+			 */
+			if (pos_base == last_base)
+				ptr = ichdev->last_pos;
+		}
 	}
+	ichdev->last_pos = ptr;
 	spin_unlock(&chip->reg_lock);
 	if (ptr >= ichdev->size)
 		return 0;