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

older
[alsa-devel] [PATCH] ASoC: rsnd:...

Takashi Iwai

11 Dec 2018 11 Dec '18

3:53 p.m.

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

-- 2.19.2

Show replies by date

Takashi Iwai

11 Dec 11 Dec

3:53 p.m.

New subject: [alsa-devel] [PATCH v2 1/7] ALSA: hda/intel: Refactoring PM code

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

Takashi Iwai

3:53 p.m.

New subject: [alsa-devel] [PATCH v2 2/7] ALSA: hda: Refactor display power management

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

Takashi Iwai

3:53 p.m.

New subject: [alsa-devel] [PATCH v2 3/7] ALSA: hda/intel: Drop superfluous AZX_DCAPS_I915_POWERWELL checks

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)

/* 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

Takashi Iwai

3:53 p.m.

New subject: [alsa-devel] [PATCH v2 4/7] ALSA: hda/intel: Properly free the display power at error path

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

Takashi Iwai

3:53 p.m.

New subject: [alsa-devel] [PATCH v2 5/7] ALSA: hda: Make snd_hdac_display_power() void function

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

Takashi Iwai

3:53 p.m.

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

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

Takashi Iwai

3:53 p.m.

New subject: [alsa-devel] [PATCH v2 7/7] ALSA: hda/hdmi: Always set display_power_control for Intel HSW+ codecs

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

Takashi Iwai

13 Dec 13 Dec

10:17 a.m.

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

Pierre-Louis Bossart

4:37 p.m.

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!

2164

Age (days ago)

2166

Last active (days ago)

List overview

Download

9 comments

2 participants

tags (0)

participants (2)

Pierre-Louis Bossart
Takashi Iwai