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

[Takashi Iwai <tiwai@suse.de>]

At Fri, 12 Jun 2015 08:37:38 +0200,
Takashi Iwai wrote:
> 
> At Fri, 12 Jun 2015 08:17:12 +0200,
> David Henningsson wrote:
> > 
> > 
> > 
> > On 2015-06-12 08:06, Takashi Iwai wrote:
> > > At Fri, 12 Jun 2015 05:50:43 +0000,
> > > Lin, Mengdong wrote:
> > >>
> > >>
> > >>> -----Original Message-----
> > >>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > >>> Sent: Friday, June 12, 2015 1:09 PM
> > >>> To: Lin, Mengdong
> > >>> Cc: alsa-devel@alsa-project.org; David Henningsson; Yang, Libin
> > >>> Subject: Re: [PATCH 1/2] ALSA: hda - Continue probing even if i915 binding fails
> > >>>
> > >>> 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.
> > >> Hi Takashi,
> > >>
> > >> I think there is no error because the i915 power well is ON by default on
> > >> Haswell and BIOS does not touch this. However, it's possible that the BIOS may
> > >> disable this power well and depends on i915 driver to turn it on when need, or
> > >> just disable the power well when disabling the Intel GPU.
> > >>
> > >> I remember previously we met audio register I/O error when HSW introduced
> > >> this power well and i915 disabled it without sync with audio driver.
> > >>
> > >> Since we cannot expect the exact BIOS behavior, I feel it's safer not to use this
> > >> HD-A controller without i915 on HSW and BDW.
> > 
> > If BIOS disables the power well, then we would get the "audio register 
> > I/O error" in the boot phase, there will be no codecs detected, and thus 
> > no broken sound card showing up.
> > 
> > The only issue is if the i915 driver disables the power well, but the 
> > case that the i915 should be functioning enough to turn off the power 
> > well at some point, and at the same time so broken that we can't bind to 
> > it, seems extremely unlikely to me.
> > 
> > Either the i915 driver works, or it does not. In both cases, continuing 
> > probing is the more useful option.
> 
> I don't see much usefulness there but rather simplicity.
> 
> The binding with i915 won't happen at the later stage, thus the device
> is just a placeholder.  Any other goodies by keeping the dead device?
> 
> > > Fair enough.  Then I'm going to queue the patch below in addition to
> > > for-linus branch.
> > 
> > We'll have to agree to disagree on that, then.
> 
> One point that hasn't been mentioned is the behavior change from the
> previous kernels.  The previous kernels abort probing, and it was OK
> since we supposed only HSW/BDW before 4.1.  Now, with continuing the
> probe, user sees the new device that hasn't been there, and yet the
> device is dead and merely a placeholder.

Is this convincing enough for you?  Or would you still like to make
the broken HDMI/DP device enabled for HSW/BDW?


Takashi