On Tue, Jan 27, 2015 at 04:36:16PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxx>
> Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
