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

[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

Re: [PATCH] ASoC: SOF: Intel: Remove deferred probe for SOF
by Takashi Iwai 19 Jul '23

19 Jul '23

On Wed, 19 Jul 2023 14:13:59 +0200, Maarten Lankhorst wrote: > > > On 2023-07-19 13:06, Takashi Iwai wrote: > > On Wed, 19 Jul 2023 11:48:06 +0200, > > Maarten Lankhorst wrote: > >> > >> The 60 seconds timeout is a thing "better than complete disablement", > >> so it's not ideal, either. Maybe we can add something like the > >> following: > >> - Check when the deferred probe takes too long, and warn > >> it > >> - Provide some runtime option to disable the component binding, so > >> that user can work around it if needed > >> A module option to snd_hdac_i915_init would probably be the > >> least of all evils > >> here. > > > > Yes, probably it's the easiest option and sufficient. > > > > > > thanks, > > > > Takashi > Hey, > > Patch below, can be applied immediately iresspective of the other patches. > > ---->8---------- > > 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. The check of video_firmware_drivers_only() may help a bit, but I believe we still need an option to override the behavior, from the same reason as why i915.modeset option behaves so. In general, nomodeset is for a debugging purpose, and without an option, you'll have no way to re-enable the HD-audio even if you could reload the graphics driver. thanks, Takashi > Signed-off-by: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > --- > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c > index 1637dc6e630a6..90bcf84f7b2ce 100644 > --- a/sound/hda/hdac_i915.c > +++ b/sound/hda/hdac_i915.c > @@ -11,6 +11,8 @@ > #include <sound/hda_i915.h> > #include <sound/hda_register.h> > > +#include <video/nomodeset.h> > + > #define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \ > ((pci)->device == 0x0c0c) || \ > ((pci)->device == 0x0d0c) || \ > @@ -122,6 +124,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci) > { > struct pci_dev *display_dev = NULL; > > + if (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 && >

1 0

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

19 Jul '23

On Wed, 19 Jul 2023 11:48:06 +0200, Maarten Lankhorst wrote: > > The 60 seconds timeout is a thing "better than complete disablement", > so it's not ideal, either. Maybe we can add something like the > following: > > - Check when the deferred probe takes too long, and warn it > - Provide some runtime option to disable the component binding, so > that user can work around it if needed > > A module option to snd_hdac_i915_init would probably be the least of all evils > here. Yes, probably it's the easiest option and sufficient. thanks, Takashi

1 0