On Thu, 10 Jan 2019 06:30:14 +0100, Yang, Libin wrote: > > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@xxxxxxx] > >Sent: Wednesday, January 9, 2019 5:14 PM > >To: Yang, Libin <libin.yang@xxxxxxxxx> > >Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>; alsa- > >devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; > >liam.r.girdwood@xxxxxxxxxxxxxxx; Lin, Mengdong <mengdong.lin@xxxxxxxxx> > >Subject: Re: [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. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
