[PATCH v2 0/7] ALSA: HD-audio display power fixes

[PATCH v2 0/7] ALSA: HD-audio display power fixes

[Takashi Iwai]

Hi,

this patch a revised patchset for fixing possible HD-audio display
power unbalance.
Basically the "fix" is done by refactoring the whole relevant code.
It starts from the Intel HD-audio runtime PM refactoring, followed
by the display PM API change, and lots of code cleanups.

Since it changes the display power API function, it hits both legacy
and ASoC drivers.

The patches are kept in topic/hda-pm-refactor branch of my sound git
tree.  This will be an immutable branch once when merged to for-next,
so that it can be merged to ASoC tree if any conflicting change needs
to be applied.

The major change in v2 series is the complete removal of
AZX_DCAPS_I915_POWERWELL flag in patch#3, and other minor fixes
suggested by Pierre.


Takashi

===

Takashi Iwai (7):
  ALSA: hda/intel: Refactoring PM code
  ALSA: hda: Refactor display power management
  ALSA: hda/intel: Drop superfluous AZX_DCAPS_I915_POWERWELL checks
  ALSA: hda/intel: Properly free the display power at error path
  ALSA: hda: Make snd_hdac_display_power() void function
  ASoC: hdac_hdmi: Add missing display power-off at driver removal
  ALSA: hda/hdmi: Always set display_power_control for Intel HSW+ codecs

 include/sound/hda_codec.h      |   1 +
 include/sound/hda_component.h  |  11 +-
 include/sound/hdaudio.h        |   7 +-
 sound/hda/hdac_component.c     |  39 +++---
 sound/hda/hdac_device.c        |  17 ---
 sound/pci/hda/hda_codec.c      |  16 ++-
 sound/pci/hda/hda_controller.c |  11 --
 sound/pci/hda/hda_controller.h |   6 +-
 sound/pci/hda/hda_intel.c      | 220 +++++++++++++--------------------
 sound/pci/hda/patch_hdmi.c     |   8 +-
 sound/soc/codecs/hdac_hdmi.c   |  24 +---
 sound/soc/intel/skylake/skl.c  |  40 ++----
 12 files changed, 148 insertions(+), 252 deletions(-)

-- 
2.19.2

[PATCH v2 1/7] ALSA: hda/intel: Refactoring PM code

[Takashi Iwai]

Make unified suspend / resume helpers and call them from both the
runtime- and the system-PM callbacks for simplifying code.

There are slight changes of call orders, but there shouldn't be any
functional difference after refactoring.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c | 160 ++++++++++++++++----------------------
 1 file changed, 69 insertions(+), 91 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 76f03abd15ab..cc06a323c817 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -930,35 +930,82 @@ static int param_set_xint(const char *val, const struct kernel_param *kp)
 	mutex_unlock(&card_list_lock);
 	return 0;
 }
-#else
-#define azx_add_card_list(chip) /* NOP */
-#define azx_del_card_list(chip) /* NOP */
-#endif /* CONFIG_PM */
 
-#ifdef CONFIG_PM_SLEEP
 /*
  * power management
  */
-static int azx_suspend(struct device *dev)
+static bool azx_is_pm_ready(struct snd_card *card)
 {
-	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
 	struct hda_intel *hda;
-	struct hdac_bus *bus;
 
 	if (!card)
-		return 0;
-
+		return false;
 	chip = card->private_data;
 	hda = container_of(chip, struct hda_intel, chip);
 	if (chip->disabled || hda->init_failed || !chip->running)
+		return false;
+	return true;
+}
+
+static void __azx_runtime_suspend(struct azx *chip)
+{
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+
+	azx_stop_chip(chip);
+	azx_enter_link_reset(chip);
+	azx_clear_irq_pending(chip);
+	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
+	    hda->need_i915_power)
+		snd_hdac_display_power(azx_bus(chip), false);
+}
+
+static void __azx_runtime_resume(struct azx *chip)
+{
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+	struct hdac_bus *bus = azx_bus(chip);
+	struct hda_codec *codec;
+	int status;
+
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
+		snd_hdac_display_power(bus, true);
+		if (hda->need_i915_power)
+			snd_hdac_i915_set_bclk(bus);
+	}
+
+	/* Read STATESTS before controller reset */
+	status = azx_readw(chip, STATESTS);
+
+	azx_init_pci(chip);
+	hda_intel_init_chip(chip, true);
+
+	if (status) {
+		list_for_each_codec(codec, &chip->bus)
+			if (status & (1 << codec->addr))
+				schedule_delayed_work(&codec->jackpoll_work,
+						      codec->jackpoll_interval);
+	}
+
+	/* power down again for link-controlled chips */
+	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
+	    !hda->need_i915_power)
+		snd_hdac_display_power(bus, false);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int azx_suspend(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip;
+	struct hdac_bus *bus;
+
+	if (!azx_is_pm_ready(card))
 		return 0;
 
+	chip = card->private_data;
 	bus = azx_bus(chip);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
-	azx_clear_irq_pending(chip);
-	azx_stop_chip(chip);
-	azx_enter_link_reset(chip);
+	__azx_runtime_suspend(chip);
 	if (bus->irq >= 0) {
 		free_irq(bus->irq, chip);
 		bus->irq = -1;
@@ -966,9 +1013,6 @@ static int azx_suspend(struct device *dev)
 
 	if (chip->msi)
 		pci_disable_msi(chip->pci);
-	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		&& hda->need_i915_power)
-		snd_hdac_display_power(bus, false);
 
 	trace_azx_suspend(chip);
 	return 0;
@@ -976,41 +1020,19 @@ static int azx_suspend(struct device *dev)
 
 static int azx_resume(struct device *dev)
 {
-	struct pci_dev *pci = to_pci_dev(dev);
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
-	struct hda_intel *hda;
-	struct hdac_bus *bus;
 
-	if (!card)
+	if (!azx_is_pm_ready(card))
 		return 0;
 
 	chip = card->private_data;
-	hda = container_of(chip, struct hda_intel, chip);
-	bus = azx_bus(chip);
-	if (chip->disabled || hda->init_failed || !chip->running)
-		return 0;
-
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		snd_hdac_display_power(bus, true);
-		if (hda->need_i915_power)
-			snd_hdac_i915_set_bclk(bus);
-	}
-
 	if (chip->msi)
-		if (pci_enable_msi(pci) < 0)
+		if (pci_enable_msi(chip->pci) < 0)
 			chip->msi = 0;
 	if (azx_acquire_irq(chip, 1) < 0)
 		return -EIO;
-	azx_init_pci(chip);
-
-	hda_intel_init_chip(chip, true);
-
-	/* power down again for link-controlled chips */
-	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
-	    !hda->need_i915_power)
-		snd_hdac_display_power(bus, false);
-
+	__azx_runtime_resume(chip);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 
 	trace_azx_resume(chip);
@@ -1045,21 +1067,14 @@ static int azx_thaw_noirq(struct device *dev)
 }
 #endif /* CONFIG_PM_SLEEP */
 
-#ifdef CONFIG_PM
 static int azx_runtime_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
-	struct hda_intel *hda;
 
-	if (!card)
+	if (!azx_is_pm_ready(card))
 		return 0;
-
 	chip = card->private_data;
-	hda = container_of(chip, struct hda_intel, chip);
-	if (chip->disabled || hda->init_failed)
-		return 0;
-
 	if (!azx_has_pm_runtime(chip))
 		return 0;
 
@@ -1067,13 +1082,7 @@ static int azx_runtime_suspend(struct device *dev)
 	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
 		  STATESTS_INT_MASK);
 
-	azx_stop_chip(chip);
-	azx_enter_link_reset(chip);
-	azx_clear_irq_pending(chip);
-	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		&& hda->need_i915_power)
-		snd_hdac_display_power(azx_bus(chip), false);
-
+	__azx_runtime_suspend(chip);
 	trace_azx_runtime_suspend(chip);
 	return 0;
 }
@@ -1082,51 +1091,18 @@ static int azx_runtime_resume(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip;
-	struct hda_intel *hda;
-	struct hdac_bus *bus;
-	struct hda_codec *codec;
-	int status;
 
-	if (!card)
+	if (!azx_is_pm_ready(card))
 		return 0;
-
 	chip = card->private_data;
-	hda = container_of(chip, struct hda_intel, chip);
-	bus = azx_bus(chip);
-	if (chip->disabled || hda->init_failed)
-		return 0;
-
 	if (!azx_has_pm_runtime(chip))
 		return 0;
-
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		snd_hdac_display_power(bus, true);
-		if (hda->need_i915_power)
-			snd_hdac_i915_set_bclk(bus);
-	}
-
-	/* Read STATESTS before controller reset */
-	status = azx_readw(chip, STATESTS);
-
-	azx_init_pci(chip);
-	hda_intel_init_chip(chip, true);
-
-	if (status) {
-		list_for_each_codec(codec, &chip->bus)
-			if (status & (1 << codec->addr))
-				schedule_delayed_work(&codec->jackpoll_work,
-						      codec->jackpoll_interval);
-	}
+	__azx_runtime_resume(chip);
 
 	/* disable controller Wake Up event*/
 	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
 			~STATESTS_INT_MASK);
 
-	/* power down again for link-controlled chips */
-	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
-	    !hda->need_i915_power)
-		snd_hdac_display_power(bus, false);
-
 	trace_azx_runtime_resume(chip);
 	return 0;
 }
@@ -1167,6 +1143,8 @@ static const struct dev_pm_ops azx_pm = {
 
 #define AZX_PM_OPS	&azx_pm
 #else
+#define azx_add_card_list(chip) /* NOP */
+#define azx_del_card_list(chip) /* NOP */
 #define AZX_PM_OPS	NULL
 #endif /* CONFIG_PM */
 
-- 
2.19.2

[PATCH v2 2/7] ALSA: hda: Refactor display power management

[Takashi Iwai]

The current HD-audio code manages the DRM audio power via too complex
redirections, and this seems even still unbalanced in a corner case as
Intel DRM CI has been intermittently reporting.  This patch is a big
surgery for addressing the complexity and the possible unbalance.

Basically the patch changes the display PM in the following ways:

- Both HD-audio controller and codec drivers call a single helper,
  snd_hdac_display_power().  (Formerly, the display power control from
  a codec was done indirectly via link_power bus ops.)

- snd_hdac_display_power() receives the codec address index.  For
  turning on/off from the controller, pass HDA_CODEC_IDX_CONTROLLER.

- snd_hdac_display_power() doesn't manage refcounts any longer, but
  keeps the power status in bitmap.  If any of controller or codecs is
  turned on, the function updates the DRM power state via get_power()
  or put_power().

Also this refactor allows us more cleanup:

- The link_power bus ops is dropped, so there is no longer indirect
  management, as mentioned in the above.

- hdac_device link_power_control flag is moved to hda_codec
  display_power_control flag, as it's only for HDA legacy.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106525
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Define HDA_CODEC_IDX_CONTROLLER with HDA_MAX_CODECS

 include/sound/hda_codec.h      |  1 +
 include/sound/hda_component.h  | 10 ++++++++--
 include/sound/hdaudio.h        |  7 ++-----
 sound/hda/hdac_component.c     | 35 +++++++++++++++++++++-------------
 sound/hda/hdac_device.c        | 17 -----------------
 sound/pci/hda/hda_codec.c      | 16 ++++++++++++----
 sound/pci/hda/hda_controller.c | 11 -----------
 sound/pci/hda/hda_intel.c      | 22 ++++++++-------------
 sound/pci/hda/patch_hdmi.c     |  4 ++--
 sound/soc/codecs/hdac_hdmi.c   |  7 +++----
 sound/soc/intel/skylake/skl.c  | 10 +++++-----
 11 files changed, 63 insertions(+), 77 deletions(-)

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index 0d98bb9068b1..7fa48b100936 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -236,6 +236,7 @@ struct hda_codec {
 	/* misc flags */
 	unsigned int in_freeing:1; /* being released */
 	unsigned int registered:1; /* codec was registered */
+	unsigned int display_power_control:1; /* needs display power */
 	unsigned int spdif_status_reset :1; /* needs to toggle SPDIF for each
 					     * status change
 					     * (e.g. Realtek codecs)
diff --git a/include/sound/hda_component.h b/include/sound/hda_component.h
index 78626cde7081..767c8d8a0230 100644
--- a/include/sound/hda_component.h
+++ b/include/sound/hda_component.h
@@ -5,10 +5,15 @@
 #define __SOUND_HDA_COMPONENT_H
 
 #include <drm/drm_audio_component.h>
+#include <sound/hdaudio.h>
+
+/* virtual idx for controller */
+#define HDA_CODEC_IDX_CONTROLLER	HDA_MAX_CODECS
 
 #ifdef CONFIG_SND_HDA_COMPONENT
 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
-int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
+int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx,
+			   bool enable);
 int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid,
 			     int dev_id, int rate);
 int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id,
@@ -25,7 +30,8 @@ static inline int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
 {
 	return 0;
 }
-static inline int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
+static inline int snd_hdac_display_power(struct hdac_bus *bus,
+					 unsigned int idx, bool enable)
 {
 	return 0;
 }
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index cd1773d0e08f..940e2b282133 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -79,7 +79,6 @@ struct hdac_device {
 
 	/* misc flags */
 	atomic_t in_pm;		/* suspend/resume being performed */
-	bool  link_power_control:1;
 
 	/* sysfs */
 	struct hdac_widget_tree *widgets;
@@ -237,8 +236,6 @@ struct hdac_bus_ops {
 	/* get a response from the last command */
 	int (*get_response)(struct hdac_bus *bus, unsigned int addr,
 			    unsigned int *res);
-	/* control the link power  */
-	int (*link_power)(struct hdac_bus *bus, bool enable);
 };
 
 /*
@@ -363,7 +360,8 @@ struct hdac_bus {
 
 	/* DRM component interface */
 	struct drm_audio_component *audio_component;
-	int drm_power_refcount;
+	long display_power_status;
+	bool display_power_active;
 
 	/* parameters required for enhanced capabilities */
 	int num_streams;
@@ -404,7 +402,6 @@ int snd_hdac_bus_send_cmd(struct hdac_bus *bus, unsigned int val);
 int snd_hdac_bus_get_response(struct hdac_bus *bus, unsigned int addr,
 			      unsigned int *res);
 int snd_hdac_bus_parse_capabilities(struct hdac_bus *bus);
-int snd_hdac_link_power(struct hdac_device *codec, bool enable);
 
 bool snd_hdac_bus_init_chip(struct hdac_bus *bus, bool full_reset);
 void snd_hdac_bus_stop_chip(struct hdac_bus *bus);
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index 6e46a9c73aed..dd766414436b 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -54,38 +54,45 @@ EXPORT_SYMBOL_GPL(snd_hdac_set_codec_wakeup);
 /**
  * snd_hdac_display_power - Power up / down the power refcount
  * @bus: HDA core bus
+ * @idx: HDA codec address, pass HDA_CODEC_IDX_CONTROLLER for controller
  * @enable: power up or down
  *
- * This function is supposed to be used only by a HD-audio controller
- * driver that needs the interaction with graphics driver.
+ * This function is used by either HD-audio controller or codec driver that
+ * needs the interaction with graphics driver.
  *
- * This function manages a refcount and calls the get_power() and
+ * This function updates the power status, and calls the get_power() and
  * put_power() ops accordingly, toggling the codec wakeup, too.
  *
  * Returns zero for success or a negative error code.
  */
-int snd_hdac_display_power(struct hdac_bus *bus, bool enable)
+int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
 {
 	struct drm_audio_component *acomp = bus->audio_component;
 
-	if (!acomp || !acomp->ops)
-		return -ENODEV;
-
 	dev_dbg(bus->dev, "display power %s\n",
 		enable ? "enable" : "disable");
+	if (enable)
+		set_bit(idx, &bus->display_power_status);
+	else
+		clear_bit(idx, &bus->display_power_status);
 
-	if (enable) {
-		if (!bus->drm_power_refcount++) {
+	if (!acomp || !acomp->ops)
+		return 0;
+
+	if (bus->display_power_status) {
+		if (!bus->display_power_active) {
 			if (acomp->ops->get_power)
 				acomp->ops->get_power(acomp->dev);
 			snd_hdac_set_codec_wakeup(bus, true);
 			snd_hdac_set_codec_wakeup(bus, false);
+			bus->display_power_active = true;
 		}
 	} else {
-		WARN_ON(!bus->drm_power_refcount);
-		if (!--bus->drm_power_refcount)
+		if (bus->display_power_active) {
 			if (acomp->ops->put_power)
 				acomp->ops->put_power(acomp->dev);
+			bus->display_power_active = false;
+		}
 	}
 
 	return 0;
@@ -321,10 +328,12 @@ int snd_hdac_acomp_exit(struct hdac_bus *bus)
 	if (!acomp)
 		return 0;
 
-	WARN_ON(bus->drm_power_refcount);
-	if (bus->drm_power_refcount > 0 && acomp->ops)
+	if (WARN_ON(bus->display_power_active) && acomp->ops)
 		acomp->ops->put_power(acomp->dev);
 
+	bus->display_power_active = false;
+	bus->display_power_status = 0;
+
 	component_master_del(dev, &hdac_component_master_ops);
 
 	bus->audio_component = NULL;
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index dbf02a3a8d2f..95b073ee4b32 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -622,23 +622,6 @@ int snd_hdac_power_down_pm(struct hdac_device *codec)
 EXPORT_SYMBOL_GPL(snd_hdac_power_down_pm);
 #endif
 
-/**
- * snd_hdac_link_power - Enable/disable the link power for a codec
- * @codec: the codec object
- * @bool: enable or disable the link power
- */
-int snd_hdac_link_power(struct hdac_device *codec, bool enable)
-{
-	if  (!codec->link_power_control)
-		return 0;
-
-	if  (codec->bus->ops->link_power)
-		return codec->bus->ops->link_power(codec->bus, enable);
-	else
-		return -EINVAL;
-}
-EXPORT_SYMBOL_GPL(snd_hdac_link_power);
-
 /* codec vendor labels */
 struct hda_vendor_id {
 	unsigned int id;
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 0957813939e5..9f8d59e7e89f 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -36,6 +36,7 @@
 #include "hda_beep.h"
 #include "hda_jack.h"
 #include <sound/hda_hwdep.h>
+#include <sound/hda_component.h>
 
 #define codec_in_pm(codec)		snd_hdac_is_in_pm(&codec->core)
 #define hda_codec_is_power_on(codec)	snd_hdac_is_power_on(&codec->core)
@@ -799,6 +800,13 @@ void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec)
 static unsigned int hda_set_power_state(struct hda_codec *codec,
 				unsigned int power_state);
 
+/* enable/disable display power per codec */
+static void codec_display_power(struct hda_codec *codec, bool enable)
+{
+	if (codec->display_power_control)
+		snd_hdac_display_power(&codec->bus->core, codec->addr, enable);
+}
+
 /* also called from hda_bind.c */
 void snd_hda_codec_register(struct hda_codec *codec)
 {
@@ -806,7 +814,7 @@ void snd_hda_codec_register(struct hda_codec *codec)
 		return;
 	if (device_is_registered(hda_codec_dev(codec))) {
 		snd_hda_register_beep_device(codec);
-		snd_hdac_link_power(&codec->core, true);
+		codec_display_power(codec, true);
 		pm_runtime_enable(hda_codec_dev(codec));
 		/* it was powered up in snd_hda_codec_new(), now all done */
 		snd_hda_power_down(codec);
@@ -834,7 +842,7 @@ static int snd_hda_codec_dev_free(struct snd_device *device)
 
 	codec->in_freeing = 1;
 	snd_hdac_device_unregister(&codec->core);
-	snd_hdac_link_power(&codec->core, false);
+	codec_display_power(codec, false);
 	put_device(hda_codec_dev(codec));
 	return 0;
 }
@@ -2926,7 +2934,7 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	    (codec_has_clkstop(codec) && codec_has_epss(codec) &&
 	     (state & AC_PWRST_CLK_STOP_OK)))
 		snd_hdac_codec_link_down(&codec->core);
-	snd_hdac_link_power(&codec->core, false);
+	codec_display_power(codec, false);
 	return 0;
 }
 
@@ -2934,7 +2942,7 @@ static int hda_codec_runtime_resume(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
 
-	snd_hdac_link_power(&codec->core, true);
+	codec_display_power(codec, true);
 	snd_hdac_codec_link_up(&codec->core);
 	hda_call_codec_resume(codec);
 	pm_runtime_mark_last_busy(dev);
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index fe2506672a72..532e081f8b8a 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -989,20 +989,9 @@ static int azx_get_response(struct hdac_bus *bus, unsigned int addr,
 		return azx_rirb_get_response(bus, addr, res);
 }
 
-static int azx_link_power(struct hdac_bus *bus, bool enable)
-{
-	struct azx *chip = bus_to_azx(bus);
-
-	if (chip->ops->link_power)
-		return chip->ops->link_power(chip, enable);
-	else
-		return -EINVAL;
-}
-
 static const struct hdac_bus_ops bus_core_ops = {
 	.command = azx_send_cmd,
 	.get_response = azx_get_response,
-	.link_power = azx_link_power,
 };
 
 #ifdef CONFIG_SND_HDA_DSP_LOADER
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index cc06a323c817..9f67425d5039 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -667,13 +667,8 @@ static int azx_position_check(struct azx *chip, struct azx_dev *azx_dev)
 	return 0;
 }
 
-/* Enable/disable i915 display power for the link */
-static int azx_intel_link_power(struct azx *chip, bool enable)
-{
-	struct hdac_bus *bus = azx_bus(chip);
-
-	return snd_hdac_display_power(bus, enable);
-}
+#define display_power(chip, enable) \
+	snd_hdac_display_power(azx_bus(chip), HDA_CODEC_IDX_CONTROLLER, enable)
 
 /*
  * Check whether the current DMA position is acceptable for updating
@@ -957,7 +952,7 @@ static void __azx_runtime_suspend(struct azx *chip)
 	azx_clear_irq_pending(chip);
 	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
 	    hda->need_i915_power)
-		snd_hdac_display_power(azx_bus(chip), false);
+		display_power(chip, false);
 }
 
 static void __azx_runtime_resume(struct azx *chip)
@@ -968,7 +963,7 @@ static void __azx_runtime_resume(struct azx *chip)
 	int status;
 
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		snd_hdac_display_power(bus, true);
+		display_power(chip, true);
 		if (hda->need_i915_power)
 			snd_hdac_i915_set_bclk(bus);
 	}
@@ -989,7 +984,7 @@ static void __azx_runtime_resume(struct azx *chip)
 	/* power down again for link-controlled chips */
 	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
 	    !hda->need_i915_power)
-		snd_hdac_display_power(bus, false);
+		display_power(chip, false);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1355,7 +1350,7 @@ static int azx_free(struct azx *chip)
 
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
 		if (hda->need_i915_power)
-			snd_hdac_display_power(bus, false);
+			display_power(chip, false);
 	}
 	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT)
 		snd_hdac_i915_exit(bus);
@@ -2056,7 +2051,6 @@ static const struct hda_controller_ops pci_hda_ops = {
 	.disable_msi_reset_irq = disable_msi_reset_irq,
 	.pcm_mmap_prepare = pcm_mmap_prepare,
 	.position_check = azx_position_check,
-	.link_power = azx_intel_link_power,
 };
 
 static int azx_probe(struct pci_dev *pci,
@@ -2239,7 +2233,7 @@ static int azx_probe_continue(struct azx *chip)
 		if (CONTROLLER_IN_GPU(pci))
 			hda->need_i915_power = 1;
 
-		err = snd_hdac_display_power(bus, true);
+		err = display_power(chip, true);
 		if (err < 0) {
 			dev_err(chip->card->dev,
 				"Cannot turn on display power on i915\n");
@@ -2295,7 +2289,7 @@ static int azx_probe_continue(struct azx *chip)
 out_free:
 	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
 		&& !hda->need_i915_power)
-		snd_hdac_display_power(bus, false);
+		display_power(chip, false);
 
 i915_power_fail:
 	if (err < 0)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 67099cbb6be2..30fe4dbdb0ae 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2620,7 +2620,7 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid)
 	 * can cover the codec power request, and so need not set this flag.
 	 */
 	if (!is_haswell(codec) && !is_broadwell(codec))
-		codec->core.link_power_control = 1;
+		codec->display_power_control = 1;
 
 	codec->patch_ops.set_power_state = haswell_set_power_state;
 	codec->depop_delay = 0;
@@ -2656,7 +2656,7 @@ static int patch_i915_byt_hdmi(struct hda_codec *codec)
 	/* For Valleyview/Cherryview, only the display codec is in the display
 	 * power well and can use link_power ops to request/release the power.
 	 */
-	codec->core.link_power_control = 1;
+	codec->display_power_control = 1;
 
 	codec->depop_delay = 0;
 	codec->auto_runtime_pm = 1;
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index e63d6e33df48..c3d551d2af7f 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -2031,14 +2031,13 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
 	 * Turned off in the runtime_suspend during the first explicit
 	 * pm_runtime_suspend call.
 	 */
-	ret = snd_hdac_display_power(hdev->bus, true);
+	ret = snd_hdac_display_power(hdev->bus, hdev->addr, true);
 	if (ret < 0) {
 		dev_err(&hdev->dev,
 			"Cannot turn on display power on i915 err: %d\n",
 			ret);
 		return ret;
 	}
-
 	ret = hdac_hdmi_parse_and_map_nid(hdev, &hdmi_dais, &num_dais);
 	if (ret < 0) {
 		dev_err(&hdev->dev,
@@ -2196,7 +2195,7 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 
 	snd_hdac_ext_bus_link_put(bus, hlink);
 
-	err = snd_hdac_display_power(bus, false);
+	err = snd_hdac_display_power(bus, hdev->addr, false);
 	if (err < 0)
 		dev_err(dev, "Cannot turn off display power on i915\n");
 
@@ -2224,7 +2223,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 
 	snd_hdac_ext_bus_link_get(bus, hlink);
 
-	err = snd_hdac_display_power(bus, true);
+	err = snd_hdac_display_power(bus, hdev->addr, true);
 	if (err < 0) {
 		dev_err(dev, "Cannot turn on display power on i915\n");
 		return err;
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 7487f388e65d..64f8433ae921 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -334,7 +334,7 @@ static int skl_suspend(struct device *dev)
 	}
 
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		ret = snd_hdac_display_power(bus, false);
+		ret = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 		if (ret < 0)
 			dev_err(bus->dev,
 				"Cannot turn OFF display power on i915\n");
@@ -353,7 +353,7 @@ static int skl_resume(struct device *dev)
 
 	/* Turned OFF in HDMI codec driver after codec reconfiguration */
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		ret = snd_hdac_display_power(bus, true);
+		ret = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
 		if (ret < 0) {
 			dev_err(bus->dev,
 				"Cannot turn on display power on i915\n");
@@ -783,7 +783,7 @@ static int skl_i915_init(struct hdac_bus *bus)
 	if (err < 0)
 		return err;
 
-	err = snd_hdac_display_power(bus, true);
+	err = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
 	if (err < 0)
 		dev_err(bus->dev, "Cannot turn on display power on i915\n");
 
@@ -838,7 +838,7 @@ static void skl_probe_work(struct work_struct *work)
 		snd_hdac_ext_bus_link_put(bus, hlink);
 
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		err = snd_hdac_display_power(bus, false);
+		err = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 		if (err < 0) {
 			dev_err(bus->dev, "Cannot turn off display power on i915\n");
 			skl_machine_device_unregister(skl);
@@ -855,7 +855,7 @@ static void skl_probe_work(struct work_struct *work)
 
 out_err:
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
-		err = snd_hdac_display_power(bus, false);
+		err = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 }
 
 /*
-- 
2.19.2

[PATCH v2 3/7] ALSA: hda/intel: Drop superfluous AZX_DCAPS_I915_POWERWELL checks

[Takashi Iwai]

snd_hdac_display_power() can be called even for a HDA controller
without DRM binding.  The same is true for other helpers,
snd_hdac_i915_set_bclk() and snd_hdac_set_codec_wakeup().
So all superfluous AZX_DCAPS_I915_POWERWELL  checks in hda_intel.c can
be dropped, and the definition of AZX_DCAPS_I915_POWERWELL itself can
be removed as well.  This simplifies the code a lot.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Drop AZX_DCAPS_I915_POWERWELL completely

 sound/pci/hda/hda_controller.h |  6 +--
 sound/pci/hda/hda_intel.c      | 73 +++++++++++++---------------------
 2 files changed, 28 insertions(+), 51 deletions(-)

diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index c95097bb5a0c..7185ed574b41 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -50,11 +50,7 @@
 /* 24 unused */
 #define AZX_DCAPS_COUNT_LPIB_DELAY  (1 << 25)	/* Take LPIB as delay */
 #define AZX_DCAPS_PM_RUNTIME	(1 << 26)	/* runtime PM support */
-#ifdef CONFIG_SND_HDA_I915
-#define AZX_DCAPS_I915_POWERWELL (1 << 27)	/* HSW i915 powerwell support */
-#else
-#define AZX_DCAPS_I915_POWERWELL 0		/* NOP */
-#endif
+/* 27 unused */
 #define AZX_DCAPS_CORBRP_SELF_CLEAR (1 << 28)	/* CORBRP clears itself after reset */
 #define AZX_DCAPS_NO_MSI64      (1 << 29)	/* Stick to 32-bit MSIs */
 #define AZX_DCAPS_SEPARATE_STREAM_TAG	(1 << 30) /* capture and playback use separate stream tag */
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 9f67425d5039..663e86effa1f 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -310,31 +310,28 @@ enum {
 #define AZX_DCAPS_INTEL_HASWELL \
 	(/*AZX_DCAPS_ALIGN_BUFSIZE |*/ AZX_DCAPS_COUNT_LPIB_DELAY |\
 	 AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_COMPONENT |\
-	 AZX_DCAPS_I915_POWERWELL | AZX_DCAPS_SNOOP_TYPE(SCH))
+	 AZX_DCAPS_SNOOP_TYPE(SCH))
 
 /* Broadwell HDMI can't use position buffer reliably, force to use LPIB */
 #define AZX_DCAPS_INTEL_BROADWELL \
 	(/*AZX_DCAPS_ALIGN_BUFSIZE |*/ AZX_DCAPS_POSFIX_LPIB |\
 	 AZX_DCAPS_PM_RUNTIME | AZX_DCAPS_I915_COMPONENT |\
-	 AZX_DCAPS_I915_POWERWELL | AZX_DCAPS_SNOOP_TYPE(SCH))
+	 AZX_DCAPS_SNOOP_TYPE(SCH))
 
 #define AZX_DCAPS_INTEL_BAYTRAIL \
-	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_I915_COMPONENT |\
-	 AZX_DCAPS_I915_POWERWELL)
+	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_I915_COMPONENT)
 
 #define AZX_DCAPS_INTEL_BRASWELL \
 	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\
