Buffering issues 2

Buffering issues 2

[Lennart Poettering]

Heya!

In addition to the buffering issues pointed out on http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007354.html
I now have some more issues with the way buffering works in ALSA.

This time I am pretty sure it's a bug in the HDA drivers.

PulseAudio basically does this (again, as part of the glitch-free playback
model, see http://0pointer.de/blog/projects/pulse-glitch-free.html):

for (;;) {
    for (;;) {
        snd_pcm_hwsync();
        n = snd_pcm_avail_update();
        if (!n) 
            break;
        fill_up(n);
    }
    usleep(buffer_time - 20ms);
}

The buffer time is 370ms, so we usually sleep for 350ms.

It's a lot more complex actually, due to deviating timing and some
code that we don't enter a busy loop if the CPU is slower then the
sound card and so on and so on. However, basically it's this
algorithm.

This works fine on USB. However on HDA it sometimes happens that
snd_pcm_avail_update() is not properly updated at the right
time. Instead it seems to return what was current one iteration
earlier. I.e. we fill up the hw buffer until _avail_update() returns
0. Then assuming that it is fully filled up we go to sleep for
350ms. When we wake up we query _avail_update() again and it will
immediately return 0. We thus don't write anything to the
device. Instead we go to sleep for another 350ms. Of course, when we
wake up again the buffer will already have underrun (since 700 ms
passed since the last write to the buffer and th buffer is only 370ms
long) and we have a problem. In short: _avail_update() told us that
everything was alright and we trusted it and it lied to us.

The happens to be configured to 2 periods. So even if _avail_update()
has some kind of period granularity (?) it should always have told us
after we come back from the sleeping that at least *one* period is
free. But it said 0 frames. Nada. Rien. Nichts. Kaputt.

I *am* calling snd_pcm_hwsync(). So I'd assume that _avail_update()
would be as up to date as it gets. But it isn't. :-(

This happens regardless if I open the device as "front:0" (i.e. with
softvol in line) or as "hw:0" (no plugins).

This doesn't always happen. It seems to depend on the machine how
often this happens. On one machine I can reliably reproduce this after
30 iterations.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Buffering issues 2

[Takashi Iwai]

At Thu, 24 Apr 2008 03:18:33 +0200,
Lennart Poettering wrote:
> 
> Heya!
> 
> In addition to the buffering issues pointed out on http://mailman.alsa-project.org/pipermail/alsa-devel/2008-April/007354.html
> I now have some more issues with the way buffering works in ALSA.
> 
> This time I am pretty sure it's a bug in the HDA drivers.

Yes, it's known that some (many?) HD-audio hardwares have bugs
regarding the DMA position inquiry.  They don't update the DMA positon
properly and/or give inaccurate positions.  The driver passes the
value as hardware gives, and this seems to fool some apps
(e.g. JACK).

I'll try to implement some workaround in the driver, at least, not to
return the wrong position in near future.


Takashi

Re: Buffering issues 2

[Lee Revell]

On Wed, Apr 23, 2008 at 9:18 PM, Lennart Poettering <mznyfn@0pointer.de> wrote:
>  The happens to be configured to 2 periods. So even if _avail_update()
>  has some kind of period granularity (?) it should always have told us
>  after we come back from the sleeping that at least *one* period is
>  free. But it said 0 frames. Nada. Rien. Nichts. Kaputt.

Should we just disallow 2 periods per buffer on HDA intel?  I've never
heard of it working for anyone on the JACK and linux audio user lists
with less than 3.

Lee

Re: Buffering issues 2

[Takashi Iwai]

At Sat, 3 May 2008 21:12:19 -0400,
Lee Revell wrote:
> 
> On Wed, Apr 23, 2008 at 9:18 PM, Lennart Poettering <mznyfn@0pointer.de> wrote:
> >  The happens to be configured to 2 periods. So even if _avail_update()
> >  has some kind of period granularity (?) it should always have told us
> >  after we come back from the sleeping that at least *one* period is
> >  free. But it said 0 frames. Nada. Rien. Nichts. Kaputt.
> 
> Should we just disallow 2 periods per buffer on HDA intel?  I've never
> heard of it working for anyone on the JACK and linux audio user lists
> with less than 3.

It's actually not the number of periods.  The fact is that the DMA
position is somehow inaacurate on HD-audio hardwares.  This should be
fixed instead of a workaround.  Setting the periods to 3 doesn't
assure that any apps works.  (For JACK, a different position_fix would
work, but this may cause another problem with dmix, etc, BTW.)

The similar side-effect is found on other drivers which typically use
timer-like IRQs.


Takashi

[PATCH] Fix DMA position inaccuracy on HDA-Intel

[Takashi Iwai]

Hi,

as mentioned in the earlier post, there is a known problem in
HDA-Intel driver about the DMA positioning.  The below is my recent
attempt to fix this problem.

Basically it adds a sanity check of the DMA position at PCM period
update in irq handler.  If it's inaccurate, it means that the period
is really not ready for update, usually a few samples behind.  Then
the driver delays the period update using a workq until the accurate
DMA position is reported.

It seems working fine on my test machines.  Please let me know if you
have any problems with this patch.

I'm not sure whether it's a 2.6.26 material.  I believe it's still OK
unless we get a bad regression with this.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e5ac9e1..3a006d0 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -285,6 +285,7 @@ struct azx_dev {
 	u32 *posbuf;		/* position buffer pointer */
 
 	unsigned int bufsize;	/* size of the play buffer in bytes */
+	unsigned int period_bytes; /* size of the period in bytes */
 	unsigned int frags;	/* number for period in the play buffer */
 	unsigned int fifo_size;	/* FIFO size */
 
@@ -301,11 +302,10 @@ struct azx_dev {
 					 */
 	unsigned char stream_tag;	/* assigned stream */
 	unsigned char index;		/* stream index */
-	/* for sanity check of position buffer */
-	unsigned int period_intr;
 
 	unsigned int opened :1;
 	unsigned int running :1;
+	unsigned int irq_pending: 1;
 };
 
 /* CORB/RIRB */
@@ -369,6 +369,9 @@ struct azx {
 
 	/* for debugging */
 	unsigned int last_cmd;	/* last issued command (to sync) */
+
+	/* for pending irqs */
+	struct work_struct irq_pending_work;
 };
 
 /* driver types */
@@ -908,6 +911,8 @@ static void azx_init_pci(struct azx *chip)
 }
 
 
+static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev);
+
 /*
  * interrupt handler
  */
