[PATCH v4 00/11] sound: Use -EPROBE_DEFER instead of i915 module loading.

older
[PATCH] ALSA: core: Use dev_name...

Maarten Lankhorst

30 Aug 2023 30 Aug '23

5:36 p.m.

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 picked up Pierre's changes to add a probe_no_wq, added remove_no_wq myself and then fixed the SOF changes to use it.

Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Cezary Rojewski cezary.rojewski@intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Liam Girdwood liam.r.girdwood@linux.intel.com Cc: Peter Ujfalusi peter.ujfalusi@linux.intel.com Cc: Bard Liao yung-chuan.liao@linux.intel.com Cc: Ranjani Sridharan ranjani.sridharan@linux.intel.com Cc: Kai Vehmanen kai.vehmanen@linux.intel.com Cc: Mark Brown broonie@kernel.org Cc: Daniel Baluta daniel.baluta@nxp.com Cc: alsa-devel@alsa-project.org Cc: linux-kernel@vger.kernel.org Cc: sound-open-firmware@alsa-project.org

Maarten Lankhorst (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

Pierre-Louis Bossart (2): ASoC: SOF: core: add 'no_wq' probe and remove callbacks ASoC: SOF: Intel: hda: start splitting the probe

-- 2.39.2

Show replies by date

Maarten Lankhorst

30 Aug 30 Aug

5:36 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

The existing DSP probe may be handled in a workqueue to allow for extra time, typically for the i915 request_module and HDAudio codec handling.

With the upcoming changes for i915/Xe driver relying on the -EPROBE_DEFER mechanism, we need to have a first pass of the probe which cannot be pushed to a workqueue. Introduce 2 new optional callbacks.

Note that instead of probe_no_wq/probe we could have use a more self-explanatory naming such as probe/probe_wq_allowed, but that would have been a very intrusive change.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- sound/soc/sof/core.c | 11 +++++++++-- sound/soc/sof/ops.h | 16 ++++++++++++++++ sound/soc/sof/sof-priv.h | 2 ++ 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..54c384a5d6140 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -327,8 +327,6 @@ static int sof_probe_continue(struct snd_sof_dev *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 +434,14 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)

sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);

+ /* + * first pass of probe which isn't allowed to run in a work-queue, + * typically to rely on -EPROBE_DEFER dependencies + */ + ret = snd_sof_probe_no_wq(sdev); + if (ret < 0) + return ret; + if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) { INIT_WORK(&sdev->probe_work, sof_probe_work); schedule_work(&sdev->probe_work); @@ -487,6 +493,7 @@ int snd_sof_device_remove(struct device *dev) snd_sof_free_debug(sdev); snd_sof_remove(sdev); } + snd_sof_remove_no_wq(sdev);

sof_ops_free(sdev);

diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h index 9ab7b9be765bc..2a03b152e9313 100644 --- a/sound/soc/sof/ops.h +++ b/sound/soc/sof/ops.h @@ -38,6 +38,14 @@ static inline void sof_ops_free(struct snd_sof_dev *sdev) /* Mandatory operations are verified during probing */

/* init */ +static inline int snd_sof_probe_no_wq(struct snd_sof_dev *sdev) +{ + if (sof_ops(sdev)->probe_no_wq) + return sof_ops(sdev)->probe_no_wq(sdev); + + return 0; +} + static inline int snd_sof_probe(struct snd_sof_dev *sdev) { return sof_ops(sdev)->probe(sdev); @@ -51,6 +59,14 @@ static inline int snd_sof_remove(struct snd_sof_dev *sdev) return 0; }

+static inline int snd_sof_remove_no_wq(struct snd_sof_dev *sdev) +{ + if (sof_ops(sdev)->remove_no_wq) + return sof_ops(sdev)->remove_no_wq(sdev); + + return 0; +} + static inline int snd_sof_shutdown(struct snd_sof_dev *sdev) { if (sof_ops(sdev)->shutdown) diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index d4f6702e93dcb..addaef282ee92 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -165,6 +165,8 @@ struct sof_firmware { struct snd_sof_dsp_ops {

/* probe/remove/shutdown */ + int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */ + int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */ int (*probe)(struct snd_sof_dev *sof_dev); /* mandatory */ int (*remove)(struct snd_sof_dev *sof_dev); /* optional */ int (*shutdown)(struct snd_sof_dev *sof_dev); /* optional */

-- 2.39.2

Pierre-Louis Bossart

7:13 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

...

+static inline int snd_sof_remove_no_wq(struct snd_sof_dev *sdev) +{
if (sof_ops(sdev)->remove_no_wq)
return sof_ops(sdev)->remove_no_wq(sdev);
return 0;
+}

...

/* probe/remove/shutdown */

int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */

int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */

My initial PR didn't have this remove_no_wq() callback.

For symmetry it could be useful if it is meant to undo what the probe_no_wq() did, but conceptually the first thing we do in the remove is make sure that workqueue is either not scheduled or we wait until it completes.

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);

Maarten Lankhorst

31 Aug 31 Aug

12:44 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

Hey,

Den 2023-08-30 kl. 19:13, skrev Pierre-Louis Bossart:

...

...
+static inline int snd_sof_remove_no_wq(struct snd_sof_dev *sdev) +{
if (sof_ops(sdev)->remove_no_wq)
return sof_ops(sdev)->remove_no_wq(sdev);
return 0;
+} /* probe/remove/shutdown */

int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */

int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */
My initial PR didn't have this remove_no_wq() callback.

For symmetry it could be useful if it is meant to undo what the probe_no_wq() did, but conceptually the first thing we do in the remove is make sure that workqueue is either not scheduled or we wait until it completes.

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);

That is exactly what I was using it for later on.

I had to have a counter to hda_init() in patch 10/11.

Cheers, Maarten

Kai Vehmanen

1 Sep 1 Sep

2:22 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

Hi,

On Wed, 30 Aug 2023, Pierre-Louis Bossart wrote:

...

...
/* probe/remove/shutdown */

int (*probe_no_wq)(struct snd_sof_dev *sof_dev); /* optional */

int (*remove_no_wq)(struct snd_sof_dev *sof_dev); /* optional */

My initial PR didn't have this remove_no_wq() callback.

For symmetry it could be useful if it is meant to undo what the probe_no_wq() did, but conceptually the first thing we do in the remove is make sure that workqueue is either not scheduled or we wait until it completes.

I think that's exactly what the patch ends up with, remove_no_wq releases resources acquired in probe_no_wq, i.e. they are symmetrical.

...

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);

So this seems ok as well: - if wq is used, at remove, we first wait wq to be finished and only then proceed with removal - the remove_no_wq is run only when the first step is completed

Br, Kai

Kai Vehmanen

2:15 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

Hi,

On Wed, 30 Aug 2023, Maarten Lankhorst wrote:

...

With the upcoming changes for i915/Xe driver relying on the -EPROBE_DEFER mechanism, we need to have a first pass of the probe which cannot be pushed to a workqueue. Introduce 2 new optional callbacks.

[...]

...

diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..54c384a5d6140 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -327,8 +327,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) dsp_err: snd_sof_remove(sdev); probe_err:

sof_ops_free(sdev);

this seems a bit out-of-place in this patch. It seems a valid change, but not really related to this patch, right?

We seem to have a related fix waiting to be sent to alsa-devel, by Peter: "ASoC: SOF: core: Only call sof_ops_free() on remove if the probe wa" https://github.com/thesofproject/linux/pull/4515

... not yet in Mark's tree.

Otherwise patch looks good to me.

Br, Kai

Péter Ujfalusi

2:44 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

On 01/09/2023 15:15, Kai Vehmanen wrote:

...

Hi,

On Wed, 30 Aug 2023, Maarten Lankhorst wrote:

...
With the upcoming changes for i915/Xe driver relying on the -EPROBE_DEFER mechanism, we need to have a first pass of the probe which cannot be pushed to a workqueue. Introduce 2 new optional callbacks.

[...]

...
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..54c384a5d6140 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -327,8 +327,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) dsp_err: snd_sof_remove(sdev); probe_err:

sof_ops_free(sdev);

this seems a bit out-of-place in this patch. It seems a valid change, but not really related to this patch, right?

The ops needs to be preserved even if the wq fails since the patch wants to call snd_sof_remove_no_wq() unconditionally on remove.

...

We seem to have a related fix waiting to be sent to alsa-devel, by Peter: "ASoC: SOF: core: Only call sof_ops_free() on remove if the probe wa" https://github.com/thesofproject/linux/pull/4515

I guess we can revert that in sof-dev, if this is the preferred way?

...

... not yet in Mark's tree.

Otherwise patch looks good to me.

I would have not created the snd_sof_remove_no_wq() as it makes not much functional sense. It might be even better if the remove in the wq would do the hda_codec_i915_exit() as the module will remain in there until the user removes it.

...

Br, Kai

-- Péter

Pierre-Louis Bossart

5 Sep 5 Sep

2:37 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

On 9/1/23 08:44, Péter Ujfalusi wrote:

...

On 01/09/2023 15:15, Kai Vehmanen wrote:

...
Hi,

On Wed, 30 Aug 2023, Maarten Lankhorst wrote:

...
With the upcoming changes for i915/Xe driver relying on the -EPROBE_DEFER mechanism, we need to have a first pass of the probe which cannot be pushed to a workqueue. Introduce 2 new optional callbacks.

[...]

...
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..54c384a5d6140 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -327,8 +327,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) dsp_err: snd_sof_remove(sdev); probe_err:

sof_ops_free(sdev);

this seems a bit out-of-place in this patch. It seems a valid change, but not really related to this patch, right?

The ops needs to be preserved even if the wq fails since the patch wants to call snd_sof_remove_no_wq() unconditionally on remove.

...
We seem to have a related fix waiting to be sent to alsa-devel, by Peter: "ASoC: SOF: core: Only call sof_ops_free() on remove if the probe wa" https://github.com/thesofproject/linux/pull/4515

I guess we can revert that in sof-dev, if this is the preferred way?

...
... not yet in Mark's tree.

Otherwise patch looks good to me.

I would have not created the snd_sof_remove_no_wq() as it makes not much functional sense. It might be even better if the remove in the wq would do the hda_codec_i915_exit() as the module will remain in there until the user removes it.

I think find all this very confusing, because there is no workqueue used in the remove steps. The workqueue is only used ONCE during the probe.

Pierre-Louis Bossart

7 Sep 7 Sep

7:29 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

On 9/5/23 08:37, Pierre-Louis Bossart wrote:

...

On 9/1/23 08:44, Péter Ujfalusi wrote:

...
On 01/09/2023 15:15, Kai Vehmanen wrote:

...
Hi,

On Wed, 30 Aug 2023, Maarten Lankhorst wrote:

...
With the upcoming changes for i915/Xe driver relying on the -EPROBE_DEFER mechanism, we need to have a first pass of the probe which cannot be pushed to a workqueue. Introduce 2 new optional callbacks.

[...]

...
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..54c384a5d6140 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -327,8 +327,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) dsp_err: snd_sof_remove(sdev); probe_err:

sof_ops_free(sdev);

this seems a bit out-of-place in this patch. It seems a valid change, but not really related to this patch, right?

The ops needs to be preserved even if the wq fails since the patch wants to call snd_sof_remove_no_wq() unconditionally on remove.

...
We seem to have a related fix waiting to be sent to alsa-devel, by Peter: "ASoC: SOF: core: Only call sof_ops_free() on remove if the probe wa" https://github.com/thesofproject/linux/pull/4515

I guess we can revert that in sof-dev, if this is the preferred way?

...
... not yet in Mark's tree.

Otherwise patch looks good to me.

I would have not created the snd_sof_remove_no_wq() as it makes not much functional sense. It might be even better if the remove in the wq would do the hda_codec_i915_exit() as the module will remain in there until the user removes it.

I think find all this very confusing, because there is no workqueue used in the remove steps. The workqueue is only used ONCE during the probe.

Maybe we should just remove any references to workqueues, and have

probe_start (cannot run in a wq) probe (may run in a wq) remove (cannot run in a wq, needs to call cancel_work_sync() if the probe runs in a wq) remove_last (cannot run in a wq, releases all resources acquired in probe_start)

