diff mbox

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

Message ID 1433931965-12337-1-git-send-email-tiwai@suse.de (mailing list archive)
State Accepted
Delegated to: Takashi Iwai
Headers show

Commit Message

Takashi Iwai June 10, 2015, 10:26 a.m. UTC 
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(-)

Comments

Lin, Mengdong June 12, 2015, 2:08 a.m. UTC | #1 
> -----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. 

- 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.

Thanks
Mengdong
Takashi Iwai June 12, 2015, 5:08 a.m. UTC | #2 
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
Lin, Mengdong June 12, 2015, 5:50 a.m. UTC | #3 
> -----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.

Thanks
Mengdong
diff mbox

Patch

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;