i915 llc for -next

i915 llc for -next

[Chris Wilson]

As nobody objected to, as expected, the modesetting refactor and the
fixes Ben did for rc6 on Ironlake, I've pushed those on to the
drm-intel-next branch.

This patchset just contains the LLC material as it has seen a lot of
changes yesterday and I'd like to review the new material in context.

Thanks in the advance for any feedback,
-Chris

[PATCH 01/13] drm/i915: Rename agp_type to cache_level

[Chris Wilson]

... to clarify just how we use it inside the driver. We still need to
translate through agp_type for interface into the fake AGP driver.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |   11 ++++++-----
 drivers/gpu/drm/i915/i915_drv.h         |   12 +++++++++---
 drivers/gpu/drm/i915/i915_gem.c         |    2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c     |   30 ++++++++++++++++++++++++------
 drivers/gpu/drm/i915/i915_irq.c         |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    4 ++--
 6 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 289dcbc..52d2306 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -106,11 +106,12 @@ static const char *get_tiling_flag(struct drm_i915_gem_object *obj)
     }
 }
 
-static const char *agp_type_str(int type)
+static const char *cache_level_str(int type)
 {
 	switch (type) {
-	case 0: return " uncached";
-	case 1: return " snooped";
+	case I915_CACHE_NONE: return " uncached";
+	case I915_CACHE_LLC: return " snooped (LLC)";
+	case I915_CACHE_LLC_MLC: return " snooped (LLC+MLC)";
 	default: return "";
 	}
 }
@@ -127,7 +128,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		   obj->base.write_domain,
 		   obj->last_rendering_seqno,
 		   obj->last_fenced_seqno,
-		   agp_type_str(obj->agp_type == AGP_USER_CACHED_MEMORY),
+		   cache_level_str(obj->cache_level),
 		   obj->dirty ? " dirty" : "",
 		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
 	if (obj->base.name)
@@ -714,7 +715,7 @@ static void print_error_buffers(struct seq_file *m,
 			   dirty_flag(err->dirty),
 			   purgeable_flag(err->purgeable),
 			   ring_str(err->ring),
-			   agp_type_str(err->agp_type));
+			   cache_level_str(err->cache_level));
 
 		if (err->name)
 			seq_printf(m, " (name: %d)", err->name);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7ee0ac8..2536334 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -188,7 +188,7 @@ struct drm_i915_error_state {
 		u32 dirty:1;
 		u32 purgeable:1;
 		u32 ring:4;
-		u32 agp_type:1;
+		u32 cache_level:2;
 	} *active_bo, *pinned_bo;
 	u32 active_bo_count, pinned_bo_count;
 	struct intel_overlay_error_state *overlay;
@@ -711,6 +711,12 @@ typedef struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 } drm_i915_private_t;
 
+enum i915_cache_level {
+	I915_CACHE_NONE,
+	I915_CACHE_LLC,
+	I915_CACHE_LLC_MLC, /* gen6+ */
+};
+
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
 
@@ -797,6 +803,8 @@ struct drm_i915_gem_object {
 	unsigned int pending_fenced_gpu_access:1;
 	unsigned int fenced_gpu_access:1;
 
+	unsigned int cache_level:2;
+
 	struct page **pages;
 
 	/**
@@ -833,8 +841,6 @@ struct drm_i915_gem_object {
 	/** Record of address bit 17 of each page at last unbind. */
 	unsigned long *bit_17;
 
-	/** AGP mapping type (AGP_USER_MEMORY or AGP_USER_CACHED_MEMORY */
-	uint32_t agp_type;
 
 	/**
 	 * If present, while GEM_DOMAIN_CPU is in the read domain this array
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7ce3f35..264bec8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3569,7 +3569,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
 
-	obj->agp_type = AGP_USER_MEMORY;
+	obj->cache_level = I915_CACHE_NONE;
 	obj->base.driver_private = NULL;
 	obj->fence_reg = I915_FENCE_REG_NONE;
 	INIT_LIST_HEAD(&obj->mm_list);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b0abdc6..2a1f8f1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -29,6 +29,22 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+/* XXX kill agp_type! */
+static uint32_t cache_level_to_agp_type(struct drm_device *dev,
+					enum i915_cache_level cache_level)
+{
+	switch (cache_level) {
+	case I915_CACHE_LLC_MLC:
+		if (INTEL_INFO(dev)->gen >= 6)
+			return AGP_USER_CACHED_MEMORY_LLC_MLC;
+	case I915_CACHE_LLC:
+		return AGP_USER_CACHED_MEMORY;
+	default:
+	case I915_CACHE_NONE:
+		return AGP_USER_MEMORY;
+	}
+}
+
 void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -39,6 +55,8 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 			      (dev_priv->mm.gtt_end - dev_priv->mm.gtt_start) / PAGE_SIZE);
 
 	list_for_each_entry(obj, &dev_priv->mm.gtt_list, gtt_list) {
+		int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
+
 		i915_gem_clflush_object(obj);
 
 		if (dev_priv->mm.gtt->needs_dmar) {
@@ -46,15 +64,14 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 
 			intel_gtt_insert_sg_entries(obj->sg_list,
 						    obj->num_sg,
-						    obj->gtt_space->start
-							>> PAGE_SHIFT,
-						    obj->agp_type);
+						    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,
-					       obj->agp_type);
+					       agp_type);
 	}
 
 	intel_gtt_chipset_flush();
@@ -64,6 +81,7 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
 	int ret;
 
 	if (dev_priv->mm.gtt->needs_dmar) {
@@ -77,12 +95,12 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
 		intel_gtt_insert_sg_entries(obj->sg_list,
 					    obj->num_sg,
 					    obj->gtt_space->start >> PAGE_SHIFT,
-					    obj->agp_type);
+					    agp_type);
 	} else
 		intel_gtt_insert_pages(obj->gtt_space->start >> PAGE_SHIFT,
 				       obj->base.size >> PAGE_SHIFT,
 				       obj->pages,
-				       obj->agp_type);
+				       agp_type);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 188b497..5c0466e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -676,7 +676,7 @@ static u32 capture_bo_list(struct drm_i915_error_buffer *err,
 		err->dirty = obj->dirty;
 		err->purgeable = obj->madv != I915_MADV_WILLNEED;
 		err->ring = obj->ring ? obj->ring->id : 0;
-		err->agp_type = obj->agp_type == AGP_USER_CACHED_MEMORY;
+		err->cache_level = obj->cache_level;
 
 		if (++i == count)
 			break;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 714f1a7..eab2565 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -236,7 +236,7 @@ init_pipe_control(struct intel_ring_buffer *ring)
 		ret = -ENOMEM;
 		goto err;
 	}
-	obj->agp_type = AGP_USER_CACHED_MEMORY;
+	obj->cache_level = I915_CACHE_LLC;
 
 	ret = i915_gem_object_pin(obj, 4096, true);
 	if (ret)
@@ -759,7 +759,7 @@ static int init_status_page(struct intel_ring_buffer *ring)
 		ret = -ENOMEM;
 		goto err;
 	}
-	obj->agp_type = AGP_USER_CACHED_MEMORY;
+	obj->cache_level = I915_CACHE_LLC;
 
 	ret = i915_gem_object_pin(obj, 4096, true);
 	if (ret != 0) {
-- 
1.7.4.1

Re: [PATCH 01/13] drm/i915: Rename agp_type to cache_level

[Keith Packard]

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

On Thu, 14 Apr 2011 10:03:35 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> ... to clarify just how we use it inside the driver. We still need to
> translate through agp_type for interface into the fake AGP driver.

agp_type has some really confusing semantics...

> -	obj->agp_type = AGP_USER_MEMORY;
> +	obj->cache_level = I915_CACHE_NONE;

AGP_USER_MEMORY is indeed uncached, so this is correct.


> +/* XXX kill agp_type! */
> +static uint32_t cache_level_to_agp_type(struct drm_device *dev,
> +					enum i915_cache_level cache_level)

This should be unsigned int to match the users of this value

> +	switch (cache_level) {
> +	case I915_CACHE_LLC_MLC:
> +		if (INTEL_INFO(dev)->gen >= 6)
> +			return AGP_USER_CACHED_MEMORY_LLC_MLC;

I like to see a comment here:

+               /* Fall through ... */

> +	case I915_CACHE_LLC:
> +		return AGP_USER_CACHED_MEMORY;
> +	default:
> +	case I915_CACHE_NONE:
> +		return AGP_USER_MEMORY;
> +	}

>  	list_for_each_entry(obj, &dev_priv->mm.gtt_list, gtt_list) {
> +		int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
> +

This should be 'unsigned int' to match the signature of
intel_gtt_insert_sg_entries and intel_gtt_insert_pages.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

[PATCH] drm/i915: Rename agp_type to cache_level

[Chris Wilson]

... to clarify just how we use it inside the driver and remove the
confusion of the poorly matching agp_type names. We still need to
translate through agp_type for interface into the fake AGP driver.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c     |   11 +++++----
 drivers/gpu/drm/i915/i915_drv.h         |   12 ++++++++--
 drivers/gpu/drm/i915/i915_gem.c         |    2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c     |   35 +++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_irq.c         |    2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c |    4 +-
 6 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 289dcbc..52d2306 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -106,11 +106,12 @@ static const char *get_tiling_flag(struct drm_i915_gem_object *obj)
     }
 }
 
-static const char *agp_type_str(int type)
+static const char *cache_level_str(int type)
 {
 	switch (type) {
-	case 0: return " uncached";
-	case 1: return " snooped";
+	case I915_CACHE_NONE: return " uncached";
+	case I915_CACHE_LLC: return " snooped (LLC)";
+	case I915_CACHE_LLC_MLC: return " snooped (LLC+MLC)";
 	default: return "";
 	}
 }
@@ -127,7 +128,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		   obj->base.write_domain,
 		   obj->last_rendering_seqno,
 		   obj->last_fenced_seqno,
-		   agp_type_str(obj->agp_type == AGP_USER_CACHED_MEMORY),
+		   cache_level_str(obj->cache_level),
 		   obj->dirty ? " dirty" : "",
 		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
 	if (obj->base.name)
@@ -714,7 +715,7 @@ static void print_error_buffers(struct seq_file *m,
 			   dirty_flag(err->dirty),
 			   purgeable_flag(err->purgeable),
 			   ring_str(err->ring),
-			   agp_type_str(err->agp_type));
+			   cache_level_str(err->cache_level));
 
 		if (err->name)
 			seq_printf(m, " (name: %d)", err->name);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7ee0ac8..2536334 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -188,7 +188,7 @@ struct drm_i915_error_state {
 		u32 dirty:1;
 		u32 purgeable:1;
 		u32 ring:4;
-		u32 agp_type:1;
+		u32 cache_level:2;
 	} *active_bo, *pinned_bo;
 	u32 active_bo_count, pinned_bo_count;
 	struct intel_overlay_error_state *overlay;
@@ -711,6 +711,12 @@ typedef struct drm_i915_private {
 	struct drm_property *broadcast_rgb_property;
 } drm_i915_private_t;
 
+enum i915_cache_level {
+	I915_CACHE_NONE,
+	I915_CACHE_LLC,
+	I915_CACHE_LLC_MLC, /* gen6+ */
+};
+
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
 
@@ -797,6 +803,8 @@ struct drm_i915_gem_object {
 	unsigned int pending_fenced_gpu_access:1;
 	unsigned int fenced_gpu_access:1;
 
+	unsigned int cache_level:2;
+
 	struct page **pages;
 
 	/**
@@ -833,8 +841,6 @@ struct drm_i915_gem_object {
 	/** Record of address bit 17 of each page at last unbind. */
 	unsigned long *bit_17;
 
-	/** AGP mapping type (AGP_USER_MEMORY or AGP_USER_CACHED_MEMORY */
-	uint32_t agp_type;
 
 	/**
 	 * If present, while GEM_DOMAIN_CPU is in the read domain this array
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7ce3f35..264bec8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3569,7 +3569,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
 
-	obj->agp_type = AGP_USER_MEMORY;
+	obj->cache_level = I915_CACHE_NONE;
 	obj->base.driver_private = NULL;
 	obj->fence_reg = I915_FENCE_REG_NONE;
 	INIT_LIST_HEAD(&obj->mm_list);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b0abdc6..e46b645 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -29,6 +29,26 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+/* XXX kill agp_type! */
+static unsigned int cache_level_to_agp_type(struct drm_device *dev,
+					    enum i915_cache_level cache_level)
+{
+	switch (cache_level) {
+	case I915_CACHE_LLC_MLC:
+		if (INTEL_INFO(dev)->gen >= 6)
+			return AGP_USER_CACHED_MEMORY_LLC_MLC;
+		/* Older chipsets do not have this extra level of CPU
+		 * cacheing, so fallthrough and request the PTE simply
+		 * as cached.
+		 */
+	case I915_CACHE_LLC:
+		return AGP_USER_CACHED_MEMORY;
+	default:
+	case I915_CACHE_NONE:
+		return AGP_USER_MEMORY;
+	}
+}
+
 void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -39,6 +59,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 			      (dev_priv->mm.gtt_end - dev_priv->mm.gtt_start) / PAGE_SIZE);
 
 	list_for_each_entry(obj, &dev_priv->mm.gtt_list, gtt_list) {
+		unsigned int agp_type =
+			cache_level_to_agp_type(dev, obj->cache_level);
+
 		i915_gem_clflush_object(obj);
 
 		if (dev_priv->mm.gtt->needs_dmar) {
@@ -46,15 +69,14 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 
 			intel_gtt_insert_sg_entries(obj->sg_list,
 						    obj->num_sg,
-						    obj->gtt_space->start
-							>> PAGE_SHIFT,
-						    obj->agp_type);
+						    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,
-					       obj->agp_type);
+					       agp_type);
 	}
 
 	intel_gtt_chipset_flush();
@@ -64,6 +86,7 @@ int i915_gem_gtt_bind_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) {
@@ -77,12 +100,12 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
 		intel_gtt_insert_sg_entries(obj->sg_list,
 					    obj->num_sg,
 					    obj->gtt_space->start >> PAGE_SHIFT,
-					    obj->agp_type);
+					    agp_type);
 	} else
 		intel_gtt_insert_pages(obj->gtt_space->start >> PAGE_SHIFT,
 				       obj->base.size >> PAGE_SHIFT,
 				       obj->pages,
-				       obj->agp_type);
+				       agp_type);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 188b497..5c0466e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -676,7 +676,7 @@ static u32 capture_bo_list(struct drm_i915_error_buffer *err,
 		err->dirty = obj->dirty;
 		err->purgeable = obj->madv != I915_MADV_WILLNEED;
 		err->ring = obj->ring ? obj->ring->id : 0;
-		err->agp_type = obj->agp_type == AGP_USER_CACHED_MEMORY;
+		err->cache_level = obj->cache_level;
 
 		if (++i == count)
 			break;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 714f1a7..eab2565 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -236,7 +236,7 @@ init_pipe_control(struct intel_ring_buffer *ring)
 		ret = -ENOMEM;
 		goto err;
 	}
-	obj->agp_type = AGP_USER_CACHED_MEMORY;
+	obj->cache_level = I915_CACHE_LLC;
 
 	ret = i915_gem_object_pin(obj, 4096, true);
 	if (ret)
@@ -759,7 +759,7 @@ static int init_status_page(struct intel_ring_buffer *ring)
 		ret = -ENOMEM;
 		goto err;
 	}
-	obj->agp_type = AGP_USER_CACHED_MEMORY;
+	obj->cache_level = I915_CACHE_LLC;
 
 	ret = i915_gem_object_pin(obj, 4096, true);
 	if (ret != 0) {
-- 
1.7.4.1

[PATCH 02/13] drm/i915: Do not clflush snooped objects

[Chris Wilson]

Rely on the GPU snooping into the CPU cache for appropriately bound
objects on MI_FLUSH. Or perhaps one day we will have a cache-coherent
CPU/GPU package...

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 264bec8..bf32527 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2878,6 +2878,17 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj)
 	if (obj->pages == NULL)
 		return;
 
