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

[Daniel Vetter <daniel@ffwll.ch>]

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