[Intel-gfx] [PATCH 2/2] drm/i915: Introduce intel_set_rps()

[Chris Wilson]

On Tue, Jan 27, 2015 at 04:36:16PM +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at chris-wilson.co.uk>
> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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.

> 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().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre