| Message ID | s5hy44cqmad.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@suse.de> > 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@suse.de> > --- > 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; > } >
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; }