[PATCH 0/5] aliasing ppgtt, advanced things

[PATCH 0/5] aliasing ppgtt, advanced things

[Daniel Vetter]

Hi all,

This patches here make bindings to the global gtt optional and are
hence preparation work for real per-process gtts. They also have the
benefits that we have fewer changes of randomly scribbling over
things.

The funky part here is the snb workaround, but we have a testcase for
that. And the workaround is indeed necessary because I've fumbled the
relocation domains in the test such that the workaround did not kick
in. And the test broke. But with the fixed up test everything works as
advertised.

The last patch also announces ppgtt hw support to userspace. This is
useful because certian MI instructions only work correctly with ppgtt
enabled (currently we don't use these for obvious reasons).

I'd like to merge this after qa has sufficiently beaten up the current
basic aliasing ppgtt implementation (and presuming nothing blows up
there).

Review, flames and comments highly welcome.

Yours, Daniel

Daniel Vetter (5):
  drm/i915: split out dma mapping from global gtt bind/unbind functions
  drm/i915: bind objects to the global gtt only when needed
  drm/i915: implement SNB workaround for lazy global gtt
  drm/i915: enable lazy global-gtt binding
  drm/i915: add HAS_ALIASING_PPGTT parameter for userspace

 drivers/gpu/drm/i915/i915_dma.c            |    3 ++
 drivers/gpu/drm/i915/i915_drv.h            |    6 ++-
 drivers/gpu/drm/i915/i915_gem.c            |   18 ++++++++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   15 ++++++++-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |   49 ++++++++++++----------------
 drivers/gpu/drm/i915/i915_irq.c            |    3 +-
 include/drm/i915_drm.h                     |    3 +-
 7 files changed, 61 insertions(+), 36 deletions(-)

-- 
1.7.9

[PATCH 1/5] drm/i915: split out dma mapping from global gtt bind/unbind functions

[Daniel Vetter]

Note that there's a functional change buried in this patch wrt the ilk
dmar workaround: We now only idle the gpu while tearing down the dmar
mappings, not while clearing the gtt. Keeping the current semantics
would have made for some really ugly code and afaik the issue is only
with the dmar unmapping that needs a fully idle gpu.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h     |    5 ++-
 drivers/gpu/drm/i915/i915_gem.c     |    6 +++-
 drivers/gpu/drm/i915/i915_gem_gtt.c |   45 +++++++++++++---------------------
 3 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b839728..f266acd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1283,10 +1283,11 @@ void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
 			      struct drm_i915_gem_object *obj);
 
 void i915_gem_restore_gtt_mappings(struct drm_device *dev);
-int __must_check i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj);
-void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
+int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
+void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
 				enum i915_cache_level cache_level);
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
+void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
 
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 19a06c2..f37527c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2107,6 +2107,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 		i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj);
 		obj->has_aliasing_ppgtt_mapping = 0;
 	}
+	i915_gem_gtt_finish_object(obj);
 
 	i915_gem_object_put_pages_gtt(obj);
 
@@ -2751,7 +2752,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 		return ret;
 	}
 
-	ret = i915_gem_gtt_bind_object(obj);
+	ret = i915_gem_gtt_prepare_object(obj);
 	if (ret) {
 		i915_gem_object_put_pages_gtt(obj);
 		drm_mm_put_block(obj->gtt_space);
@@ -2762,6 +2763,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 
 		goto search_free;
 	}
+	i915_gem_gtt_bind_object(obj, obj->cache_level);
 
 	list_add_tail(&obj->gtt_list, &dev_priv->mm.gtt_list);
 	list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
@@ -2955,7 +2957,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				return ret;
 		}
 
-		i915_gem_gtt_rebind_object(obj, cache_level);
+		i915_gem_gtt_bind_object(obj, cache_level);
 		if (obj->has_aliasing_ppgtt_mapping)
 			i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt,
 					       obj, cache_level);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2eacd78..bf33eaf 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -355,42 +355,28 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 
 	list_for_each_entry(obj, &dev_priv->mm.gtt_list, gtt_list) {
 		i915_gem_clflush_object(obj);
-		i915_gem_gtt_rebind_object(obj, obj->cache_level);
+		i915_gem_gtt_bind_object(obj, obj->cache_level);
 	}
 
 	intel_gtt_chipset_flush();
 }
 
-int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
+int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
-	int ret;
 
-	if (dev_priv->mm.gtt->needs_dmar) {
-		ret = intel_gtt_map_memory(obj->pages,
-					   obj->base.size >> PAGE_SHIFT,
-					   &obj->sg_list,
-					   &obj->num_sg);
-		if (ret != 0)
-			return ret;
-
-		intel_gtt_insert_sg_entries(obj->sg_list,
-					    obj->num_sg,
-					    obj->gtt_space->start >> PAGE_SHIFT,
-					    agp_type);
-	} else
-		intel_gtt_insert_pages(obj->gtt_space->start >> PAGE_SHIFT,
-				       obj->base.size >> PAGE_SHIFT,
-				       obj->pages,
-				       agp_type);
-
-	return 0;
+	if (dev_priv->mm.gtt->needs_dmar)
+		return intel_gtt_map_memory(obj->pages,
+					    obj->base.size >> PAGE_SHIFT,
+					    &obj->sg_list,
+					    &obj->num_sg);
+	else
+		return 0;
 }
 
-void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
-				enum i915_cache_level cache_level)
+void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
+			      enum i915_cache_level cache_level)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -412,15 +398,18 @@ void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
 
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 {
+	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
+			      obj->base.size >> PAGE_SHIFT);
+}
+
+void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
+{
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool interruptible;
 
 	interruptible = do_idling(dev_priv);
 
-	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
-			      obj->base.size >> PAGE_SHIFT);
-
 	if (obj->sg_list) {
 		intel_gtt_unmap_memory(obj->sg_list, obj->num_sg);
 		obj->sg_list = NULL;
-- 
1.7.9

[PATCH 2/5] drm/i915: bind objects to the global gtt only when needed

[Daniel Vetter]

And track the existence of such a binding similar to the aliasing
ppgtt case. Speeds up binding/unbinding in the common case where we
only need a ppgtt binding (which is accessed in a cpu coherent fashion
by the gpu) and no gloabl gtt binding (which needs uc writes for the
ptes).

This patch just puts the required tracking in place.

v2: Check that global gtt mappings exist in the error_state capture
code (with Chris Wilson's llc reloc patches batchbuffers are no longer
relocated as mappable in all situations, so this matters). Suggested
by Chris Wilson.

v3: Adapted to Chris' latest llc-reloc patches.

v4: Fix a bug in the i915 error state capture code noticed by Chris
Wilson.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h     |    1 +
 drivers/gpu/drm/i915/i915_gem.c     |   12 ++++++++++--
 drivers/gpu/drm/i915/i915_gem_gtt.c |    4 ++++
 drivers/gpu/drm/i915/i915_irq.c     |    3 ++-
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f266acd..5b3ee66 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -874,6 +874,7 @@ struct drm_i915_gem_object {
 	unsigned int cache_level:2;
 
 	unsigned int has_aliasing_ppgtt_mapping:1;
+	unsigned int has_global_gtt_mapping:1;
 
 	struct page **pages;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f37527c..5892dac 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1153,6 +1153,9 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 			goto unlock;
 	}
 
+	if (!obj->has_global_gtt_mapping)
+		i915_gem_gtt_bind_object(obj, obj->cache_level);
+
 	if (obj->tiling_mode == I915_TILING_NONE)
 		ret = i915_gem_object_put_fence(obj);
 	else
@@ -2102,7 +2105,8 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 
 	trace_i915_gem_object_unbind(obj);
 
-	i915_gem_gtt_unbind_object(obj);
+	if (obj->has_global_gtt_mapping)
+		i915_gem_gtt_unbind_object(obj);
 	if (obj->has_aliasing_ppgtt_mapping) {
 		i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj);
 		obj->has_aliasing_ppgtt_mapping = 0;
@@ -2957,7 +2961,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 				return ret;
 		}
 
-		i915_gem_gtt_bind_object(obj, cache_level);
+		if (obj->has_global_gtt_mapping)
+			i915_gem_gtt_bind_object(obj, cache_level);
 		if (obj->has_aliasing_ppgtt_mapping)
 			i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt,
 					       obj, cache_level);
@@ -3344,6 +3349,9 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
+	if (!obj->has_global_gtt_mapping && map_and_fenceable)
+		i915_gem_gtt_bind_object(obj, obj->cache_level);
+
 	if (obj->pin_count++ == 0) {
 		if (!obj->active)
 			list_move_tail(&obj->mm_list,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index bf33eaf..72be806 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -394,12 +394,16 @@ void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
 				       obj->base.size >> PAGE_SHIFT,
 				       obj->pages,
 				       agp_type);
+
+	obj->has_global_gtt_mapping = 1;
 }
 
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 {
 	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
 			      obj->base.size >> PAGE_SHIFT);
+
+	obj->has_global_gtt_mapping = 0;
 }
 
 void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index afd4e03..da95eb5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -727,7 +727,8 @@ i915_error_object_create(struct drm_i915_private *dev_priv,
 			goto unwind;
 
 		local_irq_save(flags);
-		if (reloc_offset < dev_priv->mm.gtt_mappable_end) {
+		if (reloc_offset < dev_priv->mm.gtt_mappable_end &&
+		    src->has_global_gtt_mapping) {
 			void __iomem *s;
 
 			/* Simply ignore tiling or any overlapping fence.
-- 
1.7.9

[PATCH 3/5] drm/i915: implement SNB workaround for lazy global gtt

[Daniel Vetter]

PIPE_CONTROL on snb needs global gtt mappings in place to workaround a
hw gotcha. No other commands need such a workaround. Luckily we can
detect a PIPE_CONTROL commands easily because they have a write_domain
= I915_GEM_DOMAIN_INSTRUCTION (and nothing else has that).

v2: Binding the target of such a reloc into the global gtt actually
works instead of binding the source, which is rather pointless ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 81687af..f8cabf2 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -273,6 +273,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_gem_object *target_obj;
+	struct drm_i915_gem_object *target_i915_obj;
 	uint32_t target_offset;
 	int ret = -EINVAL;
 
@@ -281,7 +282,8 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	if (unlikely(target_obj == NULL))
 		return -ENOENT;
 
-	target_offset = to_intel_bo(target_obj)->gtt_offset;
+	target_i915_obj = to_intel_bo(target_obj);
+	target_offset = target_i915_obj->gtt_offset;
 
 	/* The target buffer should have appeared before us in the
 	 * exec_object list, so it should have a GTT space bound by now.
@@ -383,6 +385,17 @@ 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);
+		target_i915_obj->has_global_gtt_mapping = 1;
+	}
+
 	/* and update the user's relocation entry */
 	reloc->presumed_offset = target_offset;
 
-- 
1.7.9

[PATCH 4/5] drm/i915: enable lazy global-gtt binding

[Daniel Vetter]

Now that everything is in place, only bind to the global gtt
when actually required. Patch split-up suggested by Chris Wilson.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5892dac..39b7eaf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2767,7 +2767,9 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 
 		goto search_free;
 	}
-	i915_gem_gtt_bind_object(obj, obj->cache_level);
+
+	if (!dev_priv->mm.aliasing_ppgtt)
+		i915_gem_gtt_bind_object(obj, obj->cache_level);
 
 	list_add_tail(&obj->gtt_list, &dev_priv->mm.gtt_list);
 	list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
-- 
1.7.9

[PATCH 5/5] drm/i915: add HAS_ALIASING_PPGTT parameter for userspace

[Daniel Vetter]

On Sanybridge a few MI read/write commands only work when ppgtt is
enabled.  Userspace therefore needs to be able to check whether ppgtt
is enabled. For added hilarity, you need to reset the "use global GTT"
bit on snb when ppgtt is enabled, otherwise it won't work.  Despite
what bspec says about automatically using ppgtt ...

Luckily PIPE_CONTROL (the only write cmd current userspace uses) is
not affected by all this, as tested by tests/gem_pipe_control_store_loop.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_dma.c |    3 +++
 include/drm/i915_drm.h          |    3 ++-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 39728fb..27f018e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -787,6 +787,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_LLC:
 		value = HAS_LLC(dev);
 		break;
+	case I915_PARAM_HAS_ALIASING_PPGTT:
+		value = dev_priv->mm.aliasing_ppgtt ? 1 : 0;
+		break;
 	default:
 		DRM_DEBUG_DRIVER("Unknown parameter %d\n",
 				 param->param);
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index da929bb..f3f8224 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -296,7 +296,8 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_EXEC_CONSTANTS	 14
 #define I915_PARAM_HAS_RELAXED_DELTA	 15
 #define I915_PARAM_HAS_GEN7_SOL_RESET	 16
-#define I915_PARAM_HAS_LLC     	 17
+#define I915_PARAM_HAS_LLC     	 	 17
+#define I915_PARAM_HAS_ALIASING_PPGTT	 18
 
 typedef struct drm_i915_getparam {
 	int param;
-- 
1.7.9

Re: [PATCH 3/5] drm/i915: implement SNB workaround for lazy global gtt

[Chris Wilson]

On Wed, 15 Feb 2012 23:50:23 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> +	/* 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);
> +		target_i915_obj->has_global_gtt_mapping = 1;

i915_gem_gtt_bind_object() sets has_global_gtt_mapping, so no need to
repeat ourselves here.

I guess that was the easy one you throw in to make sure people are
reading your patches?

A little uneasy still with that heuristic, but I have to agree that it
is the lesser of the evils, meh.

I'm pretty happy now with this series as I've been beating upon it ever
since it landed in danvet/my-next, so
Reviewed-and-tested-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915: implement SNB workaround for lazy global gtt

[Daniel Vetter]

On Wed, Feb 15, 2012 at 11:10:08PM +0000, Chris Wilson wrote:
> On Wed, 15 Feb 2012 23:50:23 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > +	/* 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);
> > +		target_i915_obj->has_global_gtt_mapping = 1;
> 
> i915_gem_gtt_bind_object() sets has_global_gtt_mapping, so no need to
> repeat ourselves here.

Indeed, that's superflous. I'll kill this line when applying it (the
machine with the patches is already down and I'm a lazy bastard ...).

> I guess that was the easy one you throw in to make sure people are
> reading your patches?
> 
> A little uneasy still with that heuristic, but I have to agree that it
> is the lesser of the evils, meh.

Luckily pipe_control writes on snb are extremely broken with ppgtt, so we
should notice immediately if someone sets the wrong domain (as I did in
the broken i-g-t test).

> I'm pretty happy now with this series as I've been beating upon it ever
> since it landed in danvet/my-next, so
> Reviewed-and-tested-by: Chris Wilson <chris@chris-wilson.co.uk>

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

Re: [PATCH 3/5] drm/i915: implement SNB workaround for lazy global gtt

[Daniel Vetter]

On Wed, Feb 15, 2012 at 11:10:08PM +0000, Chris Wilson wrote:
> On Wed, 15 Feb 2012 23:50:23 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > +	/* 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);
> > +		target_i915_obj->has_global_gtt_mapping = 1;
> 
> i915_gem_gtt_bind_object() sets has_global_gtt_mapping, so no need to
> repeat ourselves here.
> 
> I guess that was the easy one you throw in to make sure people are
> reading your patches?
> 
> A little uneasy still with that heuristic, but I have to agree that it
> is the lesser of the evils, meh.
> 
> I'm pretty happy now with this series as I've been beating upon it ever
> since it landed in danvet/my-next, so
> Reviewed-and-tested-by: Chris Wilson <chris@chris-wilson.co.uk>

I've slurped this in for -next (with the small thing you've noticed in
this patch fixed), thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48