| Message ID | 1394020839-27257-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 05, 2014 at 12:00:39PM +0000, Chris Wilson wrote: > We don't want to suffer scheduling delay when turning off the GPU after > waking it up to touch registers. Ideally, we only want to keep the GPU > awake for the register access sequence, with a single forcewake dance on > the first access and release immediately after the last. We set a timer > on the first access so that we only dance once and on the next scheduler > tick, we drop the forcewake again. > > This moves the cleanup routine from the common i915 workqueue to a timer > func so that we don't anger powertop, and drop the forcewake again > quicker. > > v2: Enable the deferred force_wake_put for regular register reads as > well. > v3: Beautification and make sure we disable forcewake when shutting > down. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ben Widawsky <ben@bwidawsk.net> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Looks good. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> The next question will be if we want this for VLV too, but that's going to be a bit more comlicated due to the dual forcewake counts. I guess we can leave that alone for now. > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_uncore.c | 35 ++++++++++++++++------------------- > 2 files changed, 17 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b0ab6330b294..bb9ea026310c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -508,7 +508,7 @@ struct intel_uncore { > unsigned fw_rendercount; > unsigned fw_mediacount; > > - struct delayed_work force_wake_work; > + struct timer_list force_wake_timer; > }; > > #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index d1e9d63953c5..e5b59ac5151a 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -289,10 +289,9 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv, > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > } > > -static void gen6_force_wake_work(struct work_struct *work) > +static void gen6_force_wake_timer(unsigned long arg) > { > - struct drm_i915_private *dev_priv = > - container_of(work, typeof(*dev_priv), uncore.force_wake_work.work); > + struct drm_i915_private *dev_priv = (void *)arg; > unsigned long irqflags; > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > @@ -405,9 +404,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); > } > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > @@ -484,17 +482,15 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > static u##x \ > gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > REG_READ_HEADER(x); \ > - if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > - if (dev_priv->uncore.forcewake_count == 0) \ > - dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > - FORCEWAKE_ALL); \ > - val = __raw_i915_read##x(dev_priv, reg); \ > - if (dev_priv->uncore.forcewake_count == 0) \ > - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ > - FORCEWAKE_ALL); \ > - } else { \ > - val = __raw_i915_read##x(dev_priv, reg); \ > + if (dev_priv->uncore.forcewake_count == 0 && \ > + NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > + dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > + FORCEWAKE_ALL); \ > + dev_priv->uncore.forcewake_count++; \ > + mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \ > + jiffies + 1); \ > } \ > + val = __raw_i915_read##x(dev_priv, reg); \ > REG_READ_FOOTER; \ > } > > @@ -682,8 +678,8 @@ void intel_uncore_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > - INIT_DELAYED_WORK(&dev_priv->uncore.force_wake_work, > - gen6_force_wake_work); > + setup_timer(&dev_priv->uncore.force_wake_timer, > + gen6_force_wake_timer, (unsigned long)dev_priv); > > if (IS_VALLEYVIEW(dev)) { > dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get; > @@ -795,10 +791,11 @@ 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); > > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev); > + intel_uncore_forcewake_reset(dev); > } > > static const struct register_whitelist { > -- > 1.9.0
On Wed, Mar 05, 2014 at 02:21:19PM +0200, Ville Syrjälä wrote: > On Wed, Mar 05, 2014 at 12:00:39PM +0000, Chris Wilson wrote: > > We don't want to suffer scheduling delay when turning off the GPU after > > waking it up to touch registers. Ideally, we only want to keep the GPU > > awake for the register access sequence, with a single forcewake dance on > > the first access and release immediately after the last. We set a timer > > on the first access so that we only dance once and on the next scheduler > > tick, we drop the forcewake again. > > > > This moves the cleanup routine from the common i915 workqueue to a timer > > func so that we don't anger powertop, and drop the forcewake again > > quicker. > > > > v2: Enable the deferred force_wake_put for regular register reads as > > well. > > v3: Beautification and make sure we disable forcewake when shutting > > down. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Ben Widawsky <ben@bwidawsk.net> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Looks good. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The next question will be if we want this for VLV too, but that's going > to be a bit more comlicated due to the dual forcewake counts. I guess we > can leave that alone for now. Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b0ab6330b294..bb9ea026310c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -508,7 +508,7 @@ struct intel_uncore { unsigned fw_rendercount; unsigned fw_mediacount; - struct delayed_work force_wake_work; + struct timer_list force_wake_timer; }; #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index d1e9d63953c5..e5b59ac5151a 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -289,10 +289,9 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv, spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); } -static void gen6_force_wake_work(struct work_struct *work) +static void gen6_force_wake_timer(unsigned long arg) { - struct drm_i915_private *dev_priv = - container_of(work, typeof(*dev_priv), uncore.force_wake_work.work); + struct drm_i915_private *dev_priv = (void *)arg; unsigned long irqflags; spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); @@ -405,9 +404,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); } spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); @@ -484,17 +482,15 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ static u##x \ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ REG_READ_HEADER(x); \ - if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ - if (dev_priv->uncore.forcewake_count == 0) \ - dev_priv->uncore.funcs.force_wake_get(dev_priv, \ - FORCEWAKE_ALL); \ - val = __raw_i915_read##x(dev_priv, reg); \ - if (dev_priv->uncore.forcewake_count == 0) \ - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ - FORCEWAKE_ALL); \ - } else { \ - val = __raw_i915_read##x(dev_priv, reg); \ + if (dev_priv->uncore.forcewake_count == 0 && \ + NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ + dev_priv->uncore.funcs.force_wake_get(dev_priv, \ + FORCEWAKE_ALL); \ + dev_priv->uncore.forcewake_count++; \ + mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \ + jiffies + 1); \ } \ + val = __raw_i915_read##x(dev_priv, reg); \ REG_READ_FOOTER; \ } @@ -682,8 +678,8 @@ void intel_uncore_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - INIT_DELAYED_WORK(&dev_priv->uncore.force_wake_work, - gen6_force_wake_work); + setup_timer(&dev_priv->uncore.force_wake_timer, + gen6_force_wake_timer, (unsigned long)dev_priv); if (IS_VALLEYVIEW(dev)) { dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get; @@ -795,10 +791,11 @@ 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); /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev); + intel_uncore_forcewake_reset(dev); } static const struct register_whitelist {
We don't want to suffer scheduling delay when turning off the GPU after waking it up to touch registers. Ideally, we only want to keep the GPU awake for the register access sequence, with a single forcewake dance on the first access and release immediately after the last. We set a timer on the first access so that we only dance once and on the next scheduler tick, we drop the forcewake again. This moves the cleanup routine from the common i915 workqueue to a timer func so that we don't anger powertop, and drop the forcewake again quicker. v2: Enable the deferred force_wake_put for regular register reads as well. v3: Beautification and make sure we disable forcewake when shutting down. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ben Widawsky <ben@bwidawsk.net> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_uncore.c | 35 ++++++++++++++++------------------- 2 files changed, 17 insertions(+), 20 deletions(-)