Re: sound skipping regression introduced in 2.6.30-rc8

[Takashi Iwai]

At Mon, 15 Jun 2009 02:25:47 -0700 (PDT),
David Miller wrote:
> 
> From: Takashi Iwai <tiwai@xxxxxxx>
> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/