-	 AZX_DCAPS_I915_COMPONENT | AZX_DCAPS_I915_POWERWELL)
+	 AZX_DCAPS_I915_COMPONENT)
 
 #define AZX_DCAPS_INTEL_SKYLAKE \
 	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\
-	 AZX_DCAPS_SEPARATE_STREAM_TAG | AZX_DCAPS_I915_COMPONENT |\
-	 AZX_DCAPS_I915_POWERWELL)
+	 AZX_DCAPS_SEPARATE_STREAM_TAG | AZX_DCAPS_I915_COMPONENT)
 
 #define AZX_DCAPS_INTEL_BROXTON \
 	(AZX_DCAPS_INTEL_PCH_BASE | AZX_DCAPS_PM_RUNTIME |\
-	 AZX_DCAPS_SEPARATE_STREAM_TAG | AZX_DCAPS_I915_COMPONENT |\
-	 AZX_DCAPS_I915_POWERWELL)
+	 AZX_DCAPS_SEPARATE_STREAM_TAG | AZX_DCAPS_I915_COMPONENT)
 
 /* quirks for ATI SB / AMD Hudson */
 #define AZX_DCAPS_PRESET_ATI_SB \
@@ -591,8 +588,7 @@ static void hda_intel_init_chip(struct azx *chip, bool full_reset)
 	struct pci_dev *pci = chip->pci;
 	u32 val;
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		snd_hdac_set_codec_wakeup(bus, true);
+	snd_hdac_set_codec_wakeup(bus, true);
 	if (chip->driver_type == AZX_DRIVER_SKL) {
 		pci_read_config_dword(pci, INTEL_HDA_CGCTL, &val);
 		val = val & ~INTEL_HDA_CGCTL_MISCBDCGE;
@@ -604,8 +600,8 @@ static void hda_intel_init_chip(struct azx *chip, bool full_reset)
 		val = val | INTEL_HDA_CGCTL_MISCBDCGE;
 		pci_write_config_dword(pci, INTEL_HDA_CGCTL, val);
 	}
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		snd_hdac_set_codec_wakeup(bus, false);
+
+	snd_hdac_set_codec_wakeup(bus, false);
 
 	/* reduce dma latency to avoid noise */
 	if (IS_BXT(pci))
@@ -945,14 +941,10 @@ static bool azx_is_pm_ready(struct snd_card *card)
 
 static void __azx_runtime_suspend(struct azx *chip)
 {
-	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
-
 	azx_stop_chip(chip);
 	azx_enter_link_reset(chip);
 	azx_clear_irq_pending(chip);
-	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
-	    hda->need_i915_power)
-		display_power(chip, false);
+	display_power(chip, false);
 }
 
 static void __azx_runtime_resume(struct azx *chip)
@@ -962,11 +954,9 @@ static void __azx_runtime_resume(struct azx *chip)
 	struct hda_codec *codec;
 	int status;
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		display_power(chip, true);
-		if (hda->need_i915_power)
-			snd_hdac_i915_set_bclk(bus);
-	}
+	display_power(chip, true);
+	if (hda->need_i915_power)
+		snd_hdac_i915_set_bclk(bus);
 
 	/* Read STATESTS before controller reset */
 	status = azx_readw(chip, STATESTS);
@@ -982,8 +972,7 @@ static void __azx_runtime_resume(struct azx *chip)
 	}
 
 	/* power down again for link-controlled chips */
-	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL) &&
-	    !hda->need_i915_power)
+	if (!hda->need_i915_power)
 		display_power(chip, false);
 }
 
@@ -1347,11 +1336,8 @@ static int azx_free(struct azx *chip)
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	release_firmware(chip->fw);
 #endif
+	display_power(chip, false);
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		if (hda->need_i915_power)
-			display_power(chip, false);
-	}
 	if (chip->driver_caps & AZX_DCAPS_I915_COMPONENT)
 		snd_hdac_i915_exit(bus);
 	kfree(hda);
@@ -1908,8 +1894,7 @@ static int azx_first_init(struct azx *chip)
 	/* initialize chip */
 	azx_init_pci(chip);
 
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		snd_hdac_i915_set_bclk(bus);
+	snd_hdac_i915_set_bclk(bus);
 
 	hda_intel_init_chip(chip, (probe_only[dev] & 2) == 0);
 
@@ -2217,10 +2202,13 @@ static int azx_probe_continue(struct azx *chip)
 				goto out_free;
 			} else {
 				/* don't bother any longer */
-				chip->driver_caps &=
-					~(AZX_DCAPS_I915_COMPONENT | AZX_DCAPS_I915_POWERWELL);
+				chip->driver_caps &= ~AZX_DCAPS_I915_COMPONENT;
 			}
 		}
+
+		/* HSW/BDW controllers need this power */
+		if (CONTROLLER_IN_GPU(pci))
+			hda->need_i915_power = 1;
 	}
 
 	/* Request display power well for the HDA controller or codec. For
@@ -2228,17 +2216,11 @@ static int azx_probe_continue(struct azx *chip)
 	 * this power. For other platforms, like Baytrail/Braswell, only the
 	 * display codec needs the power and it can be released after probe.
 	 */
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		/* HSW/BDW controllers need this power */
-		if (CONTROLLER_IN_GPU(pci))
-			hda->need_i915_power = 1;
-
-		err = display_power(chip, true);
-		if (err < 0) {
-			dev_err(chip->card->dev,
-				"Cannot turn on display power on i915\n");
-			goto i915_power_fail;
-		}
+	err = display_power(chip, true);
+	if (err < 0) {
+		dev_err(chip->card->dev,
+			"Cannot turn on display power on i915\n");
+		goto i915_power_fail;
 	}
 
 	err = azx_first_init(chip);
@@ -2287,8 +2269,7 @@ static int azx_probe_continue(struct azx *chip)
 		pm_runtime_put_autosuspend(&pci->dev);
 
 out_free:
-	if ((chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		&& !hda->need_i915_power)
+	if (!hda->need_i915_power)
 		display_power(chip, false);
 
 i915_power_fail:
-- 
2.19.2

[PATCH v2 4/7] ALSA: hda/intel: Properly free the display power at error path

[Takashi Iwai]

When an error occurs in azx_probe_continue(), we should release the
display power.  However, the current code ignores it and releases the
display power only for HSW/BDW cases.  Fix it.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 663e86effa1f..5399d0180434 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2269,7 +2269,7 @@ static int azx_probe_continue(struct azx *chip)
 		pm_runtime_put_autosuspend(&pci->dev);
 
 out_free:
-	if (!hda->need_i915_power)
+	if (err < 0 || !hda->need_i915_power)
 		display_power(chip, false);
 
 i915_power_fail:
-- 
2.19.2

[PATCH v2 5/7] ALSA: hda: Make snd_hdac_display_power() void function

[Takashi Iwai]

After the recent refactoring, snd_hdac_display_power() doesn't return
any error, hence it can be defined to return void.
This makes many error checks redundant and allows us to reduce them
gracefully.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Fix compile warnings wrt unused variables

 include/sound/hda_component.h |  9 ++++----
 sound/hda/hdac_component.c    |  8 ++-----
 sound/pci/hda/hda_intel.c     |  9 +-------
 sound/soc/codecs/hdac_hdmi.c  | 23 +++++---------------
 sound/soc/intel/skylake/skl.c | 40 ++++++++++-------------------------
 5 files changed, 23 insertions(+), 66 deletions(-)

diff --git a/include/sound/hda_component.h b/include/sound/hda_component.h
index 767c8d8a0230..2ec31b358950 100644
--- a/include/sound/hda_component.h
+++ b/include/sound/hda_component.h
@@ -12,8 +12,8 @@
 
 #ifdef CONFIG_SND_HDA_COMPONENT
 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
-int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx,
-			   bool enable);
+void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx,
+			    bool enable);
 int snd_hdac_sync_audio_rate(struct hdac_device *codec, hda_nid_t nid,
 			     int dev_id, int rate);
 int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id,
@@ -30,10 +30,9 @@ static inline int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
 {
 	return 0;
 }
-static inline int snd_hdac_display_power(struct hdac_bus *bus,
-					 unsigned int idx, bool enable)
+static inline void snd_hdac_display_power(struct hdac_bus *bus,
+					  unsigned int idx, bool enable)
 {
-	return 0;
 }
 static inline int snd_hdac_sync_audio_rate(struct hdac_device *codec,
 					   hda_nid_t nid, int dev_id, int rate)
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index dd766414436b..a6d37b9d6413 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -62,10 +62,8 @@ EXPORT_SYMBOL_GPL(snd_hdac_set_codec_wakeup);
  *
  * This function updates the power status, and calls the get_power() and
  * put_power() ops accordingly, toggling the codec wakeup, too.
- *
- * Returns zero for success or a negative error code.
  */
-int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
+void snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
 {
 	struct drm_audio_component *acomp = bus->audio_component;
 
@@ -77,7 +75,7 @@ int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
 		clear_bit(idx, &bus->display_power_status);
 
 	if (!acomp || !acomp->ops)
-		return 0;
+		return;
 
 	if (bus->display_power_status) {
 		if (!bus->display_power_active) {
@@ -94,8 +92,6 @@ int snd_hdac_display_power(struct hdac_bus *bus, unsigned int idx, bool enable)
 			bus->display_power_active = false;
 		}
 	}
-
-	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_hdac_display_power);
 
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 5399d0180434..e784130ea4e0 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2216,12 +2216,7 @@ static int azx_probe_continue(struct azx *chip)
 	 * this power. For other platforms, like Baytrail/Braswell, only the
 	 * display codec needs the power and it can be released after probe.
 	 */
-	err = display_power(chip, true);
-	if (err < 0) {
-		dev_err(chip->card->dev,
-			"Cannot turn on display power on i915\n");
-		goto i915_power_fail;
-	}
+	display_power(chip, true);
 
 	err = azx_first_init(chip);
 	if (err < 0)
@@ -2271,8 +2266,6 @@ static int azx_probe_continue(struct azx *chip)
 out_free:
 	if (err < 0 || !hda->need_i915_power)
 		display_power(chip, false);
-
-i915_power_fail:
 	if (err < 0)
 		hda->init_failed = 1;
 	complete_all(&hda->probe_wait);
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index c3d551d2af7f..552ed1968bc4 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -2031,13 +2031,8 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
 	 * Turned off in the runtime_suspend during the first explicit
 	 * pm_runtime_suspend call.
 	 */
-	ret = snd_hdac_display_power(hdev->bus, hdev->addr, true);
-	if (ret < 0) {
-		dev_err(&hdev->dev,
-			"Cannot turn on display power on i915 err: %d\n",
-			ret);
-		return ret;
-	}
+	snd_hdac_display_power(hdev->bus, hdev->addr, true);
+
 	ret = hdac_hdmi_parse_and_map_nid(hdev, &hdmi_dais, &num_dais);
 	if (ret < 0) {
 		dev_err(&hdev->dev,
@@ -2169,7 +2164,6 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
 	struct hdac_bus *bus = hdev->bus;
 	struct hdac_ext_link *hlink = NULL;
-	int err;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
 
@@ -2195,11 +2189,9 @@ static int hdac_hdmi_runtime_suspend(struct device *dev)
 
 	snd_hdac_ext_bus_link_put(bus, hlink);
 
-	err = snd_hdac_display_power(bus, hdev->addr, false);
-	if (err < 0)
-		dev_err(dev, "Cannot turn off display power on i915\n");
+	snd_hdac_display_power(bus, hdev->addr, false);
 
-	return err;
+	return 0;
 }
 
 static int hdac_hdmi_runtime_resume(struct device *dev)
@@ -2207,7 +2199,6 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
 	struct hdac_bus *bus = hdev->bus;
 	struct hdac_ext_link *hlink = NULL;
-	int err;
 
 	dev_dbg(dev, "Enter: %s\n", __func__);
 
@@ -2223,11 +2214,7 @@ static int hdac_hdmi_runtime_resume(struct device *dev)
 
 	snd_hdac_ext_bus_link_get(bus, hlink);
 
-	err = snd_hdac_display_power(bus, hdev->addr, true);
-	if (err < 0) {
-		dev_err(dev, "Cannot turn on display power on i915\n");
-		return err;
-	}
+	snd_hdac_display_power(bus, hdev->addr, true);
 
 	hdac_hdmi_skl_enable_all_pins(hdev);
 	hdac_hdmi_skl_enable_dp12(hdev);
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 64f8433ae921..5c224a0e1c7a 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -311,7 +311,7 @@ static int skl_suspend(struct device *dev)
 	struct pci_dev *pci = to_pci_dev(dev);
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 	struct skl *skl  = bus_to_skl(bus);
-	int ret = 0;
+	int ret;
 
 	/*
 	 * Do not suspend if streams which are marked ignore suspend are
@@ -333,14 +333,10 @@ static int skl_suspend(struct device *dev)
 		skl->skl_sst->fw_loaded = false;
 	}
 
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		ret = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
-		if (ret < 0)
-			dev_err(bus->dev,
-				"Cannot turn OFF display power on i915\n");
-	}
+	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
+		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 
-	return ret;
+	return 0;
 }
 
 static int skl_resume(struct device *dev)
@@ -352,14 +348,8 @@ static int skl_resume(struct device *dev)
 	int ret;
 
 	/* Turned OFF in HDMI codec driver after codec reconfiguration */
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		ret = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
-		if (ret < 0) {
-			dev_err(bus->dev,
-				"Cannot turn on display power on i915\n");
-			return ret;
-		}
-	}
+	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
@@ -783,11 +773,9 @@ static int skl_i915_init(struct hdac_bus *bus)
 	if (err < 0)
 		return err;
 
