diff mbox

ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Message ID 1427440204-49331-1-git-send-email-libin.yang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yang, Libin March 27, 2015, 7:10 a.m. UTC
From: Libin Yang <libin.yang@intel.com>

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 <libin.yang@intel.com>
---
 sound/pci/hda/hda_intel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Takashi Iwai March 27, 2015, 7:56 a.m. UTC | #1
At Fri, 27 Mar 2015 15:10:04 +0800,
libin.yang@intel.com wrote:
> 
> From: Libin Yang <libin.yang@intel.com>
> 
> 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 <libin.yang@intel.com>

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

> ---
>  sound/pci/hda/hda_intel.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 15a8299..4744a96 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -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 \
> -- 
> 1.9.1
>
Yang, Libin March 27, 2015, 8:02 a.m. UTC | #2
Hi Takashi,



> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, March 27, 2015 3:57 PM
> To: Yang, Libin
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> At Fri, 27 Mar 2015 15:10:04 +0800,
> libin.yang@intel.com wrote:
> >
> > From: Libin Yang <libin.yang@intel.com>
> >
> > 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 <libin.yang@intel.com>
> 
> So, was the previous question clarified?

Yes, I have confirmed with our silicon team.

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

> 
> 
> thanks,
> 
> Takashi


Regards,
Libin
> 
> > ---
> >  sound/pci/hda/hda_intel.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 15a8299..4744a96 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -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 \
> > --
> > 1.9.1
> >
Takashi Iwai March 27, 2015, 8:19 a.m. UTC | #3
At Fri, 27 Mar 2015 08:02:54 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, March 27, 2015 3:57 PM
> > To: Yang, Libin
> > Cc: alsa-devel@alsa-project.org
> > Subject: Re: [PATCH] ALSA: hda_intel: add
> > AZX_DCAPS_I915_POWERWELL for skl
> > 
> > At Fri, 27 Mar 2015 15:10:04 +0800,
> > libin.yang@intel.com wrote:
> > >
> > > From: Libin Yang <libin.yang@intel.com>
> > >
> > > 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 <libin.yang@intel.com>
> > 
> > So, was the previous question clarified?
> 
> Yes, I have confirmed with our silicon team.
> 
> > 
> > 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.

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
Yang, Libin March 27, 2015, 8:25 a.m. UTC | #4
Hi Takashi,


> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> bounces@alsa-project.org] On Behalf Of Takashi Iwai
> Sent: Friday, March 27, 2015 4:19 PM
> To: Yang, Libin
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> At Fri, 27 Mar 2015 08:02:54 +0000,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, March 27, 2015 3:57 PM
> > > To: Yang, Libin
> > > Cc: alsa-devel@alsa-project.org
> > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > AZX_DCAPS_I915_POWERWELL for skl
> > >
> > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > libin.yang@intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang@intel.com>
> > > >
> > > > 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 <libin.yang@intel.com>
> > >
> > > So, was the previous question clarified?
> >
> > Yes, I have confirmed with our silicon team.
> >
> > >
> > > 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.
> 
> 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
> 
> 
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Takashi Iwai March 27, 2015, 8:30 a.m. UTC | #5
At Fri, 27 Mar 2015 08:25:52 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> > -----Original Message-----
> > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > Sent: Friday, March 27, 2015 4:19 PM
> > To: Yang, Libin
> > Cc: alsa-devel@alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > AZX_DCAPS_I915_POWERWELL for skl
> > 
> > At Fri, 27 Mar 2015 08:02:54 +0000,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > To: Yang, Libin
> > > > Cc: alsa-devel@alsa-project.org
> > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > AZX_DCAPS_I915_POWERWELL for skl
> > > >
> > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > libin.yang@intel.com wrote:
> > > > >
> > > > > From: Libin Yang <libin.yang@intel.com>
> > > > >
> > > > > 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 <libin.yang@intel.com>
> > > >
> > > > So, was the previous question clarified?
> > >
> > > Yes, I have confirmed with our silicon team.
> > >
> > > >
> > > > 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.
> > 
> > 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
Yang, Libin March 27, 2015, 8:33 a.m. UTC | #6
Hi Takashi,



> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, March 27, 2015 4:30 PM
> To: Yang, Libin
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> At Fri, 27 Mar 2015 08:25:52 +0000,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> >
> > > -----Original Message-----
> > > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > Sent: Friday, March 27, 2015 4:19 PM
> > > To: Yang, Libin
> > > Cc: alsa-devel@alsa-project.org
> > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > AZX_DCAPS_I915_POWERWELL for skl
> > >
> > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > Yang, Libin wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > To: Yang, Libin
> > > > > Cc: alsa-devel@alsa-project.org
> > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > >
> > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > libin.yang@intel.com wrote:
> > > > > >
> > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > >
> > > > > > 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 <libin.yang@intel.com>
> > > > >
> > > > > So, was the previous question clarified?
> > > >
> > > > Yes, I have confirmed with our silicon team.
> > > >
> > > > >
> > > > > 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.
> > >
> > > 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.

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

> 
> 
> Takashi

Regards,
Libin
Yang, Libin March 30, 2015, 2:47 a.m. UTC | #7
Hi Takashi,


> -----Original Message-----
> From: Yang, Libin
> Sent: Friday, March 27, 2015 4:34 PM
> To: Takashi Iwai
> Cc: alsa-devel@alsa-project.org
> Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> Hi Takashi,
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, March 27, 2015 4:30 PM
> > To: Yang, Libin
> > Cc: alsa-devel@alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > AZX_DCAPS_I915_POWERWELL for skl
> >
> > At Fri, 27 Mar 2015 08:25:52 +0000,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > >
> > > > -----Original Message-----
> > > > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > To: Yang, Libin
> > > > Cc: alsa-devel@alsa-project.org
> > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > AZX_DCAPS_I915_POWERWELL for skl
> > > >
> > > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > > Yang, Libin wrote:
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > To: Yang, Libin
> > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > >
> > > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > > libin.yang@intel.com wrote:
> > > > > > >
> > > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > > >
> > > > > > > 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 <libin.yang@intel.com>
> > > > > >
> > > > > > So, was the previous question clarified?
> > > > >
> > > > > Yes, I have confirmed with our silicon team.
> > > > >
> > > > > >
> > > > > > 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.
> > > >
> > > > 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.
> 
> 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?

> 
> >
> >
> > Takashi
> 
> Regards,
> Libin

Regards,
Libin
David Henningsson April 1, 2015, 7:31 a.m. UTC | #8
On 2015-03-30 04:47, Yang, Libin wrote:
>  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?

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?
Yang, Libin April 1, 2015, 8 a.m. UTC | #9
Hi David,

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> bounces@alsa-project.org] On Behalf Of David Henningsson
> Sent: Wednesday, April 01, 2015 3:31 PM
> To: Yang, Libin; 'Takashi Iwai'
> Cc: 'alsa-devel@alsa-project.org'
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> 
> 
> On 2015-03-30 04:47, Yang, Libin wrote:
> >  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?
> 
> 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.

> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
David Henningsson April 1, 2015, 8:12 a.m. UTC | #10
On 2015-04-01 10:00, Yang, Libin wrote:
> Hi David,
>
>> -----Original Message-----
>> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
>> bounces@alsa-project.org] On Behalf Of David Henningsson
>> Sent: Wednesday, April 01, 2015 3:31 PM
>> To: Yang, Libin; 'Takashi Iwai'
>> Cc: 'alsa-devel@alsa-project.org'
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
>> AZX_DCAPS_I915_POWERWELL for skl
>>
>>
>>
>> On 2015-03-30 04:47, Yang, Libin wrote:
>>>   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?
>>
>> 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

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

> 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.
Yang, Libin April 1, 2015, 9:55 p.m. UTC | #11
Hi David,



> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Wednesday, April 01, 2015 4:12 PM
> To: Yang, Libin; 'Takashi Iwai'
> Cc: 'alsa-devel@alsa-project.org'
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> 
> 
> On 2015-04-01 10:00, Yang, Libin wrote:
> > Hi David,
> >
> >> -----Original Message-----
> >> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> >> bounces@alsa-project.org] On Behalf Of David Henningsson
> >> Sent: Wednesday, April 01, 2015 3:31 PM
> >> To: Yang, Libin; 'Takashi Iwai'
> >> Cc: 'alsa-devel@alsa-project.org'
> >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> >> AZX_DCAPS_I915_POWERWELL for skl
> >>
> >>
> >>
> >> On 2015-03-30 04:47, Yang, Libin wrote:
> >>>   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?
> >>
> >> 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
> 
> ...and we're opening the right codec - in case of analog playback, the
> powerwell can still be off, right?
> 
> > 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.

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.


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

Regards,
Libin
David Henningsson April 2, 2015, 6:52 a.m. UTC | #12
On 2015-04-01 23:55, Yang, Libin wrote:
> Hi David,
>
>
>
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Wednesday, April 01, 2015 4:12 PM
>> To: Yang, Libin; 'Takashi Iwai'
>> Cc: 'alsa-devel@alsa-project.org'
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
>> AZX_DCAPS_I915_POWERWELL for skl
>>
>>
>>
>> On 2015-04-01 10:00, Yang, Libin wrote:
>>> Hi David,
>>>
>>>> -----Original Message-----
>>>> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
>>>> bounces@alsa-project.org] On Behalf Of David Henningsson
>>>> Sent: Wednesday, April 01, 2015 3:31 PM
>>>> To: Yang, Libin; 'Takashi Iwai'
>>>> Cc: 'alsa-devel@alsa-project.org'
>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
>>>> AZX_DCAPS_I915_POWERWELL for skl
>>>>
>>>>
>>>>
>>>> On 2015-03-30 04:47, Yang, Libin wrote:
>>>>>    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?
>>>>
>>>> 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
>>
>> ...and we're opening the right codec - in case of analog playback, the
>> powerwell can still be off, right?
>>
>>> 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.
>
> 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)
Yang, Libin April 2, 2015, 7:23 a.m. UTC | #13
Hi David,



> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Thursday, April 02, 2015 2:52 PM
> To: Yang, Libin; 'Takashi Iwai'
> Cc: 'alsa-devel@alsa-project.org'
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> 
> 
> On 2015-04-01 23:55, Yang, Libin wrote:
> > Hi David,
> >
> >
> >
> >> -----Original Message-----
> >> From: David Henningsson
> [mailto:david.henningsson@canonical.com]
> >> Sent: Wednesday, April 01, 2015 4:12 PM
> >> To: Yang, Libin; 'Takashi Iwai'
> >> Cc: 'alsa-devel@alsa-project.org'
> >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> >> AZX_DCAPS_I915_POWERWELL for skl
> >>
> >>
> >>
> >> On 2015-04-01 10:00, Yang, Libin wrote:
> >>> Hi David,
> >>>
> >>>> -----Original Message-----
> >>>> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> >>>> bounces@alsa-project.org] On Behalf Of David Henningsson
> >>>> Sent: Wednesday, April 01, 2015 3:31 PM
> >>>> To: Yang, Libin; 'Takashi Iwai'
> >>>> Cc: 'alsa-devel@alsa-project.org'
> >>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> >>>> AZX_DCAPS_I915_POWERWELL for skl
> >>>>
> >>>>
> >>>>
> >>>> On 2015-03-30 04:47, Yang, Libin wrote:
> >>>>>    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?
> >>>>
> >>>> 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
> >>
> >> ...and we're opening the right codec - in case of analog playback,
> the
> >> powerwell can still be off, right?
> >>
> >>> 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.
> >
> > 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)

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.

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

Regards,
Libin
Takashi Iwai April 4, 2015, 10:44 a.m. UTC | #14
At Mon, 30 Mar 2015 02:47:45 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> > -----Original Message-----
> > From: Yang, Libin
> > Sent: Friday, March 27, 2015 4:34 PM
> > To: Takashi Iwai
> > Cc: alsa-devel@alsa-project.org
> > Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > AZX_DCAPS_I915_POWERWELL for skl
> > 
> > Hi Takashi,
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, March 27, 2015 4:30 PM
> > > To: Yang, Libin
> > > Cc: alsa-devel@alsa-project.org
> > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > AZX_DCAPS_I915_POWERWELL for skl
> > >
> > > At Fri, 27 Mar 2015 08:25:52 +0000,
> > > Yang, Libin wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> > > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > > To: Yang, Libin
> > > > > Cc: alsa-devel@alsa-project.org
> > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > >
> > > > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > > > Yang, Libin wrote:
> > > > > >
> > > > > > Hi Takashi,
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > > To: Yang, Libin
> > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > >
> > > > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > > > libin.yang@intel.com wrote:
> > > > > > > >
> > > > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > > > >
> > > > > > > > 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 <libin.yang@intel.com>
> > > > > > >
> > > > > > > So, was the previous question clarified?
> > > > > >
> > > > > > Yes, I have confirmed with our silicon team.
> > > > > >
> > > > > > >
> > > > > > > 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.
> > > > >
> > > > > 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.
> > 
> > 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?

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
Yang, Libin April 7, 2015, 5:47 a.m. UTC | #15
Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Saturday, April 04, 2015 6:44 PM
> To: Yang, Libin
> Cc: 'alsa-devel@alsa-project.org'
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> At Mon, 30 Mar 2015 02:47:45 +0000,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> >
> > > -----Original Message-----
> > > From: Yang, Libin
> > > Sent: Friday, March 27, 2015 4:34 PM
> > > To: Takashi Iwai
> > > Cc: alsa-devel@alsa-project.org
> > > Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > AZX_DCAPS_I915_POWERWELL for skl
> > >
> > > Hi Takashi,
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Friday, March 27, 2015 4:30 PM
> > > > To: Yang, Libin
> > > > Cc: alsa-devel@alsa-project.org
> > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > AZX_DCAPS_I915_POWERWELL for skl
> > > >
> > > > At Fri, 27 Mar 2015 08:25:52 +0000,
> > > > Yang, Libin wrote:
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-
> devel-
> > > > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > > > To: Yang, Libin
> > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > >
> > > > > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > > > > Yang, Libin wrote:
> > > > > > >
> > > > > > > Hi Takashi,
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > > > To: Yang, Libin
> > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > >
> > > > > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > > > > libin.yang@intel.com wrote:
> > > > > > > > >
> > > > > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > > > > >
> > > > > > > > > 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 <libin.yang@intel.com>
> > > > > > > >
> > > > > > > > So, was the previous question clarified?
> > > > > > >
> > > > > > > Yes, I have confirmed with our silicon team.
> > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > >
> > > > > > 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.
> > >
> > > 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?
> 
> 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 :-)

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

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.

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

> 
> 
> Takashi

Regards,
Libin
Takashi Iwai April 7, 2015, 6 a.m. UTC | #16
At Tue, 7 Apr 2015 05:47:50 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Saturday, April 04, 2015 6:44 PM
> > To: Yang, Libin
> > Cc: 'alsa-devel@alsa-project.org'
> > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > AZX_DCAPS_I915_POWERWELL for skl
> > 
> > At Mon, 30 Mar 2015 02:47:45 +0000,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Yang, Libin
> > > > Sent: Friday, March 27, 2015 4:34 PM
> > > > To: Takashi Iwai
> > > > Cc: alsa-devel@alsa-project.org
> > > > Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > AZX_DCAPS_I915_POWERWELL for skl
> > > >
> > > > Hi Takashi,
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Friday, March 27, 2015 4:30 PM
> > > > > To: Yang, Libin
> > > > > Cc: alsa-devel@alsa-project.org
> > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > >
> > > > > At Fri, 27 Mar 2015 08:25:52 +0000,
> > > > > Yang, Libin wrote:
> > > > > >
> > > > > > Hi Takashi,
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-
> > devel-
> > > > > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > > > > To: Yang, Libin
> > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > >
> > > > > > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > > > > > Yang, Libin wrote:
> > > > > > > >
> > > > > > > > Hi Takashi,
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > > > > To: Yang, Libin
> > > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > > >
> > > > > > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > > > > > libin.yang@intel.com wrote:
> > > > > > > > > >
> > > > > > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > > > > > >
> > > > > > > > > > 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 <libin.yang@intel.com>
> > > > > > > > >
> > > > > > > > > So, was the previous question clarified?
> > > > > > > >
> > > > > > > > Yes, I have confirmed with our silicon team.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > >
> > > > > > > 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.
> > > >
> > > > 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?
> > 
> > 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 :-)
> 
> > 
> > - 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.
> 
> 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.

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


> > 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
Yang, Libin April 7, 2015, 6:12 a.m. UTC | #17
Hi Takashi,

> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Saturday, April 04, 2015 6:44 PM
> > > To: Yang, Libin
> > > Cc: 'alsa-devel@alsa-project.org'
> > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > AZX_DCAPS_I915_POWERWELL for skl
> > >
> > > At Mon, 30 Mar 2015 02:47:45 +0000,
> > > Yang, Libin wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Yang, Libin
> > > > > Sent: Friday, March 27, 2015 4:34 PM
> > > > > To: Takashi Iwai
> > > > > Cc: alsa-devel@alsa-project.org
> > > > > Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > Sent: Friday, March 27, 2015 4:30 PM
> > > > > > To: Yang, Libin
> > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > >
> > > > > > At Fri, 27 Mar 2015 08:25:52 +0000,
> > > > > > Yang, Libin wrote:
> > > > > > >
> > > > > > > Hi Takashi,
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-
> > > devel-
> > > > > > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > > > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > > > > > To: Yang, Libin
> > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > >
> > > > > > > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > > > > > > Yang, Libin wrote:
> > > > > > > > >
> > > > > > > > > Hi Takashi,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > > > > > To: Yang, Libin
> > > > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > > > >
> > > > > > > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > > > > > > libin.yang@intel.com wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > > > > > > >
> > > > > > > > > > > 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 <libin.yang@intel.com>
> > > > > > > > > >
> > > > > > > > > > So, was the previous question clarified?
> > > > > > > > >
> > > > > > > > > Yes, I have confirmed with our silicon team.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > >
> > > > > > > > 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.
> > > > >
> > > > > 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?
> > >
> > > 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 :-)
> >
> > >
> > > - 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.
> >
> > 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.
> 
> Yes, it makes sense, but please describe it in the changelog more
> clearly.

Got it. Thanks.

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

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.

> 
> 
> Takashi


Regards,
Libin
Takashi Iwai April 7, 2015, 6:24 a.m. UTC | #18
At Tue, 7 Apr 2015 06:12:00 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Saturday, April 04, 2015 6:44 PM
> > > > To: Yang, Libin
> > > > Cc: 'alsa-devel@alsa-project.org'
> > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > AZX_DCAPS_I915_POWERWELL for skl
> > > >
> > > > At Mon, 30 Mar 2015 02:47:45 +0000,
> > > > Yang, Libin wrote:
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Yang, Libin
> > > > > > Sent: Friday, March 27, 2015 4:34 PM
> > > > > > To: Takashi Iwai
> > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > >
> > > > > > Hi Takashi,
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > Sent: Friday, March 27, 2015 4:30 PM
> > > > > > > To: Yang, Libin
> > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > >
> > > > > > > At Fri, 27 Mar 2015 08:25:52 +0000,
> > > > > > > Yang, Libin wrote:
> > > > > > > >
> > > > > > > > Hi Takashi,
> > > > > > > >
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: alsa-devel-bounces@alsa-project.org [mailto:alsa-
> > > > devel-
> > > > > > > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > > > > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > > > > > > To: Yang, Libin
> > > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > > >
> > > > > > > > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > > > > > > > Yang, Libin wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Takashi,
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > > > > > > To: Yang, Libin
> > > > > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > > > > >
> > > > > > > > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > > > > > > > libin.yang@intel.com wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > > > > > > > >
> > > > > > > > > > > > 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 <libin.yang@intel.com>
> > > > > > > > > > >
> > > > > > > > > > > So, was the previous question clarified?
> > > > > > > > > >
> > > > > > > > > > Yes, I have confirmed with our silicon team.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > 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.
> > > > > >
> > > > > > 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?
> > > >
> > > > 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 :-)
> > >
> > > >
> > > > - 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.
> > >
> > > 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.
> > 
> > Yes, it makes sense, but please describe it in the changelog more
> > clearly.
> 
> Got it. Thanks.
> 
> > 
> > 
> > > > 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.
> 
> 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.

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
Yang, Libin April 7, 2015, 6:40 a.m. UTC | #19
Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, April 07, 2015 2:24 PM
> To: Yang, Libin
> Cc: 'alsa-devel@alsa-project.org'
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> AZX_DCAPS_I915_POWERWELL for skl
> 
> At Tue, 7 Apr 2015 06:12:00 +0000,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Saturday, April 04, 2015 6:44 PM
> > > > > To: Yang, Libin
> > > > > Cc: 'alsa-devel@alsa-project.org'
> > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > >
> > > > > At Mon, 30 Mar 2015 02:47:45 +0000,
> > > > > Yang, Libin wrote:
> > > > > >
> > > > > > Hi Takashi,
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Yang, Libin
> > > > > > > Sent: Friday, March 27, 2015 4:34 PM
> > > > > > > To: Takashi Iwai
> > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > Subject: RE: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > >
> > > > > > > Hi Takashi,
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > Sent: Friday, March 27, 2015 4:30 PM
> > > > > > > > To: Yang, Libin
> > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > >
> > > > > > > > At Fri, 27 Mar 2015 08:25:52 +0000,
> > > > > > > > Yang, Libin wrote:
> > > > > > > > >
> > > > > > > > > Hi Takashi,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-
> > > > > devel-
> > > > > > > > > > bounces@alsa-project.org] On Behalf Of Takashi Iwai
> > > > > > > > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > > > > > > > To: Yang, Libin
> > > > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > > > Subject: Re: [alsa-devel] [PATCH] ALSA: hda_intel: add
> > > > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > > > >
> > > > > > > > > > At Fri, 27 Mar 2015 08:02:54 +0000,
> > > > > > > > > > Yang, Libin wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Takashi,
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > > > > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > > > > > > > To: Yang, Libin
> > > > > > > > > > > > Cc: alsa-devel@alsa-project.org
> > > > > > > > > > > > Subject: Re: [PATCH] ALSA: hda_intel: add
> > > > > > > > > > > > AZX_DCAPS_I915_POWERWELL for skl
> > > > > > > > > > > >
> > > > > > > > > > > > At Fri, 27 Mar 2015 15:10:04 +0800,
> > > > > > > > > > > > libin.yang@intel.com wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > From: Libin Yang <libin.yang@intel.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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 <libin.yang@intel.com>
> > > > > > > > > > > >
> > > > > > > > > > > > So, was the previous question clarified?
> > > > > > > > > > >
> > > > > > > > > > > Yes, I have confirmed with our silicon team.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > 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.
> > > > > > >
> > > > > > > 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?
> > > > >
> > > > > 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 :-)
> > > >
> > > > >
> > > > > - 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.
> > > >
> > > > 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.
> > >
> > > Yes, it makes sense, but please describe it in the changelog more
> > > clearly.
> >
> > Got it. Thanks.
> >
> > >
> > >
> > > > > 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.
> >
> > 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.
> 
> 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.

Got it. Thanks a lot.

> 
> 
> Takashi


Regards,
Libin
diff mbox

Patch

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 15a8299..4744a96 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -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 \