@@ -930,11 +935,18 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
 		azx_dev = &chip->azx_dev[i];
 		if (status & azx_dev->sd_int_sta_mask) {
 			azx_sd_writeb(azx_dev, SD_STS, SD_INT_MASK);
-			if (azx_dev->substream && azx_dev->running) {
-				azx_dev->period_intr++;
+			if (!azx_dev->substream || !azx_dev->running)
+				continue;
+			/* check whether this IRQ is really acceptable */
+			if (azx_position_ok(chip, azx_dev)) {
+				azx_dev->irq_pending = 0;
 				spin_unlock(&chip->reg_lock);
 				snd_pcm_period_elapsed(azx_dev->substream);
 				spin_lock(&chip->reg_lock);
+			} else {
+				/* bogus IRQ, process it later */
+				azx_dev->irq_pending = 1;
+				schedule_work(&chip->irq_pending_work);
 			}
 		}
 	}
@@ -973,6 +985,7 @@ static int azx_setup_periods(struct snd_pcm_substream *substream,
 	azx_sd_writel(azx_dev, SD_BDLPU, 0);
 
 	period_bytes = snd_pcm_lib_period_bytes(substream);
+	azx_dev->period_bytes = period_bytes;
 	periods = azx_dev->bufsize / period_bytes;
 
 	/* program the initial BDL entries */
@@ -1421,27 +1434,16 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	return 0;
 }
 
-static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
+static unsigned int azx_get_position(struct azx *chip,
+				     struct azx_dev *azx_dev)
 {
-	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
-	struct azx *chip = apcm->chip;
-	struct azx_dev *azx_dev = get_azx_dev(substream);
 	unsigned int pos;
 
 	if (chip->position_fix == POS_FIX_POSBUF ||
 	    chip->position_fix == POS_FIX_AUTO) {
 		/* use the position buffer */
 		pos = le32_to_cpu(*azx_dev->posbuf);
-		if (chip->position_fix == POS_FIX_AUTO &&
-		    azx_dev->period_intr == 1 && !pos) {
-			printk(KERN_WARNING
-			       "hda-intel: Invalid position buffer, "
-			       "using LPIB read method instead.\n");
-			chip->position_fix = POS_FIX_NONE;
-			goto read_lpib;
-		}
 	} else {
-	read_lpib:
 		/* read LPIB */
 		pos = azx_sd_readl(azx_dev, SD_LPIB);
 		if (chip->position_fix == POS_FIX_FIFO)
@@ -1449,7 +1451,90 @@ static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
 	}
 	if (pos >= azx_dev->bufsize)
 		pos = 0;
-	return bytes_to_frames(substream->runtime, pos);
+	return pos;
+}
+
+static snd_pcm_uframes_t azx_pcm_pointer(struct snd_pcm_substream *substream)
+{
+	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
+	struct azx *chip = apcm->chip;
+	struct azx_dev *azx_dev = get_azx_dev(substream);
+	return bytes_to_frames(substream->runtime,
+			       azx_get_position(chip, azx_dev));
+}
+
+/*
+ * Check whether the current DMA position is acceptable for updating
+ * periods.  Returns non-zero if it's OK.
+ *
+ * Many HD-audio controllers appear pretty inaccurate about
+ * the update-IRQ timing.  The IRQ is issued before actually the
+ * data is processed.  So, we need to process it afterwords in a
+ * workqueue.
+ */
+static int azx_position_ok(struct azx *chip, struct azx_dev *azx_dev)
+{
+	unsigned int pos;
+
+	pos = azx_get_position(chip, azx_dev);
+	if (chip->position_fix == POS_FIX_AUTO) {
+		if (!pos) {
+			printk(KERN_WARNING
+			       "hda-intel: Invalid position buffer, "
+			       "using LPIB read method instead.\n");
+			chip->position_fix = POS_FIX_NONE;
+			pos = azx_get_position(chip, azx_dev);
+		} else
+			chip->position_fix = POS_FIX_POSBUF;
+	}
+
+	if (pos % azx_dev->period_bytes > azx_dev->period_bytes / 2)
+		return 0; /* NG - it's below the period boundary */
+	return 1; /* OK, it's fine */
+}
+
+/*
+ * The work for pending PCM period updates.
+ */
+static void azx_irq_pending_work(struct work_struct *work)
+{
+	struct azx *chip = container_of(work, struct azx, irq_pending_work);
+	int i, pending;
+
+	for (;;) {
+		pending = 0;
+		spin_lock_irq(&chip->reg_lock);
+		for (i = 0; i < chip->num_streams; i++) {
+			struct azx_dev *azx_dev = &chip->azx_dev[i];
+			if (!azx_dev->irq_pending ||
+			    !azx_dev->substream ||
+			    !azx_dev->running)
+				continue;
+			if (azx_position_ok(chip, azx_dev)) {
+				azx_dev->irq_pending = 0;
+				spin_unlock(&chip->reg_lock);
+				snd_pcm_period_elapsed(azx_dev->substream);
+				spin_lock(&chip->reg_lock);
+			} else
+				pending++;
+		}
+		spin_unlock_irq(&chip->reg_lock);
+		if (!pending)
+			return;
+		cond_resched();
+	}
+}
+
+/* clear irq_pending flags and assure no on-going workq */
+static void azx_clear_irq_pending(struct azx *chip)
+{
+	int i;
+
+	spin_lock_irq(&chip->reg_lock);
+	for (i = 0; i < chip->num_streams; i++)
+		chip->azx_dev[i].irq_pending = 0;
+	spin_unlock_irq(&chip->reg_lock);
+	flush_scheduled_work();
 }
 
 static struct snd_pcm_ops azx_pcm_ops = {
@@ -1678,6 +1763,7 @@ static int azx_suspend(struct pci_dev *pci, pm_message_t state)
 	int i;
 
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+	azx_clear_irq_pending(chip);
 	for (i = 0; i < AZX_MAX_PCMS; i++)
 		snd_pcm_suspend_all(chip->pcm[i]);
 	if (chip->initialized)
@@ -1734,6 +1820,7 @@ static int azx_free(struct azx *chip)
 	int i;
 
 	if (chip->initialized) {
+		azx_clear_irq_pending(chip);
 		for (i = 0; i < chip->num_streams; i++)
 			azx_stream_stop(chip, &chip->azx_dev[i]);
 		azx_stop_chip(chip);
@@ -1859,6 +1946,7 @@ static int __devinit azx_create(struct snd_card *card, struct pci_dev *pci,
 	chip->irq = -1;
 	chip->driver_type = driver_type;
 	chip->msi = enable_msi;
+	INIT_WORK(&chip->irq_pending_work, azx_irq_pending_work);
 
 	chip->position_fix = check_position_fix(chip, position_fix[dev]);
 	check_probe_mask(chip, dev);

Re: [PATCH] Fix DMA position inaccuracy on HDA-Intel

[Takashi Iwai]

At Thu, 15 May 2008 16:09:02 +0200,
I wrote:
> 
> Hi,
> 
> as mentioned in the earlier post, there is a known problem in
> HDA-Intel driver about the DMA positioning.  The below is my recent
> attempt to fix this problem.

FYI, I applied the patch to ALSA tree as it survived a day-long test.


Takashi