[PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

On some Intel platforms, the HDMI codec is connected to PCH HDA
controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
the i915 power well failure should not block the hda controller
initialization.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 sound/hda/hdac_i915.c     |  6 +++---
 sound/pci/hda/hda_intel.c | 11 +++++++++--
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index cb78c25..4f4036a 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);
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 7933fb0..775e020 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1986,14 +1986,21 @@ static int azx_probe_continue(struct azx *chip)
 			hda->need_i915_power = 1;
 
 		err = snd_hdac_i915_init(bus);
+		/* if controller is located in powerwell,
+		 * the initialization should be stopped.
+		 * if only codec is located in powerwell,
+		 * the hd audio is still ok for analog audio
+		 */
 		if (err < 0)
-			goto i915_power_fail;
+			if (hda->need_i915_power)
+				goto i915_power_fail;
 
 		err = snd_hdac_display_power(bus, true);
 		if (err < 0) {
 			dev_err(chip->card->dev,
 				"Cannot turn on display power on i915\n");
-			goto i915_power_fail;
+			if (hda->need_i915_power)
+				goto i915_power_fail;
 		}
 	}
 
-- 
1.9.1

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[Takashi Iwai]

At Tue,  9 Jun 2015 14:05:18 +0800,
libin.yang@intel.com wrote:
> 
> From: Libin Yang <libin.yang@intel.com>
> 
> On some Intel platforms, the HDMI codec is connected to PCH HDA
> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
> the i915 power well failure should not block the hda controller
> initialization.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>

Is this fix needed for 4.1, or it's only for 4.2?
I vaguely remember of a bug report.  If there is any relevant bug
report, please give the link, too.


Takashi

> ---
>  sound/hda/hdac_i915.c     |  6 +++---
>  sound/pci/hda/hda_intel.c | 11 +++++++++--
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index cb78c25..4f4036a 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);
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 7933fb0..775e020 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1986,14 +1986,21 @@ static int azx_probe_continue(struct azx *chip)
>  			hda->need_i915_power = 1;
>  
>  		err = snd_hdac_i915_init(bus);
> +		/* if controller is located in powerwell,
> +		 * the initialization should be stopped.
> +		 * if only codec is located in powerwell,
> +		 * the hd audio is still ok for analog audio
> +		 */
>  		if (err < 0)
> -			goto i915_power_fail;
> +			if (hda->need_i915_power)
> +				goto i915_power_fail;
>  
>  		err = snd_hdac_display_power(bus, true);
>  		if (err < 0) {
>  			dev_err(chip->card->dev,
>  				"Cannot turn on display power on i915\n");
> -			goto i915_power_fail;
> +			if (hda->need_i915_power)
> +				goto i915_power_fail;
>  		}
>  	}
>  
> -- 
> 1.9.1
> 

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[David Henningsson]

On 2015-06-09 08:31, Takashi Iwai wrote:
> At Tue,  9 Jun 2015 14:05:18 +0800,
> libin.yang@intel.com wrote:
>>
>> From: Libin Yang <libin.yang@intel.com>
>>
>> On some Intel platforms, the HDMI codec is connected to PCH HDA
>> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
>> the i915 power well failure should not block the hda controller
>> initialization.
>>
>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>
> Is this fix needed for 4.1, or it's only for 4.2?

It's a bug fix, and as such should go to all kernels, even with cc to 
stable. That said, it mainly concerns Skylake, so kernels that don't 
support Skylake would not need a backport.

> I vaguely remember of a bug report.  If there is any relevant bug
> report, please give the link, too.

I think this was raised to Intel by us. The use case is when the 
integrated GPU is disabled and a discrete GPU is used. In this case the 
i915 module fails to load. If then the HDMI and analog codec are both on 
the same controller, the entire controller fails instead of just the 
HDMI codec.

I'll see if I can get the patch tested ASAP.

>
>
> Takashi
>
>> ---
>>   sound/hda/hdac_i915.c     |  6 +++---
>>   sound/pci/hda/hda_intel.c | 11 +++++++++--
>>   2 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
>> index cb78c25..4f4036a 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);
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index 7933fb0..775e020 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -1986,14 +1986,21 @@ static int azx_probe_continue(struct azx *chip)
>>   			hda->need_i915_power = 1;
>>
>>   		err = snd_hdac_i915_init(bus);
>> +		/* if controller is located in powerwell,
>> +		 * the initialization should be stopped.
>> +		 * if only codec is located in powerwell,
>> +		 * the hd audio is still ok for analog audio
>> +		 */
>>   		if (err < 0)
>> -			goto i915_power_fail;
>> +			if (hda->need_i915_power)
>> +				goto i915_power_fail;
>>
>>   		err = snd_hdac_display_power(bus, true);
>>   		if (err < 0) {
>>   			dev_err(chip->card->dev,
>>   				"Cannot turn on display power on i915\n");
>> -			goto i915_power_fail;
>> +			if (hda->need_i915_power)
>> +				goto i915_power_fail;
>>   		}
>>   	}
>>
>> --
>> 1.9.1
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

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

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[Takashi Iwai]

At Tue, 09 Jun 2015 08:46:35 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-06-09 08:31, Takashi Iwai wrote:
> > At Tue,  9 Jun 2015 14:05:18 +0800,
> > libin.yang@intel.com wrote:
> >>
> >> From: Libin Yang <libin.yang@intel.com>
> >>
> >> On some Intel platforms, the HDMI codec is connected to PCH HDA
> >> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
> >> the i915 power well failure should not block the hda controller
> >> initialization.
> >>
> >> Signed-off-by: Libin Yang <libin.yang@intel.com>
> >
> > Is this fix needed for 4.1, or it's only for 4.2?
> 
> It's a bug fix, and as such should go to all kernels, even with cc to 
> stable. That said, it mainly concerns Skylake, so kernels that don't 
> support Skylake would not need a backport.

The patch can't be applied even to 4.1 as is because of the code
structure change.  So, Cc to stable doesn't work, in anyway.

> > I vaguely remember of a bug report.  If there is any relevant bug
> > report, please give the link, too.
> 
> I think this was raised to Intel by us. The use case is when the 
> integrated GPU is disabled and a discrete GPU is used. In this case the 
> i915 module fails to load. If then the HDMI and analog codec are both on 
> the same controller, the entire controller fails instead of just the 
> HDMI codec.
> 
> I'll see if I can get the patch tested ASAP.

OK, the bug seems needed for 4.1 and earlier.  But Libin's patch is
only for 4.2.  And even worse, backporting this isn't
straightforward due to the lack of need_i915_power field.  Hmm.

I think we can make it easier by just allowing to continue the probe.
A totally untested patch for 4.1 is below.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index fea198c58196..3903a62ce1e8 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,12 @@ 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 out_free;
+		if (err < 0) {
+			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,
@@ -1865,6 +1874,7 @@ static int azx_probe_continue(struct azx *chip)
 #endif
 	}
 
+ skip_i915:
 	err = azx_first_init(chip);
 	if (err < 0)
 		goto out_free;

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, June 09, 2015 4:30 PM
> To: David Henningsson
> Cc: Yang, Libin; alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - PCH HDA controller not
> controlled by i915 power
> 
> At Tue, 09 Jun 2015 08:46:35 +0200,
> David Henningsson wrote:
> >
> >
> >
> > On 2015-06-09 08:31, Takashi Iwai wrote:
> > > At Tue,  9 Jun 2015 14:05:18 +0800,
> > > libin.yang@intel.com wrote:
> > >>
> > >> From: Libin Yang <libin.yang@intel.com>
> > >>
> > >> On some Intel platforms, the HDMI codec is connected to PCH
> HDA
> > >> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
> > >> the i915 power well failure should not block the hda controller
> > >> initialization.
> > >>
> > >> Signed-off-by: Libin Yang <libin.yang@intel.com>
> > >
> > > Is this fix needed for 4.1, or it's only for 4.2?
> >
> > It's a bug fix, and as such should go to all kernels, even with cc to
> > stable. That said, it mainly concerns Skylake, so kernels that don't
> > support Skylake would not need a backport.
> 
> The patch can't be applied even to 4.1 as is because of the code
> structure change.  So, Cc to stable doesn't work, in anyway.
> 
> > > I vaguely remember of a bug report.  If there is any relevant bug
> > > report, please give the link, too.
> >
> > I think this was raised to Intel by us. The use case is when the
> > integrated GPU is disabled and a discrete GPU is used. In this case
> the
> > i915 module fails to load. If then the HDMI and analog codec are both
> on
> > the same controller, the entire controller fails instead of just the
> > HDMI codec.

I can't find the bug number, maybe David can help.

BTW: Not only SKL but also other platforms that controller is not in 
Powerwell while the controller connects with HDMI codec will meet
such issue, like BSW.

> >
> > I'll see if I can get the patch tested ASAP.
> 
> OK, the bug seems needed for 4.1 and earlier.  But Libin's patch is
> only for 4.2.  And even worse, backporting this isn't
> straightforward due to the lack of need_i915_power field.  Hmm.
> 
> I think we can make it easier by just allowing to continue the probe.
> A totally untested patch for 4.1 is below.

Thanks for your help on this. So this patch will be merged or we will
wait for 4.2 to merge the patch?

> 
> 
> thanks,
> 
> Takashi
> 
> ---
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index fea198c58196..3903a62ce1e8 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,12 @@ 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 out_free;
> +		if (err < 0) {
> +			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,
> @@ -1865,6 +1874,7 @@ static int azx_probe_continue(struct azx
> *chip)
>  #endif
>  	}
> 
> + skip_i915:
>  	err = azx_first_init(chip);
>  	if (err < 0)
>  		goto out_free;

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[David Henningsson]

On 2015-06-09 10:30, Takashi Iwai wrote:
> At Tue, 09 Jun 2015 08:46:35 +0200,
> David Henningsson wrote:
>>
>>
>>
>> On 2015-06-09 08:31, Takashi Iwai wrote:
>>> At Tue,  9 Jun 2015 14:05:18 +0800,
>>> libin.yang@intel.com wrote:
>>>>
>>>> From: Libin Yang <libin.yang@intel.com>
>>>>
>>>> On some Intel platforms, the HDMI codec is connected to PCH HDA
>>>> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
>>>> the i915 power well failure should not block the hda controller
>>>> initialization.
>>>>
>>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>>>
>>> Is this fix needed for 4.1, or it's only for 4.2?
>>
>> It's a bug fix, and as such should go to all kernels, even with cc to
>> stable. That said, it mainly concerns Skylake, so kernels that don't
>> support Skylake would not need a backport.
>
> The patch can't be applied even to 4.1 as is because of the code
> structure change.  So, Cc to stable doesn't work, in anyway.
>
>>> I vaguely remember of a bug report.  If there is any relevant bug
>>> report, please give the link, too.
>>
>> I think this was raised to Intel by us. The use case is when the
>> integrated GPU is disabled and a discrete GPU is used. In this case the
>> i915 module fails to load. If then the HDMI and analog codec are both on
>> the same controller, the entire controller fails instead of just the
>> HDMI codec.
>>
>> I'll see if I can get the patch tested ASAP.
>
> OK, the bug seems needed for 4.1 and earlier.  But Libin's patch is
> only for 4.2.  And even worse, backporting this isn't
> straightforward due to the lack of need_i915_power field.  Hmm.
>
> I think we can make it easier by just allowing to continue the probe.
> A totally untested patch for 4.1 is below.

As part of the wider discussion, I think we could continue instead of 
failing also on Haswell and Broadwell. This is to some degree 
hypothetical, but if the i915 module does not load for some reason, then 
the i915 module would not turn the power well off either, so it remains 
on the entire time (and then audio could potentially work).

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

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[Takashi Iwai]

