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

[Takashi Iwai]

On Thu, 10 Jan 2019 06:30:14 +0100,
Yang, Libin wrote:
> 
> 
> >-----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.

OK, I'm going to queue this, but will delay the upstream merge for
5.0-rc3.

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

Indeed this looks like a problem, and it's specific to ASoC.
The legacy HD-audio codec driver has the base resume code that calls
snd_pcm_suspend_all() for the assigned PCM streams, while ASoC calls
it in snd_soc_suspend() that is invoked from the machine driver in
general.

If a codec driver has a strong dependency with the PCM state (like
HD-audio one), it'd need to stop in its suspend callback, something
like below.


thanks,

Takashi

---
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index b19d7a3e7a2c..caf512d807f5 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -106,6 +106,7 @@ struct hdac_hdmi_pcm {
 	struct list_head port_list;
 	struct hdac_hdmi_cvt *cvt;
 	struct snd_soc_jack *jack;
+	struct snd_pcm *snd_pcm;
 	int stream_tag;
 	int channels;
 	int format;
@@ -1792,6 +1793,7 @@ int hdac_hdmi_jack_init(struct snd_soc_dai *dai, int device,
 	mutex_init(&pcm->lock);
 	INIT_LIST_HEAD(&pcm->port_list);
 	snd_pcm = hdac_hdmi_get_pcm_from_id(dai->component->card, device);
+	pcm->snd_pcm = snd_pcm;
 	if (snd_pcm) {
 		err = snd_hdac_add_chmap_ctls(snd_pcm, device, &hdmi->chmap);
 		if (err < 0) {
@@ -2118,8 +2120,10 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev)
 static int hdac_hdmi_runtime_suspend(struct device *dev)
 {
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
+	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
 	struct hdac_bus *bus = hdev->bus;
 	struct hdac_ext_link *hlink = NULL;
+	struct hdac_hdmi_pcm *pcm;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
 
@@ -2127,6 +2131,12 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 	if (!bus)
 		return 0;
 
+	/* make sure all streams suspended before power down */
+	list_for_each_entry(pcm, &hdmi->pcm_list, head) {
+		if (pcm->snd_pcm)
+			snd_pcm_suspend_all(pcm->snd_pcm);
+	}
+
 	/*
 	 * Power down afg.
 	 * codec_read is preferred over codec_write to set the power state.