From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 4/6] drm/i915: Serialize all register access Date: Fri, 12 Jul 2013 13:37:28 -0700 Message-ID: <20130712203728.GI15384@bwidawsk.net> References: <1373637756-11852-1-git-send-email-chris@chris-wilson.co.uk> <1373637756-11852-4-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.localdomain (unknown [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 6F037E5EE4 for ; Fri, 12 Jul 2013 13:37:31 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1373637756-11852-4-git-send-email-chris@chris-wilson.co.uk> 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 Fri, Jul 12, 2013 at 03:02:34PM +0100, Chris Wilson wrote: > In theory, the different register blocks were meant to be only ever > touched when holding either the struct_mutex, mode_config.lock or even a > specific localised lock. This does not seem to be the case, and the > hardware reacts extremely badly if we attempt to concurrently access two > registers within the same cacheline. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=63914 > Signed-off-by: Chris Wilson You are crossing the point of no return here for doing this only on HSW where the bug is known to exist. As you are the resident performance curmudgeon I'll defer to you if that's okay or not... just pointing it out. > --- > drivers/gpu/drm/i915/intel_gt.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_gt.c b/drivers/gpu/drm/i915/intel_gt.c > index d4bc7f4..e89e901 100644 > --- a/drivers/gpu/drm/i915/intel_gt.c > +++ b/drivers/gpu/drm/i915/intel_gt.c > @@ -331,21 +331,21 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg) > > #define __i915_read(x, y) \ > u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg, bool trace) { \ > + unsigned long irqflags; \ > u##x val = 0; \ > + spin_lock_irqsave(&dev_priv->gt_lock, irqflags); \ > if (IS_GEN5(dev_priv->dev)) \ > ilk_dummy_write(dev_priv); \ > if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > - unsigned long irqflags; \ > - spin_lock_irqsave(&dev_priv->gt_lock, irqflags); \ > if (dev_priv->forcewake_count == 0) \ > dev_priv->gt.force_wake_get(dev_priv); \ > val = read##y(dev_priv->regs + reg); \ > if (dev_priv->forcewake_count == 0) \ > dev_priv->gt.force_wake_put(dev_priv); \ > - spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags); \ > } else { \ > val = read##y(dev_priv->regs + reg); \ > } \ > + spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags); \ > if (trace) trace_i915_reg_rw(false, reg, val, sizeof(val)); \ > return val; \ > } > @@ -358,8 +358,10 @@ __i915_read(64, q) > > #define __i915_write(x, y) \ > void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val, bool trace) { \ > + unsigned long irqflags; \ > u32 __fifo_ret = 0; \ > if (trace) trace_i915_reg_rw(true, reg, val, sizeof(val)); \ > + spin_lock_irqsave(&dev_priv->gt_lock, irqflags); \ > if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > __fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \ > } \ > @@ -371,6 +373,7 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val, bool tr > gen6_gt_check_fifodbg(dev_priv); \ > } \ > hsw_unclaimed_reg_check(dev_priv, reg); \ > + spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags); \ > } > __i915_write(8, b) > __i915_write(16, w) > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center