Skip to contentSkip to conversation

ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

PATCH In review [email protected] 3 participants 20 messages Mar 27, 2015 – Apr 7, 2015 Sound

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>
---
 sound/pci/hda/hda_intel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
sound/pci/hda/hda_intel.c +2 -1
index 15a8299..4744a96 100644
@@ -298,7 +298,8 @@ enum {
 	 AZX_DCAPS_SNOOP_TYPE(SCH))
 
 #define AZX_DCAPS_INTEL_SKYLAKE \
-	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_SEPARATE_STREAM_TAG)
+	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_SEPARATE_STREAM_TAG |\
+	 AZX_DCAPS_I915_POWERWELL)
 
 /* quirks for ATI SB / AMD Hudson */
 #define AZX_DCAPS_PRESET_ATI_SB \
Signature · 1 line
1.9.1
#1 lore raw

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 8 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

This certainly sucks. It means that the powerwell is on even you don't use the HDMI/DP at all. If this is intended as a temporarily workaround, it should be mentioned so. Please give more comments and backgrounds.

thanks,

Takashi

Quoted 21 lines
---
 sound/pci/hda/hda_intel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
sound/pci/hda/hda_intel.c +2 -1
index 15a8299..4744a96 100644
@@ -298,7 +298,8 @@ enum {
 	 AZX_DCAPS_SNOOP_TYPE(SCH))
 
 #define AZX_DCAPS_INTEL_SKYLAKE \
-	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_SEPARATE_STREAM_TAG)
+	(AZX_DCAPS_INTEL_PCH | AZX_DCAPS_SEPARATE_STREAM_TAG |\
+	 AZX_DCAPS_I915_POWERWELL)
 
 /* quirks for ATI SB / AMD Hudson */
 #define AZX_DCAPS_PRESET_ATI_SB \
Signature · 1 line
1.9.1
author
#2 lore raw

Hi Takashi,

Quoted 13 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 3 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

Quoted 7 lines
Quoted 5 lines

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 6 lines

This certainly sucks. It means that the powerwell is on even you don't use the HDMI/DP at all. If this is intended as a temporarily workaround, it should be mentioned so. Please give more comments and
backgrounds.

Yes, as this is added in the skl audio controller, even there is no HDMI/DP codec, we should also add this flag. Otherwise the HDMI/DP codec may not be detected correctly.

Quoted 5 lines

thanks,

Takashi

Regards,
Libin

Quoted 24 lines
Quoted 10 lines
---
 sound/pci/hda/hda_intel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
sound/pci/hda/hda_intel.c +0 -0
index 15a8299..4744a96 100644

sound/pci/hda/hda_intel.c enum { · line 301

Quoted 4 lines
sound/pci/hda/hda_intel.c +0 -1
@@ -298,7 +298,8 @@ enum {
 	 AZX_DCAPS_SNOOP_TYPE(SCH))
 
 #define AZX_DCAPS_INTEL_SKYLAKE \
-	(AZX_DCAPS_INTEL_PCH |

AZX_DCAPS_SEPARATE_STREAM_TAG)

sound/pci/hda/hda_intel.c enum { · line 301

Quoted 1 line
sound/pci/hda/hda_intel.c +1 -0
+	(AZX_DCAPS_INTEL_PCH |

AZX_DCAPS_SEPARATE_STREAM_TAG |\

Quoted 7 lines
sound/pci/hda/hda_intel.c +1 -1
+	 AZX_DCAPS_I915_POWERWELL)
 
 /* quirks for ATI SB / AMD Hudson */
 #define AZX_DCAPS_PRESET_ATI_SB \
--

1.9.1

#3 lore raw

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 35 lines · Libin

Hi Takashi,

Quoted 13 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 3 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

Quoted 7 lines
Quoted 5 lines

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 6 lines

This certainly sucks. It means that the powerwell is on even you don't use the HDMI/DP at all. If this is intended as a temporarily workaround, it should be mentioned so. Please give more comments and
backgrounds.

Quoted 3 lines

Yes, as this is added in the skl audio controller, even there is no HDMI/DP codec, we should also add this flag. Otherwise the HDMI/DP codec may not be detected correctly.

But it's possible to do it only at probing, not permanently. If so, we'll have another patch in future.

Please write more information in the changelog and resubmit.

Takashi

author
#4 lore raw

Hi Takashi,

Quoted 48 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 22 lines · Libin

Hi Takashi,

Quoted 17 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 7 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Quoted 10 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 4 lines

This certainly sucks. It means that the powerwell is on even you don't use the HDMI/DP at all. If this is intended as a temporarily workaround, it should be mentioned so. Please give more

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even there is no

Quoted 8 lines

HDMI/DP

Quoted 2 lines

codec, we should also add this flag. Otherwise the HDMI/DP codec may not be detected correctly.

But it's possible to do it only at probing, not permanently. If so, we'll have another patch in future.

Please write more information in the changelog and resubmit.

Do you mean to add more description in the patch comments?

Regards,
Libin

Quoted 7 lines

Takashi
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

#5 lore raw

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 56 lines · Libin

Hi Takashi,

Quoted 52 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 22 lines · Libin

Hi Takashi,

Quoted 17 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 7 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Quoted 10 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 4 lines

This certainly sucks. It means that the powerwell is on even you don't use the HDMI/DP at all. If this is intended as a temporarily workaround, it should be mentioned so. Please give more

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even there is no

HDMI/DP

Quoted 2 lines

codec, we should also add this flag. Otherwise the HDMI/DP codec may not be detected correctly.

Quoted 6 lines
Quoted 4 lines

But it's possible to do it only at probing, not permanently. If so, we'll have another patch in future.

Please write more information in the changelog and resubmit.

Do you mean to add more description in the patch comments?

Yes. The hardware design is different from HSW/BDW, thus applying this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a permanent fix. Such information must be written there, too.

Takashi

author
#6 lore raw

Hi Takashi,

Quoted 71 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 45 lines · Libin

Hi Takashi,

Quoted 41 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 30 lines · Libin

Hi Takashi,

Quoted 13 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 3 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

Quoted 7 lines
Quoted 5 lines

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 2 lines

This certainly sucks. It means that the powerwell is on even

you

Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines

don't use the HDMI/DP at all. If this is intended as a temporarily workaround, it should be mentioned so. Please give more

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even there is no

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the HDMI/DP

codec

Quoted 5 lines
Quoted 5 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not permanently. If so, we'll have another patch in future.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Please write more information in the changelog and resubmit.

Do you mean to add more description in the patch comments?

Yes. The hardware design is different from HSW/BDW, thus applying this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a permanent fix. Such information must be written there, too.

OK. I see. It seems we need more input from our silicon team for this issue.

Quoted 3 lines

Takashi

Regards,
Libin

author
#7 lore raw

Hi Takashi,

Quoted 93 lines

-----Original Message-----
From: Yang, Libin
Sent: Friday, March 27, 2015 4:34 PM
To: Takashi Iwai
Cc: [email protected]
Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Hi Takashi,

Quoted 49 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 39 lines · Libin

Hi Takashi,

Quoted 35 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 24 lines · Libin

Hi Takashi,

Quoted 19 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 8 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 2 lines

This certainly sucks. It means that the powerwell is on even

you

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

don't use the HDMI/DP at all. If this is intended as a

temporarily

Quoted 14 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line

workaround, it should be mentioned so. Please give more

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even there is no

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the HDMI/DP

codec

Quoted 5 lines
Quoted 5 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not permanently. If so, we'll have another patch in future.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Please write more information in the changelog and resubmit.

Do you mean to add more description in the patch comments?

Yes. The hardware design is different from HSW/BDW, thus applying this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a permanent fix. Such information must be written there, too.

Quoted 4 lines

OK. I see. It seems we need more input from our silicon team for this issue.

From our silicon team's comment, it seems we need power on

the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

What do you think?

Quoted 7 lines
Quoted 4 lines

Takashi

Regards,
Libin

Regards,
Libin

#8 lore raw
Quoted 6 lines · Libin

From our silicon team's comment, it seems we need power on
the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

Quoted 8 lines

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

What do you think?

Side question - do you know if it's the same for braswell? I e, the HDMI codec is in the powerwell but the controller is not?

Signature · 2 lines
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
author
#9 lore raw

Hi David,

Quoted 25 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of David Henningsson
Sent: Wednesday, April 01, 2015 3:31 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 13 lines · Libin

From our silicon team's comment, it seems we need power on
the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

Quoted 4 lines
Quoted 1 line

What do you think?

Side question - do you know if it's the same for braswell? I e, the HDMI codec is in the powerwell but the controller is not?

Yes, BSW has the same issue.

At first, my thinking is:
1. define a new flag, power on the power well if flag is set
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag.
4. after initialization, power off the power well if new flag is set and AZX_DCAPS_I915_POWERWELL is not set
5. power on power well when pcm open
6. power off the power well when pcm close

After a second thought, the method may have limitation:
the codec may send the unsolicited responses.

So my currently thinking is:
If the codec is in powerwell, the behavior is like the flag
AZX_DCAPS_I915_POWERWELL: always power on unless suspend.
This method may cause power consumption. But consider normally audio will be in suspend state in most case.

Quoted 8 lines

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

#10 lore raw
Quoted 34 lines · Libin

Hi David,

Quoted 25 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of David Henningsson
Sent: Wednesday, April 01, 2015 3:31 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 13 lines · Libin

From our silicon team's comment, it seems we need power on
the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

Quoted 4 lines
Quoted 1 line

What do you think?

Side question - do you know if it's the same for braswell? I e, the HDMI codec is in the powerwell but the controller is not?

Yes, BSW has the same issue.

Quoted 9 lines

At first, my thinking is:
1. define a new flag, power on the power well if flag is set
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag.
4. after initialization, power off the power well if new flag is set and AZX_DCAPS_I915_POWERWELL is not set
5. power on power well when pcm open
6. power off the power well when pcm close

...and we're opening the right codec - in case of analog playback, the powerwell can still be off, right?

Quoted 9 lines

After a second thought, the method may have limitation:
the codec may send the unsolicited responses.

So my currently thinking is:
If the codec is in powerwell, the behavior is like the flag
AZX_DCAPS_I915_POWERWELL: always power on unless suspend.
This method may cause power consumption. But consider normally audio will be in suspend state in most case.

Ok, thanks for the clarification. I was wondering - the reason the codec might send unsol events is just due to hotplug events, right?

And these unsol events are triggered by the i915 side by setting some registers, right?

Is it then possible that i915, when it detects a video hotplug event, can increment the powerwell counter (so that the powerwell is turned on), trigger the hotplug event to the audio side, and then decrement the powerwell again (possibly with a few seconds delay if needed)?

The idea is that once the audio side has got the hotplug event, it can turn on and off the powerwell itself. We just need the i915 driver to turn it on for us to get the hotplug event through.

Signature · 2 lines
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
author
#11 lore raw

Hi David,

Quoted 81 lines

-----Original Message-----
From: David Henningsson [mailto:[email protected]]
Sent: Wednesday, April 01, 2015 4:12 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 30 lines · Libin

Hi David,

Quoted 28 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of David Henningsson
Sent: Wednesday, April 01, 2015 3:31 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 6 lines · Libin

From our silicon team's comment, it seems we need power on
the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

Quoted 8 lines

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

What do you think?

Side question - do you know if it's the same for braswell? I e, the

HDMI

Quoted 4 lines
Quoted 1 line

codec is in the powerwell but the controller is not?

Yes, BSW has the same issue.

Quoted 9 lines

At first, my thinking is:
1. define a new flag, power on the power well if flag is set
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag.
4. after initialization, power off the power well if new flag is set and AZX_DCAPS_I915_POWERWELL is not set
5. power on power well when pcm open
6. power off the power well when pcm close

...and we're opening the right codec - in case of analog playback, the powerwell can still be off, right?

Quoted 9 lines

After a second thought, the method may have limitation:
the codec may send the unsolicited responses.

So my currently thinking is:
If the codec is in powerwell, the behavior is like the flag
AZX_DCAPS_I915_POWERWELL: always power on unless suspend.
This method may cause power consumption. But consider normally audio will be in suspend state in most case.

Ok, thanks for the clarification. I was wondering - the reason the codec might send unsol events is just due to hotplug events, right?

And these unsol events are triggered by the i915 side by setting some registers, right?

Is it then possible that i915, when it detects a video hotplug event, can increment the powerwell counter (so that the powerwell is turned on), trigger the hotplug event to the audio side, and then decrement the
powerwell again (possibly with a few seconds delay if needed)?

Quoted 3 lines

The idea is that once the audio side has got the hotplug event, it can turn on and off the powerwell itself. We just need the i915 driver to turn it on for us to get the hotplug event through.

Normally, it's the hotplug unsol event. But sometimes, I found on SKL, when suspending, there is some strange event sending from hdmi codec. And the controller will fallback to single cmd mode. Then the analog audio will be impacted after resume.

BSW may also have this issue. But I didn't find it on our RVP board.

I will test power on power well during unsol events.

Quoted 4 lines

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

Regards,
Libin

#12 lore raw
Quoted 89 lines · Libin

Hi David,

Quoted 81 lines

-----Original Message-----
From: David Henningsson [mailto:[email protected]]
Sent: Wednesday, April 01, 2015 4:12 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 30 lines · Libin

Hi David,

Quoted 28 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of David Henningsson
Sent: Wednesday, April 01, 2015 3:31 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 6 lines · Libin

From our silicon team's comment, it seems we need power on
the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

Quoted 8 lines

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

What do you think?

Side question - do you know if it's the same for braswell? I e, the

HDMI

Quoted 4 lines
Quoted 1 line

codec is in the powerwell but the controller is not?

Yes, BSW has the same issue.

Quoted 9 lines

At first, my thinking is:
1. define a new flag, power on the power well if flag is set
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag.
4. after initialization, power off the power well if new flag is set and AZX_DCAPS_I915_POWERWELL is not set
5. power on power well when pcm open
6. power off the power well when pcm close

...and we're opening the right codec - in case of analog playback, the powerwell can still be off, right?

Quoted 9 lines

After a second thought, the method may have limitation:
the codec may send the unsolicited responses.

So my currently thinking is:
If the codec is in powerwell, the behavior is like the flag
AZX_DCAPS_I915_POWERWELL: always power on unless suspend.
This method may cause power consumption. But consider normally audio will be in suspend state in most case.

Ok, thanks for the clarification. I was wondering - the reason the codec might send unsol events is just due to hotplug events, right?

And these unsol events are triggered by the i915 side by setting some registers, right?

Is it then possible that i915, when it detects a video hotplug event, can increment the powerwell counter (so that the powerwell is turned on), trigger the hotplug event to the audio side, and then decrement the
powerwell again (possibly with a few seconds delay if needed)?

Quoted 3 lines

The idea is that once the audio side has got the hotplug event, it can turn on and off the powerwell itself. We just need the i915 driver to turn it on for us to get the hotplug event through.

Quoted 8 lines

Normally, it's the hotplug unsol event. But sometimes, I found on SKL, when suspending, there is some strange event sending from hdmi codec. And the controller will fallback to single cmd mode. Then the analog audio will be impacted after resume.

BSW may also have this issue. But I didn't find it on our RVP board.

I will test power on power well during unsol events.

Ok. I wonder if it is possible to stop this "strange event", either because it's a hardware/firmware bug that could be fixed in later revisions of the hardware/firmware, or if there is some register we could write to (or even stop writing to!) on the i915 side that stops the unsol events.

Or as a last resort, if we can't stop the "strange event", maybe there's some way the audio side could handle the "strange event" more gracefully so we don't fall back to single cmd mode when this happens.

(I don't know much about this "strange event" so I'm just guessing here)

Signature · 2 lines
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
author
#13 lore raw

Hi David,

Quoted 122 lines

-----Original Message-----
From: David Henningsson [mailto:[email protected]]
Sent: Thursday, April 02, 2015 2:52 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 6 lines · Libin

Hi David,

Quoted 2 lines

-----Original Message-----
From: David Henningsson

[mailto:[email protected]]

Quoted 22 lines
Quoted 22 lines

Sent: Wednesday, April 01, 2015 4:12 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 13 lines · Libin

Hi David,

Quoted 11 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of David Henningsson
Sent: Wednesday, April 01, 2015 3:31 PM
To: Yang, Libin; 'Takashi Iwai'
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines · Libin
Quoted 4 lines

From our silicon team's comment, it seems we need power on
the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller

not),

Quoted 27 lines
Quoted 27 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

What do you think?

Side question - do you know if it's the same for braswell? I e, the

HDMI

Quoted 13 lines
Quoted 1 line

codec is in the powerwell but the controller is not?

Yes, BSW has the same issue.

At first, my thinking is:
1. define a new flag, power on the power well if flag is set
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag.
4. after initialization, power off the power well if new flag is set and AZX_DCAPS_I915_POWERWELL is not set
5. power on power well when pcm open
6. power off the power well when pcm close

Quoted 1 line
Quoted 1 line

...and we're opening the right codec - in case of analog playback,

the

Quoted 5 lines
Quoted 5 lines

powerwell can still be off, right?

Quoted 3 lines

After a second thought, the method may have limitation:
the codec may send the unsolicited responses.

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines

So my currently thinking is:
If the codec is in powerwell, the behavior is like the flag
AZX_DCAPS_I915_POWERWELL: always power on unless suspend.
This method may cause power consumption. But consider normally audio will be in suspend state in most case.

Ok, thanks for the clarification. I was wondering - the reason the

codec

Quoted 3 lines
Quoted 3 lines

might send unsol events is just due to hotplug events, right?

And these unsol events are triggered by the i915 side by setting

some

Quoted 4 lines
Quoted 4 lines

registers, right?

Is it then possible that i915, when it detects a video hotplug event, can increment the powerwell counter (so that the powerwell is

turned

Quoted 1 line
Quoted 1 line

on), trigger the hotplug event to the audio side, and then

decrement

Quoted 7 lines
Quoted 6 lines

the
powerwell again (possibly with a few seconds delay if needed)?

The idea is that once the audio side has got the hotplug event, it can turn on and off the powerwell itself. We just need the i915 driver to turn it on for us to get the hotplug event through.

Quoted 8 lines

Normally, it's the hotplug unsol event. But sometimes, I found on SKL, when suspending, there is some strange event sending from hdmi codec. And the controller will fallback to single cmd mode. Then the analog audio will be impacted after resume.

BSW may also have this issue. But I didn't find it on our RVP board.

I will test power on power well during unsol events.

Ok. I wonder if it is possible to stop this "strange event", either because it's a hardware/firmware bug that could be fixed in later revisions of the hardware/firmware, or if there is some register we could write to (or even stop writing to!) on the i915 side that stops the unsol events.

Quoted 7 lines

Or as a last resort, if we can't stop the "strange event", maybe there's some way the audio side could handle the "strange event" more gracefully
so we don't fall back to single cmd mode when this happens.

(I don't know much about this "strange event" so I'm just guessing here)

Yes, we should stop this unsol events. I will check with internal team later. This should be the real fix. On the other hand, for robustness, audio driver should be able to handle these events.

Quoted 4 lines

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

Regards,
Libin

#14 lore raw
In reply to Yang, Libin

At Mon, 30 Mar 2015 02:47:45 +0000,

Quoted 106 lines · Libin

Hi Takashi,

Quoted 93 lines

-----Original Message-----
From: Yang, Libin
Sent: Friday, March 27, 2015 4:34 PM
To: Takashi Iwai
Cc: [email protected]
Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Hi Takashi,

Quoted 49 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 39 lines · Libin

Hi Takashi,

Quoted 35 lines

-----Original Message-----
From: [email protected] [mailto:alsa-devel- [email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 24 lines · Libin

Hi Takashi,

Quoted 19 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 8 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 2 lines

This certainly sucks. It means that the powerwell is on even

you

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

don't use the HDMI/DP at all. If this is intended as a

temporarily

Quoted 14 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Quoted 1 line

workaround, it should be mentioned so. Please give more

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even there is no

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the HDMI/DP

codec

Quoted 5 lines
Quoted 5 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not permanently. If so, we'll have another patch in future.

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Please write more information in the changelog and resubmit.

Do you mean to add more description in the patch comments?

Yes. The hardware design is different from HSW/BDW, thus applying this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a permanent fix. Such information must be written there, too.

Quoted 4 lines

OK. I see. It seems we need more input from our silicon team for this issue.

From our silicon team's comment, it seems we need power on

the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

Quoted 8 lines

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

What do you think?

Yes, it's also similar to my plan. My plan has a bit more code changes for adapting to the new structure:

- Move hda_i915.c code into sound/hda to be used for both legacy and new drivers

- Add a reference counter to get/put, so that it can be called from both controller and codec drivers

- Move the component from struct hda_intel to hdac_bus, but dynamically allocating. The helper code can check it like

	if (bus->audio_component)
		bus->audio_component->ops->get_power(...)

(Or the NULL check can be in a helper function)

- get_power() is called at probing as is now

- put_power() is called at the end of azx_probe*()

- There will be two DCAPS flags, one indicating for the old chips that need get_power() for runtime PM, and one indicating get_power() only for probing.

Now an open question is how to make codec driver to call get_power / put_power. To be more clear, currently there is no solid way to allow the codec driver doing something special before / after hda_set_power_state(). One may implement a tricky set_power_state() ops in the codec driver. Or, we may reimplement pre_resume and post_suspend patch_ops again.

Takashi

author
#15 lore raw

Hi Takashi,

Quoted 129 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Saturday, April 04, 2015 6:44 PM
To: Yang, Libin
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Mon, 30 Mar 2015 02:47:45 +0000,

Quoted 32 lines · Libin

Hi Takashi,

Quoted 28 lines

-----Original Message-----
From: Yang, Libin
Sent: Friday, March 27, 2015 4:34 PM
To: Takashi Iwai
Cc: [email protected]
Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Hi Takashi,

Quoted 16 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 6 lines · Libin

Hi Takashi,

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:alsa-

devel-

Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines

[email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 22 lines · Libin

Hi Takashi,

Quoted 17 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 7 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 2 lines

This certainly sucks. It means that the powerwell is on

even

Quoted 9 lines
Quoted 9 lines
Quoted 2 lines

you

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

don't use the HDMI/DP at all. If this is intended as a

temporarily

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line

workaround, it should be mentioned so. Please give more

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even there is

no

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the HDMI/DP

codec

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not permanently. If

so,

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

we'll have another patch in future.

Please write more information in the changelog and resubmit.

Do you mean to add more description in the patch comments?

Yes. The hardware design is different from HSW/BDW, thus

applying

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a permanent

fix.

Quoted 18 lines
Quoted 6 lines
Quoted 1 line

Such information must be written there, too.

OK. I see. It seems we need more input from our silicon team for this issue.

From our silicon team's comment, it seems we need power on

the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

Quoted 4 lines
Quoted 1 line

What do you think?

Yes, it's also similar to my plan. My plan has a bit more code changes for adapting to the new structure:

Glad to know that :-)

Quoted 15 lines

- Move hda_i915.c code into sound/hda to be used for both legacy and new drivers

- Add a reference counter to get/put, so that it can be called from both controller and codec drivers

- Move the component from struct hda_intel to hdac_bus, but dynamically allocating. The helper code can check it like

	if (bus->audio_component)
		bus->audio_component->ops->get_power(...)

(Or the NULL check can be in a helper function)

Quoted 8 lines

- get_power() is called at probing as is now

- put_power() is called at the end of azx_probe*()

- There will be two DCAPS flags, one indicating for the old chips that need get_power() for runtime PM, and one indicating get_power()
only
for probing.

Yes, we need separate these situations. Currently, we may consider such situations:
1. HDA controller and HDA(HDMI) codec are in power well.
2. HDA(HDMI) codec is in power well.
3. HDA controller is in power well while HDA codec not.
4. HDA controller and HDA code are not in power well.

We currently don't have really boards which are the 3rd and 4th situations.

For second situation, we need power on the power well for probing and each time we use the HDMI codec.

As this may takes some time, what do you think I submit a temporary patch that using AZX_DCAPS_I915_POWERWELL for the second situation?
After the restructure is finished, we can revert the patch.

Quoted 8 lines

Now an open question is how to make codec driver to call get_power / put_power. To be more clear, currently there is no solid way to allow the codec driver doing something special before / after hda_set_power_state(). One may implement a tricky set_power_state()
ops in the codec driver. Or, we may reimplement pre_resume and post_suspend patch_ops again.

Yes, we need consider this situation. What about the suspend_late and resume_early in dev_pm_ops?

Quoted 3 lines

Takashi

Regards,
Libin

#16 lore raw

At Tue, 7 Apr 2015 05:47:50 +0000,

Quoted 173 lines · Libin

Hi Takashi,

Quoted 129 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Saturday, April 04, 2015 6:44 PM
To: Yang, Libin
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Mon, 30 Mar 2015 02:47:45 +0000,

Quoted 32 lines · Libin

Hi Takashi,

Quoted 28 lines

-----Original Message-----
From: Yang, Libin
Sent: Friday, March 27, 2015 4:34 PM
To: Takashi Iwai
Cc: [email protected]
Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Hi Takashi,

Quoted 16 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 6 lines · Libin

Hi Takashi,

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:alsa-

devel-

Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines
Quoted 31 lines

[email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 22 lines · Libin

Hi Takashi,

Quoted 17 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 7 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well. The power well must be turned on before probing the HDMI/DP codec.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 2 lines

This certainly sucks. It means that the powerwell is on

even

Quoted 9 lines
Quoted 9 lines
Quoted 2 lines

you

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

don't use the HDMI/DP at all. If this is intended as a

temporarily

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line

workaround, it should be mentioned so. Please give more

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even there is

no

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the HDMI/DP

codec

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not permanently. If

so,

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

we'll have another patch in future.

Please write more information in the changelog and resubmit.

Do you mean to add more description in the patch comments?

Yes. The hardware design is different from HSW/BDW, thus

applying

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a permanent

fix.

Quoted 18 lines
Quoted 6 lines
Quoted 1 line

Such information must be written there, too.

OK. I see. It seems we need more input from our silicon team for this issue.

From our silicon team's comment, it seems we need power on

the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller not), my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

Quoted 4 lines
Quoted 1 line

What do you think?

Yes, it's also similar to my plan. My plan has a bit more code changes for adapting to the new structure:

Glad to know that :-)

Quoted 15 lines

- Move hda_i915.c code into sound/hda to be used for both legacy and new drivers

- Add a reference counter to get/put, so that it can be called from both controller and codec drivers

- Move the component from struct hda_intel to hdac_bus, but dynamically allocating. The helper code can check it like

	if (bus->audio_component)
		bus->audio_component->ops->get_power(...)

(Or the NULL check can be in a helper function)

Quoted 8 lines

- get_power() is called at probing as is now

- put_power() is called at the end of azx_probe*()

- There will be two DCAPS flags, one indicating for the old chips that need get_power() for runtime PM, and one indicating get_power()
only
for probing.

Yes, we need separate these situations. Currently, we may consider such situations:
1. HDA controller and HDA(HDMI) codec are in power well.
2. HDA(HDMI) codec is in power well.
3. HDA controller is in power well while HDA codec not.
4. HDA controller and HDA code are not in power well.

We currently don't have really boards which are the 3rd and 4th situations.

Quoted 7 lines

For second situation, we need power on the power well for probing and each time we use the HDMI codec.

As this may takes some time, what do you think I submit a temporary patch that using AZX_DCAPS_I915_POWERWELL for the second situation?
After the restructure is finished, we can revert the patch.

Yes, it makes sense, but please describe it in the changelog more clearly.

Quoted 10 lines
Quoted 7 lines

Now an open question is how to make codec driver to call get_power / put_power. To be more clear, currently there is no solid way to allow the codec driver doing something special before / after hda_set_power_state(). One may implement a tricky set_power_state()
ops in the codec driver. Or, we may reimplement pre_resume and post_suspend patch_ops again.

Yes, we need consider this situation. What about the suspend_late and resume_early in dev_pm_ops?

It's a good alternative, too. Maybe we should clean up the codec driver code to use driver and pm ops directly at first.

Takashi

author
#17 lore raw

Hi Takashi,

Quoted 183 lines
Quoted 65 lines
Quoted 65 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Saturday, April 04, 2015 6:44 PM
To: Yang, Libin
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Mon, 30 Mar 2015 02:47:45 +0000,

Quoted 32 lines · Libin

Hi Takashi,

Quoted 28 lines

-----Original Message-----
From: Yang, Libin
Sent: Friday, March 27, 2015 4:34 PM
To: Takashi Iwai
Cc: [email protected]
Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Hi Takashi,

Quoted 16 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 6 lines · Libin

Hi Takashi,

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:alsa-

devel-

Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines

[email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 13 lines · Libin

Hi Takashi,

Quoted 8 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 5 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well.
The power well must be turned on before probing

the

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

This certainly sucks. It means that the powerwell is on

even

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

you

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

don't use the HDMI/DP at all. If this is intended as a

temporarily

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

workaround, it should be mentioned so. Please give

more

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even

there is

Quoted 3 lines
Quoted 3 lines

no

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the

HDMI/DP

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

codec

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not permanently.

If

Quoted 4 lines
Quoted 4 lines

so,

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

we'll have another patch in future.

Please write more information in the changelog and

resubmit.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Do you mean to add more description in the patch

comments?

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Yes. The hardware design is different from HSW/BDW, thus

applying

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a

permanent

Quoted 10 lines
Quoted 10 lines

fix.

Quoted 9 lines
Quoted 6 lines
Quoted 1 line

Such information must be written there, too.

OK. I see. It seems we need more input from our silicon team for this issue.

From our silicon team's comment, it seems we need power on

the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller

not),

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

Quoted 9 lines
Quoted 4 lines
Quoted 1 line

What do you think?

Yes, it's also similar to my plan. My plan has a bit more code changes for adapting to the new structure:

Glad to know that :-)

Quoted 2 lines

- Move hda_i915.c code into sound/hda to be used for both legacy

and

Quoted 35 lines
Quoted 13 lines

new drivers

- Add a reference counter to get/put, so that it can be called from both controller and codec drivers

- Move the component from struct hda_intel to hdac_bus, but dynamically allocating. The helper code can check it like

	if (bus->audio_component)
		bus->audio_component->ops->get_power(...)

(Or the NULL check can be in a helper function)

Quoted 8 lines

- get_power() is called at probing as is now

- put_power() is called at the end of azx_probe*()

- There will be two DCAPS flags, one indicating for the old chips that need get_power() for runtime PM, and one indicating get_power()
only
for probing.

Yes, we need separate these situations. Currently, we may consider such situations:
1. HDA controller and HDA(HDMI) codec are in power well.
2. HDA(HDMI) codec is in power well.
3. HDA controller is in power well while HDA codec not.
4. HDA controller and HDA code are not in power well.

We currently don't have really boards which are the 3rd and 4th situations.

For second situation, we need power on the power well for probing and each time we use the HDMI codec.

Quoted 7 lines
Quoted 4 lines

As this may takes some time, what do you think I submit a temporary patch that using AZX_DCAPS_I915_POWERWELL for the second situation?
After the restructure is finished, we can revert the patch.

Yes, it makes sense, but please describe it in the changelog more clearly.

Got it. Thanks.

Quoted 12 lines
Quoted 3 lines
Quoted 3 lines

Now an open question is how to make codec driver to call

get_power /

Quoted 1 line
Quoted 1 line

put_power. To be more clear, currently there is no solid way to

allow

Quoted 6 lines
Quoted 5 lines

the codec driver doing something special before / after hda_set_power_state(). One may implement a tricky set_power_state()
ops in the codec driver. Or, we may reimplement pre_resume and post_suspend patch_ops again.

Quoted 6 lines
Quoted 1 line

Yes, we need consider this situation. What about the suspend_late

and

Quoted 1 line

resume_early in dev_pm_ops?

It's a good alternative, too. Maybe we should clean up the codec driver code to use driver and pm ops directly at first.

Yes, use pm ops is a good idea :-)

Besides suspend/resume, I'm still wondering how to handling the unsol event. Is it possible that codec is power off, but it still send the events. Especially for HDMI codec, I met such situation, we didn't get power for HDMI codec, but when S4, BSW and SKL will get unsol event (maybe gfx driver has power on the power well), and HDA controller can handle this event correctly, and it will fallback.

Quoted 3 lines

Takashi

Regards,
Libin

#18 lore raw

At Tue, 7 Apr 2015 06:12:00 +0000,

Quoted 217 lines · Libin

Hi Takashi,

Quoted 183 lines
Quoted 65 lines
Quoted 65 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Saturday, April 04, 2015 6:44 PM
To: Yang, Libin
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Mon, 30 Mar 2015 02:47:45 +0000,

Quoted 32 lines · Libin

Hi Takashi,

Quoted 28 lines

-----Original Message-----
From: Yang, Libin
Sent: Friday, March 27, 2015 4:34 PM
To: Takashi Iwai
Cc: [email protected]
Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Hi Takashi,

Quoted 16 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 6 lines · Libin

Hi Takashi,

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:alsa-

devel-

Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines

[email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 13 lines · Libin

Hi Takashi,

Quoted 8 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 5 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well.
The power well must be turned on before probing

the

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines

HDMI/DP codec.

Signed-off-by Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 1 line
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

This certainly sucks. It means that the powerwell is on

even

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

you

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

don't use the HDMI/DP at all. If this is intended as a

temporarily

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

workaround, it should be mentioned so. Please give

more

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even

there is

Quoted 3 lines
Quoted 3 lines

no

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the

HDMI/DP

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

codec

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not permanently.

If

Quoted 4 lines
Quoted 4 lines

so,

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

we'll have another patch in future.

Please write more information in the changelog and

resubmit.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Do you mean to add more description in the patch

comments?

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Yes. The hardware design is different from HSW/BDW, thus

applying

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

this isn't straightforward but just a workaround. I don't know whether you think it's a temporary workaround or a

permanent

Quoted 10 lines
Quoted 10 lines

fix.

Quoted 9 lines
Quoted 6 lines
Quoted 1 line

Such information must be written there, too.

OK. I see. It seems we need more input from our silicon team for this issue.

From our silicon team's comment, it seems we need power on

the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller

not),

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

Quoted 9 lines
Quoted 4 lines
Quoted 1 line

What do you think?

Yes, it's also similar to my plan. My plan has a bit more code changes for adapting to the new structure:

Glad to know that :-)

Quoted 2 lines

- Move hda_i915.c code into sound/hda to be used for both legacy

and

Quoted 35 lines
Quoted 13 lines

new drivers

- Add a reference counter to get/put, so that it can be called from both controller and codec drivers

- Move the component from struct hda_intel to hdac_bus, but dynamically allocating. The helper code can check it like

	if (bus->audio_component)
		bus->audio_component->ops->get_power(...)

(Or the NULL check can be in a helper function)

Quoted 8 lines

- get_power() is called at probing as is now

- put_power() is called at the end of azx_probe*()

- There will be two DCAPS flags, one indicating for the old chips that need get_power() for runtime PM, and one indicating get_power()
only
for probing.

Yes, we need separate these situations. Currently, we may consider such situations:
1. HDA controller and HDA(HDMI) codec are in power well.
2. HDA(HDMI) codec is in power well.
3. HDA controller is in power well while HDA codec not.
4. HDA controller and HDA code are not in power well.

We currently don't have really boards which are the 3rd and 4th situations.

For second situation, we need power on the power well for probing and each time we use the HDMI codec.

Quoted 7 lines
Quoted 4 lines

As this may takes some time, what do you think I submit a temporary patch that using AZX_DCAPS_I915_POWERWELL for the second situation?
After the restructure is finished, we can revert the patch.

Yes, it makes sense, but please describe it in the changelog more clearly.

Got it. Thanks.

Quoted 12 lines
Quoted 3 lines
Quoted 3 lines

Now an open question is how to make codec driver to call

get_power /

Quoted 1 line
Quoted 1 line

put_power. To be more clear, currently there is no solid way to

allow

Quoted 6 lines
Quoted 5 lines

the codec driver doing something special before / after hda_set_power_state(). One may implement a tricky set_power_state()
ops in the codec driver. Or, we may reimplement pre_resume and post_suspend patch_ops again.

Quoted 6 lines
Quoted 1 line

Yes, we need consider this situation. What about the suspend_late

and

Quoted 1 line

resume_early in dev_pm_ops?

It's a good alternative, too. Maybe we should clean up the codec driver code to use driver and pm ops directly at first.

Yes, use pm ops is a good idea :-)

Quoted 7 lines

Besides suspend/resume, I'm still wondering how to handling the unsol event. Is it possible that codec is power off, but it still send the events. Especially for HDMI codec, I met such situation, we didn't get power for HDMI codec, but when S4, BSW and SKL will get unsol event (maybe gfx driver has power on the power well), and HDA controller can handle this event correctly, and it will fallback.

The unsol events aren't handled usually while the device is not in use (thus in power save mode). We thought that the unsol event could be delivered even in D3 when EPSS is available, but it didn't work as expected in the end.

Upon power-save resume, the driver checks the jack state. It's mostly OK, the only problem is that any GUI won't be notified at the moment the jack state changed but only at the moment the power-saving state changed. So I don't think we need anything special for SKL.

Takashi

author
#19 lore raw

Hi Takashi,

Quoted 257 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Tuesday, April 07, 2015 2:24 PM
To: Yang, Libin
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Tue, 7 Apr 2015 06:12:00 +0000,

Quoted 45 lines · Libin

Hi Takashi,

Quoted 42 lines
Quoted 42 lines
Quoted 42 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Saturday, April 04, 2015 6:44 PM
To: Yang, Libin
Cc: '[email protected]'
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Mon, 30 Mar 2015 02:47:45 +0000,

Quoted 32 lines · Libin

Hi Takashi,

Quoted 28 lines

-----Original Message-----
From: Yang, Libin
Sent: Friday, March 27, 2015 4:34 PM
To: Takashi Iwai
Cc: [email protected]
Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Hi Takashi,

Quoted 16 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 4:30 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:25:52 +0000,

Quoted 6 lines · Libin

Hi Takashi,

Quoted 2 lines

-----Original Message-----
From: [email protected]

[mailto:alsa-

Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
Quoted 23 lines

devel-

Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines

[email protected]] On Behalf Of Takashi Iwai
Sent: Friday, March 27, 2015 4:19 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

At Fri, 27 Mar 2015 08:02:54 +0000,

Quoted 13 lines · Libin

Hi Takashi,

Quoted 8 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, March 27, 2015 3:57 PM
To: Yang, Libin
Cc: [email protected]
Subject: Re: [PATCH] ALSA: hda_intel: add
AZX_DCAPS_I915_POWERWELL for skl

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

At Fri, 27 Mar 2015 15:10:04 +0800,

Quoted 5 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

HDMI/DP codec on SKL is in the power well.
The power well must be turned on before

probing

Quoted 11 lines
Quoted 11 lines

the

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
Quoted 3 lines

HDMI/DP codec.

Signed-off-by: Libin Yang <[email protected]>

So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

Quoted 2 lines

This certainly sucks. It means that the powerwell is

on

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

even

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

you

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

don't use the HDMI/DP at all. If this is intended as

a

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

temporarily

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

workaround, it should be mentioned so. Please

give

Quoted 7 lines
Quoted 7 lines

more

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

comments

Quoted 4 lines
Quoted 2 lines

and
backgrounds.

Yes, as this is added in the skl audio controller, even

there is

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines

no

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

HDMI/DP

Quoted 1 line

codec, we should also add this flag. Otherwise the

HDMI/DP

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

codec

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

may not be detected correctly.

But it's possible to do it only at probing, not

permanently.

Quoted 11 lines
Quoted 11 lines

If

Quoted 4 lines
Quoted 4 lines

so,

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

we'll have another patch in future.

Please write more information in the changelog and

resubmit.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Do you mean to add more description in the patch

comments?

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

Yes. The hardware design is different from HSW/BDW,

thus

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

applying

Quoted 1 line
Quoted 1 line
Quoted 1 line

this isn't straightforward but just a workaround. I don't

know

Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

whether you think it's a temporary workaround or a

permanent

Quoted 4 lines
Quoted 4 lines

fix.

Quoted 3 lines
Quoted 3 lines
Quoted 1 line

Such information must be written there, too.

OK. I see. It seems we need more input from our silicon

team

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

for this issue.

From our silicon team's comment, it seems we need power

on

Quoted 15 lines
Quoted 15 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

the powerwell when detecting and probing the HDMI codec.

For the skl case (HDMI codec is in powerwell, while controller

not),

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines

my thinking is:

1. power on the power well
2. read STATESTS to determine the codec_mask
3. if codec is not present, power off the power well and remove the flag. (seems we need a new flag)
4. if codec is present, we will power on the power well when dealing with HDMI codec.

What do you think?

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines

Yes, it's also similar to my plan. My plan has a bit more code changes for adapting to the new structure:

Glad to know that :-)

Quoted 2 lines

- Move hda_i915.c code into sound/hda to be used for both

legacy

Quoted 4 lines
Quoted 4 lines

and

Quoted 3 lines
Quoted 3 lines

new drivers

- Add a reference counter to get/put, so that it can be called

from

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

both controller and codec drivers

- Move the component from struct hda_intel to hdac_bus, but dynamically allocating. The helper code can check it like

	if (bus->audio_component)
		bus->audio_component->ops->get_power(...)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

(Or the NULL check can be in a helper function)

- get_power() is called at probing as is now

- put_power() is called at the end of azx_probe*()

- There will be two DCAPS flags, one indicating for the old chips

that

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

need get_power() for runtime PM, and one indicating

get_power()

Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
Quoted 2 lines

only
for probing.

Yes, we need separate these situations. Currently, we may consider such situations:
1. HDA controller and HDA(HDMI) codec are in power well.
2. HDA(HDMI) codec is in power well.
3. HDA controller is in power well while HDA codec not.
4. HDA controller and HDA code are not in power well.

We currently don't have really boards which are the 3rd and 4th situations.

For second situation, we need power on the power well for probing and each time we use the HDMI codec.

As this may takes some time, what do you think I submit a temporary patch that using AZX_DCAPS_I915_POWERWELL for the second situation?
After the restructure is finished, we can revert the patch.

Quoted 15 lines
Quoted 2 lines

Yes, it makes sense, but please describe it in the changelog more clearly.

Got it. Thanks.

Quoted 10 lines
Quoted 3 lines
Quoted 3 lines

Now an open question is how to make codec driver to call

get_power /

Quoted 1 line
Quoted 1 line

put_power. To be more clear, currently there is no solid way to

allow

Quoted 4 lines
Quoted 4 lines

the codec driver doing something special before / after hda_set_power_state(). One may implement a tricky set_power_state()
ops in the codec driver. Or, we may reimplement pre_resume

and

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line

post_suspend patch_ops again.

Yes, we need consider this situation. What about the

suspend_late

Quoted 8 lines
Quoted 5 lines

and

Quoted 1 line

resume_early in dev_pm_ops?

It's a good alternative, too. Maybe we should clean up the codec driver code to use driver and pm ops directly at first.

Yes, use pm ops is a good idea :-)

Quoted 7 lines

Besides suspend/resume, I'm still wondering how to handling the unsol event. Is it possible that codec is power off, but it still send the events. Especially for HDMI codec, I met such situation, we didn't get power for HDMI codec, but when S4, BSW and SKL will get unsol event (maybe gfx driver has power on the power well), and HDA controller can handle this event correctly, and it will fallback.

The unsol events aren't handled usually while the device is not in use (thus in power save mode). We thought that the unsol event could
be delivered even in D3 when EPSS is available, but it didn't work as expected in the end.

Quoted 4 lines

Upon power-save resume, the driver checks the jack state. It's mostly OK, the only problem is that any GUI won't be notified at the moment the jack state changed but only at the moment the power-saving state changed. So I don't think we need anything special for SKL.

Got it. Thanks a lot.

Quoted 3 lines

Takashi

Regards,
Libin