Or something similar that shows the symmetry between steps and when the wq is allowed.

Péter Ujfalusi

11 Sep 11 Sep

8:51 a.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

On 07/09/2023 20:29, Pierre-Louis Bossart wrote:

...

...
I think find all this very confusing, because there is no workqueue used in the remove steps. The workqueue is only used ONCE during the probe.

Maybe we should just remove any references to workqueues, and have

probe_start (cannot run in a wq) probe (may run in a wq) remove (cannot run in a wq, needs to call cancel_work_sync() if the probe runs in a wq) remove_last (cannot run in a wq, releases all resources acquired in probe_start)

Or something similar that shows the symmetry between steps and when the wq is allowed.

What we have atm: snd_sof_probe - might be called from wq snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step)

We want a callbacks for hardware/device probing, right, split the snd_sof_probe (and remove) to be able to support a sane level of deferred probing support.

With that in mind: snd_sof_device_probe - Not called from wq (to handle deferred probing) snd_sof_probe - might be called from wq

snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step) snd_sof_device_remove - Not called from wq (to up the snd_sof_device_probe step)

Naming option: s/device/hardware

However, I think the snd_sof_device_remove itself is redundant and we might not need it at all as in case we have wq and there is a failure in there we do want to release resources as much as possible. The module will be kept loaded (no deferred handling in wq) and that might block PM, other devices to behave correctly. Iow, if the wq has failure we should do a cleanup to the best effort to reach a level like the driver is not even loaded.

Doable? I thin it is.

-- Péter

Pierre-Louis Bossart

12 Sep 12 Sep

2:25 a.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

...

What we have atm: snd_sof_probe - might be called from wq snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step)

I don't think it's correct, snd_sof_remove cannot be called from a wq. The device core knows nothing about workqueues.

...

We want a callbacks for hardware/device probing, right, split the snd_sof_probe (and remove) to be able to support a sane level of deferred probing support.

With that in mind: snd_sof_device_probe - Not called from wq (to handle deferred probing) snd_sof_probe - might be called from wq

snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step) snd_sof_device_remove - Not called from wq (to up the snd_sof_device_probe step)

Naming option: s/device/hardware

I like the 'device' hint since it's directly related to the device (or subsystem) callbacks.

...

However, I think the snd_sof_device_remove itself is redundant and we might not need it at all as in case we have wq and there is a failure in there we do want to release resources as much as possible. The module will be kept loaded (no deferred handling in wq) and that might block PM, other devices to behave correctly. Iow, if the wq has failure we should do a cleanup to the best effort to reach a level like the driver is not even loaded.

If we have a failure in a workqueue used for probe, then we have to clean-up everything since nothing in the device core will do so for us.

Péter Ujfalusi

8:10 a.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

On 12/09/2023 03:25, Pierre-Louis Bossart wrote:

...

...
What we have atm: snd_sof_probe - might be called from wq snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step)

I don't think it's correct, snd_sof_remove cannot be called from a wq. The device core knows nothing about workqueues.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...

it is called on the error path of sof_probe_continue(), which can be run in a workque.

...

...
We want a callbacks for hardware/device probing, right, split the snd_sof_probe (and remove) to be able to support a sane level of deferred probing support.

With that in mind: snd_sof_device_probe - Not called from wq (to handle deferred probing) snd_sof_probe - might be called from wq

snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step) snd_sof_device_remove - Not called from wq (to up the snd_sof_device_probe step)

Naming option: s/device/hardware

I like the 'device' hint since it's directly related to the device (or subsystem) callbacks.

...
However, I think the snd_sof_device_remove itself is redundant and we might not need it at all as in case we have wq and there is a failure in there we do want to release resources as much as possible. The module will be kept loaded (no deferred handling in wq) and that might block PM, other devices to behave correctly. Iow, if the wq has failure we should do a cleanup to the best effort to reach a level like the driver is not even loaded.

If we have a failure in a workqueue used for probe, then we have to clean-up everything since nothing in the device core will do so for us.

Yes, this makes the snd_sof_device_remove() redundant or at least the definition of it is no longer a mirror of snd_sof_device_probe():

snd_sof_device_remove - might be called from wq (cleans up the snd_sof_device_probe step)

Any failure in sof_probe_continue() should execute the snd_sof_device_remove(), snd_sof_remove() is only involved after the snd_sof_probe() have returned with success.

I think this makes actually makes sense and it is well defined. On module remove we need to take into account the case when we have failed in wq similarly as we do currently (the resources have been freed up already).

-- Péter

Pierre-Louis Bossart

4:02 p.m.

New subject: [PATCH v4 01/11] ASoC: SOF: core: add 'no_wq' probe and remove callbacks

On 9/12/23 02:10, Péter Ujfalusi wrote:

...

On 12/09/2023 03:25, Pierre-Louis Bossart wrote:

...
...
What we have atm: snd_sof_probe - might be called from wq snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step)

I don't think it's correct, snd_sof_remove cannot be called from a wq. The device core knows nothing about workqueues.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/soun...

it is called on the error path of sof_probe_continue(), which can be run in a workque.

...
...
We want a callbacks for hardware/device probing, right, split the snd_sof_probe (and remove) to be able to support a sane level of deferred probing support.

With that in mind: snd_sof_device_probe - Not called from wq (to handle deferred probing) snd_sof_probe - might be called from wq

snd_sof_remove - might be called from wq (cleans up the snd_sof_probe step) snd_sof_device_remove - Not called from wq (to up the snd_sof_device_probe step)

Naming option: s/device/hardware

I like the 'device' hint since it's directly related to the device (or subsystem) callbacks.

...
However, I think the snd_sof_device_remove itself is redundant and we might not need it at all as in case we have wq and there is a failure in there we do want to release resources as much as possible. The module will be kept loaded (no deferred handling in wq) and that might block PM, other devices to behave correctly. Iow, if the wq has failure we should do a cleanup to the best effort to reach a level like the driver is not even loaded.

If we have a failure in a workqueue used for probe, then we have to clean-up everything since nothing in the device core will do so for us.

Yes, this makes the snd_sof_device_remove() redundant or at least the definition of it is no longer a mirror of snd_sof_device_probe():

snd_sof_device_remove - might be called from wq (cleans up the snd_sof_device_probe step)

Any failure in sof_probe_continue() should execute the snd_sof_device_remove(), snd_sof_remove() is only involved after the snd_sof_probe() have returned with success.

I think this makes actually makes sense and it is well defined. On module remove we need to take into account the case when we have failed in wq similarly as we do currently (the resources have been freed up already).

Agree, I stand corrected, thanks Peter.

Maarten Lankhorst

30 Aug 30 Aug

5:36 p.m.

New subject: [PATCH v4 02/11] ASoC: SOF: Intel: hda: start splitting the probe

From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

This patch moves the initial parts of the probe to the probe_no_wq() callback, which provides a much faster decision on whether the SOF driver shall deal with a specific platform or yield to other Intel drivers.

This is a limited functionality change, the bigger change is to move the i915/Xe initialization to the probe_no_wq().

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- sound/soc/sof/intel/hda-common-ops.c | 1 + sound/soc/sof/intel/hda.c | 16 +++++++++++++--- sound/soc/sof/intel/hda.h | 1 + 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c index 8e1cd0babd32c..803b5e9087782 100644 --- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -16,6 +16,7 @@

struct snd_sof_dsp_ops sof_hda_common_ops = { /* probe/remove/shutdown */ + .probe_no_wq = hda_dsp_probe_no_wq, .probe = hda_dsp_probe, .remove = hda_dsp_remove,

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 15e6779efaa3b..e918b5dadfa02 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1118,11 +1118,10 @@ static irqreturn_t hda_dsp_interrupt_thread(int irq, void *context) return IRQ_HANDLED; }

-int hda_dsp_probe(struct snd_sof_dev *sdev) +int hda_dsp_probe_no_wq(struct snd_sof_dev *sdev) { struct pci_dev *pci = to_pci_dev(sdev->dev); struct sof_intel_hda_dev *hdev; - struct hdac_bus *bus; const struct sof_intel_dsp_desc *chip; int ret = 0;

@@ -1162,6 +1161,17 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) sdev->pdata->hw_pdata = hdev; hdev->desc = chip;

