Re: [PATCH 2/2] drm/i915: Introduce intel_set_rps()

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

On Wed, Jan 28, 2015 at 10:29:06AM +0000, Chris Wilson wrote:
> On Tue, Jan 27, 2015 at 04:36:16PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Replace the valleyview_set_rps() and gen6_set_rps() calls with
> > intel_set_rps() which itself does the IS_VALLEYVIEW() check. The
> > code becomes simpler since the callers don't have to do this check
> > themselves.
> > 
> > Most of the change was performe with the following semantic patch:
> > @@
> > expression E1, E2;
> > @@
> > (
> > - valleyview_set_rps(E1, E2)
> > + intel_set_rps(E1, E2)
> > |
> > - gen6_set_rps(E1, E2)
> > + intel_set_rps(E1, E2)
> > )
> > 
> > @@
> > expression E1, E2, E3;
> > @@
> > - if (IS_VALLEYVIEW(E1)) {
> > -  intel_set_rps(E2, E3);
> > - } else {
> > -  intel_set_rps(E2, E3);
> > - }
> > + intel_set_rps(E2, E3);
> > 
> > Adding intel_set_rps() and making valleyview_set_rps() and gen6_set_rps()
> > static was done manually.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Hmm, I like half of them :|
> 
> The external callers from i915_debugfs.c, i915_irq.c are good. But
> inside intel_pm.c, it is more mixed. gen6_rps_boost() clearly wants
> intel_set_rps(), but from within the gen specific lowlevel functions, it
> looks odder to callback into intel_set_rps.

The semantic patch was greedy, and I figured I'd leave it that way to
avoid someone accidentally copy-pasting the wrong thing. But I can
change them back if that's the preferred way.

> 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 6ece663..2bad1e8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3750,7 +3750,7 @@ static u32 gen6_rps_pm_mask(struct drm_i915_private *dev_priv, u8 val)
> >  /* gen6_set_rps is called to update the frequency request, but should also be
> >   * called when the range (min_delay and max_delay) is modified so that we can
> >   * update the GEN6_RP_INTERRUPT_LIMITS register accordingly. */
> > -void gen6_set_rps(struct drm_device *dev, u8 val)
> > +static void gen6_set_rps(struct drm_device *dev, u8 val)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > @@ -3801,7 +3801,7 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >  
> >  	/* CHV and latest VLV don't need to force the gfx clock */
> >  	if (IS_CHERRYVIEW(dev) || dev->pdev->revision >= 0xd) {
> > -		valleyview_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +		intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >  		return;
> >  	}
> >  
> > @@ -3842,7 +3842,8 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv)
> >  		if (IS_VALLEYVIEW(dev))
> >  			vlv_set_rps_idle(dev_priv);
> >  		else
> > -			gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +			intel_set_rps(dev_priv->dev,
> > +				      dev_priv->rps.min_freq_softlimit);
> >  		dev_priv->rps.last_adj = 0;
> >  	}
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> 
> These two are the most dubious for me.
> 
> >  static void gen9_disable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -4176,7 +4180,7 @@ static void gen8_enable_rps(struct drm_device *dev)
> >  	/* 6: Ring frequency + overclocking (our driver does this later */
> >  
> >  	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> > -	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >  
> >  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> > @@ -4270,7 +4274,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> >  	}
> >  
> >  	dev_priv->rps.power = HIGH_POWER; /* force a reset */
> > -	gen6_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.min_freq_softlimit);
> >  
> >  	rc6vids = 0;
> >  	ret = sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > @@ -4812,7 +4816,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
> >  			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> >  			 dev_priv->rps.efficient_freq);
> >  
> > -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> >  
> >  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> > @@ -4896,7 +4900,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >  			 intel_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
> >  			 dev_priv->rps.efficient_freq);
> >  
> > -	valleyview_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> > +	intel_set_rps(dev_priv->dev, dev_priv->rps.efficient_freq);
> >  
> >  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> 
> However, these should probably be converted to gen6_rps_idle() calls
> instead (which requires setting dev_priv->rps.enable earlier) or movinng
> the idle call out of the gen specific enable rps functions and into the
> main intel_gen6_powersave_work().

Hmm. Yeah _idle() would seem to make sense here.

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