[alsa-devel] Buffering issues 2
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
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
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);
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
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
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
participants (3)
-
Lee Revell -
Lennart Poettering -
Takashi Iwai