Re: [PATCH] drm/i915: Display WA #1185 WaDisableDARBFClkGating:cnl, glk

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 10, 2017 at 12:24:24PM -0800, Rodrigo Vivi wrote:
> On Fri, Nov 10, 2017 at 08:13:44PM +0000, Ville Syrjälä wrote:
> > On Thu, Nov 09, 2017 at 02:26:32PM -0800, Rodrigo Vivi wrote:
> > > Display is not sending a PMRsp when a PMReq is received
> > > at the same time that all planes are turned off.
> > > State machine in the dcprunit is stuck in the WAIT4DONE
> > > state which means that there is no fill_done.
> > > 
> > > WA: disable arbiter clock gating, set bit [27] of 0x46530
> > > 
> > > v2: As Ville pointed out, based on the description the issue
> > >     can happen when disabling the planes, similar to
> > >     WaRsPkgCStateDisplayPMReq:hsw
> > >     Also description of the issue was updated on commit
> > >     message to make it more clear that we need this
> > >     earlier.
> > 
> > I guess we don't know exactly if this has the same ordering requirements
> > as WaRsPkgCStateDisplayPMReq:hsw. But playing it safe can't hurt I
> > suppose.
> > 
> > > 
> > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> > >  drivers/gpu/drm/i915/intel_display.c | 24 +++++++++++++++---------
> > >  2 files changed, 16 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 6ef33422f762..fc8c5f8260f6 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3819,6 +3819,7 @@ enum {
> > >   * GEN9 clock gating regs
> > >   */
> > >  #define GEN9_CLKGATE_DIS_0		_MMIO(0x46530)
> > > +#define   DARBF_GATING_DIS		(1 << 27)
> > >  #define   PWM2_GATING_DIS		(1 << 14)
> > >  #define   PWM1_GATING_DIS		(1 << 13)
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 84817ccc5305..a038610b66cc 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15080,6 +15080,20 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
> > >  	}
> > >  }
> > >  
> > > +/* System hang if this isn't done before disabling all planes! */
> > > +static void intel_early_display_was(struct drm_i915_private *dev_priv)
> > > +{
> > > +	/* Display WA #1185 WaDisableDARBFClkGating:cnl,glk */
> > > +	if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > 
> > Bspec says GLK isn't affected.
> 
> Bspec refers to GLK as Gen10 display...
> so GEN10:All is also GLK...
> but if you look 2 columns ahead you see this:
> GEN:BUG:1947072 [CNL, GLK]

Ah. And the hsd does indeed list glk as well.

> 
> > 
> > > +		I915_WRITE(GEN9_CLKGATE_DIS_0, I915_READ(GEN9_CLKGATE_DIS_0) |
> > > +			   DARBF_GATING_DIS);
> > > +
> > > +	/* WaRsPkgCStateDisplayPMReq:hsw */
> > 
> > Please keep the comment about the hsw system hangs here. The next guy to
> > touch this code is probably not going to be aware of it, or may simply
> > have forgotten it already.
> 
> makes sense...
> should I keep the one in the function comment as well? and duplicate
> the comment on the new one or since we are not that sure we just leave
> the comment close to the hsw one?

I'd just keep it on the hsw one since we know that's how it works
on hsw. Having it on the cnl/glk one would be more speculation at
this point.

> 
> > 
> > With that those addresses this is
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> thanks
> 
> > 
> > > +	if (IS_HASWELL(dev_priv))
> > > +		I915_WRITE(CHICKEN_PAR1_1,
> > > +			   I915_READ(CHICKEN_PAR1_1) | FORCE_ARB_IDLE_PLANES);
> > > +}
> > > +
> > >  /* Scan out the current hw modeset state,
> > >   * and sanitizes it to the current state
> > >   */
> > > @@ -15093,15 +15107,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
> > >  	struct intel_encoder *encoder;
> > >  	int i;
> > >  
> > > -	if (IS_HASWELL(dev_priv)) {
> > > -		/*
> > > -		 * WaRsPkgCStateDisplayPMReq:hsw
> > > -		 * System hang if this isn't done before disabling all planes!
> > > -		 */
> > > -		I915_WRITE(CHICKEN_PAR1_1,
> > > -			   I915_READ(CHICKEN_PAR1_1) | FORCE_ARB_IDLE_PLANES);
> > > -	}
> > > -
> > > +	intel_early_display_was(dev_priv);
> > >  	intel_modeset_readout_hw_state(dev);
> > >  
> > >  	/* HW state is read out, now we need to sanitize this mess. */
> > > -- 
> > > 2.13.6
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx