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@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.
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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
