From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 03/11] drm/i915: put runtime PM only when we actually release force_wake Date: Fri, 21 Feb 2014 12:16:58 -0800 Message-ID: <20140221121658.72caa995@jbarnes-desktop> References: <1393001548-2883-1-git-send-email-przanoni@gmail.com> <1393001548-2883-4-git-send-email-przanoni@gmail.com> <20140221093408.5ccb6c93@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy7-pub.mail.unifiedlayer.com (oproxy7-pub.mail.unifiedlayer.com [67.222.55.9]) by gabe.freedesktop.org (Postfix) with SMTP id 04740FB5BC for ; Fri, 21 Feb 2014 12:16:45 -0800 (PST) In-Reply-To: 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: Paulo Zanoni Cc: Intel Graphics Development , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, 21 Feb 2014 17:08:50 -0300 Paulo Zanoni wrote: > 2014-02-21 14:34 GMT-03:00 Jesse Barnes : > > On Fri, 21 Feb 2014 13:52:20 -0300 > > Paulo Zanoni wrote: > > > >> From: Paulo Zanoni > >> > >> When we call gen6_gt_force_wake_put we don't actually put force_wake, > >> we just schedule gen6_force_wake_work through mod_delayed_work, and > >> that will eventually release force_wake. > >> > >> The problem is that we call intel_runtime_pm_put directly at > >> gen6_gt_force_wake_put, so most of the times we put our runtime PM > >> reference before the delayed work happens, so we may runtime suspend > >> while force_wake is still supposed to be enabled if the graphics > >> autosuspend_delay_ms is too small. > >> > >> Now the nice thing about the current code is that after it triggers > >> the delayed work function it gets a refcount, and it only triggers the > >> delayed work function if refcount is zero. This guarantees that when > >> we schedule the funciton, it will run before we try to schedule it > >> again, which simplifies the problem and allows for the current > >> solution to work properly (hopefully!). > >> > >> Signed-off-by: Paulo Zanoni > >> --- > >> drivers/gpu/drm/i915/intel_uncore.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > >> index c628414..1f7226f 100644 > >> --- a/drivers/gpu/drm/i915/intel_uncore.c > >> +++ b/drivers/gpu/drm/i915/intel_uncore.c > >> @@ -299,6 +299,8 @@ static void gen6_force_wake_work(struct work_struct *work) > >> if (--dev_priv->uncore.forcewake_count == 0) > >> dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL); > >> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > >> + > >> + intel_runtime_pm_put(dev_priv); > >> } > >> > >> static void intel_uncore_forcewake_reset(struct drm_device *dev) > >> @@ -393,6 +395,7 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine) > >> void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine) > >> { > >> unsigned long irqflags; > >> + bool delayed = false; > >> > >> if (!dev_priv->uncore.funcs.force_wake_put) > >> return; > >> @@ -405,13 +408,15 @@ 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++; > >> + delayed = true; > >> mod_delayed_work(dev_priv->wq, > >> &dev_priv->uncore.force_wake_work, > >> 1); > >> } > >> spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > >> > >> - intel_runtime_pm_put(dev_priv); > >> + if (!delayed) > >> + intel_runtime_pm_put(dev_priv); > >> } > >> > >> /* We give fast paths for the really cool registers */ > > > > Do we need this for the VLV path too? > > Yeah, my patch is wrong for VLV due to that "return". I'll send a new version. > > By the way, why doesn't VLV use the delayed work queue? I would assume > the work queue is there to improve performance somehow, so it could be > a good idea to use it... And maybe try to avoid special-casing VLV > would be good too :) I don't know why VLV has an early return there rather than just using a function pointer like everything else. ISTR reviewing that patch from Deepak and suggesting something else, but I guess Daniel merged it anyway. But yes, it should be fixed up as well and should probably use the delayed mechanism. -- Jesse Barnes, Intel Open Source Technology Center