| Message ID | 20230719164141.228073-8-maarten.lankhorst@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sound: Use -EPROBE_DEFER instead of i915 module loading. | expand |
On 7/19/23 18:41, 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. > > Changes since v1: > - Use dev_err_probe() > - Don't move probed_devs bitmap unnecessarily. (tiwai) > - Move snd_hdac_i915_init slightly upward, to ensure > it's always initialised before vga-switcheroo is called. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > sound/pci/hda/hda_intel.c | 59 ++++++++++++++++++++------------------- > 1 file changed, 30 insertions(+), 29 deletions(-) > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c > index 11cf9907f039f..e3128d7d742e7 100644 > --- a/sound/pci/hda/hda_intel.c > +++ b/sound/pci/hda/hda_intel.c > @@ -2147,6 +2147,36 @@ 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 (CONTROLLER_IN_GPU(pci)) { > + dev_err_probe(card->dev, err, > + "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); > + > + goto out_free; > + } else { > + /* don't bother any longer */ > + chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; > + } > + } > + > + /* HSW/BDW controllers need this power */ > + if (CONTROLLER_IN_GPU(pci)) > + hda->need_i915_power = true; > + } > +#else > + if (CONTROLLER_IN_GPU(pci)) > + dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n"); > +#endif Is it intentional that the display_power() is left in the probe workqueue? this piece of code follows the stuff above in the existing code? /* 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 * display codec needs the power and it can be released after probe. */ display_power(chip, true); > + > err = register_vga_switcheroo(chip); > if (err < 0) { > dev_err(card->dev, "Error registering vga_switcheroo client\n"); > @@ -2174,11 +2204,6 @@ static int azx_probe(struct pci_dev *pci, > } > #endif /* CONFIG_SND_HDA_PATCH_LOADER */ > > -#ifndef CONFIG_SND_HDA_I915 > - if (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); > > @@ -2275,30 +2300,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 (CONTROLLER_IN_GPU(pci)) { > - dev_err(chip->card->dev, > - "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); > - goto out_free; > - } else { > - /* don't bother any longer */ > - chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; > - } > - } > - > - /* HSW/BDW controllers need this power */ > - if (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
Hey, On 2023-07-24 12:58, Pierre-Louis Bossart wrote: > > > On 7/19/23 18:41, 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. >> >> Changes since v1: >> - Use dev_err_probe() >> - Don't move probed_devs bitmap unnecessarily. (tiwai) >> - Move snd_hdac_i915_init slightly upward, to ensure >> it's always initialised before vga-switcheroo is called. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> sound/pci/hda/hda_intel.c | 59 ++++++++++++++++++++------------------- >> 1 file changed, 30 insertions(+), 29 deletions(-) >> >> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c >> index 11cf9907f039f..e3128d7d742e7 100644 >> --- a/sound/pci/hda/hda_intel.c >> +++ b/sound/pci/hda/hda_intel.c >> @@ -2147,6 +2147,36 @@ 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 (CONTROLLER_IN_GPU(pci)) { >> + dev_err_probe(card->dev, err, >> + "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); >> + >> + goto out_free; >> + } else { >> + /* don't bother any longer */ >> + chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; >> + } >> + } >> + >> + /* HSW/BDW controllers need this power */ >> + if (CONTROLLER_IN_GPU(pci)) >> + hda->need_i915_power = true; >> + } >> +#else >> + if (CONTROLLER_IN_GPU(pci)) >> + dev_err(card->dev, "Haswell/Broadwell HDMI/DP must build in CONFIG_SND_HDA_I915\n"); >> +#endif > > Is it intentional that the display_power() is left in the probe workqueue? > > this piece of code follows the stuff above in the existing code? > > /* 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 > * display codec needs the power and it can be released after probe. > */ > display_power(chip, true); I think for the binding itself, there isn't any harm. We are not poking any hardware when binding, only software. This appears to be true on the i915 side as well. Cheers, ~Maarten
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 11cf9907f039f..e3128d7d742e7 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2147,6 +2147,36 @@ 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 (CONTROLLER_IN_GPU(pci)) { + dev_err_probe(card->dev, err, + "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); + + goto out_free; + } else { + /* don't bother any longer */ + chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; + } + } + + /* HSW/BDW controllers need this power */ + if (CONTROLLER_IN_GPU(pci)) + hda->need_i915_power = true; + } +#else + if (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"); @@ -2174,11 +2204,6 @@ static int azx_probe(struct pci_dev *pci, } #endif /* CONFIG_SND_HDA_PATCH_LOADER */ -#ifndef CONFIG_SND_HDA_I915 - if (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); @@ -2275,30 +2300,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 (CONTROLLER_IN_GPU(pci)) { - dev_err(chip->card->dev, - "HSW/BDW HD-audio HDMI/DP requires binding with gfx driver\n"); - goto out_free; - } else { - /* don't bother any longer */ - chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT; - } - } - - /* HSW/BDW controllers need this power */ - if (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
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. Changes since v1: - Use dev_err_probe() - Don't move probed_devs bitmap unnecessarily. (tiwai) - Move snd_hdac_i915_init slightly upward, to ensure it's always initialised before vga-switcheroo is called. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- sound/pci/hda/hda_intel.c | 59 ++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 29 deletions(-)