Re: [PATCH 4/6] drm/i915: Serialize all register access

From: Ben Widawsky <ben@bwidawsk.net>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/6] drm/i915: Serialize all register access
Date: Fri, 12 Jul 2013 13:37:28 -0700	[thread overview]
Message-ID: <20130712203728.GI15384@bwidawsk.net> (raw)
In-Reply-To: <1373637756-11852-4-git-send-email-chris@chris-wilson.co.uk>

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 <chris@chris-wilson.co.uk>

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