[alsa-devel] [PATCH 0/3] Improve SKL+ position reporting for legacy HD-audio
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(-)
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);
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
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;
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...
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
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
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
participants (2)
-
Takashi Iwai
-
Vinod Koul