-	err = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
-	if (err < 0)
-		dev_err(bus->dev, "Cannot turn on display power on i915\n");
+	snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true);
 
-	return err;
+	return 0;
 }
 
 static void skl_probe_work(struct work_struct *work)
@@ -837,14 +825,8 @@ static void skl_probe_work(struct work_struct *work)
 	list_for_each_entry(hlink, &bus->hlink_list, list)
 		snd_hdac_ext_bus_link_put(bus, hlink);
 
-	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
-		err = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
-		if (err < 0) {
-			dev_err(bus->dev, "Cannot turn off display power on i915\n");
-			skl_machine_device_unregister(skl);
-			return;
-		}
-	}
+	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
+		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 
 	/* configure PM */
 	pm_runtime_put_noidle(bus->dev);
@@ -855,7 +837,7 @@ static void skl_probe_work(struct work_struct *work)
 
 out_err:
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
-		err = snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
+		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 }
 
 /*
-- 
2.19.2

[PATCH v2 6/7] ASoC: hdac_hdmi: Add missing display power-off at driver removal

[Takashi Iwai]

The display power is in unbalance at removing the driver since it
misses the snd_hdac_display_power(OFF) call.

Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: add ACK

 sound/soc/codecs/hdac_hdmi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 552ed1968bc4..6216e502cf7b 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -2059,6 +2059,8 @@ static int hdac_hdmi_dev_remove(struct hdac_device *hdev)
 	struct hdac_hdmi_port *port, *port_next;
 	int i;
 
+	snd_hdac_display_power(hdev->bus, hdev->addr, false);
+
 	list_for_each_entry_safe(pcm, pcm_next, &hdmi->pcm_list, head) {
 		pcm->cvt = NULL;
 		if (list_empty(&pcm->port_list))
-- 
2.19.2

[PATCH v2 7/7] ALSA: hda/hdmi: Always set display_power_control for Intel HSW+ codecs

[Takashi Iwai]

We've excluded the display_power_control flag for Intel HSW and BDW
codecs as the HD-audio controllers of the corresponding platforms take
care of the display power as well.  But the recent refactoring
separates the controller and the codec power accounting, so it's fine
to call the display PM even for HSW/BDW codecs.  This is less
confusing since we can avoid this well-hidden condition.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/patch_hdmi.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 30fe4dbdb0ae..15290e4706e0 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2616,11 +2616,7 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid)
 	intel_haswell_enable_all_pins(codec, true);
 	intel_haswell_fixup_enable_dp12(codec);
 
-	/* For Haswell/Broadwell, the controller is also in the power well and
-	 * can cover the codec power request, and so need not set this flag.
-	 */
-	if (!is_haswell(codec) && !is_broadwell(codec))
-		codec->display_power_control = 1;
+	codec->display_power_control = 1;
 
 	codec->patch_ops.set_power_state = haswell_set_power_state;
 	codec->depop_delay = 0;
-- 
2.19.2

Re: [PATCH v2 0/7] ALSA: HD-audio display power fixes

[Takashi Iwai]

On Tue, 11 Dec 2018 14:53:34 +0100,
Takashi Iwai wrote:
> 
> Hi,
> 
> this patch a revised patchset for fixing possible HD-audio display
> power unbalance.
> Basically the "fix" is done by refactoring the whole relevant code.
> It starts from the Intel HD-audio runtime PM refactoring, followed
> by the display PM API change, and lots of code cleanups.
> 
> Since it changes the display power API function, it hits both legacy
> and ASoC drivers.
> 
> The patches are kept in topic/hda-pm-refactor branch of my sound git
> tree.  This will be an immutable branch once when merged to for-next,
> so that it can be merged to ASoC tree if any conflicting change needs
> to be applied.

Now I merged the patchset to for-next branch, and I'll keep the
topic/hda-pm-refactor immutable.

Mark, if you happen to apply Intel ASoC patches that conflict with the
recent changes, feel free to merge that branch into yours.


thanks,

Takashi

Re: [PATCH v2 0/7] ALSA: HD-audio display power fixes

[Pierre-Louis Bossart]

On 12/13/18 2:17 AM, Takashi Iwai wrote:
> On Tue, 11 Dec 2018 14:53:34 +0100,
> Takashi Iwai wrote:
>> Hi,
>>
>> this patch a revised patchset for fixing possible HD-audio display
>> power unbalance.
>> Basically the "fix" is done by refactoring the whole relevant code.
>> It starts from the Intel HD-audio runtime PM refactoring, followed
>> by the display PM API change, and lots of code cleanups.
>>
>> Since it changes the display power API function, it hits both legacy
>> and ASoC drivers.
>>
>> The patches are kept in topic/hda-pm-refactor branch of my sound git
>> tree.  This will be an immutable branch once when merged to for-next,
>> so that it can be merged to ASoC tree if any conflicting change needs
>> to be applied.
> Now I merged the patchset to for-next branch, and I'll keep the
> topic/hda-pm-refactor immutable.
>
> Mark, if you happen to apply Intel ASoC patches that conflict with the
> recent changes, feel free to merge that branch into yours.
>
The next batch of SOF patches will depend on these changes, so having 
Takashi's topic/hda-pm-refactor merged into Mark's for-next branch would 
help. Thanks!