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

Takashi Iwai tiwai at suse.de
Wed Apr 10 08:52:30 CEST 2013


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 at suse.de]
> >>>>>> Sent: Tuesday, April 09, 2013 3:43 PM
> >>>>>> To: David Henningsson
> >>>>>> Cc: Lin, Mengdong; alsa-devel at 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
> 


More information about the Alsa-devel mailing list