[Intel-gfx] [PATCH] drm/i915: make sure GPU freq drops to minimum after entering RC6

[Jesse Barnes]

On Mon, 22 Apr 2013 14:39:17 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> On Fri, Apr 19, 2013 at 10:22:45AM -0700, Jesse Barnes wrote:
> > On VLV, the Punit doesn't automatically drop the GPU to it's minimum
> > voltage level when entering RC6, so we arm a timer to do it for us from
> > the RPS interrupt handler.  It'll generally only fire when we go idle
> > (or if for some reason there's a long delay between RPS interrupts), but
> > won't be re-armed again until the next RPS event, so shouldn't affect
> > power consumption after we go idle and it triggers.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |    4 ++++
> >  drivers/gpu/drm/i915/i915_irq.c |   11 +++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c |   32 ++++++++++++++++++++++++++++++++
> >  3 files changed, 47 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2557fc7..2900dd4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -660,6 +660,7 @@ struct i915_suspend_saved_registers {
> >  
> >  struct intel_gen6_power_mgmt {
> >  	struct work_struct work;
> > +	struct work_struct vlv_work;
> >  	u32 pm_iir;
> >  	/* lock - irqsave spinlock that protectects the work_struct and
> >  	 * pm_iir. */
> > @@ -670,6 +671,7 @@ struct intel_gen6_power_mgmt {
> >  	u8 cur_delay;
> >  	u8 min_delay;
> >  	u8 max_delay;
> > +	u8 rpe_delay;
> >  	u8 hw_max;
> >  
> >  	struct delayed_work delayed_resume_work;
> > @@ -945,6 +947,8 @@ typedef struct drm_i915_private {
> >  	} hpd_stats[HPD_NUM_PINS];
> >  	struct timer_list hotplug_reenable_timer;
> >  
> > +	struct timer_list vlv_rps_timer;
> > +
> >  	int num_pch_pll;
> >  	int num_plane;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 932e7f8..99b2e1c 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -488,6 +488,17 @@ static void gen6_pm_rps_work(struct work_struct *work)
> >  			gen6_set_rps(dev_priv->dev, new_delay);
> >  	}
> >  
> > +	if (IS_VALLEYVIEW(dev_priv->dev)) {
> > +		/*
> > +		 * On VLV, when we enter RC6 we may not be at the minimum
> > +		 * voltage level, so arm a timer to check.  It should only
> > +		 * fire when there's activity or once after we've entered
> > +		 * RC6, and then won't be re-armed until the next RPS interrupt.
> > +		 */
> > +		mod_timer(&dev_priv->vlv_rps_timer, jiffies +
> > +			  msecs_to_jiffies(100));
> > +	}
> > +
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 2557926..c1311c9 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2545,6 +2545,9 @@ static void gen6_disable_rps(struct drm_device *dev)
> >  	spin_unlock_irq(&dev_priv->rps.lock);
> >  
> >  	I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> > +
> > +	if (IS_VALLEYVIEW(dev))
> > +		del_timer_sync(&dev_priv->vlv_rps_timer);
> >  }
> >  
> >  int intel_enable_rc6(const struct drm_device *dev)
> > @@ -2822,6 +2825,30 @@ int valleyview_rps_min_freq(struct drm_i915_private *dev_priv)
> >  	return val & 0xff;
> >  }
> >  
> > +static void vlv_rps_timer_work(struct work_struct *work)
> > +{
> > +	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> > +						    rps.vlv_work);
> > +
> > +	/*
> > +	 * Timer fired, we must be idle.  Drop to min voltage state.
> > +	 * Note: we use RPe here since it should match the
> > +	 * Vmin we were shooting for.  That should give us better
> > +	 * perf when we come back out of RC6 than if we used the
> > +	 * min freq available.
> > +	 */
> > +	mutex_lock(&dev_priv->rps.hw_lock);
> > +	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +}
> > +
> > +static void valleyview_rps_timer_func(unsigned long data)
> > +{
> > +	struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
> > +
> > +	queue_work(dev_priv->wq, &dev_priv->rps.vlv_work);
> > +}
> 
> This looks somewhat like open coded delayed work to me. Would the
> standard delayed work be OK for this, or was there some specific reason
> you did it this way?

No particular reason other than not thinking about delayed work when I
wrote it. :)

I can fix it up; I guess it'll save us having the rps_timer_func.

-- 
Jesse Barnes, Intel Open Source Technology Center