* Re: [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec
2013-05-07 16:34 [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec mengdong.lin
@ 2013-05-07 11:21 ` Takashi Iwai
2013-05-07 14:10 ` Lin, Mengdong
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2013-05-07 11:21 UTC (permalink / raw)
To: mengdong.lin; +Cc: alsa-devel
At Tue, 7 May 2013 12:34:21 -0400,
mengdong.lin@intel.com wrote:
>
> From: Mengdong Lin <mengdong.lin@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@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.
Takashi
> +
> + intel_haswell_fixup_enable_dp12(codec);
> +
> + }
> +
> + snd_hda_codec_read(codec, fg, 0,
> + AC_VERB_SET_POWER_STATE,
> + power_state);
>
> + snd_hda_codec_set_power_to_all(codec, fg, power_state);
> +
> + return;
> +}
>
> /* available models for fixup */
> enum {
> @@ -1922,6 +1940,10 @@ static int patch_generic_hdmi(struct hda_codec *codec)
> return -EINVAL;
> }
> codec->patch_ops = generic_hdmi_patch_ops;
> +
> + if (codec->vendor_id == 0x80862807)
> + codec->patch_ops.set_power_state = haswell_set_power_state;
> +
> generic_hdmi_init_per_pins(codec);
>
> init_channel_allocations();
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec
2013-05-07 11:21 ` Takashi Iwai
@ 2013-05-07 14:10 ` Lin, Mengdong
2013-05-07 14:15 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Lin, Mengdong @ 2013-05-07 14:10 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, May 07, 2013 7:21 PM
> To: Lin, Mengdong
> Cc: alsa-devel@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@intel.com wrote:
> >
> > From: Mengdong Lin <mengdong.lin@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@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.
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?
Thanks
Mengdong
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec
2013-05-07 14:10 ` Lin, Mengdong
@ 2013-05-07 14:15 ` Takashi Iwai
2013-05-07 15:33 ` Lin, Mengdong
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2013-05-07 14:15 UTC (permalink / raw)
To: Lin, Mengdong; +Cc: alsa-devel
At Tue, 7 May 2013 14:10:08 +0000,
Lin, Mengdong wrote:
>
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, May 07, 2013 7:21 PM
> > To: Lin, Mengdong
> > Cc: alsa-devel@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@intel.com wrote:
> > >
> > > From: Mengdong Lin <mengdong.lin@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@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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec
2013-05-07 14:15 ` Takashi Iwai
@ 2013-05-07 15:33 ` Lin, Mengdong
2013-05-07 15:39 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Lin, Mengdong @ 2013-05-07 15:33 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, May 07, 2013 10:16 PM
> > 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?
This fixup is needed for all Intel Haswell test machines on our hand, including desktop, mobile and ultrabook machines.
All of these machines has both HDMI and DP connectors and so we need to enable these widgets to use HDMI on the 2nd pin.
Without this fixup, we observed communication failure between controller and codec after a system suspend/resume.
The BIOS team told us they will no longer change BIOS for Haswell, and they expect the Linux audio driver to enable the widgets, just like Windows driver does.
Now we hope to have a BIOS specific fixup.
Other OEM BIOS may enable the these widgets according to their machine design, or may not if they follow Intel BIOS. This need to be check case by case.
Thanks
Mengdong
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec
2013-05-07 15:33 ` Lin, Mengdong
@ 2013-05-07 15:39 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2013-05-07 15:39 UTC (permalink / raw)
To: Lin, Mengdong; +Cc: alsa-devel
At Tue, 7 May 2013 15:33:34 +0000,
Lin, Mengdong wrote:
>
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, May 07, 2013 10:16 PM
>
> > > 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?
>
> This fixup is needed for all Intel Haswell test machines on our hand, including desktop, mobile and ultrabook machines.
But obviously this doesn't match with other vendors (e.g. HP).
> All of these machines has both HDMI and DP connectors and so we need to enable these widgets to use HDMI on the 2nd pin.
> Without this fixup, we observed communication failure between controller and codec after a system suspend/resume.
... and I still see the problem with other vendors. So I suspect the
fix may be really a band-aid over Intel-devel machines but doesn't
help others in the current form.
> The BIOS team told us they will no longer change BIOS for Haswell, and they expect the Linux audio driver to enable the widgets, just like Windows driver does.
> Now we hope to have a BIOS specific fixup.
>
> Other OEM BIOS may enable the these widgets according to their machine design, or may not if they follow Intel BIOS. This need to be check case by case.
Hmm... your statement worries me pretty much. It sounds like that the
driver won't work as is unless we know what BIOS on each machine does
and how to paper over it. Can't we have a more generic solution?
Takashi
>
> Thanks
> Mengdong
>
> > > 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec
@ 2013-05-07 16:34 mengdong.lin
2013-05-07 11:21 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: mengdong.lin @ 2013-05-07 16:34 UTC (permalink / raw)
To: alsa-devel, tiwai; +Cc: Mengdong Lin
From: Mengdong Lin <mengdong.lin@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@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);
+
+ intel_haswell_fixup_enable_dp12(codec);
+
+ }
+
+ snd_hda_codec_read(codec, fg, 0,
+ AC_VERB_SET_POWER_STATE,
+ power_state);
+ snd_hda_codec_set_power_to_all(codec, fg, power_state);
+
+ return;
+}
/* available models for fixup */
enum {
@@ -1922,6 +1940,10 @@ static int patch_generic_hdmi(struct hda_codec *codec)
return -EINVAL;
}
codec->patch_ops = generic_hdmi_patch_ops;
+
+ if (codec->vendor_id == 0x80862807)
+ codec->patch_ops.set_power_state = haswell_set_power_state;
+
generic_hdmi_init_per_pins(codec);
init_channel_allocations();
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-07 15:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-07 16:34 [PATCH v2] ALSA: hda - re-apply fixup when resuming haswell hdmi codec mengdong.lin
2013-05-07 11:21 ` Takashi Iwai
2013-05-07 14:10 ` Lin, Mengdong
2013-05-07 14:15 ` Takashi Iwai
2013-05-07 15:33 ` Lin, Mengdong
2013-05-07 15:39 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.