Hi,
On 04-08-16 23:03, Takashi Iwai wrote:
[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.
I've build a 4.7 kernel with this patch and I can confirm that it
fixes the WARN_ON I was seeing in my XPS 9550 without this patch.
Thanks & Regards,
Hans
thanks,
Takashi
-- 8< --
From: Takashi Iwai <tiwai@xxxxxxx>
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 <tiwai@xxxxxxx>
---
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;
}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
