On Thu, 10 Jan 2019 06:30:14 +0100, Yang, Libin wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, January 9, 2019 5:14 PM To: Yang, Libin libin.yang@intel.com Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com; alsa- devel@alsa-project.org; broonie@kernel.org; liam.r.girdwood@linux.intel.com; Lin, Mengdong mengdong.lin@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.