At Tue, 09 Jun 2015 10:51:19 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-06-09 10:30, Takashi Iwai wrote:
> > At Tue, 09 Jun 2015 08:46:35 +0200,
> > David Henningsson wrote:
> >>
> >>
> >>
> >> On 2015-06-09 08:31, Takashi Iwai wrote:
> >>> At Tue,  9 Jun 2015 14:05:18 +0800,
> >>> libin.yang@intel.com wrote:
> >>>>
> >>>> From: Libin Yang <libin.yang@intel.com>
> >>>>
> >>>> On some Intel platforms, the HDMI codec is connected to PCH HDA
> >>>> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
> >>>> the i915 power well failure should not block the hda controller
> >>>> initialization.
> >>>>
> >>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
> >>>
> >>> Is this fix needed for 4.1, or it's only for 4.2?
> >>
> >> It's a bug fix, and as such should go to all kernels, even with cc to
> >> stable. That said, it mainly concerns Skylake, so kernels that don't
> >> support Skylake would not need a backport.
> >
> > The patch can't be applied even to 4.1 as is because of the code
> > structure change.  So, Cc to stable doesn't work, in anyway.
> >
> >>> I vaguely remember of a bug report.  If there is any relevant bug
> >>> report, please give the link, too.
> >>
> >> I think this was raised to Intel by us. The use case is when the
> >> integrated GPU is disabled and a discrete GPU is used. In this case the
> >> i915 module fails to load. If then the HDMI and analog codec are both on
> >> the same controller, the entire controller fails instead of just the
> >> HDMI codec.
> >>
> >> I'll see if I can get the patch tested ASAP.
> >
> > OK, the bug seems needed for 4.1 and earlier.  But Libin's patch is
> > only for 4.2.  And even worse, backporting this isn't
> > straightforward due to the lack of need_i915_power field.  Hmm.
> >
> > I think we can make it easier by just allowing to continue the probe.
> > A totally untested patch for 4.1 is below.
> 
> As part of the wider discussion, I think we could continue instead of 
> failing also on Haswell and Broadwell. This is to some degree 
> hypothetical, but if the i915 module does not load for some reason, then 
> the i915 module would not turn the power well off either, so it remains 
> on the entire time (and then audio could potentially work).

Without graphics HDMI/DP never works, so practically it's useless.
The problem can be reproduced easily by passing nomodeset boot
option.

OTOH, it'd simplify the change, and this was indeed my initial patch
before posting.  Then I thought this might lead to a regression; a
non-working HDMI/DP sound card appears while it wasn't before.  So I
added the check of HSW/BDW.

Meanwhile, nomodeset or failing i915 is a more or less special
situation, so we might not consider about this behavior change so
much, and take rather for the code simplicity.

Comments?


Takashi

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[Takashi Iwai]

At Tue, 9 Jun 2015 08:43:44 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, June 09, 2015 4:30 PM
> > To: David Henningsson
> > Cc: Yang, Libin; alsa-devel@alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH] ALSA: hda - PCH HDA controller not
> > controlled by i915 power
> > 
> > At Tue, 09 Jun 2015 08:46:35 +0200,
> > David Henningsson wrote:
> > >
> > >
> > >
> > > On 2015-06-09 08:31, Takashi Iwai wrote:
> > > > At Tue,  9 Jun 2015 14:05:18 +0800,
> > > > libin.yang@intel.com wrote:
> > > >>
> > > >> From: Libin Yang <libin.yang@intel.com>
> > > >>
> > > >> On some Intel platforms, the HDMI codec is connected to PCH
> > HDA
> > > >> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
> > > >> the i915 power well failure should not block the hda controller
> > > >> initialization.
> > > >>
> > > >> Signed-off-by: Libin Yang <libin.yang@intel.com>
> > > >
> > > > Is this fix needed for 4.1, or it's only for 4.2?
> > >
> > > It's a bug fix, and as such should go to all kernels, even with cc to
> > > stable. That said, it mainly concerns Skylake, so kernels that don't
> > > support Skylake would not need a backport.
> > 
> > The patch can't be applied even to 4.1 as is because of the code
> > structure change.  So, Cc to stable doesn't work, in anyway.
> > 
> > > > I vaguely remember of a bug report.  If there is any relevant bug
> > > > report, please give the link, too.
> > >
> > > I think this was raised to Intel by us. The use case is when the
> > > integrated GPU is disabled and a discrete GPU is used. In this case
> > the
> > > i915 module fails to load. If then the HDMI and analog codec are both
> > on
> > > the same controller, the entire controller fails instead of just the
> > > HDMI codec.
> 
> I can't find the bug number, maybe David can help.
> 
> BTW: Not only SKL but also other platforms that controller is not in 
> Powerwell while the controller connects with HDMI codec will meet
> such issue, like BSW.
> 
> > >
> > > I'll see if I can get the patch tested ASAP.
> > 
> > OK, the bug seems needed for 4.1 and earlier.  But Libin's patch is
> > only for 4.2.  And even worse, backporting this isn't
> > straightforward due to the lack of need_i915_power field.  Hmm.
> > 
> > I think we can make it easier by just allowing to continue the probe.
> > A totally untested patch for 4.1 is below.
> 
> Thanks for your help on this. So this patch will be merged or we will
> wait for 4.2 to merge the patch?

Let's see whether it works as expected.  
If this works for 4.1, we can merge back to 4.2 together with
additional NULL checks in hdac_i915.c.


Takashi

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[Yang, Libin]

+ Mengdong

Hi Takashi & David,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, June 09, 2015 5:06 PM
> To: David Henningsson
> Cc: Yang, Libin; alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - PCH HDA controller not
> controlled by i915 power
> 
> At Tue, 09 Jun 2015 10:51:19 +0200,
> David Henningsson wrote:
> >
> >
> >
> > On 2015-06-09 10:30, Takashi Iwai wrote:
> > > At Tue, 09 Jun 2015 08:46:35 +0200,
> > > David Henningsson wrote:
> > >>
> > >>
> > >>
> > >> On 2015-06-09 08:31, Takashi Iwai wrote:
> > >>> At Tue,  9 Jun 2015 14:05:18 +0800,
> > >>> libin.yang@intel.com wrote:
> > >>>>
> > >>>> From: Libin Yang <libin.yang@intel.com>
> > >>>>
> > >>>> On some Intel platforms, the HDMI codec is connected to PCH
> HDA
> > >>>> controller. In this case, AZX_DCAPS_I915_POWERWELL is set
> and
> > >>>> the i915 power well failure should not block the hda controller
> > >>>> initialization.
> > >>>>
> > >>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
> > >>>
> > >>> Is this fix needed for 4.1, or it's only for 4.2?
> > >>
> > >> It's a bug fix, and as such should go to all kernels, even with cc to
> > >> stable. That said, it mainly concerns Skylake, so kernels that don't
> > >> support Skylake would not need a backport.
> > >
> > > The patch can't be applied even to 4.1 as is because of the code
> > > structure change.  So, Cc to stable doesn't work, in anyway.
> > >
> > >>> I vaguely remember of a bug report.  If there is any relevant bug
> > >>> report, please give the link, too.
> > >>
> > >> I think this was raised to Intel by us. The use case is when the
> > >> integrated GPU is disabled and a discrete GPU is used. In this case
> the
> > >> i915 module fails to load. If then the HDMI and analog codec are
> both on
> > >> the same controller, the entire controller fails instead of just the
> > >> HDMI codec.
> > >>
> > >> I'll see if I can get the patch tested ASAP.
> > >
> > > OK, the bug seems needed for 4.1 and earlier.  But Libin's patch is
> > > only for 4.2.  And even worse, backporting this isn't
> > > straightforward due to the lack of need_i915_power field.  Hmm.
> > >
> > > I think we can make it easier by just allowing to continue the probe.
> > > A totally untested patch for 4.1 is below.
> >
> > As part of the wider discussion, I think we could continue instead of
> > failing also on Haswell and Broadwell. This is to some degree
> > hypothetical, but if the i915 module does not load for some reason,
> then
> > the i915 module would not turn the power well off either, so it
> remains
> > on the entire time (and then audio could potentially work).
> 
> Without graphics HDMI/DP never works, so practically it's useless.
> The problem can be reproduced easily by passing nomodeset boot
> option.
> 
> OTOH, it'd simplify the change, and this was indeed my initial patch
> before posting.  Then I thought this might lead to a regression; a
> non-working HDMI/DP sound card appears while it wasn't before.  So I
> added the check of HSW/BDW.
> 
> Meanwhile, nomodeset or failing i915 is a more or less special
> situation, so we might not consider about this behavior change so
> much, and take rather for the code simplicity.
> 
> Comments?

Yes, HDMI/DP depends on gfx.

BDW/HSW has 2 controllers: one is in power well, the other is not.
The one in powerwell connects HDMI/DP codec.
The one in PCH connects analog codec.

SKL/BSW/BYT has one controller and it connects both HDMI/DP codec
and analog codec.

So we get the conclusion:
1. whatever power well is turned on, BDW/HSW PCH HDA controller
will work.
2. If power well is turned off, BDW/HSW HDA controller in gfx will not
work.
3. if power well is turned off, SKL/BSW/BYT controller will work (analog
audio will use it), but HDMI/DP audio will not work.



> 
> 
> Takashi

Re: [PATCH] ALSA: hda - PCH HDA controller not controlled by i915 power

[Takashi Iwai]

At Tue, 09 Jun 2015 11:06:09 +0200,
Takashi Iwai wrote:
> 
> At Tue, 09 Jun 2015 10:51:19 +0200,
> David Henningsson wrote:
> > 
> > 
> > 
> > On 2015-06-09 10:30, Takashi Iwai wrote:
> > > At Tue, 09 Jun 2015 08:46:35 +0200,
> > > David Henningsson wrote:
> > >>
> > >>
> > >>
> > >> On 2015-06-09 08:31, Takashi Iwai wrote:
> > >>> At Tue,  9 Jun 2015 14:05:18 +0800,
> > >>> libin.yang@intel.com wrote:
> > >>>>
> > >>>> From: Libin Yang <libin.yang@intel.com>
> > >>>>
> > >>>> On some Intel platforms, the HDMI codec is connected to PCH HDA
> > >>>> controller. In this case, AZX_DCAPS_I915_POWERWELL is set and
> > >>>> the i915 power well failure should not block the hda controller
> > >>>> initialization.
> > >>>>
> > >>>> Signed-off-by: Libin Yang <libin.yang@intel.com>
> > >>>
> > >>> Is this fix needed for 4.1, or it's only for 4.2?
> > >>
> > >> It's a bug fix, and as such should go to all kernels, even with cc to
> > >> stable. That said, it mainly concerns Skylake, so kernels that don't
> > >> support Skylake would not need a backport.
> > >
> > > The patch can't be applied even to 4.1 as is because of the code
> > > structure change.  So, Cc to stable doesn't work, in anyway.
> > >
> > >>> I vaguely remember of a bug report.  If there is any relevant bug
> > >>> report, please give the link, too.
> > >>
> > >> I think this was raised to Intel by us. The use case is when the
> > >> integrated GPU is disabled and a discrete GPU is used. In this case the
> > >> i915 module fails to load. If then the HDMI and analog codec are both on
> > >> the same controller, the entire controller fails instead of just the
> > >> HDMI codec.
> > >>
> > >> I'll see if I can get the patch tested ASAP.
> > >
> > > OK, the bug seems needed for 4.1 and earlier.  But Libin's patch is
> > > only for 4.2.  And even worse, backporting this isn't
> > > straightforward due to the lack of need_i915_power field.  Hmm.
> > >
> > > I think we can make it easier by just allowing to continue the probe.
> > > A totally untested patch for 4.1 is below.
> > 
> > As part of the wider discussion, I think we could continue instead of 
> > failing also on Haswell and Broadwell. This is to some degree 
> > hypothetical, but if the i915 module does not load for some reason, then 
> > the i915 module would not turn the power well off either, so it remains 
> > on the entire time (and then audio could potentially work).
> 
> Without graphics HDMI/DP never works, so practically it's useless.
> The problem can be reproduced easily by passing nomodeset boot
> option.
> 
> OTOH, it'd simplify the change, and this was indeed my initial patch
> before posting.  Then I thought this might lead to a regression; a
> non-working HDMI/DP sound card appears while it wasn't before.  So I
> added the check of HSW/BDW.
> 
> Meanwhile, nomodeset or failing i915 is a more or less special
> situation, so we might not consider about this behavior change so
> much, and take rather for the code simplicity.
> 
> Comments?

After sleeping, I changed my mind: now I vote for simplicity.

I'm going to post a patch series, one for 4.1 and another for 4.2 in
addition.


Takashi