[PATCH v3 0/4] ALSA: hda: Abstract and update HOST-stream setup procedure
The patchset targets two intertwined topics:
The driver shall poll SDxFIFOS to ensure a valid value is set by the controller after programming SDxFMT. Due to amount of users and limited-number of configuration available in our CI when compared to overall possibilities on the market, the check is non-blocking.
Second topic relates to stream setup procedure. The procedure differs between HDAudio controller device revisions. Right now those differences are handled directly by a platform driver. Existing top-level 'if (pci->device == APL)' could be replaced by a abstraction in lower parts of the code instead.
With that done, the two users are updated accordingly. In avs-driver case, this updates the flow to the recommended one.
Changes in v3: - fixed issues pointed out by scripts/kernel-doc
Changes in v2: - fixed ->host_setup assignment in patch 02/04
Cezary Rojewski (4): ALSA: hda: Poll SDxFIFOS after programming SDxFMT ALSA: hda: Introduce HOST stream setup mechanism ASoC: Intel: avs: Use helper to setup HOST stream ASoC: Intel: Skylake: Use helper to setup HOST stream
include/sound/hda_register.h | 2 ++ include/sound/hdaudio.h | 3 +++ include/sound/hdaudio_ext.h | 3 +++ sound/hda/ext/hdac_ext_stream.c | 41 +++++++++++++++++++++++++++++++ sound/hda/hdac_stream.c | 8 ++++++ sound/soc/intel/avs/pcm.c | 2 +- sound/soc/intel/skylake/skl-pcm.c | 14 +---------- 7 files changed, 59 insertions(+), 14 deletions(-)
Software shall read SDxFIFOS calculated by the hardware and notify if invalid value is programmed before continuing the stream preparation.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hda_register.h | 2 ++ include/sound/hdaudio.h | 3 +++ sound/hda/hdac_stream.c | 8 ++++++++ 3 files changed, 13 insertions(+)
diff --git a/include/sound/hda_register.h b/include/sound/hda_register.h index 9c7872c0ca79..55958711d697 100644 --- a/include/sound/hda_register.h +++ b/include/sound/hda_register.h @@ -91,6 +91,8 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO1, SDO2, SDO3 }; #define AZX_REG_SD_BDLPL 0x18 #define AZX_REG_SD_BDLPU 0x1c
+#define AZX_SD_FIFOSIZE_MASK GENMASK(15, 0) + /* GTS registers */ #define AZX_REG_LLCH 0x14
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 32c59053b48e..41d725babf53 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -624,6 +624,9 @@ int snd_hdac_stream_set_lpib(struct hdac_stream *azx_dev, u32 value); #define snd_hdac_stream_readb_poll(dev, reg, val, cond, delay_us, timeout_us) \ read_poll_timeout_atomic(snd_hdac_reg_readb, val, cond, delay_us, timeout_us, \ false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) +#define snd_hdac_stream_readw_poll(dev, reg, val, cond, delay_us, timeout_us) \ + read_poll_timeout_atomic(snd_hdac_reg_readw, val, cond, delay_us, timeout_us, \ + false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) #define snd_hdac_stream_readl_poll(dev, reg, val, cond, delay_us, timeout_us) \ read_poll_timeout_atomic(snd_hdac_reg_readl, val, cond, delay_us, timeout_us, \ false, (dev)->bus, (dev)->sd_addr + AZX_REG_ ## reg) diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c index 2633a4bb1d85..5382894bebab 100644 --- a/sound/hda/hdac_stream.c +++ b/sound/hda/hdac_stream.c @@ -258,6 +258,8 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev) struct hdac_bus *bus = azx_dev->bus; struct snd_pcm_runtime *runtime; unsigned int val; + u16 reg; + int ret;
if (azx_dev->substream) runtime = azx_dev->substream->runtime; @@ -300,6 +302,12 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev) /* set the interrupt enable bits in the descriptor control register */ snd_hdac_stream_updatel(azx_dev, SD_CTL, 0, SD_INT_MASK);
+ /* Once SDxFMT is set, the controller programs SDxFIFOS to non-zero value. */ + ret = snd_hdac_stream_readw_poll(azx_dev, SD_FIFOSIZE, reg, reg & AZX_SD_FIFOSIZE_MASK, + 3, 300); + if (ret) + dev_dbg(bus->dev, "polling SD_FIFOSIZE 0x%04x failed: %d\n", + AZX_REG_SD_FIFOSIZE, ret); azx_dev->fifo_size = snd_hdac_stream_readw(azx_dev, SD_FIFOSIZE) + 1;
/* when LPIB delay correction gives a small negative value,
On 2023-09-26 10:06 AM, Cezary Rojewski wrote:
Software shall read SDxFIFOS calculated by the hardware and notify if invalid value is programmed before continuing the stream preparation.
...
@@ -300,6 +302,12 @@ int snd_hdac_stream_setup(struct hdac_stream *azx_dev) /* set the interrupt enable bits in the descriptor control register */ snd_hdac_stream_updatel(azx_dev, SD_CTL, 0, SD_INT_MASK);
- /* Once SDxFMT is set, the controller programs SDxFIFOS to non-zero value. */
- ret = snd_hdac_stream_readw_poll(azx_dev, SD_FIFOSIZE, reg, reg & AZX_SD_FIFOSIZE_MASK,
3, 300);- if (ret)
dev_dbg(bus->dev, "polling SD_FIFOSIZE 0x%04x failed: %d\n",AZX_REG_SD_FIFOSIZE, ret);
There is one (negligible?) side effect. AudioDSP firmware is the one who kicks SDxFIFOS calculation when a stream is decoupled mode. During firmware bring up procedure, there is no firmware running _and_ the code-loading stream is always a decoupled one. So, there is none to trigger the calculation and we end up with debug -110 messages. It looks like to do this in complete fashion some refactoring is needed in hdac_stream.c/hdac_ext_stream.c.
Czarek
azx_dev->fifo_size = snd_hdac_stream_readw(azx_dev, SD_FIFOSIZE) + 1;
/* when LPIB delay correction gives a small negative value,
HDAudio stream setup procedure differs between revisions of the controller device. Currently the differences are handled directly within AudioDSP platform drivers with if-statements. Implement a more generic approach and expose a function that a platform driver may use to ensure the correct procedure is followed each time.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hdaudio_ext.h | 3 +++ sound/hda/ext/hdac_ext_stream.c | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index 511211f4a2b6..d32959cb71d2 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -60,6 +60,8 @@ struct hdac_ext_stream { bool link_locked:1; bool link_prepared;
+ int (*host_setup)(struct hdac_stream *); + struct snd_pcm_substream *link_substream; };
@@ -86,6 +88,7 @@ void snd_hdac_ext_stream_start(struct hdac_ext_stream *hext_stream); void snd_hdac_ext_stream_clear(struct hdac_ext_stream *hext_stream); void snd_hdac_ext_stream_reset(struct hdac_ext_stream *hext_stream); int snd_hdac_ext_stream_setup(struct hdac_ext_stream *hext_stream, int fmt); +int snd_hdac_ext_host_stream_setup(struct hdac_ext_stream *hext_stream);
struct hdac_ext_link { struct hdac_bus *bus; diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index 11b7119cc47e..186e95bffb28 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -10,12 +10,45 @@ */
#include <linux/delay.h> +#include <linux/pci.h> +#include <linux/pci_ids.h> #include <linux/slab.h> #include <sound/pcm.h> #include <sound/hda_register.h> #include <sound/hdaudio_ext.h> #include <sound/compress_driver.h>
+/** + * snd_hdac_ext_host_stream_setup - Setup a HOST stream. + * @hext_stream: HDAudio stream to set up. + * + * Return: Zero on success or negative error code. + */ +int snd_hdac_ext_host_stream_setup(struct hdac_ext_stream *hext_stream) +{ + return hext_stream->host_setup(hdac_stream(hext_stream)); +} +EXPORT_SYMBOL_GPL(snd_hdac_ext_host_stream_setup); + +/** + * snd_hdac_apl_host_stream_setup - Setup a HOST stream following procedure + * recommended for ApolloLake devices. + * @hstream: HDAudio stream to set up. + * + * Return: Zero on success or negative error code. + */ +static int snd_hdac_apl_host_stream_setup(struct hdac_stream *hstream) +{ + struct hdac_ext_stream *hext_stream = stream_to_hdac_ext_stream(hstream); + int ret; + + snd_hdac_ext_stream_decouple(hstream->bus, hext_stream, false); + ret = snd_hdac_stream_setup(hstream); + snd_hdac_ext_stream_decouple(hstream->bus, hext_stream, true); + + return ret; +} + /** * snd_hdac_ext_stream_init - initialize each stream (aka device) * @bus: HD-audio core bus @@ -55,9 +88,16 @@ static void snd_hdac_ext_stream_init(struct hdac_bus *bus, int snd_hdac_ext_stream_init_all(struct hdac_bus *bus, int start_idx, int num_stream, int dir) { + struct pci_dev *pci = to_pci_dev(bus->dev); + int (*setup_op)(struct hdac_stream *); int stream_tag = 0; int i, tag, idx = start_idx;
+ if (pci->device == PCI_DEVICE_ID_INTEL_HDA_APL) + setup_op = snd_hdac_apl_host_stream_setup; + else + setup_op = snd_hdac_stream_setup; + for (i = 0; i < num_stream; i++) { struct hdac_ext_stream *hext_stream = kzalloc(sizeof(*hext_stream), GFP_KERNEL); @@ -66,6 +106,7 @@ int snd_hdac_ext_stream_init_all(struct hdac_bus *bus, int start_idx, tag = ++stream_tag; snd_hdac_ext_stream_init(bus, hext_stream, idx, dir, tag); idx++; + hext_stream->host_setup = setup_op; }
return 0;
snd_hdac_ext_host_stream_setup() abstracts the procedure details away. Simplify the code by using it.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index 8565a530706d..e628fdfdc018 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -625,7 +625,7 @@ static int avs_dai_fe_prepare(struct snd_pcm_substream *substream, struct snd_so if (ret < 0) return ret;
- ret = snd_hdac_stream_setup(hdac_stream(host_stream)); + ret = snd_hdac_ext_host_stream_setup(host_stream); if (ret < 0) return ret;
snd_hdac_ext_host_stream_setup() abstracts the procedure details away. Simplify the code by using it.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl-pcm.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index ac3dc8c63c26..7502b2e70e46 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -124,7 +124,6 @@ static void skl_set_suspend_active(struct snd_pcm_substream *substream, int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params) { struct hdac_bus *bus = dev_get_drvdata(dev); - struct skl_dev *skl = bus_to_skl(bus); unsigned int format_val; struct hdac_stream *hstream; struct hdac_ext_stream *stream; @@ -149,18 +148,7 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params) if (err < 0) return err;
- /* - * The recommended SDxFMT programming sequence for BXT - * platforms is to couple the stream before writing the format - */ - if (HDA_CONTROLLER_IS_APL(skl->pci)) { - snd_hdac_ext_stream_decouple(bus, stream, false); - err = snd_hdac_stream_setup(hdac_stream(stream)); - snd_hdac_ext_stream_decouple(bus, stream, true); - } else { - err = snd_hdac_stream_setup(hdac_stream(stream)); - } - + err = snd_hdac_ext_host_stream_setup(stream); if (err < 0) return err;
On Tue, 26 Sep 2023 10:06:19 +0200, Cezary Rojewski wrote:
The patchset targets two intertwined topics:
The driver shall poll SDxFIFOS to ensure a valid value is set by the controller after programming SDxFMT. Due to amount of users and limited-number of configuration available in our CI when compared to overall possibilities on the market, the check is non-blocking.
Second topic relates to stream setup procedure. The procedure differs between HDAudio controller device revisions. Right now those differences are handled directly by a platform driver. Existing top-level 'if (pci->device == APL)' could be replaced by a abstraction in lower parts of the code instead.
With that done, the two users are updated accordingly. In avs-driver case, this updates the flow to the recommended one.
Changes in v3:
- fixed issues pointed out by scripts/kernel-doc
Changes in v2:
- fixed ->host_setup assignment in patch 02/04
Cezary Rojewski (4): ALSA: hda: Poll SDxFIFOS after programming SDxFMT ALSA: hda: Introduce HOST stream setup mechanism ASoC: Intel: avs: Use helper to setup HOST stream ASoC: Intel: Skylake: Use helper to setup HOST stream
Sorry for the late reaction, as I've been (still) off since the last week.
Now applied now to for-next branch.
thanks,
Takashi
On 2023-10-06 10:44 AM, Takashi Iwai wrote:
On Tue, 26 Sep 2023 10:06:19 +0200, Cezary Rojewski wrote:
The patchset targets two intertwined topics:
The driver shall poll SDxFIFOS to ensure a valid value is set by the controller after programming SDxFMT. Due to amount of users and limited-number of configuration available in our CI when compared to overall possibilities on the market, the check is non-blocking.
Second topic relates to stream setup procedure. The procedure differs between HDAudio controller device revisions. Right now those differences are handled directly by a platform driver. Existing top-level 'if (pci->device == APL)' could be replaced by a abstraction in lower parts of the code instead.
With that done, the two users are updated accordingly. In avs-driver case, this updates the flow to the recommended one.
Changes in v3:
- fixed issues pointed out by scripts/kernel-doc
Changes in v2:
- fixed ->host_setup assignment in patch 02/04
Cezary Rojewski (4): ALSA: hda: Poll SDxFIFOS after programming SDxFMT ALSA: hda: Introduce HOST stream setup mechanism ASoC: Intel: avs: Use helper to setup HOST stream ASoC: Intel: Skylake: Use helper to setup HOST stream
Sorry for the late reaction, as I've been (still) off since the last week.
Now applied now to for-next branch.
Hello Takashi,
Now I'm the one sorry for the late replay - I've found two new things when fixing the debug message (reported by me in patch 1/4). - null-ptr-deref when assigning hdac_stream (when type=COUPLED) - azx_dev->fifo_size is initialized incorrectly
It's good to debug things, you never know what you may find!
May I send v4 as a collective update? It would address the two above and the false-positive debug message that appears when code-loading AudioDSP firmware.
Kind regards, Czarek
On Fri, 06 Oct 2023 11:01:44 +0200, Cezary Rojewski wrote:
On 2023-10-06 10:44 AM, Takashi Iwai wrote:
On Tue, 26 Sep 2023 10:06:19 +0200, Cezary Rojewski wrote:
The patchset targets two intertwined topics:
The driver shall poll SDxFIFOS to ensure a valid value is set by the controller after programming SDxFMT. Due to amount of users and limited-number of configuration available in our CI when compared to overall possibilities on the market, the check is non-blocking.
Second topic relates to stream setup procedure. The procedure differs between HDAudio controller device revisions. Right now those differences are handled directly by a platform driver. Existing top-level 'if (pci->device == APL)' could be replaced by a abstraction in lower parts of the code instead.
With that done, the two users are updated accordingly. In avs-driver case, this updates the flow to the recommended one.
Changes in v3:
- fixed issues pointed out by scripts/kernel-doc
Changes in v2:
- fixed ->host_setup assignment in patch 02/04
Cezary Rojewski (4): ALSA: hda: Poll SDxFIFOS after programming SDxFMT ALSA: hda: Introduce HOST stream setup mechanism ASoC: Intel: avs: Use helper to setup HOST stream ASoC: Intel: Skylake: Use helper to setup HOST stream
Sorry for the late reaction, as I've been (still) off since the last week.
Now applied now to for-next branch.
Hello Takashi,
Now I'm the one sorry for the late replay - I've found two new things when fixing the debug message (reported by me in patch 1/4).
- null-ptr-deref when assigning hdac_stream (when type=COUPLED)
- azx_dev->fifo_size is initialized incorrectly
It's good to debug things, you never know what you may find!
May I send v4 as a collective update? It would address the two above and the false-positive debug message that appears when code-loading AudioDSP firmware.
As other patches have been already applied, could you submit the incremental fixes on top of v3 instead? You can see the latest state in for-next branch of sound.git tree.
thanks,
Takashi
participants (2)
-
Cezary Rojewski -
Takashi Iwai