From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 2/3] drm/i915: reference counted forcewake Date: Mon, 18 Apr 2011 10:31:43 -0700 Message-ID: <20110418173143.GA10406@bwgnt.jf.intel.com> References: <1302804827-11597-1-git-send-email-ben@bwidawsk.net> <1302804827-11597-5-git-send-email-ben@bwidawsk.net> <0d30dc$ls6j08@orsmga001.jf.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 2BC2F9E8A3 for ; Mon, 18 Apr 2011 10:31:46 -0700 (PDT) Content-Disposition: inline In-Reply-To: <0d30dc$ls6j08@orsmga001.jf.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Sat, Apr 16, 2011 at 07:52:44AM +0100, Chris Wilson wrote: > On Thu, 14 Apr 2011 11:13:46 -0700, Ben Widawsky wrote: > > Provide a reference count to track the forcewake state of the GPU. The > > savings is up to 1 UC read if the unit is already awake, but the main > > goal is to give userspace some mechanism to wake the GPU. > > > > v2: > > Added a spin_lock for synchronizing access to forcewake. The lock > > should not be heavily contested because any caller will either have > > struct_mutex or config.mutex, and those locks should be much more > > serializing than this. In terms of locking order: > > > > 1.[config.mutex] > > 2. [struct_mutex] > > 3. forcewake_lock > > > > By default i915_read[bwlq] functions now acquire forcewake_lock. Added a new > > _locked version of the read function for callers wishing to prevent the > > GT from possibly going to sleep in between reads. (We could also use > > these functions to try to get system state if the lock somehow becomes > > stuck). > > This patch needs to be split again. I want the update to the existing > interface and users to be separate from the introduction of a new > interface. (This means that should we ever regret that interface we can > rip it out without undoing the fixes.) > > Replacing the get()...long sequence of read/writes...put() did make me cry > a little, but it is indeed safer to move the check into the individual > reads (though there is nothing to prevent the rc6-window from closing > during the middle of that sequence... we need to check that is ok) and too > ugly to special case those rare times when we do modify 20 regs at once > (or maybe we have to?). In case you didn't notice, there is a new interface i915_read##x##_awake. This should serve that need. In other words: gen6_gt_force_wake_get() while(foo) i915_read32_awake() gen6_gt_force_wake_put() I'll split the patches. I can also use the awake() variant for the existing users, if you're okay with the awake() function (I was actually expecting a comment from you on that). For the relevant functions, it should be as simple as: s/I915_READ32/i915_read32_awake s/__gen6_gt_force_wake_/gen6_gt_force_wake_/ Jesse has started the email to verify IIR is in fact a problem for us. So I'll postpone resubmitting the patches until that's confirmed. > > Again, let's change the macro without removing the existing get()...put() > users and then remove those as a patch+review on a per-case basis. > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -2025,6 +2025,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) > > > > spin_lock_init(&dev_priv->irq_lock); > > spin_lock_init(&dev_priv->error_lock); > > + if (IS_GEN6(dev)) > > + spin_lock_init(&dev_priv->forcewake_lock); > > Just do it. Once again, I went back and forth and picked the one I thought you would like. I'm learning... > -Chris