[alsa-devel] [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec

[Takashi Iwai]

At Tue, 7 May 2013 14:10:08 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, May 07, 2013 7:21 PM
> > To: Lin, Mengdong
> > Cc: alsa-devel at alsa-project.org
> > Subject: Re: [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi
> > codec
> > 
> > At Tue,  7 May 2013 12:34:21 -0400,
> > mengdong.lin at intel.com wrote:
> > >
> > > From: Mengdong Lin <mengdong.lin at intel.com>
> > >
> > > This patch is a supplement to a previous patch:
> > > "ALSA: hda - Add fixup for Haswell to enable all pin and convertor widgets".
> > >
> > > Some Haswell BIOS will disable the 2nd and 3rd pin/covertor widgets
> > > when the HD-A controller changes state from D3 to D0. So when the
> > > controller resumes after a system or runtime suspend, these widgets
> > > are disabled and programming these widgets to D0 will cause H/W error and
> > codec will not respond.
> > >
> > > So this patch adds a "set_power_state" ops for Haswell codec. Before
> > > setting the codec to D0, this function will apply a BIOS-specific
> > > fixup to enable the 2nd & 3rd pins/cvts if need.
> > >
> > > And since BIOS will also disable DP1.2, so this function will check
> > > and enable
> > > DP1.2 mode if need.
> > >
> > > Signed-off-by: Mengdong Lin <mengdong.lin at intel.com>
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > index adb5dce..080398e 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -1842,7 +1842,7 @@ static void intel_haswell_enable_all_pins(struct
> > > hda_codec *codec,  {
> > >  	unsigned int vendor_param;
> > >
> > > -	if (action != HDA_FIXUP_ACT_PRE_PROBE)
> > > +	if (action != HDA_FIXUP_ACT_PRE_PROBE && action !=
> > > +HDA_FIXUP_ACT_INIT)
> > >  		return;
> > >  	vendor_param = snd_hda_codec_read(codec, INTEL_VENDOR_NID, 0,
> > >  				INTEL_GET_VENDOR_VERB, 0);
> > > @@ -1855,7 +1855,8 @@ static void intel_haswell_enable_all_pins(struct
> > hda_codec *codec,
> > >  	if (vendor_param == -1)
> > >  		return;
> > >
> > > -	snd_hda_codec_update_widgets(codec);
> > > +	if (action == HDA_FIXUP_ACT_PRE_PROBE)
> > > +		snd_hda_codec_update_widgets(codec);
> > >  	return;
> > >  }
> > >
> > > @@ -1874,7 +1875,24 @@ static void
> > intel_haswell_fixup_enable_dp12(struct hda_codec *codec)
> > >  				INTEL_SET_VENDOR_VERB, vendor_param);  }
> > >
> > > +static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
> > > +				unsigned int power_state)
> > > +{
> > > +	if (AC_PWRST_D0 == power_state) {
> > > +		snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_INIT);
> > 
> > Here is no right place to apply the fixup.  If needed, call it generic_hdmi_init(),
> > for example.
> 
> Hi Takashi,
> 
> These widgets need to be enabled before hda_set_power_state(codec, AC_PWRST_D0) programs these widgets to D0.
> Otherwise, audio driver would program these disabled widgets to D0. The codec will not respond when audio driver tries to sync power state. And later verb execution will fail.

Hm, but this is needed only for the machines with PCI SSID 8086:2010,
right?   If so, this will be never in market, and I wonder the
importance of this fix.

So, the question is -- in which situation do we need this fix at all?
Isn't it needed for all Haswell, or only certain Haswell variants, or
only certain setups?

> In hda_call_codec_resume(), generic_hdmi_init() is called from codec->patch_ops.init, after setting power state to D0. It would be too late.
> Is it okay to add a new ops like "pre_resume" to apply the fixup?

No.  Using the standard fixup doesn't look correct in this case.


Takashi