+	/* If the GPU is snooping the contents of the CPU cache,
+	 * we do not need to manually clear the CPU cache lines.  However,
+	 * the caches are only snooped when the render cache is
+	 * flushed/invalidated.  As we always have to emit invalidations
+	 * and flushes when moving into and out of the RENDER domain, correct
+	 * snooping behaviour occurs naturally as the result of our domain
+	 * tracking.
+	 */
+	if (obj->cache_level != I915_CACHE_NONE)
+		return;
+
 	trace_i915_gem_object_clflush(obj);
 
 	drm_clflush_pages(obj->pages, obj->base.size / PAGE_SIZE);
-- 
1.7.4.1

[PATCH 03/13] drm/i915: Introduce i915_gem_object_finish_gpu()

[Chris Wilson]

... reincarnated from i915_gem_object_flush_gpu(). The semantic
difference is that after calling finish_gpu() the object no longer
resides in any GPU domain, and so will cause the GPU caches to be
invalidated if it is ever used again.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 +-
 drivers/gpu/drm/i915/i915_gem.c      |   29 +++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2536334..4f63d17 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1163,7 +1163,7 @@ void i915_gem_clflush_object(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj,
 					    uint32_t read_domains,
 					    uint32_t write_domain);
-int __must_check i915_gem_object_flush_gpu(struct drm_i915_gem_object *obj);
+int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_init_ringbuffer(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 void i915_gem_do_init(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bf32527..2781b40 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2167,23 +2167,29 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 		return -EINVAL;
 	}
 
+	ret = i915_gem_object_finish_gpu(obj);
+	if (ret == -ERESTARTSYS)
+		return ret;
+	/* Continue on if we fail due to EIO, the GPU is hung so we
+	 * should be safe and we need to cleanup or else we might
+	 * cause memory corruption through use-after-free.
+	 */
+
 	/* blow away mappings if mapped through GTT */
 	i915_gem_release_mmap(obj);
 
 	/* Move the object to the CPU domain to ensure that
 	 * any possible CPU writes while it's not in the GTT
-	 * are flushed when we go to remap it. This will
-	 * also ensure that all pending GPU writes are finished
-	 * before we unbind.
+	 * are flushed when we go to remap it.
 	 */
-	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
+	if (ret == 0)
+		ret = i915_gem_object_set_to_cpu_domain(obj, 1);
 	if (ret == -ERESTARTSYS)
 		return ret;
-	/* Continue on if we fail due to EIO, the GPU is hung so we
-	 * should be safe and we need to cleanup or else we might
-	 * cause memory corruption through use-after-free.
-	 */
 	if (ret) {
+		/* In the event of a disaster, abandon all caches and
+		 * hope for the best.
+		 */
 		i915_gem_clflush_object(obj);
 		obj->base.read_domains = obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	}
@@ -3045,11 +3051,11 @@ i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
 }
 
 int
-i915_gem_object_flush_gpu(struct drm_i915_gem_object *obj)
+i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj)
 {
 	int ret;
 
-	if (!obj->active)
+	if ((obj->base.read_domains & I915_GEM_GPU_DOMAINS) == 0)
 		return 0;
 
 	if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
@@ -3058,6 +3064,9 @@ i915_gem_object_flush_gpu(struct drm_i915_gem_object *obj)
 			return ret;
 	}
 
+	/* Ensure that we invalidate the GPU's caches and TLBs. */
+	obj->base.read_domains &= I915_GEM_GPU_DOMAINS;
+
 	return i915_gem_object_wait_rendering(obj);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 09b20b1..c1337e4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1966,7 +1966,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		 * This should only fail upon a hung GPU, in which case we
 		 * can safely continue.
 		 */
-		ret = i915_gem_object_flush_gpu(obj);
+		ret = i915_gem_object_finish_gpu(obj);
 		(void) ret;
 	}
 
-- 
1.7.4.1

Re: [PATCH 03/13] drm/i915: Introduce i915_gem_object_finish_gpu()

[Daniel Vetter]

On Thu, Apr 14, 2011 at 10:03:37AM +0100, Chris Wilson wrote:
> ... reincarnated from i915_gem_object_flush_gpu(). The semantic
> difference is that after calling finish_gpu() the object no longer
> resides in any GPU domain, and so will cause the GPU caches to be
> invalidated if it is ever used again.

We might want to add a bool pipelined argument to finish_gpu so we can
reuse it in object_flush_fence. But that's for a later patch.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 04/13] drm/i915: Introduce i915_gem_object_finish_gtt()

[Chris Wilson]

Like its siblings finish_gpu(), this function clears the object from the
GTT domain forcing it to be trigger a domain invalidation should we ever
need to use via the GTT again.

Note that the most important side-effect of finishing the GTT domain
(aside from clearing the tracking read/write domains) is that it imposes
an memory barrier so that all accesses are complete before it returns,
which is important if you intend to be modifying translation tables
shortly afterwards. The second most important side-effect is that it
tears down the GTT mappings forcing a page-fault and invalidation on
next user access to the object.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2781b40..5d3e69f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2151,6 +2151,30 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
+{
+	u32 old_write_domain, old_read_domains;
+
+	if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0)
+		return;
+
+	/* Act a barrier for all accesses through the GTT */
+	mb();
+
+	/* Force a pagefault for domain tracking on next user access */
+	i915_gem_release_mmap(obj);
+
+	old_read_domains = obj->base.read_domains;
+	old_write_domain = obj->base.write_domain;
+
+	obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT;
+	obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT;
+
+	trace_i915_gem_object_change_domain(obj,
+					    old_read_domains,
+					    old_write_domain);
+}
+
 /**
  * Unbinds an object from the GTT aperture.
  */
@@ -2175,8 +2199,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 	 * cause memory corruption through use-after-free.
 	 */
 
-	/* blow away mappings if mapped through GTT */
-	i915_gem_release_mmap(obj);
+	i915_gem_object_finish_gtt(obj);
 
 	/* Move the object to the CPU domain to ensure that
 	 * any possible CPU writes while it's not in the GTT
-- 
1.7.4.1

Re: [PATCH 04/13] drm/i915: Introduce i915_gem_object_finish_gtt()

[Daniel Vetter]

On Thu, Apr 14, 2011 at 10:03:38AM +0100, Chris Wilson wrote:
> Like its siblings finish_gpu(), this function clears the object from the
> GTT domain forcing it to be trigger a domain invalidation should we ever
> need to use via the GTT again.
> 
> Note that the most important side-effect of finishing the GTT domain
> (aside from clearing the tracking read/write domains) is that it imposes
> an memory barrier so that all accesses are complete before it returns,
> which is important if you intend to be modifying translation tables
> shortly afterwards. The second most important side-effect is that it
> tears down the GTT mappings forcing a page-fault and invalidation on
> next user access to the object.

Our maze of cache handling functions, all alike is starting to get
annoying. Especially these finish functions which are essentially two-way
barriers and hence contain all the code that already exists in the from of
flush_foo_write_domain.

But every time I bang my head against this particular wall, the only thing
I can come up with is some abomination from hell. And I've been tossing
around ideas for the better part of a year already with no luck.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 04/13] drm/i915: Introduce i915_gem_object_finish_gtt()

[Chris Wilson]

On Thu, 14 Apr 2011 18:12:13 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> But every time I bang my head against this particular wall, the only thing
> I can come up with is some abomination from hell. And I've been tossing
> around ideas for the better part of a year already with no luck.

Hear, hear. My feelings exactly. At the moment, I think just having a
consistent set of interfaces is good enough.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 04/13] drm/i915: Introduce i915_gem_object_finish_gtt()

[Keith Packard]

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

On Thu, 14 Apr 2011 10:03:38 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> +	obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT;
> +	obj->base.write_domain &= ~I915_GEM_DOMAIN_GTT;

I'll bet you want to modify the read_domain as well.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

[PATCH 05/13] drm/i915/gtt: Split out i915_gem_gtt_rebind_object()

[Chris Wilson]

... in preparation for changing the cache level (and thus the flags upon
the PTEs) dynamically.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |   41 +++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2a1f8f1..4032289 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -29,6 +29,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+static void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
+				       enum i915_cache_level cache_level);
+
 /* XXX kill agp_type! */
 static uint32_t cache_level_to_agp_type(struct drm_device *dev,
 					enum i915_cache_level cache_level)
@@ -55,23 +58,8 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 			      (dev_priv->mm.gtt_end - dev_priv->mm.gtt_start) / PAGE_SIZE);
 
 	list_for_each_entry(obj, &dev_priv->mm.gtt_list, gtt_list) {
-		int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
-
 		i915_gem_clflush_object(obj);
-
-		if (dev_priv->mm.gtt->needs_dmar) {
-			BUG_ON(!obj->sg_list);
-
-			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);
+		i915_gem_gtt_rebind_object(obj, obj->cache_level);
 	}
 
 	intel_gtt_chipset_flush();
@@ -105,6 +93,27 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+static void i915_gem_gtt_rebind_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;
+	int agp_type = cache_level_to_agp_type(dev, cache_level);
+
+	if (dev_priv->mm.gtt->needs_dmar) {
+		BUG_ON(!obj->sg_list);
+
+		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);
+}
+
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
 {
 	intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
-- 
1.7.4.1

Re: [PATCH 05/13] drm/i915/gtt: Split out i915_gem_gtt_rebind_object()

[Daniel Vetter]

It's great when I can just throw an idea over the wall and Chris checks
whether it actually sticks ;-)

Reviewed-by: Daniel-Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 06/13] drm/i915: Add an interface to dynamically change the cache level

[Chris Wilson]

[anholt v2: Don't forget that when going from cached to uncached, we
haven't been tracking the write domain from the CPU perspective, since
we haven't needed it for GPU coherency.]

[ickle v3: We also need to make sure we relinquish any fences on older
chipsets and clear the GTT for sane domain tracking.]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/i915_drv.h         |    5 +++
 drivers/gpu/drm/i915/i915_gem.c         |   60 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.c     |    7 +---
 drivers/gpu/drm/i915/intel_ringbuffer.c |    6 ++-
 4 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4f63d17..61ccbeb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1196,9 +1196,14 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 uint32_t
 i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj);
 
+int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
+				    enum i915_cache_level cache_level);
+
 /* i915_gem_gtt.c */
 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,
+				enum i915_cache_level cache_level);
 void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj);
 
 /* i915_gem_evict.c */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5d3e69f..b92510c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3034,6 +3034,66 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 	return 0;
 }
 
+int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
+				    enum i915_cache_level cache_level)
+{
+	int ret;
+
+	if (obj->cache_level == cache_level)
+		return 0;
+
+	if (obj->pin_count) {
+		DRM_DEBUG("can not change the cache level of pinned objects\n");
+		return -EBUSY;
+	}
+
+	if (obj->gtt_space) {
+		ret = i915_gem_object_finish_gpu(obj);
+		if (ret)
+			return ret;
+
+		i915_gem_object_finish_gtt(obj);
+
+		/* Before SandyBridge, you could not use tiling or fence
+		 * registers with snooped memory, so relinquish any fences
+		 * currently pointing to our region in the aperture.
+		 */
+		if (INTEL_INFO(obj->base.dev)->gen < 6) {
+			ret = i915_gem_object_put_fence(obj);
+			if (ret)
+				return ret;
+		}
+
+		i915_gem_gtt_rebind_object(obj, cache_level);
+	}
+
+	if (cache_level == I915_CACHE_NONE) {
+		u32 old_read_domains, old_write_domain;
+
+		/* If we're coming from LLC cached, then we haven't
+		 * actually been tracking whether the data is in the
+		 * CPU cache or not, since we only allow one bit set
+		 * in obj->write_domain and have been skipping the clflushes.
+		 * Just set it to the CPU cache for now.
+		 */
+		WARN_ON(obj->base.write_domain & ~I915_GEM_DOMAIN_CPU);
+		WARN_ON(obj->base.read_domains & ~I915_GEM_DOMAIN_CPU);
+
+		old_read_domains = obj->base.read_domains;
+		old_write_domain = obj->base.write_domain;
+
+		obj->base.read_domains = I915_GEM_DOMAIN_CPU;
+		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
+
+		trace_i915_gem_object_change_domain(obj,
+						    old_read_domains,
+						    old_write_domain);
+	}
+
+	obj->cache_level = cache_level;
+	return 0;
+}
+
 /*
  * Prepare buffer for display plane. Use uninterruptible for possible flush
  * wait, as in modesetting process we're not supposed to be interrupted.
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4032289..3453f6c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -29,9 +29,6 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-static void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
-				       enum i915_cache_level cache_level);
-
 /* XXX kill agp_type! */
 static uint32_t cache_level_to_agp_type(struct drm_device *dev,
 					enum i915_cache_level cache_level)
@@ -93,8 +90,8 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
-static void i915_gem_gtt_rebind_object(struct drm_i915_gem_object *obj,
-				       enum i915_cache_level cache_level)
+void i915_gem_gtt_rebind_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;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index eab2565..f15d80f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -236,7 +236,8 @@ init_pipe_control(struct intel_ring_buffer *ring)
 		ret = -ENOMEM;
 		goto err;
 	}
-	obj->cache_level = I915_CACHE_LLC;
+
+	i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
 
 	ret = i915_gem_object_pin(obj, 4096, true);
 	if (ret)
@@ -759,7 +760,8 @@ static int init_status_page(struct intel_ring_buffer *ring)
 		ret = -ENOMEM;
 		goto err;
 	}
-	obj->cache_level = I915_CACHE_LLC;
+
+	i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
 
 	ret = i915_gem_object_pin(obj, 4096, true);
 	if (ret != 0) {
-- 
1.7.4.1

Re: [PATCH 06/13] drm/i915: Add an interface to dynamically change the cache level

[Daniel Vetter]

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 07/13] drm/i915: Mark the cursor and the overlay as being part of the display planes

[Chris Wilson]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_overlay.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c1337e4..56741c6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5360,7 +5360,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
-		ret = i915_gem_object_set_to_gtt_domain(obj, 0);
+		ret = i915_gem_object_set_to_display_plane(obj, NULL);
 		if (ret) {
 			DRM_ERROR("failed to move cursor bo into the GTT\n");
 			goto fail_unpin;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index a670c00..e0903c5 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -777,7 +777,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_set_to_gtt_domain(new_bo, 0);
+	ret = i915_gem_object_set_to_display_plane(new_bo, NULL);
 	if (ret != 0)
 		goto out_unpin;
 
-- 
1.7.4.1

Re: [PATCH 07/13] drm/i915: Mark the cursor and the overlay as being part of the display planes

[Keith Packard]

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

On Thu, 14 Apr 2011 10:03:41 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5360,7 +5360,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  			goto fail_locked;
>  		}
>  
> -		ret = i915_gem_object_set_to_gtt_domain(obj, 0);
> +		ret = i915_gem_object_set_to_display_plane(obj, NULL);
>  		if (ret) {
>  			DRM_ERROR("failed to move cursor bo into the GTT\n");
>  			goto fail_unpin;
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index a670c00..e0903c5 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -777,7 +777,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  	if (ret != 0)
>  		return ret;
>  
> -	ret = i915_gem_object_set_to_gtt_domain(new_bo, 0);
> +	ret = i915_gem_object_set_to_display_plane(new_bo, NULL);

set_to_display_plane has a comment stating that it is always called from
a non - interruptible context and uses a non-interruptible flush wait as
a result.

I think we would want these new code paths to allow for interrupting the
operation?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 07/13] drm/i915: Mark the cursor and the overlay as being part of the display planes

[Chris Wilson]

On Wed, 04 May 2011 10:09:53 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 14 Apr 2011 10:03:41 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5360,7 +5360,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> >  			goto fail_locked;
> >  		}
> >  
> > -		ret = i915_gem_object_set_to_gtt_domain(obj, 0);
> > +		ret = i915_gem_object_set_to_display_plane(obj, NULL);
> >  		if (ret) {
> >  			DRM_ERROR("failed to move cursor bo into the GTT\n");
> >  			goto fail_unpin;
> > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > index a670c00..e0903c5 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -777,7 +777,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> >  	if (ret != 0)
> >  		return ret;
> >  
> > -	ret = i915_gem_object_set_to_gtt_domain(new_bo, 0);
> > +	ret = i915_gem_object_set_to_display_plane(new_bo, NULL);
> 
> set_to_display_plane has a comment stating that it is always called from
> a non - interruptible context and uses a non-interruptible flush wait as
> a result.
> 
> I think we would want these new code paths to allow for interrupting the
> operation?

The comment is very stale, I believe I remove it in one of the patches.
There were some fun bugs when rebinding the scanout under an
uninterruptible modeswitch that convinced me that we had a choice between
threading the interruptible flag through the entire unbind callchain, or
to simply mark the device as uninterruptible for the duration.

The code now does the latter.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 07/13] drm/i915: Mark the cursor and the overlay as being part of the display planes

[Keith Packard]

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

On Wed, 04 May 2011 19:28:35 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> to simply mark the device as uninterruptible for the duration.
> 
> The code now does the latter.

I'm good with that plan during modesetting, the question is whether the
new users of this function should allow for interrupts.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 07/13] drm/i915: Mark the cursor and the overlay as being part of the display planes

[Chris Wilson]

On Wed, 04 May 2011 11:46:42 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Wed, 04 May 2011 19:28:35 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > to simply mark the device as uninterruptible for the duration.
> > 
> > The code now does the latter.
> 
> I'm good with that plan during modesetting, the question is whether the
> new users of this function should allow for interrupts.

The callers control whether that function is interruptible or not by
setting a flag on the device. So far the only piece of code that we have
allowed to run non-interruptible is the modesetting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 08/13] drm/i915: Pin after setting to the display plane

[Chris Wilson]

A few operations we do in order to move the object into the display
plane it is important for future safety to forbid whilst pinned. As a
result, we want to pin afterwards. At the moment, setting to the display
plane of an unbound object is simply to bind it, so
set_to_display_plane() becomes a no-op.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c      |    7 +++----
 drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++----------
 drivers/gpu/drm/i915/intel_overlay.c |    8 ++++----
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b92510c..3f1181b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3105,15 +3105,14 @@ i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
 	uint32_t old_read_domains;
 	int ret;
 
-	/* Not valid to be called on unbound objects. */
-	if (obj->gtt_space == NULL)
-		return -EINVAL;
+	/* If the object is currently unbound, this is a no-op. */
+	if (obj->gtt_space)
+		return 0;
 
 	ret = i915_gem_object_flush_gpu_write_domain(obj);
 	if (ret)
 		return ret;
 
-
 	/* Currently, we are always called from an non-interruptible context. */
 	if (pipelined != obj->ring) {
 		ret = i915_gem_object_wait_rendering(obj);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56741c6..811b6e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1807,14 +1807,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	}
 
 	dev_priv->mm.interruptible = false;
-	ret = i915_gem_object_pin(obj, alignment, true);
-	if (ret)
-		goto err_interruptible;
-
 	ret = i915_gem_object_set_to_display_plane(obj, pipelined);
 	if (ret)
 		goto err_unpin;
 
+	ret = i915_gem_object_pin(obj, alignment, true);
+	if (ret)
+		goto err_interruptible;
+
 	/* Install a fence for tiled scan-out. Pre-i965 always needs a
 	 * fence, whereas 965+ only requires a fence if using
 	 * framebuffer compression.  For simplicity, we always install
@@ -5354,12 +5354,6 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
-		ret = i915_gem_object_pin(obj, PAGE_SIZE, true);
-		if (ret) {
-			DRM_ERROR("failed to pin cursor bo\n");
-			goto fail_locked;
-		}
-
 		ret = i915_gem_object_set_to_display_plane(obj, NULL);
 		if (ret) {
 			DRM_ERROR("failed to move cursor bo into the GTT\n");
@@ -5372,6 +5366,12 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 			goto fail_unpin;
 		}
 
+		ret = i915_gem_object_pin(obj, PAGE_SIZE, true);
+		if (ret) {
+			DRM_ERROR("failed to pin cursor bo\n");
+			goto fail_locked;
+		}
+
 		addr = obj->gtt_offset;
 	} else {
 		int align = IS_I830(dev) ? 16 * 1024 : 256;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e0903c5..b4ae58d 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -773,10 +773,6 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_pin(new_bo, PAGE_SIZE, true);
-	if (ret != 0)
-		return ret;
-
 	ret = i915_gem_object_set_to_display_plane(new_bo, NULL);
 	if (ret != 0)
 		goto out_unpin;
@@ -785,6 +781,10 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret)
 		goto out_unpin;
 
+	ret = i915_gem_object_pin(new_bo, PAGE_SIZE, true);
+	if (ret != 0)
+		return ret;
+
 	if (!overlay->active) {
 		regs = intel_overlay_map_regs(overlay);
 		if (!regs) {
-- 
1.7.4.1

Re: [PATCH 08/13] drm/i915: Pin after setting to the display plane

[Daniel Vetter]

On Thu, Apr 14, 2011 at 10:03:42AM +0100, Chris Wilson wrote:
> A few operations we do in order to move the object into the display
> plane it is important for future safety to forbid whilst pinned. As a
> result, we want to pin afterwards. At the moment, setting to the display
> plane of an unbound object is simply to bind it, so
> set_to_display_plane() becomes a no-op.

After the movement all three code-paths suffer from
	if (ret)
		goto foo_unpin;
before anything is actually pinned. With that fixed, it's

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 08/13] drm/i915: Pin after setting to the display plane

[Chris Wilson]

On Thu, 14 Apr 2011 19:34:17 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> After the movement all three code-paths suffer from
> 	if (ret)
> 		goto foo_unpin;
> before anything is actually pinned. With that fixed, it's

My only defense was that was about the third or fifth variation that I
tried in a couple of hours with lots of painful rebasing to get the patch
flow right...

I know for one iteration I had remembered to update the error paths.

Oh well,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/i915: Combine pinning after setting to the display plane

[Chris Wilson]

We need to perform a few operations in order to move the object into the
display plane (where it can be accessed coherently by the display
engine) that are important for future safety to forbid whilst pinned. As a
result, we want to need to perform some of operations before pinning,
but some are required once we have been bound into the GTT. So combine
the pinning performed by all the callers with set_to_display_plane(), so
this complication is contained within the single function.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |    3 ++-
 drivers/gpu/drm/i915/i915_gem.c      |   24 ++++++++++--------------
 drivers/gpu/drm/i915/intel_display.c |   22 +++++-----------------
 drivers/gpu/drm/i915/intel_overlay.c |    8 ++------
 4 files changed, 19 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 61ccbeb..759045a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1182,7 +1182,8 @@ int __must_check
 i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj,
 				  bool write);
 int __must_check
-i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
+i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
+				     u32 alignment,
 				     struct intel_ring_buffer *pipelined);
 int i915_gem_attach_phys_object(struct drm_device *dev,
 				struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7271956..9c1ff7d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3099,21 +3099,16 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
  * wait, as in modesetting process we're not supposed to be interrupted.
  */
 int
-i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
+i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
+				     u32 alignment,
 				     struct intel_ring_buffer *pipelined)
 {
-	uint32_t old_read_domains;
 	int ret;
 
-	/* Not valid to be called on unbound objects. */
-	if (obj->gtt_space == NULL)
-		return -EINVAL;
-
 	ret = i915_gem_object_flush_gpu_write_domain(obj);
 	if (ret)
 		return ret;
 
-
 	/* Currently, we are always called from an non-interruptible context. */
 	if (pipelined != obj->ring) {
 		ret = i915_gem_object_wait_rendering(obj);
@@ -3121,14 +3116,15 @@ i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
-	i915_gem_object_flush_cpu_write_domain(obj);
-
-	old_read_domains = obj->base.read_domains;
-	obj->base.read_domains |= I915_GEM_DOMAIN_GTT;
+	ret = i915_gem_object_pin(obj, alignment, true);
+	if (ret)
+		return ret;
 
-	trace_i915_gem_object_change_domain(obj,
-					    old_read_domains,
-					    obj->base.write_domain);
+	ret = i915_gem_object_set_to_gtt_domain(obj, false);
+	if (ret) {
+		i915_gem_object_unpin(obj);
+		return ret;
+	}
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56741c6..c3e61ef 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1807,14 +1807,10 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	}
 
 	dev_priv->mm.interruptible = false;
-	ret = i915_gem_object_pin(obj, alignment, true);
+	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)
 		goto err_interruptible;
 
-	ret = i915_gem_object_set_to_display_plane(obj, pipelined);
-	if (ret)
-		goto err_unpin;
-
 	/* Install a fence for tiled scan-out. Pre-i965 always needs a
 	 * fence, whereas 965+ only requires a fence if using
 	 * framebuffer compression.  For simplicity, we always install
@@ -5354,22 +5350,16 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
-		ret = i915_gem_object_pin(obj, PAGE_SIZE, true);
-		if (ret) {
-			DRM_ERROR("failed to pin cursor bo\n");
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_set_to_display_plane(obj, NULL);
+		ret = i915_gem_object_pin_to_display_plane(obj, 0, NULL);
 		if (ret) {
 			DRM_ERROR("failed to move cursor bo into the GTT\n");
-			goto fail_unpin;
+			goto fail_locked;
 		}
 
 		ret = i915_gem_object_put_fence(obj);
 		if (ret) {
-			DRM_ERROR("failed to move cursor bo into the GTT\n");
-			goto fail_unpin;
+			DRM_ERROR("failed to release fence for cursor");
+			goto fail_locked;
 		}
 
 		addr = obj->gtt_offset;
@@ -5408,8 +5398,6 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	return 0;
-fail_unpin:
-	i915_gem_object_unpin(obj);
 fail_locked:
 	mutex_unlock(&dev->struct_mutex);
 fail:
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e0903c5..67fd337 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -773,17 +773,13 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_pin(new_bo, PAGE_SIZE, true);
+	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_set_to_display_plane(new_bo, NULL);
-	if (ret != 0)
-		goto out_unpin;
-
 	ret = i915_gem_object_put_fence(new_bo);
 	if (ret)
-		goto out_unpin;
+		return ret;
 
 	if (!overlay->active) {
 		regs = intel_overlay_map_regs(overlay);
-- 
1.7.4.1

[PATCH 2/2] drm/i915: Use the uncached domain for the display planes

[Chris Wilson]

From: Eric Anholt <eric@anholt.net>

The simplest and common method for ensuring scanout coherency on all
chipsets is to mark the scanout buffers as uncached (and for
userspace to remember to flush the render cache every so often).

We can improve upon this for later generations by marking scanout
objects as GFDT and only flush those cachelines when required. However,
we start simple.

[v2: Move the set to uncached above the clflush.  Otherwise, we'd skip
the clflush and try to scan out data that was still sitting in the
cache.]

Signed-off-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9c1ff7d..5e6c504 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3116,6 +3116,19 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
+	/* The display engine is not coherent with the LLC cache on gen6.  As
+	 * a result, we make sure that the pinning that is about to occur is
+	 * done with uncached PTEs. This is lowest common denominator for all
+	 * chipsets.
+	 *
+	 * However for gen6+, we could do better by using the GFDT bit instead
+	 * of uncaching, which would allow us to flush all the LLC-cached data
+	 * with that bit in the PTE to main memory with just one PIPE_CONTROL.
+	 */
+	ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
+	if (ret)
+		return ret;
+
 	ret = i915_gem_object_pin(obj, alignment, true);
 	if (ret)
 		return ret;
-- 
1.7.4.1

Re: [PATCH 2/2] drm/i915: Use the uncached domain for the display planes

[Daniel Vetter]

Now that the pinning and display plane domain changes settle ...

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/2] drm/i915: Combine pinning after setting to the display plane

[Daniel Vetter]

On Fri, Apr 15, 2011 at 07:04:13AM +0100, Chris Wilson wrote:
> We need to perform a few operations in order to move the object into the
> display plane (where it can be accessed coherently by the display
> engine) that are important for future safety to forbid whilst pinned. As a
> result, we want to need to perform some of operations before pinning,
> but some are required once we have been bound into the GTT. So combine
> the pinning performed by all the callers with set_to_display_plane(), so
> this complication is contained within the single function.

The latest version from -next-proposed with the fail_unpin: hunk
reinstated does indeed get rid of the cacheline dirt I've seen on my ilk.

But in that version you're calling set_to_gtt_domain which calls
flush_gpu_write_domain and then waits synchronously when
obj->pending_gpu_write is set. Which is almost guaranteed to happen for
pageflips.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/2] drm/i915: Combine pinning after setting to the display plane

[Chris Wilson]

On Fri, 15 Apr 2011 14:11:25 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> But in that version you're calling set_to_gtt_domain which calls
> flush_gpu_write_domain and then waits synchronously when
> obj->pending_gpu_write is set. Which is almost guaranteed to happen for
> pageflips.

Gah. Completely destroying the sole reason why set_to_display_plane()
was first introduced.

I've slept, had some coffee, even felt inspired to add some more
comments...

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Combine pinning with setting to the display plane

[Chris Wilson]

We need to perform a few operations in order to move the object into the
display plane (where it can be accessed coherently by the display
engine) that are important for future safety to forbid whilst pinned. As a
result, we want to need to perform some of the operations before pinning,
but some are required once we have been bound into the GTT. So combine
the pinning performed by all the callers with set_to_display_plane(), so
this complication is contained within the single function.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |    3 +-
 drivers/gpu/drm/i915/i915_gem.c      |   37 +++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c |   18 +++------------
 drivers/gpu/drm/i915/intel_overlay.c |    6 +----
 4 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 61ccbeb..759045a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1182,7 +1182,8 @@ int __must_check
 i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj,
 				  bool write);
 int __must_check
-i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
+i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
+				     u32 alignment,
 				     struct intel_ring_buffer *pipelined);
 int i915_gem_attach_phys_object(struct drm_device *dev,
 				struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7271956..c73eeaf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3095,40 +3095,55 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 }
 
 /*
- * Prepare buffer for display plane. Use uninterruptible for possible flush
- * wait, as in modesetting process we're not supposed to be interrupted.
+ * Prepare buffer for display plane (scanout, cursors, etc).
+ * Expects to be called from an uninterruptible phase (modesetting) and allows
+ * any flushes to be pipelined (for pageflips).
+ *
+ * For the display plane, we want to be in the GTT but out of any write
+ * domains. So in many ways this looks like set_to_gtt_domain() apart from the
+ * ability to pipeline the waits, pinning and any additional subtleties
+ * that may differentiate the display plane from ordinary buffers.
  */
 int
-i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
+i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
+				     u32 alignment,
 				     struct intel_ring_buffer *pipelined)
 {
-	uint32_t old_read_domains;
+	u32 old_read_domains, old_write_domain;
 	int ret;
 
-	/* Not valid to be called on unbound objects. */
-	if (obj->gtt_space == NULL)
-		return -EINVAL;
-
 	ret = i915_gem_object_flush_gpu_write_domain(obj);
 	if (ret)
 		return ret;
 
-
-	/* Currently, we are always called from an non-interruptible context. */
 	if (pipelined != obj->ring) {
 		ret = i915_gem_object_wait_rendering(obj);
 		if (ret)
 			return ret;
 	}
 
+	/* As the user may map the buffer once pinned in the display plane
+	 * (e.g. libkms for the bootup splash), we have to ensure that we
+	 * always use map_and_fenceable for all scanout buffers.
+	 */
+	ret = i915_gem_object_pin(obj, alignment, true);
+	if (ret)
+		return ret;
+
 	i915_gem_object_flush_cpu_write_domain(obj);
 
+	old_write_domain = obj->base.write_domain;
 	old_read_domains = obj->base.read_domains;
+
+	/* It should now be out of any other write domains, and we can update
+	 * the domain values for our changes.
+	 */
+	BUG_ON((obj->base.write_domain & ~I915_GEM_DOMAIN_GTT) != 0);
 	obj->base.read_domains |= I915_GEM_DOMAIN_GTT;
 
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
-					    obj->base.write_domain);
+					    old_write_domain);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56741c6..62f9e52 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1807,14 +1807,10 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	}
 
 	dev_priv->mm.interruptible = false;
-	ret = i915_gem_object_pin(obj, alignment, true);
+	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 	if (ret)
 		goto err_interruptible;
 
-	ret = i915_gem_object_set_to_display_plane(obj, pipelined);
-	if (ret)
-		goto err_unpin;
-
 	/* Install a fence for tiled scan-out. Pre-i965 always needs a
 	 * fence, whereas 965+ only requires a fence if using
 	 * framebuffer compression.  For simplicity, we always install
@@ -5354,21 +5350,15 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
 			goto fail_locked;
 		}
 
-		ret = i915_gem_object_pin(obj, PAGE_SIZE, true);
-		if (ret) {
-			DRM_ERROR("failed to pin cursor bo\n");
-			goto fail_locked;
-		}
-
-		ret = i915_gem_object_set_to_display_plane(obj, NULL);
+		ret = i915_gem_object_pin_to_display_plane(obj, 0, NULL);
 		if (ret) {
 			DRM_ERROR("failed to move cursor bo into the GTT\n");
-			goto fail_unpin;
+			goto fail_locked;
 		}
 
 		ret = i915_gem_object_put_fence(obj);
 		if (ret) {
-			DRM_ERROR("failed to move cursor bo into the GTT\n");
+			DRM_ERROR("failed to release fence for cursor");
 			goto fail_unpin;
 		}
 
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index e0903c5..fcf6fcb 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -773,14 +773,10 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_pin(new_bo, PAGE_SIZE, true);
+	ret = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
 	if (ret != 0)
 		return ret;
 
-	ret = i915_gem_object_set_to_display_plane(new_bo, NULL);
-	if (ret != 0)
-		goto out_unpin;
-
 	ret = i915_gem_object_put_fence(new_bo);
 	if (ret)
 		goto out_unpin;
-- 
1.7.4.1

Re: [PATCH] drm/i915: Combine pinning with setting to the display plane

[Daniel Vetter]

On Sat, Apr 16, 2011 at 07:27:31AM +0100, Chris Wilson wrote:
> We need to perform a few operations in order to move the object into the
> display plane (where it can be accessed coherently by the display
> engine) that are important for future safety to forbid whilst pinned. As a
> result, we want to need to perform some of the operations before pinning,
> but some are required once we have been bound into the GTT. So combine
> the pinning performed by all the callers with set_to_display_plane(), so
> this complication is contained within the single function.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

One small comment-nitpick below.

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7271956..c73eeaf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3095,40 +3095,55 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  }
>  
>  /*
> - * Prepare buffer for display plane. Use uninterruptible for possible flush
> - * wait, as in modesetting process we're not supposed to be interrupted.
> + * Prepare buffer for display plane (scanout, cursors, etc).
> + * Expects to be called from an uninterruptible phase (modesetting) and allows

Shouldn't that be "Can be called from uninterruptible ..." At least that's
the case for cursor setting and I think we could also change the pageflip
code to interruptible. User will then only see the old screen if we
interrupt and bail out, not a dying scanout engine.
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Combine pinning with setting to the display plane

[Chris Wilson]

On Sat, 16 Apr 2011 12:52:35 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> One small comment-nitpick below.
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 7271956..c73eeaf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3095,40 +3095,55 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  }
> >  
> >  /*
> > - * Prepare buffer for display plane. Use uninterruptible for possible flush
> > - * wait, as in modesetting process we're not supposed to be interrupted.
> > + * Prepare buffer for display plane (scanout, cursors, etc).
> > + * Expects to be called from an uninterruptible phase (modesetting) and allows
> 
> Shouldn't that be "Can be called from uninterruptible ..."

Yes, I misphrased. I was thinking along the lines that it should be able
to cope with uninterruptible phases but not that it was mandatory.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 09/13] drm/i915: Use the uncached domain for the display planes

[Chris Wilson]

From: Eric Anholt <eric@anholt.net>

The simplest and common method for ensuring scanout coherency on all
chipsets is to mark the scanout buffers as uncached (and for
userspace to remember to flush the render cache every so often).

We can improve upon this for later generations by marking scanout
objects as GFDT and only flush those cachelines when required. However,
we start simple.

[v2: Move the set to uncached above the clflush.  Otherwise, we'd skip
the clflush and try to scan out data that was still sitting in the
cache.]

Signed-off-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   32 +++++++++++++++++++++++---------
 1 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3f1181b..ea8e7e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3105,10 +3105,6 @@ i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
 	uint32_t old_read_domains;
 	int ret;
 
-	/* If the object is currently unbound, this is a no-op. */
-	if (obj->gtt_space)
-		return 0;
-
 	ret = i915_gem_object_flush_gpu_write_domain(obj);
 	if (ret)
 		return ret;
@@ -3120,14 +3116,32 @@ i915_gem_object_set_to_display_plane(struct drm_i915_gem_object *obj,
 			return ret;
 	}
 
+	/* The display engine is not coherent with the LLC cache on gen6.  As
+	 * a result, we make sure that the pinning that is about to occur is
+	 * done with uncached PTEs. This is lowest common denominator for all
+	 * chipsets.
+	 *
+	 * However for gen6+, we could do better by using the GFDT bit instead
+	 * of uncaching, which would allow us to flush all the LLC-cached data
+	 * with that bit in the PTE to main memory with just one PIPE_CONTROL.
+	 */
+	ret = i915_gem_object_set_cache_level(obj, I915_CACHE_NONE);
+	if (ret)
+		return ret;
+
 	i915_gem_object_flush_cpu_write_domain(obj);
 
-	old_read_domains = obj->base.read_domains;
-	obj->base.read_domains |= I915_GEM_DOMAIN_GTT;
+	/* If the object is not bound, it will be moved into the GTT domain
+	 * when pinned otherwise do so here.
+	 */
+	if (obj->gtt_space) {
+		old_read_domains = obj->base.read_domains;
+		obj->base.read_domains |= I915_GEM_DOMAIN_GTT;
 
-	trace_i915_gem_object_change_domain(obj,
-					    old_read_domains,
-					    obj->base.write_domain);
+		trace_i915_gem_object_change_domain(obj,
+						    old_read_domains,
+						    obj->base.write_domain);
+	}
 
 	return 0;
 }
-- 
1.7.4.1

[PATCH 10/13] drm/i915: Use the CPU domain for snooped pwrites

[Chris Wilson]

The docs warn us to be particularly careful not to write to a snooped
page through the GTT. Fortunately, this ties in very well with the
existing pwrite infrastucture to use the CPU domain where preferable and
the API already implies that the write is CPU linear..

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ea8e7e2..dd2dc9d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1001,6 +1001,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 	if (obj->phys_obj)
 		ret = i915_gem_phys_pwrite(dev, obj, args, file);
 	else if (obj->gtt_space &&
+		 obj->cache_level == I915_CACHE_NONE &&
 		 obj->base.write_domain != I915_GEM_DOMAIN_CPU) {
 		ret = i915_gem_object_pin(obj, 0, true);
 		if (ret)
-- 
1.7.4.1

Re: [PATCH 10/13] drm/i915: Use the CPU domain for snooped pwrites

[Daniel Vetter]

On Thu, Apr 14, 2011 at 10:03:44AM +0100, Chris Wilson wrote:
> The docs warn us to be particularly careful not to write to a snooped
> page through the GTT. Fortunately, this ties in very well with the
> existing pwrite infrastucture to use the CPU domain where preferable and
> the API already implies that the write is CPU linear..

Perhaps add "... careful not to write to a snooped page through the GTT *on
pre-snb*". On snb it better work (but might be beneficial)!

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 11/13] drm/i915: Prevent mmap access through the GTT of snooped pages

[Chris Wilson]

The docs have a dire warning not to attempt to access snooped (the old
style of cache sharing on pre-SandyBridge chipsets) pages through the GTT.
Prevent userspace from doing so by sending them a SIGBUS if they try.

[Now it is possible with a bit of extra complexity to map the snooped
CPU page into the vma and return that through i915_gem_fault() instead.
The question is: is it simpler to do that workaround in the kernel than
it is to do it in userspace?]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd2dc9d..1f57f99 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1211,6 +1211,16 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 
 	trace_i915_gem_object_fault(obj, page_offset, true, write);
 
+	/* The docs warn of dire consequences if we try to write to a snooped
+	 * page through the GTT. So kill the driver/app early with a SIGBUS.
+	 */
+	if (INTEL_INFO(dev)->gen < 6 && obj->cache_level != I915_CACHE_NONE) {
+		DRM_DEBUG("Attempting to read a snooped page through the GTT, "
+			  "this is illegal on pre-SandyBridge chipsets.\n");
+		ret = -EINVAL;
+		goto unlock;
+	}
+
 	/* Now bind it into the GTT if needed */
 	if (!obj->map_and_fenceable) {
 		ret = i915_gem_object_unbind(obj);
-- 
1.7.4.1

Re: [PATCH 11/13] drm/i915: Prevent mmap access through the GTT of snooped pages

[Keith Packard]

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

On Thu, 14 Apr 2011 10:03:45 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> The docs have a dire warning not to attempt to access snooped (the old
> style of cache sharing on pre-SandyBridge chipsets) pages through the GTT.
> Prevent userspace from doing so by sending them a SIGBUS if they try.

There is not plan for the user mode driver to use snooped access on
pre-SNB chipsets. So, the kernel should not try to support this in any
way.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

[PATCH 12/13] drm/i915: Prevent mixing of snooped and tiling modes for old chipsets

[Chris Wilson]

Older chipsets do not support snooping (i.e. cache sharing between the
CPU and GPU) on tiled surfaces. So prevent userspace from being silly
should we one day expose the ability to change cache levels from
userspace.

It does enforce a strict ordering for mode changing though. So in order
to change a buffer to snooped, the driver has to clear any tiling first
and then change the cache level. This is consistent with how we flush
and update the PTEs and seems a reasonable imposition on the driver.
Deferring the check until use, whilst providing flexibilty here, implies
forcing extra unbinds and a more complicated error message from, for
example, execbuffer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c        |    8 ++++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |    9 +++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1f57f99..46b63c3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3053,6 +3053,14 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 	if (obj->cache_level == cache_level)
 		return 0;
 
+	if (INTEL_INFO(obj->base.dev)->gen < 6 &&
+	    obj->tiling_mode != I915_TILING_NONE &&
+	    cache_level != I915_CACHE_NONE) {
+		DRM_DEBUG("can not enable snooping on a tiled surface, "
+			  "it must be linear for pre-SandyBridge chipsets\n");
+		return -EINVAL;
+	}
+
 	if (obj->pin_count) {
 		DRM_DEBUG("can not change the cache level of pinned objects\n");
 		return -EBUSY;
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 281ad3d..ca69fd4 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -331,6 +331,14 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 	}
 
 	mutex_lock(&dev->struct_mutex);
+	if (INTEL_INFO(dev)->gen < 6 &&
+	    args->tiling_mode != I915_TILING_NONE &&
+	    obj->cache_level != I915_CACHE_NONE) {
+		DRM_DEBUG("can't not set a tiling mode on snooped memory,"
+			  "it must be linear for pre-SandyBridge chipsets\n");
+		ret = -EINVAL;
+		goto err;
+	}
 	if (args->tiling_mode != obj->tiling_mode ||
 	    args->stride != obj->stride) {
 		/* We need to rebind the object if its current allocation
@@ -360,6 +368,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		}
 	}
 	/* we have to maintain this existing ABI... */
+err:
 	args->stride = obj->stride;
 	args->tiling_mode = obj->tiling_mode;
 	drm_gem_object_unreference(&obj->base);
-- 
1.7.4.1

Re: [PATCH 12/13] drm/i915: Prevent mixing of snooped and tiling modes for old chipsets

[Daniel Vetter]

On Thu, Apr 14, 2011 at 10:03:46AM +0100, Chris Wilson wrote:
> Older chipsets do not support snooping (i.e. cache sharing between the
> CPU and GPU) on tiled surfaces. So prevent userspace from being silly
> should we one day expose the ability to change cache levels from
> userspace.
> 
> It does enforce a strict ordering for mode changing though. So in order
> to change a buffer to snooped, the driver has to clear any tiling first
> and then change the cache level. This is consistent with how we flush
> and update the PTEs and seems a reasonable imposition on the driver.
> Deferring the check until use, whilst providing flexibilty here, implies
> forcing extra unbinds and a more complicated error message from, for
> example, execbuffer.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Small error in the debug output below.

> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 281ad3d..ca69fd4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -331,6 +331,14 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  	}
>  
>  	mutex_lock(&dev->struct_mutex);
> +	if (INTEL_INFO(dev)->gen < 6 &&
> +	    args->tiling_mode != I915_TILING_NONE &&
> +	    obj->cache_level != I915_CACHE_NONE) {
> +		DRM_DEBUG("can't not set a tiling mode on snooped memory,"

One negation too much.

> +			  "it must be linear for pre-SandyBridge chipsets\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
>  	if (args->tiling_mode != obj->tiling_mode ||
>  	    args->stride != obj->stride) {
>  		/* We need to rebind the object if its current allocation
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 12/13] drm/i915: Prevent mixing of snooped and tiling modes for old chipsets

[Chris Wilson]

On Thu, 14 Apr 2011 19:43:35 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 14, 2011 at 10:03:46AM +0100, Chris Wilson wrote:
> > +	if (INTEL_INFO(dev)->gen < 6 &&
> > +	    args->tiling_mode != I915_TILING_NONE &&
> > +	    obj->cache_level != I915_CACHE_NONE) {
> > +		DRM_DEBUG("can't not set a tiling mode on snooped memory,"
> 
> One negation too much.

D'oh. That was me trying to be extra careful and not succumb to my usual
habit of over-using contractions. Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 12/13] drm/i915: Prevent mixing of snooped and tiling modes for old chipsets

[Keith Packard]

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

On Thu, 14 Apr 2011 10:03:46 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Older chipsets do not support snooping (i.e. cache sharing between the
> CPU and GPU) on tiled surfaces. So prevent userspace from being silly
> should we one day expose the ability to change cache levels from
> userspace.

We will not need this as there is no plan to have user-mode use snooped
access on older chipsets.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

[PATCH 13/13] drm/i915: Use the LLC mode on gen6 for everything but display.

[Chris Wilson]

From: Eric Anholt <eric@anholt.net>

Improves full-screen openarena on my laptop 20.3% +/- 4.0% (n=3)
Improves 800x600 nexuiz on my laptop 12.3% +/- 0.1% (n=3)

We have more room to improve with doing LLC caching for display using
GFDT, and in doing LLC+MLC caching, but this was an easy performance
win and incremental improvement toward those two.

Signed-off-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 46b63c3..7992308 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3704,7 +3704,23 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
 	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
 
-	obj->cache_level = I915_CACHE_NONE;
+	if (IS_GEN6(dev)) {
+		/* On Gen6, we can have the GPU use the LLC (the CPU
+		 * cache) for about a 10% performance improvement
+		 * compared to uncached.  Graphics requests other than
+		 * display scanout are coherent with the CPU in
+		 * accessing this cache.  This means in this mode we
+		 * don't need to clflush on the CPU side, and on the
+		 * GPU side we only need to flush internal caches to
+		 * get data visible to the CPU.
+		 *
+		 * However, we maintain the display planes as UC, and so
+		 * need to rebind when first used as such.
+		 */
+		obj->cache_level = I915_CACHE_LLC;
+	} else
+		obj->cache_level = I915_CACHE_NONE;
+
 	obj->base.driver_private = NULL;
 	obj->fence_reg = I915_FENCE_REG_NONE;
 	INIT_LIST_HEAD(&obj->mm_list);
-- 
1.7.4.1

Re: i915 llc for -next

[Chris Wilson]

Just a quite note to say that drm-intel-next-proposed has been updated
with all the feedback received yesterday. Get testing!

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre