Re: [PATCH v3] drm/i915/icl: Fix clockgating issue when using scalers

From: "Sripada, Radhakrishna" <radhakrishna.sripada@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v3] drm/i915/icl: Fix clockgating issue when using scalers
Date: Mon, 25 Mar 2019 19:45:39 +0000	[thread overview]
Message-ID: <5bf9d99727d53ee2333faf5dbcb67a02ca5ce9da.camel@intel.com> (raw)
In-Reply-To: <20190322131449.GQ3888@intel.com>

On Fri, 2019-03-22 at 15:14 +0200, Ville Syrjälä wrote:
> On Thu, Mar 21, 2019 at 02:44:31PM -0700, Radhakrishna Sripada wrote:
> > Fixes the clock-gating issue when pipe scaling is enabled.
> > (Lineage #2006604312)
> > 
> > V2: Fix typo in headline(Chris)
> >     Handle the non double buffered nature of the register(Ville)
> > V3: Fix checkpatch warning. BAT failure for V2 on gen3 looks
> > unrelated.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Aditya Swarup <aditya.swarup@intel.com>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com
> > >
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 43 +++++++++++++++++-------
> > ----
> >  1 file changed, 27 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 7c15b428ff84..cfa19ae12e22 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -469,13 +469,22 @@ static const struct intel_limit
> > intel_limits_bxt = {
> >  static void
> >  skl_wa_clkgate(struct drm_i915_private *dev_priv, int pipe, bool
> > enable)
> >  {
> > +	u32 val = 0;
> > +
> > +	/*
> > +	 * Wa_2006604312:icl
> > +	 */
> > +	if (IS_ICELAKE(dev_priv))
> > +		val = DPFR_GATING_DIS;
> > +	else
> > +		val = DUPS1_GATING_DIS | DUPS2_GATING_DIS;
> > +
> > +	/* WA Display #0827: Gen9:all */
> 
> You're now conflating two workaround and splitting their
> implementation
> between two functions in a confusing way. Better to keep them
> separate
> IMO.

I encountered a similar question while writing the code. I discussed
with Rodrigo about a possible way of implementing a function pointer
kind of prototype followed by ICL/SKL wa to differentiate b/ween the
two implementations. It was suggested that the approach is overly
complicated. Since we do use the same register in both cases and due to
the non double buffered nature of the register I have resorted to the
stupid/dirty way. Do you suggest otherwise?

- Radhakrishna(RK) Sripada 
> 
> >  	if (enable)
> > -		I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > -			   DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> > +		I915_WRITE(CLKGATE_DIS_PSL(pipe), val);
> >  	else
> >  		I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > -			   I915_READ(CLKGATE_DIS_PSL(pipe)) &
> > -			   ~(DUPS1_GATING_DIS | DUPS2_GATING_DIS));
> > +			   I915_READ(CLKGATE_DIS_PSL(pipe)) & ~val);
> >  }
> >  
> >  static bool
> > @@ -5481,14 +5490,18 @@ static bool
> > hsw_post_update_enable_ips(const struct intel_crtc_state
> > *old_crtc_s
> >  	return !old_crtc_state->ips_enabled;
> >  }
> >  
> > -static bool needs_nv12_wa(struct drm_i915_private *dev_priv,
> > -			  const struct intel_crtc_state *crtc_state)
> > +static bool skl_needs_clk_wa(struct drm_i915_private *dev_priv,
> > +			     const struct intel_crtc_state *crtc_state)
> >  {
> > -	if (!crtc_state->nv12_planes)
> > -		return false;
> > -
> >  	/* WA Display #0827: Gen9:all */
> > -	if (IS_GEN(dev_priv, 9) && !IS_GEMINILAKE(dev_priv))
> > +	if (!!crtc_state->nv12_planes && IS_GEN(dev_priv, 9) &&
> > +	    !IS_GEMINILAKE(dev_priv))
> > +		return true;
> > +
> > +	/*
> > +	 * Wa_2006604312:icl
> > +	 */
> > +	if (IS_ICELAKE(dev_priv) && crtc_state->pch_pfit.enabled)
> >  		return true;
> >  
> >  	return false;
> > @@ -5527,9 +5540,8 @@ static void intel_post_plane_update(struct
> > intel_crtc_state *old_crtc_state)
> >  			intel_post_enable_primary(&crtc->base,
> > pipe_config);
> >  	}
> >  
> > -	/* Display WA 827 */
> > -	if (needs_nv12_wa(dev_priv, old_crtc_state) &&
> > -	    !needs_nv12_wa(dev_priv, pipe_config)) {
> > +	if (skl_needs_clk_wa(dev_priv, old_crtc_state) &&
> > +	    !skl_needs_clk_wa(dev_priv, pipe_config)) {
> >  		skl_wa_clkgate(dev_priv, crtc->pipe, false);
> >  	}
> >  }
> > @@ -5566,9 +5578,8 @@ static void intel_pre_plane_update(struct
> > intel_crtc_state *old_crtc_state,
> >  			intel_set_cpu_fifo_underrun_reporting(dev_priv,
> > crtc->pipe, false);
> >  	}
> >  
> > -	/* Display WA 827 */
> > -	if (!needs_nv12_wa(dev_priv, old_crtc_state) &&
> > -	    needs_nv12_wa(dev_priv, pipe_config)) {
> > +	if (!skl_needs_clk_wa(dev_priv, old_crtc_state) &&
> > +	    skl_needs_clk_wa(dev_priv, pipe_config)) {
> >  		skl_wa_clkgate(dev_priv, crtc->pipe, true);
> >  	}
> >  
> > -- 
> > 2.20.0.rc2.7.g965798d1f299
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx