[PATCH 0/3] Multiple GGTT views

[PATCH 0/3] Multiple GGTT views

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This series continues what was previously a single patch called "drm/i915:
Infrastructure for supporting different GGTT views per object".

To start with we break the assumption GGTT VMA is at the head of the list to
smoke out any potential hidden assumptions in the code. I have run a few IGTs
here and things look OK.

Trailing the series is a small DocBook section describing the new feature.

And the main patch has it's own change log.

Tvrtko Ursulin (3):
  drm/i915: Stop putting GGTT VMA at the head of the list
  drm/i915: Infrastructure for supporting different GGTT views per
    object
  drm/i915: Documentation for multiple GGTT views.

 Documentation/DocBook/drm.tmpl             |   5 ++
 drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
 drivers/gpu/drm/i915/i915_drv.h            |  46 +++++++++-
 drivers/gpu/drm/i915/i915_gem.c            |  81 ++++++++++--------
 drivers/gpu/drm/i915/i915_gem_context.c    |   4 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 130 ++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 ++++-
 drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +-
 9 files changed, 232 insertions(+), 73 deletions(-)

-- 
2.1.1

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

[PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Multiple GGTT VMAs per object will be introduced in the near future which will
make it impossible to guarantee normal GGTT view is at the head of the list.

Purpose of this patch is to break this assumption straight away so any
potential hidden assumptions in the code base can be bisected to this
simple patch.

For: VIZ-4544
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c     | 10 ++++++----
 drivers/gpu/drm/i915/i915_gem_gtt.c |  8 ++------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9d362d3..c1c1141 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5303,11 +5303,13 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 
 struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 {
+	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
 	struct i915_vma *vma;
 
-	vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link);
-	if (vma->vm != i915_obj_to_ggtt(obj))
-		return NULL;
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (vma->vm == ggtt)
+			return vma;
+	}
 
-	return vma;
+	return NULL;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 171f6ea..ac03a38 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2202,13 +2202,9 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 		BUG();
 	}
 
-	/* Keep GGTT vmas first to make debug easier */
-	if (i915_is_ggtt(vm))
-		list_add(&vma->vma_link, &obj->vma_list);
-	else {
-		list_add_tail(&vma->vma_link, &obj->vma_list);
+	list_add_tail(&vma->vma_link, &obj->vma_list);
+	if (!i915_is_ggtt(vm))
 		i915_ppgtt_get(i915_vm_to_ppgtt(vm));
-	}
 
 	return vma;
 }
-- 
2.1.1

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

[PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
to map objects into the same address space multiple times.

Added a GGTT view concept and linked it with the VMA to distinguish between
multiple instances per address space.

New objects and GEM functions which do not take this new view as a parameter
assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
previous behaviour.

This now means that objects can have multiple VMA entries so the code which
assumed there will only be one also had to be modified.

Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
which is DMA mapped on first VMA instantiation and unmapped on the last one
going away.

v2:
    * Removed per view special casing in i915_gem_ggtt_prepare /
      finish_object in favour of creating and destroying DMA mappings
      on first VMA instantiation and last VMA destruction. (Daniel Vetter)
    * Simplified i915_vma_unbind which does not need to count the GGTT views.
      (Daniel Vetter)
    * Also moved obj->map_and_fenceable reset under the same check.
    * Checkpatch cleanups.

v3:
    * Only retire objects once the last VMA is unbound.

For: VIZ-4544
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |  5 +-
 drivers/gpu/drm/i915/i915_drv.h            | 46 +++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem.c            | 73 ++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_gem_context.c    |  4 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 61 +++++++++++++++++++------
 drivers/gpu/drm/i915/i915_gem_gtt.h        | 22 ++++++++-
 drivers/gpu/drm/i915/i915_gpu_error.c      |  8 +---
 8 files changed, 159 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6c16939..bd08289 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 			seq_puts(m, " (pp");
 		else
 			seq_puts(m, " (g");
-		seq_printf(m, "gtt offset: %08lx, size: %08lx)",
-			   vma->node.start, vma->node.size);
+		seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)",
+			   vma->node.start, vma->node.size,
+			   vma->ggtt_view.type);
 	}
 	if (obj->stolen)
 		seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 049482f..03c8132 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
 #define PIN_GLOBAL 0x4
 #define PIN_OFFSET_BIAS 0x8
 #define PIN_OFFSET_MASK (~4095)
+int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  uint32_t alignment,
+					  uint64_t flags,
+					  const struct i915_ggtt_view *view);
+static inline
 int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
 				     struct i915_address_space *vm,
 				     uint32_t alignment,
-				     uint64_t flags);
+				     uint64_t flags)
+{
+	return i915_gem_object_pin_view(obj, vm, alignment, flags,
+						&i915_ggtt_view_normal);
+}
+
+void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
+		   u32 flags);
 int __must_check i915_vma_unbind(struct i915_vma *vma);
 int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
 void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
@@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
 
 void i915_gem_restore_fences(struct drm_device *dev);
 
+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
+				       struct i915_address_space *vm,
+				       enum i915_ggtt_view_type view);
+static inline
 unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
-				  struct i915_address_space *vm);
+				  struct i915_address_space *vm)
+{
+	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
+}
 bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
 bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
 			struct i915_address_space *vm);
 unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 				struct i915_address_space *vm);
+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  const struct i915_ggtt_view *view);
+static inline
 struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-				     struct i915_address_space *vm);
+				     struct i915_address_space *vm)
+{
+	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
+}
+
+struct i915_vma *
+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
+				       struct i915_address_space *vm,
+				       const struct i915_ggtt_view *view);
+
+static inline
 struct i915_vma *
 i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
-				  struct i915_address_space *vm);
+				  struct i915_address_space *vm)
+{
+	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
+						&i915_ggtt_view_normal);
+}
 
 struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
 static inline bool i915_gem_obj_is_pinned(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 c1c1141..976597c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2087,8 +2087,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			/* For the unbound phase, this should be a no-op! */
 			list_for_each_entry_safe(vma, v,
 						 &obj->vma_list, vma_link)
-				if (i915_vma_unbind(vma))
-					break;
+				i915_vma_unbind(vma);
 
 			if (i915_gem_object_put_pages(obj) == 0)
 				count += obj->base.size >> PAGE_SHIFT;
@@ -2297,17 +2296,14 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 static void
 i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 {
-	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
-	struct i915_address_space *vm;
 	struct i915_vma *vma;
 
 	BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
 	BUG_ON(!obj->active);
 
-	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
-		vma = i915_gem_obj_to_vma(obj, vm);
-		if (vma && !list_empty(&vma->mm_list))
-			list_move_tail(&vma->mm_list, &vm->inactive_list);
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (!list_empty(&vma->mm_list))
+			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
 	}
 
 	intel_fb_obj_flush(obj, true);
@@ -3062,10 +3058,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 	 * cause memory corruption through use-after-free.
 	 */
 
-	/* Throw away the active reference before moving to the unbound list */
-	i915_gem_object_retire(obj);
-
-	if (i915_is_ggtt(vma->vm)) {
+	if (i915_is_ggtt(vma->vm) &&
+	    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
 		i915_gem_object_finish_gtt(obj);
 
 		/* release the fence reg _after_ flushing */
@@ -3079,7 +3073,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 	vma->unbind_vma(vma);
 
 	list_del_init(&vma->mm_list);
-	if (i915_is_ggtt(vma->vm))
+	if (i915_is_ggtt(vma->vm) &&
+	    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
 		obj->map_and_fenceable = false;
 
 	drm_mm_remove_node(&vma->node);
@@ -3088,6 +3083,10 @@ int i915_vma_unbind(struct i915_vma *vma)
 	/* Since the unbound list is global, only move to that list if
 	 * no more VMAs exist. */
 	if (list_empty(&obj->vma_list)) {
+		/* Throw away the active reference before
+		 * moving to the unbound list. */
+		i915_gem_object_retire(obj);
+
 		i915_gem_gtt_finish_object(obj);
 		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
 	}
@@ -3498,7 +3497,8 @@ static struct i915_vma *
 i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 			   struct i915_address_space *vm,
 			   unsigned alignment,
-			   uint64_t flags)
+			   uint64_t flags,
+			   const struct i915_ggtt_view *view)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3548,7 +3548,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 
 	i915_gem_object_pin_pages(obj);
 
-	vma = i915_gem_obj_lookup_or_create_vma(obj, vm);
+	vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
 	if (IS_ERR(vma))
 		goto err_unpin;
 
@@ -3582,7 +3582,7 @@ search_free:
 	list_add_tail(&vma->mm_list, &vm->inactive_list);
 
 	trace_i915_vma_bind(vma, flags);
-	vma->bind_vma(vma, obj->cache_level,
+	i915_vma_bind(vma, obj->cache_level,
 		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
 
 	return vma;
@@ -3790,8 +3790,8 @@ 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))
-				vma->bind_vma(vma, cache_level,
-						vma->bound & GLOBAL_BIND);
+				i915_vma_bind(vma, cache_level,
+					      vma->bound & GLOBAL_BIND);
 	}
 
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
@@ -4144,10 +4144,11 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
 }
 
 int
-i915_gem_object_pin(struct drm_i915_gem_object *obj,
-		    struct i915_address_space *vm,
-		    uint32_t alignment,
-		    uint64_t flags)
+i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
+			 struct i915_address_space *vm,
+			 uint32_t alignment,
+			 uint64_t flags,
+			 const struct i915_ggtt_view *view)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	struct i915_vma *vma;
@@ -4163,7 +4164,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 	if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
 		return -EINVAL;
 
-	vma = i915_gem_obj_to_vma(obj, vm);
+	vma = i915_gem_obj_to_vma_view(obj, vm, view);
 	if (vma) {
 		if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
 			return -EBUSY;
@@ -4173,7 +4174,8 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 			     "bo is already pinned with incorrect alignment:"
 			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
 			     " obj->map_and_fenceable=%d\n",
-			     i915_gem_obj_offset(obj, vm), alignment,
+			     i915_gem_obj_offset_view(obj, vm, view->type),
+			     alignment,
 			     !!(flags & PIN_MAPPABLE),
 			     obj->map_and_fenceable);
 			ret = i915_vma_unbind(vma);
@@ -4186,13 +4188,14 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 
 	bound = vma ? vma->bound : 0;
 	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
-		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
+		vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
+						 flags, view);
 		if (IS_ERR(vma))
 			return PTR_ERR(vma);
 	}
 
 	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND))
-		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
+		i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
 
 	if ((bound ^ vma->bound) & GLOBAL_BIND) {
 		bool mappable, fenceable;
@@ -4528,12 +4531,13 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	intel_runtime_pm_put(dev_priv);
 }
 
-struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-				     struct i915_address_space *vm)
+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
-		if (vma->vm == vm)
+		if (vma->vm == vm && vma->ggtt_view.type == view->type)
 			return vma;
 
 	return NULL;
@@ -5147,8 +5151,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 }
 
 /* All the new VM stuff */
-unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
-				  struct i915_address_space *vm)
+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
+				       struct i915_address_space *vm,
+				       enum i915_ggtt_view_type view)
 {
 	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
 	struct i915_vma *vma;
@@ -5156,7 +5161,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
 	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
 
 	list_for_each_entry(vma, &o->vma_list, vma_link) {
-		if (vma->vm == vm)
+		if (vma->vm == vm && vma->ggtt_view.type == view)
 			return vma->node.start;
 
 	}
@@ -5307,7 +5312,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 	struct i915_vma *vma;
 
 	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		if (vma->vm == ggtt)
+		if (vma->vm != ggtt)
+			continue;
+		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
 			return vma;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5cd2b97..c2e6f2e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -591,8 +591,8 @@ static int do_switch(struct intel_engine_cs *ring,
 
 	vma = i915_gem_obj_to_ggtt(to->legacy_hw_ctx.rcs_state);
 	if (!(vma->bound & GLOBAL_BIND))
-		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level,
-				GLOBAL_BIND);
+		i915_vma_bind(vma, to->legacy_hw_ctx.rcs_state->cache_level,
+			      GLOBAL_BIND);
 
 	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
 		hw_flags |= MI_RESTORE_INHIBIT;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0c25f62..56ac483 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -361,8 +361,8 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	if (unlikely(IS_GEN6(dev) &&
 	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
 	    !(target_vma->bound & GLOBAL_BIND)))
-		target_vma->bind_vma(target_vma, target_i915_obj->cache_level,
-				GLOBAL_BIND);
+		i915_vma_bind(target_vma, target_i915_obj->cache_level,
+			      GLOBAL_BIND);
 
 	/* Validate that the target is in a valid r/w GPU domain */
 	if (unlikely(reloc->write_domain & (reloc->write_domain - 1))) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ac03a38..02eabf9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,8 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+const struct i915_ggtt_view i915_ggtt_view_normal;
+
 static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
 static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
 
@@ -76,7 +78,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 }
 
 
-static void ppgtt_bind_vma(struct i915_vma *vma,
+static void ppgtt_bind_vma(struct i915_vma *vma, struct sg_table *pages,
 			   enum i915_cache_level cache_level,
 			   u32 flags);
 static void ppgtt_unbind_vma(struct i915_vma *vma);
@@ -1200,7 +1202,7 @@ void  i915_ppgtt_release(struct kref *kref)
 }
 
 static void
-ppgtt_bind_vma(struct i915_vma *vma,
+ppgtt_bind_vma(struct i915_vma *vma, struct sg_table *pages,
 	       enum i915_cache_level cache_level,
 	       u32 flags)
 {
@@ -1208,7 +1210,7 @@ ppgtt_bind_vma(struct i915_vma *vma,
 	if (vma->obj->gt_ro)
 		flags |= PTE_READ_ONLY;
 
-	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
+	vma->vm->insert_entries(vma->vm, pages, vma->node.start,
 				cache_level, flags);
 }
 
@@ -1343,7 +1345,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 		 * without telling our object about it. So we need to fake it.
 		 */
 		vma->bound &= ~GLOBAL_BIND;
-		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
+		i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
 	}
 
 
@@ -1529,7 +1531,7 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 }
 
 
-static void i915_ggtt_bind_vma(struct i915_vma *vma,
+static void i915_ggtt_bind_vma(struct i915_vma *vma, struct sg_table *pages,
 			       enum i915_cache_level cache_level,
 			       u32 unused)
 {
@@ -1538,7 +1540,7 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
 		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
-	intel_gtt_insert_sg_entries(vma->obj->pages, entry, flags);
+	intel_gtt_insert_sg_entries(pages, entry, flags);
 	vma->bound = GLOBAL_BIND;
 }
 
@@ -1562,7 +1564,7 @@ static void i915_ggtt_unbind_vma(struct i915_vma *vma)
 	intel_gtt_clear_range(first, size);
 }
 
-static void ggtt_bind_vma(struct i915_vma *vma,
+static void ggtt_bind_vma(struct i915_vma *vma, struct sg_table *pages,
 			  enum i915_cache_level cache_level,
 			  u32 flags)
 {
@@ -1588,7 +1590,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
 		if (!(vma->bound & GLOBAL_BIND) ||
 		    (cache_level != obj->cache_level)) {
-			vma->vm->insert_entries(vma->vm, obj->pages,
+			vma->vm->insert_entries(vma->vm, pages,
 						vma->node.start,
 						cache_level, flags);
 			vma->bound |= GLOBAL_BIND;
@@ -1600,7 +1602,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	     (cache_level != obj->cache_level))) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base,
-					    vma->obj->pages,
+					    pages,
 					    vma->node.start,
 					    cache_level, flags);
 		vma->bound |= LOCAL_BIND;
@@ -2165,7 +2167,8 @@ int i915_gem_gtt_init(struct drm_device *dev)
 }
 
 static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
-					      struct i915_address_space *vm)
+					      struct i915_address_space *vm,
+					      const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
 	if (vma == NULL)
@@ -2176,6 +2179,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 	INIT_LIST_HEAD(&vma->exec_list);
 	vma->vm = vm;
 	vma->obj = obj;
+	vma->ggtt_view = *view;
 
 	switch (INTEL_INFO(vm->dev)->gen) {
 	case 9:
@@ -2210,14 +2214,43 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 }
 
 struct i915_vma *
-i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
-				  struct i915_address_space *vm)
+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
+				       struct i915_address_space *vm,
+				       const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
 
-	vma = i915_gem_obj_to_vma(obj, vm);
+	vma = i915_gem_obj_to_vma_view(obj, vm, view);
 	if (!vma)
-		vma = __i915_gem_vma_create(obj, vm);
+		vma = __i915_gem_vma_create(obj, vm, view);
 
 	return vma;
 }
+
+static inline
+struct sg_table *i915_ggtt_view_pages(struct i915_vma *vma)
+{
+	struct sg_table *pages = ERR_PTR(-EINVAL);
+
+	if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
+		pages = vma->obj->pages;
+	else
+		WARN(1, "Not implemented!\n");
+
+	return pages;
+}
+
+void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
+		   u32 flags)
+{
+	struct sg_table *pages = i915_ggtt_view_pages(vma);
+
+	if (pages && !IS_ERR(pages)) {
+		vma->bind_vma(vma, pages, cache_level, flags);
+
+		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
+			sg_free_table(pages);
+			kfree(pages);
+		}
+	}
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index dd849df..a6a0e27 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -109,7 +109,18 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
 #define GEN8_PPAT_ELLC_OVERRIDE		(0<<2)
 #define GEN8_PPAT(i, x)			((uint64_t) (x) << ((i) * 8))
 
+enum i915_ggtt_view_type {
+	I915_GGTT_VIEW_NORMAL = 0,
+};
+
+struct i915_ggtt_view {
+	enum i915_ggtt_view_type type;
+};
+
+extern const struct i915_ggtt_view i915_ggtt_view_normal;
+
 enum i915_cache_level;
+
 /**
  * A VMA represents a GEM BO that is bound into an address space. Therefore, a
  * VMA's presence cannot be guaranteed before binding, or after unbinding the
@@ -129,6 +140,15 @@ struct i915_vma {
 #define PTE_READ_ONLY	(1<<2)
 	unsigned int bound : 4;
 
+	/**
+	 * Support different GGTT views into the same object.
+	 * This means there can be multiple VMA mappings per object and per VM.
+	 * i915_ggtt_view_type is used to distinguish between those entries.
+	 * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also
+	 * assumed in GEM functions which take no ggtt view parameter.
+	 */
+	struct i915_ggtt_view ggtt_view;
+
 	/** This object's place on the active/inactive lists */
 	struct list_head mm_list;
 
@@ -160,7 +180,7 @@ 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. */
-	void (*bind_vma)(struct i915_vma *vma,
+	void (*bind_vma)(struct i915_vma *vma, struct sg_table *pages,
 			 enum i915_cache_level cache_level,
 			 u32 flags);
 };
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index c4536e1..f97479a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 			break;
 
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
+			if (vma->vm == vm && vma->pin_count > 0)
 				capture_bo(err++, vma);
-				break;
-			}
 	}
 
 	return err - first;
@@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
+			if (vma->vm == vm && vma->pin_count > 0)
 				i++;
-				break;
-			}
 	}
 	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
 
-- 
2.1.1

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

[PATCH 3/3] drm/i915: Documentation for multiple GGTT views.

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A short section describing background, implementation and intended usage.

For: VIZ-4544
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 Documentation/DocBook/drm.tmpl      |  5 +++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 61 +++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 56e2a9b..c975f26 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4033,6 +4033,11 @@ int num_ioctls;</synopsis>
 !Pdrivers/gpu/drm/i915/intel_lrc.c Logical Rings, Logical Ring Contexts and Execlists
 !Idrivers/gpu/drm/i915/intel_lrc.c
       </sect2>
+      <sect2>
+        <title>Global GTT views</title>
+!Pdrivers/gpu/drm/i915/i915_gem_gtt.c Global GTT views
+!Idrivers/gpu/drm/i915/i915_gem_gtt.c
+      </sect2>
     </sect1>
 
     <sect1>
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 02eabf9..633f063 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,67 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+/**
+ * DOC: Global GTT views (GGTT)
+ *
+ * Background and previous state
+ *
+ * Historically objects could exists (be bound) in global GTT space only as
+ * singular instances with a view representing all of the object's backing pages
+ * in a linear fashion. This view will be called a normal view.
+ *
+ * To support multiple views of the same object, where the number of mapped
+ * pages is not equal to the backing store, or where the layout of the pages
+ * is not linear, concept of a GGTT view was added.
+ *
+ * One example of an alternative view is a stereo display driven by a single
+ * image. In this case we would have a framebuffer looking like this
+ * (2x2 pages):
+ *
+ *    12
+ *    34
+ *
+ * Above would represent a normal GGTT view as normally mapped for GPU or CPU
+ * rendering. In contrast, fed to the display engine would be an alternative
+ * view which could look something like this:
+ *
+ *   1212
+ *   3434
+ *
+ * In this example both the size and layout of pages in the alternative view is
+ * different from the normal view.
+ *
+ * Implementation and usage
+ *
+ * GGTT views are implemented using VMAs and are distinguished via enum
+ * i915_ggtt_view_type and struct i915_ggtt_view.
+ *
+ * A new flavour of core GEM functions which work with GGTT bound objects were
+ * added with the _view suffix. They take the struct i915_ggtt_view parameter
+ * encapsulating all metadata required to implement a view.
+ *
+ * As a helper for callers which are only interested in the normal view,
+ * globally const i915_ggtt_view_normal singleton instance exists. All old core
+ * GEM API functions, the ones not taking the view parameter, are operating on,
+ * or with the normal GGTT view.
+ *
+ * Code wanting to add or use a new GGTT view needs to:
+ *
+ * 1. Add a new enum.
+ * 2. Extend the metadata in the i915_ggtt_view structure if required.
+ * 3. Add support to i915_ggtt_view_pages().
+ *
+ * New views are required to build a throw-away scatter-gather table
+ * at the time they are bound into the GGTT space (3rd item from above). This is
+ * intended to simplify the code and is not considered a performance sensitive
+ * path at the moment.
+ *
+ * Core API is designed to have copy semantics which means that passed in
+ * struct i915_ggtt_view does not need to be persistent (left around after
+ * calling the core API functions).
+ *
+ */
+
 const struct i915_ggtt_view i915_ggtt_view_normal;
 
 static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
-- 
2.1.1

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

Re: [PATCH 3/3] drm/i915: Documentation for multiple GGTT views.

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK                 -1              365/366              364/366
SNB                                  450/450              450/450
IVB                                  498/498              498/498
BYT                                  289/289              289/289
HSW                 -1              564/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_plain-flip-ts-check-interruptible      DMESG_WARN(2, M26)PASS(4, M26)      DMESG_WARN(1, M26)
*HSW  igt_kms_plane_plane-panning-bottom-right-pipe-B-plane-2      PASS(2, M19M40)      DMESG_WARN(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Chris Wilson]

On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Multiple GGTT VMAs per object will be introduced in the near future which will
> make it impossible to guarantee normal GGTT view is at the head of the list.
> 
> Purpose of this patch is to break this assumption straight away so any
> potential hidden assumptions in the code base can be bisected to this
> simple patch.

No, please don't. The rationale for putting ggtt first is because it
puts the one vma that every object is likely to use at the front, and
makes it also the first in the list for debugging.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Chris Wilson]

On Wed, Dec 03, 2014 at 02:59:25PM +0000, Tvrtko Ursulin wrote:
> +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> +		   u32 flags)
> +{
> +	struct sg_table *pages = i915_ggtt_view_pages(vma);
> +
> +	if (pages && !IS_ERR(pages)) {
> +		vma->bind_vma(vma, pages, cache_level, flags);
> +
> +		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
> +			sg_free_table(pages);
> +			kfree(pages);
> +		}
> +	}
> +}

Stop. Even if the failure path is impossible with the present
implementation, here you are masking the error only to go and pretend
the binding succeeded.

Don't be lazy, this is a very nasty bug that should be hit during igt -
or else you are not testing well enough.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Tvrtko Ursulin]

On 12/04/2014 09:48 AM, Chris Wilson wrote:
> On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Multiple GGTT VMAs per object will be introduced in the near future which will
>> make it impossible to guarantee normal GGTT view is at the head of the list.
>>
>> Purpose of this patch is to break this assumption straight away so any
>> potential hidden assumptions in the code base can be bisected to this
>> simple patch.
>
> No, please don't. The rationale for putting ggtt first is because it
> puts the one vma that every object is likely to use at the front, and
> makes it also the first in the list for debugging.

This came from talking with Daniel, but I don't understand why every 
object is likely to have a GGTT mapping?

With multiple GGTT views it may happen that only single GGTT VMA exists 
in the list but it is not the one current code would expect. So the idea 
was to break that to flesh out any potential hidden assumptions in the 
code. (I did not find any by just looking though.)

What kind of debugging you have in mind, error capture? Or actually 
inspecting memory of a live kernel?

Regards,

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

Re: [PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Chris Wilson]

On Thu, Dec 04, 2014 at 10:02:19AM +0000, Tvrtko Ursulin wrote:
> 
> On 12/04/2014 09:48 AM, Chris Wilson wrote:
> >On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Multiple GGTT VMAs per object will be introduced in the near future which will
> >>make it impossible to guarantee normal GGTT view is at the head of the list.
> >>
> >>Purpose of this patch is to break this assumption straight away so any
> >>potential hidden assumptions in the code base can be bisected to this
> >>simple patch.
> >
> >No, please don't. The rationale for putting ggtt first is because it
> >puts the one vma that every object is likely to use at the front, and
> >makes it also the first in the list for debugging.
> 
> This came from talking with Daniel, but I don't understand why every
> object is likely to have a GGTT mapping?

Because the userspace usage pattern is such that it is typical that
every object is accessed through the GTT at some point in its life.

It's actually becoming less so as we use alternative mmappings, but it
will remain so for quite some time yet.
 
> With multiple GGTT views it may happen that only single GGTT VMA
> exists in the list but it is not the one current code would expect.
> So the idea was to break that to flesh out any potential hidden
> assumptions in the code. (I did not find any by just looking
> though.)
> 
> What kind of debugging you have in mind, error capture? Or actually
> inspecting memory of a live kernel?

Error capture inspection of memory of a live kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

On 12/04/2014 09:53 AM, Chris Wilson wrote:
> On Wed, Dec 03, 2014 at 02:59:25PM +0000, Tvrtko Ursulin wrote:
>> +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>> +		   u32 flags)
>> +{
>> +	struct sg_table *pages = i915_ggtt_view_pages(vma);
>> +
>> +	if (pages && !IS_ERR(pages)) {
>> +		vma->bind_vma(vma, pages, cache_level, flags);
>> +
>> +		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
>> +			sg_free_table(pages);
>> +			kfree(pages);
>> +		}
>> +	}
>> +}
>
> Stop. Even if the failure path is impossible with the present
> implementation, here you are masking the error only to go and pretend
> the binding succeeded.
>
> Don't be lazy, this is a very nasty bug that should be hit during igt -
> or else you are not testing well enough.

Fair comment, even if a bit too assuming. I actually had this as TODO 
but somehow lost it.

I don't have any ideas on how to provoke this to fail from an IGT? Even 
with future implementations it boils down to a couple of small 
allocations which would have to fail reliably.

But will add the error path for it.

Regards,

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

Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Chris Wilson]

On Thu, Dec 04, 2014 at 10:19:09AM +0000, Tvrtko Ursulin wrote:
> 
> On 12/04/2014 09:53 AM, Chris Wilson wrote:
> >On Wed, Dec 03, 2014 at 02:59:25PM +0000, Tvrtko Ursulin wrote:
> >>+void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> >>+		   u32 flags)
> >>+{
> >>+	struct sg_table *pages = i915_ggtt_view_pages(vma);
> >>+
> >>+	if (pages && !IS_ERR(pages)) {
> >>+		vma->bind_vma(vma, pages, cache_level, flags);
> >>+
> >>+		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
> >>+			sg_free_table(pages);
> >>+			kfree(pages);
> >>+		}
> >>+	}
> >>+}
> >
> >Stop. Even if the failure path is impossible with the present
> >implementation, here you are masking the error only to go and pretend
> >the binding succeeded.
> >
> >Don't be lazy, this is a very nasty bug that should be hit during igt -
> >or else you are not testing well enough.
> 
> Fair comment, even if a bit too assuming. I actually had this as
> TODO but somehow lost it.
> 
> I don't have any ideas on how to provoke this to fail from an IGT?
> Even with future implementations it boils down to a couple of small
> allocations which would have to fail reliably.

We have quite a few thrash tests now that are fairly good at getting
even the small allocations to fail.

What we don't have is a single-fd, multi-ctx thrash test (well except
for some GL tests...)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Tvrtko Ursulin]

On 12/04/2014 10:17 AM, Chris Wilson wrote:
> On Thu, Dec 04, 2014 at 10:02:19AM +0000, Tvrtko Ursulin wrote:
>>
>> On 12/04/2014 09:48 AM, Chris Wilson wrote:
>>> On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Multiple GGTT VMAs per object will be introduced in the near future which will
>>>> make it impossible to guarantee normal GGTT view is at the head of the list.
>>>>
>>>> Purpose of this patch is to break this assumption straight away so any
>>>> potential hidden assumptions in the code base can be bisected to this
>>>> simple patch.
>>>
>>> No, please don't. The rationale for putting ggtt first is because it
>>> puts the one vma that every object is likely to use at the front, and
>>> makes it also the first in the list for debugging.
>>
>> This came from talking with Daniel, but I don't understand why every
>> object is likely to have a GGTT mapping?
>
> Because the userspace usage pattern is such that it is typical that
> every object is accessed through the GTT at some point in its life.
>
> It's actually becoming less so as we use alternative mmappings, but it
> will remain so for quite some time yet.
>
>> With multiple GGTT views it may happen that only single GGTT VMA
>> exists in the list but it is not the one current code would expect.
>> So the idea was to break that to flesh out any potential hidden
>> assumptions in the code. (I did not find any by just looking
>> though.)
>>
>> What kind of debugging you have in mind, error capture? Or actually
>> inspecting memory of a live kernel?
>
> Error capture inspection of memory of a live kernel.

So personally you don't think it should be of any concern if a GGTT VMA 
is at the head of the list, but it is not the same GGTT VMA which you 
would find there in majority of cases?

Regards,

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

Re: [PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Chris Wilson]

On Thu, Dec 04, 2014 at 10:30:30AM +0000, Tvrtko Ursulin wrote:
> 
> On 12/04/2014 10:17 AM, Chris Wilson wrote:
> >On Thu, Dec 04, 2014 at 10:02:19AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 12/04/2014 09:48 AM, Chris Wilson wrote:
> >>>On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote:
> >>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>>Multiple GGTT VMAs per object will be introduced in the near future which will
> >>>>make it impossible to guarantee normal GGTT view is at the head of the list.
> >>>>
> >>>>Purpose of this patch is to break this assumption straight away so any
> >>>>potential hidden assumptions in the code base can be bisected to this
> >>>>simple patch.
> >>>
> >>>No, please don't. The rationale for putting ggtt first is because it
> >>>puts the one vma that every object is likely to use at the front, and
> >>>makes it also the first in the list for debugging.
> >>
> >>This came from talking with Daniel, but I don't understand why every
> >>object is likely to have a GGTT mapping?
> >
> >Because the userspace usage pattern is such that it is typical that
> >every object is accessed through the GTT at some point in its life.
> >
> >It's actually becoming less so as we use alternative mmappings, but it
> >will remain so for quite some time yet.
> >
> >>With multiple GGTT views it may happen that only single GGTT VMA
> >>exists in the list but it is not the one current code would expect.
> >>So the idea was to break that to flesh out any potential hidden
> >>assumptions in the code. (I did not find any by just looking
> >>though.)
> >>
> >>What kind of debugging you have in mind, error capture? Or actually
> >>inspecting memory of a live kernel?
> >
> >Error capture inspection of memory of a live kernel.
> 
> So personally you don't think it should be of any concern if a GGTT
> VMA is at the head of the list, but it is not the same GGTT VMA
> which you would find there in majority of cases?

Actually, regarding error capture it is sorting of the vm list that is
important - so a different list.

I am still confused about why having different GGTT views makes it
difficult putting the normal GGTT vm first. I would very much like the
standard GGTT view remain first, and I would actually like the MRU
ppgtt last (so that the two most common uses are easily found).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Tvrtko Ursulin]

On 12/04/2014 10:39 AM, Chris Wilson wrote:
> On Thu, Dec 04, 2014 at 10:30:30AM +0000, Tvrtko Ursulin wrote:
>> So personally you don't think it should be of any concern if a GGTT
>> VMA is at the head of the list, but it is not the same GGTT VMA
>> which you would find there in majority of cases?
>
> Actually, regarding error capture it is sorting of the vm list that is
> important - so a different list.
>
> I am still confused about why having different GGTT views makes it
> difficult putting the normal GGTT vm first. I would very much like the
> standard GGTT view remain first, and I would actually like the MRU
> ppgtt last (so that the two most common uses are easily found).

It is not difficult, but it is not guaranteed that, when there is a 
single GGTT vma on the list, it is a normal one.

+ Daniel since this is at least how I interpreted our discussion about 
this. Maybe I got it wrong, who knows.

Regards,

Tvrtko




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

Re: [PATCH 1/3] drm/i915: Stop putting GGTT VMA at the head of the list

[Daniel Vetter]

On Thu, Dec 04, 2014 at 09:48:11AM +0000, Chris Wilson wrote:
> On Wed, Dec 03, 2014 at 02:59:24PM +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Multiple GGTT VMAs per object will be introduced in the near future which will
> > make it impossible to guarantee normal GGTT view is at the head of the list.
> > 
> > Purpose of this patch is to break this assumption straight away so any
> > potential hidden assumptions in the code base can be bisected to this
> > simple patch.
> 
> No, please don't. The rationale for putting ggtt first is because it
> puts the one vma that every object is likely to use at the front, and
> makes it also the first in the list for debugging.

With hindsight I think this was just premature optimization (and way too
many obj->vma lookups to boot). Imo we should just fix the remaining
spurious obj->vma lookups we have and if there's still a problem then we
need to have actual benchmark data or profiles to support this trick.

Hence I pulled this patch in, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Thu, Dec 04, 2014 at 10:26:14AM +0000, Chris Wilson wrote:
> On Thu, Dec 04, 2014 at 10:19:09AM +0000, Tvrtko Ursulin wrote:
> > 
> > On 12/04/2014 09:53 AM, Chris Wilson wrote:
> > >On Wed, Dec 03, 2014 at 02:59:25PM +0000, Tvrtko Ursulin wrote:
> > >>+void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> > >>+		   u32 flags)
> > >>+{
> > >>+	struct sg_table *pages = i915_ggtt_view_pages(vma);
> > >>+
> > >>+	if (pages && !IS_ERR(pages)) {
> > >>+		vma->bind_vma(vma, pages, cache_level, flags);
> > >>+
> > >>+		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
> > >>+			sg_free_table(pages);
> > >>+			kfree(pages);
> > >>+		}
> > >>+	}
> > >>+}
> > >
> > >Stop. Even if the failure path is impossible with the present
> > >implementation, here you are masking the error only to go and pretend
> > >the binding succeeded.
> > >
> > >Don't be lazy, this is a very nasty bug that should be hit during igt -
> > >or else you are not testing well enough.
> > 
> > Fair comment, even if a bit too assuming. I actually had this as
> > TODO but somehow lost it.
> > 
> > I don't have any ideas on how to provoke this to fail from an IGT?
> > Even with future implementations it boils down to a couple of small
> > allocations which would have to fail reliably.
> 
> We have quite a few thrash tests now that are fairly good at getting
> even the small allocations to fail.
> 
> What we don't have is a single-fd, multi-ctx thrash test (well except
> for some GL tests...)

But none of these tests result in permanent memory failures (only the
occasional ioctl restart when waiting for gpu rendering). And sg table
alloc only recurses through the shrinker so that can't happen. So I think
we just have to get by with review.

We did have issues with sg table allocations in stress tests though,
before we've added the recursive shrinker locking, hence sg table alloc
can indeed go south.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

On 12/04/2014 10:59 AM, Daniel Vetter wrote:
> On Thu, Dec 04, 2014 at 10:26:14AM +0000, Chris Wilson wrote:
>> On Thu, Dec 04, 2014 at 10:19:09AM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 12/04/2014 09:53 AM, Chris Wilson wrote:
>>>> On Wed, Dec 03, 2014 at 02:59:25PM +0000, Tvrtko Ursulin wrote:
>>>>> +void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>>>>> +		   u32 flags)
>>>>> +{
>>>>> +	struct sg_table *pages = i915_ggtt_view_pages(vma);
>>>>> +
>>>>> +	if (pages && !IS_ERR(pages)) {
>>>>> +		vma->bind_vma(vma, pages, cache_level, flags);
>>>>> +
>>>>> +		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
>>>>> +			sg_free_table(pages);
>>>>> +			kfree(pages);
>>>>> +		}
>>>>> +	}
>>>>> +}
>>>>
>>>> Stop. Even if the failure path is impossible with the present
>>>> implementation, here you are masking the error only to go and pretend
>>>> the binding succeeded.
>>>>
>>>> Don't be lazy, this is a very nasty bug that should be hit during igt -
>>>> or else you are not testing well enough.
>>>
>>> Fair comment, even if a bit too assuming. I actually had this as
>>> TODO but somehow lost it.
>>>
>>> I don't have any ideas on how to provoke this to fail from an IGT?
>>> Even with future implementations it boils down to a couple of small
>>> allocations which would have to fail reliably.
>>
>> We have quite a few thrash tests now that are fairly good at getting
>> even the small allocations to fail.
>>
>> What we don't have is a single-fd, multi-ctx thrash test (well except
>> for some GL tests...)
>
> But none of these tests result in permanent memory failures (only the
> occasional ioctl restart when waiting for gpu rendering). And sg table
> alloc only recurses through the shrinker so that can't happen. So I think
> we just have to get by with review.
>
> We did have issues with sg table allocations in stress tests though,
> before we've added the recursive shrinker locking, hence sg table alloc
> can indeed go south.

I looked at propagating errors from i915_vma_bind() out to callers and 
it is mostly all fine apart from the i915_gem_restore_gtt_mappings 
during i915_drm_resume.

I don't see how this is fixable apart by going back and having sgls stay 
around for the lifetime of their VMAs. It shouldn't be such a big deal - 
they are not so big even with non-coalesced entries.

Thoughts?

Regards,

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

Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Chris Wilson]

On Thu, Dec 04, 2014 at 12:17:51PM +0000, Tvrtko Ursulin wrote:
> On 12/04/2014 10:59 AM, Daniel Vetter wrote:
> >On Thu, Dec 04, 2014 at 10:26:14AM +0000, Chris Wilson wrote:
> >>On Thu, Dec 04, 2014 at 10:19:09AM +0000, Tvrtko Ursulin wrote:
> >>>
> >>>On 12/04/2014 09:53 AM, Chris Wilson wrote:
> >>>>On Wed, Dec 03, 2014 at 02:59:25PM +0000, Tvrtko Ursulin wrote:
> >>>>>+void i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> >>>>>+		   u32 flags)
> >>>>>+{
> >>>>>+	struct sg_table *pages = i915_ggtt_view_pages(vma);
> >>>>>+
> >>>>>+	if (pages && !IS_ERR(pages)) {
> >>>>>+		vma->bind_vma(vma, pages, cache_level, flags);
> >>>>>+
> >>>>>+		if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
> >>>>>+			sg_free_table(pages);
> >>>>>+			kfree(pages);
> >>>>>+		}
> >>>>>+	}
> >>>>>+}
> >>>>
> >>>>Stop. Even if the failure path is impossible with the present
> >>>>implementation, here you are masking the error only to go and pretend
> >>>>the binding succeeded.
> >>>>
> >>>>Don't be lazy, this is a very nasty bug that should be hit during igt -
> >>>>or else you are not testing well enough.
> >>>
> >>>Fair comment, even if a bit too assuming. I actually had this as
> >>>TODO but somehow lost it.
> >>>
> >>>I don't have any ideas on how to provoke this to fail from an IGT?
> >>>Even with future implementations it boils down to a couple of small
> >>>allocations which would have to fail reliably.
> >>
> >>We have quite a few thrash tests now that are fairly good at getting
> >>even the small allocations to fail.
> >>
> >>What we don't have is a single-fd, multi-ctx thrash test (well except
> >>for some GL tests...)
> >
> >But none of these tests result in permanent memory failures (only the
> >occasional ioctl restart when waiting for gpu rendering). And sg table
> >alloc only recurses through the shrinker so that can't happen. So I think
> >we just have to get by with review.
> >
> >We did have issues with sg table allocations in stress tests though,
> >before we've added the recursive shrinker locking, hence sg table alloc
> >can indeed go south.
> 
> I looked at propagating errors from i915_vma_bind() out to callers
> and it is mostly all fine apart from the
> i915_gem_restore_gtt_mappings during i915_drm_resume.
> 
> I don't see how this is fixable apart by going back and having sgls
> stay around for the lifetime of their VMAs. It shouldn't be such a
> big deal - they are not so big even with non-coalesced entries.

The good news is that for a rebind, you already have the sg lists
already allocated and converted to dma addresses, and so you can just do
something like:

static inline void
i915_vma_rebind(struct i915_vma *vma, enum i915_cache_level cache_level)
{
        unsigned rebind;

        rebind = vma->bound;
        vma->bound = 0;

        WARN_ON(vma->bind_vma(vma, cache_level, rebind));
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Thu, Dec 4, 2014 at 1:17 PM, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
> I looked at propagating errors from i915_vma_bind() out to callers and it is
> mostly all fine apart from the i915_gem_restore_gtt_mappings during
> i915_drm_resume.

Bummer.

> I don't see how this is fixable apart by going back and having sgls stay
> around for the lifetime of their VMAs. It shouldn't be such a big deal -
> they are not so big even with non-coalesced entries.
>
> Thoughts?

I guess we'll just have to eat the additional pointer in vma structs
and that's it. If you want to go fancy we could have a new subtype:

struct i915_vma_special_view {
    struct i915_vma base;
    struct sg_table *sgt;
}

And then typecast accordingly for non-normal views. Feels a bit like
overkill though, at least until we start using vmas for other crazy
things which need special handling.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 0/2] Multiple GGTT views

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This series continues what was previously a single patch called "drm/i915:
Infrastructure for supporting different GGTT views per object".

v2:
 * One less patch due early prep work getting merged.
 * Main patch reworked to keep the pages pointer around for the lifetime of the
   VMA. More detailed changelog in the patch itself.

Tvrtko Ursulin (2):
  drm/i915: Infrastructure for supporting different GGTT views per
    object
  drm/i915: Documentation for multiple GGTT views

 Documentation/DocBook/drm.tmpl             |   5 ++
 drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
 drivers/gpu/drm/i915/i915_drv.h            |  46 +++++++++-
 drivers/gpu/drm/i915/i915_gem.c            | 101 +++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_context.c    |  11 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   9 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 130 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 +++++
 drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +-
 9 files changed, 271 insertions(+), 66 deletions(-)

-- 
2.1.1

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

[PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
to map objects into the same address space multiple times.

Added a GGTT view concept and linked it with the VMA to distinguish between
multiple instances per address space.

New objects and GEM functions which do not take this new view as a parameter
assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
previous behaviour.

This now means that objects can have multiple VMA entries so the code which
assumed there will only be one also had to be modified.

Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
which is DMA mapped on first VMA instantiation and unmapped on the last one
going away.

v2:
    * Removed per view special casing in i915_gem_ggtt_prepare /
      finish_object in favour of creating and destroying DMA mappings
      on first VMA instantiation and last VMA destruction. (Daniel Vetter)
    * Simplified i915_vma_unbind which does not need to count the GGTT views.
      (Daniel Vetter)
    * Also moved obj->map_and_fenceable reset under the same check.
    * Checkpatch cleanups.

v3:
    * Only retire objects once the last VMA is unbound.

v4:
    * Keep scatter-gather table for alternative views persistent for the
      lifetime of the VMA.
    * Propagate binding errors to callers and handle appropriately.

For: VIZ-4544
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
 drivers/gpu/drm/i915/i915_drv.h            |  46 +++++++++++--
 drivers/gpu/drm/i915/i915_gem.c            | 101 ++++++++++++++++++-----------
 drivers/gpu/drm/i915/i915_gem_context.c    |  11 +++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   9 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |  70 +++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 +++++++
 drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +--
 8 files changed, 206 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 6c16939..bd08289 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 			seq_puts(m, " (pp");
 		else
 			seq_puts(m, " (g");
-		seq_printf(m, "gtt offset: %08lx, size: %08lx)",
-			   vma->node.start, vma->node.size);
+		seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)",
+			   vma->node.start, vma->node.size,
+			   vma->ggtt_view.type);
 	}
 	if (obj->stolen)
 		seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 049482f..b2f6f7d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
 #define PIN_GLOBAL 0x4
 #define PIN_OFFSET_BIAS 0x8
 #define PIN_OFFSET_MASK (~4095)
+int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  uint32_t alignment,
+					  uint64_t flags,
+					  const struct i915_ggtt_view *view);
+static inline
 int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
 				     struct i915_address_space *vm,
 				     uint32_t alignment,
-				     uint64_t flags);
+				     uint64_t flags)
+{
+	return i915_gem_object_pin_view(obj, vm, alignment, flags,
+						&i915_ggtt_view_normal);
+}
+
+int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
+		  u32 flags);
 int __must_check i915_vma_unbind(struct i915_vma *vma);
 int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
 void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
@@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
 
 void i915_gem_restore_fences(struct drm_device *dev);
 
+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
+				       struct i915_address_space *vm,
+				       enum i915_ggtt_view_type view);
+static inline
 unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
