Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

Is it okay to add a new parameter to snd_hda_resume(), to indicate whether codec resume should be delayed or not?
Is because snd_hda_resume() is only only called by azx_resume() but also called by azx_bus_reset(). 
The latter can be called on a fatal verb execution failure and codec resume delay is not suitable for this case.

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 03/26/2013 07:12 PM, mengdong.lin@intel.com wrote:
> From: Mengdong Lin <mengdong.lin@intel.com>
>
> In system resume, Haswell codec cannot be programmed to D0 before Gfx driver
> initializes the display pipeline and audio, which will trigger an unsol event
> on the pin with HDMI/DP cable connected. Otherwise, the connected pin will
> stay in D3 with right channel muted and thus no sound can be heard.

Thanks for this patch!

Just a question: Is there a difference between real system S3 here, and 
just the normal idle power down? I e, I'm a little unsure myself, but it 
seems like this 300 ms delay would trigger in both cases, causing an 
(unnecessary?) 300 ms delay every time the codec is idle and you want to 
start playback. Is this correct, or did I get something wrong?


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

[PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

In system resume, Haswell codec cannot be programmed to D0 before Gfx driver
initializes the display pipeline and audio, which will trigger an unsol event
on the pin with HDMI/DP cable connected. Otherwise, the connected pin will
stay in D3 with right channel muted and thus no sound can be heard.

This patch
- adds a codec flag to delay resuming a codec. System resume will skip the
  codecs if this flag is set, and these codecs will be resumed on later codec
  access.
- adds a set_power_state ops for Haswell HDMI codec. In a delayed resume, this
  ops will enable and wait for the unsol event, and then resume the codec. A
  300ms timeout is set in case unsol event is lost.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 04b5738..bcb7205 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -5508,11 +5508,15 @@ int snd_hda_resume(struct hda_bus *bus)
 	struct hda_codec *codec;
 
 	list_for_each_entry(codec, &bus->codec_list, list) {
-		hda_call_codec_resume(codec);
+		if (codec->support_delay_resume)
+			codec->resume_delayed = 1;
+		else
+			hda_call_codec_resume(codec);
 	}
 	return 0;
 }
 EXPORT_SYMBOL_HDA(snd_hda_resume);
+
 #endif /* CONFIG_PM */
 
 /*
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 23ca172..5b5e5f4 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -886,6 +886,8 @@ struct hda_codec {
 	unsigned int d3_stop_clk:1;	/* support D3 operation without BCLK */
 	unsigned int pm_down_notified:1; /* PM notified to controller */
 	unsigned int in_pm:1;		/* suspend/resume being performed */
+	unsigned int support_delay_resume:1; /* codec support delay resume */
+	unsigned int resume_delayed:1; /* resume delayed by PM */
 	int power_transition;	/* power-state in transition */
 	int power_count;	/* current (global) power refcount */
 	struct delayed_work power_work; /* delayed task for powerdown */
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 78e1827..d116908 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -98,6 +98,14 @@ struct hdmi_spec {
 	 */
 	struct hda_multi_out multiout;
 	struct hda_pcm_stream pcm_playback;
+
+#ifdef CONFIG_PM
+	/*
+	 * Non-generic Intel Haswell specific
+	 */
+	unsigned int ready_to_resume:1;
+	wait_queue_head_t resume_wq;
+#endif
 };
 
 
@@ -977,6 +985,13 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 	if (pin_idx < 0)
 		return;
 
+#ifdef CONFIG_PM
+	if (codec->resume_delayed) {
+		spec->ready_to_resume = 1;
+		wake_up(&spec->resume_wq);
+	}
+#endif
+
 	hdmi_present_sense(&spec->pins[pin_idx], 1);
 	snd_hda_jack_report_sync(codec);
 }
@@ -1846,6 +1861,63 @@ static const struct hda_fixup hdmi_fixups[] = {
 };
 
 
+#ifdef CONFIG_PM
+static void intel_haswell_wait_ready_to_resume(struct hda_codec *codec)
+{
+	struct hdmi_spec *spec = codec->spec;
+	int pin_idx;
+
+	spec->ready_to_resume = 0;
+
+	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+		struct hdmi_spec_per_pin *per_pin;
+		hda_nid_t pin_nid;
+		struct hda_jack_tbl *jack;
+
+		per_pin = &spec->pins[pin_idx];
+		pin_nid = per_pin->pin_nid;
+		jack = snd_hda_jack_tbl_get(codec, pin_nid);
+		if (jack)
+			snd_hda_codec_write(codec, pin_nid, 0,
+				 AC_VERB_SET_UNSOLICITED_ENABLE,
+				 AC_USRSP_EN | jack->tag);
+	}
+
+	wait_event_timeout(spec->resume_wq,
+			spec->ready_to_resume, msecs_to_jiffies(300));
+	if (!spec->ready_to_resume)
+		snd_printd(KERN_WARNING "HDMI: Haswell not ready to resume\n");
+}
+
+static void intel_haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
+				 unsigned int power_state)
+{
+	if (codec->resume_delayed && power_state ==  AC_PWRST_D0) {
+		intel_haswell_wait_ready_to_resume(codec);
+		codec->resume_delayed = 0;
+	}
+
+	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);
+}
+
+static inline void intel_haswell_allow_delay_resume(struct hda_codec *codec)
+{
+	struct hdmi_spec *spec = codec->spec;
+
+	init_waitqueue_head(&spec->resume_wq);
+	codec->support_delay_resume = 1;
+
+	codec->patch_ops.set_power_state =
+				intel_haswell_set_power_state;
+}
+#else
+define intel_haswell_allow_delay_resume NULL
+#endif
+
 static int patch_generic_hdmi(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec;
@@ -1868,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)
+		intel_haswell_allow_delay_resume(codec);
+
 	generic_hdmi_init_per_pins(codec);
 
 	init_channel_allocations();
-- 
1.7.10.4

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson
> Sent: Tuesday, March 26, 2013 6:58 PM


> Just a question: Is there a difference between real system S3 here, and just the
> normal idle power down? I e, I'm a little unsure myself, but it seems like this 300
> ms delay would trigger in both cases, causing an
> (unnecessary?) 300 ms delay every time the codec is idle and you want to start
> playback. Is this correct, or did I get something wrong?


Hi David,

Thanks for your feedback!
There will not be 300ms delay every time in normal idle power down case.

Only the system resume will delay resuming a codec and cause a waiting on first codec access (eg. 1st playback ) after a system resume. Snd_hda_resume(), called by azx_resume(), will check whether a codec can support delay resume, skip these codecs and set the flag "resume_delayed" on them. Normal idle power down will not touch this flag. 

The Haswell codec set_power_state ops (intel_haswell_set_power_state) will only wait if this is a delayed resume and clear this flag after waiting. And actually, there is no waiting even in this case. Because when 1st user operation after system resume happens, Gfx already finishes resuming and audio initialization, so as long as intel_haswell_wait_ready_to_resume() enable the unsol event, the unsol event comes and so so waiting finishes. The 300ms time out is set for safety consideration in case unsol event is lost. I've not observed any unsol event lost till now.

BTW, would you please tell us how Ubuntu decides whether to enable the normal idle power down (parameter "power_save")?
We found this bug on a Haswell mobile machine with "power_save" disabled, which means only system resume will program the codec back to D0. But on a another machine with "power_save" enabled, this bug is not visible because later runtime codec power up can program the codec to D0 and unmute the pin again.

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 03/27/2013 03:03 AM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: alsa-devel-bounces@alsa-project.org
>> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson
>> Sent: Tuesday, March 26, 2013 6:58 PM
>
>
>> Just a question: Is there a difference between real system S3 here, and just the
>> normal idle power down? I e, I'm a little unsure myself, but it seems like this 300
>> ms delay would trigger in both cases, causing an
>> (unnecessary?) 300 ms delay every time the codec is idle and you want to start
>> playback. Is this correct, or did I get something wrong?
>
>
> Hi David,
>
> Thanks for your feedback!
> There will not be 300ms delay every time in normal idle power down case.
>
> Only the system resume will delay resuming a codec and cause a waiting on first codec access (eg. 1st playback ) after a system resume. Snd_hda_resume(), called by azx_resume(), will check whether a codec can support delay resume, skip these codecs and set the flag "resume_delayed" on them. Normal idle power down will not touch this flag.

Right.

> The Haswell codec set_power_state ops (intel_haswell_set_power_state) will only wait if this is a delayed resume and clear this flag after waiting. And actually, there is no waiting even in this case. Because when 1st user operation after system resume happens, Gfx already finishes resuming and audio initialization, so as long as intel_haswell_wait_ready_to_resume() enable the unsol event, the unsol event comes and so so waiting finishes. The 300ms time out is set for safety consideration in case unsol event is lost. I've not observed any unsol event lost till now.

As for the timeout, I suggest you use the codec->bus->workq instead of 
creating a new workq. I think that will also give us some serialisation, 
i e, protection against race conditions if the timeout happen at the 
same time as the unsol event.


> BTW, would you please tell us how Ubuntu decides whether to enable the normal idle power down (parameter "power_save")?
> We found this bug on a Haswell mobile machine with "power_save" disabled, which means only system resume will program the codec back to D0. But on a another machine with "power_save" enabled, this bug is not visible because later runtime codec power up can program the codec to D0 and unmute the pin again.

Hmm. I don't think there is any difference between the upstream default 
and Ubuntu in this case. I remember we had to turn off power_save for 
Pantherpoint once, before keep_power_link_on for PantherPoint was 
discovered and upstreamed. But that was quite a while ago.

Then there is of course people playing with powertop and other tools, to 
override the defaults.


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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Wednesday, March 27, 2013 4:19 PM

> > The Haswell codec set_power_state ops (intel_haswell_set_power_state) will
> only wait if this is a delayed resume and clear this flag after waiting. And
> actually, there is no waiting even in this case. Because when 1st user operation
> after system resume happens, Gfx already finishes resuming and audio
> initialization, so as long as intel_haswell_wait_ready_to_resume() enable the
> unsol event, the unsol event comes and so so waiting finishes. The 300ms time
> out is set for safety consideration in case unsol event is lost. I've not observed
> any unsol event lost till now.
> 
> As for the timeout, I suggest you use the codec->bus->workq instead of
> creating a new workq. I think that will also give us some serialisation, i e,
> protection against race conditions if the timeout happen at the same time as
> the unsol event.

Hi David,

The new added "resume_wq" for hdmi codec is a wait queue, not a work queue like codec->bus->workq.
It's expected to wake up as soon as the unsol event is got.

And I think there will not be race between unsol event and idle time out. 
Because until the first user operation trigger the delayed resume, the codec keeps in D3.
And only until intel_haswell_set_power_state(), which is called by hda_call_codec_resume(), finishes waiting and programing the codec to D0,
__snd_hda_power_up() will keep 'codec->power_transition' to '1'. Thus __snd_hda_power_down() will not schedule idle time out.


> > BTW, would you please tell us how Ubuntu decides whether to enable the
> normal idle power down (parameter "power_save")?
> > We found this bug on a Haswell mobile machine with "power_save" disabled,
> which means only system resume will program the codec back to D0. But on a
> another machine with "power_save" enabled, this bug is not visible because
> later runtime codec power up can program the codec to D0 and unmute the pin
> again.
> 
> Hmm. I don't think there is any difference between the upstream default and
> Ubuntu in this case. I remember we had to turn off power_save for
> Pantherpoint once, before keep_power_link_on for PantherPoint was
> discovered and upstreamed. But that was quite a while ago.
> 
> Then there is of course people playing with powertop and other tools, to
> override the defaults.

The default power_value is 0 in Kconfig, which means off.
But on some HSW machines with ALC889 codec and Haswell display codec, both with EPSS support, I found that power_save is 1 after boot.
On another HSW machine with ALC262, no EPSS, power_save is 0.

So I suspect the rule is that if all codecs support EPSS, power_save will be enabled. But who enables this?

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 26 Mar 2013 07:02:54 +0000,
Lin, Mengdong wrote:
> 
> Is it okay to add a new parameter to snd_hda_resume(), to indicate whether codec resume should be delayed or not?
> Is because snd_hda_resume() is only only called by azx_resume() but also called by azx_bus_reset(). 
> The latter can be called on a fatal verb execution failure and codec resume delay is not suitable for this case.

Well, I prefer creating a new function (e.g. snd_hda_bus_reset())
called from azx_bus_reset().


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 03/27/2013 11:01 AM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Wednesday, March 27, 2013 4:19 PM
>
>>> The Haswell codec set_power_state ops (intel_haswell_set_power_state) will
>> only wait if this is a delayed resume and clear this flag after waiting. And
>> actually, there is no waiting even in this case. Because when 1st user operation
>> after system resume happens, Gfx already finishes resuming and audio
>> initialization, so as long as intel_haswell_wait_ready_to_resume() enable the
>> unsol event, the unsol event comes and so so waiting finishes. The 300ms time
>> out is set for safety consideration in case unsol event is lost. I've not observed
>> any unsol event lost till now.
>>
>> As for the timeout, I suggest you use the codec->bus->workq instead of
>> creating a new workq. I think that will also give us some serialisation, i e,
>> protection against race conditions if the timeout happen at the same time as
>> the unsol event.
>
> Hi David,
>
> The new added "resume_wq" for hdmi codec is a wait queue, not a work queue like codec->bus->workq.
> It's expected to wake up as soon as the unsol event is got.

Sure; but I don't see why you need a wait queue for that? Why don't you 
just call the resume path from the unsol event handler 
(hdmi_present_sense, or its caller), and then also cancel the timeout 
handler (which can then be in the normal workq)?


> And I think there will not be race between unsol event and idle time out.
> Because until the first user operation trigger the delayed resume, the codec keeps in D3.
> And only until intel_haswell_set_power_state(), which is called by hda_call_codec_resume(), finishes waiting and programing the codec to D0,
> __snd_hda_power_up() will keep 'codec->power_transition' to '1'. Thus __snd_hda_power_down() will not schedule idle time out.
>
>
>>> BTW, would you please tell us how Ubuntu decides whether to enable the
>> normal idle power down (parameter "power_save")?
>>> We found this bug on a Haswell mobile machine with "power_save" disabled,
>> which means only system resume will program the codec back to D0. But on a
>> another machine with "power_save" enabled, this bug is not visible because
>> later runtime codec power up can program the codec to D0 and unmute the pin
>> again.
>>
>> Hmm. I don't think there is any difference between the upstream default and
>> Ubuntu in this case. I remember we had to turn off power_save for
>> Pantherpoint once, before keep_power_link_on for PantherPoint was
>> discovered and upstreamed. But that was quite a while ago.
>>
>> Then there is of course people playing with powertop and other tools, to
>> override the defaults.
>
> The default power_value is 0 in Kconfig, which means off.
> But on some HSW machines with ALC889 codec and Haswell display codec, both with EPSS support, I found that power_save is 1 after boot.
> On another HSW machine with ALC262, no EPSS, power_save is 0.
>
> So I suspect the rule is that if all codecs support EPSS, power_save will be enabled. But who enables this?

That does not seem to make sense. It might be something different?


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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 02 Apr 2013 13:53:16 +0200,
David Henningsson wrote:
> 
> On 03/27/2013 11:01 AM, Lin, Mengdong wrote:
> >> -----Original Message-----
> >> From: David Henningsson [mailto:david.henningsson@canonical.com]
> >> Sent: Wednesday, March 27, 2013 4:19 PM
> >
> >>> The Haswell codec set_power_state ops (intel_haswell_set_power_state) will
> >> only wait if this is a delayed resume and clear this flag after waiting. And
> >> actually, there is no waiting even in this case. Because when 1st user operation
> >> after system resume happens, Gfx already finishes resuming and audio
> >> initialization, so as long as intel_haswell_wait_ready_to_resume() enable the
> >> unsol event, the unsol event comes and so so waiting finishes. The 300ms time
> >> out is set for safety consideration in case unsol event is lost. I've not observed
> >> any unsol event lost till now.
> >>
> >> As for the timeout, I suggest you use the codec->bus->workq instead of
> >> creating a new workq. I think that will also give us some serialisation, i e,
> >> protection against race conditions if the timeout happen at the same time as
> >> the unsol event.
> >
> > Hi David,
> >
> > The new added "resume_wq" for hdmi codec is a wait queue, not a work queue like codec->bus->workq.
> > It's expected to wake up as soon as the unsol event is got.
> 
> Sure; but I don't see why you need a wait queue for that? Why don't you 
> just call the resume path from the unsol event handler 
> (hdmi_present_sense, or its caller), and then also cancel the timeout 
> handler (which can then be in the normal workq)?

Because the delayed resume actually fakes as if the resume is done.
This is necessary not to block other device's resume operation.

Since it looks as if ready, user-space might restart things soon again
before the delayed resume is really finished.  So, some serialization
is required there.


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/02/2013 02:18 PM, Takashi Iwai wrote:
> At Tue, 02 Apr 2013 13:53:16 +0200,
> David Henningsson wrote:
>>
>> On 03/27/2013 11:01 AM, Lin, Mengdong wrote:
>>>> -----Original Message-----
>>>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>>>> Sent: Wednesday, March 27, 2013 4:19 PM
>>>
>>>>> The Haswell codec set_power_state ops (intel_haswell_set_power_state) will
>>>> only wait if this is a delayed resume and clear this flag after waiting. And
>>>> actually, there is no waiting even in this case. Because when 1st user operation
>>>> after system resume happens, Gfx already finishes resuming and audio
>>>> initialization, so as long as intel_haswell_wait_ready_to_resume() enable the
>>>> unsol event, the unsol event comes and so so waiting finishes. The 300ms time
>>>> out is set for safety consideration in case unsol event is lost. I've not observed
>>>> any unsol event lost till now.
>>>>
>>>> As for the timeout, I suggest you use the codec->bus->workq instead of
>>>> creating a new workq. I think that will also give us some serialisation, i e,
>>>> protection against race conditions if the timeout happen at the same time as
>>>> the unsol event.
>>>
>>> Hi David,
>>>
>>> The new added "resume_wq" for hdmi codec is a wait queue, not a work queue like codec->bus->workq.
>>> It's expected to wake up as soon as the unsol event is got.
>>
>> Sure; but I don't see why you need a wait queue for that? Why don't you
>> just call the resume path from the unsol event handler
>> (hdmi_present_sense, or its caller), and then also cancel the timeout
>> handler (which can then be in the normal workq)?
>
> Because the delayed resume actually fakes as if the resume is done.
> This is necessary not to block other device's resume operation.
>
> Since it looks as if ready, user-space might restart things soon again
> before the delayed resume is really finished.  So, some serialization
> is required there.

Lin, would it be possible to add some chain of events description to the 
bug commit? The different contexts are just boggling my mind :-)

Assume we have two cases, system idle after S3 and immediate playback 
after S3.

Is this correct:

System idle:
  1. System skips hda_call_codec_resume and sets codec->resume_delayed.
  2. Since the codec is not reinitialized, nothing powers up the codec, 
so this is all that happens.
  3. Since unsol events are not enabled (?), we have a bug that jack 
detection does not work and cannot be detected from userspace...?

Immediate playback:
  1. System skips hda_call_codec_resume and sets codec->resume_delayed.
  2. Process context wants to start playback, which powers up the codec 
and calls intel_haswell_set_power_state.
  3. intel_haswell_wait_ready_to_resume enables unsol events and starts 
to wait on ready_to_resume
  4. Gfx init finishes, and workq context fires the unsol event, which 
calls hdmi_intrinsic_event, which triggers the resume_wq.
  5. Process context and workq context now continues in parallel, 
potentially (but hopefully not) leading to race conditions?


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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 02 Apr 2013 14:49:40 +0200,
David Henningsson wrote:
> 
> On 04/02/2013 02:18 PM, Takashi Iwai wrote:
> > At Tue, 02 Apr 2013 13:53:16 +0200,
> > David Henningsson wrote:
> >>
> >> On 03/27/2013 11:01 AM, Lin, Mengdong wrote:
> >>>> -----Original Message-----
> >>>> From: David Henningsson [mailto:david.henningsson@canonical.com]
> >>>> Sent: Wednesday, March 27, 2013 4:19 PM
> >>>
> >>>>> The Haswell codec set_power_state ops (intel_haswell_set_power_state) will
> >>>> only wait if this is a delayed resume and clear this flag after waiting. And
> >>>> actually, there is no waiting even in this case. Because when 1st user operation
> >>>> after system resume happens, Gfx already finishes resuming and audio
> >>>> initialization, so as long as intel_haswell_wait_ready_to_resume() enable the
> >>>> unsol event, the unsol event comes and so so waiting finishes. The 300ms time
> >>>> out is set for safety consideration in case unsol event is lost. I've not observed
> >>>> any unsol event lost till now.
> >>>>
> >>>> As for the timeout, I suggest you use the codec->bus->workq instead of
> >>>> creating a new workq. I think that will also give us some serialisation, i e,
> >>>> protection against race conditions if the timeout happen at the same time as
> >>>> the unsol event.
> >>>
> >>> Hi David,
> >>>
> >>> The new added "resume_wq" for hdmi codec is a wait queue, not a work queue like codec->bus->workq.
> >>> It's expected to wake up as soon as the unsol event is got.
> >>
> >> Sure; but I don't see why you need a wait queue for that? Why don't you
> >> just call the resume path from the unsol event handler
> >> (hdmi_present_sense, or its caller), and then also cancel the timeout
> >> handler (which can then be in the normal workq)?
> >
> > Because the delayed resume actually fakes as if the resume is done.
> > This is necessary not to block other device's resume operation.
> >
> > Since it looks as if ready, user-space might restart things soon again
> > before the delayed resume is really finished.  So, some serialization
> > is required there.
> 
> Lin, would it be possible to add some chain of events description to the 
> bug commit? The different contexts are just boggling my mind :-)

Yeah, a nice code flow picture would be helpful :)

> Assume we have two cases, system idle after S3 and immediate playback 
> after S3.
> 
> Is this correct:
> 
> System idle:
>   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
>   2. Since the codec is not reinitialized, nothing powers up the codec, 
> so this is all that happens.
>   3. Since unsol events are not enabled (?), we have a bug that jack 
> detection does not work and cannot be detected from userspace...?
> 
> Immediate playback:
>   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
>   2. Process context wants to start playback, which powers up the codec 
> and calls intel_haswell_set_power_state.
>   3. intel_haswell_wait_ready_to_resume enables unsol events and starts 
> to wait on ready_to_resume
>   4. Gfx init finishes, and workq context fires the unsol event, which 
> calls hdmi_intrinsic_event, which triggers the resume_wq.
>   5. Process context and workq context now continues in parallel, 
> potentially (but hopefully not) leading to race conditions?

The current patch has a small race, but it can be avoided by clearing
spec->ready_to_resume early enough, e.g. in suspend callback or in
init callback, like the patch below.


Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 67e11ba..d0eafee 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1740,6 +1740,9 @@ static int generic_hdmi_init(struct hda_codec *codec)
 		hdmi_init_pin(codec, pin_nid);
 		snd_hda_jack_detect_enable(codec, pin_nid, pin_nid);
 	}
+
+	spec->ready_to_resume = 0;
+
 	return 0;
 }
 
@@ -1876,8 +1879,6 @@ static void intel_haswell_wait_ready_to_resume(struct hda_codec *codec)
 	struct hdmi_spec *spec = codec->spec;
 	int pin_idx;
 
-	spec->ready_to_resume = 0;
-
 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
 		struct hdmi_spec_per_pin *per_pin;
 		hda_nid_t pin_nid;

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 26 Mar 2013 14:12:52 -0400,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> In system resume, Haswell codec cannot be programmed to D0 before Gfx driver
> initializes the display pipeline and audio, which will trigger an unsol event
> on the pin with HDMI/DP cable connected. Otherwise, the connected pin will
> stay in D3 with right channel muted and thus no sound can be heard.
> 
> This patch
> - adds a codec flag to delay resuming a codec. System resume will skip the
>   codecs if this flag is set, and these codecs will be resumed on later codec
>   access.
> - adds a set_power_state ops for Haswell HDMI codec. In a delayed resume, this
>   ops will enable and wait for the unsol event, and then resume the codec. A
>   300ms timeout is set in case unsol event is lost.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

I tried the patch on my HSW test machine, but this doesn't seem to
work well.  The machine shows the dead codec communication after S3.
And I hoped that the problem could be fixed by this, but it wasn't.

It seems that the graphics doesn't give any unsol event after S3.
And it doesn't power up correctly as well.  So still something is
wrong around graphics, maybe some initialization bits are missing.

FWIW, I tested the patch on vanilla 3.9-rc5+, with and without my 
for-linus/for-next branch merges, and also with Daniel's
drm-intel-next branch.

Which machine did you test it?


I don't mind to merge the revised patch, though, since it doesn't
break things (it's already broken), though.


thanks,

Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Fri, 05 Apr 2013 15:01:47 +0200,
Takashi Iwai wrote:
> 
> At Tue, 02 Apr 2013 14:49:40 +0200,
> David Henningsson wrote:
> > 
> > On 04/02/2013 02:18 PM, Takashi Iwai wrote:
> > > At Tue, 02 Apr 2013 13:53:16 +0200,
> > > David Henningsson wrote:
> > >>
> > >> On 03/27/2013 11:01 AM, Lin, Mengdong wrote:
> > >>>> -----Original Message-----
> > >>>> From: David Henningsson [mailto:david.henningsson@canonical.com]
> > >>>> Sent: Wednesday, March 27, 2013 4:19 PM
> > >>>
> > >>>>> The Haswell codec set_power_state ops (intel_haswell_set_power_state) will
> > >>>> only wait if this is a delayed resume and clear this flag after waiting. And
> > >>>> actually, there is no waiting even in this case. Because when 1st user operation
> > >>>> after system resume happens, Gfx already finishes resuming and audio
> > >>>> initialization, so as long as intel_haswell_wait_ready_to_resume() enable the
> > >>>> unsol event, the unsol event comes and so so waiting finishes. The 300ms time
> > >>>> out is set for safety consideration in case unsol event is lost. I've not observed
> > >>>> any unsol event lost till now.
> > >>>>
> > >>>> As for the timeout, I suggest you use the codec->bus->workq instead of
> > >>>> creating a new workq. I think that will also give us some serialisation, i e,
> > >>>> protection against race conditions if the timeout happen at the same time as
> > >>>> the unsol event.
> > >>>
> > >>> Hi David,
> > >>>
> > >>> The new added "resume_wq" for hdmi codec is a wait queue, not a work queue like codec->bus->workq.
> > >>> It's expected to wake up as soon as the unsol event is got.
> > >>
> > >> Sure; but I don't see why you need a wait queue for that? Why don't you
> > >> just call the resume path from the unsol event handler
> > >> (hdmi_present_sense, or its caller), and then also cancel the timeout
> > >> handler (which can then be in the normal workq)?
> > >
> > > Because the delayed resume actually fakes as if the resume is done.
> > > This is necessary not to block other device's resume operation.
> > >
> > > Since it looks as if ready, user-space might restart things soon again
> > > before the delayed resume is really finished.  So, some serialization
> > > is required there.
> > 
> > Lin, would it be possible to add some chain of events description to the 
> > bug commit? The different contexts are just boggling my mind :-)
> 
> Yeah, a nice code flow picture would be helpful :)
> 
> > Assume we have two cases, system idle after S3 and immediate playback 
> > after S3.
> > 
> > Is this correct:
> > 
> > System idle:
> >   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
> >   2. Since the codec is not reinitialized, nothing powers up the codec, 
> > so this is all that happens.
> >   3. Since unsol events are not enabled (?), we have a bug that jack 
> > detection does not work and cannot be detected from userspace...?
> > 
> > Immediate playback:
> >   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
> >   2. Process context wants to start playback, which powers up the codec 
> > and calls intel_haswell_set_power_state.
> >   3. intel_haswell_wait_ready_to_resume enables unsol events and starts 
> > to wait on ready_to_resume
> >   4. Gfx init finishes, and workq context fires the unsol event, which 
> > calls hdmi_intrinsic_event, which triggers the resume_wq.
> >   5. Process context and workq context now continues in parallel, 
> > potentially (but hopefully not) leading to race conditions?
> 
> The current patch has a small race, but it can be avoided by clearing
> spec->ready_to_resume early enough, e.g. in suspend callback or in
> init callback, like the patch below.
> 
> 
> Takashi
> 
> ---
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 67e11ba..d0eafee 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1740,6 +1740,9 @@ static int generic_hdmi_init(struct hda_codec *codec)
>  		hdmi_init_pin(codec, pin_nid);
>  		snd_hda_jack_detect_enable(codec, pin_nid, pin_nid);
>  	}
> +
> +	spec->ready_to_resume = 0;
> +

Here needs #ifdef CONFIG_PM, obviously...


Takashi

>  	return 0;
>  }
>  
> @@ -1876,8 +1879,6 @@ static void intel_haswell_wait_ready_to_resume(struct hda_codec *codec)
>  	struct hdmi_spec *spec = codec->spec;
>  	int pin_idx;
>  
> -	spec->ready_to_resume = 0;
> -
>  	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>  		struct hdmi_spec_per_pin *per_pin;
>  		hda_nid_t pin_nid;
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 26 Mar 2013 14:12:52 -0400,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> In system resume, Haswell codec cannot be programmed to D0 before Gfx driver
> initializes the display pipeline and audio, which will trigger an unsol event
> on the pin with HDMI/DP cable connected. Otherwise, the connected pin will
> stay in D3 with right channel muted and thus no sound can be heard.
> 
> This patch
> - adds a codec flag to delay resuming a codec. System resume will skip the
>   codecs if this flag is set, and these codecs will be resumed on later codec
>   access.
> - adds a set_power_state ops for Haswell HDMI codec. In a delayed resume, this
>   ops will enable and wait for the unsol event, and then resume the codec. A
>   300ms timeout is set in case unsol event is lost.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

