Re: [PATCH 1/2] ALSA: hda - Continue probing even if i915 binding fails

[Takashi Iwai <tiwai@suse.de>]

At Fri, 12 Jun 2015 02:08:50 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, June 10, 2015 6:26 PM
> > 
> > Currently snd-hda-intel driver aborts the probing of Intel HD-audio controller
> > with i915 power well management when binding with i915 driver via
> > hda_i915_init() fails.  This is no big problem for Haswell and Broadwell where
> > the HD-audio controllers are dedicated to HDMI/DP, thus i915 link is
> > mandatory.  However, Skylake, Baytrail and Braswell have only one controller
> > and both HDMI/DP and analog codecs share the same bus.  Thus, even if
> > HDMI/DP isn't usable, we should keep the controller working for other codecs.
> > 
> > For fixing this, this patch simply allows continuing the probing even if
> > hda_i915_init() call fails.  This may leave stale sound components for
> > HDMI/DP devices that are unbound with graphics.  We could abort the
> > probing selectively, but from the code simplicity POV, it's better to continue in
> > all cases.
> > 
> > Reported-by: Libin Yang <libin.yang@intel.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > This is for 4.1.  Appying to 4.2 may result in trivial conflicts.
> > 
> >  sound/pci/hda/hda_intel.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index
> > fea198c58196..8a0af6770e1d 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1855,7 +1855,7 @@ static int azx_probe_continue(struct azx *chip)
> > #ifdef CONFIG_SND_HDA_I915
> >  		err = hda_i915_init(hda);
> >  		if (err < 0)
> > -			goto out_free;
> > +			goto skip_i915;
> >  		err = hda_display_power(hda, true);
> >  		if (err < 0) {
> >  			dev_err(chip->card->dev,
> > @@ -1865,6 +1865,7 @@ static int azx_probe_continue(struct azx *chip)
> > #endif
> >  	}
> > 
> > + skip_i915:
> >  	err = azx_first_init(chip);
> >  	if (err < 0)
> >  		goto out_free;
> > --
> > 2.4.3
> 
> Thanks for your patch. But maybe we should not skip i915 for the dedicated
> display HD-A controller on Haswell/Broadwell.
> 
> For HSW/BDW, their HD-A controller for display audio (PCI dev#3) is *partly*
> in GPU power domain:
> - Its PCI configuration space is out of i915 power well, but in an always-on
>  power well. So even if the BIOS disables the Intel GPU (PCI dev#2), the HD-A
>  controller is still present, and we can still access all standard registers in PCI
>  config space. 

Right, and changing this doesn't break things.

> - Its audio functions are in the i915 power well. If hda_i915_init() fails and
>  I915 power well is unavailable, read/write to the audio register of the
>  Controller will fail and we'll get kernel error messages. So I feel it's better
>  to abort if hda_i915_init() fails for this HD-A controller.

By skipping, the access to i915 power well won't happen later, since
the component ops isn't set.

I tested Haswell machines with nomodeset to simulate the situation,
and there was no any kernel errors except for the first one indicating
the failure of i915 binding.

I haven't tested Broadwell, though.


thanks,

Takashi