Skip to content

Commit

Permalink
ASoC: intel: skl: Fix display power regression
Browse files Browse the repository at this point in the history 
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: 029d92c ("ALSA: hda: Refactor display power management")
Reported-by: Libin Yang <libin.yang@intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
  • Loading branch information
Takashi Iwai committed Jan 14, 2019
1 parent 82aa0d7 commit 687ae9e
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 112 deletions.
116 changes: 13 additions & 103 deletions sound/soc/codecs/hdac_hdmi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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[] = {
Expand Down
13 changes: 4 additions & 9 deletions sound/soc/intel/skylake/skl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 687ae9e

Please sign in to comment.