HD-audio runtime->delay accounting

HD-audio runtime->delay accounting

[Takashi Iwai]

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 },

Re: HD-audio runtime->delay accounting

[David Henningsson]

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.deb


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: HD-audio runtime->delay accounting

[Takashi Iwai]

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.deb

That's great.


thanks,

Takashi