[dropped stable ML and add a few other relevant people to Cc] On Thu, 04 Aug 2016 20:05:27 +0200, Takashi Iwai wrote: > > On Thu, 04 Aug 2016 18:44:11 +0200, > Daniel Vetter wrote: > > > > On Wed, Aug 03, 2016 at 05:09:00PM +0100, Chris Wilson wrote: > > > On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display > > > power well and so the sna-hda audio codec acquires the display power > > > well while it is operational. However, Skylake separates the powerwells > > > again, but yet we still need the audio powerwell to setup the registers. > > > (But then the hardware uses those registers even while powered off???) > > > > Yeah feels fishy, but will at least duct-tape over the breakage from the > > audio side. Most likely the reg writes go exactly nowhere and there's a > > bug on the audio side. And this patch doesn't fix that. > > Well, if the relevant code paths are only over these callbacks, I > guess the following fix would work instead. > > Can anyone check?
Scratch the previous one. There is another place calling similarly. Now looking back at the code again, I believe that the better way would be to properly do power up / down at the resume callbacks. Below is an untested patch. Let me know if this actually works. thanks, Takashi -- 8< -- From: Takashi Iwai <[email protected]> Subject: [PATCH] ALSA: hda - Manage power well properly for resume For SKL and later Intel chips, we control the power well per codec basis via link_power callback since the commit [03b135cebc47: ALSA: hda - remove dependency on i915 power well for SKL]. However, there are a few exceptional cases where the gfx registers are accessed from the audio driver: namely the wakeup override bit toggling at (both system and runtime) resume. This seems causing a kernel warning when accessed during the power well down (and likely resulting in the bogus register accesses). This patch puts the proper power up / down sequence around the resume code so that the wakeup bit is fiddled properly while the power is up. (The other callback, sync_audio_rate, is used only in the PCM callback, so it's guaranteed in the power-on.) Also, by this proper power up/down, the instantaneous flip of wakeup bit in the resume callback that was introduced by the commit [033ea349a7cd: ALSA: hda - Fix Skylake codec timeout] becomes superfluous, as snd_hdac_display_power() already does it. So we can clean it up together. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214 Fixes: 03b135cebc47 ('ALSA: hda - remove dependency on i915 power well for SKL') Signed-off-by: Takashi Iwai <[email protected]> --- sound/pci/hda/hda_intel.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 89dacf9b4e6c..160c7f713722 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -906,20 +906,23 @@ static int azx_resume(struct device *dev) struct snd_card *card = dev_get_drvdata(dev); struct azx *chip; struct hda_intel *hda; + struct hdac_bus *bus; if (!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 - && hda->need_i915_power) { - snd_hdac_display_power(azx_bus(chip), true); - snd_hdac_i915_set_bclk(azx_bus(chip)); + 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) chip->msi = 0; @@ -929,6 +932,11 @@ static int azx_resume(struct device *dev) 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); + snd_power_change_state(card, SNDRV_CTL_POWER_D0); trace_azx_resume(chip); @@ -1008,6 +1016,7 @@ static int azx_runtime_resume(struct device *dev) chip = card->private_data; hda = container_of(chip, struct hda_intel, chip); + bus = azx_bus(chip); if (chip->disabled || hda->init_failed) return 0; @@ -1015,15 +1024,9 @@ static int azx_runtime_resume(struct device *dev) return 0; if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) { - bus = azx_bus(chip); - if (hda->need_i915_power) { - snd_hdac_display_power(bus, true); + snd_hdac_display_power(bus, true); + if (hda->need_i915_power) snd_hdac_i915_set_bclk(bus); - } else { - /* toggle codec wakeup bit for STATESTS read */ - snd_hdac_set_codec_wakeup(bus, true); - snd_hdac_set_codec_wakeup(bus, false); - } } /* Read STATESTS before controller reset */ @@ -1043,6 +1046,11 @@ static int azx_runtime_resume(struct device *dev) 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; } -- 2.9.2 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
