[PATCH] drm/i915: Workaround incoherence with fence updates on Valleyview

[PATCH] drm/i915: Workaround incoherence with fence updates on Valleyview

[Chris Wilson]

In commit 25ff1195f8a0b3724541ae7bbe331b4296de9c06
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Apr 4 21:31:03 2013 +0100

    drm/i915: Workaround incoherence between fences and LLC across multiple CPUs

we introduced an empirical workaround for memory corruption when using
fences from multiple CPUs. At the time, we did not have any results for
Valleyview, so the presumption was that it was limited to recent
generations using LLC. Now we have evidence that Valleyview also suffers
incoherence and requires a similar but different workaround. For
Valleyview, the wbinvd instruction is insufficient and we require the
serialising register write per-CPU. Conversely, that serialising
register write is not enough for SNB/IVB/HSW. To compromise and keep the
code relatively clean, employ both serialisation techniques in the same
workaround.

Reported-by: Jon Bloomfield <jon.bloomfield@intel.com>
Tested-by: Jon Bloomfield <jon.bloomfield@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=62191
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 38c8f11..0c6f0bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2711,18 +2711,33 @@ static inline int fence_number(struct drm_i915_private *dev_priv,
 	return fence - dev_priv->fence_regs;
 }
 
+struct write_fence {
+	struct drm_device *dev;
+	struct drm_i915_gem_object *obj;
+	int fence;
+};
+
 static void i915_gem_write_fence__ipi(void *data)
 {
+	struct write_fence *args = data;
+
+	/* Required for SNB+ with LLC */
 	wbinvd();
+
+	/* Required for VLV */
+	i915_gem_write_fence(args->dev, args->fence, args->obj);
 }
 
 static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj,
 					 struct drm_i915_fence_reg *fence,
 					 bool enable)
 {
-	struct drm_device *dev = obj->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int fence_reg = fence_number(dev_priv, fence);
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+	struct write_fence args = {
+		.dev = obj->base.dev,
+		.fence = fence_number(dev_priv, fence),
+		.obj = enable ? obj : NULL,
+	};
 
 	/* In order to fully serialize access to the fenced region and
 	 * the update to the fence register we need to take extreme
@@ -2733,13 +2748,19 @@ static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj,
 	 * SNB+ we need to take a step further and emit an explicit wbinvd()
 	 * on each processor in order to manually flush all memory
 	 * transactions before updating the fence register.
+	 *
+	 * However, Valleyview complicates matter. There the wbinvd is
+	 * insufficient and unlike SNB/IVB requires the serialising
+	 * register write. (Note that that register write by itself is
+	 * conversely not sufficient for SNB+.) To compromise, we do both.
 	 */
-	if (HAS_LLC(obj->base.dev))
-		on_each_cpu(i915_gem_write_fence__ipi, NULL, 1);
-	i915_gem_write_fence(dev, fence_reg, enable ? obj : NULL);
+	if (INTEL_INFO(args.dev)->gen >= 6)
+		on_each_cpu(i915_gem_write_fence__ipi, &args, 1);
+	else
+		i915_gem_write_fence(args.dev, args.fence, args.obj);
 
 	if (enable) {
-		obj->fence_reg = fence_reg;
+		obj->fence_reg = args.fence;
 		fence->obj = obj;
 		list_move_tail(&fence->lru_list, &dev_priv->mm.fence_list);
 	} else {
-- 
1.7.10.4

Re: [PATCH] drm/i915: Workaround incoherence with fence updates on Valleyview

[Daniel Vetter]

On Wed, May 22, 2013 at 05:08:06PM +0100, Chris Wilson wrote:
> In commit 25ff1195f8a0b3724541ae7bbe331b4296de9c06
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Apr 4 21:31:03 2013 +0100
> 
>     drm/i915: Workaround incoherence between fences and LLC across multiple CPUs
> 
> we introduced an empirical workaround for memory corruption when using
> fences from multiple CPUs. At the time, we did not have any results for
> Valleyview, so the presumption was that it was limited to recent
> generations using LLC. Now we have evidence that Valleyview also suffers
> incoherence and requires a similar but different workaround. For
> Valleyview, the wbinvd instruction is insufficient and we require the
> serialising register write per-CPU. Conversely, that serialising
> register write is not enough for SNB/IVB/HSW. To compromise and keep the
> code relatively clean, employ both serialisation techniques in the same
> workaround.
> 
> Reported-by: Jon Bloomfield <jon.bloomfield@intel.com>
> Tested-by: Jon Bloomfield <jon.bloomfield@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=62191
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next (since vlv isn't yet declared stable in 3.10), thanks for
the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch