From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func Date: Wed, 5 Mar 2014 14:21:19 +0200 Message-ID: <20140305122119.GN3852@intel.com> References: <20140303144620.GY3852@intel.com> <1394020839-27257-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 99578FA238 for ; Wed, 5 Mar 2014 04:21:23 -0800 (PST) Content-Disposition: inline In-Reply-To: <1394020839-27257-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org 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 > Cc: Ben Widawsky > Cc: Ville Syrj=E4l=E4 Looks good. Reviewed-by: Ville Syrj=E4l=E4 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/i= ntel_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 =3D > - container_of(work, typeof(*dev_priv), uncore.force_wake_work.work); > + struct drm_i915_private *dev_priv =3D (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 =3D=3D 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 =3D=3D 0) \ > - dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > - FORCEWAKE_ALL); \ > - val =3D __raw_i915_read##x(dev_priv, reg); \ > - if (dev_priv->uncore.forcewake_count =3D=3D 0) \ > - dev_priv->uncore.funcs.force_wake_put(dev_priv, \ > - FORCEWAKE_ALL); \ > - } else { \ > - val =3D __raw_i915_read##x(dev_priv, reg); \ > + if (dev_priv->uncore.forcewake_count =3D=3D 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 =3D __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 =3D 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 =3D __vlv_force_wake_get; > @@ -795,10 +791,11 @@ void intel_uncore_fini(struct drm_device *dev) > { > struct drm_i915_private *dev_priv =3D 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 -- = Ville Syrj=E4l=E4 Intel OTC