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

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

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

19 Jul '23

Hi, thank you Maarten for doing the series! I think a lot of people will be happy to get rid of the 60sec timeout. I kicked off a CI run SOF public infra for the whole series at https://github.com/thesofproject/linux/pull/4478 Some results still in progress but so far so good. Some concerns inline: On Tue, 18 Jul 2023, 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. We actually have a request_module() for the regular HDA codec drivers as well (sof_probe_continue() -> snd_sof_probe() -> hda_dsp_probe() -> hda_init_caps() -> hda_codec_probe_bus(). But right, this is not necessarily a problem on its own, so it looks we indeed can drop the work queue. Nice! > 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; 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. With this patch, not having a workign display driver means that there is also no audio in the system as the SOF driver will never get probed. In current mainline, one will get the 60sec timeout warning and then audio driver will proceed to probe and you'll have audio support (minus HDMI/DP). This is mostly an issue with very new hardware (e.g. hw is still behind force_probe flag in xe/i915 driver), but we've had some odd cases with e.g. systems with both Intel IGFX and other vendors' DGPU. Audio drivers see the Intel VGA controller in system and will call snd_hdac_i915_init(), but the audio component bind will never succeed if the the Intel IGFX is not in actual use. Will need a bit of time to think about possible scenarios. Possibly this is not an issue outside early development systems. In theory if IGFX is disabled in BIOS, and not visible to OS, we are good, and if it's visible, the i915/xe driver should be loaded, so we are good again. Br, Kai

2 1

Re: [v2 PATCH 1/2] ALSA: hda/intel: Fix error handling in azx_probe()
by Takashi Iwai 18 Jul '23

18 Jul '23

On Tue, 18 Jul 2023 13:57:33 +0200, Maarten Lankhorst wrote: > > > Make sure azx is freed after azx_create() succeeded and an error was > encountered. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > --- > sound/pci/hda/hda_intel.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 5af1138e745bc..196ca76ac43ad 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -2150,7 +2150,7 @@ static int azx_probe(struct pci_dev *pci, > err = register_vga_switcheroo(chip); > if (err < 0) { > dev_err(card->dev, "Error registering vga_switcheroo client\n"); > - goto out_free; > + goto out_azx_free; > } > > if (check_hdmi_disabled(pci)) { > @@ -2169,7 +2169,7 @@ static int azx_probe(struct pci_dev *pci, > &pci->dev, GFP_KERNEL, card, > azx_firmware_cb); > if (err < 0) > - goto out_free; > + goto out_azx_free; > schedule_probe = false; /* continued in azx_firmware_cb() */ > } > #endif /* CONFIG_SND_HDA_PATCH_LOADER */ > @@ -2187,6 +2187,9 @@ static int azx_probe(struct pci_dev *pci, > complete_all(&hda->probe_wait); > return 0; > > +out_azx_free: > + azx_free(chip); This is superfluous. Once when azx_create() succeeds, azx_free() is called from snd_card_free(). That is... > + pci_set_drvdata(pci, NULL); ... only this call was missing. And this can be put under out_free label, as this is safe to call at any exit path. So, it'll be a oneliner patch. thanks, Takashi > out_free: > snd_card_free(card); > return err; > -- > 2.39.2 >

1 0

Re: [PATCH 5/7] ALSA: hda/intel: Move snd_hdac_i915_init to before probe_work.
by Takashi Iwai 18 Jul '23

18 Jul '23

On Tue, 18 Jul 2023 10:45:20 +0200, Maarten Lankhorst wrote: > > Now that we can use -EPROBE_DEFER, it's no longer required to spin off > the snd_hdac_i915_init into a workqueue. > > Use the -EPROBE_DEFER mechanism instead, which must be returned in the > probe function. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > --- > sound/pci/hda/hda_intel.c | 58 +++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 27 deletions(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 5af1138e745bc..d40345a0088d8 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -213,6 +213,7 @@ MODULE_DESCRIPTION("Intel HDA driver"); > #endif > #endif > > +static DECLARE_BITMAP(probed_devs, SNDRV_CARDS); > > /* > */ > @@ -2094,8 +2095,6 @@ static const struct hda_controller_ops pci_hda_ops = { > .position_check = azx_position_check, > }; > > -static DECLARE_BITMAP(probed_devs, SNDRV_CARDS); > - > static int azx_probe(struct pci_dev *pci, > const struct pci_device_id *pci_id) > { Any specific reason to move the definition? Otherwise let's concentrate on the needed change. > @@ -2174,7 +2173,36 @@ static int azx_probe(struct pci_dev *pci, > } > #endif /* CONFIG_SND_HDA_PATCH_LOADER */ > > -#ifndef CONFIG_SND_HDA_I915 > +#ifdef CONFIG_SND_HDA_I915 > + /* bind with i915 if needed */ > + if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { > + err = snd_hdac_i915_init(azx_bus(chip), false); > + if (err < 0) { > + /* if the controller is bound only with HDMI/DP > + * (for HSW and BDW), we need to abort the probe; > + * for other chips, still continue probing as other > + * codecs can be on the same link. > + */ > + if (CONTROLLER_IN_GPU(pci)) { > + if (err != -EPROBE_DEFER) > + dev_err(card->dev, > + "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); > + > + clear_bit(chip->dev_index, probed_devs); > + pci_set_drvdata(pci, NULL); > + snd_device_free(card, chip); > + return err; This may leak resources, I'm afraid. Here you just need to "goto out_free;" instead of manual resource releases, which eventually calls snd_card_free(), and that's all. (Though, pci_set_drvdata(pci, NULL) might be still missing; but it's not only for this change, and we'll need to address it in another patch.) thanks, Takashi

1 0

Re: [PATCH 0/7] sound: Use -EPROBE_DEFER instead of i915 module loading.
by Takashi Iwai 18 Jul '23

18 Jul '23

On Tue, 18 Jul 2023 10:45:15 +0200, 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. Thanks for the patches. I naively thought that issuing the component binding at the early stage could be problematic, but in this case, this doesn't look OK, since the all i915 stuff is bound always at the HD-audio controller level, not at the codec level like others. So, it's definitely worth to try. Takashi > Maarten Lankhorst (7): > 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 > > 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 > > sound/hda/hdac_i915.c | 15 +++------ > sound/pci/hda/hda_intel.c | 58 +++++++++++++++++++---------------- > 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, 75 insertions(+), 140 deletions(-) > > -- > 2.39.2 >

1 0

Re: [PATCH] ASoC: SOF: ipc3-dtrace: uninitialized data in dfsentry_trace_filter_write()
by Mark Brown 17 Jul '23

17 Jul '23

On Fri, 07 Jul 2023 14:25:23 +0300, Dan Carpenter wrote: > This doesn't check how many bytes the simple_write_to_buffer() writes to > the buffer. The only thing that we know is that the first byte is > initialized and the last byte of the buffer is set to NUL. However > the middle bytes could be uninitialized. > > There is no need to use simple_write_to_buffer(). This code does not > support partial writes but instead passes "pos = 0" as the starting > offset regardless of what the user passed as "*ppos". Just use the > copy_from_user() function and initialize the whole buffer. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: SOF: ipc3-dtrace: uninitialized data in dfsentry_trace_filter_write() commit: 469e2f28c2cbee2430058c1c9bb6d1675d7195fb All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark

1 0

Re: (subset) [PATCH 1/2] ASoC: SOF: amd: add revision check for sending sha dma completion command
by Mark Brown 12 Jul '23

12 Jul '23

On Fri, 30 Jun 2023 12:35:42 +0530, Mastan Katragadda wrote: > ACP driver should send SHA DMA completion command to PSP module for RN > platform only. > Add a revision check for RN platform. > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [2/2] ASoC: SOF: amd: refactor PSP smn_read commit: 2b48d170fb9965dda9d41edcb0bbfc9ee4c6584f All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark

1 0