From: Maarten Lankhorst <[email protected]>
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.
---
Changes since previous version:
- Keep the workqueue for soc/sof/intel, instead only move out
snd_hdac_i915_init. (Patch 8/9) Rest of patches unchanged.
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: Move binding to display driver outside of deferred
probe
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/core.c | 19 ++++-------
5 files changed, 70 insertions(+), 78 deletions(-)
--
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]>
Reviewed-by: Kai Vehmanen <[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 3311a6f14200..64e7a4e650a8 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
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 30db685cc5f4..cd4d06d1800b 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 f1fd5b44aaac..344b61576c0e 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
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]>
Reviewed-by: Peter Ujfalusi <[email protected]>
Reviewed-by: Kai Vehmanen <[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 961fcd3397f4..12c1f8d93499 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.
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]>
Reviewed-by: Kai Vehmanen <[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 11cf9907f039..e3128d7d742e 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
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]>
---
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 4d93b8690467..ff80d83a9fb7 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 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]>
Reviewed-by: Kai Vehmanen <[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 f91bd6636086..6b79614a893b 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -9,12 +9,12 @@
#ifdef CONFIG_SND_HDA_I915
void snd_hdac_i915_set_bclk(struct hdac_bus *bus);
-int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe);
+int snd_hdac_i915_init(struct hdac_bus *bus);
#else
static inline void snd_hdac_i915_set_bclk(struct hdac_bus *bus)
{
}
-static inline int snd_hdac_i915_init(struct hdac_bus *bus, bool allow_modprobe)
+static inline int snd_hdac_i915_init(struct hdac_bus *bus)
{
return -ENODEV;
}
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 12c1f8d93499..ad13f0e2f94f 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 e3128d7d742e..b4fa925a992b 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 64e7a4e650a8..d350204a1d86 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 ff80d83a9fb7..49147ee3a76d 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 344b61576c0e..8a5e99a898ec 100644
--- a/sound/soc/sof/intel/hda-codec.c
+++ b/sound/soc/sof/intel/hda-codec.c
@@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev)
return 0;
/* i915 exposes a HDA codec for HDMI audio */
- ret = snd_hdac_i915_init(bus, false);
+ ret = snd_hdac_i915_init(bus);
if (ret < 0)
return ret;
--
2.39.2
On 8/7/23 04:00, 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.
I don't think this patch is aligned with the previous discussions. What
we agreed on is that snd_hdac_i915_init() would be called from and not
from the workqueue.
But this patch also moves all codec initialization out of the workqueue.
I think we need two callbacks for device-specific initilization, one
that is called from the probe function and one from the workqueue,
otherwise we'll have a structure that differs from the snd-hda-intel -
which would be rather silly in terms of support/debug.
I realize there's quite a bit of surgery involved, and most likely the
SOF folks should provide this patch for you to build on.
> 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 30db685cc5f4..cd4d06d1800b 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 f1fd5b44aaac..344b61576c0e 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;
>
On 8/7/23 04:00, 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.
same issue with changes.
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> Reviewed-by: Kai Vehmanen <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[email protected]>
> @@ -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));
heads-up that I have a conflicting patch to make the 60s delay
configurable, see https://github.com/thesofproject/linux/pull/4505
> - }
> - }
> 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;
> }
On 8/7/23 04:00, 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.
The wording hasn't changed and remains confusing, likely to trigger all
paranoia triplines. Consider rewording if there's an update.
> 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 | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 961fcd3397f4..12c1f8d93499 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 8/7/23 04:00, Maarten Lankhorst wrote:
> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue
> can be destroyed, but I don't have the means to test this.
>
> Removing the workqueue would simplify init even further, but is left
> as exercise for the reviewer.
>
> Changes since v1:
> - Rename error label.
same issue with changes, they need to be ...
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Kai Vehmanen <[email protected]>> ---
...here
> 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 3311a6f14200..64e7a4e650a8 100644
> --- a/sound/soc/intel/avs/core.c
> +++ b/sound/soc/intel/avs/core.c
> @@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work)
>
> pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
>
> - ret = snd_hdac_i915_init(bus, true);
> - if (ret < 0)
> - dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
> -
> snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
> avs_hdac_bus_init_chip(bus, true);
> avs_hdac_bus_probe_codecs(bus);
> @@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
> pci_set_drvdata(pci, bus);
> device_disable_async_suspend(dev);
>
> + ret = snd_hdac_i915_init(bus, false);
> + if (ret == -EPROBE_DEFER)
> + goto err_i915_init;
> + else if (ret < 0)
> + dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
> +
> schedule_work(&adev->probe_work);
>
> return 0;
>
> +err_i915_init:
> + pci_clear_master(pci);
> + pci_set_drvdata(pci, NULL);
> err_acquire_irq:
> snd_hdac_bus_free_stream_pages(bus);
> snd_hdac_ext_stream_free_all(bus);
On Mon, 07 Aug 2023 16:26:53 +0200,
Pierre-Louis Bossart wrote:
>
>
>
> On 8/7/23 04:00, 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.
>
> I don't think this patch is aligned with the previous discussions. What
> we agreed on is that snd_hdac_i915_init() would be called from and not
> from the workqueue.
>
> But this patch also moves all codec initialization out of the workqueue.
>
> I think we need two callbacks for device-specific initilization, one
> that is called from the probe function and one from the workqueue,
> otherwise we'll have a structure that differs from the snd-hda-intel -
> which would be rather silly in terms of support/debug.
>
> I realize there's quite a bit of surgery involved, and most likely the
> SOF folks should provide this patch for you to build on.
So this patch looks like the only significant concern in the whole
patch set. Can we reach to some agreement for merging to 6.6 in time?
thanks,
Takashi
Ping on this?
On 2023-08-12 10:17, Takashi Iwai wrote:
> On Mon, 07 Aug 2023 16:26:53 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>
>> On 8/7/23 04:00, 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.
>>
>> I don't think this patch is aligned with the previous discussions. What
>> we agreed on is that snd_hdac_i915_init() would be called from and not
>> from the workqueue.
>>
>> But this patch also moves all codec initialization out of the workqueue.
>>
>> I think we need two callbacks for device-specific initilization, one
>> that is called from the probe function and one from the workqueue,
>> otherwise we'll have a structure that differs from the snd-hda-intel -
>> which would be rather silly in terms of support/debug.
>>
>> I realize there's quite a bit of surgery involved, and most likely the
>> SOF folks should provide this patch for you to build on.
>
> So this patch looks like the only significant concern in the whole
> patch set. Can we reach to some agreement for merging to 6.6 in time?
>
>
> thanks,
>
> Takashi