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

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

[Takashi Iwai]

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

[PATCH 2/2] ALSA: hda - Allow calling snd_hdac_i915_*() without actual binding

[Takashi Iwai]

Add the missing NULL checks so that snd_hdac_i915*() can be called
even after the binding with i915 failed.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
This is only for 4.2 (for-next branch).

 sound/hda/hdac_i915.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index cb78c25585ac..442500e06b7c 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -27,7 +27,7 @@ int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
 {
 	struct i915_audio_component *acomp = bus->audio_component;
 
-	if (!acomp->ops)
+	if (!acomp || !acomp->ops)
 		return -ENODEV;
 
 	if (!acomp->ops->codec_wake_override) {
@@ -49,7 +49,7 @@ int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
 {
 	struct i915_audio_component *acomp = bus->audio_component;
 
-	if (!acomp->ops)
+	if (!acomp || !acomp->ops)
 		return -ENODEV;
 
 	dev_dbg(bus->dev, "display power %s\n",
@@ -72,7 +72,7 @@ int snd_hdac_get_display_clk(struct hdac_bus *bus)
 {
 	struct i915_audio_component *acomp = bus->audio_component;
 
-	if (!acomp->ops)
+	if (!acomp || !acomp->ops)
 		return -ENODEV;
 
 	return acomp->ops->get_cdclk_freq(acomp->dev);
@@ -179,8 +179,11 @@ int snd_hdac_i915_exit(struct hdac_bus *bus)
 	struct device *dev = bus->dev;
 	struct i915_audio_component *acomp = bus->audio_component;
 
+	if (!acomp)
+		return 0;
+
 	WARN_ON(bus->i915_power_refcount);
-	if (bus->i915_power_refcount > 0 && acomp && acomp->ops)
+	if (bus->i915_power_refcount > 0 && acomp->ops)
 		acomp->ops->put_power(acomp->dev);
 
 	component_master_del(dev, &hdac_component_master_ops);
-- 
2.4.3

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

[Lin, Mengdong]

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

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

[Takashi Iwai]

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

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

[Lin, Mengdong]

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

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

[Takashi Iwai]

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.

Fair enough.  Then I'm going to queue the patch below in addition to
for-linus branch.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: hda - Abort the probe without i915 binding for HSW/BDW

The previous patch tried to continue the probe if i915 binding fails.
For for simplicity reason, we haven't implemented abort even for
controller chips that are dedicated for HDMI/DP on HSW and BDW.
However, Mengdong suggested that this can be dangerous; BIOS may
disable gfx power well although the PCI entry for HD-audio is left,
and this may result in the unexpected behavior, kernel errors, etc.

For avoiding this situation, abort the probe at i915 binding failure
only for HSW/BDW chips selectively.  For other chips, it still
continues.

Fixes: bf06848bdbe5 ('ALSA: hda - Continue probing even if i915 binding fails')
Reported-by: Mengdong Lin <mengdong.lin@intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 8a0af6770e1d..a244ba706317 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -340,6 +340,11 @@ enum {
 #define use_vga_switcheroo(chip)	0
 #endif
 
+#define CONTROLLER_IN_GPU(pci) (((pci)->device == 0x0a0c) || \
+					((pci)->device == 0x0c0c) || \
+					((pci)->device == 0x0d0c) || \
+					((pci)->device == 0x160c))
+
 static char *driver_short_names[] = {
 	[AZX_DRIVER_ICH] = "HDA Intel",
 	[AZX_DRIVER_PCH] = "HDA Intel PCH",
@@ -1854,8 +1859,17 @@ static int azx_probe_continue(struct azx *chip)
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
 #ifdef CONFIG_SND_HDA_I915
 		err = hda_i915_init(hda);
-		if (err < 0)
-			goto skip_i915;
+		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))
+				goto out_free;
+			else
+				goto skip_i915;
+		}
 		err = hda_display_power(hda, true);
 		if (err < 0) {
 			dev_err(chip->card->dev,
-- 
2.4.3

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

[David Henningsson]

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.

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

>
>
> thanks,
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: hda - Abort the probe without i915 binding for HSW/BDW
>
> The previous patch tried to continue the probe if i915 binding fails.
> For for simplicity reason, we haven't implemented abort even for
> controller chips that are dedicated for HDMI/DP on HSW and BDW.
> However, Mengdong suggested that this can be dangerous; BIOS may
> disable gfx power well although the PCI entry for HD-audio is left,
> and this may result in the unexpected behavior, kernel errors, etc.
>
> For avoiding this situation, abort the probe at i915 binding failure
> only for HSW/BDW chips selectively.  For other chips, it still
> continues.
>
> Fixes: bf06848bdbe5 ('ALSA: hda - Continue probing even if i915 binding fails')
> Reported-by: Mengdong Lin <mengdong.lin@intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   sound/pci/hda/hda_intel.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 8a0af6770e1d..a244ba706317 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -340,6 +340,11 @@ enum {
>   #define use_vga_switcheroo(chip)	0
>   #endif
>   
> +#define CONTROLLER_IN_GPU(pci) (((pci)->device == 0x0a0c) || \
> +					((pci)->device == 0x0c0c) || \
> +					((pci)->device == 0x0d0c) || \
> +					((pci)->device == 0x160c))
> +
>   static char *driver_short_names[] = {
>   	[AZX_DRIVER_ICH] = "HDA Intel",
>   	[AZX_DRIVER_PCH] = "HDA Intel PCH",
> @@ -1854,8 +1859,17 @@ static int azx_probe_continue(struct azx *chip)
>   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
>   #ifdef CONFIG_SND_HDA_I915
>   		err = hda_i915_init(hda);
> -		if (err < 0)
> -			goto skip_i915;
> +		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))
> +				goto out_free;
> +			else
> +				goto skip_i915;
> +		}
>   		err = hda_display_power(hda, true);
>   		if (err < 0) {
>   			dev_err(chip->card->dev,

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

[Takashi Iwai]

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.


thanks,

Takashi

> >
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: hda - Abort the probe without i915 binding for HSW/BDW
> >
> > The previous patch tried to continue the probe if i915 binding fails.
> > For for simplicity reason, we haven't implemented abort even for
> > controller chips that are dedicated for HDMI/DP on HSW and BDW.
> > However, Mengdong suggested that this can be dangerous; BIOS may
> > disable gfx power well although the PCI entry for HD-audio is left,
> > and this may result in the unexpected behavior, kernel errors, etc.
> >
> > For avoiding this situation, abort the probe at i915 binding failure
> > only for HSW/BDW chips selectively.  For other chips, it still
> > continues.
> >
> > Fixes: bf06848bdbe5 ('ALSA: hda - Continue probing even if i915 binding fails')
> > Reported-by: Mengdong Lin <mengdong.lin@intel.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   sound/pci/hda/hda_intel.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 8a0af6770e1d..a244ba706317 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -340,6 +340,11 @@ enum {
> >   #define use_vga_switcheroo(chip)	0
> >   #endif
> >   
> > +#define CONTROLLER_IN_GPU(pci) (((pci)->device == 0x0a0c) || \
> > +					((pci)->device == 0x0c0c) || \
> > +					((pci)->device == 0x0d0c) || \
> > +					((pci)->device == 0x160c))
> > +
> >   static char *driver_short_names[] = {
> >   	[AZX_DRIVER_ICH] = "HDA Intel",
> >   	[AZX_DRIVER_PCH] = "HDA Intel PCH",
> > @@ -1854,8 +1859,17 @@ static int azx_probe_continue(struct azx *chip)
> >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
> >   #ifdef CONFIG_SND_HDA_I915
> >   		err = hda_i915_init(hda);
> > -		if (err < 0)
> > -			goto skip_i915;
> > +		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))
> > +				goto out_free;
> > +			else
> > +				goto skip_i915;
> > +		}
> >   		err = hda_display_power(hda, true);
> >   		if (err < 0) {
> >   			dev_err(chip->card->dev,
> 

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

[Takashi Iwai]

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

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

[David Henningsson]

On 2015-06-12 16:14, Takashi Iwai wrote:
> At Fri, 12 Jun 2015 08:37:38 +0200,
> Takashi Iwai wrote:
>>
>> At Fri, 12 Jun 2015 08:17:12 +0200,
>> David Henningsson wrote:
>>> 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?

I guess the question is whether HDMI/DP can actually work without the 
i915 driver, e g when running with nomodeset. If there is absolutely no 
possibility whatsoever (e g maybe BIOS could set up the device so that 
it works?), then it does not matter much, I suppose.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

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

[Takashi Iwai]

At Fri, 12 Jun 2015 16:33:32 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-06-12 16:14, Takashi Iwai wrote:
> > At Fri, 12 Jun 2015 08:37:38 +0200,
> > Takashi Iwai wrote:
> >>
> >> At Fri, 12 Jun 2015 08:17:12 +0200,
> >> David Henningsson wrote:
> >>> 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?
> 
> I guess the question is whether HDMI/DP can actually work without the 
> i915 driver, e g when running with nomodeset. If there is absolutely no 
> possibility whatsoever (e g maybe BIOS could set up the device so that 
> it works?), then it does not matter much, I suppose.

It can't work because we don't get proper ELD thus cannot setup
HDMI/DP CA bits.


Takashi

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

[Lin, Mengdong]

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Friday, June 12, 2015 2:17 PM
> To: Takashi Iwai; Lin, Mengdong
> Cc: alsa-devel@alsa-project.org; Yang, Libin
> Subject: Re: [PATCH 1/2] ALSA: hda - Continue probing even if i915 binding fails
> 
> 
> 
> 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.

The BIOS behavior is not decided. If i915 driver is broken, the power well status
is not decided. Without this power well, the audio functions in this HD-A
controller is dead, all access to HW will fail including the operation to reset link.
So I feel it's better we need not continue the probing.

Thanks
Mengdong

> 
> > 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.
> 
> >
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: hda - Abort the probe without i915 binding for
> > HSW/BDW
> >
> > The previous patch tried to continue the probe if i915 binding fails.
> > For for simplicity reason, we haven't implemented abort even for
> > controller chips that are dedicated for HDMI/DP on HSW and BDW.
> > However, Mengdong suggested that this can be dangerous; BIOS may
> > disable gfx power well although the PCI entry for HD-audio is left,
> > and this may result in the unexpected behavior, kernel errors, etc.
> >
> > For avoiding this situation, abort the probe at i915 binding failure
> > only for HSW/BDW chips selectively.  For other chips, it still
> > continues.
> >
> > Fixes: bf06848bdbe5 ('ALSA: hda - Continue probing even if i915
> > binding fails')
> > Reported-by: Mengdong Lin <mengdong.lin@intel.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   sound/pci/hda/hda_intel.c | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 8a0af6770e1d..a244ba706317 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -340,6 +340,11 @@ enum {
> >   #define use_vga_switcheroo(chip)	0
> >   #endif
> >
> > +#define CONTROLLER_IN_GPU(pci) (((pci)->device == 0x0a0c) || \
> > +					((pci)->device == 0x0c0c) || \
> > +					((pci)->device == 0x0d0c) || \
> > +					((pci)->device == 0x160c))
> > +
> >   static char *driver_short_names[] = {
> >   	[AZX_DRIVER_ICH] = "HDA Intel",
> >   	[AZX_DRIVER_PCH] = "HDA Intel PCH", @@ -1854,8 +1859,17 @@
> static
> > int azx_probe_continue(struct azx *chip)
> >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
> >   #ifdef CONFIG_SND_HDA_I915
> >   		err = hda_i915_init(hda);
> > -		if (err < 0)
> > -			goto skip_i915;
> > +		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))
> > +				goto out_free;
> > +			else
> > +				goto skip_i915;
> > +		}
> >   		err = hda_display_power(hda, true);
> >   		if (err < 0) {
> >   			dev_err(chip->card->dev,