From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function Date: Thu, 13 Mar 2014 18:21:45 +0100 Message-ID: <20140313172145.GS30571@phenom.ffwll.local> References: <20140313115719.GB8998@nuc-i3427.alporthouse.com> <1394712029-715-1-git-send-email-chris@chris-wilson.co.uk> <877g7yp81h.fsf@gaia.fi.intel.com> <20140313131037.GC8998@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f44.google.com (mail-ee0-f44.google.com [74.125.83.44]) by gabe.freedesktop.org (Postfix) with ESMTP id D31A0FA8ED for ; Thu, 13 Mar 2014 10:21:49 -0700 (PDT) Received: by mail-ee0-f44.google.com with SMTP id e49so544807eek.31 for ; Thu, 13 Mar 2014 10:21:49 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140313131037.GC8998@nuc-i3427.alporthouse.com> 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 , Mika Kuoppala , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 13, 2014 at 01:10:37PM +0000, Chris Wilson wrote: > On Thu, Mar 13, 2014 at 03:00:58PM +0200, Mika Kuoppala wrote: > > Chris Wilson writes: > > > @@ -957,13 +986,6 @@ static int gen6_do_reset(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > int ret; > > > - unsigned long irqflags; > > > - u32 fw_engine = 0; > > > - > > > - /* Hold uncore.lock across reset to prevent any register access > > > - * with forcewake not set correctly > > > - */ > > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > > > /* Reset the chip */ > > > > > > @@ -976,29 +998,8 @@ static int gen6_do_reset(struct drm_device *dev) > > > /* Spin waiting for the device to ack the reset request */ > > > ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500); > > > > We used to go through the reset with uncore lock held, > > which is not the case anymore. Will it create problems? > > I don't think so. gen6_do_reset() is itself serialised and there should > be no other access to GDRST anyway, so from that perspective we won't > trigger hw errors. So the only thing that the uncore lock was protecting > in this function is the forcewake dance. Yes, protecting the forcewake dance was the only reason to hold the uncore lock over gen6 reset. As long as we hold the uncore lock while restoring any forcewake references after reset we're good, no need to shove the entire reset under that spinlock. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch