Re: [PATCH] drm/i915/glk, cnl: Implement WaDisableScalarClockGating

[Imre Deak <imre.deak@intel.com>]

On Thu, Sep 28, 2017 at 01:18:30PM -0700, Rodrigo Vivi wrote:
> On Thu, Sep 28, 2017 at 07:54:36PM +0000, Imre Deak wrote:
> > On GLK and CNL enabling a pipe with its pipe scaler enabled will result
> > in a FIFO underrun. This happens only once after driver loading or
> > system/runtime resume, more specifically after power well 1 gets
> > enabled; subsequent modesets seem to be free of underruns. The BSpec
> > workaround for this is to disable the pipe scaler clock gating for the
> > duration of modeset. Based on my tests disabling clock gating must be
> > done before enabling pipe scaling and we can re-enable it after the pipe
> > is enabled and one vblank has passed.
> 
> Oh! Great!
> I had this Wa in my list here, but I was without access to HDSES
> and was postponing it...
> 
> But now that you mention the bits it was easy to find on BSpec as well.
> 
> > 
> > For consistency I also checked if plane scaling would cause the same
> > problem, but that doesn't seem to trigger this problem.
> > 
> > The patch is based on an earlier version from Ander.
> > 
> > Cc: Ander Conselvan de Oliveira <conselvan2@gmail.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100302
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      |  8 ++++++++
> >  drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 82f36dd0cd94..40a3c045d9d0 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3811,6 +3811,14 @@ enum {
> >  #define   PWM2_GATING_DIS		(1 << 14)
> >  #define   PWM1_GATING_DIS		(1 << 13)
> >  
> > +#define _CLKGATE_DIS_PSL_A		0x46520
> > +#define _CLKGATE_DIS_PSL_B		0x46524
> > +#define _CLKGATE_DIS_PSL_C		0x46528
> > +#define   DPF_GATING_DIS		(1 << 10)
> 
> On BSpec they also tells us to disable bits 8 and 9:
> 
> "To disable Scaler clock gating, set bits 8, 9, and 10 of 0x46520 (Pipe A), 0x46524 (Pipe B), or 0x46528 (Pipe C)"

Right, thanks for catching this. The original WA was only bit 10, and it
does get rid of the problem. I haven't noticed this change, will update
the patch based on that.

> 
> > +
> > +#define CLKGATE_DIS_PSL(pipe) \
> > +	_MMIO_PIPE(pipe, _CLKGATE_DIS_PSL_A, _CLKGATE_DIS_PSL_B)
> > +
> >  /*
> >   * GEN10 clock gating regs
> >   */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 026fa5460fe5..9d0b5a5596a5 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5459,6 +5459,19 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
> >  	return HAS_IPS(to_i915(crtc->base.dev)) && crtc->pipe == PIPE_A;
> >  }
> >  
> > +static void glk_pipe_scaler_clock_gating_wa(struct drm_i915_private *dev_priv,
> > +					    enum pipe pipe, bool apply)
> > +{
> > +	u32 tmp = I915_READ(CLKGATE_DIS_PSL(pipe));
> > +
> > +	if (apply)
> > +		tmp |= DPF_GATING_DIS;
> > +	else
> > +		tmp &= ~DPF_GATING_DIS;
> > +
> > +	I915_WRITE(CLKGATE_DIS_PSL(pipe), tmp);
> > +}
> > +
> >  static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> >  				struct drm_atomic_state *old_state)
> >  {
> > @@ -5469,6 +5482,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> >  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> >  	struct intel_atomic_state *old_intel_state =
> >  		to_intel_atomic_state(old_state);
> > +	bool psl_clkgate_wa;
> >  
> >  	if (WARN_ON(intel_crtc->active))
> >  		return;
> > @@ -5522,6 +5536,12 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> >  	if (!transcoder_is_dsi(cpu_transcoder))
> >  		intel_ddi_enable_pipe_clock(pipe_config);
> >  
> > +	/* WaDisableScalarClockGating: glk, cnl */
> 
> Please also add the BSpec reference Display WA #1180.

Ok.

> > +	psl_clkgate_wa = (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) &&
> > +			 intel_crtc->config->pch_pfit.enabled;
> > +	if (psl_clkgate_wa)
> > +		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
> > +
> 
> Is this place enough? Wouldn't be better to start that on atomic commit before we set cdclock
> and mainly before we do a pre plane update?

I'd prefer keeping it here leaving the WA's range narrow, unless it's
proven that it's not enough. If CD clock would be a factor, then we'd
have to force a CD clock disable/re-enable if the CD clock rate wouldn't
change and that's not required by the WA. pre_plane_update is only
called on the fastset path, and this WA is only needed during a full
modeset (luckily b/c of the vblank wait).

> 
> Also on spec they say:
> "
> Chance of underflows when the Pipe Scaler is enabled during a mode-set while the Planes are disabled.
> Workaround: Disable the Scaler clock gating when entering a mode-set routine.
> The Scaler clock gating should be re-enabled at the end of the mode-set sequence.
> "
> 
> So I wonder if here is not already too late to disable the clock gatings.

Yea, the wording is a bit unclear, it doesn't either say for example
when to re-enable the clock gating. I will ask for clarification, but
I'd say it's a reasonable assumption that the pipe scaler clock gating
doesn't have any effect until the pipe scaler is enabled and this is
also what I see during my tests. So imo we can go with this until proven
otherwise.

> 
> >  	if (INTEL_GEN(dev_priv) >= 9)
> >  		skylake_pfit_enable(intel_crtc);
> >  	else
> > @@ -5555,6 +5575,11 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> >  
> >  	intel_encoders_enable(crtc, pipe_config, old_state);
> >  
> > +	if (psl_clkgate_wa) {
> > +		intel_wait_for_vblank(dev_priv, pipe);
> > +		glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, false);
> > +	}
> > +
> >  	if (intel_crtc->config->has_pch_encoder) {
> >  		intel_wait_for_vblank(dev_priv, pipe);
> >  		intel_wait_for_vblank(dev_priv, pipe);
> > -- 
> > 2.13.2
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx