[PATCH 0/7] sound: Use -EPROBE_DEFER instead of i915 module loading.
Explicitly loading i915 becomes a problem when upstreaming the new intel driver for Tiger Lake and higher graphics (xe). By loading i915, it doesn't wait for driver load of xe, and will fail completely before it loads.
-EPROBE_DEFER has to be returned before any device is created in probe(), otherwise the removal of the device will cause EPROBE_DEFER to try again in an infinite loop.
The conversion is done in gradual steps. First I add an argument to snd_hdac_i915_init to allow for -EPROBE_DEFER so I can convert each driver separately. Then I convert each driver to move snd_hdac_i915_init out of the workqueue. Finally I drop the ability to choose modprobe behavior after the last user is converted.
I suspect the avs and skylake drivers used snd_hdac_i915_init purely for the modprobe, but I don't have the hardware to test if it can be safely removed. It can still be done easily in a followup patch to simplify probing.
Maarten Lankhorst (7): ALSA: hda/i915: Add an allow_modprobe argument to snd_hdac_i915_init ALSA: hda/i915: Allow xe as match for i915_component_master_match ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work. ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work. ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work. ASoC: SOF: Intel: Remove deferred probe for SOF ALSA: hda/i915: Remove extra argument from snd_hdac_i915_init
Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Cezary Rojewski cezary.rojewski@intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Liam Girdwood liam.r.girdwood@linux.intel.com Cc: Peter Ujfalusi peter.ujfalusi@linux.intel.com Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Ranjani Sridharan ranjani.sridharan@linux.intel.com Cc: Kai Vehmanen kai.vehmanen@linux.intel.com Cc: Mark Brown broonie@kernel.org Cc: Daniel Baluta daniel.baluta@nxp.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Cc: sound-open-firmware@alsa-project.org
sound/hda/hdac_i915.c | 15 +++------ sound/pci/hda/hda_intel.c | 58 +++++++++++++++++++---------------- sound/soc/intel/avs/core.c | 13 +++++--- sound/soc/intel/skylake/skl.c | 31 ++++++------------- sound/soc/sof/Kconfig | 19 ------------ sound/soc/sof/core.c | 38 ++--------------------- sound/soc/sof/intel/Kconfig | 1 - sound/soc/sof/intel/hda.c | 32 +++++++++++-------- sound/soc/sof/sof-pci-dev.c | 3 +- sound/soc/sof/sof-priv.h | 5 --- 10 files changed, 75 insertions(+), 140 deletions(-)
Xe is a new GPU driver, that re-uses the display (and sound) code from i915. It's no longer possible to load i915, as the GPU can be driven by the xe driver instead.
The new behavior will return -EPROBE_DEFER, and wait for a compatible driver to be loaded instead of modprobing i915.
Converting all drivers at the same time is a lot of work, instead we will convert each user one by one.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- include/sound/hda_i915.h | 4 ++-- sound/hda/hdac_i915.c | 11 +++++++---- sound/pci/hda/hda_intel.c | 2 +- sound/soc/intel/avs/core.c | 2 +- sound/soc/intel/skylake/skl.c | 2 +- sound/soc/sof/intel/hda-codec.c | 2 +- 6 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index 6b79614a893b9..f91bd66360865 100644 --- a/include/sound/hda_i915.h +++ b/include/sound/hda_i915.h @@ -9,12 +9,12 @@
#ifdef CONFIG_SND_HDA_I915 void snd_hdac_i915_set_bclk(struct hdac_bus *bus); -int snd_hdac_i915_init(struct hdac_bus *bus); +int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe); #else static inline void snd_hdac_i915_set_bclk(struct hdac_bus *bus) { } -static inline int snd_hdac_i915_init(struct hdac_bus *bus) +static inline int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe) { return -ENODEV; } diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 161a9711cd63e..12f93008ad361 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -145,7 +145,7 @@ static int i915_gfx_present(struct pci_dev *hdac_pci) * * Returns zero for success or a negative error code. */ -int snd_hdac_i915_init(struct hdac_bus *bus) +int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe) { struct drm_audio_component *acomp; int err; @@ -161,7 +161,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus) acomp = bus->audio_component; if (!acomp) return -ENODEV; - if (!acomp->ops) { + if (allow_modprobe && !acomp->ops) { if (!IS_ENABLED(CONFIG_MODULES) || !request_module("i915")) { /* 60s timeout */ @@ -170,9 +170,12 @@ int snd_hdac_i915_init(struct hdac_bus *bus) } } if (!acomp->ops) { - dev_info(bus->dev, "couldn't bind with audio component\n"); + if (allow_modprobe) + dev_info(bus->dev, "couldn't bind with audio component\n"); + else + dev_dbg(bus->dev, "couldn't bind with audio component\n"); snd_hdac_acomp_exit(bus); - return -ENODEV; + return allow_modprobe ? -ENODEV : -EPROBE_DEFER; } return 0; } diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index ef831770ca7da..5af1138e745bc 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2276,7 +2276,7 @@ static int azx_probe_continue(struct azx *chip)
/* bind with i915 if needed */ if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { - err = snd_hdac_i915_init(bus); + err = snd_hdac_i915_init(bus, true); if (err < 0) { /* if the controller is bound only with HDMI/DP * (for HSW and BDW), we need to abort the probe; diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index 6375018507288..3311a6f142001 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -191,7 +191,7 @@ static void avs_hda_probe_work(struct work_struct *work)
pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
- ret = snd_hdac_i915_init(bus); + ret = snd_hdac_i915_init(bus, true); if (ret < 0) dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 998bd0232cf1d..4d93b86904673 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -791,7 +791,7 @@ static int skl_i915_init(struct hdac_bus *bus) * The HDMI codec is in GPU so we need to ensure that it is powered * up and ready for probe */ - err = snd_hdac_i915_init(bus); + err = snd_hdac_i915_init(bus, true); if (err < 0) return err;
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 8a5e99a898ecb..f1fd5b44aaac9 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0;
/* i915 exposes a HDA codec for HDMI audio */ - ret = snd_hdac_i915_init(bus); + ret = snd_hdac_i915_init(bus, true); if (ret < 0) return ret;
xe is a new driver for intel GPU's that shares the sound related code with i915.
Don't allow it to be modprobed though; the module is not upstream yet and we should exclusively use the EPROBE_DEFER mechanism.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- sound/hda/hdac_i915.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 12f93008ad361..c88f251388e80 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -108,7 +108,8 @@ static int i915_component_master_match(struct device *dev, int subcomponent, hdac_pci = to_pci_dev(bus->dev); i915_pci = to_pci_dev(dev);
- if (!strcmp(dev->driver->name, "i915") && + if ((!strcmp(dev->driver->name, "i915") || + !strcmp(dev->driver->name, "xe")) && subcomponent == I915_COMPONENT_AUDIO && connectivity_check(i915_pci, hdac_pci)) return 1;
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.
Removing the workqueue would simplify init even further, but is left as exercise for the reviewer.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- sound/soc/intel/avs/core.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index 3311a6f142001..d3a7f42387e9b 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work)
pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
- ret = snd_hdac_i915_init(bus, true); - if (ret < 0) - dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret); - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); avs_hdac_bus_init_chip(bus, true); avs_hdac_bus_probe_codecs(bus); @@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) pci_set_drvdata(pci, bus); device_disable_async_suspend(dev);
+ ret = snd_hdac_i915_init(bus, false); + if (ret == -EPROBE_DEFER) + goto err_unmaster; + else if (ret < 0) + dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret); + schedule_work(&adev->probe_work);
return 0;
+err_unmaster: + pci_clear_master(pci); + pci_set_drvdata(pci, NULL); err_acquire_irq: snd_hdac_bus_free_stream_pages(bus); snd_hdac_ext_stream_free_all(bus);
On Tue, Jul 18, 2023 at 10:45:18AM +0200, Maarten Lankhorst wrote:
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.
Acked-by: Mark Brown broonie@kernel.org
On 2023-07-18 10:45 AM, Maarten Lankhorst wrote:
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.
Removing the workqueue would simplify init even further, but is left as exercise for the reviewer.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
sound/soc/intel/avs/core.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index 3311a6f142001..d3a7f42387e9b 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work)
pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
- ret = snd_hdac_i915_init(bus, true);
- if (ret < 0)
dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);- snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); avs_hdac_bus_init_chip(bus, true); avs_hdac_bus_probe_codecs(bus);
@@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) pci_set_drvdata(pci, bus); device_disable_async_suspend(dev);
- ret = snd_hdac_i915_init(bus, false);
- if (ret == -EPROBE_DEFER)
goto err_unmaster;- else if (ret < 0)
dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
While our tests are currently passing I have my doubts about EPROBE_DEFER. We do want to have audio functionality there even if some problems with HDMI arise along the way - some audio is better than no audio. Here, i915 may ruin the day for a platform equipped with hda/hdmi/i2c/dmic chips simultaneously.
Also, why call snd_hdac_i915_init() _after_ setting drvdata?
schedule_work(&adev->probe_work);
return 0;
+err_unmaster:
- pci_clear_master(pci);
- pci_set_drvdata(pci, NULL);
Not a fan. It's hard to grasp entire error-step within name of a single label. Thus I'd suggest "err_<cause>" naming pattern instead. Even here, under "unmaster" you hid clearing master and drvdata both. Let's do "err_i915_init" instead.
err_acquire_irq: snd_hdac_bus_free_stream_pages(bus); snd_hdac_ext_stream_free_all(bus);
On Wed, 19 Jul 2023 17:26:24 +0200, Cezary Rojewski wrote:
On 2023-07-18 10:45 AM, Maarten Lankhorst wrote:
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.
Removing the workqueue would simplify init even further, but is left as exercise for the reviewer.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
sound/soc/intel/avs/core.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index 3311a6f142001..d3a7f42387e9b 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work) pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
- ret = snd_hdac_i915_init(bus, true);
- if (ret < 0)
dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);- snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); avs_hdac_bus_init_chip(bus, true); avs_hdac_bus_probe_codecs(bus);
@@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) pci_set_drvdata(pci, bus); device_disable_async_suspend(dev);
- ret = snd_hdac_i915_init(bus, false);
- if (ret == -EPROBE_DEFER)
goto err_unmaster;- else if (ret < 0)
dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);While our tests are currently passing I have my doubts about EPROBE_DEFER. We do want to have audio functionality there even if some problems with HDMI arise along the way - some audio is better than no audio. Here, i915 may ruin the day for a platform equipped with hda/hdmi/i2c/dmic chips simultaneously.
Yeah, we need some workaround to let user to skip the i915 binding, as discussed in the thread. I guess an option for hdac_i915 (that will be in snd-hda-core module) should suffice.
thanks,
Takashi
Hey,
On 2023-07-19 17:26, Cezary Rojewski wrote:
On 2023-07-18 10:45 AM, Maarten Lankhorst wrote:
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.
Removing the workqueue would simplify init even further, but is left as exercise for the reviewer.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
sound/soc/intel/avs/core.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index 3311a6f142001..d3a7f42387e9b 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work) pm_runtime_set_active(bus->dev); /* clear runtime_error flag */ - ret = snd_hdac_i915_init(bus, true); - if (ret < 0) - dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); avs_hdac_bus_init_chip(bus, true); avs_hdac_bus_probe_codecs(bus); @@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) pci_set_drvdata(pci, bus); device_disable_async_suspend(dev); + ret = snd_hdac_i915_init(bus, false); + if (ret == -EPROBE_DEFER) + goto err_unmaster; + else if (ret < 0) + dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
While our tests are currently passing I have my doubts about EPROBE_DEFER. We do want to have audio functionality there even if some problems with HDMI arise along the way - some audio is better than no audio. Here, i915 may ruin the day for a platform equipped with hda/hdmi/i2c/dmic chips simultaneously.
Also, why call snd_hdac_i915_init() _after_ setting drvdata?
I wanted to make the most cautious change. The previous flow called the function immediately from probe_work, so I moved it to right before the scheduling of probe_work
schedule_work(&adev->probe_work); return 0; +err_unmaster: + pci_clear_master(pci); + pci_set_drvdata(pci, NULL);
Not a fan. It's hard to grasp entire error-step within name of a single label. Thus I'd suggest "err_<cause>" naming pattern instead. Even here, under "unmaster" you hid clearing master and drvdata both. Let's do "err_i915_init" instead.
Ok, will rename.
~Maarten
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.
Removing the workqueue would simplify init even further, but is left as exercise for the reviewer.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- sound/soc/intel/skylake/skl.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 4d93b86904673..ff80d83a9fb72 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -783,23 +783,6 @@ static void skl_codec_create(struct hdac_bus *bus) } }
-static int skl_i915_init(struct hdac_bus *bus) -{ - int err; - - /* - * The HDMI codec is in GPU so we need to ensure that it is powered - * up and ready for probe - */ - err = snd_hdac_i915_init(bus, true); - if (err < 0) - return err; - - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); - - return 0; -} - static void skl_probe_work(struct work_struct *work) { struct skl_dev *skl = container_of(work, struct skl_dev, probe_work); @@ -807,11 +790,8 @@ static void skl_probe_work(struct work_struct *work) struct hdac_ext_link *hlink; int err;
- if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { - err = skl_i915_init(bus); - if (err < 0) - return; - } + if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) + snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
skl_init_pci(skl); skl_dum_set(bus); @@ -1075,10 +1055,17 @@ static int skl_probe(struct pci_dev *pci, goto out_dsp_free; }
+ if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { + err = snd_hdac_i915_init(bus, false); + if (err < 0) + goto out_dmic_unregister; + } schedule_work(&skl->probe_work);
return 0;
+out_dmic_unregister: + skl_dmic_device_unregister(skl); out_dsp_free: skl_free_dsp(skl); out_clk_free:
On Tue, Jul 18, 2023 at 10:45:19AM +0200, Maarten Lankhorst wrote:
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.
Acked-by: Mark Brown broonie@kernel.org
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue.
Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- sound/pci/hda/hda_intel.c | 58 +++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 5af1138e745bc..d40345a0088d8 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -213,6 +213,7 @@ MODULE_DESCRIPTION("Intel HDA driver"); #endif #endif
+static DECLARE_BITMAP(probed_devs, SNDRV_CARDS);
/* */ @@ -2094,8 +2095,6 @@ static const struct hda_controller_ops pci_hda_ops = { .position_check = azx_position_check, };
-static DECLARE_BITMAP(probed_devs, SNDRV_CARDS); - static int azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) { @@ -2174,7 +2173,36 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */
-#ifndef CONFIG_SND_HDA_I915 +#ifdef CONFIG_SND_HDA_I915 + /* bind with i915 if needed */ + if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { + err = snd_hdac_i915_init(azx_bus(chip), false); + if (err < 0) { + /* if the controller is bound only with HDMI/DP + * (for HSW and BDW), we need to abort the probe; + * for other chips, still continue probing as other + * codecs can be on the same link. + */ + if (CONTROLLER_IN_GPU(pci)) { + if (err != -EPROBE_DEFER) + dev_err(card->dev, + "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); + + clear_bit(chip->dev_index, probed_devs); + pci_set_drvdata(pci, NULL); + snd_device_free(card, chip); + return err; + } else { + /* don't bother any longer */ + chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; + } + } + + /* HSW/BDW controllers need this power */ + if (CONTROLLER_IN_GPU(pci)) + hda->need_i915_power = true; + } +#else if (CONTROLLER_IN_GPU(pci)) dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n"); #endif @@ -2274,30 +2302,6 @@ static int azx_probe_continue(struct azx *chip) to_hda_bus(bus)->bus_probing = 1; hda->probe_continued = 1;
- /* bind with i915 if needed */ - if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { - err = snd_hdac_i915_init(bus, true); - if (err < 0) { - /* if the controller is bound only with HDMI/DP - * (for HSW and BDW), we need to abort the probe; - * for other chips, still continue probing as other - * codecs can be on the same link. - */ - if (CONTROLLER_IN_GPU(pci)) { - dev_err(chip->card->dev, - "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); - goto out_free; - } else { - /* don't bother any longer */ - chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; - } - } - - /* HSW/BDW controllers need this power */ - if (CONTROLLER_IN_GPU(pci)) - hda->need_i915_power = true; - } - /* Request display power well for the HDA controller or codec. For * Haswell/Broadwell, both the display HDA controller and codec need * this power. For other platforms, like Baytrail/Braswell, only the
On Tue, 18 Jul 2023 10:45:20 +0200, Maarten Lankhorst wrote:
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue.
Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
sound/pci/hda/hda_intel.c | 58 +++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 5af1138e745bc..d40345a0088d8 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -213,6 +213,7 @@ MODULE_DESCRIPTION("Intel HDA driver"); #endif #endif
+static DECLARE_BITMAP(probed_devs, SNDRV_CARDS);
/* */ @@ -2094,8 +2095,6 @@ static const struct hda_controller_ops pci_hda_ops = { .position_check = azx_position_check, };
-static DECLARE_BITMAP(probed_devs, SNDRV_CARDS);
static int azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) {
Any specific reason to move the definition? Otherwise let's concentrate on the needed change.
@@ -2174,7 +2173,36 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */
-#ifndef CONFIG_SND_HDA_I915 +#ifdef CONFIG_SND_HDA_I915
- /* bind with i915 if needed */
- if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
err = snd_hdac_i915_init(azx_bus(chip), false);if (err < 0) {/* if the controller is bound only with HDMI/DP* (for HSW and BDW), we need to abort the probe;* for other chips, still continue probing as other* codecs can be on the same link.*/if (CONTROLLER_IN_GPU(pci)) {if (err != -EPROBE_DEFER)dev_err(card->dev,"HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n");clear_bit(chip->dev_index, probed_devs);pci_set_drvdata(pci, NULL);snd_device_free(card, chip);return err;
This may leak resources, I'm afraid.
Here you just need to "goto out_free;" instead of manual resource releases, which eventually calls snd_card_free(), and that's all.
(Though, pci_set_drvdata(pci, NULL) might be still missing; but it's not only for this change, and we'll need to address it in another patch.)
thanks,
Takashi
Hey,
On 2023-07-18 11:57, Takashi Iwai wrote:
On Tue, 18 Jul 2023 10:45:20 +0200, Maarten Lankhorst wrote:
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue.
Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function.
Signed-off-by: Maarten Lankhorstmaarten.lankhorst@linux.intel.com
sound/pci/hda/hda_intel.c | 58 +++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 5af1138e745bc..d40345a0088d8 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -213,6 +213,7 @@ MODULE_DESCRIPTION("Intel HDA driver"); #endif #endif
+static DECLARE_BITMAP(probed_devs, SNDRV_CARDS);
/* */ @@ -2094,8 +2095,6 @@ static const struct hda_controller_ops pci_hda_ops = { .position_check = azx_position_check, };
-static DECLARE_BITMAP(probed_devs, SNDRV_CARDS);
- static int azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) {
Any specific reason to move the definition? Otherwise let's concentrate on the needed change.
Originally I moved the chunk to azx_create(), but I felt it was the wrong place. Fixed now.
@@ -2174,7 +2173,36 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */
-#ifndef CONFIG_SND_HDA_I915 +#ifdef CONFIG_SND_HDA_I915
- /* bind with i915 if needed */
- if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
err = snd_hdac_i915_init(azx_bus(chip), false);if (err < 0) {/* if the controller is bound only with HDMI/DP* (for HSW and BDW), we need to abort the probe;* for other chips, still continue probing as other* codecs can be on the same link.*/if (CONTROLLER_IN_GPU(pci)) {if (err != -EPROBE_DEFER)dev_err(card->dev,"HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n");clear_bit(chip->dev_index, probed_devs);pci_set_drvdata(pci, NULL);snd_device_free(card, chip);return err;This may leak resources, I'm afraid.
Here you just need to "goto out_free;" instead of manual resource releases, which eventually calls snd_card_free(), and that's all.
(Though, pci_set_drvdata(pci, NULL) might be still missing; but it's not only for this change, and we'll need to address it in another patch.)
git am --scissors patch below for cleanup of error path.
It seems azx_free will unregister from vga_switcheroo safely.
------------>8---
Make sure azx is freed after azx_create() succeeded and an error was encountered.
Signed-off-by: Maarten Lankhorstmaarten.lankhorst@linux.intel.com --- sound/pci/hda/hda_intel.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 5af1138e745bc..196ca76ac43ad 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2150,7 +2150,7 @@ static int azx_probe(struct pci_dev *pci, err = register_vga_switcheroo(chip); if (err < 0) { dev_err(card->dev, "Error registering vga_switcheroo client\n"); - goto out_free; + goto out_azx_free; }
if (check_hdmi_disabled(pci)) { @@ -2169,7 +2169,7 @@ static int azx_probe(struct pci_dev *pci, &pci->dev, GFP_KERNEL, card, azx_firmware_cb); if (err < 0) - goto out_free; + goto out_azx_free; schedule_probe = false; /* continued in azx_firmware_cb() */ } #endif /* CONFIG_SND_HDA_PATCH_LOADER */ @@ -2187,6 +2187,9 @@ static int azx_probe(struct pci_dev *pci, complete_all(&hda->probe_wait); return 0;
+out_azx_free: + azx_free(chip); + pci_set_drvdata(pci, NULL); out_free: snd_card_free(card); return err;
On Tue, 18 Jul 2023 13:57:33 +0200, Maarten Lankhorst wrote:
Make sure azx is freed after azx_create() succeeded and an error was encountered.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
sound/pci/hda/hda_intel.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 5af1138e745bc..196ca76ac43ad 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2150,7 +2150,7 @@ static int azx_probe(struct pci_dev *pci, err = register_vga_switcheroo(chip); if (err < 0) { dev_err(card->dev, "Error registering vga_switcheroo client\n");
goto out_free;
goto out_azx_free; } if (check_hdmi_disabled(pci)) {@@ -2169,7 +2169,7 @@ static int azx_probe(struct pci_dev *pci, &pci->dev, GFP_KERNEL, card, azx_firmware_cb); if (err < 0)
goto out_free;
goto out_azx_free; schedule_probe = false; /* continued in azx_firmware_cb() */ }#endif /* CONFIG_SND_HDA_PATCH_LOADER */ @@ -2187,6 +2187,9 @@ static int azx_probe(struct pci_dev *pci, complete_all(&hda->probe_wait); return 0;
+out_azx_free:
azx_free(chip);
This is superfluous. Once when azx_create() succeeds, azx_free() is called from snd_card_free(). That is...
pci_set_drvdata(pci, NULL);
... only this call was missing. And this can be put under out_free label, as this is safe to call at any exit path. So, it'll be a oneliner patch.
thanks,
Takashi
out_free: snd_card_free(card); return err; -- 2.39.2
Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue.
Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function.
Changes since v1: - Use dev_err_probe() - Don't move probed_devs bitmap unnecessarily. (tiwai) - Use goto out_azx_free after cleanup of error path. (tiwai) - Move snd_hdac_i915_init slightly upward, to ensure it's always initialised before vga-switcheroo is called.
Signed-off-by: Maarten Lankhorstmaarten.lankhorst@linux.intel.com --- sound/pci/hda/hda_intel.c | 60 ++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 29 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 196ca76ac43ad..59039a1f01035 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2147,6 +2147,36 @@ static int azx_probe(struct pci_dev *pci,
pci_set_drvdata(pci, card);
+#ifdef CONFIG_SND_HDA_I915 + /* bind with i915 if needed */ + if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { + err = snd_hdac_i915_init(azx_bus(chip), false); + if (err < 0) { + /* if the controller is bound only with HDMI/DP + * (for HSW and BDW), we need to abort the probe; + * for other chips, still continue probing as other + * codecs can be on the same link. + */ + if (CONTROLLER_IN_GPU(pci)) { + dev_err_probe(card->dev, err, + "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); + + goto out_azx_free; + } else { + /* don't bother any longer */ + chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; + } + } + + /* HSW/BDW controllers need this power */ + if (CONTROLLER_IN_GPU(pci)) + hda->need_i915_power = true; + } +#else + if (CONTROLLER_IN_GPU(pci)) + dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n"); +#endif + err = register_vga_switcheroo(chip); if (err < 0) { dev_err(card->dev, "Error registering vga_switcheroo client\n"); @@ -2174,11 +2205,6 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */
-#ifndef CONFIG_SND_HDA_I915 - if (CONTROLLER_IN_GPU(pci)) - dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n"); -#endif - if (schedule_probe) schedule_delayed_work(&hda->probe_work, 0);
@@ -2277,30 +2303,6 @@ static int azx_probe_continue(struct azx *chip) to_hda_bus(bus)->bus_probing = 1; hda->probe_continued = 1;
- /* bind with i915 if needed */ - if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { - err = snd_hdac_i915_init(bus, true); - if (err < 0) { - /* if the controller is bound only with HDMI/DP - * (for HSW and BDW), we need to abort the probe; - * for other chips, still continue probing as other - * codecs can be on the same link. - */ - if (CONTROLLER_IN_GPU(pci)) { - dev_err(chip->card->dev, - "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); - goto out_free; - } else { - /* don't bother any longer */ - chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; - } - } - - /* HSW/BDW controllers need this power */ - if (CONTROLLER_IN_GPU(pci)) - hda->need_i915_power = true; - } - /* Request display power well for the HDA controller or codec. For * Haswell/Broadwell, both the display HDA controller and codec need * this power. For other platforms, like Baytrail/Braswell, only the
This was only used to allow modprobing i915, by converting to the -EPROBE_DEFER mechanism, it can be completely removed, and is in fact counterproductive since -EPROBE_DEFER otherwise won't be handled correctly.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Matthew Auld matthew.auld@intel.com --- sound/soc/sof/Kconfig | 19 ----------------- sound/soc/sof/core.c | 38 ++------------------------------- sound/soc/sof/intel/Kconfig | 1 - sound/soc/sof/intel/hda-codec.c | 2 +- sound/soc/sof/intel/hda.c | 32 ++++++++++++++++----------- sound/soc/sof/sof-pci-dev.c | 3 +-- sound/soc/sof/sof-priv.h | 5 ----- 7 files changed, 23 insertions(+), 77 deletions(-)
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index 80361139a49ad..8ee39e5558062 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -82,17 +82,6 @@ config SND_SOC_SOF_DEVELOPER_SUPPORT
if SND_SOC_SOF_DEVELOPER_SUPPORT
-config SND_SOC_SOF_FORCE_PROBE_WORKQUEUE - bool "SOF force probe workqueue" - select SND_SOC_SOF_PROBE_WORK_QUEUE - help - This option forces the use of a probe workqueue, which is only used - when HDaudio is enabled due to module dependencies. Forcing this - option is intended for debug only, but this should not add any - functional issues in nominal cases. - Say Y if you are involved in SOF development and need this option. - If not, select N. - config SND_SOC_SOF_NOCODEC tristate
@@ -271,14 +260,6 @@ config SND_SOC_SOF module dependencies but since the module or built-in type is decided at the top level it doesn't matter.
-config SND_SOC_SOF_PROBE_WORK_QUEUE - bool - help - This option is not user-selectable but automagically handled by - 'select' statements at a higher level. - When selected, the probe is handled in two steps, for example to - avoid lockdeps if request_module is used in the probe. - # Supported IPC versions config SND_SOC_SOF_IPC3 bool diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..cdf86dc4a8a87 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -191,7 +191,8 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) /* probe the DSP hardware */ ret = snd_sof_probe(sdev); if (ret < 0) { - dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret); + if (ret != -EPROBE_DEFER) + dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret); goto probe_err; }
@@ -309,8 +310,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) if (plat_data->sof_probe_complete) plat_data->sof_probe_complete(sdev->dev);
- sdev->probe_completed = true; - return 0;
sof_machine_err: @@ -336,19 +335,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) return ret; }
-static void sof_probe_work(struct work_struct *work) -{ - struct snd_sof_dev *sdev = - container_of(work, struct snd_sof_dev, probe_work); - int ret; - - ret = sof_probe_continue(sdev); - if (ret < 0) { - /* errors cannot be propagated, log */ - dev_err(sdev->dev, "error: %s failed err: %d\n", __func__, ret); - } -} - int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data) { struct snd_sof_dev *sdev; @@ -436,33 +422,16 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);
- if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) { - INIT_WORK(&sdev->probe_work, sof_probe_work); - schedule_work(&sdev->probe_work); - return 0; - } - return sof_probe_continue(sdev); } EXPORT_SYMBOL(snd_sof_device_probe);
-bool snd_sof_device_probe_completed(struct device *dev) -{ - struct snd_sof_dev *sdev = dev_get_drvdata(dev); - - return sdev->probe_completed; -} -EXPORT_SYMBOL(snd_sof_device_probe_completed); - int snd_sof_device_remove(struct device *dev) { struct snd_sof_dev *sdev = dev_get_drvdata(dev); struct snd_sof_pdata *pdata = sdev->pdata; int ret;
- if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) - cancel_work_sync(&sdev->probe_work); - /* * Unregister any registered client device first before IPC and debugfs * to allow client drivers to be removed cleanly @@ -501,9 +470,6 @@ int snd_sof_device_shutdown(struct device *dev) { struct snd_sof_dev *sdev = dev_get_drvdata(dev);
- if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) - cancel_work_sync(&sdev->probe_work); - if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) { sof_fw_trace_free(sdev); return snd_sof_shutdown(sdev); diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 69c1a370d3b61..d9e87a91670a3 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -293,7 +293,6 @@ config SND_SOC_SOF_HDA_LINK config SND_SOC_SOF_HDA_AUDIO_CODEC bool "SOF support for HDAudio codecs" depends on SND_SOC_SOF_HDA_LINK - select SND_SOC_SOF_PROBE_WORK_QUEUE help This adds support for HDAudio codecs with Sound Open Firmware for Intel(R) platforms. diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index f1fd5b44aaac9..344b61576c0e3 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0;
/* i915 exposes a HDA codec for HDMI audio */ - ret = snd_hdac_i915_init(bus, true); + ret = snd_hdac_i915_init(bus, false); if (ret < 0) return ret;
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 64bebe1a72bbc..a8b7a68142c05 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -801,8 +801,11 @@ static int hda_init(struct snd_sof_dev *sdev)
/* init i915 and HDMI codecs */ ret = hda_codec_i915_init(sdev); - if (ret < 0) - dev_warn(sdev->dev, "init of i915 and HDMI codec failed\n"); + if (ret < 0) { + if (ret != -EPROBE_DEFER) + dev_warn(sdev->dev, "init of i915 and HDMI codec failed: %i\n", ret); + return ret; + }
/* get controller capabilities */ ret = hda_dsp_ctrl_get_caps(sdev); @@ -1115,14 +1118,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) sdev->pdata->hw_pdata = hdev; hdev->desc = chip;
- hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", - PLATFORM_DEVID_NONE, - NULL, 0); - if (IS_ERR(hdev->dmic_dev)) { - dev_err(sdev->dev, "error: failed to create DMIC device\n"); - return PTR_ERR(hdev->dmic_dev); - } - /* * use position update IPC if either it is forced * or we don't have other choice @@ -1142,6 +1137,15 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) if (ret < 0) goto hdac_bus_unmap;
+ hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", + PLATFORM_DEVID_NONE, + NULL, 0); + if (IS_ERR(hdev->dmic_dev)) { + dev_err(sdev->dev, "error: failed to create DMIC device\n"); + ret = PTR_ERR(hdev->dmic_dev); + goto hdac_exit; + } + if (sdev->dspless_mode_selected) goto skip_dsp_setup;
@@ -1150,7 +1154,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) if (!sdev->bar[HDA_DSP_BAR]) { dev_err(sdev->dev, "error: ioremap error\n"); ret = -ENXIO; - goto hdac_bus_unmap; + goto platform_unreg; }
sdev->mmio_bar = HDA_DSP_BAR; @@ -1248,10 +1252,12 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) /* dsp_unmap: not currently used */ if (!sdev->dspless_mode_selected) iounmap(sdev->bar[HDA_DSP_BAR]); -hdac_bus_unmap: +platform_unreg: platform_device_unregister(hdev->dmic_dev); - iounmap(bus->remap_addr); +hdac_exit: hda_codec_i915_exit(sdev); +hdac_bus_unmap: + iounmap(bus->remap_addr); err: return ret; } diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index f5ece43d0ec24..0fa424613082e 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -339,8 +339,7 @@ void sof_pci_remove(struct pci_dev *pci) snd_sof_device_remove(&pci->dev);
/* follow recommendation in pci-driver.c to increment usage counter */ - if (snd_sof_device_probe_completed(&pci->dev) && - !(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) + if (!(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) pm_runtime_get_noresume(&pci->dev);
/* release pci regions and disable device */ diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index d4f6702e93dcb..71db636cfdccc 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -564,10 +564,6 @@ struct snd_sof_dev { enum sof_fw_state fw_state; bool first_boot;
- /* work queue in case the probe is implemented in two steps */ - struct work_struct probe_work; - bool probe_completed; - /* DSP HW differentiation */ struct snd_sof_pdata *pdata;
@@ -675,7 +671,6 @@ struct snd_sof_dev { int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data); int snd_sof_device_remove(struct device *dev); int snd_sof_device_shutdown(struct device *dev); -bool snd_sof_device_probe_completed(struct device *dev);
int snd_sof_runtime_suspend(struct device *dev); int snd_sof_runtime_resume(struct device *dev);
On Tue, Jul 18, 2023 at 10:45:21AM +0200, Maarten Lankhorst wrote:
This was only used to allow modprobing i915, by converting to the -EPROBE_DEFER mechanism, it can be completely removed, and is in fact counterproductive since -EPROBE_DEFER otherwise won't be handled correctly.
Acked-by: Mark Brown broonie@kernel.org
Hi,
thank you Maarten for doing the series! I think a lot of people will be happy to get rid of the 60sec timeout.
I kicked off a CI run SOF public infra for the whole series at https://github.com/thesofproject/linux/pull/4478 Some results still in progress but so far so good.
Some concerns inline:
On Tue, 18 Jul 2023, Maarten Lankhorst wrote:
This was only used to allow modprobing i915, by converting to the -EPROBE_DEFER mechanism, it can be completely removed, and is in fact counterproductive since -EPROBE_DEFER otherwise won't be handled correctly.
We actually have a request_module() for the regular HDA codec drivers as well (sof_probe_continue() -> snd_sof_probe() -> hda_dsp_probe() -> hda_init_caps() -> hda_codec_probe_bus(). But right, this is not necessarily a problem on its own, so it looks we indeed can drop the work queue. Nice!
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index f1fd5b44aaac9..344b61576c0e3 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0;
/* i915 exposes a HDA codec for HDMI audio */
- ret = snd_hdac_i915_init(bus, true);
- ret = snd_hdac_i915_init(bus, false); if (ret < 0) return ret;
My only bigger concern is corner cases where the display PCI device is on the bus and visible to kernel, but for some reason there is no working driver in the system or it is disabled.
With this patch, not having a workign display driver means that there is also no audio in the system as the SOF driver will never get probed.
In current mainline, one will get the 60sec timeout warning and then audio driver will proceed to probe and you'll have audio support (minus HDMI/DP).
This is mostly an issue with very new hardware (e.g. hw is still behind force_probe flag in xe/i915 driver), but we've had some odd cases with e.g. systems with both Intel IGFX and other vendors' DGPU. Audio drivers see the Intel VGA controller in system and will call snd_hdac_i915_init(), but the audio component bind will never succeed if the the Intel IGFX is not in actual use.
Will need a bit of time to think about possible scenarios. Possibly this is not an issue outside early development systems. In theory if IGFX is disabled in BIOS, and not visible to OS, we are good, and if it's visible, the i915/xe driver should be loaded, so we are good again.
Br, Kai
On Tue, 18 Jul 2023 19:04:41 +0200, Kai Vehmanen wrote:
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index f1fd5b44aaac9..344b61576c0e3 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0;
/* i915 exposes a HDA codec for HDMI audio */
- ret = snd_hdac_i915_init(bus, true);
- ret = snd_hdac_i915_init(bus, false); if (ret < 0) return ret;
My only bigger concern is corner cases where the display PCI device is on the bus and visible to kernel, but for some reason there is no working driver in the system or it is disabled.
With this patch, not having a workign display driver means that there is also no audio in the system as the SOF driver will never get probed.
In current mainline, one will get the 60sec timeout warning and then audio driver will proceed to probe and you'll have audio support (minus HDMI/DP).
Yeah, that was a concern in the past, too. e.g. when you pass "nomodeset" boot option, the driver will become unusable, even if the bus is used generically for both analog and HDMI codecs.
This is mostly an issue with very new hardware (e.g. hw is still behind force_probe flag in xe/i915 driver), but we've had some odd cases with e.g. systems with both Intel IGFX and other vendors' DGPU. Audio drivers see the Intel VGA controller in system and will call snd_hdac_i915_init(), but the audio component bind will never succeed if the the Intel IGFX is not in actual use.
Will need a bit of time to think about possible scenarios. Possibly this is not an issue outside early development systems. In theory if IGFX is disabled in BIOS, and not visible to OS, we are good, and if it's visible, the i915/xe driver should be loaded, so we are good again.
The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following:
- Check when the deferred probe takes too long, and warn it - Provide some runtime option to disable the component binding, so that user can work around it if needed
thanks,
Takashi
Hey,
On 2023-07-19 08:01, Takashi Iwai wrote:
On Tue, 18 Jul 2023 19:04:41 +0200, Kai Vehmanen wrote:
diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index f1fd5b44aaac9..344b61576c0e3 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0;
/* i915 exposes a HDA codec for HDMI audio */
- ret = snd_hdac_i915_init(bus, true);
- ret = snd_hdac_i915_init(bus, false); if (ret < 0) return ret;
My only bigger concern is corner cases where the display PCI device is on the bus and visible to kernel, but for some reason there is no working driver in the system or it is disabled.
With this patch, not having a workign display driver means that there is also no audio in the system as the SOF driver will never get probed.
In current mainline, one will get the 60sec timeout warning and then audio driver will proceed to probe and you'll have audio support (minus HDMI/DP).
Yeah, that was a concern in the past, too. e.g. when you pass "nomodeset" boot option, the driver will become unusable, even if the bus is used generically for both analog and HDMI codecs.
Yeah, I have no answer for this. My guess is that in an ideal world, the optional features related to HDMI outputs would be put in a separate sub-driver, which could -EPROBE_DEFER. Only when this driver loads, features related to display will work, but the main audio driver could still load.
This is mostly an issue with very new hardware (e.g. hw is still behind force_probe flag in xe/i915 driver), but we've had some odd cases with e.g. systems with both Intel IGFX and other vendors' DGPU. Audio drivers see the Intel VGA controller in system and will call snd_hdac_i915_init(), but the audio component bind will never succeed if the the Intel IGFX is not in actual use.
Will need a bit of time to think about possible scenarios. Possibly this is not an issue outside early development systems. In theory if IGFX is disabled in BIOS, and not visible to OS, we are good, and if it's visible, the i915/xe driver should be loaded, so we are good again.
The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following:
- Check when the deferred probe takes too long, and warn it
- Provide some runtime option to disable the component binding, so that user can work around it if needed
A module option to snd_hdac_i915_init would probably be the least of all evils here.
I see the removal of the 60 second timeout as a good thing regardless. :-) Usually when nomodeset is used, it's just for safe mode.
With the addition of the xe driver, blindly modprobing i915 will fall apart regardless.
~Maarten
On Wed, 19 Jul 2023 11:48:06 +0200, Maarten Lankhorst wrote:
The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following: - Check when the deferred probe takes too long, and warn it - Provide some runtime option to disable the component binding, so that user can work around it if neededA module option to snd_hdac_i915_init would probably be the least of all evils here.
Yes, probably it's the easiest option and sufficient.
thanks,
Takashi
On 2023-07-19 13:06, Takashi Iwai wrote:
On Wed, 19 Jul 2023 11:48:06 +0200, Maarten Lankhorst wrote:
The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following: - Check when the deferred probe takes too long, and warn it - Provide some runtime option to disable the component binding, so that user can work around it if neededA module option to snd_hdac_i915_init would probably be the least of all evils here.
Yes, probably it's the easiest option and sufficient.
thanks,
Takashi
Hey,
Patch below, can be applied immediately iresspective of the other patches.
---->8----------
Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports video_firmware_drivers_only(). This can be used as a first approximation on whether i915 will be available. It's safe to use as this is only built when CONFIG_SND_HDA_I915 is selected by CONFIG_I915.
It's not completely fool proof, as you can boot with "nomodeset i915.modeset=1" to make i915 load regardless, or use "i915.force_probe=!*" to never load i915, but the common case of booting with nomodeset to disable all GPU drivers this will work as intended.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 1637dc6e630a6..90bcf84f7b2ce 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -11,6 +11,8 @@ #include <sound/hda_i915.h> #include <sound/hda_register.h>
+#include <video/nomodeset.h> + #define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \ ((pci)->device == 0x0c0c) || \ ((pci)->device == 0x0d0c) || \ @@ -122,6 +124,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci) { struct pci_dev *display_dev = NULL;
+ if (video_firmware_drivers_only()) + return false; + for_each_pci_dev(display_dev) { if (display_dev->vendor == PCI_VENDOR_ID_INTEL && (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
On Wed, Jul 19, 2023 at 02:13:59PM +0200, Maarten Lankhorst wrote:
On 2023-07-19 13:06, Takashi Iwai wrote:
On Wed, 19 Jul 2023 11:48:06 +0200, Maarten Lankhorst wrote:
The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following: - Check when the deferred probe takes too long, and warn it - Provide some runtime option to disable the component binding, so that user can work around it if neededA module option to snd_hdac_i915_init would probably be the least of all evils here.
Yes, probably it's the easiest option and sufficient.
thanks,
Takashi
Hey,
Patch below, can be applied immediately iresspective of the other patches.
---->8----------
Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports video_firmware_drivers_only(). This can be used as a first approximation on whether i915 will be available. It's safe to use as this is only built
Please don't bury new patches in the middle of mails, it just makes it hard to apply the patch since tooling doesn't understand what's going on.
Please don't send new patches in reply to old patches or serieses, this makes it harder for both people and tools to understand what is going on - it can bury things in mailboxes and make it difficult to keep track of what current patches are, both for the new patches and the old ones.
Hey,
On 2023-07-19 14:39, Mark Brown wrote:
On Wed, Jul 19, 2023 at 02:13:59PM +0200, Maarten Lankhorst wrote:
On 2023-07-19 13:06, Takashi Iwai wrote:
On Wed, 19 Jul 2023 11:48:06 +0200, Maarten Lankhorst wrote:
The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following: - Check when the deferred probe takes too long, and warn it - Provide some runtime option to disable the component binding, so that user can work around it if neededA module option to snd_hdac_i915_init would probably be the least of all evils here.
Yes, probably it's the easiest option and sufficient.
thanks,
Takashi
Hey,
Patch below, can be applied immediately iresspective of the other patches.
---->8----------
Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports video_firmware_drivers_only(). This can be used as a first approximation on whether i915 will be available. It's safe to use as this is only built
Please don't bury new patches in the middle of mails, it just makes it hard to apply the patch since tooling doesn't understand what's going on.
Please don't send new patches in reply to old patches or serieses, this makes it harder for both people and tools to understand what is going on - it can bury things in mailboxes and make it difficult to keep track of what current patches are, both for the new patches and the old ones.
I will send a new version in a bit, with all comments addressed.
I need to finish testing first.
~Maarten
On Wed, 19 Jul 2023 14:13:59 +0200, Maarten Lankhorst wrote:
On 2023-07-19 13:06, Takashi Iwai wrote:
On Wed, 19 Jul 2023 11:48:06 +0200, Maarten Lankhorst wrote:
The 60 seconds timeout is a thing "better than complete disablement", so it's not ideal, either. Maybe we can add something like the following: - Check when the deferred probe takes too long, and warnit - Provide some runtime option to disable the component binding, so that user can work around it if needed A module option to snd_hdac_i915_init would probably be the least of all evils here.
Yes, probably it's the easiest option and sufficient.
thanks,
Takashi
Hey,
Patch below, can be applied immediately iresspective of the other patches.
---->8----------
Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports video_firmware_drivers_only(). This can be used as a first approximation on whether i915 will be available. It's safe to use as this is only built when CONFIG_SND_HDA_I915 is selected by CONFIG_I915.
It's not completely fool proof, as you can boot with "nomodeset i915.modeset=1" to make i915 load regardless, or use "i915.force_probe=!*" to never load i915, but the common case of booting with nomodeset to disable all GPU drivers this will work as intended.
The check of video_firmware_drivers_only() may help a bit, but I believe we still need an option to override the behavior, from the same reason as why i915.modeset option behaves so. In general, nomodeset is for a debugging purpose, and without an option, you'll have no way to re-enable the HD-audio even if you could reload the graphics driver.
thanks,
Takashi
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 1637dc6e630a6..90bcf84f7b2ce 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -11,6 +11,8 @@ #include <sound/hda_i915.h> #include <sound/hda_register.h>
+#include <video/nomodeset.h>
#define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \ ((pci)->device == 0x0c0c) || \ ((pci)->device == 0x0d0c) || \ @@ -122,6 +124,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci) { struct pci_dev *display_dev = NULL;
- if (video_firmware_drivers_only())
return false;- for_each_pci_dev(display_dev) { if (display_dev->vendor == PCI_VENDOR_ID_INTEL && (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
Hi,
On Wed, 19 Jul 2023, Maarten Lankhorst wrote:
On Tue, 18 Jul 2023 19:04:41 +0200, Kai Vehmanen wrote:
My only bigger concern is corner cases where the display PCI device is
on
the bus and visible to kernel, but for some reason there is no working driver in the system or it is disabled.
Yeah, I have no answer for this. My guess is that in an ideal world, the optional features related to HDMI outputs would be put in a separate sub-driver, which could -EPROBE_DEFER. Only when this driver loads, features related to display will work, but the main audio driver could still load.
in longer term, we have ongoing work in SOF to allow exposing multiple cards (e.g. to have a separate card for HDMI/DP PCM devices), and we are continuously working at improving the data we get from ACPI to have less guesswork in the driver. But this really doesn't help in the shortterm and/or cover all scenarios.
So for now, this is legacy we just need to deal with. OTOH, I do agree that...
A module option to snd_hdac_i915_init would probably be the least of all evils here.
I see the removal of the 60 second timeout as a good thing regardless. :-) Usually when nomodeset is used, it's just for safe mode.
With the addition of the xe driver, blindly modprobing i915 will fall apart regardless.
The modprobing of i915 from the audio driver, has always felt a bit out-of-place, and with the xe driver, this simply won't scale anymore.
The test results so far look good and this patchset works ok even if some of the more complex multi-GPU configurations we have, so I think with a module option to snd_hdac_i915, I'd be ready to go with this.
Br, Kai
Now that all drivers have moved from modprobe loading to handling -EPROBE_DEFER, we can remove the argument again.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- include/sound/hda_i915.h | 4 ++-- sound/hda/hdac_i915.c | 17 +++-------------- sound/pci/hda/hda_intel.c | 2 +- sound/soc/intel/avs/core.c | 2 +- sound/soc/intel/skylake/skl.c | 2 +- sound/soc/sof/intel/hda-codec.c | 2 +- 6 files changed, 9 insertions(+), 20 deletions(-)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index f91bd66360865..6b79614a893b9 100644 --- a/include/sound/hda_i915.h +++ b/include/sound/hda_i915.h @@ -9,12 +9,12 @@
#ifdef CONFIG_SND_HDA_I915 void snd_hdac_i915_set_bclk(struct hdac_bus *bus); -int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe); +int snd_hdac_i915_init(struct hdac_bus *bus); #else static inline void snd_hdac_i915_set_bclk(struct hdac_bus *bus) { } -static inline int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe) +static inline int snd_hdac_i915_init(struct hdac_bus *bus) { return -ENODEV; } diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index c88f251388e80..1637dc6e630a6 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -146,7 +146,7 @@ static int i915_gfx_present(struct pci_dev *hdac_pci) * * Returns zero for success or a negative error code. */ -int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe) +int snd_hdac_i915_init(struct hdac_bus *bus) { struct drm_audio_component *acomp; int err; @@ -162,21 +162,10 @@ int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe) acomp = bus->audio_component; if (!acomp) return -ENODEV; - if (allow_modprobe && !acomp->ops) { - if (!IS_ENABLED(CONFIG_MODULES) || - !request_module("i915")) { - /* 60s timeout */ - wait_for_completion_killable_timeout(&acomp->master_bind_complete, - msecs_to_jiffies(60 * 1000)); - } - } if (!acomp->ops) { - if (allow_modprobe) - dev_info(bus->dev, "couldn't bind with audio component\n"); - else - dev_dbg(bus->dev, "couldn't bind with audio component\n"); + dev_dbg(bus->dev, "couldn't bind with audio component\n"); snd_hdac_acomp_exit(bus); - return allow_modprobe ? -ENODEV : -EPROBE_DEFER; + return -EPROBE_DEFER; } return 0; } diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index d40345a0088d8..0959e86b9a165 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2176,7 +2176,7 @@ static int azx_probe(struct pci_dev *pci, #ifdef CONFIG_SND_HDA_I915 /* bind with i915 if needed */ if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { - err = snd_hdac_i915_init(azx_bus(chip), false); + err = snd_hdac_i915_init(azx_bus(chip)); if (err < 0) { /* if the controller is bound only with HDMI/DP * (for HSW and BDW), we need to abort the probe; diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index d3a7f42387e9b..bd1caf8cf90c4 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -461,7 +461,7 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) pci_set_drvdata(pci, bus); device_disable_async_suspend(dev);
- ret = snd_hdac_i915_init(bus, false); + ret = snd_hdac_i915_init(bus); if (ret == -EPROBE_DEFER) goto err_unmaster; else if (ret < 0) diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index ff80d83a9fb72..49147ee3a76db 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -1056,7 +1056,7 @@ static int skl_probe(struct pci_dev *pci, }
if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { - err = snd_hdac_i915_init(bus, false); + err = snd_hdac_i915_init(bus); if (err < 0) goto out_dmic_unregister; } diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 344b61576c0e3..8a5e99a898ecb 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0;
/* i915 exposes a HDA codec for HDMI audio */ - ret = snd_hdac_i915_init(bus, false); + ret = snd_hdac_i915_init(bus); if (ret < 0) return ret;
On Tue, 18 Jul 2023 10:45:15 +0200, Maarten Lankhorst wrote:
Explicitly loading i915 becomes a problem when upstreaming the new intel driver for Tiger Lake and higher graphics (xe). By loading i915, it doesn't wait for driver load of xe, and will fail completely before it loads.
-EPROBE_DEFER has to be returned before any device is created in probe(), otherwise the removal of the device will cause EPROBE_DEFER to try again in an infinite loop.
The conversion is done in gradual steps. First I add an argument to snd_hdac_i915_init to allow for -EPROBE_DEFER so I can convert each driver separately. Then I convert each driver to move snd_hdac_i915_init out of the workqueue. Finally I drop the ability to choose modprobe behavior after the last user is converted.
I suspect the avs and skylake drivers used snd_hdac_i915_init purely for the modprobe, but I don't have the hardware to test if it can be safely removed. It can still be done easily in a followup patch to simplify probing.
Thanks for the patches. I naively thought that issuing the component binding at the early stage could be problematic, but in this case, this doesn't look OK, since the all i915 stuff is bound always at the HD-audio controller level, not at the codec level like others.
So, it's definitely worth to try.
Takashi
Maarten Lankhorst (7): ALSA: hda/i915: Add an allow_modprobe argument to snd_hdac_i915_init ALSA: hda/i915: Allow xe as match for i915_component_master_match ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work. ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work. ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work. ASoC: SOF: Intel: Remove deferred probe for SOF ALSA: hda/i915: Remove extra argument from snd_hdac_i915_init
Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Cezary Rojewski cezary.rojewski@intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Liam Girdwood liam.r.girdwood@linux.intel.com Cc: Peter Ujfalusi peter.ujfalusi@linux.intel.com Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Ranjani Sridharan ranjani.sridharan@linux.intel.com Cc: Kai Vehmanen kai.vehmanen@linux.intel.com Cc: Mark Brown broonie@kernel.org Cc: Daniel Baluta daniel.baluta@nxp.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Cc: sound-open-firmware@alsa-project.org
sound/hda/hdac_i915.c | 15 +++------ sound/pci/hda/hda_intel.c | 58 +++++++++++++++++++---------------- sound/soc/intel/avs/core.c | 13 +++++--- sound/soc/intel/skylake/skl.c | 31 ++++++------------- sound/soc/sof/Kconfig | 19 ------------ sound/soc/sof/core.c | 38 ++--------------------- sound/soc/sof/intel/Kconfig | 1 - sound/soc/sof/intel/hda.c | 32 +++++++++++-------- sound/soc/sof/sof-pci-dev.c | 3 +- sound/soc/sof/sof-priv.h | 5 --- 10 files changed, 75 insertions(+), 140 deletions(-)
-- 2.39.2
participants (5)
-
Cezary Rojewski -
Kai Vehmanen -
Maarten Lankhorst -
Mark Brown -
Takashi Iwai