Explicitly loading i915 becomes a problem when upstreaming the new intel driver
for Tiger Lake and higher graphics (xe). By loading i915, it doesn't wait for
driver load of xe, and will fail completely before it loads.
-EPROBE_DEFER has to be returned before any device is created in probe(),
otherwise the removal of the device will cause EPROBE_DEFER to try again
in an infinite loop.
The conversion is done in gradual steps. First I add an argument to
snd_hdac_i915_init to allow for -EPROBE_DEFER so I can convert each driver
separately. Then I convert each driver to move snd_hdac_i915_init out of the
workqueue. Finally I drop the ability to choose modprobe behavior after the
last user is converted.
Compared to previous version, I added a fix for sof_ops_free() missing call,
renamed probe_no_wq and remove_no_wq to probe_early/probe_late, and fixed
the resulting fallout.
Cc: Jaroslav Kysela <[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 (10):
ASoC/SOF/core: Ensure sof_ops_free() is still called when probe never
ran.
ALSA: hda/intel: Fix error handling in azx_probe()
ALSA: hda/i915: Allow override of gpu binding.
ALSA: hda/i915: Add an allow_modprobe argument to snd_hdac_i915_init
ALSA: hda/i915: Allow xe as match for i915_component_master_match
ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work.
ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.
ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work.
ASoC: SOF: Intel: Move binding to display driver outside of deferred
probe
ALSA: hda/i915: Remove extra argument from snd_hdac_i915_init
Pierre-Louis Bossart (2):
ASoC: SOF: core: Add probe_early and remove_late callbacks
ASoC: SOF: Intel: hda: start splitting the probe
sound/hda/hdac_i915.c | 24 ++++++-----
sound/pci/hda/hda_intel.c | 60 ++++++++++++++--------------
sound/soc/intel/avs/core.c | 13 ++++--
sound/soc/intel/skylake/skl.c | 31 +++++---------
sound/soc/sof/core.c | 17 +++++++-
sound/soc/sof/intel/hda-common-ops.c | 2 +
sound/soc/sof/intel/hda.c | 30 +++++++++-----
sound/soc/sof/intel/hda.h | 2 +
sound/soc/sof/ops.h | 16 ++++++++
sound/soc/sof/sof-priv.h | 2 +
10 files changed, 119 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.
Signed-off-by: Maarten Lankhorst <[email protected]>
Acked-by: Mark Brown <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
Reviewed-by: Amadeusz Sławiński <[email protected]>
---
sound/soc/intel/avs/core.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index bbb40339c75f4..8a20639582487 100644
--- a/sound/soc/intel/avs/core.c
+++ b/sound/soc/intel/avs/core.c
@@ -191,10 +191,6 @@ static void avs_hda_probe_work(struct work_struct *work)
pm_runtime_set_active(bus->dev); /* clear runtime_error flag */
- ret = snd_hdac_i915_init(bus, true);
- if (ret < 0)
- dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
-
snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
avs_hdac_bus_init_chip(bus, true);
avs_hdac_bus_probe_codecs(bus);
@@ -465,10 +461,19 @@ static int avs_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
pci_set_drvdata(pci, bus);
device_disable_async_suspend(dev);
+ ret = snd_hdac_i915_init(bus, false);
+ if (ret == -EPROBE_DEFER)
+ goto err_i915_init;
+ else if (ret < 0)
+ dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret);
+
schedule_work(&adev->probe_work);
return 0;
+err_i915_init:
+ pci_clear_master(pci);
+ pci_set_drvdata(pci, NULL);
err_acquire_irq:
snd_hdac_bus_free_stream_pages(bus);
snd_hdac_ext_stream_free_all(bus);
--
2.39.2
In an effort to not call sof_ops_free twice, we stopped running it when
probe was aborted.
Check the result of cancel_work_sync to see if this was the case.
Fixes: 31bb7bd9ffee ("ASoC: SOF: core: Only call sof_ops_free() on remove if the probe was successful")
Cc: Peter Ujfalusi <[email protected]>
---
sound/soc/sof/core.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index 2d1616b81485c..0938b259f7034 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -459,9 +459,10 @@ int snd_sof_device_remove(struct device *dev)
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
struct snd_sof_pdata *pdata = sdev->pdata;
int ret;
+ bool aborted = false;
if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
- cancel_work_sync(&sdev->probe_work);
+ aborted = cancel_work_sync(&sdev->probe_work);
/*
* Unregister any registered client device first before IPC and debugfs
@@ -487,6 +488,9 @@ int snd_sof_device_remove(struct device *dev)
snd_sof_free_debug(sdev);
snd_sof_remove(sdev);
sof_ops_free(sdev);
+ } else if (aborted) {
+ /* probe_work never ran */
+ sof_ops_free(sdev);
}
/* release firmware */
--
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.
Signed-off-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
Reviewed-by: Pierre-Louis Bossart <[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 a4a712c795c3d..ffa35d7a367c0 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 b4ac0d43c09bc..bfe6232bb0e0e 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2266,7 +2266,7 @@ static int azx_probe_continue(struct azx *chip)
/* bind with i915 if needed */
if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
- err = snd_hdac_i915_init(bus);
+ err = snd_hdac_i915_init(bus, true);
if (err < 0) {
/* if the controller is bound only with HDMI/DP
* (for HSW and BDW), we need to abort the probe;
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 859b217fc761b..bbb40339c75f4 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 77408a981b977..4f7acb4f6680b 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
On Fri, Sep 29, 2023 at 04:51:14PM +0200, Maarten Lankhorst wrote:
> ASoC/SOF/core: Ensure sof_ops_free() is still called when probe never
> ran.
> ASoC: SOF: Intel: Move binding to display driver outside of deferred
> probe
> ASoC: SOF: core: Add probe_early and remove_late callbacks
> ASoC: SOF: Intel: hda: start splitting the probe
The subject line styles aren't consistent within the series here.
On Fri, Sep 29, 2023 at 04:51:15PM +0200, Maarten Lankhorst wrote:
> In an effort to not call sof_ops_free twice, we stopped running it when
> probe was aborted.
Acked-by: Mark Brown <[email protected]>
Now that we can use -EPROBE_DEFER, it's no longer required to spin off
the snd_hdac_i915_init into a workqueue.
Use the -EPROBE_DEFER mechanism instead, which must be returned in the
probe function.
The previously added probe_early can be used for this,
and we also use the newly added remove_late for unbinding afterwards.
Signed-off-by: Maarten Lankhorst <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
---
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Cezary Rojewski <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Peter Ujfalusi <[email protected]>
Cc: Bard Liao <[email protected]>
Cc: Ranjani Sridharan <[email protected]>
Cc: Kai Vehmanen <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Daniel Baluta <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
sound/soc/sof/intel/hda-common-ops.c | 1 +
sound/soc/sof/intel/hda.c | 14 ++++++--------
sound/soc/sof/intel/hda.h | 1 +
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c
index 1cc18fb2b75bb..26105d8f1bdc7 100644
--- a/sound/soc/sof/intel/hda-common-ops.c
+++ b/sound/soc/sof/intel/hda-common-ops.c
@@ -19,6 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
.probe_early = hda_dsp_probe_early,
.probe = hda_dsp_probe,
.remove = hda_dsp_remove,
+ .remove_late = hda_dsp_remove_late,
/* Register IO uses direct mmio */
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 86a2571488bcc..32ac0581f9f63 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -1160,6 +1160,7 @@ int hda_dsp_probe_early(struct snd_sof_dev *sdev)
return -ENOMEM;
sdev->pdata->hw_pdata = hdev;
hdev->desc = chip;
+ ret = hda_init(sdev);
err:
return ret;
@@ -1195,9 +1196,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
/* set up HDA base */
bus = sof_to_bus(sdev);
- ret = hda_init(sdev);
- if (ret < 0)
- goto hdac_bus_unmap;
if (sdev->dspless_mode_selected)
goto skip_dsp_setup;
@@ -1307,8 +1305,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
iounmap(sdev->bar[HDA_DSP_BAR]);
hdac_bus_unmap:
platform_device_unregister(hdev->dmic_dev);
- iounmap(bus->remap_addr);
- hda_codec_i915_exit(sdev);
return ret;
}
@@ -1317,7 +1313,6 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
const struct sof_intel_dsp_desc *chip = hda->desc;
- struct hdac_bus *bus = sof_to_bus(sdev);
struct pci_dev *pci = to_pci_dev(sdev->dev);
struct nhlt_acpi_table *nhlt = hda->nhlt;
@@ -1368,10 +1363,13 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
if (!sdev->dspless_mode_selected)
iounmap(sdev->bar[HDA_DSP_BAR]);
- iounmap(bus->remap_addr);
+ return 0;
+}
+int hda_dsp_remove_late(struct snd_sof_dev *sdev)
+{
+ iounmap(sof_to_bus(sdev)->remap_addr);
sof_hda_bus_exit(sdev);
-
hda_codec_i915_exit(sdev);
return 0;
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index e13cdc933ca6b..8e846684279e7 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -576,6 +576,7 @@ struct sof_intel_hda_stream {
int hda_dsp_probe_early(struct snd_sof_dev *sdev);
int hda_dsp_probe(struct snd_sof_dev *sdev);
int hda_dsp_remove(struct snd_sof_dev *sdev);
+int hda_dsp_remove_late(struct snd_sof_dev *sdev);
int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);
int hda_dsp_core_run(struct snd_sof_dev *sdev, unsigned int core_mask);
int hda_dsp_enable_core(struct snd_sof_dev *sdev, unsigned int core_mask);
--
2.39.2
Now that all drivers have moved from modprobe loading to
handling -EPROBE_DEFER, we can remove the argument again.
Signed-off-by: Maarten Lankhorst <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
---
Cc: Jaroslav Kysela <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Cezary Rojewski <[email protected]>
Cc: Pierre-Louis Bossart <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Peter Ujfalusi <[email protected]>
Cc: Bard Liao <[email protected]>
Cc: Ranjani Sridharan <[email protected]>
Cc: Kai Vehmanen <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Daniel Baluta <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
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 0765e5350e7ba..365c36fdf2058 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 5cf7676114dc6..5255df16fbac5 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2138,7 +2138,7 @@ static int azx_probe(struct pci_dev *pci,
#ifdef CONFIG_SND_HDA_I915
/* bind with i915 if needed */
if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) {
- err = snd_hdac_i915_init(azx_bus(chip), false);
+ err = snd_hdac_i915_init(azx_bus(chip));
if (err < 0) {
/* if the controller is bound only with HDMI/DP
* (for HSW and BDW), we need to abort the probe;
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
index 8a20639582487..33044f353575d 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 24bdbe2a53bec..f46f109d5856e 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 f1fd5b44aaac9..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, true);
+ ret = snd_hdac_i915_init(bus);
if (ret < 0)
return ret;
--
2.39.2
Now that we can use -EPROBE_DEFER, it's no longer required to spin off
the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue
can be destroyed, but I don't have the means to test this.
Removing the workqueue would simplify init even further, but is left
as exercise for the reviewer.
Signed-off-by: Maarten Lankhorst <[email protected]>
Acked-by: Mark Brown <[email protected]>
Reviewed-by: Kai Vehmanen <[email protected]>
Reviewed-by: Amadeusz Sławiński <[email protected]>
---
sound/soc/intel/skylake/skl.c | 31 +++++++++----------------------
1 file changed, 9 insertions(+), 22 deletions(-)
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]
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 4f7acb4f6680b..24bdbe2a53bec 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
On 10/2/2023 6:52 PM, Maarten Lankhorst wrote:
> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
> the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue
> can be destroyed, but I don't have the means to test this.
>
> Removing the workqueue would simplify init even further, but is left
> as exercise for the reviewer.
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> Acked-by: Mark Brown <[email protected]>
> Reviewed-by: Kai Vehmanen <[email protected]>
> Reviewed-by: Amadeusz Sławiński <[email protected]>
> ---
Isn't the convention that your Signed-off should be last when you are
sending the patches? Or does it only apply to Signed-off lines
themselves and other lines can be anywhere?
On 10/3/2023 8:55 AM, Amadeusz Sławiński wrote:
> On 10/2/2023 6:52 PM, Maarten Lankhorst wrote:
>> Now that we can use -EPROBE_DEFER, it's no longer required to spin off
>> the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue
>> can be destroyed, but I don't have the means to test this.
>>
>> Removing the workqueue would simplify init even further, but is left
>> as exercise for the reviewer.
>>
>> Signed-off-by: Maarten Lankhorst <[email protected]>
>> Acked-by: Mark Brown <[email protected]>
>> Reviewed-by: Kai Vehmanen <[email protected]>
>> Reviewed-by: Amadeusz Sławiński <[email protected]>
>> ---
>
> Isn't the convention that your Signed-off should be last when you are
> sending the patches? Or does it only apply to Signed-off lines
> themselves and other lines can be anywhere?
>
And rereading kernel documentation, it really seems to be a bit
ambiguous, well ignore the comment then.