- Sound-open-firmware - mailman.alsa-project.org

[PATCH AUTOSEL 6.4 50/58] ASoC: SOF: core: Free the firmware trace before calling snd_sof_shutdown()
by Sasha Levin 24 Jul '23

24 Jul '23

From: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> [ Upstream commit d389dcb3a48cec4f03c16434c0bf98a4c635372a ] The shutdown is called on reboot/shutdown of the machine. At this point the firmware tracing cannot be used anymore but in case of IPC3 it is using and keeping a DMA channel active (dtrace). For Tiger Lake platforms we have a quirk in place to fix rare reboot issues when a DMA was active before rebooting the system. If the tracing is enabled this quirk will be always used and a print appears on the kernel log which might be misleading or not even correct. Release the fw tracing before executing the shutdown to make sure that this known DMA user is cleared away. Reviewed-by: Kai Vehmanen <kai.vehmanen(a)linux.intel.com> Reviewed-by: Daniel Baluta <daniel.baluta(a)nxp.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> Reviewed-by: Rander Wang <rander.wang(a)intel.com> Reviewed-by: Bard Liao <yung-chuan.liao(a)linux.intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Link: https://lore.kernel.org/r/20230616100039.378150-4-pierre-louis.bossart@linu… Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 9a9d82220fd0d..30db685cc5f4b 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -504,8 +504,10 @@ int snd_sof_device_shutdown(struct device *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) + if (sdev->fw_state == SOF_FW_BOOT_COMPLETE) { + sof_fw_trace_free(sdev); return snd_sof_shutdown(sdev); + } return 0; } -- 2.39.2

1 0

[PATCH AUTOSEL 6.4 29/58] ASoC: SOF: Intel: fix SoundWire/HDaudio mutual exclusion
by Sasha Levin 24 Jul '23

24 Jul '23

From: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> [ Upstream commit f751b99255cacd9ffe8c4bbf99767ad670cee1f7 ] The functionality described in Commit 61bef9e68dca ("ASoC: SOF: Intel: hda: enforce exclusion between HDaudio and SoundWire") does not seem to be properly implemented with two issues that need to be corrected. a) The test used is incorrect when DisplayAudio codecs are not supported. b) Conversely when only Display Audio codecs can be found, we do want to start the SoundWire links, if any. That will help add the relevant topologies and machine descriptors, and identify cases where the SoundWire information in ACPI needs to be modified with a quirk. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao(a)linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> Link: https://lore.kernel.org/r/20230606222529.57156-2-pierre-louis.bossart@linux… Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/intel/hda.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 3153e21f100ab..3853582e32e12 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -1343,12 +1343,22 @@ static void hda_generic_machine_select(struct snd_sof_dev *sdev, hda_mach->mach_params.dmic_num = dmic_num; pdata->tplg_filename = tplg_filename; - if (codec_num == 2) { + if (codec_num == 2 || + (codec_num == 1 && !HDA_IDISP_CODEC(bus->codec_mask))) { /* * Prevent SoundWire links from starting when an external * HDaudio codec is used */ hda_mach->mach_params.link_mask = 0; + } else { + /* + * Allow SoundWire links to start when no external HDaudio codec + * was detected. This will not create a SoundWire card but + * will help detect if any SoundWire codec reports as ATTACHED. + */ + struct sof_intel_hda_dev *hdev = sdev->pdata->hw_pdata; + + hda_mach->mach_params.link_mask = hdev->info.link_mask; } *mach = hda_mach; -- 2.39.2

1 0

[PATCH AUTOSEL 6.4 15/58] ASoC: SOF: amd: Add pci revision id check
by Sasha Levin 24 Jul '23

24 Jul '23

From: Venkata Prasad Potturu <venkataprasad.potturu(a)amd.com> [ Upstream commit 1d4a84632b90d88316986b05bcdfe715399a33db ] Add pci revision id check for renoir and rembrandt platforms. Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu(a)amd.com> Link: https://lore.kernel.org/r/20230523072009.2379198-1-venkataprasad.potturu@am… Signed-off-by: Mark Brown <broonie(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- sound/soc/sof/amd/acp.h | 3 +++ sound/soc/sof/amd/pci-rmb.c | 3 +++ sound/soc/sof/amd/pci-rn.c | 3 +++ 3 files changed, 9 insertions(+) diff --git a/sound/soc/sof/amd/acp.h b/sound/soc/sof/amd/acp.h index 1c535cc6c3a95..dc624f727aa37 100644 --- a/sound/soc/sof/amd/acp.h +++ b/sound/soc/sof/amd/acp.h @@ -55,6 +55,9 @@ #define ACP_DSP_TO_HOST_IRQ 0x04 +#define ACP_RN_PCI_ID 0x01 +#define ACP_RMB_PCI_ID 0x6F + #define HOST_BRIDGE_CZN 0x1630 #define HOST_BRIDGE_RMB 0x14B5 #define ACP_SHA_STAT 0x8000 diff --git a/sound/soc/sof/amd/pci-rmb.c b/sound/soc/sof/amd/pci-rmb.c index eaf70ea6e556e..58b3092425f1a 100644 --- a/sound/soc/sof/amd/pci-rmb.c +++ b/sound/soc/sof/amd/pci-rmb.c @@ -65,6 +65,9 @@ static int acp_pci_rmb_probe(struct pci_dev *pci, const struct pci_device_id *pc { unsigned int flag; + if (pci->revision != ACP_RMB_PCI_ID) + return -ENODEV; + flag = snd_amd_acp_find_config(pci); if (flag != FLAG_AMD_SOF && flag != FLAG_AMD_SOF_ONLY_DMIC) return -ENODEV; diff --git a/sound/soc/sof/amd/pci-rn.c b/sound/soc/sof/amd/pci-rn.c index 4809cb644152b..7409e21ce5aa7 100644 --- a/sound/soc/sof/amd/pci-rn.c +++ b/sound/soc/sof/amd/pci-rn.c @@ -65,6 +65,9 @@ static int acp_pci_rn_probe(struct pci_dev *pci, const struct pci_device_id *pci { unsigned int flag; + if (pci->revision != ACP_RN_PCI_ID) + return -ENODEV; + flag = snd_amd_acp_find_config(pci); if (flag != FLAG_AMD_SOF && flag != FLAG_AMD_SOF_ONLY_DMIC) return -ENODEV; -- 2.39.2

1 0

Re: [PATCH v2 4/9] ALSA: hda/i915: Allow xe as match for i915_component_master_match
by Péter Ujfalusi 21 Jul '23

21 Jul '23

On 19/07/2023 19:41, Maarten Lankhorst wrote: > xe is a new driver for intel GPU's that shares the sound related code > with i915. > > Don't allow it to be modprobed though; the module is not upstream yet > and we should exclusively use the EPROBE_DEFER mechanism. Reviewed-by: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > --- > sound/hda/hdac_i915.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c > index 961fcd3397f40..12c1f8d93499f 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; -- Péter

1 0

Re: [PATCH v2 0/9] sound: Use -EPROBE_DEFER instead of i915 module loading.
by Péter Ujfalusi 21 Jul '23

21 Jul '23

Hi Maarten, On 19/07/2023 19:41, Maarten Lankhorst wrote: > Explicitly loading i915 becomes a problem when upstreaming the new intel driver > for Tiger Lake and higher graphics (xe). By loading i915, it doesn't wait for > driver load of xe, and will fail completely before it loads. > > -EPROBE_DEFER has to be returned before any device is created in probe(), > otherwise the removal of the device will cause EPROBE_DEFER to try again > in an infinite loop. > > The conversion is done in gradual steps. First I add an argument to > snd_hdac_i915_init to allow for -EPROBE_DEFER so I can convert each driver > separately. Then I convert each driver to move snd_hdac_i915_init out of the > workqueue. Finally I drop the ability to choose modprobe behavior after the > last user is converted. > > I suspect the avs and skylake drivers used snd_hdac_i915_init purely for the > modprobe, but I don't have the hardware to test if it can be safely removed. > It can still be done easily in a followup patch to simplify probing. Apart from the few comments I had, this looks great and works OK on the machines I have tested (iow, no regression so far). Thank you for the work! -- Péter > > --- > New since first version: > > - snd_hda_core.gpu_bind is added as a mechanism to force gpu binding, > for testing. snd_hda_core.gpu_bind=0 forces waiting for GPU bind to > off, snd_hda_core.gpu_bind=1 forces waiting for gpu bind. Default > setting depends on whether kernel booted with nomodeset. > - Incorporated all feedback review. > > Cc: Jaroslav Kysela <perex(a)perex.cz> > Cc: Takashi Iwai <tiwai(a)suse.com> > Cc: Cezary Rojewski <cezary.rojewski(a)intel.com> > Cc: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> > Cc: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> > Cc: Peter Ujfalusi <peter.ujfalusi(a)linux.intel.com> > Cc: Bard Liao <yung-chuan.liao(a)linux.intel.com> > Cc: Ranjani Sridharan <ranjani.sridharan(a)linux.intel.com> > Cc: Kai Vehmanen <kai.vehmanen(a)linux.intel.com> > Cc: Mark Brown <broonie(a)kernel.org> > Cc: Daniel Baluta <daniel.baluta(a)nxp.com> > Cc: alsa-devel(a)alsa-project.org > Cc: linux-kernel(a)vger.kernel.org > Cc: sound-open-firmware(a)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: Remove deferred probe for SOF > ALSA: hda/i915: Remove extra argument from snd_hdac_i915_init > > sound/hda/hdac_i915.c | 25 ++++++++------- > sound/pci/hda/hda_intel.c | 60 ++++++++++++++++++----------------- > sound/soc/intel/avs/core.c | 13 +++++--- > sound/soc/intel/skylake/skl.c | 31 ++++++------------ > sound/soc/sof/Kconfig | 19 ----------- > sound/soc/sof/core.c | 38 ++-------------------- > sound/soc/sof/intel/Kconfig | 1 - > sound/soc/sof/intel/hda.c | 32 +++++++++++-------- > sound/soc/sof/sof-pci-dev.c | 3 +- > sound/soc/sof/sof-priv.h | 5 --- > 10 files changed, 85 insertions(+), 142 deletions(-) >

1 0

Re: [PATCH v2 8/9] ASoC: SOF: Intel: Remove deferred probe for SOF
by Péter Ujfalusi 21 Jul '23

21 Jul '23

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(a)linux.intel.com> > Acked-by: Matthew Auld <matthew.auld(a)intel.com> > Acked-by: Mark Brown <broonie(a)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); -- Péter

1 0

Re: [PATCH v2 3/9] ALSA: hda/i915: Add an allow_modprobe argument to snd_hdac_i915_init
by Péter Ujfalusi 21 Jul '23

21 Jul '23

On 19/07/2023 19:41, Maarten Lankhorst wrote: > 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. > > Changes since v1: > - Use dev_err_probe to set a probe reason for debugfs' deferred_devices. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > --- > 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/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c > index c32709fa4115f..961fcd3397f40 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; Add one blank line here. > snd_hdac_acomp_exit(bus); > - return -ENODEV; > + return dev_err_probe(bus->dev, err, "couldn't bind with audio component\n"); > } > return 0; > } -- Péter

1 0

Re: [PATCH v2 0/9] sound: Use -EPROBE_DEFER instead of i915 module loading.
by Kai Vehmanen 21 Jul '23

21 Jul '23

Hi, On Wed, 19 Jul 2023, Maarten Lankhorst wrote: > 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. thanks, series looks good to me now. We'll need to adopt the new gpu_bind parameter in a number of CI systems (where we test without i915/xe), but this looks perfectly doable. I'll give my Reviewed-by: Kai Vehmanen <kai.vehmanen(a)linux.intel.com> ... for the hdac_i915.c changes. For AVS and SOF, I'd ask for some more review time to allow Cezary, Pierre et al to weigh in. I don't personally recall e.g. where we've used CONFIG_SOF_FORCE_PROBE_WORKQUEUE and do we have grounds to keep it even if workqueue is no longer set for HDA codec support. Br, Kai

1 0

Re: [PATCH 3/7] ASoC: Intel: avs: Move snd_hdac_i915_init to before probe_work.
by Cezary Rojewski 19 Jul '23

19 Jul '23

On 2023-07-18 10:45 AM, 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(a)linux.intel.com> > --- > 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 3311a6f142001..d3a7f42387e9b 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_unmaster; > + else if (ret < 0) > + dev_info(bus->dev, "i915 init unsuccessful: %d\n", ret); > + While our tests are currently passing I have my doubts about EPROBE_DEFER. We do want to have audio functionality there even if some problems with HDMI arise along the way - some audio is better than no audio. Here, i915 may ruin the day for a platform equipped with hda/hdmi/i2c/dmic chips simultaneously. Also, why call snd_hdac_i915_init() _after_ setting drvdata? > schedule_work(&adev->probe_work); > > return 0; > > +err_unmaster: > + pci_clear_master(pci); > + pci_set_drvdata(pci, NULL); Not a fan. It's hard to grasp entire error-step within name of a single label. Thus I'd suggest "err_<cause>" naming pattern instead. Even here, under "unmaster" you hid clearing master and drvdata both. Let's do "err_i915_init" instead. > err_acquire_irq: > snd_hdac_bus_free_stream_pages(bus); > snd_hdac_ext_stream_free_all(bus);

2 1

Re: [PATCH 6/7] ASoC: SOF: Intel: Remove deferred probe for SOF
by Kai Vehmanen 19 Jul '23

19 Jul '23

Hi, On Wed, 19 Jul 2023, Maarten Lankhorst wrote: > On Tue, 18 Jul 2023 19:04:41 +0200, Kai Vehmanen wrote: >> My only bigger concern is corner cases where the display PCI device is on >> the bus and visible to kernel, but for some reason there is no working >> driver in the system or it is disabled. > > Yeah, I have no answer for this. My guess is that in an ideal world, the optional features > related to HDMI outputs would be put in a separate sub-driver, which could -EPROBE_DEFER. > Only when this driver loads, features related to display will work, but the main audio driver > could still load. in longer term, we have ongoing work in SOF to allow exposing multiple cards (e.g. to have a separate card for HDMI/DP PCM devices), and we are continuously working at improving the data we get from ACPI to have less guesswork in the driver. But this really doesn't help in the shortterm and/or cover all scenarios. So for now, this is legacy we just need to deal with. OTOH, I do agree that... > A module option to snd_hdac_i915_init would probably be the least of all evils here. > > I see the removal of the 60 second timeout as a good thing regardless. :-) Usually when nomodeset is used, it's just for safe > mode. > > With the addition of the xe driver, blindly modprobing i915 will fall apart regardless. The modprobing of i915 from the audio driver, has always felt a bit out-of-place, and with the xe driver, this simply won't scale anymore. The test results so far look good and this patchset works ok even if some of the more complex multi-GPU configurations we have, so I think with a module option to snd_hdac_i915, I'd be ready to go with this. Br, Kai

1 0