[Intel-gfx] [PATCH 1/2] drm/i915: Stop using RP_DOWN_EI on Baytrail

[Chris Wilson]

On Thu, Mar 09, 2017 at 05:24:14PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > On Baytrail, we manually calculate busyness over the evaluation interval
> > to avoid issues with miscaluations with RC6 enabled. However, it turns
> > out that the DOWN_EI interrupt generator is completely bust - it
> > operates in two modes, continuous or never. Neither of which are
> > conducive to good behaviour. Stop unmask the DOWN_EI interrupt and just
> > compute everything from the UP_EI which does seem to correspond to the
> > desired interval.
> >
> > v2: Fixup gen6_rps_pm_mask() as well
> >
> > Fixes: 43cf3bf084ba ("drm/i915: Improved w/a for rps on Baytrail")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Cc: <stable at vger.kernel.org> # v4.1+
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  2 +-
> >  drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++-------------------
> >  drivers/gpu/drm/i915/intel_pm.c |  5 +++--
> >  3 files changed, 13 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9c5b3dd9f6f1..d70bbbd6a5fd 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1366,7 +1366,7 @@ struct intel_gen6_power_mgmt {
> >  	unsigned boosts;
> >  
> >  	/* manual wa residency calculations */
> > -	struct intel_rps_ei up_ei, down_ei;
> > +	struct intel_rps_ei ei;
> >  
> >  	/*
> >  	 * Protects RPS/RC6 register access and PCU communication.
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index bc70e2c451b2..29b10bab38b6 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1050,10 +1050,10 @@ static void vlv_c0_read(struct drm_i915_private *dev_priv,
> >  }
> >  
> >  static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> > -			 const struct intel_rps_ei *old,
> >  			 const struct intel_rps_ei *now,
> >  			 int threshold)
> >  {
> > +	const struct intel_rps_ei *old = &dev_priv->rps.ei;
> >  	u64 time, c0;
> >  	unsigned int mul = 100;
> >  
> > @@ -1079,8 +1079,7 @@ static bool vlv_c0_above(struct drm_i915_private *dev_priv,
> >  
> >  void gen6_rps_reset_ei(struct drm_i915_private *dev_priv)
> >  {
> > -	vlv_c0_read(dev_priv, &dev_priv->rps.down_ei);
> > -	dev_priv->rps.up_ei = dev_priv->rps.down_ei;
> > +	memset(&dev_priv->rps.ei, 0, sizeof(dev_priv->rps.ei));
> 
> With this change you will always downclock with the first change
> after reset. Is this desired? (due to busy handling the rampup?).

No, it was just noticing the skip for vlv_c0_above without thinking
about the effect of ! for DOWN.
 
> I would have made it so that vlv_c0_above returns true if
> the value was reset. Then we would be on the safe side going
> upwards for the first tick.

Easier, just don't evaluate vlv_v0_above.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre