[PATCH] drm/i915: Don't forget to apply SNB PIPE_CONTROL GTT workaround.

[PATCH] drm/i915: Don't forget to apply SNB PIPE_CONTROL GTT workaround.

[Eric Anholt]

If a buffer that was the target of a PIPE_CONTROL from userland was a
reused one that hadn't been evicted which had not previously had this
workaround applied, then we would not bind it into the GTT and the
write would land somewhere else.

Based on a doubting-my-sanity debugging session with cworth, I'm
pretty sure this will fix his reproducible GL_EXT_timer_query
failures, and hopefully the intermittent OQ issues on snb that
danvet's been working on.

I have not tested it yet, but hopefully when cworth gets home he will.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 25b2c54..afb312e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -117,6 +117,16 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	target_i915_obj = to_intel_bo(target_obj);
 	target_offset = target_i915_obj->gtt_offset;
 
+	/* Sandybridge PPGTT errata: We need a global gtt mapping for MI and
+	 * pipe_control writes because the gpu doesn't properly redirect them
+	 * through the ppgtt for non_secure batchbuffers. */
+	if (unlikely(IS_GEN6(dev) &&
+	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
+	    !target_i915_obj->has_global_gtt_mapping)) {
+		i915_gem_gtt_bind_object(target_i915_obj,
+					 target_i915_obj->cache_level);
+	}
+
 	/* The target buffer should have appeared before us in the
 	 * exec_object list, so it should have a GTT space bound by now.
 	 */
@@ -225,16 +235,6 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 		io_mapping_unmap_atomic(reloc_page);
 	}
 
-	/* Sandybridge PPGTT errata: We need a global gtt mapping for MI and
-	 * pipe_control writes because the gpu doesn't properly redirect them
-	 * through the ppgtt for non_secure batchbuffers. */
-	if (unlikely(IS_GEN6(dev) &&
-	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
-	    !target_i915_obj->has_global_gtt_mapping)) {
-		i915_gem_gtt_bind_object(target_i915_obj,
-					 target_i915_obj->cache_level);
-	}
-
 	/* and update the user's relocation entry */
 	reloc->presumed_offset = target_offset;
 
-- 
1.7.10.4

Re: [PATCH] drm/i915: Don't forget to apply SNB PIPE_CONTROL GTT workaround.

[Carl Worth]

[-- Attachment #1.1: Type: text/plain, Size: 277 bytes --]

Eric Anholt <eric@anholt.net> writes:
> I have not tested it yet, but hopefully when cworth gets home he will.

Reviewed-by: Carl Worth <cworth@cworth.org>
Tested-by: Carl Worth <cworth@cworth.org>

Thanks, Eric! This is perfect.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Don't forget to apply SNB PIPE_CONTROL GTT workaround.

[Chris Wilson]

On Mon, 30 Jul 2012 18:24:26 -0700, Eric Anholt <eric@anholt.net> wrote:
> If a buffer that was the target of a PIPE_CONTROL from userland was a
> reused one that hadn't been evicted which had not previously had this
> workaround applied, then we would not bind it into the GTT and the
> write would land somewhere else.
> 
> Based on a doubting-my-sanity debugging session with cworth, I'm
> pretty sure this will fix his reproducible GL_EXT_timer_query
> failures, and hopefully the intermittent OQ issues on snb that
> danvet's been working on.
> 
> I have not tested it yet, but hopefully when cworth gets home he will.

Had to look hard to find the secret ingredient. The reason why this
patch will work is that that in the middle of the error checking we have
an early success return after applying the new reloc domains iff the
object still holds the same GTT offset as presumed by the reloc.

Probably deserves a little more clarification in the changelog as to why
the patch works.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I think this should also fix
  https://bugs.freedesktop.org/show_bug.cgi?id=48019
but my snb has decided today is the day to hard hang instead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Don't forget to apply SNB PIPE_CONTROL GTT workaround.

[Eric Anholt]

If a buffer that was the target of a PIPE_CONTROL from userland was a
reused one that hadn't been evicted which had not previously had this
workaround applied, then the early return for a correct
presumed_offset in this function meant we would not bind it into the
GTT and the write would land somewhere else.

Fixes reproducible failures with GL_EXT_timer_query usage in apitrace,
and I also expect it to fix the intermittent OQ issues on snb that
danvet's been working on.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48019
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Carl Worth <cworth@cworth.org>
Tested-by: Carl Worth <cworth@cworth.org>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 25b2c54..afb312e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -117,6 +117,16 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	target_i915_obj = to_intel_bo(target_obj);
 	target_offset = target_i915_obj->gtt_offset;
 
+	/* Sandybridge PPGTT errata: We need a global gtt mapping for MI and
+	 * pipe_control writes because the gpu doesn't properly redirect them
+	 * through the ppgtt for non_secure batchbuffers. */
+	if (unlikely(IS_GEN6(dev) &&
+	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
+	    !target_i915_obj->has_global_gtt_mapping)) {
+		i915_gem_gtt_bind_object(target_i915_obj,
+					 target_i915_obj->cache_level);
+	}
+
 	/* The target buffer should have appeared before us in the
 	 * exec_object list, so it should have a GTT space bound by now.
 	 */
@@ -225,16 +235,6 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 		io_mapping_unmap_atomic(reloc_page);
 	}
 
-	/* Sandybridge PPGTT errata: We need a global gtt mapping for MI and
-	 * pipe_control writes because the gpu doesn't properly redirect them
-	 * through the ppgtt for non_secure batchbuffers. */
-	if (unlikely(IS_GEN6(dev) &&
-	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
-	    !target_i915_obj->has_global_gtt_mapping)) {
-		i915_gem_gtt_bind_object(target_i915_obj,
-					 target_i915_obj->cache_level);
-	}
-
 	/* and update the user's relocation entry */
 	reloc->presumed_offset = target_offset;
 
-- 
1.7.10.4

Re: [PATCH] drm/i915: Don't forget to apply SNB PIPE_CONTROL GTT workaround.

[Daniel Vetter]

On Tue, Jul 31, 2012 at 03:35:01PM -0700, Eric Anholt wrote:
> If a buffer that was the target of a PIPE_CONTROL from userland was a
> reused one that hadn't been evicted which had not previously had this
> workaround applied, then the early return for a correct
> presumed_offset in this function meant we would not bind it into the
> GTT and the write would land somewhere else.
> 
> Fixes reproducible failures with GL_EXT_timer_query usage in apitrace,
> and I also expect it to fix the intermittent OQ issues on snb that
> danvet's been working on.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48019
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Carl Worth <cworth@cworth.org>
> Tested-by: Carl Worth <cworth@cworth.org>

Picked up for -fixes, thanks for the patch. I've also added a bz line for
#52932 to the commit.

I should have noticed this while banging against this particular wall, I
guess I owe you a few beers for tracking it down ;-)

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48