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: Mon, 3 Mar 2014 16:46:20 +0200 Message-ID: <20140303144620.GY3852@intel.com> References: <20140228091633.GO3852@intel.com> <1393613043-23213-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 00A1310575B for ; Mon, 3 Mar 2014 06:46:30 -0800 (PST) Content-Disposition: inline In-Reply-To: <1393613043-23213-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 Fri, Feb 28, 2014 at 06:44:03PM +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. > = > Signed-off-by: Chris Wilson > Cc: Ben Widawsky > Cc: Ville Syrj=E4l=E4 > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/intel_uncore.c | 34 +++++++++++++++----------------= --- > 2 files changed, 16 insertions(+), 20 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index b22765192018..8af8e0dd3943 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 c62841404c82..8ee171178bfe 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -289,10 +289,8 @@ 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(struct drm_i915_private *dev_priv) > { > - struct drm_i915_private *dev_priv =3D > - container_of(work, typeof(*dev_priv), uncore.force_wake_work.work); > unsigned long irqflags; > = > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > @@ -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 =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); 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. > } > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > = > @@ -484,17 +481,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; \ > } > = > @@ -681,8 +676,9 @@ 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, > + (void (*)(unsigned long))gen6_force_wake_timer, I'd prefer to do the required casting in gen6_force_wake_timer(). Seems a bit less error prone since we'd at least get type checking for the function pointer. > + (unsigned long)dev_priv); > = > if (IS_VALLEYVIEW(dev)) { > dev_priv->uncore.funcs.force_wake_get =3D __vlv_force_wake_get; > @@ -794,7 +790,7 @@ 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); This could leave force wake enabled. > = > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev); > -- = > 1.9.0 -- = Ville Syrj=E4l=E4 Intel OTC