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.
---
New since first version:
- snd_hda_core.gpu_bind is added as a mechanism to force gpu binding,
for testing. snd_hda_core.gpu_bind=0 forces waiting for GPU bind to
off, snd_hda_core.gpu_bind=1 forces waiting for gpu bind. Default
setting depends on whether kernel booted with nomodeset.
- Incorporated all feedback review.
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 (9):
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.
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
sound/hda/hdac_i915.c | 25 ++++++++-------
sound/pci/hda/hda_intel.c | 60 ++++++++++++++++++-----------------
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, 85 insertions(+), 142 deletions(-)
--
2.39.2
Add missing pci_set_drv to NULL call on error.
Signed-off-by: Maarten Lankhorst <[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 ef831770ca7da..0d2d6bc6c75ef 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2188,6 +2188,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. 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]>
---
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:
--
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)
- Move snd_hdac_i915_init slightly upward, to ensure
it's always initialised before vga-switcheroo is called.
Signed-off-by: Maarten Lankhorst <[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 11cf9907f039f..e3128d7d742e7 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_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 +2204,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);
@@ -2275,30 +2300,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
--
2.39.2
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.
Changes since v1:
- Use dev_err_probe to set a probe reason for debugfs' deferred_devices.
Signed-off-by: Maarten Lankhorst <[email protected]>
---
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 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 c32709fa4115f..961fcd3397f40 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 0d2d6bc6c75ef..11cf9907f039f 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2277,7 +2277,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;
--
2.39.2
Now that all drivers have moved from modprobe loading to
handling -EPROBE_DEFER, we can remove the argument again.
Changes since v1:
- Use dev_err_probe() to set reason in debugfs for deferred probe.
Signed-off-by: Maarten Lankhorst <[email protected]>
---
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 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 12c1f8d93499f..ad13f0e2f94fc 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 e3128d7d742e7..b4fa925a992b5 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,
#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 64e7a4e650a86..d350204a1d860 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 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;
--
2.39.2
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 <[email protected]>
---
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 961fcd3397f40..12c1f8d93499f 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;
--
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.
Changes since v1:
- Rename error label.
Signed-off-by: Maarten Lankhorst <[email protected]>
Acked-by: Mark Brown <[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 3311a6f142001..64e7a4e650a86 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
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 <[email protected]>
Acked-by: Matthew Auld <[email protected]>
Acked-by: Mark Brown <[email protected]>
---
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);
--
2.39.2
Hi,
On Wed, 19 Jul 2023, 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.
thanks, series looks good to me now. We'll need to adopt the new gpu_bind
parameter in a number of CI systems (where we test without i915/xe), but
this looks perfectly doable.
I'll give my
Reviewed-by: Kai Vehmanen <[email protected]>
... for the hdac_i915.c changes. For AVS and SOF, I'd ask for some
more review time to allow Cezary, Pierre et al to weigh in. I don't
personally recall e.g. where we've used CONFIG_SOF_FORCE_PROBE_WORKQUEUE
and do we have grounds to keep it even if workqueue is no longer set
for HDA codec support.
Br, Kai
On 19/07/2023 19:41, Maarten Lankhorst wrote:
> 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.
>
> Changes since v1:
> - Use dev_err_probe to set a probe reason for debugfs' deferred_devices.
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> ---
> 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/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index c32709fa4115f..961fcd3397f40 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;
Add one blank line here.
> snd_hdac_acomp_exit(bus);
> - return -ENODEV;
> + return dev_err_probe(bus->dev, err, "couldn't bind with audio component\n");
> }
> return 0;
> }
--
Péter
On 19/07/2023 19:41, Maarten Lankhorst wrote:
> Add missing pci_set_drv to NULL call on error.
>
> Signed-off-by: Maarten Lankhorst <[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 ef831770ca7da..0d2d6bc6c75ef 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -2188,6 +2188,7 @@ static int azx_probe(struct pci_dev *pci,
> return 0;
>
> out_free:
> + pci_set_drvdata(pci, NULL);
The original patch added this:
f4c482a4d0b3 ("ALSA: hda - Fix yet another race of vga_switcheroo registration")
but got removed later by:
20a24225d8f9 ("ALSA: PCI: Remove superfluous pci_set_drvdata(pci, NULL) at remove")
and partially added back (to azx_remove) by:
e81478bbe7a1 ("ALSA: hda: fix general protection fault in azx_runtime_idle")
I guess, it should do not harm to add it back...
> snd_card_free(card);
> return err;
> }
Reviewed-by: Peter Ujfalusi <[email protected]>
Hi Maarten,
On 19/07/2023 19:41, 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.
Apart from the few comments I had, this looks great and works OK on the
machines I have tested (iow, no regression so far).
Thank you for the work!
--
Péter
>
> ---
> New since first version:
>
> - snd_hda_core.gpu_bind is added as a mechanism to force gpu binding,
> for testing. snd_hda_core.gpu_bind=0 forces waiting for GPU bind to
> off, snd_hda_core.gpu_bind=1 forces waiting for gpu bind. Default
> setting depends on whether kernel booted with nomodeset.
> - Incorporated all feedback review.
>
> 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 (9):
> 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.
> 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
>
> sound/hda/hdac_i915.c | 25 ++++++++-------
> sound/pci/hda/hda_intel.c | 60 ++++++++++++++++++-----------------
> 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, 85 insertions(+), 142 deletions(-)
>
On 19/07/2023 19:41, Maarten Lankhorst wrote:
> 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.
Reviewed-by: Peter Ujfalusi <[email protected]>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> ---
> 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 961fcd3397f40..12c1f8d93499f 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;
--
Péter
On 19/07/2023 19:41, 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.
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> Acked-by: Matthew Auld <[email protected]>
> Acked-by: Mark Brown <[email protected]>
> ---
> 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);
While at it, can you drop thee "error:" prefix from the print?
> 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");
From here also, let's not add them back.
> + 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);
--
Péter
On 7/19/23 18:41, 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.
I personally remember only that the request_module("i915") was the main
motivation for the use of the workqueue, but when it comes to the
HDaudio codec management we don't even know what we don't know.
I am a bit worried that the snd-hda-intel driver keeps the workqueue for
HDaudio codec initialization, and this patch removes the workqueue
completely for SOF. That doesn't seem right. Either both drivers need a
workqueue or none need a workqueue.
Maybe what we need is to move the i915/xe initialization out of the
workqueue, and see in a second pass if that workqueue can be safely
removed from the SOF driver?
> Signed-off-by: Maarten Lankhorst <[email protected]>
> Acked-by: Matthew Auld <[email protected]>
> Acked-by: Mark Brown <[email protected]>
> ---
> 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;
> + }
> +
I am not following why we have to move dmic-related code, can we try to
move this in a separate patch?
> 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 7/21/23 13:34, Péter Ujfalusi wrote:
>
>
> On 19/07/2023 19:41, Maarten Lankhorst wrote:
>> Add missing pci_set_drv to NULL call on error.
>>
>> Signed-off-by: Maarten Lankhorst <[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 ef831770ca7da..0d2d6bc6c75ef 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -2188,6 +2188,7 @@ static int azx_probe(struct pci_dev *pci,
>> return 0;
>>
>> out_free:
>> + pci_set_drvdata(pci, NULL);
> The original patch added this:
> f4c482a4d0b3 ("ALSA: hda - Fix yet another race of vga_switcheroo registration")
>
> but got removed later by:
> 20a24225d8f9 ("ALSA: PCI: Remove superfluous pci_set_drvdata(pci, NULL) at remove")
>
> and partially added back (to azx_remove) by:
> e81478bbe7a1 ("ALSA: hda: fix general protection fault in azx_runtime_idle")
>
> I guess, it should do not harm to add it back...
>
>> snd_card_free(card);
>> return err;
>> }
>
> Reviewed-by: Peter Ujfalusi <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
On 7/19/23 18:41, Maarten Lankhorst wrote:
> 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.
I can't figure out what this comment means.
how would the -EPROBE_DEFER mechanism help if the driver that will
trigger a new probe is not upstream?
Not following at all what you intended to explain.
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> ---
> 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 961fcd3397f40..12c1f8d93499f 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;
On 7/19/23 18:41, 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.
>
> Changes since v1:
> - Use dev_err_probe()
> - Don't move probed_devs bitmap unnecessarily. (tiwai)
> - Move snd_hdac_i915_init slightly upward, to ensure
> it's always initialised before vga-switcheroo is called.
>
> Signed-off-by: Maarten Lankhorst <[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 11cf9907f039f..e3128d7d742e7 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_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
Is it intentional that the display_power() is left in the probe workqueue?
this piece of code follows the stuff above in the existing code?
/* 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
* display codec needs the power and it can be released after probe.
*/
display_power(chip, true);
> +
> err = register_vga_switcheroo(chip);
> if (err < 0) {
> dev_err(card->dev, "Error registering vga_switcheroo client\n");
> @@ -2174,11 +2204,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);
>
> @@ -2275,30 +2300,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
Hey,
On 2023-07-24 12:28, Pierre-Louis Bossart wrote:
>
>
> On 7/19/23 18:41, Maarten Lankhorst wrote:
>> 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.
>
> I can't figure out what this comment means.
>
> how would the -EPROBE_DEFER mechanism help if the driver that will
> trigger a new probe is not upstream?
>
> Not following at all what you intended to explain.
What I mean is that there is code inside the current code that does
request_module("i915"), the comment meant I didn't try to add any logic
for request_module("xe"), as the driver is not merged yet.
Additionally I am removing the request_module logic, but this comment
was written when I first tried the simple solution of request_module("xe").
Turns out telepathy is hard, and using -EPROBE_DEFER is much simpler. :-)
Cheers,
~Maarten
Hey,
On 2023-07-24 13:32, Pierre-Louis Bossart wrote:
>
>
> On 7/19/23 18:41, 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.
>
> I personally remember only that the request_module("i915") was the main
> motivation for the use of the workqueue, but when it comes to the
> HDaudio codec management we don't even know what we don't know.
>
> I am a bit worried that the snd-hda-intel driver keeps the workqueue for
> HDaudio codec initialization, and this patch removes the workqueue
> completely for SOF. That doesn't seem right. Either both drivers need a
> workqueue or none need a workqueue.
>
> Maybe what we need is to move the i915/xe initialization out of the
> workqueue, and see in a second pass if that workqueue can be safely
> removed from the SOF driver?
>
As I mentioned in some of the other sound driver conversions. I believe
it's possible to completely kill off most workqueues.
However, I don´t have the hardware or knowledge to test it. I saw that
the SOF had the non-workqueue path already, so it felt less risky to
simply convert it to always use that path.
avs/skylake drivers should be easy to convert too. This is why I left
the comment: "Removing the workqueue would simplify init even further,
but is left as exercise for the reviewer."
HDA-intel has this retry-probe logic used on AMD's,
which makes me more hesitant to convert it.
I wanted to tackle one problem at a time, I believe workqueue removal
can be done by anyone.
Cheers,
~Maarten
Hey,
On 2023-07-24 12:58, Pierre-Louis Bossart wrote:
>
>
> On 7/19/23 18:41, 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.
>>
>> Changes since v1:
>> - Use dev_err_probe()
>> - Don't move probed_devs bitmap unnecessarily. (tiwai)
>> - Move snd_hdac_i915_init slightly upward, to ensure
>> it's always initialised before vga-switcheroo is called.
>>
>> Signed-off-by: Maarten Lankhorst <[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 11cf9907f039f..e3128d7d742e7 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_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
>
> Is it intentional that the display_power() is left in the probe workqueue?
>
> this piece of code follows the stuff above in the existing code?
>
> /* 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
> * display codec needs the power and it can be released after probe.
> */
> display_power(chip, true);
I think for the binding itself, there isn't any harm. We are not poking
any hardware when binding,
only software. This appears to be true on the i915 side as well.
Cheers,
~Maarten
On Tue, 25 Jul 2023 12:29:07 +0200,
Maarten Lankhorst wrote:
>
> Hey,
>
> On 2023-07-24 13:32, Pierre-Louis Bossart wrote:
> >
> >
> > On 7/19/23 18:41, 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.
> >
> > I personally remember only that the request_module("i915") was the main
> > motivation for the use of the workqueue, but when it comes to the
> > HDaudio codec management we don't even know what we don't know.
> >
> > I am a bit worried that the snd-hda-intel driver keeps the workqueue for
> > HDaudio codec initialization, and this patch removes the workqueue
> > completely for SOF. That doesn't seem right. Either both drivers need a
> > workqueue or none need a workqueue.
> >
> > Maybe what we need is to move the i915/xe initialization out of the
> > workqueue, and see in a second pass if that workqueue can be safely
> > removed from the SOF driver?
> >
> As I mentioned in some of the other sound driver conversions. I
> believe it's possible to completely kill off most workqueues.
>
> However, I don?t have the hardware or knowledge to test it. I saw
> that the SOF had the non-workqueue path already, so it felt less risky
> to simply convert it to always use that path.
>
> avs/skylake drivers should be easy to convert too. This is why I left
> the comment: "Removing the workqueue would simplify init even further,
> but is left as exercise for the reviewer."
>
> HDA-intel has this retry-probe logic used on AMD's,
> which makes me more hesitant to convert it.
Yes, HDA-Intel requires either a workqueue or async firmware-loader
callback because there is some codec module-autoload mechanism that
may happen during the probe phase. It's not only on AMD, but it's
required in general for all codecs.
Takashi
On Wed, 19 Jul 2023 18:41:32 +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.
>
> ---
> New since first version:
>
> - snd_hda_core.gpu_bind is added as a mechanism to force gpu binding,
> for testing. snd_hda_core.gpu_bind=0 forces waiting for GPU bind to
> off, snd_hda_core.gpu_bind=1 forces waiting for gpu bind. Default
> setting depends on whether kernel booted with nomodeset.
> - Incorporated all feedback review.
Maarten, are you working on v3 patch set?
Or, for moving forward, should we merge v2 now and fix the rest based
on that later?
thanks,
Takashi
Hey,
Den 2023-07-31 kl. 17:51, skrev Takashi Iwai:
> On Wed, 19 Jul 2023 18:41:32 +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.
>>
>> ---
>> New since first version:
>>
>> - snd_hda_core.gpu_bind is added as a mechanism to force gpu binding,
>> for testing. snd_hda_core.gpu_bind=0 forces waiting for GPU bind to
>> off, snd_hda_core.gpu_bind=1 forces waiting for gpu bind. Default
>> setting depends on whether kernel booted with nomodeset.
>> - Incorporated all feedback review.
> Maarten, are you working on v3 patch set?
> Or, for moving forward, should we merge v2 now and fix the rest based
> on that later?
I've been working on a small change to keep the workqueue in SOF and
only move the binding to the probe function to match what snd-hda-intel
is doing, but I don't know if that is needed?
It was a bit unclear to me based on feedback if I should try to kill the
workqueue on all drivers (but with no way to test), or keep it around.
Cheers,
~Maarten
On Mon, 31 Jul 2023 18:37:36 +0200,
Maarten Lankhorst wrote:
>
> Hey,
>
> Den 2023-07-31 kl. 17:51, skrev Takashi Iwai:
> > On Wed, 19 Jul 2023 18:41:32 +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.
> >>
> >> ---
> >> New since first version:
> >>
> >> - snd_hda_core.gpu_bind is added as a mechanism to force gpu binding,
> >> for testing. snd_hda_core.gpu_bind=0 forces waiting for GPU bind to
> >> off, snd_hda_core.gpu_bind=1 forces waiting for gpu bind. Default
> >> setting depends on whether kernel booted with nomodeset.
> >> - Incorporated all feedback review.
> > Maarten, are you working on v3 patch set?
> > Or, for moving forward, should we merge v2 now and fix the rest based
> > on that later?
>
> I've been working on a small change to keep the workqueue in SOF and
> only move the binding to the probe function to match what
> snd-hda-intel is doing, but I don't know if that is needed?
>
> It was a bit unclear to me based on feedback if I should try to kill
> the workqueue on all drivers (but with no way to test), or keep it
> around.
I guess it's still safer to keep the workqueue in many drivers. There
can be modprobe or firmware loading at any later stage.
We can get rid of the workqueue once after confirming that it's really
safe, too.
So, if you can work on the patch set in that regard, it's fine, I can
wait for it.
thanks,
Takashi
Hey,
Den 2023-07-31 kl. 21:32, skrev Takashi Iwai:
> On Mon, 31 Jul 2023 18:37:36 +0200,
> Maarten Lankhorst wrote:
>> Hey,
>>
>> Den 2023-07-31 kl. 17:51, skrev Takashi Iwai:
>>> On Wed, 19 Jul 2023 18:41:32 +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.
>>>>
>>>> ---
>>>> New since first version:
>>>>
>>>> - snd_hda_core.gpu_bind is added as a mechanism to force gpu binding,
>>>> for testing. snd_hda_core.gpu_bind=0 forces waiting for GPU bind to
>>>> off, snd_hda_core.gpu_bind=1 forces waiting for gpu bind. Default
>>>> setting depends on whether kernel booted with nomodeset.
>>>> - Incorporated all feedback review.
>>> Maarten, are you working on v3 patch set?
>>> Or, for moving forward, should we merge v2 now and fix the rest based
>>> on that later?
>> I've been working on a small change to keep the workqueue in SOF and
>> only move the binding to the probe function to match what
>> snd-hda-intel is doing, but I don't know if that is needed?
>>
>> It was a bit unclear to me based on feedback if I should try to kill
>> the workqueue on all drivers (but with no way to test), or keep it
>> around.
> I guess it's still safer to keep the workqueue in many drivers. There
> can be modprobe or firmware loading at any later stage.
> We can get rid of the workqueue once after confirming that it's really
> safe, too.
>
> So, if you can work on the patch set in that regard, it's fine, I can
> wait for it.
I've finished that patch, but it caused regressions (oops) while
rebooting. I think it's safer to kill the workqueue for SOC, and then
convert all other drivers later.
Cheers,
~Maarten
> I've been working on a small change to keep the workqueue in SOF and
> only move the binding to the probe function to match what snd-hda-intel
> is doing, but I don't know if that is needed?
>
> It was a bit unclear to me based on feedback if I should try to kill the
> workqueue on all drivers (but with no way to test), or keep it around.
My understanding is that we only want to move the binding to the probe
function and leave the workqueue removal for another day - possibly never.
Hey,
On 2023-08-01 18:32, Pierre-Louis Bossart wrote:
>
>> I've been working on a small change to keep the workqueue in SOF and
>> only move the binding to the probe function to match what snd-hda-intel
>> is doing, but I don't know if that is needed?
>>
>> It was a bit unclear to me based on feedback if I should try to kill the
>> workqueue on all drivers (but with no way to test), or keep it around.
>
> My understanding is that we only want to move the binding to the probe
> function and leave the workqueue removal for another day - possibly never.
Patch 8/9 removed the workqueue, but can be replaced by the patch below,
that simply moves out snd_sof_probe().
I've attempted this before, but didn't have snd_sof_remove in
snd_sof_device_remove, which is why I would get a OOPS when attempting
to do a shutdown/reboot.
With that I hopefully addressed the last concern.
Cheers,
Maarten
This mail can be applied with git am -c.
------8<---------
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]>
---
sound/soc/sof/core.c | 19 +++++++------------
sound/soc/sof/intel/hda-codec.c | 2 +-
2 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index 30db685cc5f4b..cd4d06d1800b1 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -188,13 +188,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
struct snd_sof_pdata *plat_data = sdev->pdata;
int ret;
- /* probe the DSP hardware */
- ret = snd_sof_probe(sdev);
- if (ret < 0) {
- dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret);
- goto probe_err;
- }
-
sof_set_fw_state(sdev, SOF_FW_BOOT_PREPARE);
/* check machine info */
@@ -325,10 +318,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
dbg_err:
snd_sof_free_debug(sdev);
dsp_err:
- snd_sof_remove(sdev);
-probe_err:
- sof_ops_free(sdev);
-
/* all resources freed, update state to match */
sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);
sdev->first_boot = true;
@@ -436,6 +425,12 @@ int snd_sof_device_probe(struct device *dev, struct
snd_sof_pdata *plat_data)
sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);
+ ret = snd_sof_probe(sdev);
+ if (ret) {
+ dev_err_probe(sdev->dev, ret, "failed to probe DSP\n");
+ 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);
@@ -485,9 +480,9 @@ 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(sdev);
sof_ops_free(sdev);
/* release firmware */
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;
--
2.39.2
On Fri, Aug 04, 2023 at 12:47:54PM +0200, Maarten Lankhorst wrote:
> On 2023-08-01 18:32, Pierre-Louis Bossart wrote:
> This mail can be applied with git am -c.
> ------8<---------
> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
Don't do this, it breaks my automation and means I very nearly just
skipped the patch entirely since it looked like the middle of some x86
discussion.
On Fri, Aug 04, 2023 at 04:31:21PM +0200, Maarten Lankhorst wrote:
> Den 2023-08-04 kl. 13:59, skrev Mark Brown:
> > > On 2023-08-01 18:32, Pierre-Louis Bossart wrote:
> > > This mail can be applied with git am -c.
> > > ------8<---------
> > > Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> > Don't do this, it breaks my automation and means I very nearly just
> > skipped the patch entirely since it looked like the middle of some x86
> > discussion.
> Yeah, it's replacing the patch from earlier. I can resend, but means having
> to add all acks, r-b'd, etc. :)
*Defintely* do not do that:
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.
> If you have scripts that do that, all the better.
If you're using b4 then b4 trailers --update.
Hey,
Den 2023-08-04 kl. 13:59, skrev Mark Brown:
> On Fri, Aug 04, 2023 at 12:47:54PM +0200, Maarten Lankhorst wrote:
>> On 2023-08-01 18:32, Pierre-Louis Bossart wrote:
>> This mail can be applied with git am -c.
>> ------8<---------
>> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> Don't do this, it breaks my automation and means I very nearly just
> skipped the patch entirely since it looked like the middle of some x86
> discussion.
Yeah, it's replacing the patch from earlier. I can resend, but means
having to add all acks, r-b'd, etc. :)
If you have scripts that do that, all the better.
Cheers,
~Maarten