From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock Date: Sun, 06 Nov 2011 21:01:44 +0000 Message-ID: References: <1320582922-5618-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 997239E77A for ; Sun, 6 Nov 2011 13:01:51 -0800 (PST) In-Reply-To: <1320582922-5618-1-git-send-email-daniel.vetter@ffwll.ch> 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: intel-gfx Cc: Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org On Sun, 6 Nov 2011 13:35:22 +0100, Daniel Vetter wrote: > The problem this patch solves is that the forcewake accounting > necessary for register reads is protected by dev->struct_mutex. But the > hangcheck and error_capture code need to access registers without > grabbing this mutex because we hold it while waiting for the gpu. > So a new lock is required. Because currently the error_state capture > is called from the error irq handler and the hangcheck code runs from > a timer, it needs to be an irqsafe spinlock (note that the registers > used by the irq handler (neglecting the error handling part) only uses > registers that don't need the forcewake dance). > > We could tune this down to a normal spinlock when we rework the > error_state capture and hangcheck code to run from a workqueue. But > we don't have any read in a fastpath that needs forcewake, so I've > decided to not care much about overhead. > > This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my > snb on recent kernels - something must have slightly changed the > timings. On previous kernels it only trigger a WARN about the broken > locking. > > v2: Drop the previous patch for the register writes. > > v3: Improve the commit message per Chris Wilson's suggestions. > > Signed-Off-by: Daniel Vetter Reviewed-by: Chris Wilson One minor oddity left ;-) > --- > drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++++-- > drivers/gpu/drm/i915/i915_dma.c | 1 + > drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++------ > drivers/gpu/drm/i915/i915_drv.h | 10 +++++++--- > 4 files changed, 27 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 5ba63ad..51b21eb 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1320,9 +1320,13 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data) > struct drm_info_node *node = (struct drm_info_node *) m->private; > struct drm_device *dev = node->minor->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned forcewake_count; > > - seq_printf(m, "forcewake count = %d\n", > - atomic_read(&dev_priv->forcewake_count)); > + spin_lock_irq(&dev_priv->gt_lock); > + forcewake_count = dev_priv->forcewake_count; > + spin_unlock_irq(&dev_priv->gt_lock); > + > + seq_printf(m, "forcewake count = %d\n", forcewake_count); Is it signed or unsigned? Who cares? ;-) -Chris -- Chris Wilson, Intel Open Source Technology Centre