Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio suspend/resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux