[alsa-devel] [PATCH 3/3] ALSA: hda - delay resuming Haswell HDMI codec in system resume

[Takashi Iwai]

At Mon,  8 Apr 2013 13:54:55 -0400,
mengdong.lin at intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin at 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.
> 
> In this patch, Haswell codec will claim it needs "delaying resume" by setting
> flag "support_delay_resume", so system resume will skip it and mark it as
> "delay_resumed". On later codec access, Haswell codec will be resumed and
> hda_call_codec_resume() will call its set_power_states ops intel_haswell_set_
> power_state(). For 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 at intel.com>
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index ede8215..f39e374 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -89,6 +89,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
>  };
>  
>  
> @@ -975,6 +983,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(get_pin(spec, pin_idx), 1);
>  	snd_hda_jack_report_sync(codec);
>  }
> @@ -1725,6 +1740,11 @@ 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);
>  	}
> +
> +#ifdef CONFIG_PM
> +	spec->ready_to_resume = 0;
> +#endif
> +
>  	return 0;
>  }
>  
> @@ -1855,6 +1875,61 @@ 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;
> +
> +	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 = get_pin(spec, 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");
> +}

IMO, this should be shown no matter whether with or w/o debug option,
i.e. use snd_printk().

> +}
> +
> +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) {

A superfluous space after '=='

> +		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)

You don't have to put inline for such a case.
Rely more on the compiler.  It shall do that anyway.

> +{
> +	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;

No need for a new line...

> +}
> +#else
> +define intel_haswell_allow_delay_resume NULL

This should be an empty function instead

#define intel_haswell_allow_delay_resume() do { } while (0)


Takashi

> +#endif
> +
>  static int patch_generic_hdmi(struct hda_codec *codec)
>  {
>  	struct hdmi_spec *spec;
> @@ -1878,6 +1953,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
>