[alsa-devel] [PATCH] ALSA: hda_intel: add AZX_DCAPS_I915_POWERWELL for skl

Takashi Iwai tiwai at suse.de
Tue Apr 7 08:24:00 CEST 2015 

At Tue, 7 Apr 2015 06:12:00 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai at suse.de]
> > > > Sent: Saturday, April 04, 2015 6:44 PM
> > > > To: Yang, Libin
> > > > Cc: 'alsa-devel at 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 at 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 at suse.de]
> > > > > > > Sent: Friday, March 27, 2015 4:30 PM
> > > > > > > To: Yang, Libin
> > > > > > > Cc: alsa-devel at 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 at alsa-project.org [mailto:alsa-
> > > > devel-
> > > > > > > > > bounces at alsa-project.org] On Behalf Of Takashi Iwai
> > > > > > > > > Sent: Friday, March 27, 2015 4:19 PM
> > > > > > > > > To: Yang, Libin
> > > > > > > > > Cc: alsa-devel at 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 at suse.de]
> > > > > > > > > > > Sent: Friday, March 27, 2015 3:57 PM
> > > > > > > > > > > To: Yang, Libin
> > > > > > > > > > > Cc: alsa-devel at 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 at intel.com wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > From: Libin Yang <libin.yang at 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 at 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

More information about the Alsa-devel mailing list