+err: + return ret; +} + +int hda_dsp_probe(struct snd_sof_dev *sdev) +{ + struct pci_dev *pci = to_pci_dev(sdev->dev); + struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata; + struct hdac_bus *bus; + int ret = 0; + hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", PLATFORM_DEVID_NONE, NULL, 0); @@ -1299,7 +1309,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) platform_device_unregister(hdev->dmic_dev); iounmap(bus->remap_addr); hda_codec_i915_exit(sdev); -err: + return ret; }

diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 5c517ec57d4a2..89b8c239e9a5e 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -573,6 +573,7 @@ struct sof_intel_hda_stream { /* * DSP Core services. */ +int hda_dsp_probe_no_wq(struct snd_sof_dev *sdev); int hda_dsp_probe(struct snd_sof_dev *sdev); int hda_dsp_remove(struct snd_sof_dev *sdev); int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);

-- 2.39.2

Maarten Lankhorst

5:36 p.m.

New subject: [PATCH v4 03/11] ALSA: hda/intel: Fix error handling in azx_probe()

Add missing pci_set_drv to NULL call on error.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/pci/hda/hda_intel.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 765d95e798617..bf6210506a2da 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2176,6 +2176,7 @@ static int azx_probe(struct pci_dev *pci, return 0;

out_free: + pci_set_drvdata(pci, NULL); snd_card_free(card); return err; }

-- 2.39.2

Maarten Lankhorst

5:36 p.m.

New subject: [PATCH v4 04/11] ALSA: hda/i915: Allow override of gpu binding.

Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports video_firmware_drivers_only(). This can be used as a first approximation on whether i915 will be available. It's safe to use as this is only built when CONFIG_SND_HDA_I915 is selected by CONFIG_I915.

It's not completely fool proof, as you can boot with "nomodeset i915.modeset=1" to make i915 load regardless, or use "i915.force_probe=!*" to never load i915, but the common case of booting with nomodeset to disable all GPU drivers this will work as intended.

Because of this, we add an extra module parameter, snd_hda_core.gpu_bind that can be used to signal users intent. -1 follows nomodeset, 0 disables binding, 1 forces wait/-EPROBE_DEFER on binding.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/hda/hdac_i915.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index b428537f284c7..a4a712c795c3d 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -10,6 +10,12 @@ #include <sound/hdaudio.h> #include <sound/hda_i915.h> #include <sound/hda_register.h> +#include <video/nomodeset.h> + +static int gpu_bind = -1; +module_param(gpu_bind, int, 0644); +MODULE_PARM_DESC(gpu_bind, "Whether to bind sound component to GPU " + "(1=always, 0=never, -1=on nomodeset(default))");

/** * snd_hdac_i915_set_bclk - Reprogram BCLK for HSW/BDW @@ -122,6 +128,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci) { struct pci_dev *display_dev = NULL;

+ if (!gpu_bind || (gpu_bind < 0 && video_firmware_drivers_only())) + return false; + for_each_pci_dev(display_dev) { if (display_dev->vendor == PCI_VENDOR_ID_INTEL && (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&

-- 2.39.2

Maarten Lankhorst

5:36 p.m.

New subject: [PATCH v4 05/11] ALSA: hda/i915: Add an allow_modprobe argument to snd_hdac_i915_init

Xe is a new GPU driver that re-uses the display (and sound) code from i915. It's no longer possible to load i915, as the GPU can be driven by the xe driver instead.

The new behavior will return -EPROBE_DEFER, and wait for a compatible driver to be loaded instead of modprobing i915.

Converting all drivers at the same time is a lot of work, instead we will convert each user one by one.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- Changes since v1: - Use dev_err_probe to set a probe reason for debugfs' deferred_devices. --- 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 bf6210506a2da..85fa15bbd8513 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2265,7 +2265,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

Maarten Lankhorst

5:36 p.m.

New subject: [PATCH v4 06/11] ALSA: hda/i915: Allow xe as match for i915_component_master_match

Xe is a new driver for intel GPU's that shares the sound related code with i915.

The modprobe mechanism is being replaced by the -EPROBE_DEFER mechanism, so we don't need to add a modprobe xe call. Adding this would have required a telepathy module to correctly guess whether to load i915 or xe.

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index ffa35d7a367c0..0765e5350e7ba 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

Maarten Lankhorst

5:36 p.m.

New subject: [PATCH v4 07/11] ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work.

Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue. It's likely the whole workqueue can be destroyed, but I don't have the means to test this.

Removing the workqueue would simplify init even further, but is left as exercise for the reviewer.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Mark Brown broonie@kernel.org Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- Changes since v1: - Rename error label. --- 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

Amadeusz Sławiński

31 Aug 31 Aug

12:14 p.m.

New subject: [PATCH v4 07/11] ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work.

On 8/30/2023 5:36 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 maarten.lankhorst@linux.intel.com Acked-by: Mark Brown broonie@kernel.org Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com

Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com

Maarten Lankhorst

30 Aug 30 Aug

5:36 p.m.

New subject: [PATCH v4 08/11] ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work.

Removing the workqueue would simplify init even further, but is left as exercise for the reviewer.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Mark Brown broonie@kernel.org Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- 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 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

Amadeusz Sławiński

31 Aug 31 Aug

12:15 p.m.

New subject: [PATCH v4 08/11] ASoC: Intel: Skylake: Move snd_hdac_i915_init to before probe_work.

On 8/30/2023 5:36 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 maarten.lankhorst@linux.intel.com Acked-by: Mark Brown broonie@kernel.org Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com

Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com

Maarten Lankhorst

30 Aug 30 Aug

5:36 p.m.

New subject: [PATCH v4 09/11] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.

Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue.

Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

--- 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. --- 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 85fa15bbd8513..d529ef21f033a 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2135,6 +2135,36 @@ static int azx_probe(struct pci_dev *pci,

pci_set_drvdata(pci, card);

+#ifdef CONFIG_SND_HDA_I915 + /* bind with i915 if needed */ + if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { + err = snd_hdac_i915_init(azx_bus(chip), false); + if (err < 0) { + /* if the controller is bound only with HDMI/DP + * (for HSW and BDW), we need to abort the probe; + * for other chips, still continue probing as other + * codecs can be on the same link. + */ + if (HDA_CONTROLLER_IN_GPU(pci)) { + dev_err_probe(card->dev, err, + "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); + + goto out_free; + } else { + /* don't bother any longer */ + chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; + } + } + + /* HSW/BDW controllers need this power */ + if (HDA_CONTROLLER_IN_GPU(pci)) + hda->need_i915_power = true; + } +#else + if (HDA_CONTROLLER_IN_GPU(pci)) + dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n"); +#endif + err = register_vga_switcheroo(chip); if (err < 0) { dev_err(card->dev, "Error registering vga_switcheroo client\n"); @@ -2162,11 +2192,6 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */

-#ifndef CONFIG_SND_HDA_I915 - if (HDA_CONTROLLER_IN_GPU(pci)) - dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n"); -#endif - if (schedule_probe) schedule_delayed_work(&hda->probe_work, 0);

@@ -2263,30 +2288,6 @@ static int azx_probe_continue(struct azx *chip) to_hda_bus(bus)->bus_probing = 1; hda->probe_continued = 1;

- /* bind with i915 if needed */ - if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { - err = snd_hdac_i915_init(bus, true); - if (err < 0) { - /* if the controller is bound only with HDMI/DP - * (for HSW and BDW), we need to abort the probe; - * for other chips, still continue probing as other - * codecs can be on the same link. - */ - if (HDA_CONTROLLER_IN_GPU(pci)) { - dev_err(chip->card->dev, - "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); - goto out_free; - } else { - /* don't bother any longer */ - chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; - } - } - - /* HSW/BDW controllers need this power */ - if (HDA_CONTROLLER_IN_GPU(pci)) - hda->need_i915_power = true; - } - /* Request display power well for the HDA controller or codec. For * Haswell/Broadwell, both the display HDA controller and codec need * this power. For other platforms, like Baytrail/Braswell, only the

-- 2.39.2

Maarten Lankhorst

5:36 p.m.

New subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

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_no_wq can be used for this, and we also use the newly added remove_no_wq for unbinding afterwards.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/intel/hda-common-ops.c | 1 + sound/soc/sof/intel/hda.c | 15 ++++++--------- sound/soc/sof/intel/hda.h | 1 + 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c index 803b5e9087782..1e2e9b6a5c1c2 100644 --- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -17,6 +17,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { /* probe/remove/shutdown */ .probe_no_wq = hda_dsp_probe_no_wq, + .remove_no_wq = hda_dsp_remove_no_wq, .probe = hda_dsp_probe, .remove = hda_dsp_remove,

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index e918b5dadfa02..886073598e40f 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1160,6 +1160,7 @@ int hda_dsp_probe_no_wq(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,12 @@ int hda_dsp_remove(struct snd_sof_dev *sdev) if (!sdev->dspless_mode_selected) iounmap(sdev->bar[HDA_DSP_BAR]);

- iounmap(bus->remap_addr); - - sof_hda_bus_exit(sdev); + return 0; +}

+int hda_dsp_remove_no_wq(struct snd_sof_dev *sdev) +{ + iounmap(sof_to_bus(sdev)->remap_addr); hda_codec_i915_exit(sdev);

return 0; diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 89b8c239e9a5e..26ae11e4e1240 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -574,6 +574,7 @@ struct sof_intel_hda_stream { * DSP Core services. */ int hda_dsp_probe_no_wq(struct snd_sof_dev *sdev); +int hda_dsp_remove_no_wq(struct snd_sof_dev *sdev); int hda_dsp_probe(struct snd_sof_dev *sdev); int hda_dsp_remove(struct snd_sof_dev *sdev); int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);

-- 2.39.2

kernel test robot

31 Aug 31 Aug

1:06 a.m.

New subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Hi Maarten,

kernel test robot noticed the following build warnings:

[auto build test WARNING on broonie-sound/for-next] [also build test WARNING on tiwai-sound/for-next tiwai-sound/for-linus linus/master next-20230830] [cannot apply to v6.5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/ASoC-SOF-co... base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next patch link: https://lore.kernel.org/r/20230830153652.217855-11-maarten.lankhorst%40linux... patch subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230831/202308310618.kqqYvniK-lkp@i...) compiler: alpha-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230831/202308310618.kqqYvniK-lkp@i...)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202308310618.kqqYvniK-lkp@intel.com/

All warnings (new ones prefixed by >>):

sound/soc/sof/intel/hda.c: In function 'hda_dsp_probe':

...

...
sound/soc/sof/intel/hda.c:1173:26: warning: variable 'bus' set but not used [-Wunused-but-set-variable]

1173 | struct hdac_bus *bus; | ^~~

vim +/bus +1173 sound/soc/sof/intel/hda.c

47f868f27a979a Pierre-Louis Bossart 2023-08-30 1168 47f868f27a979a Pierre-Louis Bossart 2023-08-30 1169 int hda_dsp_probe(struct snd_sof_dev *sdev) 47f868f27a979a Pierre-Louis Bossart 2023-08-30 1170 { 47f868f27a979a Pierre-Louis Bossart 2023-08-30 1171 struct pci_dev *pci = to_pci_dev(sdev->dev); 47f868f27a979a Pierre-Louis Bossart 2023-08-30 1172 struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata; 47f868f27a979a Pierre-Louis Bossart 2023-08-30 @1173 struct hdac_bus *bus; 47f868f27a979a Pierre-Louis Bossart 2023-08-30 1174 int ret = 0; 47f868f27a979a Pierre-Louis Bossart 2023-08-30 1175 dd96daca6c83ec Liam Girdwood 2019-04-12 1176 hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", dd96daca6c83ec Liam Girdwood 2019-04-12 1177 PLATFORM_DEVID_NONE, dd96daca6c83ec Liam Girdwood 2019-04-12 1178 NULL, 0); dd96daca6c83ec Liam Girdwood 2019-04-12 1179 if (IS_ERR(hdev->dmic_dev)) { dd96daca6c83ec Liam Girdwood 2019-04-12 1180 dev_err(sdev->dev, "error: failed to create DMIC device\n"); dd96daca6c83ec Liam Girdwood 2019-04-12 1181 return PTR_ERR(hdev->dmic_dev); dd96daca6c83ec Liam Girdwood 2019-04-12 1182 } dd96daca6c83ec Liam Girdwood 2019-04-12 1183 dd96daca6c83ec Liam Girdwood 2019-04-12 1184 /* dd96daca6c83ec Liam Girdwood 2019-04-12 1185 * use position update IPC if either it is forced dd96daca6c83ec Liam Girdwood 2019-04-12 1186 * or we don't have other choice dd96daca6c83ec Liam Girdwood 2019-04-12 1187 */ dd96daca6c83ec Liam Girdwood 2019-04-12 1188 #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_FORCE_IPC_POSITION) dd96daca6c83ec Liam Girdwood 2019-04-12 1189 hdev->no_ipc_position = 0; dd96daca6c83ec Liam Girdwood 2019-04-12 1190 #else dd96daca6c83ec Liam Girdwood 2019-04-12 1191 hdev->no_ipc_position = sof_ops(sdev)->pcm_pointer ? 1 : 0; dd96daca6c83ec Liam Girdwood 2019-04-12 1192 #endif dd96daca6c83ec Liam Girdwood 2019-04-12 1193 1f7b5d52be130e Peter Ujfalusi 2023-04-04 1194 if (sdev->dspless_mode_selected) 1f7b5d52be130e Peter Ujfalusi 2023-04-04 1195 hdev->no_ipc_position = 1; 1f7b5d52be130e Peter Ujfalusi 2023-04-04 1196 dd96daca6c83ec Liam Girdwood 2019-04-12 1197 /* set up HDA base */ dd96daca6c83ec Liam Girdwood 2019-04-12 1198 bus = sof_to_bus(sdev); dd96daca6c83ec Liam Girdwood 2019-04-12 1199 9fc6786f549c4d Pierre-Louis Bossart 2023-04-04 1200 if (sdev->dspless_mode_selected) 9fc6786f549c4d Pierre-Louis Bossart 2023-04-04 1201 goto skip_dsp_setup; 9fc6786f549c4d Pierre-Louis Bossart 2023-04-04 1202 dd96daca6c83ec Liam Girdwood 2019-04-12 1203 /* DSP base */ dd96daca6c83ec Liam Girdwood 2019-04-12 1204 sdev->bar[HDA_DSP_BAR] = pci_ioremap_bar(pci, HDA_DSP_BAR); dd96daca6c83ec Liam Girdwood 2019-04-12 1205 if (!sdev->bar[HDA_DSP_BAR]) { dd96daca6c83ec Liam Girdwood 2019-04-12 1206 dev_err(sdev->dev, "error: ioremap error\n"); dd96daca6c83ec Liam Girdwood 2019-04-12 1207 ret = -ENXIO; dd96daca6c83ec Liam Girdwood 2019-04-12 1208 goto hdac_bus_unmap; dd96daca6c83ec Liam Girdwood 2019-04-12 1209 } dd96daca6c83ec Liam Girdwood 2019-04-12 1210 dd96daca6c83ec Liam Girdwood 2019-04-12 1211 sdev->mmio_bar = HDA_DSP_BAR; dd96daca6c83ec Liam Girdwood 2019-04-12 1212 sdev->mailbox_bar = HDA_DSP_BAR; 9fc6786f549c4d Pierre-Louis Bossart 2023-04-04 1213 skip_dsp_setup: dd96daca6c83ec Liam Girdwood 2019-04-12 1214 dd96daca6c83ec Liam Girdwood 2019-04-12 1215 /* allow 64bit DMA address if supported by H/W */ ab152afa2427bb Takashi Iwai 2021-01-14 1216 if (dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(64))) { dd96daca6c83ec Liam Girdwood 2019-04-12 1217 dev_dbg(sdev->dev, "DMA mask is 32 bit\n"); ab152afa2427bb Takashi Iwai 2021-01-14 1218 dma_set_mask_and_coherent(&pci->dev, DMA_BIT_MASK(32)); dd96daca6c83ec Liam Girdwood 2019-04-12 1219 } 8872fc0d045929 Takashi Iwai 2022-02-15 1220 dma_set_max_seg_size(&pci->dev, UINT_MAX); dd96daca6c83ec Liam Girdwood 2019-04-12 1221 dd96daca6c83ec Liam Girdwood 2019-04-12 1222 /* init streams */ dd96daca6c83ec Liam Girdwood 2019-04-12 1223 ret = hda_dsp_stream_init(sdev); dd96daca6c83ec Liam Girdwood 2019-04-12 1224 if (ret < 0) { dd96daca6c83ec Liam Girdwood 2019-04-12 1225 dev_err(sdev->dev, "error: failed to init streams\n"); dd96daca6c83ec Liam Girdwood 2019-04-12 1226 /* dd96daca6c83ec Liam Girdwood 2019-04-12 1227 * not all errors are due to memory issues, but trying dd96daca6c83ec Liam Girdwood 2019-04-12 1228 * to free everything does not harm dd96daca6c83ec Liam Girdwood 2019-04-12 1229 */ dd96daca6c83ec Liam Girdwood 2019-04-12 1230 goto free_streams; dd96daca6c83ec Liam Girdwood 2019-04-12 1231 } dd96daca6c83ec Liam Girdwood 2019-04-12 1232 dd96daca6c83ec Liam Girdwood 2019-04-12 1233 /* dd96daca6c83ec Liam Girdwood 2019-04-12 1234 * register our IRQ dd96daca6c83ec Liam Girdwood 2019-04-12 1235 * let's try to enable msi firstly dd96daca6c83ec Liam Girdwood 2019-04-12 1236 * if it fails, use legacy interrupt mode 672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22 1237 * TODO: support msi multiple vectors dd96daca6c83ec Liam Girdwood 2019-04-12 1238 */ bb67dd1878de57 Pierre-Louis Bossart 2019-08-06 1239 if (hda_use_msi && pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_MSI) > 0) { 672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22 1240 dev_info(sdev->dev, "use msi interrupt mode\n"); 7c11af9fcdc425 Bard Liao 2019-12-04 1241 sdev->ipc_irq = pci_irq_vector(pci, 0); 672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22 1242 /* initialised to "false" by kzalloc() */ 672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22 1243 sdev->msi_enabled = true; 672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22 1244 } 672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22 1245 672ff5e3596ee2 Guennadi Liakhovetski 2019-07-22 1246 if (!sdev->msi_enabled) { dd96daca6c83ec Liam Girdwood 2019-04-12 1247 dev_info(sdev->dev, "use legacy interrupt mode\n"); dd96daca6c83ec Liam Girdwood 2019-04-12 1248 /* dd96daca6c83ec Liam Girdwood 2019-04-12 1249 * in IO-APIC mode, hda->irq and ipc_irq are using the same dd96daca6c83ec Liam Girdwood 2019-04-12 1250 * irq number of pci->irq dd96daca6c83ec Liam Girdwood 2019-04-12 1251 */ dd96daca6c83ec Liam Girdwood 2019-04-12 1252 sdev->ipc_irq = pci->irq; dd96daca6c83ec Liam Girdwood 2019-04-12 1253 } dd96daca6c83ec Liam Girdwood 2019-04-12 1254 dd96daca6c83ec Liam Girdwood 2019-04-12 1255 dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq); 7c11af9fcdc425 Bard Liao 2019-12-04 1256 ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_interrupt_handler, 7c11af9fcdc425 Bard Liao 2019-12-04 1257 hda_dsp_interrupt_thread, 7c11af9fcdc425 Bard Liao 2019-12-04 1258 IRQF_SHARED, "AudioDSP", sdev); dd96daca6c83ec Liam Girdwood 2019-04-12 1259 if (ret < 0) { dd96daca6c83ec Liam Girdwood 2019-04-12 1260 dev_err(sdev->dev, "error: failed to register IPC IRQ %d\n", dd96daca6c83ec Liam Girdwood 2019-04-12 1261 sdev->ipc_irq); 7c11af9fcdc425 Bard Liao 2019-12-04 1262 goto free_irq_vector; dd96daca6c83ec Liam Girdwood 2019-04-12 1263 } dd96daca6c83ec Liam Girdwood 2019-04-12 1264 dd96daca6c83ec Liam Girdwood 2019-04-12 1265 pci_set_master(pci); dd96daca6c83ec Liam Girdwood 2019-04-12 1266 synchronize_irq(pci->irq); dd96daca6c83ec Liam Girdwood 2019-04-12 1267 dd96daca6c83ec Liam Girdwood 2019-04-12 1268 /* dd96daca6c83ec Liam Girdwood 2019-04-12 1269 * clear TCSEL to clear playback on some HD Audio dd96daca6c83ec Liam Girdwood 2019-04-12 1270 * codecs. PCI TCSEL is defined in the Intel manuals. dd96daca6c83ec Liam Girdwood 2019-04-12 1271 */ dd96daca6c83ec Liam Girdwood 2019-04-12 1272 snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0); dd96daca6c83ec Liam Girdwood 2019-04-12 1273 dd96daca6c83ec Liam Girdwood 2019-04-12 1274 /* init HDA capabilities */ dd96daca6c83ec Liam Girdwood 2019-04-12 1275 ret = hda_init_caps(sdev); dd96daca6c83ec Liam Girdwood 2019-04-12 1276 if (ret < 0) dd96daca6c83ec Liam Girdwood 2019-04-12 1277 goto free_ipc_irq; dd96daca6c83ec Liam Girdwood 2019-04-12 1278 9fc6786f549c4d Pierre-Louis Bossart 2023-04-04 1279 if (!sdev->dspless_mode_selected) { 1f5253b08e06bc Zhu Yingjiang 2019-05-22 1280 /* enable ppcap interrupt */ 1f5253b08e06bc Zhu Yingjiang 2019-05-22 1281 hda_dsp_ctrl_ppcap_enable(sdev, true); 1f5253b08e06bc Zhu Yingjiang 2019-05-22 1282 hda_dsp_ctrl_ppcap_int_enable(sdev, true); dd96daca6c83ec Liam Girdwood 2019-04-12 1283 dd96daca6c83ec Liam Girdwood 2019-04-12 1284 /* set default mailbox offset for FW ready message */ dd96daca6c83ec Liam Girdwood 2019-04-12 1285 sdev->dsp_box.offset = HDA_DSP_MBOX_UPLINK_OFFSET; dd96daca6c83ec Liam Girdwood 2019-04-12 1286 63e51fd33fef04 Ranjani Sridharan 2020-01-29 1287 INIT_DELAYED_WORK(&hdev->d0i3_work, hda_dsp_d0i3_work); 9fc6786f549c4d Pierre-Louis Bossart 2023-04-04 1288 } 63e51fd33fef04 Ranjani Sridharan 2020-01-29 1289 e2379d4a83da44 Pierre-Louis Bossart 2022-09-20 1290 init_waitqueue_head(&hdev->waitq); e2379d4a83da44 Pierre-Louis Bossart 2022-09-20 1291 95fa7a62e16463 Pierre-Louis Bossart 2022-04-21 1292 hdev->nhlt = intel_nhlt_init(sdev->dev); 95fa7a62e16463 Pierre-Louis Bossart 2022-04-21 1293 dd96daca6c83ec Liam Girdwood 2019-04-12 1294 return 0; dd96daca6c83ec Liam Girdwood 2019-04-12 1295 dd96daca6c83ec Liam Girdwood 2019-04-12 1296 free_ipc_irq: dd96daca6c83ec Liam Girdwood 2019-04-12 1297 free_irq(sdev->ipc_irq, sdev); dd96daca6c83ec Liam Girdwood 2019-04-12 1298 free_irq_vector: dd96daca6c83ec Liam Girdwood 2019-04-12 1299 if (sdev->msi_enabled) dd96daca6c83ec Liam Girdwood 2019-04-12 1300 pci_free_irq_vectors(pci); dd96daca6c83ec Liam Girdwood 2019-04-12 1301 free_streams: dd96daca6c83ec Liam Girdwood 2019-04-12 1302 hda_dsp_stream_free(sdev); dd96daca6c83ec Liam Girdwood 2019-04-12 1303 /* dsp_unmap: not currently used */ 9fc6786f549c4d Pierre-Louis Bossart 2023-04-04 1304 if (!sdev->dspless_mode_selected) dd96daca6c83ec Liam Girdwood 2019-04-12 1305 iounmap(sdev->bar[HDA_DSP_BAR]); dd96daca6c83ec Liam Girdwood 2019-04-12 1306 hdac_bus_unmap: 5bb0ecddb2a7f6 Pierre-Louis Bossart 2021-03-01 1307 platform_device_unregister(hdev->dmic_dev); 47f868f27a979a Pierre-Louis Bossart 2023-08-30 1308 dd96daca6c83ec Liam Girdwood 2019-04-12 1309 return ret; dd96daca6c83ec Liam Girdwood 2019-04-12 1310 } dd96daca6c83ec Liam Girdwood 2019-04-12 1311

-- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

kernel test robot

1:59 a.m.

New subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Hi Maarten,

kernel test robot noticed the following build warnings:

url: https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/ASoC-SOF-co... base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next patch link: https://lore.kernel.org/r/20230830153652.217855-11-maarten.lankhorst%40linux... patch subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe config: x86_64-randconfig-005-20230831 (https://download.01.org/0day-ci/archive/20230831/202308310715.lBXHTY4I-lkp@i...) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230831/202308310715.lBXHTY4I-lkp@i...)

All warnings (new ones prefixed by >>):

...

...
sound/soc/sof/intel/hda.c:1173:19: warning: variable 'bus' set but not used [-Wunused-but-set-variable]

struct hdac_bus *bus; ^ 1 warning generated.

vim +/bus +1173 sound/soc/sof/intel/hda.c

-- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki

Kai Vehmanen

1 Sep 1 Sep

2:33 p.m.

New subject: [PATCH v4 10/11] ASoC: SOF: Intel: Move binding to display driver outside of deferred probe

Hey,

On Wed, 30 Aug 2023, Maarten Lankhorst wrote:

...

Now that we can use -EPROBE_DEFER, it's no longer required to spin off the snd_hdac_i915_init into a workqueue.

Use the -EPROBE_DEFER mechanism instead, which must be returned in the probe function.

The previously added probe_no_wq can be used for this, and we also use the newly added remove_no_wq for unbinding afterwards.

[...]

...

@@ -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,12 @@ int hda_dsp_remove(struct snd_sof_dev *sdev) if (!sdev->dspless_mode_selected) iounmap(sdev->bar[HDA_DSP_BAR]);

iounmap(bus->remap_addr);

sof_hda_bus_exit(sdev);

return 0;

+}

+int hda_dsp_remove_no_wq(struct snd_sof_dev *sdev) +{

iounmap(sof_to_bus(sdev)->remap_addr); hda_codec_i915_exit(sdev);

I think here we drop the call to sof_hda_bus_exit() which should be done in hda_dsp_remove_no_wq() to counter hda_init().

Rest looks good to me, the "no_wq" variants do symmetric ops, so we can handle both wq an non-wq cases.

Br, Kai

Maarten Lankhorst

30 Aug 30 Aug

5:36 p.m.

New subject: [PATCH v4 11/11] ALSA: hda/i915: Remove extra argument from snd_hdac_i915_init

Now that all drivers have moved from modprobe loading to handling -EPROBE_DEFER, we can remove the argument again.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- Changes since v1: - Use dev_err_probe() to set reason in debugfs for deferred probe. --- 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 d529ef21f033a..e0f01a3ab231c 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

466

Age (days ago)

479

Last active (days ago)

List overview

Download

27 comments

6 participants

tags (0)

participants (6)

Amadeusz Sławiński
Kai Vehmanen
kernel test robot
Maarten Lankhorst
Pierre-Louis Bossart
Péter Ujfalusi