| Message ID | 20191001163555.24356-1-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Fix audio power up sequence for gen10/11 | expand |
Quoting Kai Vehmanen (2019-10-01 17:35:54) > On gen10/11 platforms, driver must set the enable bit of AUD_PIN_BUF_CTL > as part of audio power up sequence. > > Failing to do this resulted in errors during display audio codec probe, > and failures during resume from suspend. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111214 > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 5 +++++ > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index 54638d99e021..a731af7ada08 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -862,6 +862,11 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) > /* Force CDCLK to 2*BCLK as long as we need audio powered. */ > if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > glk_force_audio_cdclk(dev_priv, true); > + > + if (INTEL_GEN(dev_priv) == 11 || INTEL_GEN(dev_priv) == 10) > + I915_WRITE(AUD_PIN_BUF_CTL, > + (I915_READ(AUD_PIN_BUF_CTL) | > + AUD_PIN_BUF_ENABLE)); From the observation that this reduces the module reload time from 196s to 2s, I'd say this works. Do you need to disable the bit later? -Chris
Hey, On Wed, 2 Oct 2019, Chris Wilson wrote: > > + if (INTEL_GEN(dev_priv) == 11 || INTEL_GEN(dev_priv) == 10) > > + I915_WRITE(AUD_PIN_BUF_CTL, > > + (I915_READ(AUD_PIN_BUF_CTL) | > > + AUD_PIN_BUF_ENABLE)); > > From the observation that this reduces the module reload time from 196s > to 2s, I'd say this works. yes indeed, the CI results look very promising. The problem does not occur every time, so this has been bugging us for a long time, but in scenarios and systems where this happens, the patch seems to help. Upon further investigation, I believe this needs to be applied on GLK as well. This would explain some of the other sightings. E.g. here's one from from audio/SOF CI from this week: https://sof-ci.01.org/sofpr/PR1864/build3366/devicetest/GLK_BOB_DA7219/verify-pcm-list.sh/dmesg.txt I'll update both patches to include GLK. > Do you need to disable the bit later? Now that is trickier. I'm still digging more info about this. Based on info I have, to avoid unsolicited wakes from i915 audio codec, we should clear the bit after we have turned off i915 audio power domain. In practise onsystems I have for testing, this results in frequent probe failures, so I left this out from the patch. I will try to search for more info on this (especially if we get still some failures with the patch). But enabling it every time does seem to help and so far has not caused any regressions elsewhere, so at least this part seems good. Br, Kai
On Tue, Oct 01, 2019 at 07:35:54PM +0300, Kai Vehmanen wrote: > On gen10/11 platforms, driver must set the enable bit of AUD_PIN_BUF_CTL > as part of audio power up sequence. > > Failing to do this resulted in errors during display audio codec probe, > and failures during resume from suspend. Good catch, seems to match bspec 21352 (and 49280 for GEN12+). Before setting this bit the sequence has an other step done in the HDA driver (LCTL reg write in sound/pci/hda/hda_intel.c, intel_init_lctl()) before setting this bit. If that order is important we'd need another hook in drm_audio_component_ops (and also clear the bit). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111214 > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 5 +++++ > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index 54638d99e021..a731af7ada08 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -862,6 +862,11 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) > /* Force CDCLK to 2*BCLK as long as we need audio powered. */ > if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > glk_force_audio_cdclk(dev_priv, true); > + > + if (INTEL_GEN(dev_priv) == 11 || INTEL_GEN(dev_priv) == 10) > + I915_WRITE(AUD_PIN_BUF_CTL, > + (I915_READ(AUD_PIN_BUF_CTL) | > + AUD_PIN_BUF_ENABLE)); > } > > return ret; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 058aa5ca8b73..daff9058f0e8 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9133,6 +9133,8 @@ enum { > #define SKL_AUD_CODEC_WAKE_SIGNAL (1 << 15) > > #define AUD_FREQ_CNTRL _MMIO(0x65900) > +#define AUD_PIN_BUF_CTL _MMIO(0x48414) > +#define AUD_PIN_BUF_ENABLE BIT(31) > > /* > * HSW - ICL power wells > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 01 Oct 2019, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote: > On gen10/11 platforms, driver must set the enable bit of AUD_PIN_BUF_CTL > as part of audio power up sequence. > > Failing to do this resulted in errors during display audio codec probe, > and failures during resume from suspend. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111214 > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 5 +++++ > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index 54638d99e021..a731af7ada08 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -862,6 +862,11 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) > /* Force CDCLK to 2*BCLK as long as we need audio powered. */ > if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) > glk_force_audio_cdclk(dev_priv, true); > + > + if (INTEL_GEN(dev_priv) == 11 || INTEL_GEN(dev_priv) == 10) > + I915_WRITE(AUD_PIN_BUF_CTL, > + (I915_READ(AUD_PIN_BUF_CTL) | > + AUD_PIN_BUF_ENABLE)); > } > > return ret; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 058aa5ca8b73..daff9058f0e8 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9133,6 +9133,8 @@ enum { > #define SKL_AUD_CODEC_WAKE_SIGNAL (1 << 15) > > #define AUD_FREQ_CNTRL _MMIO(0x65900) > +#define AUD_PIN_BUF_CTL _MMIO(0x48414) > +#define AUD_PIN_BUF_ENABLE BIT(31) Drive-by comment, please use REG_BIT() in i915_reg.h. BIT becomes UL which in turn can cause trouble later on. BR, Jani. > > /* > * HSW - ICL power wells
Hey, On Wed, 2 Oct 2019, Imre Deak wrote: > On Tue, Oct 01, 2019 at 07:35:54PM +0300, Kai Vehmanen wrote: > > On gen10/11 platforms, driver must set the enable bit of AUD_PIN_BUF_CTL > > as part of audio power up sequence. > > Good catch, seems to match bspec 21352 (and 49280 for GEN12+). > > Before setting this bit the sequence has an other step done in the HDA > driver (LCTL reg write in sound/pci/hda/hda_intel.c, intel_init_lctl()) > before setting this bit. If that order is important we'd need another > hook in drm_audio_component_ops (and also clear the bit). that is true. The full sequences to avoid unsolicited responses are quite awkward as there are multiple (new) hops between display and hda drivers. I don't think we strictly need these on Linux, but it's definitely a problem if we don't ensure AUD_PIN_BUF_CTL is set. I have one failing case left on ICL where v1 patchset does not seem sufficient. The test case involves a loop of doing S3 suspend, resume, unload driver, load driver, play audio via HDMI and repeat. I get systematically better results with this patch, but it still fails before 100 iterations. It's a definite improvement, so probably this patch should go in in any case. I have a wip patch to HDA driver side that seems to cure the remaining issue as well. The problem seems slightly different -- we miss an IRQ but the display-pch transactions are functional, so this can be a different problem. I'll continue testing a bit and once confident enough, send out the v2 patch. Br, Kai
Hi, On Wed, 2 Oct 2019, Kai Vehmanen wrote: > I have one failing case left on ICL where v1 patchset does not seem > sufficient. The test case involves a loop of doing S3 suspend, resume, > unload driver, load driver, play audio via HDMI and repeat. I get > systematically better results with this patch, but it still fails before > 100 iterations. It's a definite improvement, so probably this patch > should go in in any case. ok, this turned out to be an issue on HDA driver side and only happens with SOF driver (not with the snd_hda_intel non-DSP HDA codec driver). So the v2 version of the patch is ready to go. Br, Kai
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index 54638d99e021..a731af7ada08 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c @@ -862,6 +862,11 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) /* Force CDCLK to 2*BCLK as long as we need audio powered. */ if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) glk_force_audio_cdclk(dev_priv, true); + + if (INTEL_GEN(dev_priv) == 11 || INTEL_GEN(dev_priv) == 10) + I915_WRITE(AUD_PIN_BUF_CTL, + (I915_READ(AUD_PIN_BUF_CTL) | + AUD_PIN_BUF_ENABLE)); } return ret; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 058aa5ca8b73..daff9058f0e8 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9133,6 +9133,8 @@ enum { #define SKL_AUD_CODEC_WAKE_SIGNAL (1 << 15) #define AUD_FREQ_CNTRL _MMIO(0x65900) +#define AUD_PIN_BUF_CTL _MMIO(0x48414) +#define AUD_PIN_BUF_ENABLE BIT(31) /* * HSW - ICL power wells
On gen10/11 platforms, driver must set the enable bit of AUD_PIN_BUF_CTL as part of audio power up sequence. Failing to do this resulted in errors during display audio codec probe, and failures during resume from suspend. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111214 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_audio.c | 5 +++++ drivers/gpu/drm/i915/i915_reg.h | 2 ++ 2 files changed, 7 insertions(+)