| Message ID | 20230719164141.228073-9-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sound: Use -EPROBE_DEFER instead of i915 module loading. | expand |
On 19/07/2023 19:41, Maarten Lankhorst wrote: > This was only used to allow modprobing i915, by converting to the > -EPROBE_DEFER mechanism, it can be completely removed, and is in > fact counterproductive since -EPROBE_DEFER otherwise won't be > handled correctly. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Acked-by: Matthew Auld <matthew.auld@intel.com> > Acked-by: Mark Brown <broonie@kernel.org> > --- > sound/soc/sof/Kconfig | 19 ----------------- > sound/soc/sof/core.c | 38 ++------------------------------- > sound/soc/sof/intel/Kconfig | 1 - > sound/soc/sof/intel/hda-codec.c | 2 +- > sound/soc/sof/intel/hda.c | 32 ++++++++++++++++----------- > sound/soc/sof/sof-pci-dev.c | 3 +-- > sound/soc/sof/sof-priv.h | 5 ----- > 7 files changed, 23 insertions(+), 77 deletions(-) > > diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig > index 80361139a49ad..8ee39e5558062 100644 > --- a/sound/soc/sof/Kconfig > +++ b/sound/soc/sof/Kconfig > @@ -82,17 +82,6 @@ config SND_SOC_SOF_DEVELOPER_SUPPORT > > if SND_SOC_SOF_DEVELOPER_SUPPORT > > -config SND_SOC_SOF_FORCE_PROBE_WORKQUEUE > - bool "SOF force probe workqueue" > - select SND_SOC_SOF_PROBE_WORK_QUEUE > - help > - This option forces the use of a probe workqueue, which is only used > - when HDaudio is enabled due to module dependencies. Forcing this > - option is intended for debug only, but this should not add any > - functional issues in nominal cases. > - Say Y if you are involved in SOF development and need this option. > - If not, select N. > - > config SND_SOC_SOF_NOCODEC > tristate > > @@ -271,14 +260,6 @@ config SND_SOC_SOF > module dependencies but since the module or built-in type is decided > at the top level it doesn't matter. > > -config SND_SOC_SOF_PROBE_WORK_QUEUE > - bool > - help > - This option is not user-selectable but automagically handled by > - 'select' statements at a higher level. > - When selected, the probe is handled in two steps, for example to > - avoid lockdeps if request_module is used in the probe. > - > # Supported IPC versions > config SND_SOC_SOF_IPC3 > bool > diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c > index 30db685cc5f4b..cdf86dc4a8a87 100644 > --- a/sound/soc/sof/core.c > +++ b/sound/soc/sof/core.c > @@ -191,7 +191,8 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) > /* probe the DSP hardware */ > ret = snd_sof_probe(sdev); > if (ret < 0) { > - dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret); > + if (ret != -EPROBE_DEFER) > + dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret); While at it, can you drop thee "error:" prefix from the print? > goto probe_err; > } > > @@ -309,8 +310,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) > if (plat_data->sof_probe_complete) > plat_data->sof_probe_complete(sdev->dev); > > - sdev->probe_completed = true; > - > return 0; > > sof_machine_err: > @@ -336,19 +335,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) > return ret; > } > > -static void sof_probe_work(struct work_struct *work) > -{ > - struct snd_sof_dev *sdev = > - container_of(work, struct snd_sof_dev, probe_work); > - int ret; > - > - ret = sof_probe_continue(sdev); > - if (ret < 0) { > - /* errors cannot be propagated, log */ > - dev_err(sdev->dev, "error: %s failed err: %d\n", __func__, ret); > - } > -} > - > int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data) > { > struct snd_sof_dev *sdev; > @@ -436,33 +422,16 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data) > > sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED); > > - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) { > - INIT_WORK(&sdev->probe_work, sof_probe_work); > - schedule_work(&sdev->probe_work); > - return 0; > - } > - > return sof_probe_continue(sdev); > } > EXPORT_SYMBOL(snd_sof_device_probe); > > -bool snd_sof_device_probe_completed(struct device *dev) > -{ > - struct snd_sof_dev *sdev = dev_get_drvdata(dev); > - > - return sdev->probe_completed; > -} > -EXPORT_SYMBOL(snd_sof_device_probe_completed); > - > int snd_sof_device_remove(struct device *dev) > { > struct snd_sof_dev *sdev = dev_get_drvdata(dev); > struct snd_sof_pdata *pdata = sdev->pdata; > int ret; > > - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) > - cancel_work_sync(&sdev->probe_work); > - > /* > * Unregister any registered client device first before IPC and debugfs > * to allow client drivers to be removed cleanly > @@ -501,9 +470,6 @@ int snd_sof_device_shutdown(struct device *dev) > { > struct snd_sof_dev *sdev = dev_get_drvdata(dev); > > - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) > - cancel_work_sync(&sdev->probe_work); > - > if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) { > sof_fw_trace_free(sdev); > return snd_sof_shutdown(sdev); > diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig > index 69c1a370d3b61..d9e87a91670a3 100644 > --- a/sound/soc/sof/intel/Kconfig > +++ b/sound/soc/sof/intel/Kconfig > @@ -293,7 +293,6 @@ config SND_SOC_SOF_HDA_LINK > config SND_SOC_SOF_HDA_AUDIO_CODEC > bool "SOF support for HDAudio codecs" > depends on SND_SOC_SOF_HDA_LINK > - select SND_SOC_SOF_PROBE_WORK_QUEUE > help > This adds support for HDAudio codecs with Sound Open Firmware > for Intel(R) platforms. > diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c > index f1fd5b44aaac9..344b61576c0e3 100644 > --- a/sound/soc/sof/intel/hda-codec.c > +++ b/sound/soc/sof/intel/hda-codec.c > @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) > return 0; > > /* i915 exposes a HDA codec for HDMI audio */ > - ret = snd_hdac_i915_init(bus, true); > + ret = snd_hdac_i915_init(bus, false); > if (ret < 0) > return ret; > > diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c > index 64bebe1a72bbc..a8b7a68142c05 100644 > --- a/sound/soc/sof/intel/hda.c > +++ b/sound/soc/sof/intel/hda.c > @@ -801,8 +801,11 @@ static int hda_init(struct snd_sof_dev *sdev) > > /* init i915 and HDMI codecs */ > ret = hda_codec_i915_init(sdev); > - if (ret < 0) > - dev_warn(sdev->dev, "init of i915 and HDMI codec failed\n"); > + if (ret < 0) { > + if (ret != -EPROBE_DEFER) > + dev_warn(sdev->dev, "init of i915 and HDMI codec failed: %i\n", ret); > + return ret; > + } > > /* get controller capabilities */ > ret = hda_dsp_ctrl_get_caps(sdev); > @@ -1115,14 +1118,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) > sdev->pdata->hw_pdata = hdev; > hdev->desc = chip; > > - hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", > - PLATFORM_DEVID_NONE, > - NULL, 0); > - if (IS_ERR(hdev->dmic_dev)) { > - dev_err(sdev->dev, "error: failed to create DMIC device\n"); > - return PTR_ERR(hdev->dmic_dev); > - } > - > /* > * use position update IPC if either it is forced > * or we don't have other choice > @@ -1142,6 +1137,15 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) > if (ret < 0) > goto hdac_bus_unmap; > > + hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", > + PLATFORM_DEVID_NONE, > + NULL, 0); > + if (IS_ERR(hdev->dmic_dev)) { > + dev_err(sdev->dev, "error: failed to create DMIC device\n"); >From here also, let's not add them back. > + ret = PTR_ERR(hdev->dmic_dev); > + goto hdac_exit; > + } > + > if (sdev->dspless_mode_selected) > goto skip_dsp_setup; > > @@ -1150,7 +1154,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) > if (!sdev->bar[HDA_DSP_BAR]) { > dev_err(sdev->dev, "error: ioremap error\n"); > ret = -ENXIO; > - goto hdac_bus_unmap; > + goto platform_unreg; > } > > sdev->mmio_bar = HDA_DSP_BAR; > @@ -1248,10 +1252,12 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) > /* dsp_unmap: not currently used */ > if (!sdev->dspless_mode_selected) > iounmap(sdev->bar[HDA_DSP_BAR]); > -hdac_bus_unmap: > +platform_unreg: > platform_device_unregister(hdev->dmic_dev); > - iounmap(bus->remap_addr); > +hdac_exit: > hda_codec_i915_exit(sdev); > +hdac_bus_unmap: > + iounmap(bus->remap_addr); > err: > return ret; > } > diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c > index f5ece43d0ec24..0fa424613082e 100644 > --- a/sound/soc/sof/sof-pci-dev.c > +++ b/sound/soc/sof/sof-pci-dev.c > @@ -339,8 +339,7 @@ void sof_pci_remove(struct pci_dev *pci) > snd_sof_device_remove(&pci->dev); > > /* follow recommendation in pci-driver.c to increment usage counter */ > - if (snd_sof_device_probe_completed(&pci->dev) && > - !(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) > + if (!(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) > pm_runtime_get_noresume(&pci->dev); > > /* release pci regions and disable device */ > diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h > index d4f6702e93dcb..71db636cfdccc 100644 > --- a/sound/soc/sof/sof-priv.h > +++ b/sound/soc/sof/sof-priv.h > @@ -564,10 +564,6 @@ struct snd_sof_dev { > enum sof_fw_state fw_state; > bool first_boot; > > - /* work queue in case the probe is implemented in two steps */ > - struct work_struct probe_work; > - bool probe_completed; > - > /* DSP HW differentiation */ > struct snd_sof_pdata *pdata; > > @@ -675,7 +671,6 @@ struct snd_sof_dev { > int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data); > int snd_sof_device_remove(struct device *dev); > int snd_sof_device_shutdown(struct device *dev); > -bool snd_sof_device_probe_completed(struct device *dev); > > int snd_sof_runtime_suspend(struct device *dev); > int snd_sof_runtime_resume(struct device *dev);
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@linux.intel.com> > Acked-by: Matthew Auld <matthew.auld@intel.com> > Acked-by: Mark Brown <broonie@kernel.org> > --- > 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);
Hey, On 2023-07-24 13:32, Pierre-Louis Bossart wrote: > > > On 7/19/23 18:41, Maarten Lankhorst wrote: >> This was only used to allow modprobing i915, by converting to the >> -EPROBE_DEFER mechanism, it can be completely removed, and is in >> fact counterproductive since -EPROBE_DEFER otherwise won't be >> handled correctly. > > I personally remember only that the request_module("i915") was the main > motivation for the use of the workqueue, but when it comes to the > HDaudio codec management we don't even know what we don't know. > > I am a bit worried that the snd-hda-intel driver keeps the workqueue for > HDaudio codec initialization, and this patch removes the workqueue > completely for SOF. That doesn't seem right. Either both drivers need a > workqueue or none need a workqueue. > > Maybe what we need is to move the i915/xe initialization out of the > workqueue, and see in a second pass if that workqueue can be safely > removed from the SOF driver? > As I mentioned in some of the other sound driver conversions. I believe it's possible to completely kill off most workqueues. However, I don´t have the hardware or knowledge to test it. I saw that the SOF had the non-workqueue path already, so it felt less risky to simply convert it to always use that path. avs/skylake drivers should be easy to convert too. This is why I left the comment: "Removing the workqueue would simplify init even further, but is left as exercise for the reviewer." HDA-intel has this retry-probe logic used on AMD's, which makes me more hesitant to convert it. I wanted to tackle one problem at a time, I believe workqueue removal can be done by anyone. Cheers, ~Maarten
On Tue, 25 Jul 2023 12:29:07 +0200, Maarten Lankhorst wrote: > > Hey, > > On 2023-07-24 13:32, Pierre-Louis Bossart wrote: > > > > > > On 7/19/23 18:41, Maarten Lankhorst wrote: > >> This was only used to allow modprobing i915, by converting to the > >> -EPROBE_DEFER mechanism, it can be completely removed, and is in > >> fact counterproductive since -EPROBE_DEFER otherwise won't be > >> handled correctly. > > > > I personally remember only that the request_module("i915") was the main > > motivation for the use of the workqueue, but when it comes to the > > HDaudio codec management we don't even know what we don't know. > > > > I am a bit worried that the snd-hda-intel driver keeps the workqueue for > > HDaudio codec initialization, and this patch removes the workqueue > > completely for SOF. That doesn't seem right. Either both drivers need a > > workqueue or none need a workqueue. > > > > Maybe what we need is to move the i915/xe initialization out of the > > workqueue, and see in a second pass if that workqueue can be safely > > removed from the SOF driver? > > > As I mentioned in some of the other sound driver conversions. I > believe it's possible to completely kill off most workqueues. > > However, I don´t have the hardware or knowledge to test it. I saw > that the SOF had the non-workqueue path already, so it felt less risky > to simply convert it to always use that path. > > avs/skylake drivers should be easy to convert too. This is why I left > the comment: "Removing the workqueue would simplify init even further, > but is left as exercise for the reviewer." > > HDA-intel has this retry-probe logic used on AMD's, > which makes me more hesitant to convert it. Yes, HDA-Intel requires either a workqueue or async firmware-loader callback because there is some codec module-autoload mechanism that may happen during the probe phase. It's not only on AMD, but it's required in general for all codecs. Takashi
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index 80361139a49ad..8ee39e5558062 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -82,17 +82,6 @@ config SND_SOC_SOF_DEVELOPER_SUPPORT if SND_SOC_SOF_DEVELOPER_SUPPORT -config SND_SOC_SOF_FORCE_PROBE_WORKQUEUE - bool "SOF force probe workqueue" - select SND_SOC_SOF_PROBE_WORK_QUEUE - help - This option forces the use of a probe workqueue, which is only used - when HDaudio is enabled due to module dependencies. Forcing this - option is intended for debug only, but this should not add any - functional issues in nominal cases. - Say Y if you are involved in SOF development and need this option. - If not, select N. - config SND_SOC_SOF_NOCODEC tristate @@ -271,14 +260,6 @@ config SND_SOC_SOF module dependencies but since the module or built-in type is decided at the top level it doesn't matter. -config SND_SOC_SOF_PROBE_WORK_QUEUE - bool - help - This option is not user-selectable but automagically handled by - 'select' statements at a higher level. - When selected, the probe is handled in two steps, for example to - avoid lockdeps if request_module is used in the probe. - # Supported IPC versions config SND_SOC_SOF_IPC3 bool diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 30db685cc5f4b..cdf86dc4a8a87 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -191,7 +191,8 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) /* probe the DSP hardware */ ret = snd_sof_probe(sdev); if (ret < 0) { - dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret); + if (ret != -EPROBE_DEFER) + dev_err(sdev->dev, "error: failed to probe DSP %d\n", ret); goto probe_err; } @@ -309,8 +310,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) if (plat_data->sof_probe_complete) plat_data->sof_probe_complete(sdev->dev); - sdev->probe_completed = true; - return 0; sof_machine_err: @@ -336,19 +335,6 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) return ret; } -static void sof_probe_work(struct work_struct *work) -{ - struct snd_sof_dev *sdev = - container_of(work, struct snd_sof_dev, probe_work); - int ret; - - ret = sof_probe_continue(sdev); - if (ret < 0) { - /* errors cannot be propagated, log */ - dev_err(sdev->dev, "error: %s failed err: %d\n", __func__, ret); - } -} - int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data) { struct snd_sof_dev *sdev; @@ -436,33 +422,16 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data) sof_set_fw_state(sdev, SOF_FW_BOOT_NOT_STARTED); - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) { - INIT_WORK(&sdev->probe_work, sof_probe_work); - schedule_work(&sdev->probe_work); - return 0; - } - return sof_probe_continue(sdev); } EXPORT_SYMBOL(snd_sof_device_probe); -bool snd_sof_device_probe_completed(struct device *dev) -{ - struct snd_sof_dev *sdev = dev_get_drvdata(dev); - - return sdev->probe_completed; -} -EXPORT_SYMBOL(snd_sof_device_probe_completed); - int snd_sof_device_remove(struct device *dev) { struct snd_sof_dev *sdev = dev_get_drvdata(dev); struct snd_sof_pdata *pdata = sdev->pdata; int ret; - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) - cancel_work_sync(&sdev->probe_work); - /* * Unregister any registered client device first before IPC and debugfs * to allow client drivers to be removed cleanly @@ -501,9 +470,6 @@ int snd_sof_device_shutdown(struct device *dev) { struct snd_sof_dev *sdev = dev_get_drvdata(dev); - if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)) - cancel_work_sync(&sdev->probe_work); - if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) { sof_fw_trace_free(sdev); return snd_sof_shutdown(sdev); diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 69c1a370d3b61..d9e87a91670a3 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -293,7 +293,6 @@ config SND_SOC_SOF_HDA_LINK config SND_SOC_SOF_HDA_AUDIO_CODEC bool "SOF support for HDAudio codecs" depends on SND_SOC_SOF_HDA_LINK - select SND_SOC_SOF_PROBE_WORK_QUEUE help This adds support for HDAudio codecs with Sound Open Firmware for Intel(R) platforms. diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index f1fd5b44aaac9..344b61576c0e3 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -415,7 +415,7 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev) return 0; /* i915 exposes a HDA codec for HDMI audio */ - ret = snd_hdac_i915_init(bus, true); + ret = snd_hdac_i915_init(bus, false); if (ret < 0) return ret; diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 64bebe1a72bbc..a8b7a68142c05 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -801,8 +801,11 @@ static int hda_init(struct snd_sof_dev *sdev) /* init i915 and HDMI codecs */ ret = hda_codec_i915_init(sdev); - if (ret < 0) - dev_warn(sdev->dev, "init of i915 and HDMI codec failed\n"); + if (ret < 0) { + if (ret != -EPROBE_DEFER) + dev_warn(sdev->dev, "init of i915 and HDMI codec failed: %i\n", ret); + return ret; + } /* get controller capabilities */ ret = hda_dsp_ctrl_get_caps(sdev); @@ -1115,14 +1118,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) sdev->pdata->hw_pdata = hdev; hdev->desc = chip; - hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", - PLATFORM_DEVID_NONE, - NULL, 0); - if (IS_ERR(hdev->dmic_dev)) { - dev_err(sdev->dev, "error: failed to create DMIC device\n"); - return PTR_ERR(hdev->dmic_dev); - } - /* * use position update IPC if either it is forced * or we don't have other choice @@ -1142,6 +1137,15 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) if (ret < 0) goto hdac_bus_unmap; + hdev->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec", + PLATFORM_DEVID_NONE, + NULL, 0); + if (IS_ERR(hdev->dmic_dev)) { + dev_err(sdev->dev, "error: failed to create DMIC device\n"); + ret = PTR_ERR(hdev->dmic_dev); + goto hdac_exit; + } + if (sdev->dspless_mode_selected) goto skip_dsp_setup; @@ -1150,7 +1154,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) if (!sdev->bar[HDA_DSP_BAR]) { dev_err(sdev->dev, "error: ioremap error\n"); ret = -ENXIO; - goto hdac_bus_unmap; + goto platform_unreg; } sdev->mmio_bar = HDA_DSP_BAR; @@ -1248,10 +1252,12 @@ int hda_dsp_probe(struct snd_sof_dev *sdev) /* dsp_unmap: not currently used */ if (!sdev->dspless_mode_selected) iounmap(sdev->bar[HDA_DSP_BAR]); -hdac_bus_unmap: +platform_unreg: platform_device_unregister(hdev->dmic_dev); - iounmap(bus->remap_addr); +hdac_exit: hda_codec_i915_exit(sdev); +hdac_bus_unmap: + iounmap(bus->remap_addr); err: return ret; } diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index f5ece43d0ec24..0fa424613082e 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -339,8 +339,7 @@ void sof_pci_remove(struct pci_dev *pci) snd_sof_device_remove(&pci->dev); /* follow recommendation in pci-driver.c to increment usage counter */ - if (snd_sof_device_probe_completed(&pci->dev) && - !(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) + if (!(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) pm_runtime_get_noresume(&pci->dev); /* release pci regions and disable device */ diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index d4f6702e93dcb..71db636cfdccc 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -564,10 +564,6 @@ struct snd_sof_dev { enum sof_fw_state fw_state; bool first_boot; - /* work queue in case the probe is implemented in two steps */ - struct work_struct probe_work; - bool probe_completed; - /* DSP HW differentiation */ struct snd_sof_pdata *pdata; @@ -675,7 +671,6 @@ struct snd_sof_dev { int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data); int snd_sof_device_remove(struct device *dev); int snd_sof_device_shutdown(struct device *dev); -bool snd_sof_device_probe_completed(struct device *dev); int snd_sof_runtime_suspend(struct device *dev); int snd_sof_runtime_resume(struct device *dev);