[PATCH v6 00/12] sound: Use -EPROBE_DEFER instead of i915 module loading.
From: Maarten Lankhorst dev@lankhorst.se
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.
Compared to previous version, I added a fix for sof_ops_free() missing call, renamed probe_no_wq and remove_no_wq to probe_early/probe_late, and fixed the resulting fallout.
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
Maarten Lankhorst (10): ASoC: SOF: core: Ensure sof_ops_free() is still called when probe never ran. ALSA: hda: Intel: Fix error handling in azx_probe() ALSA: hda: i915: Allow override of gpu binding. 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. ALSA: hda: Intel: Move snd_hdac_i915_init to before probe_work. ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work. ASoC: SOF: Intel: Move binding to display driver outside of deferred probe ALSA: hda: i915: Remove extra argument from snd_hdac_i915_init
Pierre-Louis Bossart (2): ASoC: SOF: core: Add probe_early and remove_late callbacks ASoC: SOF: Intel: hda: start splitting the probe
sound/hda/hdac_i915.c | 24 ++++++----- sound/pci/hda/hda_intel.c | 60 ++++++++++++++-------------- sound/soc/intel/avs/core.c | 13 ++++-- sound/soc/intel/skylake/skl.c | 31 +++++--------- sound/soc/sof/core.c | 17 +++++++- sound/soc/sof/intel/hda-common-ops.c | 2 + sound/soc/sof/intel/hda.c | 32 ++++++++------- sound/soc/sof/intel/hda.h | 2 + sound/soc/sof/ops.h | 16 ++++++++ sound/soc/sof/sof-priv.h | 2 + 10 files changed, 118 insertions(+), 81 deletions(-)
In an effort to not call sof_ops_free twice, we stopped running it when probe was aborted.
Check the result of cancel_work_sync to see if this was the case.
Fixes: 31bb7bd9ffee ("ASoC: SOF: core: Only call sof_ops_free() on remove if the probe was successful") Cc: Peter Ujfalusi peter.ujfalusi@linux.intel.com Acked-by: Mark Brown broonie@kernel.org --- sound/soc/sof/core.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 2d1616b81485..0938b259f703 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -459,9 +459,10 @@ 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; + bool aborted = false;
if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) - cancel_work_sync(&sdev->probe_work); + aborted = cancel_work_sync(&sdev->probe_work);
/* * Unregister any registered client device first before IPC and debugfs @@ -487,6 +488,9 @@ int snd_sof_device_remove(struct device *dev) snd_sof_free_debug(sdev); snd_sof_remove(sdev); sof_ops_free(sdev); + } else if (aborted) { + /* probe_work never ran */ + sof_ops_free(sdev); }
/* release firmware */
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The existing DSP probe may be handled in a workqueue to allow for extra time, typically for the i915 request_module and HDAudio codec handling.
With the upcoming changes for i915/Xe driver relying on the -EPROBE_DEFER mechanism, we need to have a first pass of the probe which cannot be pushed to a workqueue. Introduce 2 new optional callbacks.
probe_early is called before the workqueue runs. remove_late may be called from the workqueue if load is unsuccesful, but will otherwise be called on module unload.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Mark Brown broonie@kernel.org --- sound/soc/sof/core.c | 11 +++++++++++ sound/soc/sof/ops.h | 16 ++++++++++++++++ sound/soc/sof/sof-priv.h | 2 ++ 3 files changed, 29 insertions(+)
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 0938b259f703..d7b090224f1b 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -327,6 +327,7 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) dsp_err: snd_sof_remove(sdev); probe_err: + snd_sof_remove_late(sdev); sof_ops_free(sdev);
/* all resources freed, update state to match */ @@ -436,6 +437,14 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);
+ /* + * first pass of probe which isn't allowed to run in a work-queue, + * typically to rely on -EPROBE_DEFER dependencies + */ + ret = snd_sof_probe_early(sdev); + if (ret < 0) + return ret; + if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) { INIT_WORK(&sdev->probe_work, sof_probe_work); schedule_work(&sdev->probe_work); @@ -487,9 +496,11 @@ int snd_sof_device_remove(struct device *dev) snd_sof_ipc_free(sdev); snd_sof_free_debug(sdev); snd_sof_remove(sdev); + snd_sof_remove_late(sdev); sof_ops_free(sdev); } else if (aborted) { /* probe_work never ran */ + snd_sof_remove_late(sdev); sof_ops_free(sdev); }
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h index 9ab7b9be765b..3ebcfc237385 100644 --- a/sound/soc/sof/ops.h +++ b/sound/soc/sof/ops.h @@ -38,6 +38,14 @@ static inline void sof_ops_free(struct snd_sof_dev *sdev) /* Mandatory operations are verified during probing */
/* init */ +static inline int snd_sof_probe_early(struct snd_sof_dev *sdev) +{ + if (sof_ops(sdev)->probe_early) + return sof_ops(sdev)->probe_early(sdev); + + return 0; +} + static inline int snd_sof_probe(struct snd_sof_dev *sdev) { return sof_ops(sdev)->probe(sdev); @@ -51,6 +59,14 @@ static inline int snd_sof_remove(struct snd_sof_dev *sdev) return 0; }
+static inline int snd_sof_remove_late(struct snd_sof_dev *sdev) +{ + if (sof_ops(sdev)->remove_late) + return sof_ops(sdev)->remove_late(sdev); + + return 0; +} + static inline int snd_sof_shutdown(struct snd_sof_dev *sdev) { if (sof_ops(sdev)->shutdown) diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index d4f6702e93dc..e73a92189fe1 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -165,8 +165,10 @@ struct sof_firmware { struct snd_sof_dsp_ops {
/* probe/remove/shutdown */ + int (*probe_early)(struct snd_sof_dev *sof_dev); /* optional */ int (*probe)(struct snd_sof_dev *sof_dev); /* mandatory */ int (*remove)(struct snd_sof_dev *sof_dev); /* optional */ + int (*remove_late)(struct snd_sof_dev *sof_dev); /* optional */ int (*shutdown)(struct snd_sof_dev *sof_dev); /* optional */
/* DSP core boot / reset */
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
This patch moves the initial parts of the probe to the probe_early() callback, which provides a much faster decision on whether the SOF driver shall deal with a specific platform or yield to other Intel drivers.
This is a limited functionality change, the bigger change is to move the i915/Xe initialization to the probe_early().
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Mark Brown broonie@kernel.org --- sound/soc/sof/intel/hda-common-ops.c | 1 + sound/soc/sof/intel/hda.c | 16 +++++++++++++--- sound/soc/sof/intel/hda.h | 1 + 3 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c index 8e1cd0babd32..1cc18fb2b75b 100644 --- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -16,6 +16,7 @@
struct snd_sof_dsp_ops sof_hda_common_ops = { /* probe/remove/shutdown */ + .probe_early = hda_dsp_probe_early, .probe = hda_dsp_probe, .remove = hda_dsp_remove,
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 15e6779efaa3..86a2571488bc 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1118,11 +1118,10 @@ static irqreturn_t hda_dsp_interrupt_thread(int irq, void *context) return IRQ_HANDLED; }
-int hda_dsp_probe(struct snd_sof_dev *sdev) +int hda_dsp_probe_early(struct snd_sof_dev *sdev) { struct pci_dev *pci = to_pci_dev(sdev->dev); struct sof_intel_hda_dev *hdev; - struct hdac_bus *bus; const struct sof_intel_dsp_desc *chip; int ret = 0;
@@ -1162,6 +1161,17 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) sdev->pdata->hw_pdata = hdev; hdev->desc = chip;
+err: + return ret; +} + +int hda_dsp_probe(struct snd_sof_dev *sdev) +{ + struct pci_dev *pci = to_pci_dev(sdev->dev); + struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata; + struct hdac_bus *bus; + int ret = 0; + hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", PLATFORM_DEVID_NONE, NULL, 0); @@ -1299,7 +1309,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) platform_device_unregister(hdev->dmic_dev); iounmap(bus->remap_addr); hda_codec_i915_exit(sdev); -err: + return ret; }
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 5c517ec57d4a..e13cdc933ca6 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -573,6 +573,7 @@ struct sof_intel_hda_stream { /* * DSP Core services. */ +int hda_dsp_probe_early(struct snd_sof_dev *sdev); int hda_dsp_probe(struct snd_sof_dev *sdev); int hda_dsp_remove(struct snd_sof_dev *sdev); int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);
Add missing pci_set_drv to NULL call on error.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/pci/hda/hda_intel.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index ca765ac4765f..b4ac0d43c09b 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2176,6 +2176,7 @@ static int azx_probe(struct pci_dev *pci, return 0;
out_free: + pci_set_drvdata(pci, NULL); snd_card_free(card); return err; }
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.
Because of this, we add an extra module parameter, snd_hda_core.gpu_bind that can be used to signal users intent. -1 follows nomodeset, 0 disables binding, 1 forces wait/-EPROBE_DEFER on binding.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/hda/hdac_i915.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index b428537f284c..a4a712c795c3 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -10,6 +10,12 @@ #include <sound/hdaudio.h> #include <sound/hda_i915.h> #include <sound/hda_register.h> +#include <video/nomodeset.h> + +static int gpu_bind = -1; +module_param(gpu_bind, int, 0644); +MODULE_PARM_DESC(gpu_bind, "Whether to bind sound component to GPU " + "(1=always, 0=never, -1=on nomodeset(default))");
/** * snd_hdac_i915_set_bclk - Reprogram BCLK for HSW/BDW @@ -122,6 +128,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci) { struct pci_dev *display_dev = NULL;
+ if (!gpu_bind || (gpu_bind < 0 && 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 &&
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 Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/sound/hda_i915.h | 4 ++-- sound/hda/hdac_i915.c | 8 ++++---- 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, 10 insertions(+), 10 deletions(-)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index 6b79614a893b..f91bd6636086 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 a4a712c795c3..ffa35d7a367c 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -155,7 +155,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; @@ -171,7 +171,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 */ @@ -180,9 +180,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus) } } if (!acomp->ops) { - dev_info(bus->dev, "couldn't bind with audio component\n"); + int err = allow_modprobe ? -ENODEV : -EPROBE_DEFER; snd_hdac_acomp_exit(bus); - return -ENODEV; + return dev_err_probe(bus->dev, err, "couldn't bind with audio component\n"); } return 0; } diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index b4ac0d43c09b..bfe6232bb0e0 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2266,7 +2266,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 859b217fc761..bbb40339c75f 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 77408a981b97..4f7acb4f6680 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 8a5e99a898ec..f1fd5b44aaac 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.
The modprobe mechanism is being replaced by the -EPROBE_DEFER mechanism, so we don't need to add a modprobe xe call. Adding this would have required a telepathy module to correctly guess whether to load i915 or xe.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@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 ffa35d7a367c..0765e5350e7b 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -115,7 +115,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 Acked-by: Mark Brown broonie@kernel.org Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@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 bbb40339c75f..8a2063958248 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_i915_init; + else if (ret < 0) + dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret); + schedule_work(&adev->probe_work);
return 0;
+err_i915_init: + 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 2023-10-04 4:55 PM, 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.
Nitpick: Half of the commit message is redundant. From the reader/maintainer point of view, presented opinions and suggestions are fine as a part of cover-letter but not in the long run from git log level.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Mark Brown broonie@kernel.org Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
Nitpick: commit title is not a sentence, should not end with full stop.
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 bbb40339c75f..8a2063958248 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_i915_init;- else if (ret < 0)
The 'else' part seems redundant. s/else if/else/.
As I'm aware that this is v6 already, my team can also clean this up later with separate series to not delay the merge any longer.
dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);schedule_work(&adev->probe_work);
return 0;
+err_i915_init:
- 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 2023-10-10 1:43 PM, Cezary Rojewski wrote:
On 2023-10-04 4:55 PM, Maarten Lankhorst wrote:
...
@@ -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_i915_init; + else if (ret < 0)
The 'else' part seems redundant. s/else if/else/.
Spelling error on my part. What I meant is s/else if/if/.
+ dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
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 Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/pci/hda/hda_intel.c | 59 ++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index bfe6232bb0e0..5cf7676114dc 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2135,6 +2135,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 (HDA_CONTROLLER_IN_GPU(pci)) { + dev_err_probe(card->dev, err, + "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 (HDA_CONTROLLER_IN_GPU(pci)) + hda->need_i915_power = true; + } +#else + if (HDA_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"); @@ -2162,11 +2192,6 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */
-#ifndef CONFIG_SND_HDA_I915 - if (HDA_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);
@@ -2264,30 +2289,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 (HDA_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 (HDA_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
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 Acked-by: Mark Brown broonie@kernel.org Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@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 4f7acb4f6680..24bdbe2a53be 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:
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.
The previously added probe_early can be used for this, and we also use the newly added remove_late for unbinding afterwards.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/hda-common-ops.c | 1 + sound/soc/sof/intel/hda.c | 18 ++++++------------ sound/soc/sof/intel/hda.h | 1 + 3 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c index 1cc18fb2b75b..26105d8f1bdc 100644 --- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { .probe_early = hda_dsp_probe_early, .probe = hda_dsp_probe, .remove = hda_dsp_remove, + .remove_late = hda_dsp_remove_late,
/* Register IO uses direct mmio */
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 86a2571488bc..4eb7f04b8ae1 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) return -ENOMEM; sdev->pdata->hw_pdata = hdev; hdev->desc = chip; + ret = hda_init(sdev);
err: return ret; @@ -1169,7 +1170,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) { struct pci_dev *pci = to_pci_dev(sdev->dev); struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata; - struct hdac_bus *bus; int ret = 0;
hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", @@ -1193,12 +1193,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) if (sdev->dspless_mode_selected) hdev->no_ipc_position = 1;
- /* set up HDA base */ - bus = sof_to_bus(sdev); - ret = hda_init(sdev); - if (ret < 0) - goto hdac_bus_unmap; - if (sdev->dspless_mode_selected) goto skip_dsp_setup;
@@ -1307,8 +1301,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) iounmap(sdev->bar[HDA_DSP_BAR]); hdac_bus_unmap: platform_device_unregister(hdev->dmic_dev); - iounmap(bus->remap_addr); - hda_codec_i915_exit(sdev);
return ret; } @@ -1317,7 +1309,6 @@ int hda_dsp_remove(struct snd_sof_dev *sdev) { struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; const struct sof_intel_dsp_desc *chip = hda->desc; - struct hdac_bus *bus = sof_to_bus(sdev); struct pci_dev *pci = to_pci_dev(sdev->dev); struct nhlt_acpi_table *nhlt = hda->nhlt;
@@ -1368,10 +1359,13 @@ int hda_dsp_remove(struct snd_sof_dev *sdev) if (!sdev->dspless_mode_selected) iounmap(sdev->bar[HDA_DSP_BAR]);
- iounmap(bus->remap_addr); + return 0; +}
+int hda_dsp_remove_late(struct snd_sof_dev *sdev) +{ + iounmap(sof_to_bus(sdev)->remap_addr); sof_hda_bus_exit(sdev); - hda_codec_i915_exit(sdev);
return 0; diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index e13cdc933ca6..8e846684279e 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -576,6 +576,7 @@ struct sof_intel_hda_stream { int hda_dsp_probe_early(struct snd_sof_dev *sdev); int hda_dsp_probe(struct snd_sof_dev *sdev); int hda_dsp_remove(struct snd_sof_dev *sdev); +int hda_dsp_remove_late(struct snd_sof_dev *sdev); int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask); int hda_dsp_core_run(struct snd_sof_dev *sdev, unsigned int core_mask); int hda_dsp_enable_core(struct snd_sof_dev *sdev, unsigned int core_mask);
Hi,
I'm good with rest of the series, but one patch requires work.
On Wed, 4 Oct 2023, 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.
The previously added probe_early can be used for this, and we also use the newly added remove_late for unbinding afterwards.
[...]
--- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { .probe_early = hda_dsp_probe_early, .probe = hda_dsp_probe, .remove = hda_dsp_remove,
.remove_late = hda_dsp_remove_late,
/* Register IO uses direct mmio */
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 86a2571488bc..4eb7f04b8ae1 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) return -ENOMEM; sdev->pdata->hw_pdata = hdev; hdev->desc = chip;
- ret = hda_init(sdev);
err: return ret;
I don't think this works. The hda_codec_i915_init() errors are ignored in hda_init() so this never returns -EPROBE_DEFER.
So something like this is needed on top (tested quickly on one SOF machine and this blocks SOF load until i915 or xe driver is loaded):
--cut-- diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 9025bfaf6a7e..8b17c82dcc89 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -863,13 +863,20 @@ 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"); + dev_warn(sdev->dev, "init of i915 and HDMI codec failed (%d)\n", ret); + + if (ret < 0 && ret != -ENODEV) + goto out;
/* get controller capabilities */ ret = hda_dsp_ctrl_get_caps(sdev); if (ret < 0) dev_err(sdev->dev, "error: get caps error\n");
+out: + if (ret < 0) + iounmap(sof_to_bus(sdev)->remap_addr); + return ret; } --cut--
Br, Kai
On 04/10/2023 19:59, Kai Vehmanen wrote:
Hi,
I'm good with rest of the series, but one patch requires work.
On Wed, 4 Oct 2023, 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.
The previously added probe_early can be used for this, and we also use the newly added remove_late for unbinding afterwards.
[...]
--- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { .probe_early = hda_dsp_probe_early, .probe = hda_dsp_probe, .remove = hda_dsp_remove,
.remove_late = hda_dsp_remove_late,
/* Register IO uses direct mmio */
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 86a2571488bc..4eb7f04b8ae1 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) return -ENOMEM; sdev->pdata->hw_pdata = hdev; hdev->desc = chip;
- ret = hda_init(sdev);
err: return ret;
I don't think this works. The hda_codec_i915_init() errors are ignored in hda_init() so this never returns -EPROBE_DEFER.
So something like this is needed on top (tested quickly on one SOF machine and this blocks SOF load until i915 or xe driver is loaded):
--cut-- diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 9025bfaf6a7e..8b17c82dcc89 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -863,13 +863,20 @@ 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 codecfailed\n");
dev_warn(sdev->dev, "init of i915 and HDMI codec failed(%d)\n", ret);
we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.
if (ret < 0 && ret != -ENODEV)goto out; /* get controller capabilities */ ret = hda_dsp_ctrl_get_caps(sdev); if (ret < 0) dev_err(sdev->dev, "error: get caps error\n");+out:
if (ret < 0)iounmap(sof_to_bus(sdev)->remap_addr);return ret;} --cut--
Br, Kai
On 2023-10-05 12:58, Péter Ujfalusi wrote:
On 04/10/2023 19:59, Kai Vehmanen wrote:
Hi,
I'm good with rest of the series, but one patch requires work.
On Wed, 4 Oct 2023, 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.
The previously added probe_early can be used for this, and we also use the newly added remove_late for unbinding afterwards.
[...]
--- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { .probe_early = hda_dsp_probe_early, .probe = hda_dsp_probe, .remove = hda_dsp_remove,
.remove_late = hda_dsp_remove_late,
/* Register IO uses direct mmio */
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 86a2571488bc..4eb7f04b8ae1 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) return -ENOMEM; sdev->pdata->hw_pdata = hdev; hdev->desc = chip;
ret = hda_init(sdev);
err: return ret;
I don't think this works. The hda_codec_i915_init() errors are ignored in hda_init() so this never returns -EPROBE_DEFER.
So something like this is needed on top (tested quickly on one SOF machine and this blocks SOF load until i915 or xe driver is loaded):
--cut-- diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 9025bfaf6a7e..8b17c82dcc89 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -863,13 +863,20 @@ 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 codecfailed\n");
dev_warn(sdev->dev, "init of i915 and HDMI codec failed(%d)\n", ret);
we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.
There's dev_err_probe, which is dev_err on error, or sets the reason for deferred probe to the arguments if the error is -EPROBE_DEFER.
~Maarten
On Thu, 05 Oct 2023 13:26:18 +0200, Maarten Lankhorst wrote:
On 2023-10-05 12:58, Péter Ujfalusi wrote:
On 04/10/2023 19:59, Kai Vehmanen wrote:
Hi,
I'm good with rest of the series, but one patch requires work.
On Wed, 4 Oct 2023, 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.
The previously added probe_early can be used for this, and we also use the newly added remove_late for unbinding afterwards.
[...]
--- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { .probe_early = hda_dsp_probe_early, .probe = hda_dsp_probe, .remove = hda_dsp_remove,
- .remove_late = hda_dsp_remove_late, /* Register IO uses direct mmio */ diff --git a/sound/soc/sof/intel/hda.c
b/sound/soc/sof/intel/hda.c index 86a2571488bc..4eb7f04b8ae1 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) return -ENOMEM; sdev->pdata->hw_pdata = hdev; hdev->desc = chip;
- ret = hda_init(sdev); err: return ret;
I don't think this works. The hda_codec_i915_init() errors are ignored in hda_init() so this never returns -EPROBE_DEFER.
So something like this is needed on top (tested quickly on one SOF machine and this blocks SOF load until i915 or xe driver is loaded):
--cut-- diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 9025bfaf6a7e..8b17c82dcc89 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -863,13 +863,20 @@ 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 codecfailed\n");
dev_warn(sdev->dev, "init of i915 and HDMI codec failed(%d)\n", ret);
we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.
There's dev_err_probe, which is dev_err on error, or sets the reason for deferred probe to the arguments if the error is -EPROBE_DEFER.
I expect you'll respin v7 for addressing this?
I'd love to merge the series for 6.7, and the time ticks...
thanks,
Takashi
Hey,
On 2023-10-09 08:23, Takashi Iwai wrote:
On Thu, 05 Oct 2023 13:26:18 +0200, Maarten Lankhorst wrote:
On 2023-10-05 12:58, Péter Ujfalusi wrote:
On 04/10/2023 19:59, Kai Vehmanen wrote:
Hi,
I'm good with rest of the series, but one patch requires work.
On Wed, 4 Oct 2023, 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.
The previously added probe_early can be used for this, and we also use the newly added remove_late for unbinding afterwards.
[...]
--- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { .probe_early = hda_dsp_probe_early, .probe = hda_dsp_probe, .remove = hda_dsp_remove,
- .remove_late = hda_dsp_remove_late, /* Register IO uses direct mmio */ diff --git a/sound/soc/sof/intel/hda.c
b/sound/soc/sof/intel/hda.c index 86a2571488bc..4eb7f04b8ae1 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev) return -ENOMEM; sdev->pdata->hw_pdata = hdev; hdev->desc = chip;
- ret = hda_init(sdev); err: return ret;
I don't think this works. The hda_codec_i915_init() errors are ignored in hda_init() so this never returns -EPROBE_DEFER.
So something like this is needed on top (tested quickly on one SOF machine and this blocks SOF load until i915 or xe driver is loaded):
--cut-- diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 9025bfaf6a7e..8b17c82dcc89 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -863,13 +863,20 @@ 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 codecfailed\n");
dev_warn(sdev->dev, "init of i915 and HDMI codec failed(%d)\n", ret);
we should not print anything or maximum dev_dbg in case of EPROBE_DEFER.
There's dev_err_probe, which is dev_err on error, or sets the reason for deferred probe to the arguments if the error is -EPROBE_DEFER.
I expect you'll respin v7 for addressing this?
I'd love to merge the series for 6.7, and the time ticks...
Done, added the error handling early in the series as a bugfix.
Cheers, ~Maarten
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 Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- include/sound/hda_i915.h | 4 ++-- sound/hda/hdac_i915.c | 14 +++----------- 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(+), 17 deletions(-)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index f91bd6636086..6b79614a893b 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 0765e5350e7b..365c36fdf205 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -156,7 +156,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; @@ -172,18 +172,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) { - int err = allow_modprobe ? -ENODEV : -EPROBE_DEFER; snd_hdac_acomp_exit(bus); - return dev_err_probe(bus->dev, err, "couldn't bind with audio component\n"); + return dev_err_probe(bus->dev, -EPROBE_DEFER, + "couldn't bind with audio component\n"); } return 0; } diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 5cf7676114dc..5255df16fbac 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2138,7 +2138,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 8a2063958248..33044f353575 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_i915_init; else if (ret < 0) diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 24bdbe2a53be..f46f109d5856 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 f1fd5b44aaac..8a5e99a898ec 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); if (ret < 0) return ret;
participants (6)
-
Cezary Rojewski -
Kai Vehmanen -
Maarten Lankhorst -
Maarten Lankhorst -
Péter Ujfalusi -
Takashi Iwai