[alsa-devel] HD-audio runtime->delay accounting
Hi,
recently Pierre questioned about the validity of COMBO positioning mode, and it turned out that the position reporting can be handled better if we take the difference between LPIB and DMAPOS as runtime->delay. (In short, LPIB points the position where the data fed to the codec, and DMAPOS points the data fetched to DMA.)
We cooked up a test patch below and it seems working well on a few machines I've tested. I think we might have still chance to put this into 3.7, as this would be a good improvement. The patch below is a combined work of two fixes: fixing the better stream start by always using SSYNC bit, and adjusting the position and runtime->delay.
Of course, the biggest question is the test coverage. David, could you check whether this doesn't break so much on (some) machines you can test? Of course, if any other people can test it, it'll be a great help, too.
The patch should be applied to the latest sound.git tree master branch, but it should be applicable (and easily resolvable) for older versions. The patch extends this new mode only for recent Intel chips, just to be sure.
thanks,
Takashi
--- diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 66fefd1..b1d99d0 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -555,6 +555,7 @@ enum { #define AZX_DCAPS_ALIGN_BUFSIZE (1 << 22) /* buffer size alignment */ #define AZX_DCAPS_4K_BDLE_BOUNDARY (1 << 23) /* BDLE in 4k boundary */ #define AZX_DCAPS_POSFIX_COMBO (1 << 24) /* Use COMBO as default */ +#define AZX_DCAPS_COUNT_LPIB_DELAY (1 << 25) /* Take LPIB as delay */
/* quirks for ATI SB / AMD Hudson */ #define AZX_DCAPS_PRESET_ATI_SB \ @@ -2003,14 +2004,14 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) }
spin_lock(&chip->reg_lock); - if (nsync > 1) { - /* first, set SYNC bits of corresponding streams */ - if (chip->driver_caps & AZX_DCAPS_OLD_SSYNC) - azx_writel(chip, OLD_SSYNC, - azx_readl(chip, OLD_SSYNC) | sbits); - else - azx_writel(chip, SSYNC, azx_readl(chip, SSYNC) | sbits); - } + + /* first, set SYNC bits of corresponding streams */ + if (chip->driver_caps & AZX_DCAPS_OLD_SSYNC) + azx_writel(chip, OLD_SSYNC, + azx_readl(chip, OLD_SSYNC) | sbits); + else + azx_writel(chip, SSYNC, azx_readl(chip, SSYNC) | sbits); + snd_pcm_group_for_each_entry(s, substream) { if (s->pcm->card != substream->pcm->card) continue; @@ -2028,8 +2029,6 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) } spin_unlock(&chip->reg_lock); if (start) { - if (nsync == 1) - return 0; /* wait until all FIFOs get ready */ for (timeout = 5000; timeout; timeout--) { nwait = 0; @@ -2062,16 +2061,14 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) cpu_relax(); } } - if (nsync > 1) { - spin_lock(&chip->reg_lock); - /* reset SYNC bits */ - if (chip->driver_caps & AZX_DCAPS_OLD_SSYNC) - azx_writel(chip, OLD_SSYNC, - azx_readl(chip, OLD_SSYNC) & ~sbits); - else - azx_writel(chip, SSYNC, azx_readl(chip, SSYNC) & ~sbits); - spin_unlock(&chip->reg_lock); - } + spin_lock(&chip->reg_lock); + /* reset SYNC bits */ + if (chip->driver_caps & AZX_DCAPS_OLD_SSYNC) + azx_writel(chip, OLD_SSYNC, + azx_readl(chip, OLD_SSYNC) & ~sbits); + else + azx_writel(chip, SSYNC, azx_readl(chip, SSYNC) & ~sbits); + spin_unlock(&chip->reg_lock); return 0; }
@@ -2164,6 +2161,28 @@ static unsigned int azx_get_position(struct azx *chip,
if (pos >= azx_dev->bufsize) pos = 0; + + /* calculate runtime delay from LPIB */ + if (azx_dev->substream->runtime && + chip->position_fix[stream] == POS_FIX_POSBUF && + (chip->driver_caps & AZX_DCAPS_COUNT_LPIB_DELAY)) { + unsigned int lpib_pos = azx_sd_readl(azx_dev, SD_LPIB); + int delay; + if (stream == SNDRV_PCM_STREAM_PLAYBACK) + delay = pos - lpib_pos; + else + delay = lpib_pos - pos; + if (delay < 0) + delay += azx_dev->bufsize; + if (delay >= azx_dev->period_bytes) { + snd_printdd("delay %d > period_bytes %d\n", + delay, azx_dev->period_bytes); + delay = 0; /* something is wrong */ + } + azx_dev->substream->runtime->delay = + bytes_to_frames(azx_dev->substream->runtime, delay); + } + return pos; }
@@ -3511,34 +3530,34 @@ static DEFINE_PCI_DEVICE_TABLE(azx_ids) = { /* CPT */ { PCI_DEVICE(0x8086, 0x1c20), .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE | AZX_DCAPS_POSFIX_COMBO }, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* PBG */ { PCI_DEVICE(0x8086, 0x1d20), .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE}, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* Panther Point */ { PCI_DEVICE(0x8086, 0x1e20), .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE | AZX_DCAPS_POSFIX_COMBO }, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* Lynx Point */ { PCI_DEVICE(0x8086, 0x8c20), .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE | AZX_DCAPS_POSFIX_COMBO }, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* Lynx Point-LP */ { PCI_DEVICE(0x8086, 0x9c20), .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE | AZX_DCAPS_POSFIX_COMBO }, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* Lynx Point-LP */ { PCI_DEVICE(0x8086, 0x9c21), .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE | AZX_DCAPS_POSFIX_COMBO }, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* Haswell */ { PCI_DEVICE(0x8086, 0x0c0c), .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE | AZX_DCAPS_POSFIX_COMBO }, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, { PCI_DEVICE(0x8086, 0x0d0c), .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_SCH_SNOOP | - AZX_DCAPS_BUFSIZE | AZX_DCAPS_POSFIX_COMBO }, + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* SCH */ { PCI_DEVICE(0x8086, 0x811b), .driver_data = AZX_DRIVER_SCH | AZX_DCAPS_SCH_SNOOP | @@ -3575,7 +3594,8 @@ static DEFINE_PCI_DEVICE_TABLE(azx_ids) = { { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_ANY_ID), .class = PCI_CLASS_MULTIMEDIA_HD_AUDIO << 8, .class_mask = 0xffffff, - .driver_data = AZX_DRIVER_ICH | AZX_DCAPS_BUFSIZE }, + .driver_data = AZX_DRIVER_ICH | + AZX_DCAPS_BUFSIZE | AZX_DCAPS_COUNT_LPIB_DELAY }, /* ATI SB 450/600/700/800/900 */ { PCI_DEVICE(0x1002, 0x437b), .driver_data = AZX_DRIVER_ATI | AZX_DCAPS_PRESET_ATI_SB },
On 09/21/2012 01:59 PM, Takashi Iwai wrote:
Hi,
recently Pierre questioned about the validity of COMBO positioning mode, and it turned out that the position reporting can be handled better if we take the difference between LPIB and DMAPOS as runtime->delay. (In short, LPIB points the position where the data fed to the codec, and DMAPOS points the data fetched to DMA.)
We cooked up a test patch below and it seems working well on a few machines I've tested. I think we might have still chance to put this into 3.7, as this would be a good improvement. The patch below is a combined work of two fixes: fixing the better stream start by always using SSYNC bit, and adjusting the position and runtime->delay.
Interesting. A quick question: is this affecting playback, recording, or both?
Of course, the biggest question is the test coverage. David, could you check whether this doesn't break so much on (some) machines you can test? Of course, if any other people can test it, it'll be a great help, too.
The patch should be applied to the latest sound.git tree master branch, but it should be applicable (and easily resolvable) for older versions.
Against sound master:
patching file sound/pci/hda/hda_intel.c Hunk #2 succeeded at 1987 (offset -17 lines). Hunk #3 succeeded at 2012 (offset -17 lines). Hunk #4 succeeded at 2044 (offset -17 lines). Hunk #5 succeeded at 2144 (offset -17 lines). Hunk #6 succeeded at 3426 (offset -104 lines). Hunk #7 succeeded at 3490 (offset -104 lines).
The patch extends this new mode only for recent Intel chips, just to be sure.
I don't think I have much such hardware here. But I've made DKMS package for easy testing by others. If you're running Ubuntu 12.04, you can just install this package, reboot, and test:
http://people.canonical.com/~diwic/temp/alsa-hda-dkms-ssync-delay_0.1_all.de...
At Fri, 21 Sep 2012 14:16:43 +0200, David Henningsson wrote:
On 09/21/2012 01:59 PM, Takashi Iwai wrote:
Hi,
recently Pierre questioned about the validity of COMBO positioning mode, and it turned out that the position reporting can be handled better if we take the difference between LPIB and DMAPOS as runtime->delay. (In short, LPIB points the position where the data fed to the codec, and DMAPOS points the data fetched to DMA.)
We cooked up a test patch below and it seems working well on a few machines I've tested. I think we might have still chance to put this into 3.7, as this would be a good improvement. The patch below is a combined work of two fixes: fixing the better stream start by always using SSYNC bit, and adjusting the position and runtime->delay.
Interesting. A quick question: is this affecting playback, recording, or both?
The runtime->delay is calculated for both directions, so if apps take account of runtime->delay, both directions are affected. If apps check only hw_ptr, it'll be only the playback. Now hw_ptr of playback points DMAPOS with the compensation of runtime->delay when this new mode is used.
Of course, the biggest question is the test coverage. David, could you check whether this doesn't break so much on (some) machines you can test? Of course, if any other people can test it, it'll be a great help, too.
The patch should be applied to the latest sound.git tree master branch, but it should be applicable (and easily resolvable) for older versions.
Against sound master:
patching file sound/pci/hda/hda_intel.c Hunk #2 succeeded at 1987 (offset -17 lines). Hunk #3 succeeded at 2012 (offset -17 lines). Hunk #4 succeeded at 2044 (offset -17 lines). Hunk #5 succeeded at 2144 (offset -17 lines). Hunk #6 succeeded at 3426 (offset -104 lines). Hunk #7 succeeded at 3490 (offset -104 lines).
The patch extends this new mode only for recent Intel chips, just to be sure.
I don't think I have much such hardware here. But I've made DKMS package for easy testing by others. If you're running Ubuntu 12.04, you can just install this package, reboot, and test:
http://people.canonical.com/~diwic/temp/alsa-hda-dkms-ssync-delay_0.1_all.de...
That's great.
thanks,
Takashi
participants (2)
-
David Henningsson -
Takashi Iwai