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 the patch
"ASoC: SOF: Intel: Fix error handling in hda_init()"
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Cezary Rojewski <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Peter Ujfalusi <[email protected]>
Cc: Bard Liao <[email protected]>
Cc: Ranjani Sridharan <[email protected]>
Cc: Kai Vehmanen <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Daniel Baluta <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Maarten Lankhorst (11):
ASoC: SOF: core: Ensure sof_ops_free() is still called when probe
never ran.
ASoC: SOF: Intel: Fix error handling in hda_init()
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 | 46 +++++++++++++--------
sound/soc/sof/intel/hda.h | 2 +
sound/soc/sof/ops.h | 16 ++++++++
sound/soc/sof/sof-priv.h | 2 +
10 files changed, 129 insertions(+), 84 deletions(-)
--
2.39.2
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 <[email protected]>
Reviewed-by: Peter Ujfalusi <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
---
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 b428537f284c7..a4a712c795c3d 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 &&
--
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. 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 <[email protected]>
Acked-by: Mark Brown <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
Reviewed-by: Amadeusz Sławiński <[email protected]>
---
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 bbb40339c75f4..8a20639582487 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);
--
2.39.2
Add missing pci_set_drv to NULL call on error.
Signed-off-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Peter Ujfalusi <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
---
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 e19274fd990d9..e1354ae905564 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;
}
--
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.
Signed-off-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
---
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 e1e832afb4a30..c729ce192a7d2 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
--
2.39.2
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Signed-off-by: Maarten Lankhorst <[email protected]>
Acked-by: Mark Brown <[email protected]>
---
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 0938b259f7034..d7b090224f1b9 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 9ab7b9be765bc..3ebcfc2373854 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 d4f6702e93dcb..e73a92189fe1f 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 */
--
2.39.2
Hi Maarteen,
On 09/10/2023 14:54, 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.
>
> Compared to previous version, I added the patch
> "ASoC: SOF: Intel: Fix error handling in hda_init()"
Thank you for the updates.
to all:
Reviewed-by: Peter Ujfalusi <[email protected]>
and for sound/soc/sof/ :
Acked-by: Peter Ujfalusi <[email protected]>
> Cc: Jaroslav Kysela <[email protected]>
> Cc: Takashi Iwai <[email protected]>
> Cc: Cezary Rojewski <[email protected]>
> Cc: Pierre-Louis Bossart <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Peter Ujfalusi <[email protected]>
> Cc: Bard Liao <[email protected]>
> Cc: Ranjani Sridharan <[email protected]>
> Cc: Kai Vehmanen <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Daniel Baluta <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Maarten Lankhorst (11):
> ASoC: SOF: core: Ensure sof_ops_free() is still called when probe
> never ran.
> ASoC: SOF: Intel: Fix error handling in hda_init()
> 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 | 46 +++++++++++++--------
> sound/soc/sof/intel/hda.h | 2 +
> sound/soc/sof/ops.h | 16 ++++++++
> sound/soc/sof/sof-priv.h | 2 +
> 10 files changed, 129 insertions(+), 84 deletions(-)
>
--
Péter