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

[Jesse Barnes]

On Tue, 23 Apr 2013 16:28:17 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> On Tue, Apr 23, 2013 at 11:45:03AM +0300, Jani Nikula wrote:
> > On Tue, 23 Apr 2013, Jesse Barnes <jbarnes at virtuousgeek.org> 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.
> > >
> > > v2: use delayed work instead of timer + work queue combo (Ville)
> > > v3: fix up delayed work cancel (must be outside lock) (Daniel)
> > >     fix up delayed work handling func for delayed work (Jesse)
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |    2 ++
> > >  drivers/gpu/drm/i915/i915_irq.c |   11 +++++++++++
> > >  drivers/gpu/drm/i915/intel_pm.c |   22 ++++++++++++++++++++++
> > >  3 files changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 2557fc7..f563f25 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 delayed_work 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;
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 932e7f8..af385e2 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_delayed_work(dev_priv->wq, &dev_priv->rps.vlv_work,
> > > +				 jiffies + msecs_to_jiffies(100));
> > 
> > Hooray for conflicting semantics. Unlike mod_timer, mod_delayed_work
> > wants a delay, not an absolute time in jiffies.
> > 
> > > +	}
> > > +
> > >  	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..c106187 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2822,6 +2822,23 @@ 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.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_enable_rps(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -2886,6 +2903,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> > >  	rpe = valleyview_rps_rpe_freq(dev_priv);
> > >  	DRM_DEBUG_DRIVER("RPe GPU freq: %d\n",
> > >  			 vlv_gpu_freq(dev_priv->mem_freq, rpe));
> > > +	dev_priv->rps.rpe_delay = rpe;
> > >  
> > >  	val = valleyview_rps_min_freq(dev_priv);
> > >  	DRM_DEBUG_DRIVER("min GPU freq: %d\n", vlv_gpu_freq(dev_priv->mem_freq,
> > > @@ -2895,6 +2913,8 @@ static void valleyview_enable_rps(struct drm_device *dev)
> > >  	DRM_DEBUG_DRIVER("setting GPU freq to %d\n",
> > >  			 vlv_gpu_freq(dev_priv->mem_freq, rpe));
> > >  
> > > +	INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work);
> > > +
> > >  	valleyview_set_rps(dev_priv->dev, rpe);
> > >  
> > >  	/* requires MSI enabled */
> > > @@ -3640,6 +3660,8 @@ void intel_disable_gt_powersave(struct drm_device *dev)
> > >  		mutex_lock(&dev_priv->rps.hw_lock);
> > >  		gen6_disable_rps(dev);
> > >  		mutex_unlock(&dev_priv->rps.hw_lock);
> > 
> > Theoretically the work could get executed here, between unlock and
> > cancel. Does that matter?
> 
> AFAICS even gen6_pm_rps_work() might be executing even after
> gen6_disable_rps() was called.
> 
> I'm not quite sure if allowing gen6_pm_rps_work() to do stuff after
> gen6_disable_rps() was called is already a problem on it's own. At
> least it can overwrite RPNSWREQ on !VLV. BTW that register doesn't seem
> to exist on VLV and yet we're poking at it from gen6_disable_rps()...

Ok just posted a new set addressing these issues, please check them out.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center