Hi, > -----Original Message----- > From: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > Sent: Tuesday, November 14, 2023 3:30 PM > To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>; Ville Syrjälä > <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Takashi Iwai <tiwai@xxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel- > xe@xxxxxxxxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Saarinen, Jani > <jani.saarinen@xxxxxxxxx>; Kurmi, Suresh Kumar > <suresh.kumar.kurmi@xxxxxxxxx> > Subject: Re: [Intel-xe] [PATCH 11/14] ALSA: hda/intel: Move snd_hdac_i915_init > to before probe_work. > > Hey, > > Den 2023-11-14 kl. 13:35, skrev Jani Nikula: > > On Tue, 14 Nov 2023, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > >> On Mon, Oct 02, 2023 at 09:38:44PM +0200, > maarten.lankhorst@xxxxxxxxxxxxxxx wrote: > >>> From: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > >>> > >>> 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. > >> This completely broke i915 audio! > >> > >> I also can't see any trace of this stuff ever being posted to > >> intel-gfx so it never went through the CI. > >> > >> Please fix or revert ASAP. > > Cc: Jani, Suresh > > > > Ville, please file a bug at gitlab so we can track this, thanks. > > I've originally tested this on TGL and DG2, so can you be more specific on what > broke? Was this series tested on CI ever as Ville saying no? How this got merged? > > Cheers, > > ~Maarten > > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > >>> Reviewed-by: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx> > >>> Reviewed-by: Pierre-Louis Bossart > >>> <pierre-louis.bossart@xxxxxxxxxxxxxxx> > >>> Signed-off-by: Maarten Lankhorst <dev@xxxxxxxxxxxx> > >>> --- > >>> sound/pci/hda/hda_intel.c | 54 +++++++++++++++++++-------------------- > >>> 1 file changed, 27 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > >>> index e42ad0e816e1..9dad3607596a 100644 > >>> --- a/sound/pci/hda/hda_intel.c > >>> +++ b/sound/pci/hda/hda_intel.c > >>> @@ -2135,6 +2135,33 @@ static int azx_probe(struct pci_dev *pci, > >>> > >>> pci_set_drvdata(pci, card); > >>> > >>> +#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 (HDA_CONTROLLER_IN_GPU(pci)) { > >>> + goto out_free; > >>> + } else { > >>> + /* don't bother any longer */ > >>> + chip->driver_caps &= > ~AZX_DCAPS_I915_COMPONENT; > >>> + } > >>> + } > >>> + > >>> + /* HSW/BDW controllers need this power */ > >>> + if (HDA_CONTROLLER_IN_GPU(pci)) > >>> + hda->need_i915_power = true; > >>> + } > >>> +#else > >>> + if (HDA_CONTROLLER_IN_GPU(pci)) > >>> + dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in > >>> +CONFIG_SND_HDA_I915\n"); #endif > >>> + > >>> err = register_vga_switcheroo(chip); > >>> if (err < 0) { > >>> dev_err(card->dev, "Error registering vga_switcheroo client\n"); > >>> @@ -2162,11 +2189,6 @@ static int azx_probe(struct pci_dev *pci, > >>> } > >>> #endif /* CONFIG_SND_HDA_PATCH_LOADER */ > >>> > >>> -#ifndef CONFIG_SND_HDA_I915 > >>> - if (HDA_CONTROLLER_IN_GPU(pci)) > >>> - dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in > CONFIG_SND_HDA_I915\n"); > >>> -#endif > >>> - > >>> if (schedule_probe) > >>> schedule_delayed_work(&hda->probe_work, 0); > >>> > >>> @@ -2264,28 +2286,6 @@ static int azx_probe_continue(struct azx *chip) > >>> to_hda_bus(bus)->bus_probing = 1; > >>> hda->probe_continued = 1; > >>> > >>> - /* bind with i915 if needed */ > >>> - if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT) { > >>> - err = snd_hdac_i915_init(bus, true); > >>> - 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 (HDA_CONTROLLER_IN_GPU(pci)) { > >>> - goto out_free; > >>> - } else { > >>> - /* don't bother any longer */ > >>> - chip->driver_caps &= > ~AZX_DCAPS_I915_COMPONENT; > >>> - } > >>> - } > >>> - > >>> - /* HSW/BDW controllers need this power */ > >>> - if (HDA_CONTROLLER_IN_GPU(pci)) > >>> - hda->need_i915_power = true; > >>> - } > >>> - > >>> /* Request display power well for the HDA controller or codec. For > >>> * Haswell/Broadwell, both the display HDA controller and codec need > >>> * this power. For other platforms, like Baytrail/Braswell, only > >>> the > >>> -- > >>> 2.40.1