On Tue, 08 Jan 2019 08:53:49 +0100, Yang, Libin wrote: > > > >-----Original Message----- > >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@xxxxxxxxxxxxxxx] > >Sent: Tuesday, January 8, 2019 12:58 AM > >To: Yang, Libin <libin.yang@xxxxxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx; > >tiwai@xxxxxxx; broonie@xxxxxxxxxx > >Cc: liam.r.girdwood@xxxxxxxxxxxxxxx; Lin, Mengdong > ><mengdong.lin@xxxxxxxxx> > >Subject: Re: [RFC PATCH v2 1/2] ASoC: refine ASoC hdmi audio > >suspend/resume > > > > > >On 1/6/19 8:22 PM, libin.yang@xxxxxxxxx wrote: > >> From: Libin Yang <libin.yang@xxxxxxxxx> > >> > >> 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? thanks, Takashi -- 8< -- From: Takashi Iwai <tiwai@xxxxxxx> Subject: [PATCH] ASoC: intel: skl: Fix display power regression Since the refactoring of HD-audio display power management, the display power status is managed per domain. Meanwhile the ASoC hdac_hdmi driver still keeps and relies (incorrectly) on the refcounting together with ASoC skl driver, and this leads to the display state always on. This patch is an attempt to address the regression by simplifying the PM code of ASoC skl and hdac_hdmi drivers. Basically, since the refactoring, we don't have to manage the display power at HD-audio controller suspend / resume but only at HD-audio HDMI codec suspend / resume. So the patch drops the superfluous snd_hdac_display_power() calls in skl driver. Meanwhile, in hdac_hdmi side, we rewrite the PM call just to re-use the runtime PM callbacks like other drivers do. Now the logic is simple: turn off at suspend and turn on at resume. The patch also fixes the possibly missing display-power off at skl driver removal as well as some error paths at probe. Fixes: 029d92c289bd ("ALSA: hda: Refactor display power management") Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> --- sound/soc/codecs/hdac_hdmi.c | 112 +++------------------------------- sound/soc/intel/skylake/skl.c | 13 ++-- 2 files changed, 13 insertions(+), 112 deletions(-) diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 3ab2949c1dfa..cb4668b82882 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1890,38 +1890,16 @@ static void hdmi_codec_remove(struct snd_soc_component *component) pm_runtime_disable(&hdev->dev); } -#ifdef CONFIG_PM -static int hdmi_codec_prepare(struct device *dev) -{ - struct hdac_device *hdev = dev_to_hdac_dev(dev); - - pm_runtime_get_sync(&hdev->dev); - - /* - * Power down afg. - * codec_read is preferred over codec_write to set the power state. - * This way verb is send to set the power state and response - * is received. So setting power state is ensured without using loop - * to read the state. - */ - snd_hdac_codec_read(hdev, hdev->afg, 0, AC_VERB_SET_POWER_STATE, - AC_PWRST_D3); - - return 0; -} - -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); + 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 @@ -1929,12 +1907,10 @@ static void hdmi_codec_complete(struct device *dev) * already set pin caps. */ hdac_hdmi_present_sense_all_pins(hdev, hdmi, false); - - pm_runtime_put_sync(&hdev->dev); + return 0; } #else -#define hdmi_codec_prepare NULL -#define hdmi_codec_complete NULL +#define hdmi_codec_resume NULL #endif static const struct snd_soc_component_driver hdmi_hda_codec = { @@ -2135,75 +2111,6 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev) } #ifdef CONFIG_PM -/* - * Power management sequences - * ========================== - * - * The following explains the PM handling of HDAC HDMI with its parent - * device SKL and display power usage - * - * Probe - * ----- - * In SKL probe, - * 1. skl_probe_work() powers up the display (refcount++ -> 1) - * 2. enumerates the codecs on the link - * 3. powers down the display (refcount-- -> 0) - * - * In HDAC HDMI probe, - * 1. hdac_hdmi_dev_probe() powers up the display (refcount++ -> 1) - * 2. probe the codec - * 3. put the HDAC HDMI device to runtime suspend - * 4. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0) - * - * Once children are runtime suspended, SKL device also goes to runtime - * suspend - * - * HDMI Playback - * ------------- - * Open HDMI device, - * 1. skl_runtime_resume() invoked - * 2. hdac_hdmi_runtime_resume() powers up the display (refcount++ -> 1) - * - * Close HDMI device, - * 1. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0) - * 2. skl_runtime_suspend() invoked - * - * S0/S3 Cycle with playback in progress - * ------------------------------------- - * When the device is opened for playback, the device is runtime active - * already and the display refcount is 1 as explained above. - * - * Entering to S3, - * 1. hdmi_codec_prepare() invoke the runtime resume of codec which just - * increments the PM runtime usage count of the codec since the device - * is in use already - * 2. skl_suspend() powers down the display (refcount-- -> 0) - * - * Wakeup from S3, - * 1. skl_resume() powers up the display (refcount++ -> 1) - * 2. hdmi_codec_complete() invokes the runtime suspend of codec which just - * decrements the PM runtime usage count of the codec since the device - * is in use already - * - * Once playback is stopped, the display refcount is set to 0 as explained - * above in the HDMI playback sequence. The PM handlings are designed in - * such way that to balance the refcount of display power when the codec - * device put to S3 while playback is going on. - * - * S0/S3 Cycle without playback in progress - * ---------------------------------------- - * Entering to S3, - * 1. hdmi_codec_prepare() invoke the runtime resume of codec - * 2. skl_runtime_resume() invoked - * 3. hdac_hdmi_runtime_resume() powers up the display (refcount++ -> 1) - * 4. skl_suspend() powers down the display (refcount-- -> 0) - * - * Wakeup from S3, - * 1. skl_resume() powers up the display (refcount++ -> 1) - * 2. hdmi_codec_complete() invokes the runtime suspend of codec - * 3. hdac_hdmi_runtime_suspend() powers down the display (refcount-- -> 0) - * 4. skl_runtime_suspend() invoked - */ static int hdac_hdmi_runtime_suspend(struct device *dev) { struct hdac_device *hdev = dev_to_hdac_dev(dev); @@ -2277,8 +2184,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev) static const struct dev_pm_ops hdac_hdmi_pm = { SET_RUNTIME_PM_OPS(hdac_hdmi_runtime_suspend, hdac_hdmi_runtime_resume, NULL) - .prepare = hdmi_codec_prepare, - .complete = hdmi_codec_complete, + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, hdmi_codec_resume) }; static const struct hda_device_id hdmi_list[] = { diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 60c94836bf5b..4ed5b7e17d44 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -336,9 +336,6 @@ static int skl_suspend(struct device *dev) skl->skl_sst->fw_loaded = false; } - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); - return 0; } @@ -350,10 +347,6 @@ static int skl_resume(struct device *dev) struct hdac_ext_link *hlink = NULL; int ret; - /* Turned OFF in HDMI codec driver after codec reconfiguration */ - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); - /* * resume only when we are not in suspend active, otherwise need to * restore the device @@ -446,8 +439,10 @@ static int skl_free(struct hdac_bus *bus) snd_hdac_ext_bus_exit(bus); cancel_work_sync(&skl->probe_work); - if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) + if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) { + snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false); snd_hdac_i915_exit(bus); + } return 0; } @@ -814,7 +809,7 @@ static void skl_probe_work(struct work_struct *work) err = skl_platform_register(bus->dev); if (err < 0) { dev_err(bus->dev, "platform register failed: %d\n", err); - return; + goto out_err; } err = skl_machine_device_register(skl); -- 2.20.1 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
