diff mbox

[RFC,04/38] drm/i915: Make pin global flags explicit

Message ID 1412701894-28905-5-git-send-email-michel.thierry@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michel Thierry Oct. 7, 2014, 5:11 p.m. UTC 
From: Ben Widawsky <benjamin.widawsky@intel.com>

The driver currently lets callers pin global, and then tries to do
things correctly inside the function. Doing so has two downsides:
1. It's not possible to exclusively pin to a global, or an aliasing
address space.
2. It's difficult to read, and understand.

The eventual goal when realized should fix both of the issues. This patch
which should have no functional impact begins to address these issues
without intentionally breaking things.

v2: Replace PIN_GLOBAL with PIN_ALIASING in _pin(). Copy paste error

v3: Rebased/reworked with flag conflict from negative relocations

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            | 14 ++++++++------
 drivers/gpu/drm/i915/i915_gem.c            | 31 +++++++++++++++++++++++-------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  3 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 12 ++++++++++--
 drivers/gpu/drm/i915/i915_gem_gtt.h        |  6 +++++-
 5 files changed, 49 insertions(+), 17 deletions(-)

Comments

Daniel Vetter Oct. 8, 2014, 1:36 p.m. UTC | #1 
On Tue, Oct 07, 2014 at 06:11:00PM +0100, Michel Thierry wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
>
> The driver currently lets callers pin global, and then tries to do
> things correctly inside the function. Doing so has two downsides:
> 1. It's not possible to exclusively pin to a global, or an aliasing
> address space.
> 2. It's difficult to read, and understand.
>
> The eventual goal when realized should fix both of the issues. This patch
> which should have no functional impact begins to address these issues
> without intentionally breaking things.
>
> v2: Replace PIN_GLOBAL with PIN_ALIASING in _pin(). Copy paste error
>
> v3: Rebased/reworked with flag conflict from negative relocations
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 14 ++++++++------
>  drivers/gpu/drm/i915/i915_gem.c            | 31 +++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  3 ++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 12 ++++++++++--
>  drivers/gpu/drm/i915/i915_gem_gtt.h        |  6 +++++-
>  5 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3c725ec..6b60e90 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2396,11 +2396,13 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
>  void i915_gem_free_object(struct drm_gem_object *obj);
>  void i915_gem_vma_destroy(struct i915_vma *vma);
>
> -#define PIN_MAPPABLE 0x1
> -#define PIN_NONBLOCK 0x2
> -#define PIN_GLOBAL 0x4
> -#define PIN_OFFSET_BIAS 0x8
> -#define PIN_OFFSET_MASK (~4095)
> +#define PIN_MAPPABLE (1<<0)
> +#define PIN_NONBLOCK (1<<1)
> +#define PIN_GLOBAL (1<<2)
> +#define PIN_ALIASING (1<<3)
> +#define PIN_GLOBAL_ALIASED (PIN_ALIASING | PIN_GLOBAL)
> +#define PIN_OFFSET_BIAS (1<<4)
> +#define PIN_OFFSET_MASK (PAGE_MASK)

#define rename should be split out.

>  int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
>       struct i915_address_space *vm,
>       uint32_t alignment,
> @@ -2618,7 +2620,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
>        unsigned flags)
>  {
>   return i915_gem_object_pin(obj, i915_obj_to_ggtt(obj),
> -   alignment, flags | PIN_GLOBAL);
> +   alignment, flags | PIN_GLOBAL_ALIASED);
>  }
>
>  static inline int
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7745d22..dfb20e6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3421,8 +3421,12 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   unsigned long end =
>   flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
>   struct i915_vma *vma;
> + u32 vma_bind_flags = 0;
>   int ret;
>
> + if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
> + flags |= PIN_GLOBAL;
> +
>   fence_size = i915_gem_get_gtt_size(dev,
>     obj->base.size,
>     obj->tiling_mode);
> @@ -3508,9 +3512,11 @@ search_free:
>
>   WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
>
> + if (flags & PIN_GLOBAL_ALIASED)
> + vma_bind_flags = GLOBAL_BIND | ALIASING_BIND;
> +
>   trace_i915_vma_bind(vma, flags);
> - i915_gem_vma_bind(vma, obj->cache_level,
> -  flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> + i915_gem_vma_bind(vma, obj->cache_level, vma_bind_flags);
>
>   return vma;
>
> @@ -3716,9 +3722,14 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>   }
>
>   list_for_each_entry(vma, &obj->vma_list, vma_link)
> - if (drm_mm_node_allocated(&vma->node))
> - i915_gem_vma_bind(vma, cache_level,
> -  obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);
> + if (drm_mm_node_allocated(&vma->node)) {
> + u32 bind_flags = 0;
> + if (obj->has_global_gtt_mapping)
> + bind_flags |= GLOBAL_BIND;
> + if (obj->has_aliasing_ppgtt_mapping)
> + bind_flags |= ALIASING_BIND;
> + i915_gem_vma_bind(vma, cache_level, bind_flags);


We should have a vma_rebind function for use here and in the gtt restore
code.

> + }
>   }
>
>   list_for_each_entry(vma, &obj->vma_list, vma_link)
> @@ -4114,8 +4125,14 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>   return PTR_ERR(vma);
>   }
>
> - if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
> - i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> + if (flags & PIN_GLOBAL_ALIASED) {
> + u32 bind_flags = 0;
> + if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
> + bind_flags |= GLOBAL_BIND;
> + if (flags & PIN_ALIASING && !obj->has_aliasing_ppgtt_mapping)
> + bind_flags |= ALIASING_BIND;
> + i915_gem_vma_bind(vma, obj->cache_level, bind_flags);
> + }
>
>   vma->pin_count++;
>   if (flags & PIN_MAPPABLE)
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 4564988..92191f0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -362,7 +362,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
>   list_first_entry(&target_i915_obj->vma_list,
>   typeof(*vma), vma_link);
>   i915_gem_vma_bind(vma, target_i915_obj->cache_level,
> -  GLOBAL_BIND);
> +  GLOBAL_BIND | ALIASING_BIND);

If you read the comment above then it's clear we actually want a global
binding here specifically. Adding the aliasing_bind flag is confusing.

>   }
>
>   /* Validate that the target is in a valid r/w GPU domain */
> @@ -533,6 +533,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>   flags = 0;
>   if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
>   flags |= PIN_MAPPABLE;
> + /* FIXME: What kind of bind does Chris want? */

This is the place where the aliasing bind should have been. Presuming
follow-up patches indeed rework the binding then this will badly break
snb. Or any gen7 machine booted with i915.enable_ppgtt=1.

>   if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
>   flags |= PIN_GLOBAL;
>   if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0c203f4..d725883 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1336,8 +1336,16 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>   * Unfortunately above, we've just wiped out the mappings
>   * without telling our object about it. So we need to fake it.
>   */
> - obj->has_global_gtt_mapping = 0;
> - i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> + if (obj->has_global_gtt_mapping || obj->has_aliasing_ppgtt_mapping) {
> + u32 bind_flags = 0;
> + if (obj->has_global_gtt_mapping)
> +        bind_flags |= GLOBAL_BIND;
> + if (obj->has_aliasing_ppgtt_mapping)
> +        bind_flags |= ALIASING_BIND;
> + obj->has_global_gtt_mapping = 0;
> + obj->has_aliasing_ppgtt_mapping = 0;
> + i915_gem_vma_bind(vma, obj->cache_level, bind_flags);
> + }
>   }
>
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index d5c14af..5fd7fa9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -155,8 +155,12 @@ struct i915_vma {
>   * setting the valid PTE entries to a reserved scratch page. */
>   void (*unbind_vma)(struct i915_vma *vma);
>   /* Map an object into an address space with the given cache flags. */
> +
> +/* Only use this if you know you want a strictly global binding */
>  #define GLOBAL_BIND (1<<0)
> -#define PTE_READ_ONLY (1<<1)
> +/* Only use this if you know you want a strictly aliased binding */
> +#define ALIASING_BIND (1<<1)
> +#define PTE_READ_ONLY (1<<2)
>   void (*bind_vma)(struct i915_vma *vma,
>   enum i915_cache_level cache_level,
>   u32 flags);
> --
> 2.0.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3c725ec..6b60e90 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2396,11 +2396,13 @@  void i915_init_vm(struct drm_i915_private *dev_priv,
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
-#define PIN_MAPPABLE 0x1
-#define PIN_NONBLOCK 0x2
-#define PIN_GLOBAL 0x4
-#define PIN_OFFSET_BIAS 0x8
-#define PIN_OFFSET_MASK (~4095)
+#define PIN_MAPPABLE	(1<<0)
+#define PIN_NONBLOCK	(1<<1)
+#define PIN_GLOBAL	(1<<2)
+#define PIN_ALIASING	(1<<3)
+#define PIN_GLOBAL_ALIASED (PIN_ALIASING | PIN_GLOBAL)
+#define PIN_OFFSET_BIAS (1<<4)
+#define PIN_OFFSET_MASK (PAGE_MASK)
 int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
 				     struct i915_address_space *vm,
 				     uint32_t alignment,
@@ -2618,7 +2620,7 @@  i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
 		      unsigned flags)
 {
 	return i915_gem_object_pin(obj, i915_obj_to_ggtt(obj),
-				   alignment, flags | PIN_GLOBAL);
+				   alignment, flags | PIN_GLOBAL_ALIASED);
 }
 
 static inline int
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7745d22..dfb20e6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3421,8 +3421,12 @@  i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 	unsigned long end =
 		flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
 	struct i915_vma *vma;
+	u32 vma_bind_flags = 0;
 	int ret;
 
+	if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
+		flags |= PIN_GLOBAL;
+
 	fence_size = i915_gem_get_gtt_size(dev,
 					   obj->base.size,
 					   obj->tiling_mode);
@@ -3508,9 +3512,11 @@  search_free:
 
 	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
 
+	if (flags & PIN_GLOBAL_ALIASED)
+		vma_bind_flags = GLOBAL_BIND | ALIASING_BIND;
+
 	trace_i915_vma_bind(vma, flags);
-	i915_gem_vma_bind(vma, obj->cache_level,
-			  flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
+	i915_gem_vma_bind(vma, obj->cache_level, vma_bind_flags);
 
 	return vma;
 
@@ -3716,9 +3722,14 @@  int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		}
 
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (drm_mm_node_allocated(&vma->node))
-				i915_gem_vma_bind(vma, cache_level,
-						  obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);
+			if (drm_mm_node_allocated(&vma->node)) {
+				u32 bind_flags = 0;
+				if (obj->has_global_gtt_mapping)
+					bind_flags |= GLOBAL_BIND;
+				if (obj->has_aliasing_ppgtt_mapping)
+					bind_flags |= ALIASING_BIND;
+				i915_gem_vma_bind(vma, cache_level, bind_flags);
+			}
 	}
 
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
@@ -4114,8 +4125,14 @@  i915_gem_object_pin(struct drm_i915_gem_object *obj,
 			return PTR_ERR(vma);
 	}
 
-	if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
-		i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
+	if (flags & PIN_GLOBAL_ALIASED) {
+		u32 bind_flags = 0;
+		if (flags & PIN_GLOBAL && !obj->has_global_gtt_mapping)
+			bind_flags |= GLOBAL_BIND;
+		if (flags & PIN_ALIASING && !obj->has_aliasing_ppgtt_mapping)
+			bind_flags |= ALIASING_BIND;
+		i915_gem_vma_bind(vma, obj->cache_level, bind_flags);
+	}
 
 	vma->pin_count++;
 	if (flags & PIN_MAPPABLE)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 4564988..92191f0 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -362,7 +362,7 @@  i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 			list_first_entry(&target_i915_obj->vma_list,
 					 typeof(*vma), vma_link);
 		i915_gem_vma_bind(vma, target_i915_obj->cache_level,
-				  GLOBAL_BIND);
+				  GLOBAL_BIND | ALIASING_BIND);
 	}
 
 	/* Validate that the target is in a valid r/w GPU domain */
@@ -533,6 +533,7 @@  i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	flags = 0;
 	if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
 		flags |= PIN_MAPPABLE;
+	/* FIXME: What kind of bind does Chris want? */
 	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
 		flags |= PIN_GLOBAL;
 	if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0c203f4..d725883 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1336,8 +1336,16 @@  void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 		 * Unfortunately above, we've just wiped out the mappings
 		 * without telling our object about it. So we need to fake it.
 		 */
-		obj->has_global_gtt_mapping = 0;
-		i915_gem_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
+		if (obj->has_global_gtt_mapping || obj->has_aliasing_ppgtt_mapping) {
+			u32 bind_flags = 0;
+			if (obj->has_global_gtt_mapping)
+				        bind_flags |= GLOBAL_BIND;
+			if (obj->has_aliasing_ppgtt_mapping)
+				        bind_flags |= ALIASING_BIND;
+			obj->has_global_gtt_mapping = 0;
+			obj->has_aliasing_ppgtt_mapping = 0;
+			i915_gem_vma_bind(vma, obj->cache_level, bind_flags);
+		}
 	}
 
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d5c14af..5fd7fa9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -155,8 +155,12 @@  struct i915_vma {
 	 * setting the valid PTE entries to a reserved scratch page. */
 	void (*unbind_vma)(struct i915_vma *vma);
 	/* Map an object into an address space with the given cache flags. */
+
+/* Only use this if you know you want a strictly global binding */
 #define GLOBAL_BIND (1<<0)
-#define PTE_READ_ONLY (1<<1)
+/* Only use this if you know you want a strictly aliased binding */
+#define ALIASING_BIND (1<<1)
+#define PTE_READ_ONLY (1<<2)
 	void (*bind_vma)(struct i915_vma *vma,
 			 enum i915_cache_level cache_level,
 			 u32 flags);