[PATCH 0/3] Improve SKL+ position reporting for legacy HD-audio

[PATCH 0/3] Improve SKL+ position reporting for legacy HD-audio

[Takashi Iwai]

Hi,

this is a port of SKL-specific hacks to the legacy HD-audio driver for
improving the PCM position reporting that has been applied to ASoC
driver so far.


Takashi

===

Takashi Iwai (3):
  ALSA: hda - Avoid tricky macros
  ALSA: hda - Move SKL+ vendor specific register definitions to
    hda_register.h
  ALSA: hda - Improved position reporting on SKL+

 include/sound/hda_register.h  | 22 ++++++++++++++++++++--
 include/sound/hdaudio.h       | 28 ++++++++++++++++++----------
 sound/hda/hdac_controller.c   |  2 +-
 sound/hda/hdac_stream.c       |  4 ++--
 sound/pci/hda/hda_intel.c     | 40 ++++++++++++++++++++++++++++++++++++----
 sound/soc/intel/skylake/skl.h | 21 ---------------------
 6 files changed, 77 insertions(+), 40 deletions(-)

-- 
2.11.1

[PATCH 1/3] ALSA: hda - Avoid tricky macros

[Takashi Iwai]

The macros _snd_hdac_chip_read() and *_write() expand to different
types (b,w,l) per their argument.  They were thought to be used only
internally for other snd_hdac_chip_*() macros, but in some situations
we need to call these directly, and they are way too ugly.

Instead of saving a few lines, we just write these macros explicitly
with the types, so that they can be used in a saner way.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hdaudio.h     | 28 ++++++++++++++++++----------
 sound/hda/hdac_controller.c |  2 +-
 sound/hda/hdac_stream.c     |  4 ++--
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 56004ec8d441..96546b30e900 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -368,24 +368,32 @@ void snd_hdac_bus_free_stream_pages(struct hdac_bus *bus);
 /*
  * macros for easy use
  */
-#define _snd_hdac_chip_write(type, chip, reg, value) \
-	((chip)->io_ops->reg_write ## type(value, (chip)->remap_addr + (reg)))
-#define _snd_hdac_chip_read(type, chip, reg) \
-	((chip)->io_ops->reg_read ## type((chip)->remap_addr + (reg)))
+#define _snd_hdac_chip_writeb(chip, reg, value) \
+	((chip)->io_ops->reg_writeb(value, (chip)->remap_addr + (reg)))
+#define _snd_hdac_chip_readb(chip, reg) \
+	((chip)->io_ops->reg_readb((chip)->remap_addr + (reg)))
+#define _snd_hdac_chip_writew(chip, reg, value) \
+	((chip)->io_ops->reg_writew(value, (chip)->remap_addr + (reg)))
+#define _snd_hdac_chip_readw(chip, reg) \
+	((chip)->io_ops->reg_readw((chip)->remap_addr + (reg)))
+#define _snd_hdac_chip_writel(chip, reg, value) \
+	((chip)->io_ops->reg_writel(value, (chip)->remap_addr + (reg)))
+#define _snd_hdac_chip_readl(chip, reg) \
+	((chip)->io_ops->reg_readl((chip)->remap_addr + (reg)))
 
 /* read/write a register, pass without AZX_REG_ prefix */
 #define snd_hdac_chip_writel(chip, reg, value) \
-	_snd_hdac_chip_write(l, chip, AZX_REG_ ## reg, value)
+	_snd_hdac_chip_writel(chip, AZX_REG_ ## reg, value)
 #define snd_hdac_chip_writew(chip, reg, value) \
-	_snd_hdac_chip_write(w, chip, AZX_REG_ ## reg, value)
+	_snd_hdac_chip_writew(chip, AZX_REG_ ## reg, value)
 #define snd_hdac_chip_writeb(chip, reg, value) \
-	_snd_hdac_chip_write(b, chip, AZX_REG_ ## reg, value)
+	_snd_hdac_chip_writeb(chip, AZX_REG_ ## reg, value)
 #define snd_hdac_chip_readl(chip, reg) \
-	_snd_hdac_chip_read(l, chip, AZX_REG_ ## reg)
+	_snd_hdac_chip_readl(chip, AZX_REG_ ## reg)
 #define snd_hdac_chip_readw(chip, reg) \
-	_snd_hdac_chip_read(w, chip, AZX_REG_ ## reg)
+	_snd_hdac_chip_readw(chip, AZX_REG_ ## reg)
 #define snd_hdac_chip_readb(chip, reg) \
-	_snd_hdac_chip_read(b, chip, AZX_REG_ ## reg)
+	_snd_hdac_chip_readb(chip, AZX_REG_ ## reg)
 
 /* update a register, pass without AZX_REG_ prefix */
 #define snd_hdac_chip_updatel(chip, reg, mask, val) \
diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c
index 043065867656..d15b653de0bf 100644
--- a/sound/hda/hdac_controller.c
+++ b/sound/hda/hdac_controller.c
@@ -272,7 +272,7 @@ int snd_hdac_bus_parse_capabilities(struct hdac_bus *bus)
 
 	/* Lets walk the linked capabilities list */
 	do {
-		cur_cap = _snd_hdac_chip_read(l, bus, offset);
+		cur_cap = _snd_hdac_chip_readl(bus, offset);
 
 		dev_dbg(bus->dev, "Capability version: 0x%x\n",
 			(cur_cap & AZX_CAP_HDR_VER_MASK) >> AZX_CAP_HDR_VER_OFF);
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index c6994ebb4567..e1472c7ab6c1 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -555,12 +555,12 @@ void snd_hdac_stream_sync_trigger(struct hdac_stream *azx_dev, bool set,
 
 	if (!reg)
 		reg = AZX_REG_SSYNC;
-	val = _snd_hdac_chip_read(l, bus, reg);
+	val = _snd_hdac_chip_readl(bus, reg);
 	if (set)
 		val |= streams;
 	else
 		val &= ~streams;
-	_snd_hdac_chip_write(l, bus, reg, val);
+	_snd_hdac_chip_writel(bus, reg, val);
 }
 EXPORT_SYMBOL_GPL(snd_hdac_stream_sync_trigger);
 
-- 
2.11.1

[PATCH 2/3] ALSA: hda - Move SKL+ vendor specific register definitions to hda_register.h

[Takashi Iwai]

They may be used by both legacy and ASoC drivers.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hda_register.h  | 22 ++++++++++++++++++++--
 sound/pci/hda/hda_intel.c     |  4 ++--
 sound/soc/intel/skylake/skl.h | 21 ---------------------
 3 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h
index 0013063db7f2..1251ff41c9d3 100644
--- a/include/sound/hda_register.h
+++ b/include/sound/hda_register.h
@@ -106,8 +106,26 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 };
 #define AZX_REG_HSW_EM4			0x100c
 #define AZX_REG_HSW_EM5			0x1010
 
-/* Skylake/Broxton display HD-A controller Extended Mode registers */
-#define AZX_REG_SKL_EM4L		0x1040
+/* Skylake/Broxton vendor-specific registers */
+#define AZX_REG_VS_EM1			0x1000
+#define AZX_REG_VS_INRC			0x1004
+#define AZX_REG_VS_OUTRC		0x1008
+#define AZX_REG_VS_FIFOTRK		0x100C
+#define AZX_REG_VS_FIFOTRK2		0x1010
+#define AZX_REG_VS_EM2			0x1030
+#define AZX_REG_VS_EM3L			0x1038
+#define AZX_REG_VS_EM3U			0x103C
+#define AZX_REG_VS_EM4L			0x1040
+#define AZX_REG_VS_EM4U			0x1044
+#define AZX_REG_VS_LTRC			0x1048
+#define AZX_REG_VS_D0I3C		0x104A
+#define AZX_REG_VS_PCE			0x104B
+#define AZX_REG_VS_L2MAGC		0x1050
+#define AZX_REG_VS_L2LAHPT		0x1054
+#define AZX_REG_VS_SDXDPIB_XBASE	0x1084
+#define AZX_REG_VS_SDXDPIB_XINTERVAL	0x20
+#define AZX_REG_VS_SDXEFIFOS_XBASE	0x1094
+#define AZX_REG_VS_SDXEFIFOS_XINTERVAL	0x20
 
 /* PCI space */
 #define AZX_PCIREG_TCSEL		0x44
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c8256a89375a..a48330f4a1a9 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -534,9 +534,9 @@ static void bxt_reduce_dma_latency(struct azx *chip)
 {
 	u32 val;
 
-	val = azx_readl(chip, SKL_EM4L);
+	val = azx_readl(chip, VS_EM4L);
 	val &= (0x3 << 20);
-	azx_writel(chip, SKL_EM4L, val);
+	azx_writel(chip, VS_EM4L, val);
 }
 
 static void hda_intel_init_chip(struct azx *chip, bool full_reset)
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index bbef77d2b917..8e2878012d53 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -27,27 +27,6 @@
 
 #define SKL_SUSPEND_DELAY 2000
 
-/* Vendor Specific Registers */
-#define AZX_REG_VS_EM1			0x1000
-#define AZX_REG_VS_INRC			0x1004
-#define AZX_REG_VS_OUTRC		0x1008
-#define AZX_REG_VS_FIFOTRK		0x100C
-#define AZX_REG_VS_FIFOTRK2		0x1010
-#define AZX_REG_VS_EM2			0x1030
-#define AZX_REG_VS_EM3L			0x1038
-#define AZX_REG_VS_EM3U			0x103C
-#define AZX_REG_VS_EM4L			0x1040
-#define AZX_REG_VS_EM4U			0x1044
-#define AZX_REG_VS_LTRC			0x1048
-#define AZX_REG_VS_D0I3C		0x104A
-#define AZX_REG_VS_PCE			0x104B
-#define AZX_REG_VS_L2MAGC		0x1050
-#define AZX_REG_VS_L2LAHPT		0x1054
-#define AZX_REG_VS_SDXDPIB_XBASE	0x1084
-#define AZX_REG_VS_SDXDPIB_XINTERVAL	0x20
-#define AZX_REG_VS_SDXEFIFOS_XBASE	0x1094
-#define AZX_REG_VS_SDXEFIFOS_XINTERVAL	0x20
-
 #define AZX_PCIREG_PGCTL		0x44
 #define AZX_PGCTL_LSRMD_MASK		(1 << 4)
 #define AZX_PCIREG_CGCTL		0x48
-- 
2.11.1

[PATCH 3/3] ALSA: hda - Improved position reporting on SKL+

[Takashi Iwai]

Apply the same methods to obtain the current stream position as ASoC
Intel SKL driver uses.  It reads the position from DPIB for a playback
stream while it still reads from the position buffer for a capture
stream.  For a capture stream, some ugly workaround is needed to
settle down the inconsistent position.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index a48330f4a1a9..64db6698214c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -77,6 +77,7 @@ enum {
 	POS_FIX_POSBUF,
 	POS_FIX_VIACOMBO,
 	POS_FIX_COMBO,
+	POS_FIX_SKL,
 };
 
 /* Defines for ATI HD Audio support in SB450 south bridge */
@@ -148,7 +149,7 @@ module_param_array(model, charp, NULL, 0444);
 MODULE_PARM_DESC(model, "Use the given board model.");
 module_param_array(position_fix, int, NULL, 0444);
 MODULE_PARM_DESC(position_fix, "DMA pointer read method."
-		 "(-1 = system default, 0 = auto, 1 = LPIB, 2 = POSBUF, 3 = VIACOMBO, 4 = COMBO).");
+		 "(-1 = system default, 0 = auto, 1 = LPIB, 2 = POSBUF, 3 = VIACOMBO, 4 = COMBO, 5 = SKL+).");
 module_param_array(bdl_pos_adj, int, NULL, 0644);
 MODULE_PARM_DESC(bdl_pos_adj, "BDL position adjustment offset.");
 module_param_array(probe_mask, int, NULL, 0444);
@@ -815,6 +816,31 @@ static unsigned int azx_via_get_position(struct azx *chip,
 	return bound_pos + mod_dma_pos;
 }
 
+static unsigned int azx_skl_get_dpib_pos(struct azx *chip,
+					 struct azx_dev *azx_dev)
+{
+	return _snd_hdac_chip_readl(azx_bus(chip),
+				    AZX_REG_VS_SDXDPIB_XBASE +
+				    (AZX_REG_VS_SDXDPIB_XINTERVAL *
+				     azx_dev->core.index));
+}
+
+/* get the current DMA position with correction on SKL+ chips */
+static unsigned int azx_get_pos_skl(struct azx *chip, struct azx_dev *azx_dev)
+{
+	/* DPIB register gives a more accurate position for playback */
+	if (azx_dev->core.substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		return azx_skl_get_dpib_pos(chip, azx_dev);
+
+	/* For capture, we need to read posbuf, but it requires a delay
+	 * for the possible boundary overlap; the read of DPIB fetches the
+	 * actual posbuf
+	 */
+	udelay(20);
+	azx_skl_get_dpib_pos(chip, azx_dev);
+	return azx_get_pos_posbuf(chip, azx_dev);
+}
+
 #ifdef CONFIG_PM
 static DEFINE_MUTEX(card_list_lock);
 static LIST_HEAD(card_list);
@@ -1351,6 +1377,7 @@ static int check_position_fix(struct azx *chip, int fix)
 	case POS_FIX_POSBUF:
 	case POS_FIX_VIACOMBO:
 	case POS_FIX_COMBO:
+	case POS_FIX_SKL:
 		return fix;
 	}
 
@@ -1371,6 +1398,10 @@ static int check_position_fix(struct azx *chip, int fix)
 		dev_dbg(chip->card->dev, "Using LPIB position fix\n");
 		return POS_FIX_LPIB;
 	}
+	if (IS_SKL_PLUS(chip->pci)) {
+		dev_dbg(chip->card->dev, "Using SKL position fix\n");
+		return POS_FIX_SKL;
+	}
 	return POS_FIX_AUTO;
 }
 
@@ -1382,6 +1413,7 @@ static void assign_position_fix(struct azx *chip, int fix)
 		[POS_FIX_POSBUF] = azx_get_pos_posbuf,
 		[POS_FIX_VIACOMBO] = azx_via_get_position,
 		[POS_FIX_COMBO] = azx_get_pos_lpib,
+		[POS_FIX_SKL] = azx_get_pos_skl,
 	};
 
 	chip->get_position[0] = chip->get_position[1] = callbacks[fix];
@@ -1390,7 +1422,7 @@ static void assign_position_fix(struct azx *chip, int fix)
 	if (fix == POS_FIX_COMBO)
 		chip->get_position[1] = NULL;
 
-	if (fix == POS_FIX_POSBUF &&
+	if ((fix == POS_FIX_POSBUF || fix == POS_FIX_SKL) &&
 	    (chip->driver_caps & AZX_DCAPS_COUNT_LPIB_DELAY)) {
 		chip->get_delay[0] = chip->get_delay[1] =
 			azx_get_delay_from_lpib;
-- 
2.11.1

Re: [PATCH 3/3] ALSA: hda - Improved position reporting on SKL+

[Vinod Koul]

On Fri, Mar 31, 2017 at 10:49:30AM +0200, Takashi Iwai wrote:
> Apply the same methods to obtain the current stream position as ASoC
> Intel SKL driver uses.  It reads the position from DPIB for a playback
> stream while it still reads from the position buffer for a capture
> stream.  For a capture stream, some ugly workaround is needed to
> settle down the inconsistent position.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_intel.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index a48330f4a1a9..64db6698214c 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -77,6 +77,7 @@ enum {
>  	POS_FIX_POSBUF,
>  	POS_FIX_VIACOMBO,
>  	POS_FIX_COMBO,
> +	POS_FIX_SKL,
>  };
>  
>  /* Defines for ATI HD Audio support in SB450 south bridge */
> @@ -148,7 +149,7 @@ module_param_array(model, charp, NULL, 0444);
>  MODULE_PARM_DESC(model, "Use the given board model.");
>  module_param_array(position_fix, int, NULL, 0444);
>  MODULE_PARM_DESC(position_fix, "DMA pointer read method."
> -		 "(-1 = system default, 0 = auto, 1 = LPIB, 2 = POSBUF, 3 = VIACOMBO, 4 = COMBO).");
> +		 "(-1 = system default, 0 = auto, 1 = LPIB, 2 = POSBUF, 3 = VIACOMBO, 4 = COMBO, 5 = SKL+).");

do we have people use this module param, or is it find what works for
them...

-- 
~Vinod

Re: [PATCH 3/3] ALSA: hda - Improved position reporting on SKL+

[Takashi Iwai]

On Mon, 03 Apr 2017 04:58:38 +0200,
Vinod Koul wrote:
> 
> On Fri, Mar 31, 2017 at 10:49:30AM +0200, Takashi Iwai wrote:
> > Apply the same methods to obtain the current stream position as ASoC
> > Intel SKL driver uses.  It reads the position from DPIB for a playback
> > stream while it still reads from the position buffer for a capture
> > stream.  For a capture stream, some ugly workaround is needed to
> > settle down the inconsistent position.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/pci/hda/hda_intel.c | 36 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index a48330f4a1a9..64db6698214c 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -77,6 +77,7 @@ enum {
> >  	POS_FIX_POSBUF,
> >  	POS_FIX_VIACOMBO,
> >  	POS_FIX_COMBO,
> > +	POS_FIX_SKL,
> >  };
> >  
> >  /* Defines for ATI HD Audio support in SB450 south bridge */
> > @@ -148,7 +149,7 @@ module_param_array(model, charp, NULL, 0444);
> >  MODULE_PARM_DESC(model, "Use the given board model.");
> >  module_param_array(position_fix, int, NULL, 0444);
> >  MODULE_PARM_DESC(position_fix, "DMA pointer read method."
> > -		 "(-1 = system default, 0 = auto, 1 = LPIB, 2 = POSBUF, 3 = VIACOMBO, 4 = COMBO).");
> > +		 "(-1 = system default, 0 = auto, 1 = LPIB, 2 = POSBUF, 3 = VIACOMBO, 4 = COMBO, 5 = SKL+).");
> 
> do we have people use this module param, or is it find what works for
> them...

It's for debug purpose, and I know people trying this for various
issues.


Takashi

Re: [PATCH 0/3] Improve SKL+ position reporting for legacy HD-audio

[Vinod Koul]

On Fri, Mar 31, 2017 at 10:49:27AM +0200, Takashi Iwai wrote:
> Hi,
> 
> this is a port of SKL-specific hacks to the legacy HD-audio driver for
> improving the PCM position reporting that has been applied to ASoC
> driver so far.

Yeah the series looks good to me:

Acked-by: Vinod Koul <vinod.koul@intel.com>

Thanks
-- 
~Vinod

Re: [PATCH 0/3] Improve SKL+ position reporting for legacy HD-audio

[Takashi Iwai]

On Mon, 03 Apr 2017 04:59:17 +0200,
Vinod Koul wrote:
> 
> On Fri, Mar 31, 2017 at 10:49:27AM +0200, Takashi Iwai wrote:
> > Hi,
> > 
> > this is a port of SKL-specific hacks to the legacy HD-audio driver for
> > improving the PCM position reporting that has been applied to ASoC
> > driver so far.
> 
> Yeah the series looks good to me:
> 
> Acked-by: Vinod Koul <vinod.koul@intel.com>

OK, merged now to for-next branch.  Thanks.


Takashi