[alsa-devel] [PATCH 0/7] ALSA: HD-audio display power fixes
Hi,
this patch is about 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.
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 | 9 +- 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_intel.c | 192 +++++++++++++-------------------- sound/pci/hda/patch_hdmi.c | 8 +- sound/soc/codecs/hdac_hdmi.c | 22 ++-- sound/soc/intel/skylake/skl.c | 40 ++----- 11 files changed, 134 insertions(+), 228 deletions(-)
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 */
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 --- include/sound/hda_codec.h | 1 + include/sound/hda_component.h | 8 ++++++-- 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 | 24 ++++++++--------------- sound/pci/hda/patch_hdmi.c | 4 ++-- sound/soc/codecs/hdac_hdmi.c | 7 +++---- sound/soc/intel/skylake/skl.c | 10 +++++----- 11 files changed, 61 insertions(+), 79 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..b78add315b1f 100644 --- a/include/sound/hda_component.h +++ b/include/sound/hda_component.h @@ -6,9 +6,12 @@
#include <drm/drm_audio_component.h>
+#define HDA_CODEC_IDX_CONTROLLER 16 /* virtual idx for controller */ + #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 +28,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..151c6ca85ec6 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 @@ -950,14 +945,12 @@ 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) - snd_hdac_display_power(azx_bus(chip), false); + display_power(chip, false); }
static void __azx_runtime_resume(struct azx *chip) @@ -968,7 +961,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 +982,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 +1348,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 +2049,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 +2231,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 +2287,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); }
/*
On 12/9/18 3:33 AM, Takashi Iwai wrote:
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.
The need for this virtual index==16 isn't fully clear to me, especially if you use the bitfields instead of reference counts.
Isn't there a risk of the controller setting the bit16 to zero, but you still have bit4 on (assuming the idx is 4). If you use this virtual index, it should override the actual physical bits when set/cleared.
Or is this meant to actually implement a preemption mechanism, where the display power remains on for as long as the controller wishes, regardless of what the patch_hdmi and hdac_hdmi code requests?
Also don't we already have the HDMI codec address already after the probe, so couldn't we provide the address directly?
On Mon, 10 Dec 2018 21:52:05 +0100, Pierre-Louis Bossart wrote:
On 12/9/18 3:33 AM, Takashi Iwai wrote:
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.
The need for this virtual index==16 isn't fully clear to me, especially if you use the bitfields instead of reference counts.
Isn't there a risk of the controller setting the bit16 to zero, but you still have bit4 on (assuming the idx is 4). If you use this virtual index, it should override the actual physical bits when set/cleared.
This is the index for a controller, i.e. we'd need bits for the max number of codecs + 1.
Actually we do support only up to 8 codecs (HDA_MAX_CODECS), so it should be 8, instead of 16, too. I'll update to be HDA_MAX_CODECS.
Or is this meant to actually implement a preemption mechanism, where the display power remains on for as long as the controller wishes, regardless of what the patch_hdmi and hdac_hdmi code requests?
Right. That's the mechanism at the initial phase, we need the display power on while probing the codec, i.e. before identifying the codec ID.
Also don't we already have the HDMI codec address already after the probe, so couldn't we provide the address directly?
The resume seemed requiring the controller to take the display power at first, so the same mechanism is used.
thanks,
Takashi
On 12/11/18 12:54 AM, Takashi Iwai wrote:
On Mon, 10 Dec 2018 21:52:05 +0100, Pierre-Louis Bossart wrote:
On 12/9/18 3:33 AM, Takashi Iwai wrote:
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.
The need for this virtual index==16 isn't fully clear to me, especially if you use the bitfields instead of reference counts.
Isn't there a risk of the controller setting the bit16 to zero, but you still have bit4 on (assuming the idx is 4). If you use this virtual index, it should override the actual physical bits when set/cleared.
This is the index for a controller, i.e. we'd need bits for the max number of codecs + 1.
Actually we do support only up to 8 codecs (HDA_MAX_CODECS), so it should be 8, instead of 16, too. I'll update to be HDA_MAX_CODECS.
Or is this meant to actually implement a preemption mechanism, where the display power remains on for as long as the controller wishes, regardless of what the patch_hdmi and hdac_hdmi code requests?
Right. That's the mechanism at the initial phase, we need the display power on while probing the codec, i.e. before identifying the codec ID.
Also don't we already have the HDMI codec address already after the probe, so couldn't we provide the address directly?
The resume seemed requiring the controller to take the display power at first, so the same mechanism is used.
ok, makes sense, thanks for the explanations.
So I guess for the SOF patches, the only change would be to add the second argument HDA_CODEC_IDX_CONTROLLER to snd_hdac_display_power() calls, the rest looks unchanged or hidden inside the hdac library or hdac_hdmi parts.
On Tue, 11 Dec 2018 14:58:37 +0100, Pierre-Louis Bossart wrote:
On 12/11/18 12:54 AM, Takashi Iwai wrote:
On Mon, 10 Dec 2018 21:52:05 +0100, Pierre-Louis Bossart wrote:
On 12/9/18 3:33 AM, Takashi Iwai wrote:
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.
The need for this virtual index==16 isn't fully clear to me, especially if you use the bitfields instead of reference counts.
Isn't there a risk of the controller setting the bit16 to zero, but you still have bit4 on (assuming the idx is 4). If you use this virtual index, it should override the actual physical bits when set/cleared.
This is the index for a controller, i.e. we'd need bits for the max number of codecs + 1.
Actually we do support only up to 8 codecs (HDA_MAX_CODECS), so it should be 8, instead of 16, too. I'll update to be HDA_MAX_CODECS.
Or is this meant to actually implement a preemption mechanism, where the display power remains on for as long as the controller wishes, regardless of what the patch_hdmi and hdac_hdmi code requests?
Right. That's the mechanism at the initial phase, we need the display power on while probing the codec, i.e. before identifying the codec ID.
Also don't we already have the HDMI codec address already after the probe, so couldn't we provide the address directly?
The resume seemed requiring the controller to take the display power at first, so the same mechanism is used.
ok, makes sense, thanks for the explanations.
So I guess for the SOF patches, the only change would be to add the second argument HDA_CODEC_IDX_CONTROLLER to snd_hdac_display_power() calls, the rest looks unchanged or hidden inside the hdac library or hdac_hdmi parts.
Yes, other than that, this change makes things easier.
Since we don't manage with refcount, the only important point is to turn off/on properly at suspend/resume (also off at remove), no matter how many times it gets called.
thanks,
Takashi
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.
The need for this virtual index==16 isn't fully clear to me, especially if you use the bitfields instead of reference counts.
Isn't there a risk of the controller setting the bit16 to zero, but you still have bit4 on (assuming the idx is 4). If you use this virtual index, it should override the actual physical bits when set/cleared.
This is the index for a controller, i.e. we'd need bits for the max number of codecs + 1.
Actually we do support only up to 8 codecs (HDA_MAX_CODECS), so it should be 8, instead of 16, too. I'll update to be HDA_MAX_CODECS.
Or is this meant to actually implement a preemption mechanism, where the display power remains on for as long as the controller wishes, regardless of what the patch_hdmi and hdac_hdmi code requests?
Right. That's the mechanism at the initial phase, we need the display power on while probing the codec, i.e. before identifying the codec ID.
Also don't we already have the HDMI codec address already after the probe, so couldn't we provide the address directly?
The resume seemed requiring the controller to take the display power at first, so the same mechanism is used.
ok, makes sense, thanks for the explanations.
So I guess for the SOF patches, the only change would be to add the second argument HDA_CODEC_IDX_CONTROLLER to snd_hdac_display_power() calls, the rest looks unchanged or hidden inside the hdac library or hdac_hdmi parts.
Yes, other than that, this change makes things easier.
Since we don't manage with refcount, the only important point is to turn off/on properly at suspend/resume (also off at remove), no matter how many times it gets called.
Ah yes, we are missing this on remove since we assumed the refcount would already be zero. I guess we'll have to revalidate this part anyways once your patches are merged (already have an SOF issue filed to track this change).
Since snd_hdac_display_power() can be called even for a HDA controller without DRM binding, lots of superfluous AZX_DCAPS_I915_POWERWELL checks in hda_intel.c can be dropped. This simplifies the code a lot.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_intel.c | 43 +++++++++++++++------------------------ 1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 151c6ca85ec6..cacee33a74a8 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -948,9 +948,7 @@ static void __azx_runtime_suspend(struct azx *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) @@ -960,11 +958,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); @@ -980,8 +976,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); }
@@ -1345,11 +1340,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); @@ -2219,6 +2211,10 @@ static int azx_probe_continue(struct azx *chip) ~(AZX_DCAPS_I915_COMPONENT | AZX_DCAPS_I915_POWERWELL); } } + + /* 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 @@ -2226,17 +2222,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); @@ -2285,8 +2275,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:
On 12/9/18 3:33 AM, Takashi Iwai wrote:
Since snd_hdac_display_power() can be called even for a HDA controller without DRM binding, lots of superfluous AZX_DCAPS_I915_POWERWELL checks in hda_intel.c can be dropped. This simplifies the code a lot.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_intel.c | 43 +++++++++++++++------------------------ 1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 151c6ca85ec6..cacee33a74a8 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -948,9 +948,7 @@ static void __azx_runtime_suspend(struct azx *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)
@@ -960,11 +958,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);
Question: I still see this 'old style' init in hda_intel.c even with all the patches applied.
/* initialize chip */ azx_init_pci(chip);
if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) snd_hdac_i915_set_bclk(bus);
is this intentional or a miss?
On Mon, 10 Dec 2018 21:56:15 +0100, Pierre-Louis Bossart wrote:
On 12/9/18 3:33 AM, Takashi Iwai wrote:
Since snd_hdac_display_power() can be called even for a HDA controller without DRM binding, lots of superfluous AZX_DCAPS_I915_POWERWELL checks in hda_intel.c can be dropped. This simplifies the code a lot.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_intel.c | 43 +++++++++++++++------------------------ 1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 151c6ca85ec6..cacee33a74a8 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -948,9 +948,7 @@ static void __azx_runtime_suspend(struct azx *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)
@@ -960,11 +958,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);Question: I still see this 'old style' init in hda_intel.c even with all the patches applied.
/* initialize chip */ azx_init_pci(chip);
if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) snd_hdac_i915_set_bclk(bus);
is this intentional or a miss?
It's intentional. The purpose of the patch isn't to eliminate the whole DCAPS_I915_POWERWELL checks, but remove the checks that are relevant with snd_hdac_display_power() calls. In the changes, some calls are reduced with only hda->need_i915 check, which is safe since need_i915 mandates AZX_DCAPS_I915_POWERWELL.
Though, actually, snd_hdac_i915_set_bclk() can be called safely for the case without GPU binding, too. So it's fine to get rid of the AZX_DCAPS check there, too.
thanks,
Takashi
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 cacee33a74a8..48b29f02f72d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2275,7 +2275,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:
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 --- 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 | 21 +++++------------- sound/soc/intel/skylake/skl.c | 40 ++++++++++------------------------- 5 files changed, 23 insertions(+), 64 deletions(-)
diff --git a/include/sound/hda_component.h b/include/sound/hda_component.h index b78add315b1f..1916f4928bf3 100644 --- a/include/sound/hda_component.h +++ b/include/sound/hda_component.h @@ -10,8 +10,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, @@ -28,10 +28,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 48b29f02f72d..e89b8933b464 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2222,12 +2222,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) @@ -2277,8 +2272,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..adce94a3b289 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, @@ -2195,11 +2190,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) @@ -2223,11 +2216,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); }
/*
The display power is in unbalance at removing the driver since it misses the snd_hdac_display_power(OFF) call.
Signed-off-by: Takashi Iwai tiwai@suse.de --- 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 adce94a3b289..6549adcf857f 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))
On Sun, Dec 09, 2018 at 10:33:17AM +0100, Takashi Iwai wrote:
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
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;
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai