[Intel-gfx] [PATCH 4/6] drm/i915: Serialize all register access

[Chris Wilson]

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.

The HSD suggests that we only need to do this workaround for display
range registers. However, upon review we need to serialize the multiple
stages in our register write functions - if only for preemption
protection.

v2: Rebase onto uncore
v3: Add wa reference, WaSerializeKmdDisplayAccess

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=63914
Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_uncore.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e787648..789a596 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -343,23 +343,31 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
 	}
 }
 
+/*
+ * Note that we need to serialise access to registers within the same
+ * cacheline - to do that we use a spinlock over all registers (or in the
+ * future perhaps register blocks).
+ *
+ * WaSerializeKmdDisplayAccess:hsw
+ */
+
 #define __i915_read(x) \
 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->uncore.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->uncore.lock, irqflags); \
 		if (dev_priv->uncore.forcewake_count == 0) \
 			dev_priv->uncore.funcs.force_wake_get(dev_priv); \
 		val = __raw_i915_read##x(dev_priv, reg); \
 		if (dev_priv->uncore.forcewake_count == 0) \
 			dev_priv->uncore.funcs.force_wake_put(dev_priv); \
-		spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); \
 	} else { \
 		val = __raw_i915_read##x(dev_priv, reg); \
 	} \
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); \
 	if (trace) trace_i915_reg_rw(false, reg, val, sizeof(val)); \
 	return val; \
 }
@@ -372,8 +380,10 @@ __i915_read(64)
 
 #define __i915_write(x) \
 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->uncore.lock, irqflags); \
 	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
 		__fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
 	} \
@@ -385,6 +395,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->uncore.lock, irqflags); \
 }
 __i915_write(8)
 __i915_write(16)
-- 
1.8.3.2