[alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Yang, Libin]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Wednesday, January 9, 2019 5:14 PM
>To: Yang, Libin <libin.yang at intel.com>
>Cc: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>; alsa-
>devel at alsa-project.org; broonie at kernel.org;
>liam.r.girdwood at linux.intel.com; Lin, Mengdong <mengdong.lin at intel.com>
>Subject: Re: [alsa-devel] [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio
>suspend/resume
>
>On Wed, 09 Jan 2019 09:29:36 +0100,
>Takashi Iwai wrote:
>>
>> On Wed, 09 Jan 2019 09:16:34 +0100,
>> Yang, Libin wrote:
>> >
>> > >-
>> > >-static void hdmi_codec_complete(struct device *dev)
>> > >+#ifdef CONFIG_PM_SLEEP
>> > >+static int hdmi_codec_resume(struct device *dev)
>> > > {
>> > > 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>> > > 	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>> > >+	int ret;
>> > >
>> > >-	/* Power up afg */
>> > >-	snd_hdac_codec_read(hdev, hdev->afg, 0,
>> > >	AC_VERB_SET_POWER_STATE,
>> > >-							AC_PWRST_D0);
>> > >-
>> > >-	hdac_hdmi_skl_enable_all_pins(hdev);
>> > >-	hdac_hdmi_skl_enable_dp12(hdev);
>> > >-
>> > >+	ret = pm_runtime_force_resume(dev);
>> >
>> > The code hopes to call hdmi pm_runtime() whenever this resume() is
>> > called? If so, I'm afraid it will not work.
>> > pm_runtime_force_resume() calls pm_runtime with conditions.
>> > Sometimes pm_runtime will not be called. In my suspend/resume test,
>> > it shows that pm_runtime() is not called. This may cause the
>> > following function
>> > hdac_hdmi_present_sense_all_pins() not work well as there is no
>> > power on.
>>
>> Hm, right, maybe we should move hdac_hdmi_present_sense_all_pins()
>> into runtime resume.  It may be called unconditionally, but safer to
>> call it conditionally only after S3 or so...
>
>Now reading back the code again, no, it's correct to call
>hdac_hdmi_present_sense_all_pins() there.  The function doesn't access any
>HD-audio codec verbs but only with the i915 audio component.
>Hence it's safe to call there no matter whether the device is opened (thus
>resumed) or not.  And, it is even mandatory to call there; if a HDMI connection
>changed during suspend, we need to notify it right after the system resume.

Yes. It seems this is safe. Thanks for clarification. SOF audio driver test passes. 
No reply from our internal SST driver team. From SOF test, this patch can fix
the display power issue.

BTW: I have another question. Maybe it is not related to this topic, but
related the general audio power sequence. I'm not sure my understand
is correct or not.
Currently, codec device's parent is the card device, right? If so, the suspend
sequence is:
codec power off (child)
card power off (parent)

However, I see some cards will call trigger for suspend in card suspend
function. This may cause issue logically (Maybe the real cards are always
OK). The codec has already powered off. So if trigger() operates with
codec may cause issue? Even, if the codec is the master, maybe the clk
is off before trigger() is called. I'm not sure this is always OK.
(This is why I submitted the second patch.)

Regards,
Libin