From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance Date: Fri, 14 Mar 2014 10:18:47 +0200 Message-ID: <20140314081847.GT20292@intel.com> References: <1394783326-10484-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 1AB67FB1B8 for ; Fri, 14 Mar 2014 01:18:53 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1394783326-10484-1-git-send-email-daniel.vetter@ffwll.ch> 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: Daniel Vetter Cc: Paulo Zanoni , Intel Graphics Development , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Fri, Mar 14, 2014 at 08:48:46AM +0100, Daniel Vetter wrote: > This regression has been introduced in > = > commit 8232644ccf099548710843e97360a3fcd6d28e04 > Author: Chris Wilson > Date: Wed Mar 5 12:00:39 2014 +0000 > = > drm/i915: Convert the forcewake worker into a timer func > = > which started to use the delayed forcewake put also for the register > I/O forcewake dance. But this conflicts functionally with the > (reviewed in parallel) > = > commit 6d88064edcfc5e5893371f7c06b9f3078dc1edf6 > Author: Paulo Zanoni > Date: Fri Feb 21 17:58:29 2014 -0300 > = > drm/i915: put runtime PM only when we actually release force_wake > = > which moved the runtime pm put calls into the delayed forcewake put > function to avoid an inversion between these. The problem is that the > register I/O function do _not_ grab a runtime pm reference, hence > dropping it is a bug. > = > So split the timer into two to re-balance the runtime pm refcounting. > The tricky bit to ensure is that the _raw timer doesn't run after > we've runtime-suspended the device. After all it only has an implicit > runtime pm reference provided by its caller. But the del_timer_sync in > the runtime suspend code will ensure this. > = > Add a comment to document this all. Yuck. Why don't we just stick the runtime pm put into gen6_gt_force_wake_put() and let Chris's timer cancellation + forcewake reset fixes take care of things? > = > Cc: Chris Wilson > Cc: Ben Widawsky > Cc: Ville Syrj=E4l=E4 > Cc: Paulo Zanoni > Cc: Jesse Barnes > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D76151 > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_drv.h | 9 +++++++++ > drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++++-- > 2 files changed, 21 insertions(+), 2 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 70fbe904016f..c2789702b9a5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -511,7 +511,16 @@ struct intel_uncore { > unsigned fw_rendercount; > unsigned fw_mediacount; > = > + /* > + * Delayed forcewake put timers. The _raw one is for register I/O > + * functions which don't grab a runtime pm reference, instead presuming > + * that someone else holds that already. > + * > + * Synchronization with runtime pm actually suspended the device happens > + * in the runtime suspend path in intel_uncore_forcewake_reset. > + */ > struct timer_list force_wake_timer; > + struct timer_list force_wake_timer_raw; > }; > = > #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 e6bb421a3dbd..8010e2caf821 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -293,7 +293,7 @@ void vlv_force_wake_put(struct drm_i915_private *dev_= priv, > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > } > = > -static void gen6_force_wake_timer(unsigned long arg) > +static void gen6_force_wake_timer_raw(unsigned long arg) > { > struct drm_i915_private *dev_priv =3D (void *)arg; > unsigned long irqflags; > @@ -304,6 +304,13 @@ static void gen6_force_wake_timer(unsigned long arg) > if (--dev_priv->uncore.forcewake_count =3D=3D 0) > dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > +} > + > +static void gen6_force_wake_timer(unsigned long arg) > +{ > + struct drm_i915_private *dev_priv =3D (void *)arg; > + > + gen6_force_wake_timer_raw(arg); > = > intel_runtime_pm_put(dev_priv); > } > @@ -314,6 +321,7 @@ static void intel_uncore_forcewake_reset(struct drm_d= evice *dev, bool restore) > unsigned long irqflags; > = > del_timer_sync(&dev_priv->uncore.force_wake_timer); > + del_timer_sync(&dev_priv->uncore.force_wake_timer_raw); > = > /* Hold uncore.lock across reset to prevent any register access > * with forcewake not set correctly > @@ -542,7 +550,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t= reg, bool trace) { \ > 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, \ > + mod_timer_pinned(&dev_priv->uncore.force_wake_timer_raw, \ > jiffies + 1); \ > } \ > val =3D __raw_i915_read##x(dev_priv, reg); \ > @@ -726,6 +734,8 @@ void intel_uncore_init(struct drm_device *dev) > = > setup_timer(&dev_priv->uncore.force_wake_timer, > gen6_force_wake_timer, (unsigned long)dev_priv); > + setup_timer(&dev_priv->uncore.force_wake_timer_raw, > + gen6_force_wake_timer_raw, (unsigned long)dev_priv); > = > if (IS_VALLEYVIEW(dev)) { > dev_priv->uncore.funcs.force_wake_get =3D __vlv_force_wake_get; > -- = > 1.8.5.2 -- = Ville Syrj=E4l=E4 Intel OTC