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()...
Hm that could cause a problem, I know some of the reserved regs in that
region have bad side effects.
I'll cook up a patch to make a valleyview_disable_rps.
Having the work done after rps disable is probably ok, but it's still
messy. I'll move the bits up above the rps disable call. Since
they're sync that should serialize things.
And I'll fix up the mod timer; converting from timers and work queues
to delayed work isn't as mindless as it ought to be (I already caught
the INIT_DELAYED_WORK bug I had before too, groan).
--
Jesse Barnes, Intel Open Source Technology Center
