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

[Yang, Libin]

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Tuesday, January 8, 2019 7:15 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 Tue, 08 Jan 2019 08:53:49 +0100,
>Yang, Libin wrote:
>>
>>
>> >-----Original Message-----
>> >From: Pierre-Louis Bossart
>> >[mailto:pierre-louis.bossart at linux.intel.com]
>> >Sent: Tuesday, January 8, 2019 12:58 AM
>> >To: Yang, Libin <libin.yang at intel.com>; alsa-devel at alsa-project.org;
>> >tiwai at suse.de; broonie at kernel.org
>> >Cc: 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 1/6/19 8:22 PM, libin.yang at intel.com wrote:
>> >> From: Libin Yang <libin.yang at intel.com>
>> >>
>> >> hdmi_codec_prepare() will trigger hdmi runtime resume, which will
>> >> set the bitmask of hdev->addr. And skl_suspend() will clear the
>> >> bitmask of HDA_CODEC_IDX_CONTROLLER. HDMI codec idx is not the
>same
>> >> as HDA_CODEC_IDX_CONTROLLER, which means i915 power will not be
>> >released
>> >> when suspend.
>> >>
>> >> On the other hand, hdmi_codec_prepare() don't need to call
>> >> pm_runtime_get_sync() to wake up the audio subsystem (HDMI auido)
>> >> for setting the codec registers. Turning display power on with
>> >> snd_hdac_display_power() is enough.
>> >>
>> >> Let's use S3 without playback as an example:
>> >> hdmi_codec_prepare() invokes the runtime resume of codec =>
>> >>    snd_hdac_display_power(bus, hdev->addr, true) skl runtime resume
>> >> skl_suspend() =>
>> >>    snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
>> >>
>> >> THis means hdev->addr will never release the display power when
>> >> suspend.
>> >>
>> >> The new sequence will be:
>> >> hdmi_codec_prepare() =>
>> >>    snd_hdac_display_power(bus, hdev->addr, true)
>> >>    snd_hdac_display_power(bus, hdev->addr, false) skl runtime
>> >> resume skl suspned
>> >
>> >I for one find this RFC difficult to follow. The documentation of
>> >"Power management sequences" seems obsolete after Takashi's changes,
>> >you may want to start with an update of the documentation which might
>> >help point out what is broken in the sequences. I also don't see the
>> >point in trying to bypass the runtime_pm code in codec_prepare and
>> >codec_complete. if we have the display on/off sequence only in
>> >probe/remove and suspend/resume it makes it easy to track states, and
>> >I wonder if it makes sense anyways to be in suspend with the display
>> >on or vice-versa in D0 with the display off. Simple state machines always
>win.
>>
>> Thanks for comments. I will modify the documentation of "Power
>> Management sequences" when I submit the formal patch. The rough flow
>> is described in my patch description. Do you think I need add more
>> details?
>>
>> Actually, the HDMI driver is a little stranger. pm runtime resume of
>> HDMI is not called even prepare() return 0. What I understand is that
>> the runtime resume should be called. I asked a friend of mine who is
>> familiar with kernel power management. He also can't tell why. Maybe
>> our ALSA experts know why :).
>>
>> My patch is based on hdmi codec pm runtime resume not being called.
>>
>> The normal flow of suspend should be:
>> Pm runtime resume => power up
>> Pm suspend => power down.
>> Unfortunately, our ASoC HDMI codec driver doesn't have pm suspend
>> Callback (maybe this is why HDMI runtime resume is not called?)
>>
>> In original code, the hdmi prepare() will call pm_runtime_get_sync()
>> which will trigger hdmi pm runtime resume, which will turn on display
>power:
>> snd_hdac_display_power(hdev->bus, hdev->addr, true). And there is no
>> chance to turn off the 'hdev->addr' display power before suspend,
>> which is wrong.
>
>Well, the problem is that HD-audio controller takes the display power
>unnecessarily at suspend and resume.  Since the refactoring, this is
>superfluous and confuses the system.
>
>Also, I see no reason to stick with prepare and complete PM calls in
>hdac_hdmi driver; the display power is managed in each domain now, so we
>shouldn't rely on the refcount done by the controller driver any longer.
>
>Below is an untested patch I cooked up for simplification and fix for this issue.
>Could you check whether this works at all?

This patch makes the flow more clear now. Thanks for help.

I have done the test on sof audio driver.
Because SOF has issue of suspend when playback, I didn't do the test
of suspend with hdmi playing. I only test the audio suspend/resume
without playing anything.

The test result is:
Suspend/resume works well.
And after suspend/resume, playback works well.

However, I don't have platform to test SST driver. I tried to setup 
the environment for SST driver test, but failed. I will continue to 
setup the SST environment. On the meantime, I will ask Intel internal
team to help on SST driver test if they are willing to help.

And please see my comments below.

[...]

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

Regards,
Libin

>+	if (ret < 0)
>+		return ret;
> 	/*
> 	 * As the ELD notify callback request is not entertained while the
> 	 * device is in suspend state. Need to manually check detection of @@