On 7/19/23 18:41, Maarten Lankhorst wrote:
> This was only used to allow modprobing i915, by converting to the
> -EPROBE_DEFER mechanism, it can be completely removed, and is in
> fact counterproductive since -EPROBE_DEFER otherwise won't be
> handled correctly.
I personally remember only that the request_module("i915") was the main
motivation for the use of the workqueue, but when it comes to the
HDaudio codec management we don't even know what we don't know.
I am a bit worried that the snd-hda-intel driver keeps the workqueue for
HDaudio codec initialization, and this patch removes the workqueue
completely for SOF. That doesn't seem right. Either both drivers need a
workqueue or none need a workqueue.
Maybe what we need is to move the i915/xe initialization out of the
workqueue, and see in a second pass if that workqueue can be safely
removed from the SOF driver?
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Acked-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> Acked-by: Mark Brown <broonie@xxxxxxxxxx>
> ---
> sound/soc/sof/Kconfig | 19 -----------------
> sound/soc/sof/core.c | 38 ++-------------------------------
> sound/soc/sof/intel/Kconfig | 1 -
> sound/soc/sof/intel/hda-codec.c | 2 +-
> sound/soc/sof/intel/hda.c | 32 ++++++++++++++++-----------
> sound/soc/sof/sof-pci-dev.c | 3 +--
> sound/soc/sof/sof-priv.h | 5 -----
> 7 files changed, 23 insertions(+), 77 deletions(-)
>
> diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
> index 80361139a49ad..8ee39e5558062 100644
> --- a/sound/soc/sof/Kconfig
> +++ b/sound/soc/sof/Kconfig
> @@ -82,17 +82,6 @@ config SND_SOC_SOF_DEVELOPER_SUPPORT
>
> if SND_SOC_SOF_DEVELOPER_SUPPORT
>
> -config SND_SOC_SOF_FORCE_PROBE_WORKQUEUE
> - bool "SOF force probe workqueue"
> - select SND_SOC_SOF_PROBE_WORK_QUEUE
> - help
> - This option forces the use of a probe workqueue, which is only used
> - when HDaudio is enabled due to module dependencies. Forcing this
> - option is intended for debug only, but this should not add any
> - functional issues in nominal cases.
> - Say Y if you are involved in SOF development and need this option.
> - If not, select N.
> -
> config SND_SOC_SOF_NOCODEC
> tristate
>
> @@ -271,14 +260,6 @@ config SND_SOC_SOF
> module dependencies but since the module or built-in type is decided
> at the top level it doesn't matter.
>
> -config SND_SOC_SOF_PROBE_WORK_QUEUE
> - bool
> - help
> - This option is not user-selectable but automagically handled by
> - 'select' statements at a higher level.
> - When selected, the probe is handled in two steps, for example to
> - avoid lockdeps if request_module is used in the probe.
> -
> # Supported IPC versions
> config SND_SOC_SOF_IPC3
> bool
> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
> index 30db685cc5f4b..cdf86dc4a8a87 100644
> --- a/sound/soc/sof/core.c
> +++ b/sound/soc/sof/core.c
> @@ -191,7 +191,8 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
> /* probe the DSP hardware */
> ret = snd_sof_probe(sdev);
> if (ret < 0) {
> - dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret);
> + if (ret != -EPROBE_DEFER)
> + dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret);
> goto probe_err;
> }
>
> @@ -309,8 +310,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
> if (plat_data->sof_probe_complete)
> plat_data->sof_probe_complete(sdev->dev);
>
> - sdev->probe_completed = true;
> -
> return 0;
>
> sof_machine_err:
> @@ -336,19 +335,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
> return ret;
> }
>
> -static void sof_probe_work(struct work_struct *work)
> -{
> - struct snd_sof_dev *sdev =
> - container_of(work, struct snd_sof_dev, probe_work);
> - int ret;
> -
> - ret = sof_probe_continue(sdev);
> - if (ret < 0) {
> - /* errors cannot be propagated, log */
> - dev_err(sdev->dev, "error: %s failed err: %d\n", __func__, ret);
> - }
> -}
> -
> int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
> {
> struct snd_sof_dev *sdev;
> @@ -436,33 +422,16 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
>
> sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED);
>
> - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) {
> - INIT_WORK(&sdev->probe_work, sof_probe_work);
> - schedule_work(&sdev->probe_work);
> - return 0;
> - }
> -
> return sof_probe_continue(sdev);
> }
> EXPORT_SYMBOL(snd_sof_device_probe);
>
> -bool snd_sof_device_probe_completed(struct device *dev)
> -{
> - struct snd_sof_dev *sdev = dev_get_drvdata(dev);
> -
> - return sdev->probe_completed;
> -}
> -EXPORT_SYMBOL(snd_sof_device_probe_completed);
> -
> int snd_sof_device_remove(struct device *dev)
> {
> struct snd_sof_dev *sdev = dev_get_drvdata(dev);
> struct snd_sof_pdata *pdata = sdev->pdata;
> int ret;
>
> - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
> - cancel_work_sync(&sdev->probe_work);
> -
> /*
> * Unregister any registered client device first before IPC and debugfs
> * to allow client drivers to be removed cleanly
> @@ -501,9 +470,6 @@ int snd_sof_device_shutdown(struct device *dev)
> {
> struct snd_sof_dev *sdev = dev_get_drvdata(dev);
>
> - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
> - cancel_work_sync(&sdev->probe_work);
> -
> if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) {
> sof_fw_trace_free(sdev);
> return snd_sof_shutdown(sdev);
> diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
> index 69c1a370d3b61..d9e87a91670a3 100644
> --- a/sound/soc/sof/intel/Kconfig
> +++ b/sound/soc/sof/intel/Kconfig
> @@ -293,7 +293,6 @@ config SND_SOC_SOF_HDA_LINK
> config SND_SOC_SOF_HDA_AUDIO_CODEC
> bool "SOF support for HDAudio codecs"
> depends on SND_SOC_SOF_HDA_LINK
> - select SND_SOC_SOF_PROBE_WORK_QUEUE
> help
> This adds support for HDAudio codecs with Sound Open Firmware
> for Intel(R) platforms.
> diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
> index f1fd5b44aaac9..344b61576c0e3 100644
> --- a/sound/soc/sof/intel/hda-codec.c
> +++ b/sound/soc/sof/intel/hda-codec.c
> @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev)
> return 0;
>
> /* i915 exposes a HDA codec for HDMI audio */
> - ret = snd_hdac_i915_init(bus, true);
> + ret = snd_hdac_i915_init(bus, false);
> if (ret < 0)
> return ret;
>
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index 64bebe1a72bbc..a8b7a68142c05 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -801,8 +801,11 @@ static int hda_init(struct snd_sof_dev *sdev)
>
> /* init i915 and HDMI codecs */
> ret = hda_codec_i915_init(sdev);
> - if (ret < 0)
> - dev_warn(sdev->dev, "init of i915 and HDMI codec failed\n");
> + if (ret < 0) {
> + if (ret != -EPROBE_DEFER)
> + dev_warn(sdev->dev, "init of i915 and HDMI codec failed: %i\n", ret);
> + return ret;
> + }
>
> /* get controller capabilities */
> ret = hda_dsp_ctrl_get_caps(sdev);
> @@ -1115,14 +1118,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
> sdev->pdata->hw_pdata = hdev;
> hdev->desc = chip;
>
> - hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",
> - PLATFORM_DEVID_NONE,
> - NULL, 0);
> - if (IS_ERR(hdev->dmic_dev)) {
> - dev_err(sdev->dev, "error: failed to create DMIC device\n");
> - return PTR_ERR(hdev->dmic_dev);
> - }
> -
> /*
> * use position update IPC if either it is forced
> * or we don't have other choice
> @@ -1142,6 +1137,15 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
> if (ret < 0)
> goto hdac_bus_unmap;
>
> + hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",
> + PLATFORM_DEVID_NONE,
> + NULL, 0);
> + if (IS_ERR(hdev->dmic_dev)) {
> + dev_err(sdev->dev, "error: failed to create DMIC device\n");
> + ret = PTR_ERR(hdev->dmic_dev);
> + goto hdac_exit;
> + }
> +
I am not following why we have to move dmic-related code, can we try to
move this in a separate patch?
> if (sdev->dspless_mode_selected)
> goto skip_dsp_setup;
>
> @@ -1150,7 +1154,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
> if (!sdev->bar[HDA_DSP_BAR]) {
> dev_err(sdev->dev, "error: ioremap error\n");
> ret = -ENXIO;
> - goto hdac_bus_unmap;
> + goto platform_unreg;
> }
>
> sdev->mmio_bar = HDA_DSP_BAR;
> @@ -1248,10 +1252,12 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
> /* dsp_unmap: not currently used */
> if (!sdev->dspless_mode_selected)
> iounmap(sdev->bar[HDA_DSP_BAR]);
> -hdac_bus_unmap:
> +platform_unreg:
> platform_device_unregister(hdev->dmic_dev);
> - iounmap(bus->remap_addr);
> +hdac_exit:
> hda_codec_i915_exit(sdev);
> +hdac_bus_unmap:
> + iounmap(bus->remap_addr);
> err:
> return ret;
> }
> diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c
> index f5ece43d0ec24..0fa424613082e 100644
> --- a/sound/soc/sof/sof-pci-dev.c
> +++ b/sound/soc/sof/sof-pci-dev.c
> @@ -339,8 +339,7 @@ void sof_pci_remove(struct pci_dev *pci)
> snd_sof_device_remove(&pci->dev);
>
> /* follow recommendation in pci-driver.c to increment usage counter */
> - if (snd_sof_device_probe_completed(&pci->dev) &&
> - !(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME))
> + if (!(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME))
> pm_runtime_get_noresume(&pci->dev);
>
> /* release pci regions and disable device */
> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index d4f6702e93dcb..71db636cfdccc 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -564,10 +564,6 @@ struct snd_sof_dev {
> enum sof_fw_state fw_state;
> bool first_boot;
>
> - /* work queue in case the probe is implemented in two steps */
> - struct work_struct probe_work;
> - bool probe_completed;
> -
> /* DSP HW differentiation */
> struct snd_sof_pdata *pdata;
>
> @@ -675,7 +671,6 @@ struct snd_sof_dev {
> int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data);
> int snd_sof_device_remove(struct device *dev);
> int snd_sof_device_shutdown(struct device *dev);
> -bool snd_sof_device_probe_completed(struct device *dev);
>
> int snd_sof_runtime_suspend(struct device *dev);
> int snd_sof_runtime_resume(struct device *dev);
