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")
Reported-by: Libin Yang <libin.yang@intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>

2 files changed, 17 insertions, 112 deletions

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 3ab2949c1dfa..b19d7a3e7a2c 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1890,51 +1890,31 @@ 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
 	 * all pins here. pin capablity change is not support, so use the
 	 * already set pin caps.
+	 *
+	 * NOTE: this is safe to call even if the codec doesn't actually resume.
+	 * The pin check involves only with DRM audio component hooks, so it
+	 * works even if the HD-audio side is still dreaming peacefully.
 	 */
 	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 +2115,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 +2188,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);