-				  struct i915_address_space *vm);
+				  struct i915_address_space *vm)
+{
+	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
+}
 bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
 bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
 			struct i915_address_space *vm);
 unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 				struct i915_address_space *vm);
+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  const struct i915_ggtt_view *view);
+static inline
 struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-				     struct i915_address_space *vm);
+				     struct i915_address_space *vm)
+{
+	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
+}
+
+struct i915_vma *
+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
+				       struct i915_address_space *vm,
+				       const struct i915_ggtt_view *view);
+
+static inline
 struct i915_vma *
 i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
-				  struct i915_address_space *vm);
+				  struct i915_address_space *vm)
+{
+	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
+						&i915_ggtt_view_normal);
+}
 
 struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
 static inline bool i915_gem_obj_is_pinned(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 c1c1141..9244f0b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2087,8 +2087,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			/* For the unbound phase, this should be a no-op! */
 			list_for_each_entry_safe(vma, v,
 						 &obj->vma_list, vma_link)
-				if (i915_vma_unbind(vma))
-					break;
+				i915_vma_unbind(vma);
 
 			if (i915_gem_object_put_pages(obj) == 0)
 				count += obj->base.size >> PAGE_SHIFT;
@@ -2297,17 +2296,14 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 static void
 i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 {
-	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
-	struct i915_address_space *vm;
 	struct i915_vma *vma;
 
 	BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
 	BUG_ON(!obj->active);
 
-	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
-		vma = i915_gem_obj_to_vma(obj, vm);
-		if (vma && !list_empty(&vma->mm_list))
-			list_move_tail(&vma->mm_list, &vm->inactive_list);
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (!list_empty(&vma->mm_list))
+			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
 	}
 
 	intel_fb_obj_flush(obj, true);
@@ -3062,10 +3058,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 	 * cause memory corruption through use-after-free.
 	 */
 
-	/* Throw away the active reference before moving to the unbound list */
-	i915_gem_object_retire(obj);
-
-	if (i915_is_ggtt(vma->vm)) {
+	if (i915_is_ggtt(vma->vm) &&
+	    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
 		i915_gem_object_finish_gtt(obj);
 
 		/* release the fence reg _after_ flushing */
@@ -3079,8 +3073,15 @@ int i915_vma_unbind(struct i915_vma *vma)
 	vma->unbind_vma(vma);
 
 	list_del_init(&vma->mm_list);
-	if (i915_is_ggtt(vma->vm))
-		obj->map_and_fenceable = false;
+	if (i915_is_ggtt(vma->vm)) {
+		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
+			obj->map_and_fenceable = false;
+		} else if (vma->ggtt_view.pages) {
+			sg_free_table(vma->ggtt_view.pages);
+			kfree(vma->ggtt_view.pages);
+			vma->ggtt_view.pages = NULL;
+		}
+	}
 
 	drm_mm_remove_node(&vma->node);
 	i915_gem_vma_destroy(vma);
@@ -3088,6 +3089,10 @@ int i915_vma_unbind(struct i915_vma *vma)
 	/* Since the unbound list is global, only move to that list if
 	 * no more VMAs exist. */
 	if (list_empty(&obj->vma_list)) {
+		/* Throw away the active reference before
+		 * moving to the unbound list. */
+		i915_gem_object_retire(obj);
+
 		i915_gem_gtt_finish_object(obj);
 		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
 	}
@@ -3498,7 +3503,8 @@ static struct i915_vma *
 i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 			   struct i915_address_space *vm,
 			   unsigned alignment,
-			   uint64_t flags)
+			   uint64_t flags,
+			   const struct i915_ggtt_view *view)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3548,7 +3554,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 
 	i915_gem_object_pin_pages(obj);
 
-	vma = i915_gem_obj_lookup_or_create_vma(obj, vm);
+	vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
 	if (IS_ERR(vma))
 		goto err_unpin;
 
@@ -3578,15 +3584,19 @@ search_free:
 	if (ret)
 		goto err_remove_node;
 
+	trace_i915_vma_bind(vma, flags);
+	ret = i915_vma_bind(vma, obj->cache_level,
+			    flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
+	if (ret)
+		goto err_finish_gtt;
+
 	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_add_tail(&vma->mm_list, &vm->inactive_list);
 
-	trace_i915_vma_bind(vma, flags);
-	vma->bind_vma(vma, obj->cache_level,
-		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
-
 	return vma;
 
+err_finish_gtt:
+	i915_gem_gtt_finish_object(obj);
 err_remove_node:
 	drm_mm_remove_node(&vma->node);
 err_free_vma:
@@ -3789,9 +3799,12 @@ 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))
-				vma->bind_vma(vma, cache_level,
-						vma->bound & GLOBAL_BIND);
+			if (drm_mm_node_allocated(&vma->node)) {
+				ret = i915_vma_bind(vma, cache_level,
+						    vma->bound & GLOBAL_BIND);
+				if (ret);
+					return ret;
+			}
 	}
 
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
@@ -4144,10 +4157,11 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
 }
 
 int
-i915_gem_object_pin(struct drm_i915_gem_object *obj,
-		    struct i915_address_space *vm,
-		    uint32_t alignment,
-		    uint64_t flags)
+i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
+			 struct i915_address_space *vm,
+			 uint32_t alignment,
+			 uint64_t flags,
+			 const struct i915_ggtt_view *view)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	struct i915_vma *vma;
@@ -4163,7 +4177,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 	if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
 		return -EINVAL;
 
-	vma = i915_gem_obj_to_vma(obj, vm);
+	vma = i915_gem_obj_to_vma_view(obj, vm, view);
 	if (vma) {
 		if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
 			return -EBUSY;
@@ -4173,7 +4187,8 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 			     "bo is already pinned with incorrect alignment:"
 			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
 			     " obj->map_and_fenceable=%d\n",
-			     i915_gem_obj_offset(obj, vm), alignment,
+			     i915_gem_obj_offset_view(obj, vm, view->type),
+			     alignment,
 			     !!(flags & PIN_MAPPABLE),
 			     obj->map_and_fenceable);
 			ret = i915_vma_unbind(vma);
@@ -4186,13 +4201,17 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 
 	bound = vma ? vma->bound : 0;
 	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
-		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
+		vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
+						 flags, view);
 		if (IS_ERR(vma))
 			return PTR_ERR(vma);
 	}
 
-	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND))
-		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
+	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) {
+		ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
+		if (ret)
+			return ret;
+	}
 
 	if ((bound ^ vma->bound) & GLOBAL_BIND) {
 		bool mappable, fenceable;
@@ -4528,12 +4547,13 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	intel_runtime_pm_put(dev_priv);
 }
 
-struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-				     struct i915_address_space *vm)
+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
-		if (vma->vm == vm)
+		if (vma->vm == vm && vma->ggtt_view.type == view->type)
 			return vma;
 
 	return NULL;
@@ -5147,8 +5167,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 }
 
 /* All the new VM stuff */
-unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
-				  struct i915_address_space *vm)
+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
+				       struct i915_address_space *vm,
+				       enum i915_ggtt_view_type view)
 {
 	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
 	struct i915_vma *vma;
@@ -5156,7 +5177,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
 	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
 
 	list_for_each_entry(vma, &o->vma_list, vma_link) {
-		if (vma->vm == vm)
+		if (vma->vm == vm && vma->ggtt_view.type == view)
 			return vma->node.start;
 
 	}
@@ -5307,7 +5328,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 	struct i915_vma *vma;
 
 	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		if (vma->vm == ggtt)
+		if (vma->vm != ggtt)
+			continue;
+		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
 			return vma;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5cd2b97..4ef8815 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -590,9 +590,14 @@ static int do_switch(struct intel_engine_cs *ring,
 		goto unpin_out;
 
 	vma = i915_gem_obj_to_ggtt(to->legacy_hw_ctx.rcs_state);
-	if (!(vma->bound & GLOBAL_BIND))
-		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level,
-				GLOBAL_BIND);
+	if (!(vma->bound & GLOBAL_BIND)) {
+		ret = i915_vma_bind(vma,
+				    to->legacy_hw_ctx.rcs_state->cache_level,
+				    GLOBAL_BIND);
+		/* This shouldn't ever fail. */
+		if (WARN_ONCE(ret, "GGTT context bind failed!"))
+			goto unpin_out;
+	}
 
 	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
 		hw_flags |= MI_RESTORE_INHIBIT;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0c25f62..3927d93 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -360,9 +360,12 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	 * through the ppgtt for non_secure batchbuffers. */
 	if (unlikely(IS_GEN6(dev) &&
 	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
-	    !(target_vma->bound & GLOBAL_BIND)))
-		target_vma->bind_vma(target_vma, target_i915_obj->cache_level,
-				GLOBAL_BIND);
+	    !(target_vma->bound & GLOBAL_BIND))) {
+		ret = i915_vma_bind(target_vma, target_i915_obj->cache_level,
+				    GLOBAL_BIND);
+		if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!"))
+			return ret;
+	}
 
 	/* Validate that the target is in a valid r/w GPU domain */
 	if (unlikely(reloc->write_domain & (reloc->write_domain - 1))) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ac03a38..73c1c0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,8 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+const struct i915_ggtt_view i915_ggtt_view_normal;
+
 static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
 static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
 
@@ -1341,9 +1343,12 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 		/* The bind_vma code tries to be smart about tracking mappings.
 		 * Unfortunately above, we've just wiped out the mappings
 		 * without telling our object about it. So we need to fake it.
+		 *
+		 * Bind is not expected to fail since this is only called on
+		 * resume and assumption is all requirements exist already.
 		 */
 		vma->bound &= ~GLOBAL_BIND;
-		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
+		WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND));
 	}
 
 
@@ -1538,7 +1543,7 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
 		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
-	intel_gtt_insert_sg_entries(vma->obj->pages, entry, flags);
+	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
 	vma->bound = GLOBAL_BIND;
 }
 
@@ -1588,7 +1593,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
 		if (!(vma->bound & GLOBAL_BIND) ||
 		    (cache_level != obj->cache_level)) {
-			vma->vm->insert_entries(vma->vm, obj->pages,
+			vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
 						vma->node.start,
 						cache_level, flags);
 			vma->bound |= GLOBAL_BIND;
@@ -1600,7 +1605,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	     (cache_level != obj->cache_level))) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base,
-					    vma->obj->pages,
+					    vma->ggtt_view.pages,
 					    vma->node.start,
 					    cache_level, flags);
 		vma->bound |= LOCAL_BIND;
@@ -2165,7 +2170,8 @@ int i915_gem_gtt_init(struct drm_device *dev)
 }
 
 static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
-					      struct i915_address_space *vm)
+					      struct i915_address_space *vm,
+					      const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
 	if (vma == NULL)
@@ -2176,6 +2182,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 	INIT_LIST_HEAD(&vma->exec_list);
 	vma->vm = vm;
 	vma->obj = obj;
+	vma->ggtt_view = *view;
 
 	switch (INTEL_INFO(vm->dev)->gen) {
 	case 9:
@@ -2210,14 +2217,59 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 }
 
 struct i915_vma *
-i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
-				  struct i915_address_space *vm)
+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
+				       struct i915_address_space *vm,
+				       const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
 
-	vma = i915_gem_obj_to_vma(obj, vm);
+	vma = i915_gem_obj_to_vma_view(obj, vm, view);
 	if (!vma)
-		vma = __i915_gem_vma_create(obj, vm);
+		vma = __i915_gem_vma_create(obj, vm, view);
 
 	return vma;
 }
+
+static inline
+int i915_get_vma_pages(struct i915_vma *vma)
+{
+	if (vma->ggtt_view.pages)
+		return 0;
+
+	if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
+		vma->ggtt_view.pages = vma->obj->pages;
+	else
+		WARN_ONCE(1, "GGTT view %u not implemented!\n",
+			  vma->ggtt_view.type);
+
+	if (!vma->ggtt_view.pages) {
+		DRM_ERROR("Failed to get pages for VMA view type %u!\n",
+			  vma->ggtt_view.type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address space.
+ * @vma: VMA to map
+ * @cache_level: mapping cache level
+ * @flags: flags like global or local mapping
+ *
+ * DMA addresses are taken from the scatter-gather table of this object (or of
+ * this VMA in case of non-default GGTT views) and PTE entries set up.
+ * Note that DMA addresses are also the only part of the SG table we care about.
+ */
+int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
+		  u32 flags)
+{
+	int ret = i915_get_vma_pages(vma);
+
+	if (ret)
+		return ret;
+
+	vma->bind_vma(vma, cache_level, flags);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index dd849df..e377c7d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -109,7 +109,20 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
 #define GEN8_PPAT_ELLC_OVERRIDE		(0<<2)
 #define GEN8_PPAT(i, x)			((uint64_t) (x) << ((i) * 8))
 
+enum i915_ggtt_view_type {
+	I915_GGTT_VIEW_NORMAL = 0,
+};
+
+struct i915_ggtt_view {
+	enum i915_ggtt_view_type type;
+
+	struct sg_table *pages;
+};
+
+extern const struct i915_ggtt_view i915_ggtt_view_normal;
+
 enum i915_cache_level;
+
 /**
  * A VMA represents a GEM BO that is bound into an address space. Therefore, a
  * VMA's presence cannot be guaranteed before binding, or after unbinding the
@@ -129,6 +142,15 @@ struct i915_vma {
 #define PTE_READ_ONLY	(1<<2)
 	unsigned int bound : 4;
 
+	/**
+	 * Support different GGTT views into the same object.
+	 * This means there can be multiple VMA mappings per object and per VM.
+	 * i915_ggtt_view_type is used to distinguish between those entries.
+	 * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also
+	 * assumed in GEM functions which take no ggtt view parameter.
+	 */
+	struct i915_ggtt_view ggtt_view;
+
 	/** This object's place on the active/inactive lists */
 	struct list_head mm_list;
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index c4536e1..f97479a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 			break;
 
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
+			if (vma->vm == vm && vma->pin_count > 0)
 				capture_bo(err++, vma);
-				break;
-			}
 	}
 
 	return err - first;
@@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
+			if (vma->vm == vm && vma->pin_count > 0)
 				i++;
-				break;
-			}
 	}
 	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
 
-- 
2.1.1

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

[PATCH 2/2] drm/i915: Documentation for multiple GGTT views

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A short section describing background, implementation and intended usage.

For: VIZ-4544
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 Documentation/DocBook/drm.tmpl      |  5 ++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 60 +++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 56e2a9b..c975f26 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4033,6 +4033,11 @@ int num_ioctls;</synopsis>
 !Pdrivers/gpu/drm/i915/intel_lrc.c Logical Rings, Logical Ring Contexts and Execlists
 !Idrivers/gpu/drm/i915/intel_lrc.c
       </sect2>
+      <sect2>
+        <title>Global GTT views</title>
+!Pdrivers/gpu/drm/i915/i915_gem_gtt.c Global GTT views
+!Idrivers/gpu/drm/i915/i915_gem_gtt.c
+      </sect2>
     </sect1>
 
     <sect1>
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 73c1c0b..0953369 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,66 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+/**
+ * DOC: Global GTT views (GGTT)
+ *
+ * Background and previous state
+ *
+ * Historically objects could exists (be bound) in global GTT space only as
+ * singular instances with a view representing all of the object's backing pages
+ * in a linear fashion. This view will be called a normal view.
+ *
+ * To support multiple views of the same object, where the number of mapped
+ * pages is not equal to the backing store, or where the layout of the pages
+ * is not linear, concept of a GGTT view was added.
+ *
+ * One example of an alternative view is a stereo display driven by a single
+ * image. In this case we would have a framebuffer looking like this
+ * (2x2 pages):
+ *
+ *    12
+ *    34
+ *
+ * Above would represent a normal GGTT view as normally mapped for GPU or CPU
+ * rendering. In contrast, fed to the display engine would be an alternative
+ * view which could look something like this:
+ *
+ *   1212
+ *   3434
+ *
+ * In this example both the size and layout of pages in the alternative view is
+ * different from the normal view.
+ *
+ * Implementation and usage
+ *
+ * GGTT views are implemented using VMAs and are distinguished via enum
+ * i915_ggtt_view_type and struct i915_ggtt_view.
+ *
+ * A new flavour of core GEM functions which work with GGTT bound objects were
+ * added with the _view suffix. They take the struct i915_ggtt_view parameter
+ * encapsulating all metadata required to implement a view.
+ *
+ * As a helper for callers which are only interested in the normal view,
+ * globally const i915_ggtt_view_normal singleton instance exists. All old core
+ * GEM API functions, the ones not taking the view parameter, are operating on,
+ * or with the normal GGTT view.
+ *
+ * Code wanting to add or use a new GGTT view needs to:
+ *
+ * 1. Add a new enum with a suitable name.
+ * 2. Extend the metadata in the i915_ggtt_view structure if required.
+ * 3. Add support to i915_get_vma_pages().
+ *
+ * New views are required to build a scatter-gather table from within the
+ * i915_get_vma_pages function. This table is stored in the vma.ggtt_view and
+ * exists for the lifetime of an VMA.
+ *
+ * Core API is designed to have copy semantics which means that passed in
+ * struct i915_ggtt_view does not need to be persistent (left around after
+ * calling the core API functions).
+ *
+ */
+
 const struct i915_ggtt_view i915_ggtt_view_normal;
 
 static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
-- 
2.1.1

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

Re: [PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Fri, Dec 05, 2014 at 12:11:45PM +0000, Tvrtko Ursulin wrote:
> +struct i915_ggtt_view {
> +	enum i915_ggtt_view_type type;
> +
> +	struct sg_table *pages;
> +};

Minor nit on semantics, not really worth a resend (except when you need
one anyway because of the detailed review): Imo the sg_table should be in
the vma directly, not in the gtt_view. Otherwise we can't just do a memcmp
if the view paramaters grow more fancy than just the type key. And maybe
call it sg_table, not pages, to avoid confusion with the real obj->pages
backing storage pointer.

Anyway just bikesheds ;-)
-Daniel

> +
> +extern const struct i915_ggtt_view i915_ggtt_view_normal;
> +
>  enum i915_cache_level;
> +
>  /**
>   * A VMA represents a GEM BO that is bound into an address space. Therefore, a
>   * VMA's presence cannot be guaranteed before binding, or after unbinding the
> @@ -129,6 +142,15 @@ struct i915_vma {
>  #define PTE_READ_ONLY	(1<<2)
>  	unsigned int bound : 4;
>  
> +	/**
> +	 * Support different GGTT views into the same object.
> +	 * This means there can be multiple VMA mappings per object and per VM.
> +	 * i915_ggtt_view_type is used to distinguish between those entries.
> +	 * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also
> +	 * assumed in GEM functions which take no ggtt view parameter.
> +	 */
> +	struct i915_ggtt_view ggtt_view;
> +
>  	/** This object's place on the active/inactive lists */
>  	struct list_head mm_list;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c4536e1..f97479a 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
>  			break;
>  
>  		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (vma->vm == vm && vma->pin_count > 0) {
> +			if (vma->vm == vm && vma->pin_count > 0)
>  				capture_bo(err++, vma);
> -				break;
> -			}
>  	}
>  
>  	return err - first;
> @@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
>  
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>  		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (vma->vm == vm && vma->pin_count > 0) {
> +			if (vma->vm == vm && vma->pin_count > 0)
>  				i++;
> -				break;
> -			}
>  	}
>  	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
>  
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Michel Thierry]

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

On 12/5/2014 12:11 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
> to map objects into the same address space multiple times.
>
> Added a GGTT view concept and linked it with the VMA to distinguish between
> multiple instances per address space.
>
> New objects and GEM functions which do not take this new view as a parameter
> assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
> previous behaviour.
>
> This now means that objects can have multiple VMA entries so the code which
> assumed there will only be one also had to be modified.
>
> Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
> which is DMA mapped on first VMA instantiation and unmapped on the last one
> going away.
>
> v2:
>      * Removed per view special casing in i915_gem_ggtt_prepare /
>        finish_object in favour of creating and destroying DMA mappings
>        on first VMA instantiation and last VMA destruction. (Daniel Vetter)
>      * Simplified i915_vma_unbind which does not need to count the GGTT views.
>        (Daniel Vetter)
>      * Also moved obj->map_and_fenceable reset under the same check.
>      * Checkpatch cleanups.
>
> v3:
>      * Only retire objects once the last VMA is unbound.
>
> v4:
>      * Keep scatter-gather table for alternative views persistent for the
>        lifetime of the VMA.
>      * Propagate binding errors to callers and handle appropriately.
>
> For: VIZ-4544
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
>   drivers/gpu/drm/i915/i915_drv.h            |  46 +++++++++++--
>   drivers/gpu/drm/i915/i915_gem.c            | 101 ++++++++++++++++++-----------
>   drivers/gpu/drm/i915/i915_gem_context.c    |  11 +++-
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   9 ++-
>   drivers/gpu/drm/i915/i915_gem_gtt.c        |  70 +++++++++++++++++---
>   drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 +++++++
>   drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +--
>   8 files changed, 206 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6c16939..bd08289 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>   			seq_puts(m, " (pp");
>   		else
>   			seq_puts(m, " (g");
> -		seq_printf(m, "gtt offset: %08lx, size: %08lx)",
> -			   vma->node.start, vma->node.size);
> +		seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)",
> +			   vma->node.start, vma->node.size,
> +			   vma->ggtt_view.type);
>   	}
>   	if (obj->stolen)
>   		seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 049482f..b2f6f7d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
>   #define PIN_GLOBAL 0x4
>   #define PIN_OFFSET_BIAS 0x8
>   #define PIN_OFFSET_MASK (~4095)
> +int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> +					  struct i915_address_space *vm,
> +					  uint32_t alignment,
> +					  uint64_t flags,
> +					  const struct i915_ggtt_view *view);
> +static inline
>   int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
>   				     struct i915_address_space *vm,
>   				     uint32_t alignment,
> -				     uint64_t flags);
> +				     uint64_t flags)
> +{
> +	return i915_gem_object_pin_view(obj, vm, alignment, flags,
> +						&i915_ggtt_view_normal);
> +}
> +
> +int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> +		  u32 flags);
>   int __must_check i915_vma_unbind(struct i915_vma *vma);
>   int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>   void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
> @@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>   
>   void i915_gem_restore_fences(struct drm_device *dev);
>   
> +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> +				       struct i915_address_space *vm,
> +				       enum i915_ggtt_view_type view);
> +static inline
>   unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> -				  struct i915_address_space *vm);
> +				  struct i915_address_space *vm)
> +{
> +	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> +}
>   bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
>   bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
>   			struct i915_address_space *vm);
>   unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
>   				struct i915_address_space *vm);
> +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> +					  struct i915_address_space *vm,
> +					  const struct i915_ggtt_view *view);
> +static inline
>   struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> -				     struct i915_address_space *vm);
> +				     struct i915_address_space *vm)
> +{
> +	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> +}
> +
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> +				       struct i915_address_space *vm,
> +				       const struct i915_ggtt_view *view);
> +
> +static inline
>   struct i915_vma *
>   i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> -				  struct i915_address_space *vm);
> +				  struct i915_address_space *vm)
> +{
> +	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> +						&i915_ggtt_view_normal);
> +}
Hi,

We also need a _vma_view version of i915_gem_obj_bound; 
i915_gem_object_ggtt_unpin checks if the obj is ggtt_bound and it could 
be that the normal view is gone but a different view is still active (it 
is also used in gpu_error and debug_fs, but I don't think it's a problem 
there).

>   
>   struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
>   static inline bool i915_gem_obj_is_pinned(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 c1c1141..9244f0b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2087,8 +2087,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>   			/* For the unbound phase, this should be a no-op! */
>   			list_for_each_entry_safe(vma, v,
>   						 &obj->vma_list, vma_link)
> -				if (i915_vma_unbind(vma))
> -					break;
> +				i915_vma_unbind(vma);
>   
>   			if (i915_gem_object_put_pages(obj) == 0)
>   				count += obj->base.size >> PAGE_SHIFT;
> @@ -2297,17 +2296,14 @@ void i915_vma_move_to_active(struct i915_vma *vma,
>   static void
>   i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>   {
> -	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> -	struct i915_address_space *vm;
>   	struct i915_vma *vma;
>   
>   	BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
>   	BUG_ON(!obj->active);
>   
> -	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> -		vma = i915_gem_obj_to_vma(obj, vm);
> -		if (vma && !list_empty(&vma->mm_list))
> -			list_move_tail(&vma->mm_list, &vm->inactive_list);
> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		if (!list_empty(&vma->mm_list))
> +			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
>   	}
>   
>   	intel_fb_obj_flush(obj, true);
> @@ -3062,10 +3058,8 @@ int i915_vma_unbind(struct i915_vma *vma)
>   	 * cause memory corruption through use-after-free.
>   	 */
>   
> -	/* Throw away the active reference before moving to the unbound list */
> -	i915_gem_object_retire(obj);
> -
> -	if (i915_is_ggtt(vma->vm)) {
> +	if (i915_is_ggtt(vma->vm) &&
> +	    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
>   		i915_gem_object_finish_gtt(obj);
>   
>   		/* release the fence reg _after_ flushing */
> @@ -3079,8 +3073,15 @@ int i915_vma_unbind(struct i915_vma *vma)
>   	vma->unbind_vma(vma);
>   
>   	list_del_init(&vma->mm_list);
> -	if (i915_is_ggtt(vma->vm))
> -		obj->map_and_fenceable = false;
> +	if (i915_is_ggtt(vma->vm)) {
> +		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
> +			obj->map_and_fenceable = false;
> +		} else if (vma->ggtt_view.pages) {
> +			sg_free_table(vma->ggtt_view.pages);
> +			kfree(vma->ggtt_view.pages);
> +			vma->ggtt_view.pages = NULL;
> +		}
> +	}
>   
>   	drm_mm_remove_node(&vma->node);
>   	i915_gem_vma_destroy(vma);
> @@ -3088,6 +3089,10 @@ int i915_vma_unbind(struct i915_vma *vma)
>   	/* Since the unbound list is global, only move to that list if
>   	 * no more VMAs exist. */
>   	if (list_empty(&obj->vma_list)) {
> +		/* Throw away the active reference before
> +		 * moving to the unbound list. */
> +		i915_gem_object_retire(obj);
> +
>   		i915_gem_gtt_finish_object(obj);
>   		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>   	}
> @@ -3498,7 +3503,8 @@ static struct i915_vma *
>   i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   			   struct i915_address_space *vm,
>   			   unsigned alignment,
> -			   uint64_t flags)
> +			   uint64_t flags,
> +			   const struct i915_ggtt_view *view)
>   {
>   	struct drm_device *dev = obj->base.dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3548,7 +3554,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   
>   	i915_gem_object_pin_pages(obj);
>   
> -	vma = i915_gem_obj_lookup_or_create_vma(obj, vm);
> +	vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
>   	if (IS_ERR(vma))
>   		goto err_unpin;
>   
> @@ -3578,15 +3584,19 @@ search_free:
>   	if (ret)
>   		goto err_remove_node;
>   
> +	trace_i915_vma_bind(vma, flags);
> +	ret = i915_vma_bind(vma, obj->cache_level,
> +			    flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
> +	if (ret)
> +		goto err_finish_gtt;
> +
>   	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
>   	list_add_tail(&vma->mm_list, &vm->inactive_list);
>   
> -	trace_i915_vma_bind(vma, flags);
> -	vma->bind_vma(vma, obj->cache_level,
> -		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
> -
>   	return vma;
>   
> +err_finish_gtt:
> +	i915_gem_gtt_finish_object(obj);
>   err_remove_node:
>   	drm_mm_remove_node(&vma->node);
>   err_free_vma:
> @@ -3789,9 +3799,12 @@ 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))
> -				vma->bind_vma(vma, cache_level,
> -						vma->bound & GLOBAL_BIND);
> +			if (drm_mm_node_allocated(&vma->node)) {
> +				ret = i915_vma_bind(vma, cache_level,
> +						    vma->bound & GLOBAL_BIND);
> +				if (ret);
> +					return ret;
> +			}
>   	}
>   
>   	list_for_each_entry(vma, &obj->vma_list, vma_link)
> @@ -4144,10 +4157,11 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
>   }
>   
>   int
> -i915_gem_object_pin(struct drm_i915_gem_object *obj,
> -		    struct i915_address_space *vm,
> -		    uint32_t alignment,
> -		    uint64_t flags)
> +i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> +			 struct i915_address_space *vm,
> +			 uint32_t alignment,
> +			 uint64_t flags,
> +			 const struct i915_ggtt_view *view)
>   {
>   	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
>   	struct i915_vma *vma;
> @@ -4163,7 +4177,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>   	if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
>   		return -EINVAL;
>   
> -	vma = i915_gem_obj_to_vma(obj, vm);
> +	vma = i915_gem_obj_to_vma_view(obj, vm, view);
>   	if (vma) {
>   		if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
>   			return -EBUSY;
> @@ -4173,7 +4187,8 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>   			     "bo is already pinned with incorrect alignment:"
>   			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
>   			     " obj->map_and_fenceable=%d\n",
> -			     i915_gem_obj_offset(obj, vm), alignment,
> +			     i915_gem_obj_offset_view(obj, vm, view->type),
> +			     alignment,
>   			     !!(flags & PIN_MAPPABLE),
>   			     obj->map_and_fenceable);
>   			ret = i915_vma_unbind(vma);
> @@ -4186,13 +4201,17 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>   
>   	bound = vma ? vma->bound : 0;
>   	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> -		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
> +		vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
> +						 flags, view);
>   		if (IS_ERR(vma))
>   			return PTR_ERR(vma);
>   	}
>   
> -	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND))
> -		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
> +	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) {
> +		ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	if ((bound ^ vma->bound) & GLOBAL_BIND) {
>   		bool mappable, fenceable;
> @@ -4528,12 +4547,13 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>   	intel_runtime_pm_put(dev_priv);
>   }
>   
> -struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> -				     struct i915_address_space *vm)
> +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> +					  struct i915_address_space *vm,
> +					  const struct i915_ggtt_view *view)
>   {
>   	struct i915_vma *vma;
>   	list_for_each_entry(vma, &obj->vma_list, vma_link)
> -		if (vma->vm == vm)
> +		if (vma->vm == vm && vma->ggtt_view.type == view->type)
>   			return vma;
>   
>   	return NULL;
> @@ -5147,8 +5167,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
>   }
>   
>   /* All the new VM stuff */
> -unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> -				  struct i915_address_space *vm)
> +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> +				       struct i915_address_space *vm,
> +				       enum i915_ggtt_view_type view)
>   {
>   	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
>   	struct i915_vma *vma;
> @@ -5156,7 +5177,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
>   	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
>   
>   	list_for_each_entry(vma, &o->vma_list, vma_link) {
> -		if (vma->vm == vm)
> +		if (vma->vm == vm && vma->ggtt_view.type == view)
>   			return vma->node.start;
>   
>   	}
> @@ -5307,7 +5328,9 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>   	struct i915_vma *vma;
>   
>   	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> -		if (vma->vm == ggtt)
> +		if (vma->vm != ggtt)
> +			continue;
> +		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>   			return vma;
>   	}
I'd prefer to have a single if (vma->vm == ggtt && type = normal view).

Everything else looks good to me.

-Michel
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 5cd2b97..4ef8815 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -590,9 +590,14 @@ static int do_switch(struct intel_engine_cs *ring,
>   		goto unpin_out;
>   
>   	vma = i915_gem_obj_to_ggtt(to->legacy_hw_ctx.rcs_state);
> -	if (!(vma->bound & GLOBAL_BIND))
> -		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level,
> -				GLOBAL_BIND);
> +	if (!(vma->bound & GLOBAL_BIND)) {
> +		ret = i915_vma_bind(vma,
> +				    to->legacy_hw_ctx.rcs_state->cache_level,
> +				    GLOBAL_BIND);
> +		/* This shouldn't ever fail. */
> +		if (WARN_ONCE(ret, "GGTT context bind failed!"))
> +			goto unpin_out;
> +	}
>   
>   	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
>   		hw_flags |= MI_RESTORE_INHIBIT;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 0c25f62..3927d93 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -360,9 +360,12 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
>   	 * through the ppgtt for non_secure batchbuffers. */
>   	if (unlikely(IS_GEN6(dev) &&
>   	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
> -	    !(target_vma->bound & GLOBAL_BIND)))
> -		target_vma->bind_vma(target_vma, target_i915_obj->cache_level,
> -				GLOBAL_BIND);
> +	    !(target_vma->bound & GLOBAL_BIND))) {
> +		ret = i915_vma_bind(target_vma, target_i915_obj->cache_level,
> +				    GLOBAL_BIND);
> +		if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!"))
> +			return ret;
> +	}
>   
>   	/* Validate that the target is in a valid r/w GPU domain */
>   	if (unlikely(reloc->write_domain & (reloc->write_domain - 1))) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ac03a38..73c1c0b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -30,6 +30,8 @@
>   #include "i915_trace.h"
>   #include "intel_drv.h"
>   
> +const struct i915_ggtt_view i915_ggtt_view_normal;
> +
>   static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
>   static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
>   
> @@ -1341,9 +1343,12 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>   		/* The bind_vma code tries to be smart about tracking mappings.
>   		 * Unfortunately above, we've just wiped out the mappings
>   		 * without telling our object about it. So we need to fake it.
> +		 *
> +		 * Bind is not expected to fail since this is only called on
> +		 * resume and assumption is all requirements exist already.
>   		 */
>   		vma->bound &= ~GLOBAL_BIND;
> -		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
> +		WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND));
>   	}
>   
>   
> @@ -1538,7 +1543,7 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
>   		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
>   
>   	BUG_ON(!i915_is_ggtt(vma->vm));
> -	intel_gtt_insert_sg_entries(vma->obj->pages, entry, flags);
> +	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
>   	vma->bound = GLOBAL_BIND;
>   }
>   
> @@ -1588,7 +1593,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>   	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
>   		if (!(vma->bound & GLOBAL_BIND) ||
>   		    (cache_level != obj->cache_level)) {
> -			vma->vm->insert_entries(vma->vm, obj->pages,
> +			vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
>   						vma->node.start,
>   						cache_level, flags);
>   			vma->bound |= GLOBAL_BIND;
> @@ -1600,7 +1605,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>   	     (cache_level != obj->cache_level))) {
>   		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
>   		appgtt->base.insert_entries(&appgtt->base,
> -					    vma->obj->pages,
> +					    vma->ggtt_view.pages,
>   					    vma->node.start,
>   					    cache_level, flags);
>   		vma->bound |= LOCAL_BIND;
> @@ -2165,7 +2170,8 @@ int i915_gem_gtt_init(struct drm_device *dev)
>   }
>   
>   static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
> -					      struct i915_address_space *vm)
> +					      struct i915_address_space *vm,
> +					      const struct i915_ggtt_view *view)
>   {
>   	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
>   	if (vma == NULL)
> @@ -2176,6 +2182,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
>   	INIT_LIST_HEAD(&vma->exec_list);
>   	vma->vm = vm;
>   	vma->obj = obj;
> +	vma->ggtt_view = *view;
>   
>   	switch (INTEL_INFO(vm->dev)->gen) {
>   	case 9:
> @@ -2210,14 +2217,59 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
>   }
>   
>   struct i915_vma *
> -i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> -				  struct i915_address_space *vm)
> +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> +				       struct i915_address_space *vm,
> +				       const struct i915_ggtt_view *view)
>   {
>   	struct i915_vma *vma;
>   
> -	vma = i915_gem_obj_to_vma(obj, vm);
> +	vma = i915_gem_obj_to_vma_view(obj, vm, view);
>   	if (!vma)
> -		vma = __i915_gem_vma_create(obj, vm);
> +		vma = __i915_gem_vma_create(obj, vm, view);
>   
>   	return vma;
>   }
> +
> +static inline
> +int i915_get_vma_pages(struct i915_vma *vma)
> +{
> +	if (vma->ggtt_view.pages)
> +		return 0;
> +
> +	if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> +		vma->ggtt_view.pages = vma->obj->pages;
> +	else
> +		WARN_ONCE(1, "GGTT view %u not implemented!\n",
> +			  vma->ggtt_view.type);
> +
> +	if (!vma->ggtt_view.pages) {
> +		DRM_ERROR("Failed to get pages for VMA view type %u!\n",
> +			  vma->ggtt_view.type);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address space.
> + * @vma: VMA to map
> + * @cache_level: mapping cache level
> + * @flags: flags like global or local mapping
> + *
> + * DMA addresses are taken from the scatter-gather table of this object (or of
> + * this VMA in case of non-default GGTT views) and PTE entries set up.
> + * Note that DMA addresses are also the only part of the SG table we care about.
> + */
> +int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> +		  u32 flags)
> +{
> +	int ret = i915_get_vma_pages(vma);
> +
> +	if (ret)
> +		return ret;
> +
> +	vma->bind_vma(vma, cache_level, flags);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index dd849df..e377c7d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -109,7 +109,20 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>   #define GEN8_PPAT_ELLC_OVERRIDE		(0<<2)
>   #define GEN8_PPAT(i, x)			((uint64_t) (x) << ((i) * 8))
>   
> +enum i915_ggtt_view_type {
> +	I915_GGTT_VIEW_NORMAL = 0,
> +};
> +
> +struct i915_ggtt_view {
> +	enum i915_ggtt_view_type type;
> +
> +	struct sg_table *pages;
> +};
> +
> +extern const struct i915_ggtt_view i915_ggtt_view_normal;
> +
>   enum i915_cache_level;
> +
>   /**
>    * A VMA represents a GEM BO that is bound into an address space. Therefore, a
>    * VMA's presence cannot be guaranteed before binding, or after unbinding the
> @@ -129,6 +142,15 @@ struct i915_vma {
>   #define PTE_READ_ONLY	(1<<2)
>   	unsigned int bound : 4;
>   
> +	/**
> +	 * Support different GGTT views into the same object.
> +	 * This means there can be multiple VMA mappings per object and per VM.
> +	 * i915_ggtt_view_type is used to distinguish between those entries.
> +	 * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also
> +	 * assumed in GEM functions which take no ggtt view parameter.
> +	 */
> +	struct i915_ggtt_view ggtt_view;
> +
>   	/** This object's place on the active/inactive lists */
>   	struct list_head mm_list;
>   
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c4536e1..f97479a 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
>   			break;
>   
>   		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (vma->vm == vm && vma->pin_count > 0) {
> +			if (vma->vm == vm && vma->pin_count > 0)
>   				capture_bo(err++, vma);
> -				break;
> -			}
>   	}
>   
>   	return err - first;
> @@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
>   
>   	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>   		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (vma->vm == vm && vma->pin_count > 0) {
> +			if (vma->vm == vm && vma->pin_count > 0)
>   				i++;
> -				break;
> -			}
>   	}
>   	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
>   


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 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 2/2] drm/i915: Documentation for multiple GGTT views

[Michel Thierry]

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

On 12/5/2014 12:11 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> A short section describing background, implementation and intended usage.
>
> For: VIZ-4544
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   Documentation/DocBook/drm.tmpl      |  5 ++++
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 60 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 65 insertions(+)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 56e2a9b..c975f26 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -4033,6 +4033,11 @@ int num_ioctls;</synopsis>
>   !Pdrivers/gpu/drm/i915/intel_lrc.c Logical Rings, Logical Ring Contexts and Execlists
>   !Idrivers/gpu/drm/i915/intel_lrc.c
>         </sect2>
> +      <sect2>
> +        <title>Global GTT views</title>
> +!Pdrivers/gpu/drm/i915/i915_gem_gtt.c Global GTT views
Isn't it missing the _(GGTT)_ of the DOC: section title?
i.e. !Pdrivers/gpu/drm/i915/i915_gem_gtt.c Global GTT views (GGTT)

> +!Idrivers/gpu/drm/i915/i915_gem_gtt.c
> +      </sect2>
>       </sect1>
>   
>       <sect1>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 73c1c0b..0953369 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -30,6 +30,66 @@
>   #include "i915_trace.h"
>   #include "intel_drv.h"
>   
> +/**
> + * DOC: Global GTT views (GGTT)
> + *
> + * Background and previous state
> + *
> + * Historically objects could exists (be bound) in global GTT space only as
> + * singular instances with a view representing all of the object's backing pages
> + * in a linear fashion. This view will be called a normal view.
> + *
> + * To support multiple views of the same object, where the number of mapped
> + * pages is not equal to the backing store, or where the layout of the pages
> + * is not linear, concept of a GGTT view was added.
> + *
> + * One example of an alternative view is a stereo display driven by a single
> + * image. In this case we would have a framebuffer looking like this
> + * (2x2 pages):
> + *
> + *    12
> + *    34
> + *
> + * Above would represent a normal GGTT view as normally mapped for GPU or CPU
> + * rendering. In contrast, fed to the display engine would be an alternative
> + * view which could look something like this:
> + *
> + *   1212
> + *   3434
> + *
> + * In this example both the size and layout of pages in the alternative view is
> + * different from the normal view.
> + *
> + * Implementation and usage
> + *
> + * GGTT views are implemented using VMAs and are distinguished via enum
> + * i915_ggtt_view_type and struct i915_ggtt_view.
> + *
> + * A new flavour of core GEM functions which work with GGTT bound objects were
> + * added with the _view suffix. They take the struct i915_ggtt_view parameter
> + * encapsulating all metadata required to implement a view.
> + *
> + * As a helper for callers which are only interested in the normal view,
> + * globally const i915_ggtt_view_normal singleton instance exists. All old core
> + * GEM API functions, the ones not taking the view parameter, are operating on,
> + * or with the normal GGTT view.
> + *
> + * Code wanting to add or use a new GGTT view needs to:
> + *
> + * 1. Add a new enum with a suitable name.
> + * 2. Extend the metadata in the i915_ggtt_view structure if required.
> + * 3. Add support to i915_get_vma_pages().
> + *
> + * New views are required to build a scatter-gather table from within the
> + * i915_get_vma_pages function. This table is stored in the vma.ggtt_view and
> + * exists for the lifetime of an VMA.
> + *
> + * Core API is designed to have copy semantics which means that passed in
> + * struct i915_ggtt_view does not need to be persistent (left around after
> + * calling the core API functions).
> + *
> + */
> +
>   const struct i915_ggtt_view i915_ggtt_view_normal;
>   
>   static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
Apart from that,
Reviewed-by: Michel Thierry <michel.thierry@intel.com>



[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 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 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Tue, Dec 09, 2014 at 03:23:35PM +0000, Michel Thierry wrote:
> On 12/5/2014 12:11 PM, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
> >to map objects into the same address space multiple times.
> >
> >Added a GGTT view concept and linked it with the VMA to distinguish between
> >multiple instances per address space.
> >
> >New objects and GEM functions which do not take this new view as a parameter
> >assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
> >previous behaviour.
> >
> >This now means that objects can have multiple VMA entries so the code which
> >assumed there will only be one also had to be modified.
> >
> >Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
> >which is DMA mapped on first VMA instantiation and unmapped on the last one
> >going away.
> >
> >v2:
> >     * Removed per view special casing in i915_gem_ggtt_prepare /
> >       finish_object in favour of creating and destroying DMA mappings
> >       on first VMA instantiation and last VMA destruction. (Daniel Vetter)
> >     * Simplified i915_vma_unbind which does not need to count the GGTT views.
> >       (Daniel Vetter)
> >     * Also moved obj->map_and_fenceable reset under the same check.
> >     * Checkpatch cleanups.
> >
> >v3:
> >     * Only retire objects once the last VMA is unbound.
> >
> >v4:
> >     * Keep scatter-gather table for alternative views persistent for the
> >       lifetime of the VMA.
> >     * Propagate binding errors to callers and handle appropriately.
> >
> >For: VIZ-4544
> >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >---
> >  drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
> >  drivers/gpu/drm/i915/i915_drv.h            |  46 +++++++++++--
> >  drivers/gpu/drm/i915/i915_gem.c            | 101 ++++++++++++++++++-----------
> >  drivers/gpu/drm/i915/i915_gem_context.c    |  11 +++-
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   9 ++-
> >  drivers/gpu/drm/i915/i915_gem_gtt.c        |  70 +++++++++++++++++---
> >  drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 +++++++
> >  drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +--
> >  8 files changed, 206 insertions(+), 66 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >index 6c16939..bd08289 100644
> >--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >@@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> >  			seq_puts(m, " (pp");
> >  		else
> >  			seq_puts(m, " (g");
> >-		seq_printf(m, "gtt offset: %08lx, size: %08lx)",
> >-			   vma->node.start, vma->node.size);
> >+		seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)",
> >+			   vma->node.start, vma->node.size,
> >+			   vma->ggtt_view.type);
> >  	}
> >  	if (obj->stolen)
> >  		seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index 049482f..b2f6f7d 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
> >  #define PIN_GLOBAL 0x4
> >  #define PIN_OFFSET_BIAS 0x8
> >  #define PIN_OFFSET_MASK (~4095)
> >+int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> >+					  struct i915_address_space *vm,
> >+					  uint32_t alignment,
> >+					  uint64_t flags,
> >+					  const struct i915_ggtt_view *view);
> >+static inline
> >  int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
> >  				     struct i915_address_space *vm,
> >  				     uint32_t alignment,
> >-				     uint64_t flags);
> >+				     uint64_t flags)
> >+{
> >+	return i915_gem_object_pin_view(obj, vm, alignment, flags,
> >+						&i915_ggtt_view_normal);
> >+}
> >+
> >+int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> >+		  u32 flags);
> >  int __must_check i915_vma_unbind(struct i915_vma *vma);
> >  int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
> >  void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
> >@@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
> >  void i915_gem_restore_fences(struct drm_device *dev);
> >+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> >+				       struct i915_address_space *vm,
> >+				       enum i915_ggtt_view_type view);
> >+static inline
> >  unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> >-				  struct i915_address_space *vm);
> >+				  struct i915_address_space *vm)
> >+{
> >+	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> >+}
> >  bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
> >  bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
> >  			struct i915_address_space *vm);
> >  unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> >  				struct i915_address_space *vm);
> >+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> >+					  struct i915_address_space *vm,
> >+					  const struct i915_ggtt_view *view);
> >+static inline
> >  struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> >-				     struct i915_address_space *vm);
> >+				     struct i915_address_space *vm)
> >+{
> >+	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> >+}
> >+
> >+struct i915_vma *
> >+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> >+				       struct i915_address_space *vm,
> >+				       const struct i915_ggtt_view *view);
> >+
> >+static inline
> >  struct i915_vma *
> >  i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> >-				  struct i915_address_space *vm);
> >+				  struct i915_address_space *vm)
> >+{
> >+	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> >+						&i915_ggtt_view_normal);
> >+}
> 
> We also need a _vma_view version of i915_gem_obj_bound;
> i915_gem_object_ggtt_unpin checks if the obj is ggtt_bound and it could be
> that the normal view is gone but a different view is still active (it is
> also used in gpu_error and debug_fs, but I don't think it's a problem
> there).

Where did you see the need for the new obj_bound variant? Probably best to
reply to the patch newly with just the relevant part quoted.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

On 12/10/2014 09:16 AM, Daniel Vetter wrote:
> On Tue, Dec 09, 2014 at 03:23:35PM +0000, Michel Thierry wrote:
>> We also need a _vma_view version of i915_gem_obj_bound;
>> i915_gem_object_ggtt_unpin checks if the obj is ggtt_bound and it could be
>> that the normal view is gone but a different view is still active (it is
>> also used in gpu_error and debug_fs, but I don't think it's a problem
>> there).
>
> Where did you see the need for the new obj_bound variant? Probably best to
> reply to the patch newly with just the relevant part quoted.

It is not in the patch but in the i915_gem_object_ggtt_unpin. Which is:

i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj)
{
         struct i915_vma *vma = i915_gem_obj_to_ggtt(obj);

         BUG_ON(!vma);
         BUG_ON(vma->pin_count == 0);
         BUG_ON(!i915_gem_obj_ggtt_bound(obj));

         if (--vma->pin_count == 0)
                 obj->pin_mappable = false;
}

The concern is the mismatch in semantics between i915_gem_obj_to_ggtt 
and i915_gem_obj_ggtt_bound. Former implies normal VMA while the latter 
hasn't been touched so it returns true on _any_ GGTT bound VMA.

I don't think this BUG_ON can trigger since normal VMA exists by the 
virtue of BUG_ON(!vma), but I do agree that there is a mismatch in 
"documentation" (BUG_ONs). So making i915_gem_obj_ggtt_bound also imply 
a normal view would be correct it seems.

Regards,

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

[PATCH v3 0/2] Multiple GGTT views

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This series continues what was previously a single patch called "drm/i915:
Infrastructure for supporting different GGTT views per object".

v2:
 * One less patch due early prep work getting merged.
 * Main patch reworked to keep the pages pointer around for the lifetime of the
   VMA. More detailed changelog in the patch itself.

v3:
 * Addressing review comments - see individual changelogs.

Tvrtko Ursulin (2):
  drm/i915: Infrastructure for supporting different GGTT views per
    object
  drm/i915: Documentation for multiple GGTT views

 Documentation/DocBook/drm.tmpl             |   5 ++
 drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
 drivers/gpu/drm/i915/i915_drv.h            |  56 +++++++++++--
 drivers/gpu/drm/i915/i915_gem.c            | 112 +++++++++++++++----------
 drivers/gpu/drm/i915/i915_gem_context.c    |  11 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   9 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 130 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 +++++
 drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +-
 9 files changed, 286 insertions(+), 72 deletions(-)

-- 
2.1.1

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

[PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
to map objects into the same address space multiple times.

Added a GGTT view concept and linked it with the VMA to distinguish between
multiple instances per address space.

New objects and GEM functions which do not take this new view as a parameter
assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
previous behaviour.

This now means that objects can have multiple VMA entries so the code which
assumed there will only be one also had to be modified.

Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
which is DMA mapped on first VMA instantiation and unmapped on the last one
going away.

v2:
    * Removed per view special casing in i915_gem_ggtt_prepare /
      finish_object in favour of creating and destroying DMA mappings
      on first VMA instantiation and last VMA destruction. (Daniel Vetter)
    * Simplified i915_vma_unbind which does not need to count the GGTT views.
      (Daniel Vetter)
    * Also moved obj->map_and_fenceable reset under the same check.
    * Checkpatch cleanups.

v3:
    * Only retire objects once the last VMA is unbound.

v4:
    * Keep scatter-gather table for alternative views persistent for the
      lifetime of the VMA.
    * Propagate binding errors to callers and handle appropriately.

v5:
    * Explicitly look for normal GGTT view in i915_gem_obj_bound to align
      usage in i915_gem_object_ggtt_unpin. (Michel Thierry)
    * Change to single if statement in i915_gem_obj_to_ggtt. (Michel Thierry)
    * Removed stray semi-colon in i915_gem_object_set_cache_level.

For: VIZ-4544
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
 drivers/gpu/drm/i915/i915_drv.h            |  56 +++++++++++++--
 drivers/gpu/drm/i915/i915_gem.c            | 112 +++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_context.c    |  11 ++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   9 ++-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |  70 +++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 ++++++
 drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +--
 8 files changed, 221 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d0e445e..a8af667 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 			seq_puts(m, " (pp");
 		else
 			seq_puts(m, " (g");
-		seq_printf(m, "gtt offset: %08lx, size: %08lx)",
-			   vma->node.start, vma->node.size);
+		seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)",
+			   vma->node.start, vma->node.size,
+			   vma->ggtt_view.type);
 	}
 	if (obj->stolen)
 		seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11e85cb..451bb6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2518,10 +2518,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
 #define PIN_GLOBAL 0x4
 #define PIN_OFFSET_BIAS 0x8
 #define PIN_OFFSET_MASK (~4095)
+int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  uint32_t alignment,
+					  uint64_t flags,
+					  const struct i915_ggtt_view *view);
+static inline
 int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
 				     struct i915_address_space *vm,
 				     uint32_t alignment,
-				     uint64_t flags);
+				     uint64_t flags)
+{
+	return i915_gem_object_pin_view(obj, vm, alignment, flags,
+						&i915_ggtt_view_normal);
+}
+
+int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
+		  u32 flags);
 int __must_check i915_vma_unbind(struct i915_vma *vma);
 int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
 void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
@@ -2683,18 +2696,51 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
 
 void i915_gem_restore_fences(struct drm_device *dev);
 
+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
+				       struct i915_address_space *vm,
+				       enum i915_ggtt_view_type view);
+static inline
 unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
-				  struct i915_address_space *vm);
+				  struct i915_address_space *vm)
+{
+	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
+}
 bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
+bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
+			     struct i915_address_space *vm,
+			     enum i915_ggtt_view_type view);
+static inline
 bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
-			struct i915_address_space *vm);
+			struct i915_address_space *vm)
+{
+	return i915_gem_obj_bound_view(o, vm, I915_GGTT_VIEW_NORMAL);
+}
+
 unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 				struct i915_address_space *vm);
+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  const struct i915_ggtt_view *view);
+static inline
 struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-				     struct i915_address_space *vm);
+				     struct i915_address_space *vm)
+{
+	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
+}
+
+struct i915_vma *
+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
+				       struct i915_address_space *vm,
+				       const struct i915_ggtt_view *view);
+
+static inline
 struct i915_vma *
 i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
-				  struct i915_address_space *vm);
+				  struct i915_address_space *vm)
+{
+	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
+						&i915_ggtt_view_normal);
+}
 
 struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj);
 static inline bool i915_gem_obj_is_pinned(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 de241eb..c6610bc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2098,8 +2098,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			/* For the unbound phase, this should be a no-op! */
 			list_for_each_entry_safe(vma, v,
 						 &obj->vma_list, vma_link)
-				if (i915_vma_unbind(vma))
-					break;
+				i915_vma_unbind(vma);
 
 			if (i915_gem_object_put_pages(obj) == 0)
 				count += obj->base.size >> PAGE_SHIFT;
@@ -2308,17 +2307,14 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 static void
 i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 {
-	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
-	struct i915_address_space *vm;
 	struct i915_vma *vma;
 
 	BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
 	BUG_ON(!obj->active);
 
-	list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
-		vma = i915_gem_obj_to_vma(obj, vm);
-		if (vma && !list_empty(&vma->mm_list))
-			list_move_tail(&vma->mm_list, &vm->inactive_list);
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (!list_empty(&vma->mm_list))
+			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
 	}
 
 	intel_fb_obj_flush(obj, true);
@@ -3073,10 +3069,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 	 * cause memory corruption through use-after-free.
 	 */
 
-	/* Throw away the active reference before moving to the unbound list */
-	i915_gem_object_retire(obj);
-
-	if (i915_is_ggtt(vma->vm)) {
+	if (i915_is_ggtt(vma->vm) &&
+	    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
 		i915_gem_object_finish_gtt(obj);
 
 		/* release the fence reg _after_ flushing */
@@ -3090,8 +3084,15 @@ int i915_vma_unbind(struct i915_vma *vma)
 	vma->unbind_vma(vma);
 
 	list_del_init(&vma->mm_list);
-	if (i915_is_ggtt(vma->vm))
-		obj->map_and_fenceable = false;
+	if (i915_is_ggtt(vma->vm)) {
+		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
+			obj->map_and_fenceable = false;
+		} else if (vma->ggtt_view.pages) {
+			sg_free_table(vma->ggtt_view.pages);
+			kfree(vma->ggtt_view.pages);
+			vma->ggtt_view.pages = NULL;
+		}
+	}
 
 	drm_mm_remove_node(&vma->node);
 	i915_gem_vma_destroy(vma);
@@ -3099,6 +3100,10 @@ int i915_vma_unbind(struct i915_vma *vma)
 	/* Since the unbound list is global, only move to that list if
 	 * no more VMAs exist. */
 	if (list_empty(&obj->vma_list)) {
+		/* Throw away the active reference before
+		 * moving to the unbound list. */
+		i915_gem_object_retire(obj);
+
 		i915_gem_gtt_finish_object(obj);
 		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
 	}
@@ -3509,7 +3514,8 @@ static struct i915_vma *
 i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 			   struct i915_address_space *vm,
 			   unsigned alignment,
-			   uint64_t flags)
+			   uint64_t flags,
+			   const struct i915_ggtt_view *view)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3559,7 +3565,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
 
 	i915_gem_object_pin_pages(obj);
 
-	vma = i915_gem_obj_lookup_or_create_vma(obj, vm);
+	vma = i915_gem_obj_lookup_or_create_vma_view(obj, vm, view);
 	if (IS_ERR(vma))
 		goto err_unpin;
 
@@ -3589,15 +3595,19 @@ search_free:
 	if (ret)
 		goto err_remove_node;
 
+	trace_i915_vma_bind(vma, flags);
+	ret = i915_vma_bind(vma, obj->cache_level,
+			    flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
+	if (ret)
+		goto err_finish_gtt;
+
 	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_add_tail(&vma->mm_list, &vm->inactive_list);
 
-	trace_i915_vma_bind(vma, flags);
-	vma->bind_vma(vma, obj->cache_level,
-		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
-
 	return vma;
 
+err_finish_gtt:
+	i915_gem_gtt_finish_object(obj);
 err_remove_node:
 	drm_mm_remove_node(&vma->node);
 err_free_vma:
@@ -3800,9 +3810,12 @@ 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))
-				vma->bind_vma(vma, cache_level,
-						vma->bound & GLOBAL_BIND);
+			if (drm_mm_node_allocated(&vma->node)) {
+				ret = i915_vma_bind(vma, cache_level,
+						    vma->bound & GLOBAL_BIND);
+				if (ret)
+					return ret;
+			}
 	}
 
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
@@ -4155,10 +4168,11 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)
 }
 
 int
-i915_gem_object_pin(struct drm_i915_gem_object *obj,
-		    struct i915_address_space *vm,
-		    uint32_t alignment,
-		    uint64_t flags)
+i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
+			 struct i915_address_space *vm,
+			 uint32_t alignment,
+			 uint64_t flags,
+			 const struct i915_ggtt_view *view)
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	struct i915_vma *vma;
@@ -4174,7 +4188,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 	if (WARN_ON((flags & (PIN_MAPPABLE | PIN_GLOBAL)) == PIN_MAPPABLE))
 		return -EINVAL;
 
-	vma = i915_gem_obj_to_vma(obj, vm);
+	vma = i915_gem_obj_to_vma_view(obj, vm, view);
 	if (vma) {
 		if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
 			return -EBUSY;
@@ -4184,7 +4198,8 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 			     "bo is already pinned with incorrect alignment:"
 			     " offset=%lx, req.alignment=%x, req.map_and_fenceable=%d,"
 			     " obj->map_and_fenceable=%d\n",
-			     i915_gem_obj_offset(obj, vm), alignment,
+			     i915_gem_obj_offset_view(obj, vm, view->type),
+			     alignment,
 			     !!(flags & PIN_MAPPABLE),
 			     obj->map_and_fenceable);
 			ret = i915_vma_unbind(vma);
@@ -4197,13 +4212,17 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 
 	bound = vma ? vma->bound : 0;
 	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
-		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
+		vma = i915_gem_object_bind_to_vm(obj, vm, alignment,
+						 flags, view);
 		if (IS_ERR(vma))
 			return PTR_ERR(vma);
 	}
 
-	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND))
-		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
+	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) {
+		ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
+		if (ret)
+			return ret;
+	}
 
 	if ((bound ^ vma->bound) & GLOBAL_BIND) {
 		bool mappable, fenceable;
@@ -4539,12 +4558,13 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	intel_runtime_pm_put(dev_priv);
 }
 
-struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-				     struct i915_address_space *vm)
+struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
+					  struct i915_address_space *vm,
+					  const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
 	list_for_each_entry(vma, &obj->vma_list, vma_link)
-		if (vma->vm == vm)
+		if (vma->vm == vm && vma->ggtt_view.type == view->type)
 			return vma;
 
 	return NULL;
@@ -5156,8 +5176,9 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 }
 
 /* All the new VM stuff */
-unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
-				  struct i915_address_space *vm)
+unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
+				       struct i915_address_space *vm,
+				       enum i915_ggtt_view_type view)
 {
 	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
 	struct i915_vma *vma;
@@ -5165,7 +5186,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
 	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
 
 	list_for_each_entry(vma, &o->vma_list, vma_link) {
-		if (vma->vm == vm)
+		if (vma->vm == vm && vma->ggtt_view.type == view)
 			return vma->node.start;
 
 	}
@@ -5174,13 +5195,16 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
 	return -1;
 }
 
-bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
-			struct i915_address_space *vm)
+bool i915_gem_obj_bound_view(struct drm_i915_gem_object *o,
+			     struct i915_address_space *vm,
+			     enum i915_ggtt_view_type view)
 {
 	struct i915_vma *vma;
 
 	list_for_each_entry(vma, &o->vma_list, vma_link)
-		if (vma->vm == vm && drm_mm_node_allocated(&vma->node))
+		if (vma->vm == vm &&
+		    vma->ggtt_view.type == view &&
+		    drm_mm_node_allocated(&vma->node))
 			return true;
 
 	return false;
@@ -5315,10 +5339,10 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 	struct i915_address_space *ggtt = i915_obj_to_ggtt(obj);
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		if (vma->vm == ggtt)
+	list_for_each_entry(vma, &obj->vma_list, vma_link)
+		if (vma->vm == ggtt &&
+		    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
 			return vma;
-	}
 
 	return NULL;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5cd2b97..4ef8815 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -590,9 +590,14 @@ static int do_switch(struct intel_engine_cs *ring,
 		goto unpin_out;
 
 	vma = i915_gem_obj_to_ggtt(to->legacy_hw_ctx.rcs_state);
-	if (!(vma->bound & GLOBAL_BIND))
-		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level,
-				GLOBAL_BIND);
+	if (!(vma->bound & GLOBAL_BIND)) {
+		ret = i915_vma_bind(vma,
+				    to->legacy_hw_ctx.rcs_state->cache_level,
+				    GLOBAL_BIND);
+		/* This shouldn't ever fail. */
+		if (WARN_ONCE(ret, "GGTT context bind failed!"))
+			goto unpin_out;
+	}
 
 	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
 		hw_flags |= MI_RESTORE_INHIBIT;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0c25f62..3927d93 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -360,9 +360,12 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	 * through the ppgtt for non_secure batchbuffers. */
 	if (unlikely(IS_GEN6(dev) &&
 	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
-	    !(target_vma->bound & GLOBAL_BIND)))
-		target_vma->bind_vma(target_vma, target_i915_obj->cache_level,
-				GLOBAL_BIND);
+	    !(target_vma->bound & GLOBAL_BIND))) {
+		ret = i915_vma_bind(target_vma, target_i915_obj->cache_level,
+				    GLOBAL_BIND);
+		if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!"))
+			return ret;
+	}
 
 	/* Validate that the target is in a valid r/w GPU domain */
 	if (unlikely(reloc->write_domain & (reloc->write_domain - 1))) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ac03a38..73c1c0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,8 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+const struct i915_ggtt_view i915_ggtt_view_normal;
+
 static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
 static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
 
@@ -1341,9 +1343,12 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 		/* The bind_vma code tries to be smart about tracking mappings.
 		 * Unfortunately above, we've just wiped out the mappings
 		 * without telling our object about it. So we need to fake it.
+		 *
+		 * Bind is not expected to fail since this is only called on
+		 * resume and assumption is all requirements exist already.
 		 */
 		vma->bound &= ~GLOBAL_BIND;
-		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
+		WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND));
 	}
 
 
@@ -1538,7 +1543,7 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
 		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
-	intel_gtt_insert_sg_entries(vma->obj->pages, entry, flags);
+	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
 	vma->bound = GLOBAL_BIND;
 }
 
@@ -1588,7 +1593,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
 		if (!(vma->bound & GLOBAL_BIND) ||
 		    (cache_level != obj->cache_level)) {
-			vma->vm->insert_entries(vma->vm, obj->pages,
+			vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages,
 						vma->node.start,
 						cache_level, flags);
 			vma->bound |= GLOBAL_BIND;
@@ -1600,7 +1605,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	     (cache_level != obj->cache_level))) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base,
-					    vma->obj->pages,
+					    vma->ggtt_view.pages,
 					    vma->node.start,
 					    cache_level, flags);
 		vma->bound |= LOCAL_BIND;
@@ -2165,7 +2170,8 @@ int i915_gem_gtt_init(struct drm_device *dev)
 }
 
 static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
-					      struct i915_address_space *vm)
+					      struct i915_address_space *vm,
+					      const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
 	if (vma == NULL)
@@ -2176,6 +2182,7 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 	INIT_LIST_HEAD(&vma->exec_list);
 	vma->vm = vm;
 	vma->obj = obj;
+	vma->ggtt_view = *view;
 
 	switch (INTEL_INFO(vm->dev)->gen) {
 	case 9:
@@ -2210,14 +2217,59 @@ static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
 }
 
 struct i915_vma *
-i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
-				  struct i915_address_space *vm)
+i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
+				       struct i915_address_space *vm,
+				       const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
 
-	vma = i915_gem_obj_to_vma(obj, vm);
+	vma = i915_gem_obj_to_vma_view(obj, vm, view);
 	if (!vma)
-		vma = __i915_gem_vma_create(obj, vm);
+		vma = __i915_gem_vma_create(obj, vm, view);
 
 	return vma;
 }
+
+static inline
+int i915_get_vma_pages(struct i915_vma *vma)
+{
+	if (vma->ggtt_view.pages)
+		return 0;
+
+	if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
+		vma->ggtt_view.pages = vma->obj->pages;
+	else
+		WARN_ONCE(1, "GGTT view %u not implemented!\n",
+			  vma->ggtt_view.type);
+
+	if (!vma->ggtt_view.pages) {
+		DRM_ERROR("Failed to get pages for VMA view type %u!\n",
+			  vma->ggtt_view.type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address space.
+ * @vma: VMA to map
+ * @cache_level: mapping cache level
+ * @flags: flags like global or local mapping
+ *
+ * DMA addresses are taken from the scatter-gather table of this object (or of
+ * this VMA in case of non-default GGTT views) and PTE entries set up.
+ * Note that DMA addresses are also the only part of the SG table we care about.
+ */
+int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
+		  u32 flags)
+{
+	int ret = i915_get_vma_pages(vma);
+
+	if (ret)
+		return ret;
+
+	vma->bind_vma(vma, cache_level, flags);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index dd849df..e377c7d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -109,7 +109,20 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
 #define GEN8_PPAT_ELLC_OVERRIDE		(0<<2)
 #define GEN8_PPAT(i, x)			((uint64_t) (x) << ((i) * 8))
 
+enum i915_ggtt_view_type {
+	I915_GGTT_VIEW_NORMAL = 0,
+};
+
+struct i915_ggtt_view {
+	enum i915_ggtt_view_type type;
+
+	struct sg_table *pages;
+};
+
+extern const struct i915_ggtt_view i915_ggtt_view_normal;
+
 enum i915_cache_level;
+
 /**
  * A VMA represents a GEM BO that is bound into an address space. Therefore, a
  * VMA's presence cannot be guaranteed before binding, or after unbinding the
@@ -129,6 +142,15 @@ struct i915_vma {
 #define PTE_READ_ONLY	(1<<2)
 	unsigned int bound : 4;
 
+	/**
+	 * Support different GGTT views into the same object.
+	 * This means there can be multiple VMA mappings per object and per VM.
+	 * i915_ggtt_view_type is used to distinguish between those entries.
+	 * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also
+	 * assumed in GEM functions which take no ggtt view parameter.
+	 */
+	struct i915_ggtt_view ggtt_view;
+
 	/** This object's place on the active/inactive lists */
 	struct list_head mm_list;
 
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index c4536e1..f97479a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 			break;
 
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
+			if (vma->vm == vm && vma->pin_count > 0)
 				capture_bo(err++, vma);
-				break;
-			}
 	}
 
 	return err - first;
@@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
-			if (vma->vm == vm && vma->pin_count > 0) {
+			if (vma->vm == vm && vma->pin_count > 0)
 				i++;
-				break;
-			}
 	}
 	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
 
-- 
2.1.1

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

[PATCH 2/2] drm/i915: Documentation for multiple GGTT views

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A short section describing background, implementation and intended usage.

v2:
    * Align section name between template and DOC comment. (Michel Thierry)

For: VIZ-4544
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 Documentation/DocBook/drm.tmpl      |  5 ++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 60 +++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 85287cb..eb84da73 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -4033,6 +4033,11 @@ int num_ioctls;</synopsis>
 !Pdrivers/gpu/drm/i915/intel_lrc.c Logical Rings, Logical Ring Contexts and Execlists
 !Idrivers/gpu/drm/i915/intel_lrc.c
       </sect2>
+      <sect2>
+        <title>Global GTT views</title>
+!Pdrivers/gpu/drm/i915/i915_gem_gtt.c Global GTT views
+!Idrivers/gpu/drm/i915/i915_gem_gtt.c
+      </sect2>
     </sect1>
 
     <sect1>
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 73c1c0b..534ebc1 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,66 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+/**
+ * DOC: Global GTT views
+ *
+ * Background and previous state
+ *
+ * Historically objects could exists (be bound) in global GTT space only as
+ * singular instances with a view representing all of the object's backing pages
+ * in a linear fashion. This view will be called a normal view.
+ *
+ * To support multiple views of the same object, where the number of mapped
+ * pages is not equal to the backing store, or where the layout of the pages
+ * is not linear, concept of a GGTT view was added.
+ *
+ * One example of an alternative view is a stereo display driven by a single
+ * image. In this case we would have a framebuffer looking like this
+ * (2x2 pages):
+ *
+ *    12
+ *    34
+ *
+ * Above would represent a normal GGTT view as normally mapped for GPU or CPU
+ * rendering. In contrast, fed to the display engine would be an alternative
+ * view which could look something like this:
+ *
+ *   1212
+ *   3434
+ *
+ * In this example both the size and layout of pages in the alternative view is
+ * different from the normal view.
+ *
+ * Implementation and usage
+ *
+ * GGTT views are implemented using VMAs and are distinguished via enum
+ * i915_ggtt_view_type and struct i915_ggtt_view.
+ *
+ * A new flavour of core GEM functions which work with GGTT bound objects were
+ * added with the _view suffix. They take the struct i915_ggtt_view parameter
+ * encapsulating all metadata required to implement a view.
+ *
+ * As a helper for callers which are only interested in the normal view,
+ * globally const i915_ggtt_view_normal singleton instance exists. All old core
+ * GEM API functions, the ones not taking the view parameter, are operating on,
+ * or with the normal GGTT view.
+ *
+ * Code wanting to add or use a new GGTT view needs to:
+ *
+ * 1. Add a new enum with a suitable name.
+ * 2. Extend the metadata in the i915_ggtt_view structure if required.
+ * 3. Add support to i915_get_vma_pages().
+ *
+ * New views are required to build a scatter-gather table from within the
+ * i915_get_vma_pages function. This table is stored in the vma.ggtt_view and
+ * exists for the lifetime of an VMA.
+ *
+ * Core API is designed to have copy semantics which means that passed in
+ * struct i915_ggtt_view does not need to be persistent (left around after
+ * calling the core API functions).
+ *
+ */
+
 const struct i915_ggtt_view i915_ggtt_view_normal;
 
 static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
-- 
2.1.1

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

Re: [PATCH 2/2] drm/i915: Documentation for multiple GGTT views

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-2              364/366              363/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_setmode_invalid-clone-exclusive-crtc      PASS(5, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset      NSPT(1, M26)PASS(4, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(5, M26)PASS(20, M26M37)      PASS(1, M26)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Michel Thierry]

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

On 12/10/2014 5:27 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
> to map objects into the same address space multiple times.
>
> Added a GGTT view concept and linked it with the VMA to distinguish between
> multiple instances per address space.
>
> New objects and GEM functions which do not take this new view as a parameter
> assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
> previous behaviour.
>
> This now means that objects can have multiple VMA entries so the code which
> assumed there will only be one also had to be modified.
>
> Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
> which is DMA mapped on first VMA instantiation and unmapped on the last one
> going away.
>
> v2:
>      * Removed per view special casing in i915_gem_ggtt_prepare /
>        finish_object in favour of creating and destroying DMA mappings
>        on first VMA instantiation and last VMA destruction. (Daniel Vetter)
>      * Simplified i915_vma_unbind which does not need to count the GGTT views.
>        (Daniel Vetter)
>      * Also moved obj->map_and_fenceable reset under the same check.
>      * Checkpatch cleanups.
>
> v3:
>      * Only retire objects once the last VMA is unbound.
>
> v4:
>      * Keep scatter-gather table for alternative views persistent for the
>        lifetime of the VMA.
>      * Propagate binding errors to callers and handle appropriately.
>
> v5:
>      * Explicitly look for normal GGTT view in i915_gem_obj_bound to align
>        usage in i915_gem_object_ggtt_unpin. (Michel Thierry)
>      * Change to single if statement in i915_gem_obj_to_ggtt. (Michel Thierry)
>      * Removed stray semi-colon in i915_gem_object_set_cache_level.
>
> For: VIZ-4544
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Good catch in i915_gem_object_set_cache_level.

Reviewed-by: Michel Thierry <michel.thierry@intel.com>



[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 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 2/2] drm/i915: Documentation for multiple GGTT views

[Michel Thierry]

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

On 12/10/2014 5:27 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> A short section describing background, implementation and intended usage.
>
> v2:
>      * Align section name between template and DOC comment. (Michel Thierry)
>
> For: VIZ-4544
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Michel Thierry <michel.thierry@intel.com>


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 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 2/2] drm/i915: Documentation for multiple GGTT views

[Daniel Vetter]

On Thu, Dec 11, 2014 at 10:19:39AM +0000, Michel Thierry wrote:
> On 12/10/2014 5:27 PM, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >A short section describing background, implementation and intended usage.
> >
> >v2:
> >     * Align section name between template and DOC comment. (Michel Thierry)
> >
> >For: VIZ-4544
> >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Both merged, thanks for patches and review. Michel, I guess you'll also
review the -internal skl patches which are based on top of this now?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Documentation for multiple GGTT views

[Michel Thierry]

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

On 12/15/2014 8:30 AM, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 10:19:39AM +0000, Michel Thierry wrote:
>> On 12/10/2014 5:27 PM, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> A short section describing background, implementation and intended usage.
>>>
>>> v2:
>>>      * Align section name between template and DOC comment. (Michel Thierry)
>>>
>>> For: VIZ-4544
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
> Both merged, thanks for patches and review. Michel, I guess you'll also
> review the -internal skl patches which are based on top of this now?
> -Daniel
Yes, I already started with them, will reply to -internal soon.

Thanks,

-Michel


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 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 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Daniel Vetter]

On Wed, Dec 10, 2014 at 05:27:58PM +0000, Tvrtko Ursulin wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index de241eb..c6610bc 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2098,8 +2098,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  			/* For the unbound phase, this should be a no-op! */
>  			list_for_each_entry_safe(vma, v,
>  						 &obj->vma_list, vma_link)
> -				if (i915_vma_unbind(vma))
> -					break;
> +				i915_vma_unbind(vma);

This hunk adds a new warning. It's the 2nd time a __must_check warning
from someone from vpg london crept through all the premerge testing. Can
you please double-check whether you have some crazy gcc flags settings or
something in your builders?

I've dropped the hunk locally.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Infrastructure for supporting different GGTT views per object

[Tvrtko Ursulin]

On 12/15/2014 10:24 AM, Daniel Vetter wrote:
> On Wed, Dec 10, 2014 at 05:27:58PM +0000, Tvrtko Ursulin wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index de241eb..c6610bc 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2098,8 +2098,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>>   			/* For the unbound phase, this should be a no-op! */
>>   			list_for_each_entry_safe(vma, v,
>>   						 &obj->vma_list, vma_link)
>> -				if (i915_vma_unbind(vma))
>> -					break;
>> +				i915_vma_unbind(vma);
>
> This hunk adds a new warning. It's the 2nd time a __must_check warning
> from someone from vpg london crept through all the premerge testing. Can
> you please double-check whether you have some crazy gcc flags settings or
> something in your builders?
>
> I've dropped the hunk locally.

Yes sorry, somehow I've lost the relevant flag from Kconfig. Doesn't 
matter for this hunk I suppose since it was in the air anyway.

Regards,

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