| Message ID | 20190920083918.27057-1-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: save AUD_FREQ_CNTRL state at audio domain suspend | expand |
On Fri, 20 Sep 2019, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote: > When audio power domain is suspended, the display driver must > save state of AUD_FREQ_CNTRL on Tiger Lake and Ice Lake > systems. The initial value of the register is set by BIOS and > is read by driver during the audio component init sequence. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Thanks for the patch, let's wait for the CI results. Reviewed-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_audio.c | 17 +++++++++++++++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index aac089c79ceb..54638d99e021 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -852,10 +852,17 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) > > ret = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); > > - /* Force CDCLK to 2*BCLK as long as we need audio to be powered. */ > - if (dev_priv->audio_power_refcount++ == 0) > + if (dev_priv->audio_power_refcount++ == 0) { > + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { > + I915_WRITE(AUD_FREQ_CNTRL, dev_priv->audio_freq_cntrl); > + DRM_DEBUG_KMS("restored AUD_FREQ_CNTRL to 0x%x\n", > + dev_priv->audio_freq_cntrl); > + } > + > + /* 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); > + } > > return ret; > } > @@ -1116,6 +1123,12 @@ static void i915_audio_component_init(struct drm_i915_private *dev_priv) > return; > } > > + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { > + dev_priv->audio_freq_cntrl = I915_READ(AUD_FREQ_CNTRL); > + DRM_DEBUG_KMS("init value of AUD_FREQ_CNTRL of 0x%x\n", > + dev_priv->audio_freq_cntrl); > + } > + > dev_priv->audio_component_registered = true; > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4faec2f94e19..5bb19f1c0ef4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1540,6 +1540,7 @@ struct drm_i915_private { > */ > struct mutex av_mutex; > int audio_power_refcount; > + u32 audio_freq_cntrl; > > struct { > struct mutex mutex; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index bf37ecebc82f..dff077aa4cc6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9119,6 +9119,8 @@ enum { > #define HSW_AUD_CHICKENBIT _MMIO(0x65f10) > #define SKL_AUD_CODEC_WAKE_SIGNAL (1 << 15) > > +#define AUD_FREQ_CNTRL _MMIO(0x65900) > + > /* > * HSW - ICL power wells > *
On Fri, 20 Sep 2019, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote: > When audio power domain is suspended, the display driver must > save state of AUD_FREQ_CNTRL on Tiger Lake and Ice Lake > systems. The initial value of the register is set by BIOS and > is read by driver during the audio component init sequence. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Pushed to drm-intel-next-queued (and thus drm-tip), thanks for the patch! BR, Jani. > --- > drivers/gpu/drm/i915/display/intel_audio.c | 17 +++++++++++++++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index aac089c79ceb..54638d99e021 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -852,10 +852,17 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) > > ret = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); > > - /* Force CDCLK to 2*BCLK as long as we need audio to be powered. */ > - if (dev_priv->audio_power_refcount++ == 0) > + if (dev_priv->audio_power_refcount++ == 0) { > + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { > + I915_WRITE(AUD_FREQ_CNTRL, dev_priv->audio_freq_cntrl); > + DRM_DEBUG_KMS("restored AUD_FREQ_CNTRL to 0x%x\n", > + dev_priv->audio_freq_cntrl); > + } > + > + /* 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); > + } > > return ret; > } > @@ -1116,6 +1123,12 @@ static void i915_audio_component_init(struct drm_i915_private *dev_priv) > return; > } > > + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { > + dev_priv->audio_freq_cntrl = I915_READ(AUD_FREQ_CNTRL); > + DRM_DEBUG_KMS("init value of AUD_FREQ_CNTRL of 0x%x\n", > + dev_priv->audio_freq_cntrl); > + } > + > dev_priv->audio_component_registered = true; > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4faec2f94e19..5bb19f1c0ef4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1540,6 +1540,7 @@ struct drm_i915_private { > */ > struct mutex av_mutex; > int audio_power_refcount; > + u32 audio_freq_cntrl; > > struct { > struct mutex mutex; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index bf37ecebc82f..dff077aa4cc6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9119,6 +9119,8 @@ enum { > #define HSW_AUD_CHICKENBIT _MMIO(0x65f10) > #define SKL_AUD_CODEC_WAKE_SIGNAL (1 << 15) > > +#define AUD_FREQ_CNTRL _MMIO(0x65900) > + > /* > * HSW - ICL power wells > *
On Fri, Sep 20, 2019 at 11:39:18AM +0300, Kai Vehmanen wrote: > When audio power domain is suspended, the display driver must > save state of AUD_FREQ_CNTRL on Tiger Lake and Ice Lake > systems. The initial value of the register is set by BIOS and > is read by driver during the audio component init sequence. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> Hi Kai. I realize this patch landed several months ago, but I was just glancing through places in the driver where we call out specific platforms with IS_FOO() rather than using generation tests and noticed this one. Should this programming be specific to just ICL and TGL, or should it also apply to other recent platforms like EHL/JSL? Our convention in i915 is to usually just assume that future platforms will follow the lead of the current latest platform until we find out otherwise. So we may want to add another patch to change the test to use either an if (INTEL_GEN(dev_priv) >= 11) or a if (INTEL_GEN(dev_priv) >= 12 || IS_ICELAKE(dev_priv)) depending on whether EHL/JSL also need this. Thanks! Matt > --- > drivers/gpu/drm/i915/display/intel_audio.c | 17 +++++++++++++++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c > index aac089c79ceb..54638d99e021 100644 > --- a/drivers/gpu/drm/i915/display/intel_audio.c > +++ b/drivers/gpu/drm/i915/display/intel_audio.c > @@ -852,10 +852,17 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) > > ret = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); > > - /* Force CDCLK to 2*BCLK as long as we need audio to be powered. */ > - if (dev_priv->audio_power_refcount++ == 0) > + if (dev_priv->audio_power_refcount++ == 0) { > + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { > + I915_WRITE(AUD_FREQ_CNTRL, dev_priv->audio_freq_cntrl); > + DRM_DEBUG_KMS("restored AUD_FREQ_CNTRL to 0x%x\n", > + dev_priv->audio_freq_cntrl); > + } > + > + /* 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); > + } > > return ret; > } > @@ -1116,6 +1123,12 @@ static void i915_audio_component_init(struct drm_i915_private *dev_priv) > return; > } > > + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { > + dev_priv->audio_freq_cntrl = I915_READ(AUD_FREQ_CNTRL); > + DRM_DEBUG_KMS("init value of AUD_FREQ_CNTRL of 0x%x\n", > + dev_priv->audio_freq_cntrl); > + } > + > dev_priv->audio_component_registered = true; > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4faec2f94e19..5bb19f1c0ef4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1540,6 +1540,7 @@ struct drm_i915_private { > */ > struct mutex av_mutex; > int audio_power_refcount; > + u32 audio_freq_cntrl; > > struct { > struct mutex mutex; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index bf37ecebc82f..dff077aa4cc6 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -9119,6 +9119,8 @@ enum { > #define HSW_AUD_CHICKENBIT _MMIO(0x65f10) > #define SKL_AUD_CODEC_WAKE_SIGNAL (1 << 15) > > +#define AUD_FREQ_CNTRL _MMIO(0x65900) > + > /* > * HSW - ICL power wells > * > -- > 2.18.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hey Matt, On Tue, 24 Dec 2019, Matt Roper wrote: > > When audio power domain is suspended, the display driver must > > save state of AUD_FREQ_CNTRL on Tiger Lake and Ice Lake > > systems. The initial value of the register is set by BIOS and > > I realize this patch landed several months ago, but I was just glancing > through places in the driver where we call out specific platforms with > IS_FOO() rather than using generation tests and noticed this one. > Should this programming be specific to just ICL and TGL, or should it > also apply to other recent platforms like EHL/JSL? > > Our convention in i915 is to usually just assume that future platforms > will follow the lead of the current latest platform until we find out > otherwise. So we may want to add another patch to change the test to this is a valid point. The current check is very limited as the issue has been only observed on these platforms (but was very severe on these devices so a quick response was needed). I did observe originally that specs would indicate the register should be saved on many other platforms as well. I was hesitant to add this to the patch, as there are many platforms that have been shipping for years with no issues reported on this. But, but, I think we should just proceed and extend the check to all documented platforms, past and future (basicly INTEL_GEN>=9). I can make a patch and let's see how it fares in CI. Br, Kai
diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index aac089c79ceb..54638d99e021 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c @@ -852,10 +852,17 @@ static unsigned long i915_audio_component_get_power(struct device *kdev) ret = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); - /* Force CDCLK to 2*BCLK as long as we need audio to be powered. */ - if (dev_priv->audio_power_refcount++ == 0) + if (dev_priv->audio_power_refcount++ == 0) { + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { + I915_WRITE(AUD_FREQ_CNTRL, dev_priv->audio_freq_cntrl); + DRM_DEBUG_KMS("restored AUD_FREQ_CNTRL to 0x%x\n", + dev_priv->audio_freq_cntrl); + } + + /* 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); + } return ret; } @@ -1116,6 +1123,12 @@ static void i915_audio_component_init(struct drm_i915_private *dev_priv) return; } + if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) { + dev_priv->audio_freq_cntrl = I915_READ(AUD_FREQ_CNTRL); + DRM_DEBUG_KMS("init value of AUD_FREQ_CNTRL of 0x%x\n", + dev_priv->audio_freq_cntrl); + } + dev_priv->audio_component_registered = true; } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 4faec2f94e19..5bb19f1c0ef4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1540,6 +1540,7 @@ struct drm_i915_private { */ struct mutex av_mutex; int audio_power_refcount; + u32 audio_freq_cntrl; struct { struct mutex mutex; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index bf37ecebc82f..dff077aa4cc6 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9119,6 +9119,8 @@ enum { #define HSW_AUD_CHICKENBIT _MMIO(0x65f10) #define SKL_AUD_CODEC_WAKE_SIGNAL (1 << 15) +#define AUD_FREQ_CNTRL _MMIO(0x65900) + /* * HSW - ICL power wells *
When audio power domain is suspended, the display driver must save state of AUD_FREQ_CNTRL on Tiger Lake and Ice Lake systems. The initial value of the register is set by BIOS and is read by driver during the audio component init sequence. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_audio.c | 17 +++++++++++++++-- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_reg.h | 2 ++ 3 files changed, 18 insertions(+), 2 deletions(-)