Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

Chris Wilson
2014-03-03T14:55:34Z

On Mon, Mar 03, 2014 at 04:46:20PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 28, 2014 at 06:44:03PM +0000, Chris Wilson wrote:
> > @@ -405,9 +403,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > if (--dev_priv->uncore.forcewake_count == 0) {
> > dev_priv->uncore.forcewake_count++;
> > - mod_delayed_work(dev_priv->wq,
> > - &dev_priv->uncore.force_wake_work,
> > - 1);
> > + mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> > + jiffies + 1);
>
> This could expire more or less immediately, but it should be fine. We'd
> just end up doing two forcewake_get()s instead of one, which should
> still be better than >2 if the theory of the timer holds.

When does the timer actually fire? I am presuming that the callbacks are
run from a soft-interrupt at the next scheduling point. Using a pinned
timer here should mean that the earlier that it could fire is after this
sequence of register writes (or at least in the next mutex_lock() etc).
At any rate, we only care that we reduce the number of times we do the
forcewake dance.

> > @@ -794,7 +790,7 @@ void intel_uncore_fini(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > - flush_delayed_work(&dev_priv->uncore.force_wake_work);
> > + del_timer_sync(&dev_priv->uncore.force_wake_timer);
>
> This could leave force wake enabled.

Bah humbug. Not our problem ;-)
-Chris

--
Chris Wilson, Intel Open Source Technology Centre