Some other comments:

- Please rebase your patch to for-next branch of sound git tree.
  It can't be applied cleanly due to recent other changes.

- Fix some strange indentations and spaces (e.g. in
  intel_haswell_set_power_state)


thanks,

Takashi


> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 04b5738..bcb7205 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -5508,11 +5508,15 @@ int snd_hda_resume(struct hda_bus *bus)
>  	struct hda_codec *codec;
>  
>  	list_for_each_entry(codec, &bus->codec_list, list) {
> -		hda_call_codec_resume(codec);
> +		if (codec->support_delay_resume)
> +			codec->resume_delayed = 1;
> +		else
> +			hda_call_codec_resume(codec);
>  	}
>  	return 0;
>  }
>  EXPORT_SYMBOL_HDA(snd_hda_resume);
> +
>  #endif /* CONFIG_PM */
>  
>  /*
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index 23ca172..5b5e5f4 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -886,6 +886,8 @@ struct hda_codec {
>  	unsigned int d3_stop_clk:1;	/* support D3 operation without BCLK */
>  	unsigned int pm_down_notified:1; /* PM notified to controller */
>  	unsigned int in_pm:1;		/* suspend/resume being performed */
> +	unsigned int support_delay_resume:1; /* codec support delay resume */
> +	unsigned int resume_delayed:1; /* resume delayed by PM */
>  	int power_transition;	/* power-state in transition */
>  	int power_count;	/* current (global) power refcount */
>  	struct delayed_work power_work; /* delayed task for powerdown */
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 78e1827..d116908 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -98,6 +98,14 @@ struct hdmi_spec {
>  	 */
>  	struct hda_multi_out multiout;
>  	struct hda_pcm_stream pcm_playback;
> +
> +#ifdef CONFIG_PM
> +	/*
> +	 * Non-generic Intel Haswell specific
> +	 */
> +	unsigned int ready_to_resume:1;
> +	wait_queue_head_t resume_wq;
> +#endif
>  };
>  
>  
> @@ -977,6 +985,13 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>  	if (pin_idx < 0)
>  		return;
>  
> +#ifdef CONFIG_PM
> +	if (codec->resume_delayed) {
> +		spec->ready_to_resume = 1;
> +		wake_up(&spec->resume_wq);
> +	}
> +#endif
> +
>  	hdmi_present_sense(&spec->pins[pin_idx], 1);
>  	snd_hda_jack_report_sync(codec);
>  }
> @@ -1846,6 +1861,63 @@ static const struct hda_fixup hdmi_fixups[] = {
>  };
>  
>  
> +#ifdef CONFIG_PM
> +static void intel_haswell_wait_ready_to_resume(struct hda_codec *codec)
> +{
> +	struct hdmi_spec *spec = codec->spec;
> +	int pin_idx;
> +
> +	spec->ready_to_resume = 0;
> +
> +	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> +		struct hdmi_spec_per_pin *per_pin;
> +		hda_nid_t pin_nid;
> +		struct hda_jack_tbl *jack;
> +
> +		per_pin = &spec->pins[pin_idx];
> +		pin_nid = per_pin->pin_nid;
> +		jack = snd_hda_jack_tbl_get(codec, pin_nid);
> +		if (jack)
> +			snd_hda_codec_write(codec, pin_nid, 0,
> +				 AC_VERB_SET_UNSOLICITED_ENABLE,
> +				 AC_USRSP_EN | jack->tag);
> +	}
> +
> +	wait_event_timeout(spec->resume_wq,
> +			spec->ready_to_resume, msecs_to_jiffies(300));
> +	if (!spec->ready_to_resume)
> +		snd_printd(KERN_WARNING "HDMI: Haswell not ready to resume\n");
> +}
> +
> +static void intel_haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
> +				 unsigned int power_state)
> +{
> +	if (codec->resume_delayed && power_state ==  AC_PWRST_D0) {
> +		intel_haswell_wait_ready_to_resume(codec);
> +		codec->resume_delayed = 0;
> +	}
> +
> +	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);
> +}
> +
> +static inline void intel_haswell_allow_delay_resume(struct hda_codec *codec)
> +{
> +	struct hdmi_spec *spec = codec->spec;
> +
> +	init_waitqueue_head(&spec->resume_wq);
> +	codec->support_delay_resume = 1;
> +
> +	codec->patch_ops.set_power_state =
> +				intel_haswell_set_power_state;
> +}
> +#else
> +define intel_haswell_allow_delay_resume NULL
> +#endif
> +
>  static int patch_generic_hdmi(struct hda_codec *codec)
>  {
>  	struct hdmi_spec *spec;
> @@ -1868,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)
> +		intel_haswell_allow_delay_resume(codec);
> +
>  	generic_hdmi_init_per_pins(codec);
>  
>  	init_channel_allocations();
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

Hi Takashi and David,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, April 05, 2013 9:02 PM
> To: David Henningsson
> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> At Tue, 02 Apr 2013 14:49:40 +0200,
> David Henningsson wrote:
> >
> > On 04/02/2013 02:18 PM, Takashi Iwai wrote:
> > > At Tue, 02 Apr 2013 13:53:16 +0200,
> > > David Henningsson wrote:
> > >>
> > >> On 03/27/2013 11:01 AM, Lin, Mengdong wrote:
> > >>>> -----Original Message-----
> > >>>> From: David Henningsson [mailto:david.henningsson@canonical.com]
> > >>>> Sent: Wednesday, March 27, 2013 4:19 PM
> > >>>
> > >>>>> The Haswell codec set_power_state ops
> > >>>>> (intel_haswell_set_power_state) will
> > >>>> only wait if this is a delayed resume and clear this flag after
> > >>>> waiting. And actually, there is no waiting even in this case.
> > >>>> Because when 1st user operation after system resume happens, Gfx
> > >>>> already finishes resuming and audio initialization, so as long as
> > >>>> intel_haswell_wait_ready_to_resume() enable the unsol event, the
> > >>>> unsol event comes and so so waiting finishes. The 300ms time out
> > >>>> is set for safety consideration in case unsol event is lost. I've not
> observed any unsol event lost till now.
> > >>>>
> > >>>> As for the timeout, I suggest you use the codec->bus->workq
> > >>>> instead of creating a new workq. I think that will also give us
> > >>>> some serialisation, i e, protection against race conditions if
> > >>>> the timeout happen at the same time as the unsol event.
> > >>>
> > >>> Hi David,
> > >>>
> > >>> The new added "resume_wq" for hdmi codec is a wait queue, not a work
> queue like codec->bus->workq.
> > >>> It's expected to wake up as soon as the unsol event is got.
> > >>
> > >> Sure; but I don't see why you need a wait queue for that? Why don't
> > >> you just call the resume path from the unsol event handler
> > >> (hdmi_present_sense, or its caller), and then also cancel the
> > >> timeout handler (which can then be in the normal workq)?
> > >
> > > Because the delayed resume actually fakes as if the resume is done.
> > > This is necessary not to block other device's resume operation.
> > >
> > > Since it looks as if ready, user-space might restart things soon
> > > again before the delayed resume is really finished.  So, some
> > > serialization is required there.
> >
> > Lin, would it be possible to add some chain of events description to
> > the bug commit? The different contexts are just boggling my mind :-)
> 
> Yeah, a nice code flow picture would be helpful :)
> 
> > Assume we have two cases, system idle after S3 and immediate playback
> > after S3.
> >
> > Is this correct:
> >
> > System idle:
> >   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
> >   2. Since the codec is not reinitialized, nothing powers up the
> > codec, so this is all that happens.
> >   3. Since unsol events are not enabled (?), we have a bug that jack
> > detection does not work and cannot be detected from userspace...?

Yes. This is a side effect of this patch. The unsol event is delayed until the user operation triggers the delayed resume.
Then intel_haswell_wait_ready_to_resume() will enable unsol events.

> >
> > Immediate playback:
> >   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
> >   2. Process context wants to start playback, which powers up the
> > codec and calls intel_haswell_set_power_state.
> >   3. intel_haswell_wait_ready_to_resume enables unsol events and
> > starts to wait on ready_to_resume
> >   4. Gfx init finishes, and workq context fires the unsol event, which
> > calls hdmi_intrinsic_event, which triggers the resume_wq.
> >   5. Process context and workq context now continues in parallel,
> > potentially (but hopefully not) leading to race conditions?
> 
> The current patch has a small race, but it can be avoided by clearing
> spec->ready_to_resume early enough, e.g. in suspend callback or in
> init callback, like the patch below.
> 

I think there is no race for Haswell. In my test I observed that the unsol evnet will not be received
until intel_haswell_wait_ready_to_resume() enables the unsol event on the pins.
So I put "spec->ready_to_resume = 0" before enabling the unsol event. 

Thanks
Mengdong

> ---
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index
> 67e11ba..d0eafee 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1740,6 +1740,9 @@ static int generic_hdmi_init(struct hda_codec
> *codec)
>  		hdmi_init_pin(codec, pin_nid);
>  		snd_hda_jack_detect_enable(codec, pin_nid, pin_nid);
>  	}
> +
> +	spec->ready_to_resume = 0;
> +
>  	return 0;
>  }
> 
> @@ -1876,8 +1879,6 @@ static void
> intel_haswell_wait_ready_to_resume(struct hda_codec *codec)
>  	struct hdmi_spec *spec = codec->spec;
>  	int pin_idx;
> 
> -	spec->ready_to_resume = 0;
> -
>  	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>  		struct hdmi_spec_per_pin *per_pin;
>  		hda_nid_t pin_nid;

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, April 02, 2013 5:53 PM
> To: Lin, Mengdong
> Cc: alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> At Tue, 26 Mar 2013 07:02:54 +0000,
> Lin, Mengdong wrote:
> >
> > Is it okay to add a new parameter to snd_hda_resume(), to indicate whether
> codec resume should be delayed or not?
> > Is because snd_hda_resume() is only only called by azx_resume() but also
> called by azx_bus_reset().
> > The latter can be called on a fatal verb execution failure and codec resume
> delay is not suitable for this case.
> 
> Well, I prefer creating a new function (e.g. snd_hda_bus_reset()) called from
> azx_bus_reset().
> 
> 
> Takashi

Hi Takashi,

Do you mean that in azx_bus_reset(), we can merge the snd_hda_suspend() and snd_hda_resume() to a new function like snd_hda_bus_reset()?
snd_hda_suspend() and snd_hda_resume() look symmetrical here and so I don't want to destroy the symmetry.

static void azx_bus_reset(struct hda_bus *bus)
{
	...
#ifdef CONFIG_PM
	if (chip->initialized) {
		struct azx_pcm *p;
		list_for_each_entry(p, &chip->pcm_list, list)
			snd_pcm_suspend_all(p->pcm);
		snd_hda_suspend(chip->bus);
		snd_hda_resume(chip->bus);
	}
#endif
	bus->in_reset = 0;
}

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Fri, 5 Apr 2013 16:42:43 +0000,
Lin, Mengdong wrote:
> 
> Hi Takashi and David,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, April 05, 2013 9:02 PM
> > To: David Henningsson
> > Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> > in system resume
> > 
> > At Tue, 02 Apr 2013 14:49:40 +0200,
> > David Henningsson wrote:
> > >
> > > On 04/02/2013 02:18 PM, Takashi Iwai wrote:
> > > > At Tue, 02 Apr 2013 13:53:16 +0200,
> > > > David Henningsson wrote:
> > > >>
> > > >> On 03/27/2013 11:01 AM, Lin, Mengdong wrote:
> > > >>>> -----Original Message-----
> > > >>>> From: David Henningsson [mailto:david.henningsson@canonical.com]
> > > >>>> Sent: Wednesday, March 27, 2013 4:19 PM
> > > >>>
> > > >>>>> The Haswell codec set_power_state ops
> > > >>>>> (intel_haswell_set_power_state) will
> > > >>>> only wait if this is a delayed resume and clear this flag after
> > > >>>> waiting. And actually, there is no waiting even in this case.
> > > >>>> Because when 1st user operation after system resume happens, Gfx
> > > >>>> already finishes resuming and audio initialization, so as long as
> > > >>>> intel_haswell_wait_ready_to_resume() enable the unsol event, the
> > > >>>> unsol event comes and so so waiting finishes. The 300ms time out
> > > >>>> is set for safety consideration in case unsol event is lost. I've not
> > observed any unsol event lost till now.
> > > >>>>
> > > >>>> As for the timeout, I suggest you use the codec->bus->workq
> > > >>>> instead of creating a new workq. I think that will also give us
> > > >>>> some serialisation, i e, protection against race conditions if
> > > >>>> the timeout happen at the same time as the unsol event.
> > > >>>
> > > >>> Hi David,
> > > >>>
> > > >>> The new added "resume_wq" for hdmi codec is a wait queue, not a work
> > queue like codec->bus->workq.
> > > >>> It's expected to wake up as soon as the unsol event is got.
> > > >>
> > > >> Sure; but I don't see why you need a wait queue for that? Why don't
> > > >> you just call the resume path from the unsol event handler
> > > >> (hdmi_present_sense, or its caller), and then also cancel the
> > > >> timeout handler (which can then be in the normal workq)?
> > > >
> > > > Because the delayed resume actually fakes as if the resume is done.
> > > > This is necessary not to block other device's resume operation.
> > > >
> > > > Since it looks as if ready, user-space might restart things soon
> > > > again before the delayed resume is really finished.  So, some
> > > > serialization is required there.
> > >
> > > Lin, would it be possible to add some chain of events description to
> > > the bug commit? The different contexts are just boggling my mind :-)
> > 
> > Yeah, a nice code flow picture would be helpful :)
> > 
> > > Assume we have two cases, system idle after S3 and immediate playback
> > > after S3.
> > >
> > > Is this correct:
> > >
> > > System idle:
> > >   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
> > >   2. Since the codec is not reinitialized, nothing powers up the
> > > codec, so this is all that happens.
> > >   3. Since unsol events are not enabled (?), we have a bug that jack
> > > detection does not work and cannot be detected from userspace...?
> 
> Yes. This is a side effect of this patch. The unsol event is delayed until the user operation triggers the delayed resume.
> Then intel_haswell_wait_ready_to_resume() will enable unsol events.
> 
> > >
> > > Immediate playback:
> > >   1. System skips hda_call_codec_resume and sets codec->resume_delayed.
> > >   2. Process context wants to start playback, which powers up the
> > > codec and calls intel_haswell_set_power_state.
> > >   3. intel_haswell_wait_ready_to_resume enables unsol events and
> > > starts to wait on ready_to_resume
> > >   4. Gfx init finishes, and workq context fires the unsol event, which
> > > calls hdmi_intrinsic_event, which triggers the resume_wq.
> > >   5. Process context and workq context now continues in parallel,
> > > potentially (but hopefully not) leading to race conditions?
> > 
> > The current patch has a small race, but it can be avoided by clearing
> > spec->ready_to_resume early enough, e.g. in suspend callback or in
> > init callback, like the patch below.
> > 
> 
> I think there is no race for Haswell. In my test I observed that the unsol evnet will not be received
> until intel_haswell_wait_ready_to_resume() enables the unsol event on the pins.
> So I put "spec->ready_to_resume = 0" before enabling the unsol event. 

Practically your patch would work well, but theoretically the unsol
event might be issued and handled by hdmi_intrinsic_event() before you
go into intel_haswell_wait_ready_to_resume().  That is, you _clear_
spec->ready_to_resume again and wait for the unsol event that had been
already processed.  That's the race David pointed.

The fix is simply not to clear spec->ready_to_resume in
intel_haswell_wait_ready_to_resume() but clear it much earlier already
before the resume path as my patch does.  Then wait_event() passes
immediately (and the unsol enablement of is anyway harmless).


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Saturday, April 06, 2013 1:05 AM


> > > > Immediate playback:
> > > >   1. System skips hda_call_codec_resume and sets
> codec->resume_delayed.
> > > >   2. Process context wants to start playback, which powers up the
> > > > codec and calls intel_haswell_set_power_state.
> > > >   3. intel_haswell_wait_ready_to_resume enables unsol events and
> > > > starts to wait on ready_to_resume
> > > >   4. Gfx init finishes, and workq context fires the unsol event,
> > > > which calls hdmi_intrinsic_event, which triggers the resume_wq.
> > > >   5. Process context and workq context now continues in parallel,
> > > > potentially (but hopefully not) leading to race conditions?
> > >
> > > The current patch has a small race, but it can be avoided by
> > > clearing
> > > spec->ready_to_resume early enough, e.g. in suspend callback or in
> > > init callback, like the patch below.
> > >
> >
> > I think there is no race for Haswell. In my test I observed that the
> > unsol evnet will not be received until intel_haswell_wait_ready_to_resume()
> enables the unsol event on the pins.
> > So I put "spec->ready_to_resume = 0" before enabling the unsol event.
> 
> Practically your patch would work well, but theoretically the unsol event might
> be issued and handled by hdmi_intrinsic_event() before you go into
> intel_haswell_wait_ready_to_resume().  That is, you _clear_
> spec->ready_to_resume again and wait for the unsol event that had been
> already processed.  That's the race David pointed.
> 
> The fix is simply not to clear spec->ready_to_resume in
> intel_haswell_wait_ready_to_resume() but clear it much earlier already before
> the resume path as my patch does.  Then wait_event() passes immediately
> (and the unsol enablement of is anyway harmless).
> 

Hi Takashi,

I see. Thanks for clarifying!

I'll revise the patch and test it again when I'm in office on Monday.
Also I'll rebase the patch to for-next branch of sound git tree and fix the indentations and spaces errors.

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Fri, 5 Apr 2013 16:58:54 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, April 02, 2013 5:53 PM
> > To: Lin, Mengdong
> > Cc: alsa-devel@alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> > in system resume
> > 
> > At Tue, 26 Mar 2013 07:02:54 +0000,
> > Lin, Mengdong wrote:
> > >
> > > Is it okay to add a new parameter to snd_hda_resume(), to indicate whether
> > codec resume should be delayed or not?
> > > Is because snd_hda_resume() is only only called by azx_resume() but also
> > called by azx_bus_reset().
> > > The latter can be called on a fatal verb execution failure and codec resume
> > delay is not suitable for this case.
> > 
> > Well, I prefer creating a new function (e.g. snd_hda_bus_reset()) called from
> > azx_bus_reset().
> > 
> > 
> > Takashi
> 
> Hi Takashi,
> 
> Do you mean that in azx_bus_reset(), we can merge the snd_hda_suspend() and snd_hda_resume() to a new function like snd_hda_bus_reset()?

Yes.  Calling snd_hda_suspend() following snd_hda_resume() immediately
is just an ad hoc way of reset.  It should have been a dedicated reset
function from the beginning.


Takashi

> snd_hda_suspend() and snd_hda_resume() look symmetrical here and so I don't want to destroy the symmetry.
> 
> static void azx_bus_reset(struct hda_bus *bus)
> {
> 	...
> #ifdef CONFIG_PM
> 	if (chip->initialized) {
> 		struct azx_pcm *p;
> 		list_for_each_entry(p, &chip->pcm_list, list)
> 			snd_pcm_suspend_all(p->pcm);
> 		snd_hda_suspend(chip->bus);
> 		snd_hda_resume(chip->bus);
> 	}
> #endif
> 	bus->in_reset = 0;
> }
> 
> Thanks
> Mengdong
> 

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/07/2013 02:10 AM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Saturday, April 06, 2013 1:05 AM
>
>
>>>>> Immediate playback:
>>>>>    1. System skips hda_call_codec_resume and sets
>> codec->resume_delayed.
>>>>>    2. Process context wants to start playback, which powers up the
>>>>> codec and calls intel_haswell_set_power_state.
>>>>>    3. intel_haswell_wait_ready_to_resume enables unsol events and
>>>>> starts to wait on ready_to_resume
>>>>>    4. Gfx init finishes, and workq context fires the unsol event,
>>>>> which calls hdmi_intrinsic_event, which triggers the resume_wq.
>>>>>    5. Process context and workq context now continues in parallel,
>>>>> potentially (but hopefully not) leading to race conditions?
>>>>
>>>> The current patch has a small race, but it can be avoided by
>>>> clearing
>>>> spec->ready_to_resume early enough, e.g. in suspend callback or in
>>>> init callback, like the patch below.
>>>>
>>>
>>> I think there is no race for Haswell. In my test I observed that the
>>> unsol evnet will not be received until intel_haswell_wait_ready_to_resume()
>> enables the unsol event on the pins.
>>> So I put "spec->ready_to_resume = 0" before enabling the unsol event.
>>
>> Practically your patch would work well, but theoretically the unsol event might
>> be issued and handled by hdmi_intrinsic_event() before you go into
>> intel_haswell_wait_ready_to_resume().  That is, you _clear_
>> spec->ready_to_resume again and wait for the unsol event that had been
>> already processed.  That's the race David pointed.
>>
>> The fix is simply not to clear spec->ready_to_resume in
>> intel_haswell_wait_ready_to_resume() but clear it much earlier already before
>> the resume path as my patch does.  Then wait_event() passes immediately
>> (and the unsol enablement of is anyway harmless).
>>
>
> Hi Takashi,
>
> I see. Thanks for clarifying!
>
> I'll revise the patch and test it again when I'm in office on Monday.
> Also I'll rebase the patch to for-next branch of sound git tree and fix the indentations and spaces errors.

I'm still not understanding this patch.

We of course cannot have a situation where HDMI jack detection is not 
correctly working after S3, which looks like would be the case here?

Second, I just saw on a machine we're working on, the symptoms: one of 
the pins in D3 and the right channel muted. And this was on a clean 
boot, not after S3 suspend/resume cycle.

To look at the problem again: If the problem is that something must be 
done on the graphics driver side first and then on the audio side, 
wouldn't the solution be for the video driver and audio driver to 
communicate somehow? And resume (and possibly init?) would not complete 
until first the graphics driver has done its resume, and after that, the 
audio driver. I e, userspace will remain frozen until both drivers have 
completed a correct resume.


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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Mon, 08 Apr 2013 11:49:09 +0200,
David Henningsson wrote:
> 
> On 04/07/2013 02:10 AM, Lin, Mengdong wrote:
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai@suse.de]
> >> Sent: Saturday, April 06, 2013 1:05 AM
> >
> >
> >>>>> Immediate playback:
> >>>>>    1. System skips hda_call_codec_resume and sets
> >> codec->resume_delayed.
> >>>>>    2. Process context wants to start playback, which powers up the
> >>>>> codec and calls intel_haswell_set_power_state.
> >>>>>    3. intel_haswell_wait_ready_to_resume enables unsol events and
> >>>>> starts to wait on ready_to_resume
> >>>>>    4. Gfx init finishes, and workq context fires the unsol event,
> >>>>> which calls hdmi_intrinsic_event, which triggers the resume_wq.
> >>>>>    5. Process context and workq context now continues in parallel,
> >>>>> potentially (but hopefully not) leading to race conditions?
> >>>>
> >>>> The current patch has a small race, but it can be avoided by
> >>>> clearing
> >>>> spec->ready_to_resume early enough, e.g. in suspend callback or in
> >>>> init callback, like the patch below.
> >>>>
> >>>
> >>> I think there is no race for Haswell. In my test I observed that the
> >>> unsol evnet will not be received until intel_haswell_wait_ready_to_resume()
> >> enables the unsol event on the pins.
> >>> So I put "spec->ready_to_resume = 0" before enabling the unsol event.
> >>
> >> Practically your patch would work well, but theoretically the unsol event might
> >> be issued and handled by hdmi_intrinsic_event() before you go into
> >> intel_haswell_wait_ready_to_resume().  That is, you _clear_
> >> spec->ready_to_resume again and wait for the unsol event that had been
> >> already processed.  That's the race David pointed.
> >>
> >> The fix is simply not to clear spec->ready_to_resume in
> >> intel_haswell_wait_ready_to_resume() but clear it much earlier already before
> >> the resume path as my patch does.  Then wait_event() passes immediately
> >> (and the unsol enablement of is anyway harmless).
> >>
> >
> > Hi Takashi,
> >
> > I see. Thanks for clarifying!
> >
> > I'll revise the patch and test it again when I'm in office on Monday.
> > Also I'll rebase the patch to for-next branch of sound git tree and fix the indentations and spaces errors.
> 
> I'm still not understanding this patch.
> 
> We of course cannot have a situation where HDMI jack detection is not 
> correctly working after S3, which looks like would be the case here?

No.  The patch itself has nothing to do with the HDMI jack detection
at all.  This intrinsic unsol event is (according to Intel) guaranteed
to be issued once at the audio codec initialization, at least for
Haswell.

> Second, I just saw on a machine we're working on, the symptoms: one of 
> the pins in D3 and the right channel muted. And this was on a clean 
> boot, not after S3 suspend/resume cycle.

Maybe another problem.  Maybe not.  Does the problem persist if you
reload the driver (without invocation of alsactl via udev)?

> To look at the problem again: If the problem is that something must be 
> done on the graphics driver side first and then on the audio side, 
> wouldn't the solution be for the video driver and audio driver to 
> communicate somehow?

Yes, the pm domain support is necessary sooner or later, as I already
discussed shortly with Dave Airlie.  However...

> And resume (and possibly init?) would not complete 
> until first the graphics driver has done its resume, and after that, the 
> audio driver. I e, userspace will remain frozen until both drivers have 
> completed a correct resume.

... the resume problem can't be fixed only by the usual pm domain
serialization.  The graphics initialization for the audio bit is done
asynchronously, thus we can't guarantee the audio codec is really
ready after the graphics driver returns from the resume callback.
It shows as if it's ready (you can turn it to D0) but actually it
doesn't change on the hardware.  Thus, the serialization with an unsol
event wait seems  mandatory for the time being.


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, April 08, 2013 6:24 PM
> To: David Henningsson
> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> At Mon, 08 Apr 2013 11:49:09 +0200,
> David Henningsson wrote:
> >
> > I'm still not understanding this patch.
> >
> > We of course cannot have a situation where HDMI jack detection is not
> > correctly working after S3, which looks like would be the case here?
> 
> No.  The patch itself has nothing to do with the HDMI jack detection at all.
> This intrinsic unsol event is (according to Intel) guaranteed to be issued once at
> the audio codec initialization, at least for Haswell.
> 
> > Second, I just saw on a machine we're working on, the symptoms: one of
> > the pins in D3 and the right channel muted. And this was on a clean
> > boot, not after S3 suspend/resume cycle.
> 
> Maybe another problem.  Maybe not.  Does the problem persist if you reload
> the driver (without invocation of alsactl via udev)?

Hi Takashi and David,

This is the same dependency issue as after system suspend/resume cycles, with same phenomenon.
I duplicated this error if I boot without an HDMI/DP cable connected and connect the cable later.
Maybe we need to delay codec operations on initialization like in resume case.

> > To look at the problem again: If the problem is that something must be
> > done on the graphics driver side first and then on the audio side,
> > wouldn't the solution be for the video driver and audio driver to
> > communicate somehow?
> 
> Yes, the pm domain support is necessary sooner or later, as I already discussed
> shortly with Dave Airlie.  However...

Liam will propose Gfx driver team to implement a pm domain.

> > And resume (and possibly init?) would not complete until first the
> > graphics driver has done its resume, and after that, the audio driver.
> > I e, userspace will remain frozen until both drivers have completed a
> > correct resume.
> 
> ... the resume problem can't be fixed only by the usual pm domain serialization.
> The graphics initialization for the audio bit is done asynchronously, thus we
> can't guarantee the audio codec is really ready after the graphics driver returns
> from the resume callback.
> It shows as if it's ready (you can turn it to D0) but actually it doesn't change on
> the hardware.  Thus, the serialization with an unsol event wait seems
> mandatory for the time being.
> 
It seems so. We have not found better solution now :-(
I've revised the patch for system suspend/resume case. Please review.

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Monday, April 08, 2013 6:24 PM
>> To: David Henningsson
>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>> in system resume
>>
>> At Mon, 08 Apr 2013 11:49:09 +0200,
>> David Henningsson wrote:
>>>
>>> I'm still not understanding this patch.
>>>
>>> We of course cannot have a situation where HDMI jack detection is not
>>> correctly working after S3, which looks like would be the case here?
>>
>> No.  The patch itself has nothing to do with the HDMI jack detection at all.
>> This intrinsic unsol event is (according to Intel) guaranteed to be issued once at
>> the audio codec initialization, at least for Haswell.

Well, if no process is using the HDMI codec actively, nothing will 
initialize the resume, and the HDMI codec will not initialized at all, i 
e, no unsol events enabled in the first place?

(The counter argument would be; if nobody uses HDMI codec, does it 
matter that it is not initialized, but I'm unsure if e g "amixer 
contents" or listeners on legacy /dev/input actually powers up the chip.)

>>> Second, I just saw on a machine we're working on, the symptoms: one of
>>> the pins in D3 and the right channel muted. And this was on a clean
>>> boot, not after S3 suspend/resume cycle.
>>
>> Maybe another problem.  Maybe not.  Does the problem persist if you reload
>> the driver (without invocation of alsactl via udev)?
>
> Hi Takashi and David,
>
> This is the same dependency issue as after system suspend/resume cycles, with same phenomenon.
> I duplicated this error if I boot without an HDMI/DP cable connected and connect the cable later.
> Maybe we need to delay codec operations on initialization like in resume case.

So it's a problem both on hotplug and S3...

>>> To look at the problem again: If the problem is that something must be
>>> done on the graphics driver side first and then on the audio side,
>>> wouldn't the solution be for the video driver and audio driver to
>>> communicate somehow?
>>
>> Yes, the pm domain support is necessary sooner or later, as I already discussed
>> shortly with Dave Airlie.  However...
>
> Liam will propose Gfx driver team to implement a pm domain.
>
>>> And resume (and possibly init?) would not complete until first the
>>> graphics driver has done its resume, and after that, the audio driver.
>>> I e, userspace will remain frozen until both drivers have completed a
>>> correct resume.
>>
>> ... the resume problem can't be fixed only by the usual pm domain serialization.
>> The graphics initialization for the audio bit is done asynchronously, thus we
>> can't guarantee the audio codec is really ready after the graphics driver returns
>> from the resume callback.
>> It shows as if it's ready (you can turn it to D0) but actually it doesn't change on
>> the hardware.  Thus, the serialization with an unsol event wait seems
>> mandatory for the time being.
>>
> It seems so. We have not found better solution now :-(
> I've revised the patch for system suspend/resume case. Please review.

If this problem can be detected by looking at the pin and finding that 
it is in D3, can we extend the "try to set the pin 10 times" algorithm 
in hda_set_power_state to check all the pins for haswell HDMI?


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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Mon, 08 Apr 2013 13:35:25 +0200,
David Henningsson wrote:
> 
> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai@suse.de]
> >> Sent: Monday, April 08, 2013 6:24 PM
> >> To: David Henningsson
> >> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >> in system resume
> >>
> >> At Mon, 08 Apr 2013 11:49:09 +0200,
> >> David Henningsson wrote:
> >>>
> >>> I'm still not understanding this patch.
> >>>
> >>> We of course cannot have a situation where HDMI jack detection is not
> >>> correctly working after S3, which looks like would be the case here?
> >>
> >> No.  The patch itself has nothing to do with the HDMI jack detection at all.
> >> This intrinsic unsol event is (according to Intel) guaranteed to be issued once at
> >> the audio codec initialization, at least for Haswell.
> 
> Well, if no process is using the HDMI codec actively, nothing will 
> initialize the resume, and the HDMI codec will not initialized at all, i 
> e, no unsol events enabled in the first place?

No, the intrinsic unsol event that is issued for this seems irrelevant
from the jack state (i.e. active usage).

> (The counter argument would be; if nobody uses HDMI codec, does it 
> matter that it is not initialized, but I'm unsure if e g "amixer 
> contents" or listeners on legacy /dev/input actually powers up the chip.)

The problem is that the codec resume is always performed no matter
whether the device is used or not.  This was different in the past but
it's been changed in that way because there are cases where you need
to initialize the hardware properly once (e.g. mute-LED toggling).


> >>> Second, I just saw on a machine we're working on, the symptoms: one of
> >>> the pins in D3 and the right channel muted. And this was on a clean
> >>> boot, not after S3 suspend/resume cycle.
> >>
> >> Maybe another problem.  Maybe not.  Does the problem persist if you reload
> >> the driver (without invocation of alsactl via udev)?
> >
> > Hi Takashi and David,
> >
> > This is the same dependency issue as after system suspend/resume cycles, with same phenomenon.
> > I duplicated this error if I boot without an HDMI/DP cable connected and connect the cable later.
> > Maybe we need to delay codec operations on initialization like in resume case.
> 
> So it's a problem both on hotplug and S3...

In the case of hotplug, you don't hit the inconsistent power sate
we're trying to solve.  The graphics has been already powered up.


> >>> To look at the problem again: If the problem is that something must be
> >>> done on the graphics driver side first and then on the audio side,
> >>> wouldn't the solution be for the video driver and audio driver to
> >>> communicate somehow?
> >>
> >> Yes, the pm domain support is necessary sooner or later, as I already discussed
> >> shortly with Dave Airlie.  However...
> >
> > Liam will propose Gfx driver team to implement a pm domain.
> >
> >>> And resume (and possibly init?) would not complete until first the
> >>> graphics driver has done its resume, and after that, the audio driver.
> >>> I e, userspace will remain frozen until both drivers have completed a
> >>> correct resume.
> >>
> >> ... the resume problem can't be fixed only by the usual pm domain serialization.
> >> The graphics initialization for the audio bit is done asynchronously, thus we
> >> can't guarantee the audio codec is really ready after the graphics driver returns
> >> from the resume callback.
> >> It shows as if it's ready (you can turn it to D0) but actually it doesn't change on
> >> the hardware.  Thus, the serialization with an unsol event wait seems
> >> mandatory for the time being.
> >>
> > It seems so. We have not found better solution now :-(
> > I've revised the patch for system suspend/resume case. Please review.
> 
> If this problem can be detected by looking at the pin and finding that 
> it is in D3,

No, you can't see that it's D3.  The controller chip _shows_ it's in
D0 while it's actually in D3.  That's the problem.

So, this is actually a hardware design bug.  But we need to live with
that.


Takashi

> can we extend the "try to set the pin 10 times" algorithm 
> in hda_set_power_state to check all the pins for haswell HDMI?

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/08/2013 01:59 PM, Takashi Iwai wrote:
> At Mon, 08 Apr 2013 13:35:25 +0200,
> David Henningsson wrote:
>>
>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
>>>> -----Original Message-----
>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>> Sent: Monday, April 08, 2013 6:24 PM
>>>> To: David Henningsson
>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>>>> in system resume
>>>>
>>>> At Mon, 08 Apr 2013 11:49:09 +0200,
>>>> David Henningsson wrote:
>>>>>
>>>>> I'm still not understanding this patch.
>>>>>
>>>>> We of course cannot have a situation where HDMI jack detection is not
>>>>> correctly working after S3, which looks like would be the case here?
>>>>
>>>> No.  The patch itself has nothing to do with the HDMI jack detection at all.
>>>> This intrinsic unsol event is (according to Intel) guaranteed to be issued once at
>>>> the audio codec initialization, at least for Haswell.
>>
>> Well, if no process is using the HDMI codec actively, nothing will
>> initialize the resume, and the HDMI codec will not initialized at all, i
>> e, no unsol events enabled in the first place?
>
> No, the intrinsic unsol event that is issued for this seems irrelevant
> from the jack state (i.e. active usage).
>
>> (The counter argument would be; if nobody uses HDMI codec, does it
>> matter that it is not initialized, but I'm unsure if e g "amixer
>> contents" or listeners on legacy /dev/input actually powers up the chip.)
>
> The problem is that the codec resume is always performed no matter
> whether the device is used or not.  This was different in the past but
> it's been changed in that way because there are cases where you need
> to initialize the hardware properly once (e.g. mute-LED toggling).
>
>
>>>>> Second, I just saw on a machine we're working on, the symptoms: one of
>>>>> the pins in D3 and the right channel muted. And this was on a clean
>>>>> boot, not after S3 suspend/resume cycle.
>>>>
>>>> Maybe another problem.  Maybe not.  Does the problem persist if you reload
>>>> the driver (without invocation of alsactl via udev)?
>>>
>>> Hi Takashi and David,
>>>
>>> This is the same dependency issue as after system suspend/resume cycles, with same phenomenon.
>>> I duplicated this error if I boot without an HDMI/DP cable connected and connect the cable later.
>>> Maybe we need to delay codec operations on initialization like in resume case.
>>
>> So it's a problem both on hotplug and S3...
>
> In the case of hotplug, you don't hit the inconsistent power sate
> we're trying to solve.  The graphics has been already powered up.
>
>
>>>>> To look at the problem again: If the problem is that something must be
>>>>> done on the graphics driver side first and then on the audio side,
>>>>> wouldn't the solution be for the video driver and audio driver to
>>>>> communicate somehow?
>>>>
>>>> Yes, the pm domain support is necessary sooner or later, as I already discussed
>>>> shortly with Dave Airlie.  However...
>>>
>>> Liam will propose Gfx driver team to implement a pm domain.
>>>
>>>>> And resume (and possibly init?) would not complete until first the
>>>>> graphics driver has done its resume, and after that, the audio driver.
>>>>> I e, userspace will remain frozen until both drivers have completed a
>>>>> correct resume.
>>>>
>>>> ... the resume problem can't be fixed only by the usual pm domain serialization.
>>>> The graphics initialization for the audio bit is done asynchronously, thus we
>>>> can't guarantee the audio codec is really ready after the graphics driver returns
>>>> from the resume callback.
>>>> It shows as if it's ready (you can turn it to D0) but actually it doesn't change on
>>>> the hardware.  Thus, the serialization with an unsol event wait seems
>>>> mandatory for the time being.
>>>>
>>> It seems so. We have not found better solution now :-(
>>> I've revised the patch for system suspend/resume case. Please review.
>>
>> If this problem can be detected by looking at the pin and finding that
>> it is in D3,
>
> No, you can't see that it's D3.  The controller chip _shows_ it's in
> D0 while it's actually in D3.  That's the problem.
>
> So, this is actually a hardware design bug.  But we need to live with
> that.

Then I'm not sure it's the same problem; because for me I can see the D3 
in the codec proc output.




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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Mon, 08 Apr 2013 14:20:28 +0200,
David Henningsson wrote:
> 
> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
> > At Mon, 08 Apr 2013 13:35:25 +0200,
> > David Henningsson wrote:
> >>
> >> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
> >>>> -----Original Message-----
> >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>> Sent: Monday, April 08, 2013 6:24 PM
> >>>> To: David Henningsson
> >>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> >>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >>>> in system resume
> >>>>
> >>>> At Mon, 08 Apr 2013 11:49:09 +0200,
> >>>> David Henningsson wrote:
> >>>>>
> >>>>> I'm still not understanding this patch.
> >>>>>
> >>>>> We of course cannot have a situation where HDMI jack detection is not
> >>>>> correctly working after S3, which looks like would be the case here?
> >>>>
> >>>> No.  The patch itself has nothing to do with the HDMI jack detection at all.
> >>>> This intrinsic unsol event is (according to Intel) guaranteed to be issued once at
> >>>> the audio codec initialization, at least for Haswell.
> >>
> >> Well, if no process is using the HDMI codec actively, nothing will
> >> initialize the resume, and the HDMI codec will not initialized at all, i
> >> e, no unsol events enabled in the first place?
> >
> > No, the intrinsic unsol event that is issued for this seems irrelevant
> > from the jack state (i.e. active usage).
> >
> >> (The counter argument would be; if nobody uses HDMI codec, does it
> >> matter that it is not initialized, but I'm unsure if e g "amixer
> >> contents" or listeners on legacy /dev/input actually powers up the chip.)
> >
> > The problem is that the codec resume is always performed no matter
> > whether the device is used or not.  This was different in the past but
> > it's been changed in that way because there are cases where you need
> > to initialize the hardware properly once (e.g. mute-LED toggling).
> >
> >
> >>>>> Second, I just saw on a machine we're working on, the symptoms: one of
> >>>>> the pins in D3 and the right channel muted. And this was on a clean
> >>>>> boot, not after S3 suspend/resume cycle.
> >>>>
> >>>> Maybe another problem.  Maybe not.  Does the problem persist if you reload
> >>>> the driver (without invocation of alsactl via udev)?
> >>>
> >>> Hi Takashi and David,
> >>>
> >>> This is the same dependency issue as after system suspend/resume cycles, with same phenomenon.
> >>> I duplicated this error if I boot without an HDMI/DP cable connected and connect the cable later.
> >>> Maybe we need to delay codec operations on initialization like in resume case.
> >>
> >> So it's a problem both on hotplug and S3...
> >
> > In the case of hotplug, you don't hit the inconsistent power sate
> > we're trying to solve.  The graphics has been already powered up.
> >
> >
> >>>>> To look at the problem again: If the problem is that something must be
> >>>>> done on the graphics driver side first and then on the audio side,
> >>>>> wouldn't the solution be for the video driver and audio driver to
> >>>>> communicate somehow?
> >>>>
> >>>> Yes, the pm domain support is necessary sooner or later, as I already discussed
> >>>> shortly with Dave Airlie.  However...
> >>>
> >>> Liam will propose Gfx driver team to implement a pm domain.
> >>>
> >>>>> And resume (and possibly init?) would not complete until first the
> >>>>> graphics driver has done its resume, and after that, the audio driver.
> >>>>> I e, userspace will remain frozen until both drivers have completed a
> >>>>> correct resume.
> >>>>
> >>>> ... the resume problem can't be fixed only by the usual pm domain serialization.
> >>>> The graphics initialization for the audio bit is done asynchronously, thus we
> >>>> can't guarantee the audio codec is really ready after the graphics driver returns
> >>>> from the resume callback.
> >>>> It shows as if it's ready (you can turn it to D0) but actually it doesn't change on
> >>>> the hardware.  Thus, the serialization with an unsol event wait seems
> >>>> mandatory for the time being.
> >>>>
> >>> It seems so. We have not found better solution now :-(
> >>> I've revised the patch for system suspend/resume case. Please review.
> >>
> >> If this problem can be detected by looking at the pin and finding that
> >> it is in D3,
> >
> > No, you can't see that it's D3.  The controller chip _shows_ it's in
> > D0 while it's actually in D3.  That's the problem.
> >
> > So, this is actually a hardware design bug.  But we need to live with
> > that.
> 
> Then I'm not sure it's the same problem; because for me I can see the D3 
> in the codec proc output.

I suppose you didn't set the powersave for it, right?

BTW, the main problem was about the FG node, which you can't judge
from the proc output, unfortunately.


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/08/2013 02:23 PM, Takashi Iwai wrote:
> At Mon, 08 Apr 2013 14:20:28 +0200,
> David Henningsson wrote:
>>
>> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
>>> At Mon, 08 Apr 2013 13:35:25 +0200,
>>> David Henningsson wrote:
>>>>
>>>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
>>>>>> -----Original Message-----
>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>>> Sent: Monday, April 08, 2013 6:24 PM
>>>>>> To: David Henningsson
>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>>>>>> in system resume
>>>>>>
>>>>>> At Mon, 08 Apr 2013 11:49:09 +0200,
>>>>>> David Henningsson wrote:
>>>>>>>
>>>>>>> I'm still not understanding this patch.
>>>>>>>
>>>>>>> We of course cannot have a situation where HDMI jack detection is not
>>>>>>> correctly working after S3, which looks like would be the case here?
>>>>>>
>>>>>> No.  The patch itself has nothing to do with the HDMI jack detection at all.
>>>>>> This intrinsic unsol event is (according to Intel) guaranteed to be issued once at
>>>>>> the audio codec initialization, at least for Haswell.
>>>>
>>>> Well, if no process is using the HDMI codec actively, nothing will
>>>> initialize the resume, and the HDMI codec will not initialized at all, i
>>>> e, no unsol events enabled in the first place?
>>>
>>> No, the intrinsic unsol event that is issued for this seems irrelevant
>>> from the jack state (i.e. active usage).
>>>
>>>> (The counter argument would be; if nobody uses HDMI codec, does it
>>>> matter that it is not initialized, but I'm unsure if e g "amixer
>>>> contents" or listeners on legacy /dev/input actually powers up the chip.)
>>>
>>> The problem is that the codec resume is always performed no matter
>>> whether the device is used or not.  This was different in the past but
>>> it's been changed in that way because there are cases where you need
>>> to initialize the hardware properly once (e.g. mute-LED toggling).
>>>
>>>
>>>>>>> Second, I just saw on a machine we're working on, the symptoms: one of
>>>>>>> the pins in D3 and the right channel muted. And this was on a clean
>>>>>>> boot, not after S3 suspend/resume cycle.
>>>>>>
>>>>>> Maybe another problem.  Maybe not.  Does the problem persist if you reload
>>>>>> the driver (without invocation of alsactl via udev)?
>>>>>
>>>>> Hi Takashi and David,
>>>>>
>>>>> This is the same dependency issue as after system suspend/resume cycles, with same phenomenon.
>>>>> I duplicated this error if I boot without an HDMI/DP cable connected and connect the cable later.
>>>>> Maybe we need to delay codec operations on initialization like in resume case.
>>>>
>>>> So it's a problem both on hotplug and S3...
>>>
>>> In the case of hotplug, you don't hit the inconsistent power sate
>>> we're trying to solve.  The graphics has been already powered up.
>>>
>>>
>>>>>>> To look at the problem again: If the problem is that something must be
>>>>>>> done on the graphics driver side first and then on the audio side,
>>>>>>> wouldn't the solution be for the video driver and audio driver to
>>>>>>> communicate somehow?
>>>>>>
>>>>>> Yes, the pm domain support is necessary sooner or later, as I already discussed
>>>>>> shortly with Dave Airlie.  However...
>>>>>
>>>>> Liam will propose Gfx driver team to implement a pm domain.
>>>>>
>>>>>>> And resume (and possibly init?) would not complete until first the
>>>>>>> graphics driver has done its resume, and after that, the audio driver.
>>>>>>> I e, userspace will remain frozen until both drivers have completed a
>>>>>>> correct resume.
>>>>>>
>>>>>> ... the resume problem can't be fixed only by the usual pm domain serialization.
>>>>>> The graphics initialization for the audio bit is done asynchronously, thus we
>>>>>> can't guarantee the audio codec is really ready after the graphics driver returns
>>>>>> from the resume callback.
>>>>>> It shows as if it's ready (you can turn it to D0) but actually it doesn't change on
>>>>>> the hardware.  Thus, the serialization with an unsol event wait seems
>>>>>> mandatory for the time being.
>>>>>>
>>>>> It seems so. We have not found better solution now :-(
>>>>> I've revised the patch for system suspend/resume case. Please review.
>>>>
>>>> If this problem can be detected by looking at the pin and finding that
>>>> it is in D3,
>>>
>>> No, you can't see that it's D3.  The controller chip _shows_ it's in
>>> D0 while it's actually in D3.  That's the problem.
>>>
>>> So, this is actually a hardware design bug.  But we need to live with
>>> that.
>>
>> Then I'm not sure it's the same problem; because for me I can see the D3
>> in the codec proc output.
>
> I suppose you didn't set the powersave for it, right?

AFAIK, powersave remains at upstream default.

> BTW, the main problem was about the FG node, which you can't judge
> from the proc output, unfortunately.

Here's what it looks like for me. Right channel muted and pin in D3.

Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP
   Control: name="HDMI/DP,pcm=3 Jack", index=0, device=0
   Control: name="IEC958 Playback Con Mask", index=0, device=0
   Control: name="IEC958 Playback Pro Mask", index=0, device=0
   Control: name="IEC958 Playback Default", index=0, device=0
   Control: name="IEC958 Playback Switch", index=0, device=0
   Control: name="ELD", index=0, device=3
   Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
   Amp-Out vals:  [0x00 0x80]
   Pincap 0x0b000094: OUT Detect HBR HDMI DP
   Pin Default 0x18560010: [Jack] Digital Out at Int HDMI
     Conn = Digital, Color = Unknown
     DefAssociation = 0x1, Sequence = 0x0
   Pin-ctls: 0x40: OUT
   Unsolicited: tag=01, enabled=1
   Power states:  D0 D3 EPSS
   Power: setting=D3, actual=D3
   Connection: 3
      0x02* 0x03 0x04



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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Mon, 08 Apr 2013 15:30:25 +0200,
David Henningsson wrote:
> 
> On 04/08/2013 02:23 PM, Takashi Iwai wrote:
> > At Mon, 08 Apr 2013 14:20:28 +0200,
> > David Henningsson wrote:
> >>
> >> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
> >>> At Mon, 08 Apr 2013 13:35:25 +0200,
> >>> David Henningsson wrote:
> >>>>
> >>>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>>>> Sent: Monday, April 08, 2013 6:24 PM
> >>>>>> To: David Henningsson
> >>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> >>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >>>>>> in system resume
> >>>>>>
> >>>>>> At Mon, 08 Apr 2013 11:49:09 +0200,
> >>>>>> David Henningsson wrote:
> >>>>>>>
> >>>>>>> I'm still not understanding this patch.
> >>>>>>>
> >>>>>>> We of course cannot have a situation where HDMI jack detection is not
> >>>>>>> correctly working after S3, which looks like would be the case here?
> >>>>>>
> >>>>>> No.  The patch itself has nothing to do with the HDMI jack detection at all.
> >>>>>> This intrinsic unsol event is (according to Intel) guaranteed to be issued once at
> >>>>>> the audio codec initialization, at least for Haswell.
> >>>>
> >>>> Well, if no process is using the HDMI codec actively, nothing will
> >>>> initialize the resume, and the HDMI codec will not initialized at all, i
> >>>> e, no unsol events enabled in the first place?
> >>>
> >>> No, the intrinsic unsol event that is issued for this seems irrelevant
> >>> from the jack state (i.e. active usage).
> >>>
> >>>> (The counter argument would be; if nobody uses HDMI codec, does it
> >>>> matter that it is not initialized, but I'm unsure if e g "amixer
> >>>> contents" or listeners on legacy /dev/input actually powers up the chip.)
> >>>
> >>> The problem is that the codec resume is always performed no matter
> >>> whether the device is used or not.  This was different in the past but
> >>> it's been changed in that way because there are cases where you need
> >>> to initialize the hardware properly once (e.g. mute-LED toggling).
> >>>
> >>>
> >>>>>>> Second, I just saw on a machine we're working on, the symptoms: one of
> >>>>>>> the pins in D3 and the right channel muted. And this was on a clean
> >>>>>>> boot, not after S3 suspend/resume cycle.
> >>>>>>
> >>>>>> Maybe another problem.  Maybe not.  Does the problem persist if you reload
> >>>>>> the driver (without invocation of alsactl via udev)?
> >>>>>
> >>>>> Hi Takashi and David,
> >>>>>
> >>>>> This is the same dependency issue as after system suspend/resume cycles, with same phenomenon.
> >>>>> I duplicated this error if I boot without an HDMI/DP cable connected and connect the cable later.
> >>>>> Maybe we need to delay codec operations on initialization like in resume case.
> >>>>
> >>>> So it's a problem both on hotplug and S3...
> >>>
> >>> In the case of hotplug, you don't hit the inconsistent power sate
> >>> we're trying to solve.  The graphics has been already powered up.
> >>>
> >>>
> >>>>>>> To look at the problem again: If the problem is that something must be
> >>>>>>> done on the graphics driver side first and then on the audio side,
> >>>>>>> wouldn't the solution be for the video driver and audio driver to
> >>>>>>> communicate somehow?
> >>>>>>
> >>>>>> Yes, the pm domain support is necessary sooner or later, as I already discussed
> >>>>>> shortly with Dave Airlie.  However...
> >>>>>
> >>>>> Liam will propose Gfx driver team to implement a pm domain.
> >>>>>
> >>>>>>> And resume (and possibly init?) would not complete until first the
> >>>>>>> graphics driver has done its resume, and after that, the audio driver.
> >>>>>>> I e, userspace will remain frozen until both drivers have completed a
> >>>>>>> correct resume.
> >>>>>>
> >>>>>> ... the resume problem can't be fixed only by the usual pm domain serialization.
> >>>>>> The graphics initialization for the audio bit is done asynchronously, thus we
> >>>>>> can't guarantee the audio codec is really ready after the graphics driver returns
> >>>>>> from the resume callback.
> >>>>>> It shows as if it's ready (you can turn it to D0) but actually it doesn't change on
> >>>>>> the hardware.  Thus, the serialization with an unsol event wait seems
> >>>>>> mandatory for the time being.
> >>>>>>
> >>>>> It seems so. We have not found better solution now :-(
> >>>>> I've revised the patch for system suspend/resume case. Please review.
> >>>>
> >>>> If this problem can be detected by looking at the pin and finding that
> >>>> it is in D3,
> >>>
> >>> No, you can't see that it's D3.  The controller chip _shows_ it's in
> >>> D0 while it's actually in D3.  That's the problem.
> >>>
> >>> So, this is actually a hardware design bug.  But we need to live with
> >>> that.
> >>
> >> Then I'm not sure it's the same problem; because for me I can see the D3
> >> in the codec proc output.
> >
> > I suppose you didn't set the powersave for it, right?
> 
> AFAIK, powersave remains at upstream default.

The option is overridden by user-space, so checking only the default
value doesn't make any sense :)

> > BTW, the main problem was about the FG node, which you can't judge
> > from the proc output, unfortunately.
> 
> Here's what it looks like for me. Right channel muted and pin in D3.
> 
> Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP
>    Control: name="HDMI/DP,pcm=3 Jack", index=0, device=0
>    Control: name="IEC958 Playback Con Mask", index=0, device=0
>    Control: name="IEC958 Playback Pro Mask", index=0, device=0
>    Control: name="IEC958 Playback Default", index=0, device=0
>    Control: name="IEC958 Playback Switch", index=0, device=0
>    Control: name="ELD", index=0, device=3
>    Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
>    Amp-Out vals:  [0x00 0x80]
>    Pincap 0x0b000094: OUT Detect HBR HDMI DP
>    Pin Default 0x18560010: [Jack] Digital Out at Int HDMI
>      Conn = Digital, Color = Unknown
>      DefAssociation = 0x1, Sequence = 0x0
>    Pin-ctls: 0x40: OUT
>    Unsolicited: tag=01, enabled=1
>    Power states:  D0 D3 EPSS
>    Power: setting=D3, actual=D3
>    Connection: 3
>       0x02* 0x03 0x04

Then this might be the different issue.


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, April 08, 2013 9:50 PM
> To: David Henningsson
> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> At Mon, 08 Apr 2013 15:30:25 +0200,
> David Henningsson wrote:
> >
> > On 04/08/2013 02:23 PM, Takashi Iwai wrote:
> > > At Mon, 08 Apr 2013 14:20:28 +0200,
> > > David Henningsson wrote:
> > >>
> > >> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
> > >>> At Mon, 08 Apr 2013 13:35:25 +0200, David Henningsson wrote:
> > >>>>
> > >>>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
> > >>>>>> -----Original Message-----
> > >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> > >>>>>> Sent: Monday, April 08, 2013 6:24 PM
> > >>>>>> To: David Henningsson
> > >>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> > >>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume
> > >>>>>> haswell hdmi codec in system resume
> > >>>>>>
> > >>>>>> At Mon, 08 Apr 2013 11:49:09 +0200, David Henningsson wrote:
> > >>>>>>>
> > >>>>>>> I'm still not understanding this patch.
> > >>>>>>>
> > >>>>>>> We of course cannot have a situation where HDMI jack detection
> > >>>>>>> is not correctly working after S3, which looks like would be the case
> here?
> > >>>>>>
> > >>>>>> No.  The patch itself has nothing to do with the HDMI jack detection
> at all.
> > >>>>>> This intrinsic unsol event is (according to Intel) guaranteed
> > >>>>>> to be issued once at the audio codec initialization, at least for
> Haswell.
> > >>>>
> > >>>> Well, if no process is using the HDMI codec actively, nothing
> > >>>> will initialize the resume, and the HDMI codec will not
> > >>>> initialized at all, i e, no unsol events enabled in the first place?
> > >>>
> > >>> No, the intrinsic unsol event that is issued for this seems
> > >>> irrelevant from the jack state (i.e. active usage).
> > >>>
> > >>>> (The counter argument would be; if nobody uses HDMI codec, does
> > >>>> it matter that it is not initialized, but I'm unsure if e g
> > >>>> "amixer contents" or listeners on legacy /dev/input actually
> > >>>> powers up the chip.)
> > >>>
> > >>> The problem is that the codec resume is always performed no matter
> > >>> whether the device is used or not.  This was different in the past
> > >>> but it's been changed in that way because there are cases where
> > >>> you need to initialize the hardware properly once (e.g. mute-LED
> toggling).
> > >>>
> > >>>
> > >>>>>>> Second, I just saw on a machine we're working on, the
> > >>>>>>> symptoms: one of the pins in D3 and the right channel muted.
> > >>>>>>> And this was on a clean boot, not after S3 suspend/resume cycle.
> > >>>>>>
> > >>>>>> Maybe another problem.  Maybe not.  Does the problem persist if
> > >>>>>> you reload the driver (without invocation of alsactl via udev)?
> > >>>>>
> > >>>>> Hi Takashi and David,
> > >>>>>
> > >>>>> This is the same dependency issue as after system suspend/resume
> cycles, with same phenomenon.
> > >>>>> I duplicated this error if I boot without an HDMI/DP cable connected
> and connect the cable later.
> > >>>>> Maybe we need to delay codec operations on initialization like in
> resume case.
> > >>>>
> > >>>> So it's a problem both on hotplug and S3...
> > >>>
> > >>> In the case of hotplug, you don't hit the inconsistent power sate
> > >>> we're trying to solve.  The graphics has been already powered up.
> > >>>
> > >>>
> > >>>>>>> To look at the problem again: If the problem is that something
> > >>>>>>> must be done on the graphics driver side first and then on the
> > >>>>>>> audio side, wouldn't the solution be for the video driver and
> > >>>>>>> audio driver to communicate somehow?
> > >>>>>>
> > >>>>>> Yes, the pm domain support is necessary sooner or later, as I
> > >>>>>> already discussed shortly with Dave Airlie.  However...
> > >>>>>
> > >>>>> Liam will propose Gfx driver team to implement a pm domain.
> > >>>>>
> > >>>>>>> And resume (and possibly init?) would not complete until first
> > >>>>>>> the graphics driver has done its resume, and after that, the audio
> driver.
> > >>>>>>> I e, userspace will remain frozen until both drivers have
> > >>>>>>> completed a correct resume.
> > >>>>>>
> > >>>>>> ... the resume problem can't be fixed only by the usual pm domain
> serialization.
> > >>>>>> The graphics initialization for the audio bit is done
> > >>>>>> asynchronously, thus we can't guarantee the audio codec is
> > >>>>>> really ready after the graphics driver returns from the resume
> callback.
> > >>>>>> It shows as if it's ready (you can turn it to D0) but actually
> > >>>>>> it doesn't change on the hardware.  Thus, the serialization
> > >>>>>> with an unsol event wait seems mandatory for the time being.
> > >>>>>>
> > >>>>> It seems so. We have not found better solution now :-( I've
> > >>>>> revised the patch for system suspend/resume case. Please review.
> > >>>>
> > >>>> If this problem can be detected by looking at the pin and finding
> > >>>> that it is in D3,
> > >>>
> > >>> No, you can't see that it's D3.  The controller chip _shows_ it's
> > >>> in
> > >>> D0 while it's actually in D3.  That's the problem.
> > >>>
> > >>> So, this is actually a hardware design bug.  But we need to live
> > >>> with that.
> > >>
> > >> Then I'm not sure it's the same problem; because for me I can see
> > >> the D3 in the codec proc output.
> > >
> > > I suppose you didn't set the powersave for it, right?
> >
> > AFAIK, powersave remains at upstream default.
> 
> The option is overridden by user-space, so checking only the default value
> doesn't make any sense :)
> 
> > > BTW, the main problem was about the FG node, which you can't judge
> > > from the proc output, unfortunately.
> >
> > Here's what it looks like for me. Right channel muted and pin in D3.
> >
> > Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP
> >    Control: name="HDMI/DP,pcm=3 Jack", index=0, device=0
> >    Control: name="IEC958 Playback Con Mask", index=0, device=0
> >    Control: name="IEC958 Playback Pro Mask", index=0, device=0
> >    Control: name="IEC958 Playback Default", index=0, device=0
> >    Control: name="IEC958 Playback Switch", index=0, device=0
> >    Control: name="ELD", index=0, device=3
> >    Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
> >    Amp-Out vals:  [0x00 0x80]
> >    Pincap 0x0b000094: OUT Detect HBR HDMI DP
> >    Pin Default 0x18560010: [Jack] Digital Out at Int HDMI
> >      Conn = Digital, Color = Unknown
> >      DefAssociation = 0x1, Sequence = 0x0
> >    Pin-ctls: 0x40: OUT
> >    Unsolicited: tag=01, enabled=1
> >    Power states:  D0 D3 EPSS
> >    Power: setting=D3, actual=D3
> >    Connection: 3
> >       0x02* 0x03 0x04
> 
> Then this might be the different issue.

This is same as what I observed after S3 or connect HDMI/DP cable after boot, on machines with power_save is '0'.
Amp-Out vals:  [0x00 0x80]
Power: setting=D3, actual=D3

If we set the pin state to D0 and unmute the pin again, HDMI can work. So I still think this is also caused by dependency on Gfx.

But the controller cannot find the error when it programs the codec to D0
-  when audio driver programs the codec and pins to D0, HW does not return error 
-  I also added code to check their power state immediately after setting state to D0, HW reports their state did change to D0.
However, after Gfx driver finishes setting up the display pipeline and enabling audio, the pin's state changed to D3 and get muted, as shown by /proc.
It seems to me that Gfx HW initialization can override some audio driver settings. So we choose to delay resuming the codec until the unsol event which indicates Gfx is ready.

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/08/2013 05:47 PM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Monday, April 08, 2013 9:50 PM
>> To: David Henningsson
>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>> in system resume
>>
>> At Mon, 08 Apr 2013 15:30:25 +0200,
>> David Henningsson wrote:
>>>
>>> On 04/08/2013 02:23 PM, Takashi Iwai wrote:
>>>> At Mon, 08 Apr 2013 14:20:28 +0200,
>>>> David Henningsson wrote:
>>>>>
>>>>> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
>>>>>> At Mon, 08 Apr 2013 13:35:25 +0200, David Henningsson wrote:
>>>>>>>
>>>>>>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>>>>>> Sent: Monday, April 08, 2013 6:24 PM
>>>>>>>>> To: David Henningsson
>>>>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
>>>>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume
>>>>>>>>> haswell hdmi codec in system resume
>>>>>>>>>
>>>>>>>>> At Mon, 08 Apr 2013 11:49:09 +0200, David Henningsson wrote:
>>>>>>>>>>
>>>>>>>>>> I'm still not understanding this patch.
>>>>>>>>>>
>>>>>>>>>> We of course cannot have a situation where HDMI jack detection
>>>>>>>>>> is not correctly working after S3, which looks like would be the case
>> here?
>>>>>>>>>
>>>>>>>>> No.  The patch itself has nothing to do with the HDMI jack detection
>> at all.
>>>>>>>>> This intrinsic unsol event is (according to Intel) guaranteed
>>>>>>>>> to be issued once at the audio codec initialization, at least for
>> Haswell.
>>>>>>>
>>>>>>> Well, if no process is using the HDMI codec actively, nothing
>>>>>>> will initialize the resume, and the HDMI codec will not
>>>>>>> initialized at all, i e, no unsol events enabled in the first place?
>>>>>>
>>>>>> No, the intrinsic unsol event that is issued for this seems
>>>>>> irrelevant from the jack state (i.e. active usage).
>>>>>>
>>>>>>> (The counter argument would be; if nobody uses HDMI codec, does
>>>>>>> it matter that it is not initialized, but I'm unsure if e g
>>>>>>> "amixer contents" or listeners on legacy /dev/input actually
>>>>>>> powers up the chip.)
>>>>>>
>>>>>> The problem is that the codec resume is always performed no matter
>>>>>> whether the device is used or not.  This was different in the past
>>>>>> but it's been changed in that way because there are cases where
>>>>>> you need to initialize the hardware properly once (e.g. mute-LED
>> toggling).
>>>>>>
>>>>>>
>>>>>>>>>> Second, I just saw on a machine we're working on, the
>>>>>>>>>> symptoms: one of the pins in D3 and the right channel muted.
>>>>>>>>>> And this was on a clean boot, not after S3 suspend/resume cycle.
>>>>>>>>>
>>>>>>>>> Maybe another problem.  Maybe not.  Does the problem persist if
>>>>>>>>> you reload the driver (without invocation of alsactl via udev)?
>>>>>>>>
>>>>>>>> Hi Takashi and David,
>>>>>>>>
>>>>>>>> This is the same dependency issue as after system suspend/resume
>> cycles, with same phenomenon.
>>>>>>>> I duplicated this error if I boot without an HDMI/DP cable connected
>> and connect the cable later.
>>>>>>>> Maybe we need to delay codec operations on initialization like in
>> resume case.
>>>>>>>
>>>>>>> So it's a problem both on hotplug and S3...
>>>>>>
>>>>>> In the case of hotplug, you don't hit the inconsistent power sate
>>>>>> we're trying to solve.  The graphics has been already powered up.
>>>>>>
>>>>>>
>>>>>>>>>> To look at the problem again: If the problem is that something
>>>>>>>>>> must be done on the graphics driver side first and then on the
>>>>>>>>>> audio side, wouldn't the solution be for the video driver and
>>>>>>>>>> audio driver to communicate somehow?
>>>>>>>>>
>>>>>>>>> Yes, the pm domain support is necessary sooner or later, as I
>>>>>>>>> already discussed shortly with Dave Airlie.  However...
>>>>>>>>
>>>>>>>> Liam will propose Gfx driver team to implement a pm domain.
>>>>>>>>
>>>>>>>>>> And resume (and possibly init?) would not complete until first
>>>>>>>>>> the graphics driver has done its resume, and after that, the audio
>> driver.
>>>>>>>>>> I e, userspace will remain frozen until both drivers have
>>>>>>>>>> completed a correct resume.
>>>>>>>>>
>>>>>>>>> ... the resume problem can't be fixed only by the usual pm domain
>> serialization.
>>>>>>>>> The graphics initialization for the audio bit is done
>>>>>>>>> asynchronously, thus we can't guarantee the audio codec is
>>>>>>>>> really ready after the graphics driver returns from the resume
>> callback.
>>>>>>>>> It shows as if it's ready (you can turn it to D0) but actually
>>>>>>>>> it doesn't change on the hardware.  Thus, the serialization
>>>>>>>>> with an unsol event wait seems mandatory for the time being.
>>>>>>>>>
>>>>>>>> It seems so. We have not found better solution now :-( I've
>>>>>>>> revised the patch for system suspend/resume case. Please review.
>>>>>>>
>>>>>>> If this problem can be detected by looking at the pin and finding
>>>>>>> that it is in D3,
>>>>>>
>>>>>> No, you can't see that it's D3.  The controller chip _shows_ it's
>>>>>> in
>>>>>> D0 while it's actually in D3.  That's the problem.
>>>>>>
>>>>>> So, this is actually a hardware design bug.  But we need to live
>>>>>> with that.
>>>>>
>>>>> Then I'm not sure it's the same problem; because for me I can see
>>>>> the D3 in the codec proc output.
>>>>
>>>> I suppose you didn't set the powersave for it, right?
>>>
>>> AFAIK, powersave remains at upstream default.
>>
>> The option is overridden by user-space, so checking only the default value
>> doesn't make any sense :)
>>
>>>> BTW, the main problem was about the FG node, which you can't judge
>>>> from the proc output, unfortunately.
>>>
>>> Here's what it looks like for me. Right channel muted and pin in D3.
>>>
>>> Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP
>>>     Control: name="HDMI/DP,pcm=3 Jack", index=0, device=0
>>>     Control: name="IEC958 Playback Con Mask", index=0, device=0
>>>     Control: name="IEC958 Playback Pro Mask", index=0, device=0
>>>     Control: name="IEC958 Playback Default", index=0, device=0
>>>     Control: name="IEC958 Playback Switch", index=0, device=0
>>>     Control: name="ELD", index=0, device=3
>>>     Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
>>>     Amp-Out vals:  [0x00 0x80]
>>>     Pincap 0x0b000094: OUT Detect HBR HDMI DP
>>>     Pin Default 0x18560010: [Jack] Digital Out at Int HDMI
>>>       Conn = Digital, Color = Unknown
>>>       DefAssociation = 0x1, Sequence = 0x0
>>>     Pin-ctls: 0x40: OUT
>>>     Unsolicited: tag=01, enabled=1
>>>     Power states:  D0 D3 EPSS
>>>     Power: setting=D3, actual=D3
>>>     Connection: 3
>>>        0x02* 0x03 0x04
>>
>> Then this might be the different issue.
>
> This is same as what I observed after S3 or connect HDMI/DP cable after boot, on machines with power_save is '0'.
> Amp-Out vals:  [0x00 0x80]
> Power: setting=D3, actual=D3
>
> If we set the pin state to D0 and unmute the pin again, HDMI can work. So I still think this is also caused by dependency on Gfx.
>
> But the controller cannot find the error when it programs the codec to D0
> -  when audio driver programs the codec and pins to D0, HW does not return error
> -  I also added code to check their power state immediately after setting state to D0, HW reports their state did change to D0.
> However, after Gfx driver finishes setting up the display pipeline and enabling audio, the pin's state changed to D3 and get muted, as shown by /proc.
> It seems to me that Gfx HW initialization can override some audio driver settings. So we choose to delay resuming the codec until the unsol event which indicates Gfx is ready.

I've been answering the power_save questions a little bit uncertain so 
far, and that's because I don't really know. I've not investigated power 
management much.

But the person with the hardware reports that this problem happens when 
on AC power only. And that makes sense with your observations about 
power_save - there could very well be something that turns power_save 
off when on AC power and on when on battery power.

So, if it's just a matter of re-initializing the pin, what do you think 
about this solution:

  - Resume as normal (this will enable unsol events)
  - Any time an unsol event comes in, have a haswell specific function 
that checks relevant pins to see that is still in D0 and mute state 
correct. If not, correct it.
  - This check could possibly also be done in the prepare hook for extra 
safety.

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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 09 Apr 2013 08:45:32 +0200,
David Henningsson wrote:
> 
> On 04/08/2013 05:47 PM, Lin, Mengdong wrote:
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai@suse.de]
> >> Sent: Monday, April 08, 2013 9:50 PM
> >> To: David Henningsson
> >> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >> in system resume
> >>
> >> At Mon, 08 Apr 2013 15:30:25 +0200,
> >> David Henningsson wrote:
> >>>
> >>> On 04/08/2013 02:23 PM, Takashi Iwai wrote:
> >>>> At Mon, 08 Apr 2013 14:20:28 +0200,
> >>>> David Henningsson wrote:
> >>>>>
> >>>>> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
> >>>>>> At Mon, 08 Apr 2013 13:35:25 +0200, David Henningsson wrote:
> >>>>>>>
> >>>>>>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>>>>>>> Sent: Monday, April 08, 2013 6:24 PM
> >>>>>>>>> To: David Henningsson
> >>>>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> >>>>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume
> >>>>>>>>> haswell hdmi codec in system resume
> >>>>>>>>>
> >>>>>>>>> At Mon, 08 Apr 2013 11:49:09 +0200, David Henningsson wrote:
> >>>>>>>>>>
> >>>>>>>>>> I'm still not understanding this patch.
> >>>>>>>>>>
> >>>>>>>>>> We of course cannot have a situation where HDMI jack detection
> >>>>>>>>>> is not correctly working after S3, which looks like would be the case
> >> here?
> >>>>>>>>>
> >>>>>>>>> No.  The patch itself has nothing to do with the HDMI jack detection
> >> at all.
> >>>>>>>>> This intrinsic unsol event is (according to Intel) guaranteed
> >>>>>>>>> to be issued once at the audio codec initialization, at least for
> >> Haswell.
> >>>>>>>
> >>>>>>> Well, if no process is using the HDMI codec actively, nothing
> >>>>>>> will initialize the resume, and the HDMI codec will not
> >>>>>>> initialized at all, i e, no unsol events enabled in the first place?
> >>>>>>
> >>>>>> No, the intrinsic unsol event that is issued for this seems
> >>>>>> irrelevant from the jack state (i.e. active usage).
> >>>>>>
> >>>>>>> (The counter argument would be; if nobody uses HDMI codec, does
> >>>>>>> it matter that it is not initialized, but I'm unsure if e g
> >>>>>>> "amixer contents" or listeners on legacy /dev/input actually
> >>>>>>> powers up the chip.)
> >>>>>>
> >>>>>> The problem is that the codec resume is always performed no matter
> >>>>>> whether the device is used or not.  This was different in the past
> >>>>>> but it's been changed in that way because there are cases where
> >>>>>> you need to initialize the hardware properly once (e.g. mute-LED
> >> toggling).
> >>>>>>
> >>>>>>
> >>>>>>>>>> Second, I just saw on a machine we're working on, the
> >>>>>>>>>> symptoms: one of the pins in D3 and the right channel muted.
> >>>>>>>>>> And this was on a clean boot, not after S3 suspend/resume cycle.
> >>>>>>>>>
> >>>>>>>>> Maybe another problem.  Maybe not.  Does the problem persist if
> >>>>>>>>> you reload the driver (without invocation of alsactl via udev)?
> >>>>>>>>
> >>>>>>>> Hi Takashi and David,
> >>>>>>>>
> >>>>>>>> This is the same dependency issue as after system suspend/resume
> >> cycles, with same phenomenon.
> >>>>>>>> I duplicated this error if I boot without an HDMI/DP cable connected
> >> and connect the cable later.
> >>>>>>>> Maybe we need to delay codec operations on initialization like in
> >> resume case.
> >>>>>>>
> >>>>>>> So it's a problem both on hotplug and S3...
> >>>>>>
> >>>>>> In the case of hotplug, you don't hit the inconsistent power sate
> >>>>>> we're trying to solve.  The graphics has been already powered up.
> >>>>>>
> >>>>>>
> >>>>>>>>>> To look at the problem again: If the problem is that something
> >>>>>>>>>> must be done on the graphics driver side first and then on the
> >>>>>>>>>> audio side, wouldn't the solution be for the video driver and
> >>>>>>>>>> audio driver to communicate somehow?
> >>>>>>>>>
> >>>>>>>>> Yes, the pm domain support is necessary sooner or later, as I
> >>>>>>>>> already discussed shortly with Dave Airlie.  However...
> >>>>>>>>
> >>>>>>>> Liam will propose Gfx driver team to implement a pm domain.
> >>>>>>>>
> >>>>>>>>>> And resume (and possibly init?) would not complete until first
> >>>>>>>>>> the graphics driver has done its resume, and after that, the audio
> >> driver.
> >>>>>>>>>> I e, userspace will remain frozen until both drivers have
> >>>>>>>>>> completed a correct resume.
> >>>>>>>>>
> >>>>>>>>> ... the resume problem can't be fixed only by the usual pm domain
> >> serialization.
> >>>>>>>>> The graphics initialization for the audio bit is done
> >>>>>>>>> asynchronously, thus we can't guarantee the audio codec is
> >>>>>>>>> really ready after the graphics driver returns from the resume
> >> callback.
> >>>>>>>>> It shows as if it's ready (you can turn it to D0) but actually
> >>>>>>>>> it doesn't change on the hardware.  Thus, the serialization
> >>>>>>>>> with an unsol event wait seems mandatory for the time being.
> >>>>>>>>>
> >>>>>>>> It seems so. We have not found better solution now :-( I've
> >>>>>>>> revised the patch for system suspend/resume case. Please review.
> >>>>>>>
> >>>>>>> If this problem can be detected by looking at the pin and finding
> >>>>>>> that it is in D3,
> >>>>>>
> >>>>>> No, you can't see that it's D3.  The controller chip _shows_ it's
> >>>>>> in
> >>>>>> D0 while it's actually in D3.  That's the problem.
> >>>>>>
> >>>>>> So, this is actually a hardware design bug.  But we need to live
> >>>>>> with that.
> >>>>>
> >>>>> Then I'm not sure it's the same problem; because for me I can see
> >>>>> the D3 in the codec proc output.
> >>>>
> >>>> I suppose you didn't set the powersave for it, right?
> >>>
> >>> AFAIK, powersave remains at upstream default.
> >>
> >> The option is overridden by user-space, so checking only the default value
> >> doesn't make any sense :)
> >>
> >>>> BTW, the main problem was about the FG node, which you can't judge
> >>>> from the proc output, unfortunately.
> >>>
> >>> Here's what it looks like for me. Right channel muted and pin in D3.
> >>>
> >>> Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP
> >>>     Control: name="HDMI/DP,pcm=3 Jack", index=0, device=0
> >>>     Control: name="IEC958 Playback Con Mask", index=0, device=0
> >>>     Control: name="IEC958 Playback Pro Mask", index=0, device=0
> >>>     Control: name="IEC958 Playback Default", index=0, device=0
> >>>     Control: name="IEC958 Playback Switch", index=0, device=0
> >>>     Control: name="ELD", index=0, device=3
> >>>     Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
> >>>     Amp-Out vals:  [0x00 0x80]
> >>>     Pincap 0x0b000094: OUT Detect HBR HDMI DP
> >>>     Pin Default 0x18560010: [Jack] Digital Out at Int HDMI
> >>>       Conn = Digital, Color = Unknown
> >>>       DefAssociation = 0x1, Sequence = 0x0
> >>>     Pin-ctls: 0x40: OUT
> >>>     Unsolicited: tag=01, enabled=1
> >>>     Power states:  D0 D3 EPSS
> >>>     Power: setting=D3, actual=D3
> >>>     Connection: 3
> >>>        0x02* 0x03 0x04
> >>
> >> Then this might be the different issue.
> >
> > This is same as what I observed after S3 or connect HDMI/DP cable after boot, on machines with power_save is '0'.
> > Amp-Out vals:  [0x00 0x80]
> > Power: setting=D3, actual=D3
> >
> > If we set the pin state to D0 and unmute the pin again, HDMI can work. So I still think this is also caused by dependency on Gfx.
> >
> > But the controller cannot find the error when it programs the codec to D0
> > -  when audio driver programs the codec and pins to D0, HW does not return error
> > -  I also added code to check their power state immediately after setting state to D0, HW reports their state did change to D0.
> > However, after Gfx driver finishes setting up the display pipeline and enabling audio, the pin's state changed to D3 and get muted, as shown by /proc.
> > It seems to me that Gfx HW initialization can override some audio driver settings. So we choose to delay resuming the codec until the unsol event which indicates Gfx is ready.
> 
> I've been answering the power_save questions a little bit uncertain so 
> far, and that's because I don't really know. I've not investigated power 
> management much.
> 
> But the person with the hardware reports that this problem happens when 
> on AC power only. And that makes sense with your observations about 
> power_save - there could very well be something that turns power_save 
> off when on AC power and on when on battery power.
> 
> So, if it's just a matter of re-initializing the pin, what do you think 
> about this solution:
> 
>   - Resume as normal (this will enable unsol events)
>   - Any time an unsol event comes in, have a haswell specific function 
> that checks relevant pins to see that is still in D0 and mute state 
> correct. If not, correct it.
>   - This check could possibly also be done in the prepare hook for extra 
> safety.

It won't work reliably.  The problem is that the codec might be set up
wrongly if it's used before the unsol event comes up.  For example, a
PCM resume may be performed immediately after the normal resume is
finished.  Meanwhile the first unsol may arrive much later than that.

Thus we need to delay the resume operation in anyway.  And if so,
there is no merit to perform the normal resume operation at the first
step.


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/09/2013 08:53 AM, Takashi Iwai wrote:
> At Tue, 09 Apr 2013 08:45:32 +0200,
> David Henningsson wrote:
>>
>> On 04/08/2013 05:47 PM, Lin, Mengdong wrote:
>>>> -----Original Message-----
>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>> Sent: Monday, April 08, 2013 9:50 PM
>>>> To: David Henningsson
>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>>>> in system resume
>>>>
>>>> At Mon, 08 Apr 2013 15:30:25 +0200,
>>>> David Henningsson wrote:
>>>>>
>>>>> On 04/08/2013 02:23 PM, Takashi Iwai wrote:
>>>>>> At Mon, 08 Apr 2013 14:20:28 +0200,
>>>>>> David Henningsson wrote:
>>>>>>>
>>>>>>> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
>>>>>>>> At Mon, 08 Apr 2013 13:35:25 +0200, David Henningsson wrote:
>>>>>>>>>
>>>>>>>>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>>>>>>>> Sent: Monday, April 08, 2013 6:24 PM
>>>>>>>>>>> To: David Henningsson
>>>>>>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
>>>>>>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume
>>>>>>>>>>> haswell hdmi codec in system resume
>>>>>>>>>>>
>>>>>>>>>>> At Mon, 08 Apr 2013 11:49:09 +0200, David Henningsson wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> I'm still not understanding this patch.
>>>>>>>>>>>>
>>>>>>>>>>>> We of course cannot have a situation where HDMI jack detection
>>>>>>>>>>>> is not correctly working after S3, which looks like would be the case
>>>> here?
>>>>>>>>>>>
>>>>>>>>>>> No.  The patch itself has nothing to do with the HDMI jack detection
>>>> at all.
>>>>>>>>>>> This intrinsic unsol event is (according to Intel) guaranteed
>>>>>>>>>>> to be issued once at the audio codec initialization, at least for
>>>> Haswell.
>>>>>>>>>
>>>>>>>>> Well, if no process is using the HDMI codec actively, nothing
>>>>>>>>> will initialize the resume, and the HDMI codec will not
>>>>>>>>> initialized at all, i e, no unsol events enabled in the first place?
>>>>>>>>
>>>>>>>> No, the intrinsic unsol event that is issued for this seems
>>>>>>>> irrelevant from the jack state (i.e. active usage).
>>>>>>>>
>>>>>>>>> (The counter argument would be; if nobody uses HDMI codec, does
>>>>>>>>> it matter that it is not initialized, but I'm unsure if e g
>>>>>>>>> "amixer contents" or listeners on legacy /dev/input actually
>>>>>>>>> powers up the chip.)
>>>>>>>>
>>>>>>>> The problem is that the codec resume is always performed no matter
>>>>>>>> whether the device is used or not.  This was different in the past
>>>>>>>> but it's been changed in that way because there are cases where
>>>>>>>> you need to initialize the hardware properly once (e.g. mute-LED
>>>> toggling).
>>>>>>>>
>>>>>>>>
>>>>>>>>>>>> Second, I just saw on a machine we're working on, the
>>>>>>>>>>>> symptoms: one of the pins in D3 and the right channel muted.
>>>>>>>>>>>> And this was on a clean boot, not after S3 suspend/resume cycle.
>>>>>>>>>>>
>>>>>>>>>>> Maybe another problem.  Maybe not.  Does the problem persist if
>>>>>>>>>>> you reload the driver (without invocation of alsactl via udev)?
>>>>>>>>>>
>>>>>>>>>> Hi Takashi and David,
>>>>>>>>>>
>>>>>>>>>> This is the same dependency issue as after system suspend/resume
>>>> cycles, with same phenomenon.
>>>>>>>>>> I duplicated this error if I boot without an HDMI/DP cable connected
>>>> and connect the cable later.
>>>>>>>>>> Maybe we need to delay codec operations on initialization like in
>>>> resume case.
>>>>>>>>>
>>>>>>>>> So it's a problem both on hotplug and S3...
>>>>>>>>
>>>>>>>> In the case of hotplug, you don't hit the inconsistent power sate
>>>>>>>> we're trying to solve.  The graphics has been already powered up.
>>>>>>>>
>>>>>>>>
>>>>>>>>>>>> To look at the problem again: If the problem is that something
>>>>>>>>>>>> must be done on the graphics driver side first and then on the
>>>>>>>>>>>> audio side, wouldn't the solution be for the video driver and
>>>>>>>>>>>> audio driver to communicate somehow?
>>>>>>>>>>>
>>>>>>>>>>> Yes, the pm domain support is necessary sooner or later, as I
>>>>>>>>>>> already discussed shortly with Dave Airlie.  However...
>>>>>>>>>>
>>>>>>>>>> Liam will propose Gfx driver team to implement a pm domain.
>>>>>>>>>>
>>>>>>>>>>>> And resume (and possibly init?) would not complete until first
>>>>>>>>>>>> the graphics driver has done its resume, and after that, the audio
>>>> driver.
>>>>>>>>>>>> I e, userspace will remain frozen until both drivers have
>>>>>>>>>>>> completed a correct resume.
>>>>>>>>>>>
>>>>>>>>>>> ... the resume problem can't be fixed only by the usual pm domain
>>>> serialization.
>>>>>>>>>>> The graphics initialization for the audio bit is done
>>>>>>>>>>> asynchronously, thus we can't guarantee the audio codec is
>>>>>>>>>>> really ready after the graphics driver returns from the resume
>>>> callback.
>>>>>>>>>>> It shows as if it's ready (you can turn it to D0) but actually
>>>>>>>>>>> it doesn't change on the hardware.  Thus, the serialization
>>>>>>>>>>> with an unsol event wait seems mandatory for the time being.
>>>>>>>>>>>
>>>>>>>>>> It seems so. We have not found better solution now :-( I've
>>>>>>>>>> revised the patch for system suspend/resume case. Please review.
>>>>>>>>>
>>>>>>>>> If this problem can be detected by looking at the pin and finding
>>>>>>>>> that it is in D3,
>>>>>>>>
>>>>>>>> No, you can't see that it's D3.  The controller chip _shows_ it's
>>>>>>>> in
>>>>>>>> D0 while it's actually in D3.  That's the problem.
>>>>>>>>
>>>>>>>> So, this is actually a hardware design bug.  But we need to live
>>>>>>>> with that.
>>>>>>>
>>>>>>> Then I'm not sure it's the same problem; because for me I can see
>>>>>>> the D3 in the codec proc output.
>>>>>>
>>>>>> I suppose you didn't set the powersave for it, right?
>>>>>
>>>>> AFAIK, powersave remains at upstream default.
>>>>
>>>> The option is overridden by user-space, so checking only the default value
>>>> doesn't make any sense :)
>>>>
>>>>>> BTW, the main problem was about the FG node, which you can't judge
>>>>>> from the proc output, unfortunately.
>>>>>
>>>>> Here's what it looks like for me. Right channel muted and pin in D3.
>>>>>
>>>>> Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP
>>>>>      Control: name="HDMI/DP,pcm=3 Jack", index=0, device=0
>>>>>      Control: name="IEC958 Playback Con Mask", index=0, device=0
>>>>>      Control: name="IEC958 Playback Pro Mask", index=0, device=0
>>>>>      Control: name="IEC958 Playback Default", index=0, device=0
>>>>>      Control: name="IEC958 Playback Switch", index=0, device=0
>>>>>      Control: name="ELD", index=0, device=3
>>>>>      Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
>>>>>      Amp-Out vals:  [0x00 0x80]
>>>>>      Pincap 0x0b000094: OUT Detect HBR HDMI DP
>>>>>      Pin Default 0x18560010: [Jack] Digital Out at Int HDMI
>>>>>        Conn = Digital, Color = Unknown
>>>>>        DefAssociation = 0x1, Sequence = 0x0
>>>>>      Pin-ctls: 0x40: OUT
>>>>>      Unsolicited: tag=01, enabled=1
>>>>>      Power states:  D0 D3 EPSS
>>>>>      Power: setting=D3, actual=D3
>>>>>      Connection: 3
>>>>>         0x02* 0x03 0x04
>>>>
>>>> Then this might be the different issue.
>>>
>>> This is same as what I observed after S3 or connect HDMI/DP cable after boot, on machines with power_save is '0'.
>>> Amp-Out vals:  [0x00 0x80]
>>> Power: setting=D3, actual=D3
>>>
>>> If we set the pin state to D0 and unmute the pin again, HDMI can work. So I still think this is also caused by dependency on Gfx.
>>>
>>> But the controller cannot find the error when it programs the codec to D0
>>> -  when audio driver programs the codec and pins to D0, HW does not return error
>>> -  I also added code to check their power state immediately after setting state to D0, HW reports their state did change to D0.
>>> However, after Gfx driver finishes setting up the display pipeline and enabling audio, the pin's state changed to D3 and get muted, as shown by /proc.
>>> It seems to me that Gfx HW initialization can override some audio driver settings. So we choose to delay resuming the codec until the unsol event which indicates Gfx is ready.
>>
>> I've been answering the power_save questions a little bit uncertain so
>> far, and that's because I don't really know. I've not investigated power
>> management much.
>>
>> But the person with the hardware reports that this problem happens when
>> on AC power only. And that makes sense with your observations about
>> power_save - there could very well be something that turns power_save
>> off when on AC power and on when on battery power.
>>
>> So, if it's just a matter of re-initializing the pin, what do you think
>> about this solution:
>>
>>    - Resume as normal (this will enable unsol events)
>>    - Any time an unsol event comes in, have a haswell specific function
>> that checks relevant pins to see that is still in D0 and mute state
>> correct. If not, correct it.
>>    - This check could possibly also be done in the prepare hook for extra
>> safety.
>
> It won't work reliably.  The problem is that the codec might be set up
> wrongly if it's used before the unsol event comes up.  For example, a
> PCM resume may be performed immediately after the normal resume is
> finished.  Meanwhile the first unsol may arrive much later than that.

Right, but if this will only mean a few samples (max 300 ms?) will never 
reach the HDMI cable, it might be the least bad workaround.

> Thus we need to delay the resume operation in anyway.  And if so,
> there is no merit to perform the normal resume operation at the first
> step.

The normal resume operation enables unsol events, which is important, 
otherwise we don't get the unsol notification from when the gfx pipeline 
has finished.

I guess the question here is how much of the codec setup is really 
destroyed by the gfx driver? Is it just a pin in D3 and a right channel 
mute, or is it much more that needs to be re-done after the gfx driver 
has finished?


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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 09 Apr 2013 09:10:27 +0200,
David Henningsson wrote:
> 
> On 04/09/2013 08:53 AM, Takashi Iwai wrote:
> > At Tue, 09 Apr 2013 08:45:32 +0200,
> > David Henningsson wrote:
> >>
> >> On 04/08/2013 05:47 PM, Lin, Mengdong wrote:
> >>>> -----Original Message-----
> >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>> Sent: Monday, April 08, 2013 9:50 PM
> >>>> To: David Henningsson
> >>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> >>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >>>> in system resume
> >>>>
> >>>> At Mon, 08 Apr 2013 15:30:25 +0200,
> >>>> David Henningsson wrote:
> >>>>>
> >>>>> On 04/08/2013 02:23 PM, Takashi Iwai wrote:
> >>>>>> At Mon, 08 Apr 2013 14:20:28 +0200,
> >>>>>> David Henningsson wrote:
> >>>>>>>
> >>>>>>> On 04/08/2013 01:59 PM, Takashi Iwai wrote:
> >>>>>>>> At Mon, 08 Apr 2013 13:35:25 +0200, David Henningsson wrote:
> >>>>>>>>>
> >>>>>>>>> On 04/08/2013 01:06 PM, Lin, Mengdong wrote:
> >>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>>>>>>>>> Sent: Monday, April 08, 2013 6:24 PM
> >>>>>>>>>>> To: David Henningsson
> >>>>>>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org
> >>>>>>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume
> >>>>>>>>>>> haswell hdmi codec in system resume
> >>>>>>>>>>>
> >>>>>>>>>>> At Mon, 08 Apr 2013 11:49:09 +0200, David Henningsson wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'm still not understanding this patch.
> >>>>>>>>>>>>
> >>>>>>>>>>>> We of course cannot have a situation where HDMI jack detection
> >>>>>>>>>>>> is not correctly working after S3, which looks like would be the case
> >>>> here?
> >>>>>>>>>>>
> >>>>>>>>>>> No.  The patch itself has nothing to do with the HDMI jack detection
> >>>> at all.
> >>>>>>>>>>> This intrinsic unsol event is (according to Intel) guaranteed
> >>>>>>>>>>> to be issued once at the audio codec initialization, at least for
> >>>> Haswell.
> >>>>>>>>>
> >>>>>>>>> Well, if no process is using the HDMI codec actively, nothing
> >>>>>>>>> will initialize the resume, and the HDMI codec will not
> >>>>>>>>> initialized at all, i e, no unsol events enabled in the first place?
> >>>>>>>>
> >>>>>>>> No, the intrinsic unsol event that is issued for this seems
> >>>>>>>> irrelevant from the jack state (i.e. active usage).
> >>>>>>>>
> >>>>>>>>> (The counter argument would be; if nobody uses HDMI codec, does
> >>>>>>>>> it matter that it is not initialized, but I'm unsure if e g
> >>>>>>>>> "amixer contents" or listeners on legacy /dev/input actually
> >>>>>>>>> powers up the chip.)
> >>>>>>>>
> >>>>>>>> The problem is that the codec resume is always performed no matter
> >>>>>>>> whether the device is used or not.  This was different in the past
> >>>>>>>> but it's been changed in that way because there are cases where
> >>>>>>>> you need to initialize the hardware properly once (e.g. mute-LED
> >>>> toggling).
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>>>> Second, I just saw on a machine we're working on, the
> >>>>>>>>>>>> symptoms: one of the pins in D3 and the right channel muted.
> >>>>>>>>>>>> And this was on a clean boot, not after S3 suspend/resume cycle.
> >>>>>>>>>>>
> >>>>>>>>>>> Maybe another problem.  Maybe not.  Does the problem persist if
> >>>>>>>>>>> you reload the driver (without invocation of alsactl via udev)?
> >>>>>>>>>>
> >>>>>>>>>> Hi Takashi and David,
> >>>>>>>>>>
> >>>>>>>>>> This is the same dependency issue as after system suspend/resume
> >>>> cycles, with same phenomenon.
> >>>>>>>>>> I duplicated this error if I boot without an HDMI/DP cable connected
> >>>> and connect the cable later.
> >>>>>>>>>> Maybe we need to delay codec operations on initialization like in
> >>>> resume case.
> >>>>>>>>>
> >>>>>>>>> So it's a problem both on hotplug and S3...
> >>>>>>>>
> >>>>>>>> In the case of hotplug, you don't hit the inconsistent power sate
> >>>>>>>> we're trying to solve.  The graphics has been already powered up.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>>>> To look at the problem again: If the problem is that something
> >>>>>>>>>>>> must be done on the graphics driver side first and then on the
> >>>>>>>>>>>> audio side, wouldn't the solution be for the video driver and
> >>>>>>>>>>>> audio driver to communicate somehow?
> >>>>>>>>>>>
> >>>>>>>>>>> Yes, the pm domain support is necessary sooner or later, as I
> >>>>>>>>>>> already discussed shortly with Dave Airlie.  However...
> >>>>>>>>>>
> >>>>>>>>>> Liam will propose Gfx driver team to implement a pm domain.
> >>>>>>>>>>
> >>>>>>>>>>>> And resume (and possibly init?) would not complete until first
> >>>>>>>>>>>> the graphics driver has done its resume, and after that, the audio
> >>>> driver.
> >>>>>>>>>>>> I e, userspace will remain frozen until both drivers have
> >>>>>>>>>>>> completed a correct resume.
> >>>>>>>>>>>
> >>>>>>>>>>> ... the resume problem can't be fixed only by the usual pm domain
> >>>> serialization.
> >>>>>>>>>>> The graphics initialization for the audio bit is done
> >>>>>>>>>>> asynchronously, thus we can't guarantee the audio codec is
> >>>>>>>>>>> really ready after the graphics driver returns from the resume
> >>>> callback.
> >>>>>>>>>>> It shows as if it's ready (you can turn it to D0) but actually
> >>>>>>>>>>> it doesn't change on the hardware.  Thus, the serialization
> >>>>>>>>>>> with an unsol event wait seems mandatory for the time being.
> >>>>>>>>>>>
> >>>>>>>>>> It seems so. We have not found better solution now :-( I've
> >>>>>>>>>> revised the patch for system suspend/resume case. Please review.
> >>>>>>>>>
> >>>>>>>>> If this problem can be detected by looking at the pin and finding
> >>>>>>>>> that it is in D3,
> >>>>>>>>
> >>>>>>>> No, you can't see that it's D3.  The controller chip _shows_ it's
> >>>>>>>> in
> >>>>>>>> D0 while it's actually in D3.  That's the problem.
> >>>>>>>>
> >>>>>>>> So, this is actually a hardware design bug.  But we need to live
> >>>>>>>> with that.
> >>>>>>>
> >>>>>>> Then I'm not sure it's the same problem; because for me I can see
> >>>>>>> the D3 in the codec proc output.
> >>>>>>
> >>>>>> I suppose you didn't set the powersave for it, right?
> >>>>>
> >>>>> AFAIK, powersave remains at upstream default.
> >>>>
> >>>> The option is overridden by user-space, so checking only the default value
> >>>> doesn't make any sense :)
> >>>>
> >>>>>> BTW, the main problem was about the FG node, which you can't judge
> >>>>>> from the proc output, unfortunately.
> >>>>>
> >>>>> Here's what it looks like for me. Right channel muted and pin in D3.
> >>>>>
> >>>>> Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP
> >>>>>      Control: name="HDMI/DP,pcm=3 Jack", index=0, device=0
> >>>>>      Control: name="IEC958 Playback Con Mask", index=0, device=0
> >>>>>      Control: name="IEC958 Playback Pro Mask", index=0, device=0
> >>>>>      Control: name="IEC958 Playback Default", index=0, device=0
> >>>>>      Control: name="IEC958 Playback Switch", index=0, device=0
> >>>>>      Control: name="ELD", index=0, device=3
> >>>>>      Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
> >>>>>      Amp-Out vals:  [0x00 0x80]
> >>>>>      Pincap 0x0b000094: OUT Detect HBR HDMI DP
> >>>>>      Pin Default 0x18560010: [Jack] Digital Out at Int HDMI
> >>>>>        Conn = Digital, Color = Unknown
> >>>>>        DefAssociation = 0x1, Sequence = 0x0
> >>>>>      Pin-ctls: 0x40: OUT
> >>>>>      Unsolicited: tag=01, enabled=1
> >>>>>      Power states:  D0 D3 EPSS
> >>>>>      Power: setting=D3, actual=D3
> >>>>>      Connection: 3
> >>>>>         0x02* 0x03 0x04
> >>>>
> >>>> Then this might be the different issue.
> >>>
> >>> This is same as what I observed after S3 or connect HDMI/DP cable after boot, on machines with power_save is '0'.
> >>> Amp-Out vals:  [0x00 0x80]
> >>> Power: setting=D3, actual=D3
> >>>
> >>> If we set the pin state to D0 and unmute the pin again, HDMI can work. So I still think this is also caused by dependency on Gfx.
> >>>
> >>> But the controller cannot find the error when it programs the codec to D0
> >>> -  when audio driver programs the codec and pins to D0, HW does not return error
> >>> -  I also added code to check their power state immediately after setting state to D0, HW reports their state did change to D0.
> >>> However, after Gfx driver finishes setting up the display pipeline and enabling audio, the pin's state changed to D3 and get muted, as shown by /proc.
> >>> It seems to me that Gfx HW initialization can override some audio driver settings. So we choose to delay resuming the codec until the unsol event which indicates Gfx is ready.
> >>
> >> I've been answering the power_save questions a little bit uncertain so
> >> far, and that's because I don't really know. I've not investigated power
> >> management much.
> >>
> >> But the person with the hardware reports that this problem happens when
> >> on AC power only. And that makes sense with your observations about
> >> power_save - there could very well be something that turns power_save
> >> off when on AC power and on when on battery power.
> >>
> >> So, if it's just a matter of re-initializing the pin, what do you think
> >> about this solution:
> >>
> >>    - Resume as normal (this will enable unsol events)
> >>    - Any time an unsol event comes in, have a haswell specific function
> >> that checks relevant pins to see that is still in D0 and mute state
> >> correct. If not, correct it.
> >>    - This check could possibly also be done in the prepare hook for extra
> >> safety.
> >
> > It won't work reliably.  The problem is that the codec might be set up
> > wrongly if it's used before the unsol event comes up.  For example, a
> > PCM resume may be performed immediately after the normal resume is
> > finished.  Meanwhile the first unsol may arrive much later than that.
> 
> Right, but if this will only mean a few samples (max 300 ms?) will never 
> reach the HDMI cable, it might be the least bad workaround.

It's not only about the missing few samples.  The whole setup isn't
reliable at that point, and you don't know what to re-setup for the
*running* stream.

> > Thus we need to delay the resume operation in anyway.  And if so,
> > there is no merit to perform the normal resume operation at the first
> > step.
> 
> The normal resume operation enables unsol events, which is important, 
> otherwise we don't get the unsol notification from when the gfx pipeline 
> has finished.

It seems that the intrinsic unsol event is always issued once when the
unsol event is enabled, according to Intel.  So, you won't miss it.

But heck, we need more test coverage, obviously.

> I guess the question here is how much of the codec setup is really 
> destroyed by the gfx driver? Is it just a pin in D3 and a right channel 
> mute, or is it much more that needs to be re-done after the gfx driver 
> has finished?

I'm afraid it's too native to assume that...


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, April 09, 2013 3:43 PM
> To: David Henningsson
> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume

> > >> I've been answering the power_save questions a little bit uncertain
> > >> so far, and that's because I don't really know. I've not
> > >> investigated power management much.
> > >>
> > >> But the person with the hardware reports that this problem happens
> > >> when on AC power only. And that makes sense with your observations
> > >> about power_save - there could very well be something that turns
> > >> power_save off when on AC power and on when on battery power.
> > >>
> > >> So, if it's just a matter of re-initializing the pin, what do you
> > >> think about this solution:
> > >>
> > >>    - Resume as normal (this will enable unsol events)
> > >>    - Any time an unsol event comes in, have a haswell specific
> > >> function that checks relevant pins to see that is still in D0 and
> > >> mute state correct. If not, correct it.
> > >>    - This check could possibly also be done in the prepare hook for
> > >> extra safety.
> > >
> > > It won't work reliably.  The problem is that the codec might be set
> > > up wrongly if it's used before the unsol event comes up.  For
> > > example, a PCM resume may be performed immediately after the normal
> > > resume is finished.  Meanwhile the first unsol may arrive much later than
> that.
> >
> > Right, but if this will only mean a few samples (max 300 ms?) will
> > never reach the HDMI cable, it might be the least bad workaround.

I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.

> It's not only about the missing few samples.  The whole setup isn't reliable at
> that point, and you don't know what to re-setup for the
> *running* stream.
> 
> > > Thus we need to delay the resume operation in anyway.  And if so,
> > > there is no merit to perform the normal resume operation at the
> > > first step.
> >
> > The normal resume operation enables unsol events, which is important,
> > otherwise we don't get the unsol notification from when the gfx
> > pipeline has finished.
> 
> It seems that the intrinsic unsol event is always issued once when the unsol
> event is enabled, according to Intel.  So, you won't miss it.
> 
> But heck, we need more test coverage, obviously.
> 
> > I guess the question here is how much of the codec setup is really
> > destroyed by the gfx driver? Is it just a pin in D3 and a right
> > channel mute, or is it much more that needs to be re-done after the
> > gfx driver has finished?
> 
> I'm afraid it's too native to assume that...

Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]

On 04/09/2013 10:18 AM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Tuesday, April 09, 2013 3:43 PM
>> To: David Henningsson
>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>> in system resume
>
>>>>> I've been answering the power_save questions a little bit uncertain
>>>>> so far, and that's because I don't really know. I've not
>>>>> investigated power management much.
>>>>>
>>>>> But the person with the hardware reports that this problem happens
>>>>> when on AC power only. And that makes sense with your observations
>>>>> about power_save - there could very well be something that turns
>>>>> power_save off when on AC power and on when on battery power.
>>>>>
>>>>> So, if it's just a matter of re-initializing the pin, what do you
>>>>> think about this solution:
>>>>>
>>>>>     - Resume as normal (this will enable unsol events)
>>>>>     - Any time an unsol event comes in, have a haswell specific
>>>>> function that checks relevant pins to see that is still in D0 and
>>>>> mute state correct. If not, correct it.
>>>>>     - This check could possibly also be done in the prepare hook for
>>>>> extra safety.
>>>>
>>>> It won't work reliably.  The problem is that the codec might be set
>>>> up wrongly if it's used before the unsol event comes up.  For
>>>> example, a PCM resume may be performed immediately after the normal
>>>> resume is finished.  Meanwhile the first unsol may arrive much later than
>> that.
>>>
>>> Right, but if this will only mean a few samples (max 300 ms?) will
>>> never reach the HDMI cable, it might be the least bad workaround.
>
> I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.
>
>> It's not only about the missing few samples.  The whole setup isn't reliable at
>> that point, and you don't know what to re-setup for the
>> *running* stream.
>>
>>>> Thus we need to delay the resume operation in anyway.  And if so,
>>>> there is no merit to perform the normal resume operation at the
>>>> first step.
>>>
>>> The normal resume operation enables unsol events, which is important,
>>> otherwise we don't get the unsol notification from when the gfx
>>> pipeline has finished.
>>
>> It seems that the intrinsic unsol event is always issued once when the unsol
>> event is enabled, according to Intel.  So, you won't miss it.
>>
>> But heck, we need more test coverage, obviously.
>>
>>> I guess the question here is how much of the codec setup is really
>>> destroyed by the gfx driver? Is it just a pin in D3 and a right
>>> channel mute, or is it much more that needs to be re-done after the
>>> gfx driver has finished?
>>
>> I'm afraid it's too native to assume that...
>
> Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.

I made a test patch (attached), sprinkled with printk, and asked the 
person with hw to test it. He said it fixed the issue. Now, I'm sure he 
did a simple playback test only, rather than having streams running 
during S3 or anything like that.

The patch simply sets the pin to D0 and the copies the mute value from 
the left channel to the right channel. The printk's confirmed this happened.

But at least it should show that not too much of the settings are 
destroyed...or would that be jumping to conclusions too early?

Also, a different thought: what are the possibilities that Mengdong can 
fix the gfx driver side not to destroy the settings in the first place?

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

[-- Attachment #2: hsw_d3_workaround.patch --]
[-- Type: text/x-patch, Size: 2276 bytes --]

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index ede8215..edee301 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
 		hdmi_non_intrinsic_event(codec, res);
 }
 
+static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
+{
+	int pwr, lamp, ramp;
+	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
+
+	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
+	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
+	printk("Haswell: Found pin in state D%d\n", pwr);
+	if (pwr != AC_PWRST_D0) {
+		printk("Haswell: Trying to set power to D0\n");
+		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
+				    AC_PWRST_D0);
+		msleep(40);
+		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
+		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
+		printk("Haswell: Found pin in state D%d\n", pwr);
+	}
+
+	lamp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
+	ramp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
+	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
+	if (lamp != ramp) {
+		printk("Haswell: Setting r amp mute to l amp mute\n");
+		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
+				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
+
+		lamp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
+		ramp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
+		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
+	}
+}
+
 /*
  * Callbacks
  */
@@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
 	int pinctl;
 	int new_pinctl = 0;
 
+	if (codec->vendor_id == 0x80862807)
+		haswell_verify_pin_D0(codec, pin_nid);
+
 	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
 		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
 					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Tue, 09 Apr 2013 11:15:13 +0200,
David Henningsson wrote:
> 
> [1  <text/plain; ISO-8859-1 (7bit)>]
> On 04/09/2013 10:18 AM, Lin, Mengdong wrote:
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai@suse.de]
> >> Sent: Tuesday, April 09, 2013 3:43 PM
> >> To: David Henningsson
> >> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >> in system resume
> >
> >>>>> I've been answering the power_save questions a little bit uncertain
> >>>>> so far, and that's because I don't really know. I've not
> >>>>> investigated power management much.
> >>>>>
> >>>>> But the person with the hardware reports that this problem happens
> >>>>> when on AC power only. And that makes sense with your observations
> >>>>> about power_save - there could very well be something that turns
> >>>>> power_save off when on AC power and on when on battery power.
> >>>>>
> >>>>> So, if it's just a matter of re-initializing the pin, what do you
> >>>>> think about this solution:
> >>>>>
> >>>>>     - Resume as normal (this will enable unsol events)
> >>>>>     - Any time an unsol event comes in, have a haswell specific
> >>>>> function that checks relevant pins to see that is still in D0 and
> >>>>> mute state correct. If not, correct it.
> >>>>>     - This check could possibly also be done in the prepare hook for
> >>>>> extra safety.
> >>>>
> >>>> It won't work reliably.  The problem is that the codec might be set
> >>>> up wrongly if it's used before the unsol event comes up.  For
> >>>> example, a PCM resume may be performed immediately after the normal
> >>>> resume is finished.  Meanwhile the first unsol may arrive much later than
> >> that.
> >>>
> >>> Right, but if this will only mean a few samples (max 300 ms?) will
> >>> never reach the HDMI cable, it might be the least bad workaround.
> >
> > I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.
> >
> >> It's not only about the missing few samples.  The whole setup isn't reliable at
> >> that point, and you don't know what to re-setup for the
> >> *running* stream.
> >>
> >>>> Thus we need to delay the resume operation in anyway.  And if so,
> >>>> there is no merit to perform the normal resume operation at the
> >>>> first step.
> >>>
> >>> The normal resume operation enables unsol events, which is important,
> >>> otherwise we don't get the unsol notification from when the gfx
> >>> pipeline has finished.
> >>
> >> It seems that the intrinsic unsol event is always issued once when the unsol
> >> event is enabled, according to Intel.  So, you won't miss it.
> >>
> >> But heck, we need more test coverage, obviously.
> >>
> >>> I guess the question here is how much of the codec setup is really
> >>> destroyed by the gfx driver? Is it just a pin in D3 and a right
> >>> channel mute, or is it much more that needs to be re-done after the
> >>> gfx driver has finished?
> >>
> >> I'm afraid it's too native to assume that...
> >
> > Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.
> 
> I made a test patch (attached), sprinkled with printk, and asked the 
> person with hw to test it. He said it fixed the issue. Now, I'm sure he 
> did a simple playback test only, rather than having streams running 
> during S3 or anything like that.
> 
> The patch simply sets the pin to D0 and the copies the mute value from 
> the left channel to the right channel. The printk's confirmed this happened.
> 
> But at least it should show that not too much of the settings are 
> destroyed...or would that be jumping to conclusions too early?

In your test case, the whole stream setup is called after the gfx
power up.  It's much different from the case where the stream is
resumed before the gfx power up.  This is the biggest concern.

That being said, the PCM can't be prepared before the gfx chip gets
ready.

> Also, a different thought: what are the possibilities that Mengdong can 
> fix the gfx driver side not to destroy the settings in the first place?

The serialization with the graphics side is mandatory more or less.
But it'd be good if the pm domain implementation would suffice...


Takashi

> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> [2 hsw_d3_workaround.patch <text/x-patch (7bit)>]
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index ede8215..edee301 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
>  		hdmi_non_intrinsic_event(codec, res);
>  }
>  
> +static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
> +{
> +	int pwr, lamp, ramp;
> +	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
> +
> +	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
> +	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
> +	printk("Haswell: Found pin in state D%d\n", pwr);
> +	if (pwr != AC_PWRST_D0) {
> +		printk("Haswell: Trying to set power to D0\n");
> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
> +				    AC_PWRST_D0);
> +		msleep(40);
> +		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
> +		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
> +		printk("Haswell: Found pin in state D%d\n", pwr);
> +	}
> +
> +	lamp = snd_hda_codec_read(codec, nid, 0,
> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> +	ramp = snd_hda_codec_read(codec, nid, 0,
> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> +	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
> +	if (lamp != ramp) {
> +		printk("Haswell: Setting r amp mute to l amp mute\n");
> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
> +				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
> +
> +		lamp = snd_hda_codec_read(codec, nid, 0,
> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> +		ramp = snd_hda_codec_read(codec, nid, 0,
> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> +		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
> +	}
> +}
> +
>  /*
>   * Callbacks
>   */
> @@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
>  	int pinctl;
>  	int new_pinctl = 0;
>  
> +	if (codec->vendor_id == 0x80862807)
> +		haswell_verify_pin_D0(codec, pin_nid);
> +
>  	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
>  		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
>  					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/09/2013 11:26 AM, Takashi Iwai wrote:
> At Tue, 09 Apr 2013 11:15:13 +0200,
> David Henningsson wrote:
>>
>> [1  <text/plain; ISO-8859-1 (7bit)>]
>> On 04/09/2013 10:18 AM, Lin, Mengdong wrote:
>>>> -----Original Message-----
>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>> Sent: Tuesday, April 09, 2013 3:43 PM
>>>> To: David Henningsson
>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>>>> in system resume
>>>
>>>>>>> I've been answering the power_save questions a little bit uncertain
>>>>>>> so far, and that's because I don't really know. I've not
>>>>>>> investigated power management much.
>>>>>>>
>>>>>>> But the person with the hardware reports that this problem happens
>>>>>>> when on AC power only. And that makes sense with your observations
>>>>>>> about power_save - there could very well be something that turns
>>>>>>> power_save off when on AC power and on when on battery power.
>>>>>>>
>>>>>>> So, if it's just a matter of re-initializing the pin, what do you
>>>>>>> think about this solution:
>>>>>>>
>>>>>>>      - Resume as normal (this will enable unsol events)
>>>>>>>      - Any time an unsol event comes in, have a haswell specific
>>>>>>> function that checks relevant pins to see that is still in D0 and
>>>>>>> mute state correct. If not, correct it.
>>>>>>>      - This check could possibly also be done in the prepare hook for
>>>>>>> extra safety.
>>>>>>
>>>>>> It won't work reliably.  The problem is that the codec might be set
>>>>>> up wrongly if it's used before the unsol event comes up.  For
>>>>>> example, a PCM resume may be performed immediately after the normal
>>>>>> resume is finished.  Meanwhile the first unsol may arrive much later than
>>>> that.
>>>>>
>>>>> Right, but if this will only mean a few samples (max 300 ms?) will
>>>>> never reach the HDMI cable, it might be the least bad workaround.
>>>
>>> I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.
>>>
>>>> It's not only about the missing few samples.  The whole setup isn't reliable at
>>>> that point, and you don't know what to re-setup for the
>>>> *running* stream.
>>>>
>>>>>> Thus we need to delay the resume operation in anyway.  And if so,
>>>>>> there is no merit to perform the normal resume operation at the
>>>>>> first step.
>>>>>
>>>>> The normal resume operation enables unsol events, which is important,
>>>>> otherwise we don't get the unsol notification from when the gfx
>>>>> pipeline has finished.
>>>>
>>>> It seems that the intrinsic unsol event is always issued once when the unsol
>>>> event is enabled, according to Intel.  So, you won't miss it.
>>>>
>>>> But heck, we need more test coverage, obviously.
>>>>
>>>>> I guess the question here is how much of the codec setup is really
>>>>> destroyed by the gfx driver? Is it just a pin in D3 and a right
>>>>> channel mute, or is it much more that needs to be re-done after the
>>>>> gfx driver has finished?
>>>>
>>>> I'm afraid it's too native to assume that...
>>>
>>> Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.
>>
>> I made a test patch (attached), sprinkled with printk, and asked the
>> person with hw to test it. He said it fixed the issue. Now, I'm sure he
>> did a simple playback test only, rather than having streams running
>> during S3 or anything like that.
>>
>> The patch simply sets the pin to D0 and the copies the mute value from
>> the left channel to the right channel. The printk's confirmed this happened.
>>
>> But at least it should show that not too much of the settings are
>> destroyed...or would that be jumping to conclusions too early?
>
> In your test case, the whole stream setup is called after the gfx
> power up.  It's much different from the case where the stream is
> resumed before the gfx power up.  This is the biggest concern.
>
> That being said, the PCM can't be prepared before the gfx chip gets
> ready.

Shall we have another stab at this today...

I think we need to figure out what actually happens if we do start a 
stream before gfx is initialized. Mengdong, could you give more 
information on this?

If it's something we can easily recover from (e g by just executing a 
few verbs in the unsol event handler), that might be the best option.

If it's not, then we might need the wait event to synchronise the PCM 
start with the unsol event handler. But there is still no need to delay 
rest of the chip initialization/resume for that.

But part of this is also that we're discussing two different symptoms 
here. I've only seen the D3 + right channel mute problem so far. We can 
easily recover from that, as my workaround patch shows.
Takashi was seeing the fg node being D3 (?). I have not seen that 
problem, does Mengdong know if it also exists on the latest hardware 
revision?

>
>> Also, a different thought: what are the possibilities that Mengdong can
>> fix the gfx driver side not to destroy the settings in the first place?
>
> The serialization with the graphics side is mandatory more or less.
> But it'd be good if the pm domain implementation would suffice...
>
>
> Takashi
>
>> --
>> David Henningsson, Canonical Ltd.
>> https://launchpad.net/~diwic
>> [2 hsw_d3_workaround.patch <text/x-patch (7bit)>]
>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>> index ede8215..edee301 100644
>> --- a/sound/pci/hda/patch_hdmi.c
>> +++ b/sound/pci/hda/patch_hdmi.c
>> @@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
>>   		hdmi_non_intrinsic_event(codec, res);
>>   }
>>
>> +static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
>> +{
>> +	int pwr, lamp, ramp;
>> +	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
>> +
>> +	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
>> +	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
>> +	printk("Haswell: Found pin in state D%d\n", pwr);
>> +	if (pwr != AC_PWRST_D0) {
>> +		printk("Haswell: Trying to set power to D0\n");
>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
>> +				    AC_PWRST_D0);
>> +		msleep(40);
>> +		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
>> +		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
>> +		printk("Haswell: Found pin in state D%d\n", pwr);
>> +	}
>> +
>> +	lamp = snd_hda_codec_read(codec, nid, 0,
>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
>> +	ramp = snd_hda_codec_read(codec, nid, 0,
>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
>> +	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
>> +	if (lamp != ramp) {
>> +		printk("Haswell: Setting r amp mute to l amp mute\n");
>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
>> +				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
>> +
>> +		lamp = snd_hda_codec_read(codec, nid, 0,
>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
>> +		ramp = snd_hda_codec_read(codec, nid, 0,
>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
>> +		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
>> +	}
>> +}
>> +
>>   /*
>>    * Callbacks
>>    */
>> @@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
>>   	int pinctl;
>>   	int new_pinctl = 0;
>>
>> +	if (codec->vendor_id == 0x80862807)
>> +		haswell_verify_pin_D0(codec, pin_nid);
>> +
>>   	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
>>   		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
>>   					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
>



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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Wed, 10 Apr 2013 07:29:51 +0200,
David Henningsson wrote:
> 
> On 04/09/2013 11:26 AM, Takashi Iwai wrote:
> > At Tue, 09 Apr 2013 11:15:13 +0200,
> > David Henningsson wrote:
> >>
> >> [1  <text/plain; ISO-8859-1 (7bit)>]
> >> On 04/09/2013 10:18 AM, Lin, Mengdong wrote:
> >>>> -----Original Message-----
> >>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>> Sent: Tuesday, April 09, 2013 3:43 PM
> >>>> To: David Henningsson
> >>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> >>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >>>> in system resume
> >>>
> >>>>>>> I've been answering the power_save questions a little bit uncertain
> >>>>>>> so far, and that's because I don't really know. I've not
> >>>>>>> investigated power management much.
> >>>>>>>
> >>>>>>> But the person with the hardware reports that this problem happens
> >>>>>>> when on AC power only. And that makes sense with your observations
> >>>>>>> about power_save - there could very well be something that turns
> >>>>>>> power_save off when on AC power and on when on battery power.
> >>>>>>>
> >>>>>>> So, if it's just a matter of re-initializing the pin, what do you
> >>>>>>> think about this solution:
> >>>>>>>
> >>>>>>>      - Resume as normal (this will enable unsol events)
> >>>>>>>      - Any time an unsol event comes in, have a haswell specific
> >>>>>>> function that checks relevant pins to see that is still in D0 and
> >>>>>>> mute state correct. If not, correct it.
> >>>>>>>      - This check could possibly also be done in the prepare hook for
> >>>>>>> extra safety.
> >>>>>>
> >>>>>> It won't work reliably.  The problem is that the codec might be set
> >>>>>> up wrongly if it's used before the unsol event comes up.  For
> >>>>>> example, a PCM resume may be performed immediately after the normal
> >>>>>> resume is finished.  Meanwhile the first unsol may arrive much later than
> >>>> that.
> >>>>>
> >>>>> Right, but if this will only mean a few samples (max 300 ms?) will
> >>>>> never reach the HDMI cable, it might be the least bad workaround.
> >>>
> >>> I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.
> >>>
> >>>> It's not only about the missing few samples.  The whole setup isn't reliable at
> >>>> that point, and you don't know what to re-setup for the
> >>>> *running* stream.
> >>>>
> >>>>>> Thus we need to delay the resume operation in anyway.  And if so,
> >>>>>> there is no merit to perform the normal resume operation at the
> >>>>>> first step.
> >>>>>
> >>>>> The normal resume operation enables unsol events, which is important,
> >>>>> otherwise we don't get the unsol notification from when the gfx
> >>>>> pipeline has finished.
> >>>>
> >>>> It seems that the intrinsic unsol event is always issued once when the unsol
> >>>> event is enabled, according to Intel.  So, you won't miss it.
> >>>>
> >>>> But heck, we need more test coverage, obviously.
> >>>>
> >>>>> I guess the question here is how much of the codec setup is really
> >>>>> destroyed by the gfx driver? Is it just a pin in D3 and a right
> >>>>> channel mute, or is it much more that needs to be re-done after the
> >>>>> gfx driver has finished?
> >>>>
> >>>> I'm afraid it's too native to assume that...
> >>>
> >>> Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.
> >>
> >> I made a test patch (attached), sprinkled with printk, and asked the
> >> person with hw to test it. He said it fixed the issue. Now, I'm sure he
> >> did a simple playback test only, rather than having streams running
> >> during S3 or anything like that.
> >>
> >> The patch simply sets the pin to D0 and the copies the mute value from
> >> the left channel to the right channel. The printk's confirmed this happened.
> >>
> >> But at least it should show that not too much of the settings are
> >> destroyed...or would that be jumping to conclusions too early?
> >
> > In your test case, the whole stream setup is called after the gfx
> > power up.  It's much different from the case where the stream is
> > resumed before the gfx power up.  This is the biggest concern.
> >
> > That being said, the PCM can't be prepared before the gfx chip gets
> > ready.
> 
> Shall we have another stab at this today...
> 
> I think we need to figure out what actually happens if we do start a 
> stream before gfx is initialized. Mengdong, could you give more 
> information on this?
> 
> If it's something we can easily recover from (e g by just executing a 
> few verbs in the unsol event handler), that might be the best option.
> 
> If it's not, then we might need the wait event to synchronise the PCM 
> start with the unsol event handler. But there is still no need to delay 
> rest of the chip initialization/resume for that.

.... if the graphics / audio resume serialization is implemented.
Currently not, so we take a brute-force way.

> But part of this is also that we're discussing two different symptoms 
> here. I've only seen the D3 + right channel mute problem so far. We can 
> easily recover from that, as my workaround patch shows.
> Takashi was seeing the fg node being D3 (?). I have not seen that 
> problem, does Mengdong know if it also exists on the latest hardware 
> revision?

As mentioned, you can't see it from user-space.  If you access, the
codec is always woken up.


Actually, I prefer the delayed resume to band-aiding the broken setup
afterwards.  The delayed resume mechanism was proved to work well for
long time (it was so as default).  We switched to the full resume just
because of a few other devices.

So, the biggest concern in the scheme Mengdong suggested is only about
the reliability of the unsol event.


Takashi

> 
> >
> >> Also, a different thought: what are the possibilities that Mengdong can
> >> fix the gfx driver side not to destroy the settings in the first place?
> >
> > The serialization with the graphics side is mandatory more or less.
> > But it'd be good if the pm domain implementation would suffice...
> >
> >
> > Takashi
> >
> >> --
> >> David Henningsson, Canonical Ltd.
> >> https://launchpad.net/~diwic
> >> [2 hsw_d3_workaround.patch <text/x-patch (7bit)>]
> >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> >> index ede8215..edee301 100644
> >> --- a/sound/pci/hda/patch_hdmi.c
> >> +++ b/sound/pci/hda/patch_hdmi.c
> >> @@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
> >>   		hdmi_non_intrinsic_event(codec, res);
> >>   }
> >>
> >> +static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
> >> +{
> >> +	int pwr, lamp, ramp;
> >> +	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
> >> +
> >> +	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
> >> +	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
> >> +	printk("Haswell: Found pin in state D%d\n", pwr);
> >> +	if (pwr != AC_PWRST_D0) {
> >> +		printk("Haswell: Trying to set power to D0\n");
> >> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
> >> +				    AC_PWRST_D0);
> >> +		msleep(40);
> >> +		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
> >> +		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
> >> +		printk("Haswell: Found pin in state D%d\n", pwr);
> >> +	}
> >> +
> >> +	lamp = snd_hda_codec_read(codec, nid, 0,
> >> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> >> +	ramp = snd_hda_codec_read(codec, nid, 0,
> >> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> >> +	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
> >> +	if (lamp != ramp) {
> >> +		printk("Haswell: Setting r amp mute to l amp mute\n");
> >> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
> >> +				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
> >> +
> >> +		lamp = snd_hda_codec_read(codec, nid, 0,
> >> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> >> +		ramp = snd_hda_codec_read(codec, nid, 0,
> >> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> >> +		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
> >> +	}
> >> +}
> >> +
> >>   /*
> >>    * Callbacks
> >>    */
> >> @@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
> >>   	int pinctl;
> >>   	int new_pinctl = 0;
> >>
> >> +	if (codec->vendor_id == 0x80862807)
> >> +		haswell_verify_pin_D0(codec, pin_nid);
> >> +
> >>   	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
> >>   		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> >>   					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
> >
> 
> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> 

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/10/2013 07:47 AM, Takashi Iwai wrote:
> At Wed, 10 Apr 2013 07:29:51 +0200,
> David Henningsson wrote:
>>
>> On 04/09/2013 11:26 AM, Takashi Iwai wrote:
>>> At Tue, 09 Apr 2013 11:15:13 +0200,
>>> David Henningsson wrote:
>>>>
>>>> [1  <text/plain; ISO-8859-1 (7bit)>]
>>>> On 04/09/2013 10:18 AM, Lin, Mengdong wrote:
>>>>>> -----Original Message-----
>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>>> Sent: Tuesday, April 09, 2013 3:43 PM
>>>>>> To: David Henningsson
>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>>>>>> in system resume
>>>>>
>>>>>>>>> I've been answering the power_save questions a little bit uncertain
>>>>>>>>> so far, and that's because I don't really know. I've not
>>>>>>>>> investigated power management much.
>>>>>>>>>
>>>>>>>>> But the person with the hardware reports that this problem happens
>>>>>>>>> when on AC power only. And that makes sense with your observations
>>>>>>>>> about power_save - there could very well be something that turns
>>>>>>>>> power_save off when on AC power and on when on battery power.
>>>>>>>>>
>>>>>>>>> So, if it's just a matter of re-initializing the pin, what do you
>>>>>>>>> think about this solution:
>>>>>>>>>
>>>>>>>>>       - Resume as normal (this will enable unsol events)
>>>>>>>>>       - Any time an unsol event comes in, have a haswell specific
>>>>>>>>> function that checks relevant pins to see that is still in D0 and
>>>>>>>>> mute state correct. If not, correct it.
>>>>>>>>>       - This check could possibly also be done in the prepare hook for
>>>>>>>>> extra safety.
>>>>>>>>
>>>>>>>> It won't work reliably.  The problem is that the codec might be set
>>>>>>>> up wrongly if it's used before the unsol event comes up.  For
>>>>>>>> example, a PCM resume may be performed immediately after the normal
>>>>>>>> resume is finished.  Meanwhile the first unsol may arrive much later than
>>>>>> that.
>>>>>>>
>>>>>>> Right, but if this will only mean a few samples (max 300 ms?) will
>>>>>>> never reach the HDMI cable, it might be the least bad workaround.
>>>>>
>>>>> I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.
>>>>>
>>>>>> It's not only about the missing few samples.  The whole setup isn't reliable at
>>>>>> that point, and you don't know what to re-setup for the
>>>>>> *running* stream.
>>>>>>
>>>>>>>> Thus we need to delay the resume operation in anyway.  And if so,
>>>>>>>> there is no merit to perform the normal resume operation at the
>>>>>>>> first step.
>>>>>>>
>>>>>>> The normal resume operation enables unsol events, which is important,
>>>>>>> otherwise we don't get the unsol notification from when the gfx
>>>>>>> pipeline has finished.
>>>>>>
>>>>>> It seems that the intrinsic unsol event is always issued once when the unsol
>>>>>> event is enabled, according to Intel.  So, you won't miss it.
>>>>>>
>>>>>> But heck, we need more test coverage, obviously.
>>>>>>
>>>>>>> I guess the question here is how much of the codec setup is really
>>>>>>> destroyed by the gfx driver? Is it just a pin in D3 and a right
>>>>>>> channel mute, or is it much more that needs to be re-done after the
>>>>>>> gfx driver has finished?
>>>>>>
>>>>>> I'm afraid it's too native to assume that...
>>>>>
>>>>> Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.
>>>>
>>>> I made a test patch (attached), sprinkled with printk, and asked the
>>>> person with hw to test it. He said it fixed the issue. Now, I'm sure he
>>>> did a simple playback test only, rather than having streams running
>>>> during S3 or anything like that.
>>>>
>>>> The patch simply sets the pin to D0 and the copies the mute value from
>>>> the left channel to the right channel. The printk's confirmed this happened.
>>>>
>>>> But at least it should show that not too much of the settings are
>>>> destroyed...or would that be jumping to conclusions too early?
>>>
>>> In your test case, the whole stream setup is called after the gfx
>>> power up.  It's much different from the case where the stream is
>>> resumed before the gfx power up.  This is the biggest concern.
>>>
>>> That being said, the PCM can't be prepared before the gfx chip gets
>>> ready.
>>
>> Shall we have another stab at this today...
>>
>> I think we need to figure out what actually happens if we do start a
>> stream before gfx is initialized. Mengdong, could you give more
>> information on this?
>>
>> If it's something we can easily recover from (e g by just executing a
>> few verbs in the unsol event handler), that might be the best option.
>>
>> If it's not, then we might need the wait event to synchronise the PCM
>> start with the unsol event handler. But there is still no need to delay
>> rest of the chip initialization/resume for that.
>
> .... if the graphics / audio resume serialization is implemented.
> Currently not, so we take a brute-force way.
>
>> But part of this is also that we're discussing two different symptoms
>> here. I've only seen the D3 + right channel mute problem so far. We can
>> easily recover from that, as my workaround patch shows.
>> Takashi was seeing the fg node being D3 (?). I have not seen that
>> problem, does Mengdong know if it also exists on the latest hardware
>> revision?
>
> As mentioned, you can't see it from user-space.  If you access, the
> codec is always woken up.
>
>
> Actually, I prefer the delayed resume to band-aiding the broken setup
> afterwards.  The delayed resume mechanism was proved to work well for
> long time (it was so as default).  We switched to the full resume just
> because of a few other devices.
>
> So, the biggest concern in the scheme Mengdong suggested is only about
> the reliability of the unsol event.

I'm going to make one more try to explain what I think won't work with 
this scheme.

1. System wakes up from S3 suspend/resume.
2. No initialization is performed because resume is delayed.
3. HDMI/DP cable is plugged into the system.
4. Because unsol events are not enabled (due to the lack of 
initialization), userspace is not notified that HDMI/DP cable has been 
plugged in.
5. Userspace now has the wrong idea about whether HDMI/DP cable is 
plugged in or not.


>
>
> Takashi
>
>>
>>>
>>>> Also, a different thought: what are the possibilities that Mengdong can
>>>> fix the gfx driver side not to destroy the settings in the first place?
>>>
>>> The serialization with the graphics side is mandatory more or less.
>>> But it'd be good if the pm domain implementation would suffice...
>>>
>>>
>>> Takashi
>>>
>>>> --
>>>> David Henningsson, Canonical Ltd.
>>>> https://launchpad.net/~diwic
>>>> [2 hsw_d3_workaround.patch <text/x-patch (7bit)>]
>>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>>>> index ede8215..edee301 100644
>>>> --- a/sound/pci/hda/patch_hdmi.c
>>>> +++ b/sound/pci/hda/patch_hdmi.c
>>>> @@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
>>>>    		hdmi_non_intrinsic_event(codec, res);
>>>>    }
>>>>
>>>> +static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
>>>> +{
>>>> +	int pwr, lamp, ramp;
>>>> +	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
>>>> +
>>>> +	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
>>>> +	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
>>>> +	printk("Haswell: Found pin in state D%d\n", pwr);
>>>> +	if (pwr != AC_PWRST_D0) {
>>>> +		printk("Haswell: Trying to set power to D0\n");
>>>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
>>>> +				    AC_PWRST_D0);
>>>> +		msleep(40);
>>>> +		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
>>>> +		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
>>>> +		printk("Haswell: Found pin in state D%d\n", pwr);
>>>> +	}
>>>> +
>>>> +	lamp = snd_hda_codec_read(codec, nid, 0,
>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
>>>> +	ramp = snd_hda_codec_read(codec, nid, 0,
>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
>>>> +	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
>>>> +	if (lamp != ramp) {
>>>> +		printk("Haswell: Setting r amp mute to l amp mute\n");
>>>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
>>>> +				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
>>>> +
>>>> +		lamp = snd_hda_codec_read(codec, nid, 0,
>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
>>>> +		ramp = snd_hda_codec_read(codec, nid, 0,
>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
>>>> +		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
>>>> +	}
>>>> +}
>>>> +
>>>>    /*
>>>>     * Callbacks
>>>>     */
>>>> @@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
>>>>    	int pinctl;
>>>>    	int new_pinctl = 0;
>>>>
>>>> +	if (codec->vendor_id == 0x80862807)
>>>> +		haswell_verify_pin_D0(codec, pin_nid);
>>>> +
>>>>    	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
>>>>    		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
>>>>    					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
>>>
>>
>>
>>
>> --
>> David Henningsson, Canonical Ltd.
>> https://launchpad.net/~diwic
>>
>



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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Wed, 10 Apr 2013 08:02:23 +0200,
David Henningsson wrote:
> 
> On 04/10/2013 07:47 AM, Takashi Iwai wrote:
> > At Wed, 10 Apr 2013 07:29:51 +0200,
> > David Henningsson wrote:
> >>
> >> On 04/09/2013 11:26 AM, Takashi Iwai wrote:
> >>> At Tue, 09 Apr 2013 11:15:13 +0200,
> >>> David Henningsson wrote:
> >>>>
> >>>> [1  <text/plain; ISO-8859-1 (7bit)>]
> >>>> On 04/09/2013 10:18 AM, Lin, Mengdong wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>>>> Sent: Tuesday, April 09, 2013 3:43 PM
> >>>>>> To: David Henningsson
> >>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> >>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >>>>>> in system resume
> >>>>>
> >>>>>>>>> I've been answering the power_save questions a little bit uncertain
> >>>>>>>>> so far, and that's because I don't really know. I've not
> >>>>>>>>> investigated power management much.
> >>>>>>>>>
> >>>>>>>>> But the person with the hardware reports that this problem happens
> >>>>>>>>> when on AC power only. And that makes sense with your observations
> >>>>>>>>> about power_save - there could very well be something that turns
> >>>>>>>>> power_save off when on AC power and on when on battery power.
> >>>>>>>>>
> >>>>>>>>> So, if it's just a matter of re-initializing the pin, what do you
> >>>>>>>>> think about this solution:
> >>>>>>>>>
> >>>>>>>>>       - Resume as normal (this will enable unsol events)
> >>>>>>>>>       - Any time an unsol event comes in, have a haswell specific
> >>>>>>>>> function that checks relevant pins to see that is still in D0 and
> >>>>>>>>> mute state correct. If not, correct it.
> >>>>>>>>>       - This check could possibly also be done in the prepare hook for
> >>>>>>>>> extra safety.
> >>>>>>>>
> >>>>>>>> It won't work reliably.  The problem is that the codec might be set
> >>>>>>>> up wrongly if it's used before the unsol event comes up.  For
> >>>>>>>> example, a PCM resume may be performed immediately after the normal
> >>>>>>>> resume is finished.  Meanwhile the first unsol may arrive much later than
> >>>>>> that.
> >>>>>>>
> >>>>>>> Right, but if this will only mean a few samples (max 300 ms?) will
> >>>>>>> never reach the HDMI cable, it might be the least bad workaround.
> >>>>>
> >>>>> I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.
> >>>>>
> >>>>>> It's not only about the missing few samples.  The whole setup isn't reliable at
> >>>>>> that point, and you don't know what to re-setup for the
> >>>>>> *running* stream.
> >>>>>>
> >>>>>>>> Thus we need to delay the resume operation in anyway.  And if so,
> >>>>>>>> there is no merit to perform the normal resume operation at the
> >>>>>>>> first step.
> >>>>>>>
> >>>>>>> The normal resume operation enables unsol events, which is important,
> >>>>>>> otherwise we don't get the unsol notification from when the gfx
> >>>>>>> pipeline has finished.
> >>>>>>
> >>>>>> It seems that the intrinsic unsol event is always issued once when the unsol
> >>>>>> event is enabled, according to Intel.  So, you won't miss it.
> >>>>>>
> >>>>>> But heck, we need more test coverage, obviously.
> >>>>>>
> >>>>>>> I guess the question here is how much of the codec setup is really
> >>>>>>> destroyed by the gfx driver? Is it just a pin in D3 and a right
> >>>>>>> channel mute, or is it much more that needs to be re-done after the
> >>>>>>> gfx driver has finished?
> >>>>>>
> >>>>>> I'm afraid it's too native to assume that...
> >>>>>
> >>>>> Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.
> >>>>
> >>>> I made a test patch (attached), sprinkled with printk, and asked the
> >>>> person with hw to test it. He said it fixed the issue. Now, I'm sure he
> >>>> did a simple playback test only, rather than having streams running
> >>>> during S3 or anything like that.
> >>>>
> >>>> The patch simply sets the pin to D0 and the copies the mute value from
> >>>> the left channel to the right channel. The printk's confirmed this happened.
> >>>>
> >>>> But at least it should show that not too much of the settings are
> >>>> destroyed...or would that be jumping to conclusions too early?
> >>>
> >>> In your test case, the whole stream setup is called after the gfx
> >>> power up.  It's much different from the case where the stream is
> >>> resumed before the gfx power up.  This is the biggest concern.
> >>>
> >>> That being said, the PCM can't be prepared before the gfx chip gets
> >>> ready.
> >>
> >> Shall we have another stab at this today...
> >>
> >> I think we need to figure out what actually happens if we do start a
> >> stream before gfx is initialized. Mengdong, could you give more
> >> information on this?
> >>
> >> If it's something we can easily recover from (e g by just executing a
> >> few verbs in the unsol event handler), that might be the best option.
> >>
> >> If it's not, then we might need the wait event to synchronise the PCM
> >> start with the unsol event handler. But there is still no need to delay
> >> rest of the chip initialization/resume for that.
> >
> > .... if the graphics / audio resume serialization is implemented.
> > Currently not, so we take a brute-force way.
> >
> >> But part of this is also that we're discussing two different symptoms
> >> here. I've only seen the D3 + right channel mute problem so far. We can
> >> easily recover from that, as my workaround patch shows.
> >> Takashi was seeing the fg node being D3 (?). I have not seen that
> >> problem, does Mengdong know if it also exists on the latest hardware
> >> revision?
> >
> > As mentioned, you can't see it from user-space.  If you access, the
> > codec is always woken up.
> >
> >
> > Actually, I prefer the delayed resume to band-aiding the broken setup
> > afterwards.  The delayed resume mechanism was proved to work well for
> > long time (it was so as default).  We switched to the full resume just
> > because of a few other devices.
> >
> > So, the biggest concern in the scheme Mengdong suggested is only about
> > the reliability of the unsol event.
> 
> I'm going to make one more try to explain what I think won't work with 
> this scheme.
> 
> 1. System wakes up from S3 suspend/resume.
> 2. No initialization is performed because resume is delayed.
> 3. HDMI/DP cable is plugged into the system.
> 4. Because unsol events are not enabled (due to the lack of 
> initialization), userspace is not notified that HDMI/DP cable has been 
> plugged in.
> 5. Userspace now has the wrong idea about whether HDMI/DP cable is 
> plugged in or not.

Well, we can enable only unsol event without setting to D0.
For example, instead of adding codec->support_delay_resume, add a new
codec ops, codec->patch_ops.delay_resume().  For HDMI codecs, this
callback just calls 

	spec->ready_to_resume = 0;

	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
		struct hdmi_spec_per_pin *per_pin;
		hda_nid_t pin_nid;
		struct hda_jack_tbl *jack;

		per_pin = &spec->pins[pin_idx];
		pin_nid = per_pin->pin_nid;
		jack = snd_hda_jack_tbl_get(codec, pin_nid);
		if (jack)
			snd_hda_codec_write(codec, pin_nid, 0,
				 AC_VERB_SET_UNSOLICITED_ENABLE,
				 AC_USRSP_EN | jack->tag);
	}

and returns.  The rest would work almost as is.  The pin-detection
itself should work even in D3 for this chip.


Takashi

> 
> 
> >
> >
> > Takashi
> >
> >>
> >>>
> >>>> Also, a different thought: what are the possibilities that Mengdong can
> >>>> fix the gfx driver side not to destroy the settings in the first place?
> >>>
> >>> The serialization with the graphics side is mandatory more or less.
> >>> But it'd be good if the pm domain implementation would suffice...
> >>>
> >>>
> >>> Takashi
> >>>
> >>>> --
> >>>> David Henningsson, Canonical Ltd.
> >>>> https://launchpad.net/~diwic
> >>>> [2 hsw_d3_workaround.patch <text/x-patch (7bit)>]
> >>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> >>>> index ede8215..edee301 100644
> >>>> --- a/sound/pci/hda/patch_hdmi.c
> >>>> +++ b/sound/pci/hda/patch_hdmi.c
> >>>> @@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
> >>>>    		hdmi_non_intrinsic_event(codec, res);
> >>>>    }
> >>>>
> >>>> +static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
> >>>> +{
> >>>> +	int pwr, lamp, ramp;
> >>>> +	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
> >>>> +
> >>>> +	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
> >>>> +	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
> >>>> +	printk("Haswell: Found pin in state D%d\n", pwr);
> >>>> +	if (pwr != AC_PWRST_D0) {
> >>>> +		printk("Haswell: Trying to set power to D0\n");
> >>>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
> >>>> +				    AC_PWRST_D0);
> >>>> +		msleep(40);
> >>>> +		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
> >>>> +		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
> >>>> +		printk("Haswell: Found pin in state D%d\n", pwr);
> >>>> +	}
> >>>> +
> >>>> +	lamp = snd_hda_codec_read(codec, nid, 0,
> >>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> >>>> +	ramp = snd_hda_codec_read(codec, nid, 0,
> >>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> >>>> +	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
> >>>> +	if (lamp != ramp) {
> >>>> +		printk("Haswell: Setting r amp mute to l amp mute\n");
> >>>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
> >>>> +				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
> >>>> +
> >>>> +		lamp = snd_hda_codec_read(codec, nid, 0,
> >>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> >>>> +		ramp = snd_hda_codec_read(codec, nid, 0,
> >>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> >>>> +		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
> >>>> +	}
> >>>> +}
> >>>> +
> >>>>    /*
> >>>>     * Callbacks
> >>>>     */
> >>>> @@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
> >>>>    	int pinctl;
> >>>>    	int new_pinctl = 0;
> >>>>
> >>>> +	if (codec->vendor_id == 0x80862807)
> >>>> +		haswell_verify_pin_D0(codec, pin_nid);
> >>>> +
> >>>>    	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
> >>>>    		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> >>>>    					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
> >>>
> >>
> >>
> >>
> >> --
> >> David Henningsson, Canonical Ltd.
> >> https://launchpad.net/~diwic
> >>
> >
> 
> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> 

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 11440 bytes --]

On 04/10/2013 08:52 AM, Takashi Iwai wrote:
> At Wed, 10 Apr 2013 08:02:23 +0200,
> David Henningsson wrote:
>>
>> On 04/10/2013 07:47 AM, Takashi Iwai wrote:
>>> At Wed, 10 Apr 2013 07:29:51 +0200,
>>> David Henningsson wrote:
>>>>
>>>> On 04/09/2013 11:26 AM, Takashi Iwai wrote:
>>>>> At Tue, 09 Apr 2013 11:15:13 +0200,
>>>>> David Henningsson wrote:
>>>>>>
>>>>>> [1  <text/plain; ISO-8859-1 (7bit)>]
>>>>>> On 04/09/2013 10:18 AM, Lin, Mengdong wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>>>>> Sent: Tuesday, April 09, 2013 3:43 PM
>>>>>>>> To: David Henningsson
>>>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>>>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>>>>>>>> in system resume
>>>>>>>
>>>>>>>>>>> I've been answering the power_save questions a little bit uncertain
>>>>>>>>>>> so far, and that's because I don't really know. I've not
>>>>>>>>>>> investigated power management much.
>>>>>>>>>>>
>>>>>>>>>>> But the person with the hardware reports that this problem happens
>>>>>>>>>>> when on AC power only. And that makes sense with your observations
>>>>>>>>>>> about power_save - there could very well be something that turns
>>>>>>>>>>> power_save off when on AC power and on when on battery power.
>>>>>>>>>>>
>>>>>>>>>>> So, if it's just a matter of re-initializing the pin, what do you
>>>>>>>>>>> think about this solution:
>>>>>>>>>>>
>>>>>>>>>>>        - Resume as normal (this will enable unsol events)
>>>>>>>>>>>        - Any time an unsol event comes in, have a haswell specific
>>>>>>>>>>> function that checks relevant pins to see that is still in D0 and
>>>>>>>>>>> mute state correct. If not, correct it.
>>>>>>>>>>>        - This check could possibly also be done in the prepare hook for
>>>>>>>>>>> extra safety.
>>>>>>>>>>
>>>>>>>>>> It won't work reliably.  The problem is that the codec might be set
>>>>>>>>>> up wrongly if it's used before the unsol event comes up.  For
>>>>>>>>>> example, a PCM resume may be performed immediately after the normal
>>>>>>>>>> resume is finished.  Meanwhile the first unsol may arrive much later than
>>>>>>>> that.
>>>>>>>>>
>>>>>>>>> Right, but if this will only mean a few samples (max 300 ms?) will
>>>>>>>>> never reach the HDMI cable, it might be the least bad workaround.
>>>>>>>
>>>>>>> I've tried enabling the unsol event on system resume, the unsol event will arrive in 70~80ms for HDMI and about 10ms for DP.
>>>>>>>
>>>>>>>> It's not only about the missing few samples.  The whole setup isn't reliable at
>>>>>>>> that point, and you don't know what to re-setup for the
>>>>>>>> *running* stream.
>>>>>>>>
>>>>>>>>>> Thus we need to delay the resume operation in anyway.  And if so,
>>>>>>>>>> there is no merit to perform the normal resume operation at the
>>>>>>>>>> first step.
>>>>>>>>>
>>>>>>>>> The normal resume operation enables unsol events, which is important,
>>>>>>>>> otherwise we don't get the unsol notification from when the gfx
>>>>>>>>> pipeline has finished.
>>>>>>>>
>>>>>>>> It seems that the intrinsic unsol event is always issued once when the unsol
>>>>>>>> event is enabled, according to Intel.  So, you won't miss it.
>>>>>>>>
>>>>>>>> But heck, we need more test coverage, obviously.
>>>>>>>>
>>>>>>>>> I guess the question here is how much of the codec setup is really
>>>>>>>>> destroyed by the gfx driver? Is it just a pin in D3 and a right
>>>>>>>>> channel mute, or is it much more that needs to be re-done after the
>>>>>>>>> gfx driver has finished?
>>>>>>>>
>>>>>>>> I'm afraid it's too native to assume that...
>>>>>>>
>>>>>>> Actually, we're not sure how much setting are destroyed, although it seems only the pin power state and mute status are affected.
>>>>>>
>>>>>> I made a test patch (attached), sprinkled with printk, and asked the
>>>>>> person with hw to test it. He said it fixed the issue. Now, I'm sure he
>>>>>> did a simple playback test only, rather than having streams running
>>>>>> during S3 or anything like that.
>>>>>>
>>>>>> The patch simply sets the pin to D0 and the copies the mute value from
>>>>>> the left channel to the right channel. The printk's confirmed this happened.
>>>>>>
>>>>>> But at least it should show that not too much of the settings are
>>>>>> destroyed...or would that be jumping to conclusions too early?
>>>>>
>>>>> In your test case, the whole stream setup is called after the gfx
>>>>> power up.  It's much different from the case where the stream is
>>>>> resumed before the gfx power up.  This is the biggest concern.
>>>>>
>>>>> That being said, the PCM can't be prepared before the gfx chip gets
>>>>> ready.
>>>>
>>>> Shall we have another stab at this today...
>>>>
>>>> I think we need to figure out what actually happens if we do start a
>>>> stream before gfx is initialized. Mengdong, could you give more
>>>> information on this?
>>>>
>>>> If it's something we can easily recover from (e g by just executing a
>>>> few verbs in the unsol event handler), that might be the best option.
>>>>
>>>> If it's not, then we might need the wait event to synchronise the PCM
>>>> start with the unsol event handler. But there is still no need to delay
>>>> rest of the chip initialization/resume for that.
>>>
>>> .... if the graphics / audio resume serialization is implemented.
>>> Currently not, so we take a brute-force way.
>>>
>>>> But part of this is also that we're discussing two different symptoms
>>>> here. I've only seen the D3 + right channel mute problem so far. We can
>>>> easily recover from that, as my workaround patch shows.
>>>> Takashi was seeing the fg node being D3 (?). I have not seen that
>>>> problem, does Mengdong know if it also exists on the latest hardware
>>>> revision?
>>>
>>> As mentioned, you can't see it from user-space.  If you access, the
>>> codec is always woken up.
>>>
>>>
>>> Actually, I prefer the delayed resume to band-aiding the broken setup
>>> afterwards.  The delayed resume mechanism was proved to work well for
>>> long time (it was so as default).  We switched to the full resume just
>>> because of a few other devices.
>>>
>>> So, the biggest concern in the scheme Mengdong suggested is only about
>>> the reliability of the unsol event.
>>
>> I'm going to make one more try to explain what I think won't work with
>> this scheme.
>>
>> 1. System wakes up from S3 suspend/resume.
>> 2. No initialization is performed because resume is delayed.
>> 3. HDMI/DP cable is plugged into the system.
>> 4. Because unsol events are not enabled (due to the lack of
>> initialization), userspace is not notified that HDMI/DP cable has been
>> plugged in.
>> 5. Userspace now has the wrong idea about whether HDMI/DP cable is
>> plugged in or not.
>
> Well, we can enable only unsol event without setting to D0.

Yes, this can work. My point is that we can't just do nothing on S3 resume.

It seems Mengdong is not around today. While pm domain synchronisation 
and more advanced patches are discussed and written, I might try to 
deploy the attached patch in Ubuntu as an intermediate solution. Feel 
free to do the same if you wish.

> For example, instead of adding codec->support_delay_resume, add a new
> codec ops, codec->patch_ops.delay_resume().  For HDMI codecs, this
> callback just calls
>
> 	spec->ready_to_resume = 0;
>
> 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> 		struct hdmi_spec_per_pin *per_pin;
> 		hda_nid_t pin_nid;
> 		struct hda_jack_tbl *jack;
>
> 		per_pin = &spec->pins[pin_idx];
> 		pin_nid = per_pin->pin_nid;
> 		jack = snd_hda_jack_tbl_get(codec, pin_nid);
> 		if (jack)
> 			snd_hda_codec_write(codec, pin_nid, 0,
> 				 AC_VERB_SET_UNSOLICITED_ENABLE,
> 				 AC_USRSP_EN | jack->tag);
> 	}
>
> and returns.  The rest would work almost as is.  The pin-detection
> itself should work even in D3 for this chip.
>
>
> Takashi
>
>>
>>
>>>
>>>
>>> Takashi
>>>
>>>>
>>>>>
>>>>>> Also, a different thought: what are the possibilities that Mengdong can
>>>>>> fix the gfx driver side not to destroy the settings in the first place?
>>>>>
>>>>> The serialization with the graphics side is mandatory more or less.
>>>>> But it'd be good if the pm domain implementation would suffice...
>>>>>
>>>>>
>>>>> Takashi
>>>>>
>>>>>> --
>>>>>> David Henningsson, Canonical Ltd.
>>>>>> https://launchpad.net/~diwic
>>>>>> [2 hsw_d3_workaround.patch <text/x-patch (7bit)>]
>>>>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>>>>>> index ede8215..edee301 100644
>>>>>> --- a/sound/pci/hda/patch_hdmi.c
>>>>>> +++ b/sound/pci/hda/patch_hdmi.c
>>>>>> @@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
>>>>>>     		hdmi_non_intrinsic_event(codec, res);
>>>>>>     }
>>>>>>
>>>>>> +static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
>>>>>> +{
>>>>>> +	int pwr, lamp, ramp;
>>>>>> +	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
>>>>>> +
>>>>>> +	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
>>>>>> +	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
>>>>>> +	printk("Haswell: Found pin in state D%d\n", pwr);
>>>>>> +	if (pwr != AC_PWRST_D0) {
>>>>>> +		printk("Haswell: Trying to set power to D0\n");
>>>>>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
>>>>>> +				    AC_PWRST_D0);
>>>>>> +		msleep(40);
>>>>>> +		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
>>>>>> +		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
>>>>>> +		printk("Haswell: Found pin in state D%d\n", pwr);
>>>>>> +	}
>>>>>> +
>>>>>> +	lamp = snd_hda_codec_read(codec, nid, 0,
>>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
>>>>>> +	ramp = snd_hda_codec_read(codec, nid, 0,
>>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
>>>>>> +	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
>>>>>> +	if (lamp != ramp) {
>>>>>> +		printk("Haswell: Setting r amp mute to l amp mute\n");
>>>>>> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
>>>>>> +				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
>>>>>> +
>>>>>> +		lamp = snd_hda_codec_read(codec, nid, 0,
>>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
>>>>>> +		ramp = snd_hda_codec_read(codec, nid, 0,
>>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
>>>>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
>>>>>> +		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
>>>>>> +	}
>>>>>> +}
>>>>>> +
>>>>>>     /*
>>>>>>      * Callbacks
>>>>>>      */
>>>>>> @@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
>>>>>>     	int pinctl;
>>>>>>     	int new_pinctl = 0;
>>>>>>
>>>>>> +	if (codec->vendor_id == 0x80862807)
>>>>>> +		haswell_verify_pin_D0(codec, pin_nid);
>>>>>> +
>>>>>>     	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
>>>>>>     		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
>>>>>>     					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> David Henningsson, Canonical Ltd.
>>>> https://launchpad.net/~diwic
>>>>
>>>
>>
>>
>>
>> --
>> David Henningsson, Canonical Ltd.
>> https://launchpad.net/~diwic
>>
>



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

[-- Attachment #2: 0001-ALSA-hda-fixup-D3-pin-and-right-channel-mute-on-Hasw.patch --]
[-- Type: text/x-patch, Size: 2844 bytes --]

>From ff4190603d846fae9049a7b96d76da1602542961 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Wed, 10 Apr 2013 12:26:07 +0200
Subject: [PATCH] ALSA: hda - fixup D3 pin and right channel mute on Haswell
 HDMI audio

When graphics initializes the HDMI chip, sometimes this leads to
pins going into D3 and right channel being muted. If the audio driver
finishes initialization before the graphic driver does, this situation
becomes permanent.

This is a workaround that checks for this situation and corrects it on
playback prepare. It has been verified working on at least one machine.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_hdmi.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index ede8215..32930e6 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1018,6 +1018,41 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
 		hdmi_non_intrinsic_event(codec, res);
 }
 
+static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
+{
+	int pwr, lamp, ramp;
+
+	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
+	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
+	if (pwr != AC_PWRST_D0) {
+		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
+				    AC_PWRST_D0);
+		msleep(40);
+		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
+		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
+		snd_printd("Haswell HDMI audio: Power for pin 0x%x is now D%d\n", nid, pwr);
+	}
+
+	lamp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
+	ramp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
+	if (lamp != ramp) {
+		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
+				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
+
+		lamp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
+		ramp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
+		snd_printd("Haswell HDMI audio: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
+	}
+}
+
 /*
  * Callbacks
  */
@@ -1032,6 +1067,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
 	int pinctl;
 	int new_pinctl = 0;
 
+	if (codec->vendor_id == 0x80862807)
+		haswell_verify_pin_D0(codec, pin_nid);
+
 	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
 		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
 					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

Hi David and Takashi,

I'm sorry for the late response. I was assigned other tasks this week

We don't have a better solution for this issue now, still trying. 
The delay_resume() ops for a codec can help not delaying the unsol event. So our patch can solve the problem after system suspend/resume if the cable is connected.
But I'm afraid that if the HDMI/DP cable removed during system suspend and connected again sometime after system is resumed, 
and if the codec access happens before the cable is connected, waiting for unsol event can be time out and codec cannot be properly resumed. 
And if runtime power saving is also disabled, the audio driver has no chance to resume the codec again. I cannot verify such hot-plug case during system/suspend now because my Haswell machines cannot reach a stable S3 and then resume. 
But we do observed a similar bug: if cable is connected after boot, the pin is in D3 with right channel muted, as the codec is initialized before unsol event comes.
Audio driver cannot find a suitable time out to wait for the usnsol event as we don't know when the cable will be connected.

We'll try if we can fix this dependency issue in Gfx driver side, and by sync Gfx and audio driver processing.

And we cannot implement pm domain atm. Last Thursday, we have a meeting with Gfx team, for Gfx power well support and audio dependency on Gfx.  
Linux PM maintainer Rafael also attended the meeting. He suggested us not use pm domain now because it cannot fully support PCI devices (PM domains ops will override PCI bus ops). We will use and extend the existing private gfx driver API to control the powerwell and sequence the initialization/suspend/resume events between gfx and audio for internal releases. Once this is working well with the private API, we will then look at either implementing this functionality as PM domain or PM runtime depending on the best fit and upstream.

I'm working on HD-A RTD3 for legacy audio in one or two weeks. After that, I'll continue to work on this issue.  

Thanks
Mengdong

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Wednesday, April 10, 2013 6:39 PM
> To: Takashi Iwai
> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> On 04/10/2013 08:52 AM, Takashi Iwai wrote:
> > At Wed, 10 Apr 2013 08:02:23 +0200,
> > David Henningsson wrote:
> >>
> >> On 04/10/2013 07:47 AM, Takashi Iwai wrote:
> >>> At Wed, 10 Apr 2013 07:29:51 +0200,
> >>> David Henningsson wrote:
> >>>>
> >>>> On 04/09/2013 11:26 AM, Takashi Iwai wrote:
> >>>>> At Tue, 09 Apr 2013 11:15:13 +0200, David Henningsson wrote:
> >>>>>>
> >>>>>> [1  <text/plain; ISO-8859-1 (7bit)>] On 04/09/2013 10:18 AM, Lin,
> >>>>>> Mengdong wrote:
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
> >>>>>>>> Sent: Tuesday, April 09, 2013 3:43 PM
> >>>>>>>> To: David Henningsson
> >>>>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam
> >>>>>>>> R
> >>>>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume
> >>>>>>>> haswell hdmi codec in system resume
> >>>>>>>
> >>>>>>>>>>> I've been answering the power_save questions a little bit
> >>>>>>>>>>> uncertain so far, and that's because I don't really know.
> >>>>>>>>>>> I've not investigated power management much.
> >>>>>>>>>>>
> >>>>>>>>>>> But the person with the hardware reports that this problem
> >>>>>>>>>>> happens when on AC power only. And that makes sense with
> >>>>>>>>>>> your observations about power_save - there could very well
> >>>>>>>>>>> be something that turns power_save off when on AC power and
> on when on battery power.
> >>>>>>>>>>>
> >>>>>>>>>>> So, if it's just a matter of re-initializing the pin, what
> >>>>>>>>>>> do you think about this solution:
> >>>>>>>>>>>
> >>>>>>>>>>>        - Resume as normal (this will enable unsol events)
> >>>>>>>>>>>        - Any time an unsol event comes in, have a haswell
> >>>>>>>>>>> specific function that checks relevant pins to see that is
> >>>>>>>>>>> still in D0 and mute state correct. If not, correct it.
> >>>>>>>>>>>        - This check could possibly also be done in the
> >>>>>>>>>>> prepare hook for extra safety.
> >>>>>>>>>>
> >>>>>>>>>> It won't work reliably.  The problem is that the codec might
> >>>>>>>>>> be set up wrongly if it's used before the unsol event comes
> >>>>>>>>>> up.  For example, a PCM resume may be performed immediately
> >>>>>>>>>> after the normal resume is finished.  Meanwhile the first
> >>>>>>>>>> unsol may arrive much later than
> >>>>>>>> that.
> >>>>>>>>>
> >>>>>>>>> Right, but if this will only mean a few samples (max 300 ms?)
> >>>>>>>>> will never reach the HDMI cable, it might be the least bad
> workaround.
> >>>>>>>
> >>>>>>> I've tried enabling the unsol event on system resume, the unsol event
> will arrive in 70~80ms for HDMI and about 10ms for DP.
> >>>>>>>
> >>>>>>>> It's not only about the missing few samples.  The whole setup
> >>>>>>>> isn't reliable at that point, and you don't know what to
> >>>>>>>> re-setup for the
> >>>>>>>> *running* stream.
> >>>>>>>>
> >>>>>>>>>> Thus we need to delay the resume operation in anyway.  And if
> >>>>>>>>>> so, there is no merit to perform the normal resume operation
> >>>>>>>>>> at the first step.
> >>>>>>>>>
> >>>>>>>>> The normal resume operation enables unsol events, which is
> >>>>>>>>> important, otherwise we don't get the unsol notification from
> >>>>>>>>> when the gfx pipeline has finished.
> >>>>>>>>
> >>>>>>>> It seems that the intrinsic unsol event is always issued once
> >>>>>>>> when the unsol event is enabled, according to Intel.  So, you won't
> miss it.
> >>>>>>>>
> >>>>>>>> But heck, we need more test coverage, obviously.
> >>>>>>>>
> >>>>>>>>> I guess the question here is how much of the codec setup is
> >>>>>>>>> really destroyed by the gfx driver? Is it just a pin in D3 and
> >>>>>>>>> a right channel mute, or is it much more that needs to be
> >>>>>>>>> re-done after the gfx driver has finished?
> >>>>>>>>
> >>>>>>>> I'm afraid it's too native to assume that...
> >>>>>>>
> >>>>>>> Actually, we're not sure how much setting are destroyed, although it
> seems only the pin power state and mute status are affected.
> >>>>>>
> >>>>>> I made a test patch (attached), sprinkled with printk, and asked
> >>>>>> the person with hw to test it. He said it fixed the issue. Now,
> >>>>>> I'm sure he did a simple playback test only, rather than having
> >>>>>> streams running during S3 or anything like that.
> >>>>>>
> >>>>>> The patch simply sets the pin to D0 and the copies the mute value
> >>>>>> from the left channel to the right channel. The printk's confirmed this
> happened.
> >>>>>>
> >>>>>> But at least it should show that not too much of the settings are
> >>>>>> destroyed...or would that be jumping to conclusions too early?
> >>>>>
> >>>>> In your test case, the whole stream setup is called after the gfx
> >>>>> power up.  It's much different from the case where the stream is
> >>>>> resumed before the gfx power up.  This is the biggest concern.
> >>>>>
> >>>>> That being said, the PCM can't be prepared before the gfx chip
> >>>>> gets ready.
> >>>>
> >>>> Shall we have another stab at this today...
> >>>>
> >>>> I think we need to figure out what actually happens if we do start
> >>>> a stream before gfx is initialized. Mengdong, could you give more
> >>>> information on this?
> >>>>
> >>>> If it's something we can easily recover from (e g by just executing
> >>>> a few verbs in the unsol event handler), that might be the best option.
> >>>>
> >>>> If it's not, then we might need the wait event to synchronise the
> >>>> PCM start with the unsol event handler. But there is still no need
> >>>> to delay rest of the chip initialization/resume for that.
> >>>
> >>> .... if the graphics / audio resume serialization is implemented.
> >>> Currently not, so we take a brute-force way.
> >>>
> >>>> But part of this is also that we're discussing two different
> >>>> symptoms here. I've only seen the D3 + right channel mute problem
> >>>> so far. We can easily recover from that, as my workaround patch shows.
> >>>> Takashi was seeing the fg node being D3 (?). I have not seen that
> >>>> problem, does Mengdong know if it also exists on the latest
> >>>> hardware revision?
> >>>
> >>> As mentioned, you can't see it from user-space.  If you access, the
> >>> codec is always woken up.
> >>>
> >>>
> >>> Actually, I prefer the delayed resume to band-aiding the broken
> >>> setup afterwards.  The delayed resume mechanism was proved to work
> >>> well for long time (it was so as default).  We switched to the full
> >>> resume just because of a few other devices.
> >>>
> >>> So, the biggest concern in the scheme Mengdong suggested is only
> >>> about the reliability of the unsol event.
> >>
> >> I'm going to make one more try to explain what I think won't work
> >> with this scheme.
> >>
> >> 1. System wakes up from S3 suspend/resume.
> >> 2. No initialization is performed because resume is delayed.
> >> 3. HDMI/DP cable is plugged into the system.
> >> 4. Because unsol events are not enabled (due to the lack of
> >> initialization), userspace is not notified that HDMI/DP cable has
> >> been plugged in.
> >> 5. Userspace now has the wrong idea about whether HDMI/DP cable is
> >> plugged in or not.
> >
> > Well, we can enable only unsol event without setting to D0.
> 
> Yes, this can work. My point is that we can't just do nothing on S3 resume.
> 
> It seems Mengdong is not around today. While pm domain synchronisation and
> more advanced patches are discussed and written, I might try to deploy the
> attached patch in Ubuntu as an intermediate solution. Feel free to do the same
> if you wish.
> 
> > For example, instead of adding codec->support_delay_resume, add a new
> > codec ops, codec->patch_ops.delay_resume().  For HDMI codecs, this
> > callback just calls
> >
> > 	spec->ready_to_resume = 0;
> >
> > 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> > 		struct hdmi_spec_per_pin *per_pin;
> > 		hda_nid_t pin_nid;
> > 		struct hda_jack_tbl *jack;
> >
> > 		per_pin = &spec->pins[pin_idx];
> > 		pin_nid = per_pin->pin_nid;
> > 		jack = snd_hda_jack_tbl_get(codec, pin_nid);
> > 		if (jack)
> > 			snd_hda_codec_write(codec, pin_nid, 0,
> > 				 AC_VERB_SET_UNSOLICITED_ENABLE,
> > 				 AC_USRSP_EN | jack->tag);
> > 	}
> >
> > and returns.  The rest would work almost as is.  The pin-detection
> > itself should work even in D3 for this chip.
> >
> >
> > Takashi
> >
> >>
> >>
> >>>
> >>>
> >>> Takashi
> >>>
> >>>>
> >>>>>
> >>>>>> Also, a different thought: what are the possibilities that
> >>>>>> Mengdong can fix the gfx driver side not to destroy the settings in the
> first place?
> >>>>>
> >>>>> The serialization with the graphics side is mandatory more or less.
> >>>>> But it'd be good if the pm domain implementation would suffice...
> >>>>>
> >>>>>
> >>>>> Takashi
> >>>>>
> >>>>>> --
> >>>>>> David Henningsson, Canonical Ltd.
> >>>>>> https://launchpad.net/~diwic
> >>>>>> [2 hsw_d3_workaround.patch <text/x-patch (7bit)>] diff --git
> >>>>>> a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index
> >>>>>> ede8215..edee301 100644
> >>>>>> --- a/sound/pci/hda/patch_hdmi.c
> >>>>>> +++ b/sound/pci/hda/patch_hdmi.c
> >>>>>> @@ -1018,6 +1018,46 @@ static void hdmi_unsol_event(struct
> hda_codec *codec, unsigned int res)
> >>>>>>     		hdmi_non_intrinsic_event(codec, res);
> >>>>>>     }
> >>>>>>
> >>>>>> +static void haswell_verify_pin_D0(struct hda_codec *codec,
> >>>>>> +hda_nid_t nid) {
> >>>>>> +	int pwr, lamp, ramp;
> >>>>>> +	printk("Haswell: Verify pin D0 on pin 0x%x\n", nid);
> >>>>>> +
> >>>>>> +	pwr = snd_hda_codec_read(codec, nid, 0,
> AC_VERB_GET_POWER_STATE, 0);
> >>>>>> +	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
> >>>>>> +	printk("Haswell: Found pin in state D%d\n", pwr);
> >>>>>> +	if (pwr != AC_PWRST_D0) {
> >>>>>> +		printk("Haswell: Trying to set power to D0\n");
> >>>>>> +		snd_hda_codec_write(codec, nid, 0,
> AC_VERB_SET_POWER_STATE,
> >>>>>> +				    AC_PWRST_D0);
> >>>>>> +		msleep(40);
> >>>>>> +		pwr = snd_hda_codec_read(codec, nid, 0,
> AC_VERB_GET_POWER_STATE, 0);
> >>>>>> +		pwr = (pwr & AC_PWRST_ACTUAL) >>
> AC_PWRST_ACTUAL_SHIFT;
> >>>>>> +		printk("Haswell: Found pin in state D%d\n", pwr);
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	lamp = snd_hda_codec_read(codec, nid, 0,
> >>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> >>>>>> +	ramp = snd_hda_codec_read(codec, nid, 0,
> >>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> >>>>>> +	printk("Haswell: Verify mute on pin 0x%x: [0x%x 0x%x]\n", nid, lamp,
> ramp);
> >>>>>> +	if (lamp != ramp) {
> >>>>>> +		printk("Haswell: Setting r amp mute to l amp mute\n");
> >>>>>> +		snd_hda_codec_write(codec, nid, 0,
> AC_VERB_SET_AMP_GAIN_MUTE,
> >>>>>> +				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT |
> lamp);
> >>>>>> +
> >>>>>> +		lamp = snd_hda_codec_read(codec, nid, 0,
> >>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>>>> +				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
> >>>>>> +		ramp = snd_hda_codec_read(codec, nid, 0,
> >>>>>> +				  AC_VERB_GET_AMP_GAIN_MUTE,
> >>>>>> +				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
> >>>>>> +		printk("Haswell: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid,
> lamp, ramp);
> >>>>>> +	}
> >>>>>> +}
> >>>>>> +
> >>>>>>     /*
> >>>>>>      * Callbacks
> >>>>>>      */
> >>>>>> @@ -1032,6 +1072,9 @@ static int hdmi_setup_stream(struct
> hda_codec *codec, hda_nid_t cvt_nid,
> >>>>>>     	int pinctl;
> >>>>>>     	int new_pinctl = 0;
> >>>>>>
> >>>>>> +	if (codec->vendor_id == 0x80862807)
> >>>>>> +		haswell_verify_pin_D0(codec, pin_nid);
> >>>>>> +
> >>>>>>     	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR)
> {
> >>>>>>     		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> >>>>>>     					    AC_VERB_GET_PIN_WIDGET_CONTROL,
> 0);
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> David Henningsson, Canonical Ltd.
> >>>> https://launchpad.net/~diwic
> >>>>
> >>>
> >>
> >>
> >>
> >> --
> >> David Henningsson, Canonical Ltd.
> >> https://launchpad.net/~diwic
> >>
> >
> 
> 
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 04/14/2013 03:48 PM, Lin, Mengdong wrote:
> Hi David and Takashi,
>
> I'm sorry for the late response. I was assigned other tasks this week
>
> We don't have a better solution for this issue now, still trying.
> The delay_resume() ops for a codec can help not delaying the unsol event. So our patch can solve the problem after system suspend/resume if the cable is connected.
> But I'm afraid that if the HDMI/DP cable removed during system suspend and connected again sometime after system is resumed,
> and if the codec access happens before the cable is connected, waiting for unsol event can be time out and codec cannot be properly resumed.
> And if runtime power saving is also disabled, the audio driver has no chance to resume the codec again. I cannot verify such hot-plug case during system/suspend now because my Haswell machines cannot reach a stable S3 and then resume.
> But we do observed a similar bug: if cable is connected after boot, the pin is in D3 with right channel muted, as the codec is initialized before unsol event comes.
> Audio driver cannot find a suitable time out to wait for the usnsol event as we don't know when the cable will be connected.

Takashi's latest suggestion was to enable unsol events, and nothing 
else, directly on S3 resume. See suggested patch below.

Will that not help here? Then we would at least get some unsol events on 
hotplug I assume?

> We'll try if we can fix this dependency issue in Gfx driver side, and by sync Gfx and audio driver processing.
>
> And we cannot implement pm domain atm. Last Thursday, we have a meeting with Gfx team, for Gfx power well support and audio dependency on Gfx.
> Linux PM maintainer Rafael also attended the meeting. He suggested us not use pm domain now because it cannot fully support PCI devices (PM domains ops will override PCI bus ops). We will use and extend the existing private gfx driver API to control the powerwell and sequence the initialization/suspend/resume events between gfx and audio for internal releases. Once this is working well with the private API, we will then look at either implementing this functionality as PM domain or PM runtime depending on the best fit and upstream.
>
> I'm working on HD-A RTD3 for legacy audio in one or two weeks. After that, I'll continue to work on this issue.

The dependency on the Gfx driver, is it both codec-wide and per pin? It 
seems to be at least per pin, which means that whenever we plug 
something in, we need to redo our initialization of that pin after the 
Gfx driver has finished, is that correct?


>
> Thanks
> Mengdong
>
>>>> I'm going to make one more try to explain what I think won't work
>>>> with this scheme.
>>>>
>>>> 1. System wakes up from S3 suspend/resume.
>>>> 2. No initialization is performed because resume is delayed.
>>>> 3. HDMI/DP cable is plugged into the system.
>>>> 4. Because unsol events are not enabled (due to the lack of
>>>> initialization), userspace is not notified that HDMI/DP cable has
>>>> been plugged in.
>>>> 5. Userspace now has the wrong idea about whether HDMI/DP cable is
>>>> plugged in or not.
>>>
>>> Well, we can enable only unsol event without setting to D0.
>>
>> Yes, this can work. My point is that we can't just do nothing on S3 resume.
>>
>> It seems Mengdong is not around today. While pm domain synchronisation and
>> more advanced patches are discussed and written, I might try to deploy the
>> attached patch in Ubuntu as an intermediate solution. Feel free to do the same
>> if you wish.
>>
>>> For example, instead of adding codec->support_delay_resume, add a new
>>> codec ops, codec->patch_ops.delay_resume().  For HDMI codecs, this
>>> callback just calls
>>>
>>> 	spec->ready_to_resume = 0;
>>>
>>> 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>>> 		struct hdmi_spec_per_pin *per_pin;
>>> 		hda_nid_t pin_nid;
>>> 		struct hda_jack_tbl *jack;
>>>
>>> 		per_pin = &spec->pins[pin_idx];
>>> 		pin_nid = per_pin->pin_nid;
>>> 		jack = snd_hda_jack_tbl_get(codec, pin_nid);
>>> 		if (jack)
>>> 			snd_hda_codec_write(codec, pin_nid, 0,
>>> 				 AC_VERB_SET_UNSOLICITED_ENABLE,
>>> 				 AC_USRSP_EN | jack->tag);
>>> 	}
>>>
>>> and returns.  The rest would work almost as is.  The pin-detection
>>> itself should work even in D3 for this chip.


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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Monday, April 15, 2013 3:03 PM
> To: Lin, Mengdong
> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> On 04/14/2013 03:48 PM, Lin, Mengdong wrote:
> > Hi David and Takashi,
> >
> > I'm sorry for the late response. I was assigned other tasks this week
> >
> > We don't have a better solution for this issue now, still trying.
> > The delay_resume() ops for a codec can help not delaying the unsol event. So
> our patch can solve the problem after system suspend/resume if the cable is
> connected.
> > But I'm afraid that if the HDMI/DP cable removed during system suspend
> > and connected again sometime after system is resumed, and if the codec
> access happens before the cable is connected, waiting for unsol event can be
> time out and codec cannot be properly resumed.
> > And if runtime power saving is also disabled, the audio driver has no chance to
> resume the codec again. I cannot verify such hot-plug case during
> system/suspend now because my Haswell machines cannot reach a stable S3
> and then resume.
> > But we do observed a similar bug: if cable is connected after boot, the pin is in
> D3 with right channel muted, as the codec is initialized before unsol event
> comes.
> > Audio driver cannot find a suitable time out to wait for the usnsol event as we
> don't know when the cable will be connected.
> 
> Takashi's latest suggestion was to enable unsol events, and nothing else,
> directly on S3 resume. See suggested patch below.
> 
> Will that not help here? Then we would at least get some unsol events on
> hotplug I assume?

I think Takashi's patch can help when cable is connected during suspend/resume cycles.
This patch enables unsol event, so make sure unsol event won't be delayed.

So shall I improve the patch as Takashi suggested, to solve current suspend/resume issue?


> > We'll try if we can fix this dependency issue in Gfx driver side, and by sync Gfx
> and audio driver processing.
> >
> > And we cannot implement pm domain atm. Last Thursday, we have a meeting
> with Gfx team, for Gfx power well support and audio dependency on Gfx.
> > Linux PM maintainer Rafael also attended the meeting. He suggested us not
> use pm domain now because it cannot fully support PCI devices (PM domains
> ops will override PCI bus ops). We will use and extend the existing private gfx
> driver API to control the powerwell and sequence the
> initialization/suspend/resume events between gfx and audio for internal
> releases. Once this is working well with the private API, we will then look at
> either implementing this functionality as PM domain or PM runtime depending
> on the best fit and upstream.
> >
> > I'm working on HD-A RTD3 for legacy audio in one or two weeks. After that,
> I'll continue to work on this issue.
> 
> The dependency on the Gfx driver, is it both codec-wide and per pin? It seems to
> be at least per pin, which means that whenever we plug something in, we need
> to redo our initialization of that pin after the Gfx driver has finished, is that
> correct?

We're still investigating the Gfx driver sequence.
In fact, not every hot plug will cause trouble. If the system is booted with cable connected, later hot-plug does not cause error, including switch between DP and HDMI cable.
The dependency issue only happens on boot and resume. 
Since the hot-plug will also make Gfx reconfig the display pipeline, so we want to compare the difference and locate the accurate dependency.

Thanks
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 4367 bytes --]

On 04/15/2013 09:48 AM, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>> Sent: Monday, April 15, 2013 3:03 PM
>> To: Lin, Mengdong
>> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>> in system resume
>>
>> On 04/14/2013 03:48 PM, Lin, Mengdong wrote:
>>> Hi David and Takashi,
>>>
>>> I'm sorry for the late response. I was assigned other tasks this week
>>>
>>> We don't have a better solution for this issue now, still trying.
>>> The delay_resume() ops for a codec can help not delaying the unsol event. So
>> our patch can solve the problem after system suspend/resume if the cable is
>> connected.
>>> But I'm afraid that if the HDMI/DP cable removed during system suspend
>>> and connected again sometime after system is resumed, and if the codec
>> access happens before the cable is connected, waiting for unsol event can be
>> time out and codec cannot be properly resumed.
>>> And if runtime power saving is also disabled, the audio driver has no chance to
>> resume the codec again. I cannot verify such hot-plug case during
>> system/suspend now because my Haswell machines cannot reach a stable S3
>> and then resume.
>>> But we do observed a similar bug: if cable is connected after boot, the pin is in
>> D3 with right channel muted, as the codec is initialized before unsol event
>> comes.
>>> Audio driver cannot find a suitable time out to wait for the usnsol event as we
>> don't know when the cable will be connected.
>>
>> Takashi's latest suggestion was to enable unsol events, and nothing else,
>> directly on S3 resume. See suggested patch below.
>>
>> Will that not help here? Then we would at least get some unsol events on
>> hotplug I assume?
>
> I think Takashi's patch can help when cable is connected during suspend/resume cycles.
> This patch enables unsol event, so make sure unsol event won't be delayed.
>
> So shall I improve the patch as Takashi suggested, to solve current suspend/resume issue?

If you do, in what scenarios will this not be enough? I e, what will 
still require more advanced synchronisation between gfx and audio driver 
(as outlined below)?

>
>
>>> We'll try if we can fix this dependency issue in Gfx driver side, and by sync Gfx
>> and audio driver processing.
>>>
>>> And we cannot implement pm domain atm. Last Thursday, we have a meeting
>> with Gfx team, for Gfx power well support and audio dependency on Gfx.
>>> Linux PM maintainer Rafael also attended the meeting. He suggested us not
>> use pm domain now because it cannot fully support PCI devices (PM domains
>> ops will override PCI bus ops). We will use and extend the existing private gfx
>> driver API to control the powerwell and sequence the
>> initialization/suspend/resume events between gfx and audio for internal
>> releases. Once this is working well with the private API, we will then look at
>> either implementing this functionality as PM domain or PM runtime depending
>> on the best fit and upstream.
>>>
>>> I'm working on HD-A RTD3 for legacy audio in one or two weeks. After that,
>> I'll continue to work on this issue.
>>
>> The dependency on the Gfx driver, is it both codec-wide and per pin? It seems to
>> be at least per pin, which means that whenever we plug something in, we need
>> to redo our initialization of that pin after the Gfx driver has finished, is that
>> correct?
>
> We're still investigating the Gfx driver sequence.
> In fact, not every hot plug will cause trouble. If the system is booted with cable connected, later hot-plug does not cause error, including switch between DP and HDMI cable.
> The dependency issue only happens on boot and resume.
> Since the hot-plug will also make Gfx reconfig the display pipeline, so we want to compare the difference and locate the accurate dependency.

It looks like this will take some time to implement, and the 3.10 merge 
window will soon open. It also sounds like the complexity makes it 
possible that a full fix can not be applied later during 3.10 rc cycle.

Takashi, may I ask you to apply the attached workaround in the mean 
time? I've confirmed it to resolve the problem on at least two different 
machines.


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

[-- Attachment #2: 0001-ALSA-hda-fixup-D3-pin-and-right-channel-mute-on-Hasw.patch --]
[-- Type: text/x-patch, Size: 2893 bytes --]

>From 37b4fbb88dc03160fd71d1ec11ba14e83e6a79dc Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Wed, 10 Apr 2013 12:26:07 +0200
Subject: [PATCH] ALSA: hda - fixup D3 pin and right channel mute on Haswell
 HDMI audio

When graphics initializes the HDMI chip, sometimes this leads to
pins going into D3 and right channel being muted. If the audio driver
finishes initialization before the graphic driver does, this situation
becomes permanent.

This is a workaround that checks for this situation and corrects it on
playback prepare. It has been verified working on at least one machine.

BugLink: https://bugs.launchpad.net/bugs/1167270
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_hdmi.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index ede8215..32930e6 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1018,6 +1018,41 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
 		hdmi_non_intrinsic_event(codec, res);
 }
 
+static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid)
+{
+	int pwr, lamp, ramp;
+
+	pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
+	pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
+	if (pwr != AC_PWRST_D0) {
+		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
+				    AC_PWRST_D0);
+		msleep(40);
+		pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0);
+		pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT;
+		snd_printd("Haswell HDMI audio: Power for pin 0x%x is now D%d\n", nid, pwr);
+	}
+
+	lamp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
+	ramp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
+	if (lamp != ramp) {
+		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,
+				    AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp);
+
+		lamp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT);
+		ramp = snd_hda_codec_read(codec, nid, 0,
+				  AC_VERB_GET_AMP_GAIN_MUTE,
+				  AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT);
+		snd_printd("Haswell HDMI audio: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp);
+	}
+}
+
 /*
  * Callbacks
  */
@@ -1032,6 +1067,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid,
 	int pinctl;
 	int new_pinctl = 0;
 
+	if (codec->vendor_id == 0x80862807)
+		haswell_verify_pin_D0(codec, pin_nid);
+
 	if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) {
 		pinctl = snd_hda_codec_read(codec, pin_nid, 0,
 					    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
-- 
1.7.9.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Wed, 17 Apr 2013 07:51:47 +0200,
David Henningsson wrote:
> 
> On 04/15/2013 09:48 AM, Lin, Mengdong wrote:
> >> -----Original Message-----
> >> From: David Henningsson [mailto:david.henningsson@canonical.com]
> >> Sent: Monday, April 15, 2013 3:03 PM
> >> To: Lin, Mengdong
> >> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R
> >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> >> in system resume
> >>
> >> On 04/14/2013 03:48 PM, Lin, Mengdong wrote:
> >>> Hi David and Takashi,
> >>>
> >>> I'm sorry for the late response. I was assigned other tasks this week
> >>>
> >>> We don't have a better solution for this issue now, still trying.
> >>> The delay_resume() ops for a codec can help not delaying the unsol event. So
> >> our patch can solve the problem after system suspend/resume if the cable is
> >> connected.
> >>> But I'm afraid that if the HDMI/DP cable removed during system suspend
> >>> and connected again sometime after system is resumed, and if the codec
> >> access happens before the cable is connected, waiting for unsol event can be
> >> time out and codec cannot be properly resumed.
> >>> And if runtime power saving is also disabled, the audio driver has no chance to
> >> resume the codec again. I cannot verify such hot-plug case during
> >> system/suspend now because my Haswell machines cannot reach a stable S3
> >> and then resume.
> >>> But we do observed a similar bug: if cable is connected after boot, the pin is in
> >> D3 with right channel muted, as the codec is initialized before unsol event
> >> comes.
> >>> Audio driver cannot find a suitable time out to wait for the usnsol event as we
> >> don't know when the cable will be connected.
> >>
> >> Takashi's latest suggestion was to enable unsol events, and nothing else,
> >> directly on S3 resume. See suggested patch below.
> >>
> >> Will that not help here? Then we would at least get some unsol events on
> >> hotplug I assume?
> >
> > I think Takashi's patch can help when cable is connected during suspend/resume cycles.
> > This patch enables unsol event, so make sure unsol event won't be delayed.
> >
> > So shall I improve the patch as Takashi suggested, to solve current suspend/resume issue?
> 
> If you do, in what scenarios will this not be enough? I e, what will 
> still require more advanced synchronisation between gfx and audio driver 
> (as outlined below)?
> 
> >
> >
> >>> We'll try if we can fix this dependency issue in Gfx driver side, and by sync Gfx
> >> and audio driver processing.
> >>>
> >>> And we cannot implement pm domain atm. Last Thursday, we have a meeting
> >> with Gfx team, for Gfx power well support and audio dependency on Gfx.
> >>> Linux PM maintainer Rafael also attended the meeting. He suggested us not
> >> use pm domain now because it cannot fully support PCI devices (PM domains
> >> ops will override PCI bus ops). We will use and extend the existing private gfx
> >> driver API to control the powerwell and sequence the
> >> initialization/suspend/resume events between gfx and audio for internal
> >> releases. Once this is working well with the private API, we will then look at
> >> either implementing this functionality as PM domain or PM runtime depending
> >> on the best fit and upstream.
> >>>
> >>> I'm working on HD-A RTD3 for legacy audio in one or two weeks. After that,
> >> I'll continue to work on this issue.
> >>
> >> The dependency on the Gfx driver, is it both codec-wide and per pin? It seems to
> >> be at least per pin, which means that whenever we plug something in, we need
> >> to redo our initialization of that pin after the Gfx driver has finished, is that
> >> correct?
> >
> > We're still investigating the Gfx driver sequence.
> > In fact, not every hot plug will cause trouble. If the system is booted with cable connected, later hot-plug does not cause error, including switch between DP and HDMI cable.
> > The dependency issue only happens on boot and resume.
> > Since the hot-plug will also make Gfx reconfig the display pipeline, so we want to compare the difference and locate the accurate dependency.
> 
> It looks like this will take some time to implement, and the 3.10 merge 
> window will soon open. It also sounds like the complexity makes it 
> possible that a full fix can not be applied later during 3.10 rc cycle.
> 
> Takashi, may I ask you to apply the attached workaround in the mean 
> time? I've confirmed it to resolve the problem on at least two different 
> machines.

Yeah, looking through the development, I'm inclined to postpone the
delayed resume scheme for 3.10.  As a bandaid fix, I'm going to apply
David's patch for now.

IMO, the biggest concern in the current delayed resume implementation
is that we don't know how reliable the unsol event generation and
handling.  In other words, if the graphics driver can _always_ sets
and/or wakes up for the hardware readiness state, it'd be a much
better way to control; it's more deterministic.

Then we can implement, e.g. a platform device shared by both graphics
and audio drivers and let pm operations sync there, as Liam once
suggested.  This would work even w/o pm domain implementation (it's a
kind of own pm domain implementation, after all).


Takashi

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

Hi Takashi and David,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, April 17, 2013 2:13 PM
> To: David Henningsson
> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> At Wed, 17 Apr 2013 07:51:47 +0200,
> David Henningsson wrote:
> > >> The dependency on the Gfx driver, is it both codec-wide and per
> > >> pin? It seems to be at least per pin, which means that whenever we
> > >> plug something in, we need to redo our initialization of that pin
> > >> after the Gfx driver has finished, is that correct?

It was confirmed that this a per pin dependency, not codec-wide. 

For Haswell, every pin is in D3 with amplifier muted by default. 
The audio driver must program the pin to D0 and unmute the pin, after the gfx driver connect the GPU pipe and port, and enable the transcoder 
(The transcoder is the component where audio is mixed with video). 
Otherwise, the pin's default power and mute state will not change, by checking the Gfx side audio register, although audio codec does not report error.

In addition, HDMI/DP cable hot plug will trigger Gfx driver to disconnect/connect the pipeline, and port may be connected to a new transcoder.
If the transcoder has changed, the pin can return to D3 and muted again. There are 3 pipelines in GPU, a pipe/port connection only affects one pin, not the other pins and convertors.
So our previous patch to delay powering up the whole codec once is not enough and not suitable.

Since audio driver does not and should not have any knowledge how GfX maniplute the display pipeline.
The unsolicited event can be used to as flag that the pin widget is ready to power up.

And because the unsol event can happen at D3 and it not necessarily to power up the pin at once on the unsol event,
we prefer to check and fix the pin state when a PCM stream is setting up. This can also avoid race in PM.

We also checked the info frame. Current mechanism to check and refresh audio info frame works well.

So David's patch is a reliable fix to solve the display audio dependency issue. There is no need to write a new patch atm.

Thanks
Mengdong

> > It looks like this will take some time to implement, and the 3.10
> > merge window will soon open. It also sounds like the complexity makes
> > it possible that a full fix can not be applied later during 3.10 rc cycle.
> >
> > Takashi, may I ask you to apply the attached workaround in the mean
> > time? I've confirmed it to resolve the problem on at least two
> > different machines.
> 
> Yeah, looking through the development, I'm inclined to postpone the delayed
> resume scheme for 3.10.  As a bandaid fix, I'm going to apply David's patch for
> now.
> 
> IMO, the biggest concern in the current delayed resume implementation is that
> we don't know how reliable the unsol event generation and handling.  In other
> words, if the graphics driver can _always_ sets and/or wakes up for the
> hardware readiness state, it'd be a much better way to control; it's more
> deterministic.
> 
> Then we can implement, e.g. a platform device shared by both graphics and
> audio drivers and let pm operations sync there, as Liam once suggested.  This
> would work even w/o pm domain implementation (it's a kind of own pm domain
> implementation, after all).

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 05/03/2013 08:56 AM, Lin, Mengdong wrote:
> Hi Takashi and David,
>
>> -----Original Message-----
>> From: Takashi Iwai [mailto:tiwai@suse.de]
>> Sent: Wednesday, April 17, 2013 2:13 PM
>> To: David Henningsson
>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>> in system resume
>>
>> At Wed, 17 Apr 2013 07:51:47 +0200,
>> David Henningsson wrote:
>>>>> The dependency on the Gfx driver, is it both codec-wide and per
>>>>> pin? It seems to be at least per pin, which means that whenever we
>>>>> plug something in, we need to redo our initialization of that pin
>>>>> after the Gfx driver has finished, is that correct?
>
> It was confirmed that this a per pin dependency, not codec-wide.
>
> For Haswell, every pin is in D3 with amplifier muted by default.
> The audio driver must program the pin to D0 and unmute the pin, after the gfx driver connect the GPU pipe and port, and enable the transcoder
> (The transcoder is the component where audio is mixed with video).
> Otherwise, the pin's default power and mute state will not change, by checking the Gfx side audio register, although audio codec does not report error.
>
> In addition, HDMI/DP cable hot plug will trigger Gfx driver to disconnect/connect the pipeline, and port may be connected to a new transcoder.
> If the transcoder has changed, the pin can return to D3 and muted again. There are 3 pipelines in GPU, a pipe/port connection only affects one pin, not the other pins and convertors.
> So our previous patch to delay powering up the whole codec once is not enough and not suitable.

This makes me a bit worried actually. If the transcoder to port 
connection can be changed at runtime, how does this affect the default 
pin config on the audio codec pin node?

E g, if the machine only has one physical HDMI output, it would be a 
good thing to mark only one of the pin nodes as a "jack" and the other 
two as "not connected".

Now, if the transcoder used for this HDMI output is varying, it can be 
sometimes not matching the audio codec pin node set to "jack", and then 
we have a problem...!

Or is the gfx engine taking the default pin config of the audio codec 
into account somehow?

>
> Since audio driver does not and should not have any knowledge how GfX maniplute the display pipeline.
> The unsolicited event can be used to as flag that the pin widget is ready to power up.
>
> And because the unsol event can happen at D3 and it not necessarily to power up the pin at once on the unsol event,
> we prefer to check and fix the pin state when a PCM stream is setting up. This can also avoid race in PM.
>
> We also checked the info frame. Current mechanism to check and refresh audio info frame works well.
>
> So David's patch is a reliable fix to solve the display audio dependency issue. There is no need to write a new patch atm.
>
> Thanks
> Mengdong
>
>>> It looks like this will take some time to implement, and the 3.10
>>> merge window will soon open. It also sounds like the complexity makes
>>> it possible that a full fix can not be applied later during 3.10 rc cycle.
>>>
>>> Takashi, may I ask you to apply the attached workaround in the mean
>>> time? I've confirmed it to resolve the problem on at least two
>>> different machines.
>>
>> Yeah, looking through the development, I'm inclined to postpone the delayed
>> resume scheme for 3.10.  As a bandaid fix, I'm going to apply David's patch for
>> now.
>>
>> IMO, the biggest concern in the current delayed resume implementation is that
>> we don't know how reliable the unsol event generation and handling.  In other
>> words, if the graphics driver can _always_ sets and/or wakes up for the
>> hardware readiness state, it'd be a much better way to control; it's more
>> deterministic.
>>
>> Then we can implement, e.g. a platform device shared by both graphics and
>> audio drivers and let pm operations sync there, as Liam once suggested.  This
>> would work even w/o pm domain implementation (it's a kind of own pm domain
>> implementation, after all).
>
>
>
>



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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Friday, May 03, 2013 3:20 PM
> To: Lin, Mengdong
> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R; Wang,
> Xingchao; Li, Jocelyn
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> On 05/03/2013 08:56 AM, Lin, Mengdong wrote:
> > Hi Takashi and David,
> >
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai@suse.de]
> >> Sent: Wednesday, April 17, 2013 2:13 PM
> >> To: David Henningsson
> >> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell
> >> hdmi codec in system resume
> >>
> >> At Wed, 17 Apr 2013 07:51:47 +0200,
> >> David Henningsson wrote:
> >>>>> The dependency on the Gfx driver, is it both codec-wide and per
> >>>>> pin? It seems to be at least per pin, which means that whenever we
> >>>>> plug something in, we need to redo our initialization of that pin
> >>>>> after the Gfx driver has finished, is that correct?
> >
> > It was confirmed that this a per pin dependency, not codec-wide.
> >
> > For Haswell, every pin is in D3 with amplifier muted by default.
> > The audio driver must program the pin to D0 and unmute the pin, after
> > the gfx driver connect the GPU pipe and port, and enable the transcoder (The
> transcoder is the component where audio is mixed with video).
> > Otherwise, the pin's default power and mute state will not change, by
> checking the Gfx side audio register, although audio codec does not report
> error.
> >
> > In addition, HDMI/DP cable hot plug will trigger Gfx driver to
> disconnect/connect the pipeline, and port may be connected to a new
> transcoder.
> > If the transcoder has changed, the pin can return to D3 and muted again.
> There are 3 pipelines in GPU, a pipe/port connection only affects one pin, not
> the other pins and convertors.
> > So our previous patch to delay powering up the whole codec once is not
> enough and not suitable.
> 
> This makes me a bit worried actually. If the transcoder to port connection can
> be changed at runtime, how does this affect the default pin config on the audio
> codec pin node?
> 
> E g, if the machine only has one physical HDMI output, it would be a good thing
> to mark only one of the pin nodes as a "jack" and the other two as "not
> connected".
> 
> Now, if the transcoder used for this HDMI output is varying, it can be sometimes
> not matching the audio codec pin node set to "jack", and then we have a
> problem...!
> 
> Or is the gfx engine taking the default pin config of the audio codec into
> account somehow?

I think the pin configuration default should not be affected, as it's for BIOS programming.

The DDI port is bound to a HDMI or DP connector on the board.
And although the transcoder can be changed at runtime, the pin report the eld info does not change.

Anyway, we'll double check this issue. 

Thanks for reminding!
Mengdong

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

Hi David,

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Lin, Mengdong
> Sent: Friday, May 03, 2013 3:30 PM
> To: David Henningsson
> Cc: Takashi Iwai; Li, Jocelyn; alsa-devel@alsa-project.org; Wang, Xingchao;
> Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> > -----Original Message-----
> > From: David Henningsson [mailto:david.henningsson@canonical.com]
> > Sent: Friday, May 03, 2013 3:20 PM
> > To: Lin, Mengdong
> > Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R; Wang,
> > Xingchao; Li, Jocelyn
> > Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell
> > hdmi codec in system resume
> >
> > On 05/03/2013 08:56 AM, Lin, Mengdong wrote:
> > > Hi Takashi and David,
> > >
> > >> -----Original Message-----
> > >> From: Takashi Iwai [mailto:tiwai@suse.de]
> > >> Sent: Wednesday, April 17, 2013 2:13 PM
> > >> To: David Henningsson
> > >> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
> > >> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell
> > >> hdmi codec in system resume
> > >>
> > >> At Wed, 17 Apr 2013 07:51:47 +0200, David Henningsson wrote:
> > >>>>> The dependency on the Gfx driver, is it both codec-wide and per
> > >>>>> pin? It seems to be at least per pin, which means that whenever
> > >>>>> we plug something in, we need to redo our initialization of that
> > >>>>> pin after the Gfx driver has finished, is that correct?
> > >
> > > It was confirmed that this a per pin dependency, not codec-wide.
> > >
> > > For Haswell, every pin is in D3 with amplifier muted by default.
> > > The audio driver must program the pin to D0 and unmute the pin,
> > > after the gfx driver connect the GPU pipe and port, and enable the
> > > transcoder (The
> > transcoder is the component where audio is mixed with video).
> > > Otherwise, the pin's default power and mute state will not change,
> > > by
> > checking the Gfx side audio register, although audio codec does not
> > report error.
> > >
> > > In addition, HDMI/DP cable hot plug will trigger Gfx driver to
> > disconnect/connect the pipeline, and port may be connected to a new
> > transcoder.
> > > If the transcoder has changed, the pin can return to D3 and muted again.
> > There are 3 pipelines in GPU, a pipe/port connection only affects one
> > pin, not the other pins and convertors.
> > > So our previous patch to delay powering up the whole codec once is
> > > not
> > enough and not suitable.
> >
> > This makes me a bit worried actually. If the transcoder to port
> > connection can be changed at runtime, how does this affect the default
> > pin config on the audio codec pin node?
> >
> > E g, if the machine only has one physical HDMI output, it would be a
> > good thing to mark only one of the pin nodes as a "jack" and the other
> > two as "not connected".
> >
> > Now, if the transcoder used for this HDMI output is varying, it can be
> > sometimes not matching the audio codec pin node set to "jack", and
> > then we have a problem...!
> >
> > Or is the gfx engine taking the default pin config of the audio codec
> > into account somehow?
> 
> I think the pin configuration default should not be affected, as it's for BIOS
> programming.
> 
> The DDI port is bound to a HDMI or DP connector on the board.
> And although the transcoder can be changed at runtime, the pin report the eld
> info does not change.
> 
> Anyway, we'll double check this issue.

The pin configuration default will not change after the DDI port is connected to a different transcoder.
I verified this on my haswell machine. 

Thanks
Mengdong 

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[David Henningsson]

On 05/06/2013 08:27 AM, Lin, Mengdong wrote:
> Hi David,
>
>> -----Original Message-----
>> From: alsa-devel-bounces@alsa-project.org
>> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Lin, Mengdong
>> Sent: Friday, May 03, 2013 3:30 PM
>> To: David Henningsson
>> Cc: Takashi Iwai; Li, Jocelyn; alsa-devel@alsa-project.org; Wang, Xingchao;
>> Girdwood, Liam R
>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
>> in system resume
>>
>>> -----Original Message-----
>>> From: David Henningsson [mailto:david.henningsson@canonical.com]
>>> Sent: Friday, May 03, 2013 3:20 PM
>>> To: Lin, Mengdong
>>> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R; Wang,
>>> Xingchao; Li, Jocelyn
>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell
>>> hdmi codec in system resume
>>>
>>> On 05/03/2013 08:56 AM, Lin, Mengdong wrote:
>>>> Hi Takashi and David,
>>>>
>>>>> -----Original Message-----
>>>>> From: Takashi Iwai [mailto:tiwai@suse.de]
>>>>> Sent: Wednesday, April 17, 2013 2:13 PM
>>>>> To: David Henningsson
>>>>> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; Girdwood, Liam R
>>>>> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell
>>>>> hdmi codec in system resume
>>>>>
>>>>> At Wed, 17 Apr 2013 07:51:47 +0200, David Henningsson wrote:
>>>>>>>> The dependency on the Gfx driver, is it both codec-wide and per
>>>>>>>> pin? It seems to be at least per pin, which means that whenever
>>>>>>>> we plug something in, we need to redo our initialization of that
>>>>>>>> pin after the Gfx driver has finished, is that correct?
>>>>
>>>> It was confirmed that this a per pin dependency, not codec-wide.
>>>>
>>>> For Haswell, every pin is in D3 with amplifier muted by default.
>>>> The audio driver must program the pin to D0 and unmute the pin,
>>>> after the gfx driver connect the GPU pipe and port, and enable the
>>>> transcoder (The
>>> transcoder is the component where audio is mixed with video).
>>>> Otherwise, the pin's default power and mute state will not change,
>>>> by
>>> checking the Gfx side audio register, although audio codec does not
>>> report error.
>>>>
>>>> In addition, HDMI/DP cable hot plug will trigger Gfx driver to
>>> disconnect/connect the pipeline, and port may be connected to a new
>>> transcoder.
>>>> If the transcoder has changed, the pin can return to D3 and muted again.
>>> There are 3 pipelines in GPU, a pipe/port connection only affects one
>>> pin, not the other pins and convertors.
>>>> So our previous patch to delay powering up the whole codec once is
>>>> not
>>> enough and not suitable.
>>>
>>> This makes me a bit worried actually. If the transcoder to port
>>> connection can be changed at runtime, how does this affect the default
>>> pin config on the audio codec pin node?
>>>
>>> E g, if the machine only has one physical HDMI output, it would be a
>>> good thing to mark only one of the pin nodes as a "jack" and the other
>>> two as "not connected".
>>>
>>> Now, if the transcoder used for this HDMI output is varying, it can be
>>> sometimes not matching the audio codec pin node set to "jack", and
>>> then we have a problem...!
>>>
>>> Or is the gfx engine taking the default pin config of the audio codec
>>> into account somehow?
>>
>> I think the pin configuration default should not be affected, as it's for BIOS
>> programming.
>>
>> The DDI port is bound to a HDMI or DP connector on the board.
>> And although the transcoder can be changed at runtime, the pin report the eld
>> info does not change.
>>
>> Anyway, we'll double check this issue.
>
> The pin configuration default will not change after the DDI port is connected to a different transcoder.
> I verified this on my haswell machine.

The core of the problem is that the DDI port can sometimes be connected 
to a transcoder that in turn is connected to an audio codec pin which 
default pin config is set to "not connected". In this case, audio will 
not work correctly. (It might sometimes work by accident.)

The solution is that DDI to transcoder connection cannot change, it must 
always be connected to the audio codec pin which is marked as "jack".

Btw, even if all audio codec pins are marked as "jack", it would be a 
bad habit to change the connection, because people will be confused when 
we show these pins to the user as e g "HDMI 1", "HDMI 2" and "HDMI 3", 
and which one is corresponding to the physical output is varying.

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

Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Lin, Mengdong]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson
> Sent: Monday, May 06, 2013 3:08 PM
> To: Lin, Mengdong
> Cc: Takashi Iwai; Li, Jocelyn; alsa-devel@alsa-project.org; Girdwood, Liam R;
> Wang, Xingchao
> Subject: Re: [alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec
> in system resume
> 
> The core of the problem is that the DDI port can sometimes be connected to a
> transcoder that in turn is connected to an audio codec pin which default pin
> config is set to "not connected". In this case, audio will not work correctly. (It
> might sometimes work by accident.)
> 
> The solution is that DDI to transcoder connection cannot change, it must always
> be connected to the audio codec pin which is marked as "jack".

Hi David,

The gfx driver will decide which pipe is used for a DDI port. And the connection can change by hot plug events.
The gfx driver will always try to use the 1st pipe if it's free, because the 1st pipe is not affected by the power down well.
Thus the user can get display while the other parts are turned off for power saving.

I think if everything is properly configured, the pin to DDI port mapping should not change even if the port is connected to a different transcoder.
There may be something wrong or missing in current audio or gfx driver.

> 
> Btw, even if all audio codec pins are marked as "jack", it would be a bad habit to
> change the connection, because people will be confused when we show these
> pins to the user as e g "HDMI 1", "HDMI 2" and "HDMI 3", and which one is
> corresponding to the physical output is varying.

Yes. This would confuse users.

Thanks
Mengdong