drm/i915: Infrastructure for supporting different GGTT views per object

Commit Message

Tvrtko Ursulin Nov. 27, 2014, 2:52 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

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.

Issue: VIZ-4544
Signed-off-by: Tvrtko Ursulin <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
---
 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(-)

Comments

Shuang He Nov. 28, 2014, 12:59 a.m. UTC | #1

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: [e-mail])
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              364/364              363/364
ILK                 -9              365/366              356/366
SNB                                  450/450              450/450
IVB                                  498/498              498/498
BYT                                  289/289              289/289
HSW                                  564/564              564/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt_gen3_mixed_blits      PASS(2, M25M7)      CRASH(1, M7)
 ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_busy-flip-interruptible      PASS(3, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_flip-vs-panning      PASS(3, M26)      NSPT(1, M26)
*ILK  igt_kms_flip_plain-flip-fb-recreate-interruptible      PASS(3, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_plain-flip-ts-check-interruptible      DMESG_WARN(1, M26)PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-dpms      PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_rcs-flip-vs-modeset      NSPT(1, M26)PASS(2, M26)      DMESG_WARN(1, M26)
*ILK  igt_kms_flip_vblank-vs-hang      PASS(3, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(3, M26)      DMESG_WARN(1, M26)
Note: You need to pay more attention to line start with '*'

Daniel Vetter Nov. 28, 2014, 5:31 p.m. UTC | #2

On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <[e-mail]>
> 
> 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.
> 
> Issue: VIZ-4544
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>

lgtm overall. Please find someone for detailed review for knowledge
sharing (so different geo/team).

A few comemnts and questions below still. Also could you please do a
follow-up patch which adds a DOC: section with a short exlanation of gtt
views and pulls it into the i915 docbook template? We need to start
somewhere with gem docs ...

Cheers, Daniel

> ---
>  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 319da61..1a9569f 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -154,8 +154,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 c4f2cb6..6250a2c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2501,10 +2501,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);
> @@ -2656,18 +2669,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 86cf428..6213c07 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2090,8 +2090,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);

Why drop the early break if a vma_unbind fails? Looks like a superflous
hunk to me.

>  
>  			if (i915_gem_object_put_pages(obj) == 0)
>  				count += obj->base.size >> PAGE_SHIFT;
> @@ -2292,17 +2291,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);
> @@ -3043,7 +3039,8 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	/* 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);

If anyone adds subobject gtt mmap fault support we'll need to undo this
again, but makes sense for the current usecase. So imo ok either way right
now.

>  
>  		/* release the fence reg _after_ flushing */
> @@ -3057,7 +3054,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;

Same here.

>  
>  	drm_mm_remove_node(&vma->node);
> @@ -3476,7 +3474,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;
> @@ -3526,7 +3525,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;
>  
> @@ -3560,7 +3559,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;
> @@ -3768,8 +3767,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)
> @@ -4123,10 +4122,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;
> @@ -4142,7 +4142,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;
> @@ -4152,7 +4152,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);
> @@ -4165,13 +4166,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;
> @@ -4583,12 +4585,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;
> @@ -5272,8 +5275,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;
> @@ -5281,7 +5285,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;
>  
>  	}
> @@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
> +			continue;
> +		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> +			return vma;
> +	}

We fairly put the ggtt vma into the head of the list. Imo better to keep
the head slot reserved for the ggtt normal view, might be some random code
relying upon this.

>  
> -	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 d17ff43..4bf9f79 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -580,8 +580,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 e1ed85a..e79caf9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -358,8 +358,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 bee5b0a..2144738 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:
> @@ -2214,14 +2218,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 d0562d0..3534727 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;
>  
> @@ -161,7 +181,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 89a2f3d..77f1bdc 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -717,10 +717,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;

Not fully sure about this one, but can't hurt I guess.

> -			}
>  	}
>  
>  	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
>

Tvrtko Ursulin Dec. 1, 2014, 11:32 a.m. UTC | #3

On 11/28/2014 05:31 PM, Daniel Vetter wrote:
> On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> 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.
>>
>> Issue: VIZ-4544
>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>
> lgtm overall. Please find someone for detailed review for knowledge
> sharing (so different geo/team).

With the current state of who is doing what and where it made most sense 
for Michel to do this.

> A few comemnts and questions below still. Also could you please do a
> follow-up patch which adds a DOC: section with a short exlanation of gtt
> views and pulls it into the i915 docbook template? We need to start
> somewhere with gem docs ...

Sure, I'll find some previous patches from this area to see how roughly 
it should look like.

> Cheers, Daniel
>
>> ---
>>   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 319da61..1a9569f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -154,8 +154,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 c4f2cb6..6250a2c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2501,10 +2501,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);
>> @@ -2656,18 +2669,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 86cf428..6213c07 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2090,8 +2090,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);
>
> Why drop the early break if a vma_unbind fails? Looks like a superflous
> hunk to me.

I wasn't sure about this. (Does it makes sense to try and unbind other 
VMAs if one couldn't be unbound?)

In fact, looking at it now, I am not sure about the unbind flow 
(i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to 
inactive list on first VMA unbind? Retire only on last VMA going away?

>> @@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
>> +			continue;
>> +		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>> +			return vma;
>> +	}
>
> We fairly put the ggtt vma into the head of the list. Imo better to keep
> the head slot reserved for the ggtt normal view, might be some random code
> relying upon this.

Ok.

>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 89a2f3d..77f1bdc 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -717,10 +717,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;
>
> Not fully sure about this one, but can't hurt I guess.

Not sure if it is useful at the moment or at all?

Regards,

Tvrtko

Tvrtko Ursulin Dec. 1, 2014, 2:46 p.m. UTC | #4

On 12/01/2014 11:32 AM, Tvrtko Ursulin wrote:
>>> @@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
>>> drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
>>> +            continue;
>>> +        if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>>> +            return vma;
>>> +    }
>>
>> We fairly put the ggtt vma into the head of the list. Imo better to keep
>> the head slot reserved for the ggtt normal view, might be some random
>> code
>> relying upon this.
>
> Ok.

Although on a second thought - I am not sure this makes sense since 
alternative views can exist without the normal one. Thoughts?

Regards,

Tvrtko

Daniel Vetter Dec. 1, 2014, 4:01 p.m. UTC | #5

On Mon, Dec 01, 2014 at 02:46:29PM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2014 11:32 AM, Tvrtko Ursulin wrote:
> >>>@@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
> >>>drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
> >>>+            continue;
> >>>+        if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> >>>+            return vma;
> >>>+    }
> >>
> >>We fairly put the ggtt vma into the head of the list. Imo better to keep
> >>the head slot reserved for the ggtt normal view, might be some random
> >>code
> >>relying upon this.
> >
> >Ok.
> 
> Although on a second thought - I am not sure this makes sense since
> alternative views can exist without the normal one. Thoughts?

Yeah, hence we need to put the normal ggtt view at the front and
everything else at the back. I'm just somewhat afraid of something
expecting the normal ggtt view to be the first one and which then
accidentally breaks.

But if you think this is too much fuzz then please split this change out
into a separate patch (i.e. the change to the lookup loop + no longer
inserting ggtt vmas a the front). That way when any regression bisects to
this patch it's clear what's going on.
-Daniel

Daniel Vetter Dec. 1, 2014, 4:07 p.m. UTC | #6

On Mon, Dec 01, 2014 at 11:32:42AM +0000, Tvrtko Ursulin wrote:
> On 11/28/2014 05:31 PM, Daniel Vetter wrote:
> >On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <[e-mail]>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index 86cf428..6213c07 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -2090,8 +2090,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);
> >
> >Why drop the early break if a vma_unbind fails? Looks like a superflous
> >hunk to me.
> 
> I wasn't sure about this. (Does it makes sense to try and unbind other VMAs
> if one couldn't be unbound?)
> 
> In fact, looking at it now, I am not sure about the unbind flow
> (i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to inactive
> list on first VMA unbind? Retire only on last VMA going away?

Yeah only the first vma_unbind might fail with the current code. The
problem though is that you ignore all failures.

Aside: In general this is also about reducing the size of the diff to only
have essential changes. I'm happy when people throw in more cleanup, but
it should be done as follow-up/prep patches. This is because often the
only evidence for fixing a bug we have is "it bisects to this commit". So
making commits in complex code like gem minimal is the name of the game.

> >>@@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
> >>+			continue;
> >>+		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> >>+			return vma;
> >>+	}
> >
> >We fairly put the ggtt vma into the head of the list. Imo better to keep
> >the head slot reserved for the ggtt normal view, might be some random code
> >relying upon this.
> 
> Ok.
> 
> >>diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>index 89a2f3d..77f1bdc 100644
> >>--- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >>+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>@@ -717,10 +717,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;
> >
> >Not fully sure about this one, but can't hurt I guess.
> 
> Not sure if it is useful at the moment or at all?

Probably not useful right now. Otoh if we ever wire up the display fault
registers on modern platforms this migh become useful to cross-check that
the current display plane register settings match up with the
corresponding buffer. Won't hurt either though.

If you feel like make it a separate patch perhaps.
-Daniel

Tvrtko Ursulin Dec. 1, 2014, 4:34 p.m. UTC | #7

On 12/01/2014 04:07 PM, Daniel Vetter wrote:
> On Mon, Dec 01, 2014 at 11:32:42AM +0000, Tvrtko Ursulin wrote:
>> On 11/28/2014 05:31 PM, Daniel Vetter wrote:
>>> On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <[e-mail]>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 86cf428..6213c07 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -2090,8 +2090,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);
>>>
>>> Why drop the early break if a vma_unbind fails? Looks like a superflous
>>> hunk to me.
>>
>> I wasn't sure about this. (Does it makes sense to try and unbind other VMAs
>> if one couldn't be unbound?)
>>
>> In fact, looking at it now, I am not sure about the unbind flow
>> (i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to inactive
>> list on first VMA unbind? Retire only on last VMA going away?
>
> Yeah only the first vma_unbind might fail with the current code. The
> problem though is that you ignore all failures.

I am not sure what you mean. Why only the first unbind can fail?

The part I was unsure about was this break removal in the shrinker. 
Whether or not it makes sense to go through all VMAs regardless if one 
failed to unbind? Is there any space to be gained by doing that?

Alternatively, I also looked at it as: If it doesn't make sense to go 
through all of then, then what to do if the first unbind succeeds and 
some other fails?  End results sounds the same as trying to unbind as 
much as possible. So I opted for doing that.

My second concern is that object retire on 1st VMA unbind. Should that 
only be done when the last VMA is going away?

As it stands (in my v2 patch) it can move all VMAs onto the inactive 
list when first one is unbound which looks wrong.

> Aside: In general this is also about reducing the size of the diff to only
> have essential changes. I'm happy when people throw in more cleanup, but
> it should be done as follow-up/prep patches. This is because often the
> only evidence for fixing a bug we have is "it bisects to this commit". So
> making commits in complex code like gem minimal is the name of the game.

Sure, as soon as one understands what is essential and what is not. :)

>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>> index 89a2f3d..77f1bdc 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>> @@ -717,10 +717,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;
>>>
>>> Not fully sure about this one, but can't hurt I guess.
>>
>> Not sure if it is useful at the moment or at all?
>
> Probably not useful right now. Otoh if we ever wire up the display fault
> registers on modern platforms this migh become useful to cross-check that
> the current display plane register settings match up with the
> corresponding buffer. Won't hurt either though.
>
> If you feel like make it a separate patch perhaps.

I don't know, it sounds like an overkill to do that for this short hunk 
so I prefer to leave it in if you don't mind.

Regards,

Tvrtko

Tvrtko Ursulin Dec. 1, 2014, 4:39 p.m. UTC | #8

On 12/01/2014 04:01 PM, Daniel Vetter wrote:
> On Mon, Dec 01, 2014 at 02:46:29PM +0000, Tvrtko Ursulin wrote:
>>
>> On 12/01/2014 11:32 AM, Tvrtko Ursulin wrote:
>>>>> @@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
>>>>> drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
>>>>> +            continue;
>>>>> +        if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>>>>> +            return vma;
>>>>> +    }
>>>>
>>>> We fairly put the ggtt vma into the head of the list. Imo better to keep
>>>> the head slot reserved for the ggtt normal view, might be some random
>>>> code
>>>> relying upon this.
>>>
>>> Ok.
>>
>> Although on a second thought - I am not sure this makes sense since
>> alternative views can exist without the normal one. Thoughts?
>
> Yeah, hence we need to put the normal ggtt view at the front and
> everything else at the back. I'm just somewhat afraid of something
> expecting the normal ggtt view to be the first one and which then
> accidentally breaks.

No, my point was that we can't guarantee that since the first entry will 
be an alternative view when it is the only item on the list. So in the 
light of that does it make sense to bother with this special casing at all?

> But if you think this is too much fuzz then please split this change out
> into a separate patch (i.e. the change to the lookup loop + no longer
> inserting ggtt vmas a the front). That way when any regression bisects to
> this patch it's clear what's going on.

To double check if I understand what you mean, you lost me with "fuzz":

If you agree with my comment above, then the recommendation is for 
another prep patch to do what you say here? Ie. remove the special case 
for GGTT VMA list_add ?

i915_gem_obj_ti_ggtt can remain untouched in that prep patch, in my 
view, since it will be the only GGTT VMA, no?

Regards,

Tvrtko

Daniel Vetter Dec. 1, 2014, 5:16 p.m. UTC | #9

On Mon, Dec 01, 2014 at 04:39:36PM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2014 04:01 PM, Daniel Vetter wrote:
> >On Mon, Dec 01, 2014 at 02:46:29PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 12/01/2014 11:32 AM, Tvrtko Ursulin wrote:
> >>>>>@@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
> >>>>>drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
> >>>>>+            continue;
> >>>>>+        if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
> >>>>>+            return vma;
> >>>>>+    }
> >>>>
> >>>>We fairly put the ggtt vma into the head of the list. Imo better to keep
> >>>>the head slot reserved for the ggtt normal view, might be some random
> >>>>code
> >>>>relying upon this.
> >>>
> >>>Ok.
> >>
> >>Although on a second thought - I am not sure this makes sense since
> >>alternative views can exist without the normal one. Thoughts?
> >
> >Yeah, hence we need to put the normal ggtt view at the front and
> >everything else at the back. I'm just somewhat afraid of something
> >expecting the normal ggtt view to be the first one and which then
> >accidentally breaks.
> 
> No, my point was that we can't guarantee that since the first entry will be
> an alternative view when it is the only item on the list. So in the light of
> that does it make sense to bother with this special casing at all?
> 
> >But if you think this is too much fuzz then please split this change out
> >into a separate patch (i.e. the change to the lookup loop + no longer
> >inserting ggtt vmas a the front). That way when any regression bisects to
> >this patch it's clear what's going on.
> 
> To double check if I understand what you mean, you lost me with "fuzz":

Hm I guess my understanding of fuzz isn't quite in line with common usage.
I've meant "hairy work". Oh well ...

> If you agree with my comment above, then the recommendation is for another
> prep patch to do what you say here? Ie. remove the special case for GGTT VMA
> list_add ?

Yes.

> i915_gem_obj_ti_ggtt can remain untouched in that prep patch, in my view,
> since it will be the only GGTT VMA, no?

If you don't insert the ggtt view any more at the front the current
obj_to_ggtt won't work any more. So you must change that in the same patch
(otherwise you break bisectability, which is the entire point of the
split-out).
-Daniel

Daniel Vetter Dec. 1, 2014, 5:19 p.m. UTC | #10

On Mon, Dec 01, 2014 at 04:34:16PM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2014 04:07 PM, Daniel Vetter wrote:
> >On Mon, Dec 01, 2014 at 11:32:42AM +0000, Tvrtko Ursulin wrote:
> >>On 11/28/2014 05:31 PM, Daniel Vetter wrote:
> >>>On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
> >>>>From: Tvrtko Ursulin <[e-mail]>
> >>>>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>>>index 86cf428..6213c07 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>>>@@ -2090,8 +2090,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);
> >>>
> >>>Why drop the early break if a vma_unbind fails? Looks like a superflous
> >>>hunk to me.
> >>
> >>I wasn't sure about this. (Does it makes sense to try and unbind other VMAs
> >>if one couldn't be unbound?)
> >>
> >>In fact, looking at it now, I am not sure about the unbind flow
> >>(i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to inactive
> >>list on first VMA unbind? Retire only on last VMA going away?
> >
> >Yeah only the first vma_unbind might fail with the current code. The
> >problem though is that you ignore all failures.
> 
> I am not sure what you mean. Why only the first unbind can fail?
> 
> The part I was unsure about was this break removal in the shrinker. Whether
> or not it makes sense to go through all VMAs regardless if one failed to
> unbind? Is there any space to be gained by doing that?
> 
> Alternatively, I also looked at it as: If it doesn't make sense to go
> through all of then, then what to do if the first unbind succeeds and some
> other fails?  End results sounds the same as trying to unbind as much as
> possible. So I opted for doing that.
> 
> My second concern is that object retire on 1st VMA unbind. Should that only
> be done when the last VMA is going away?
> 
> As it stands (in my v2 patch) it can move all VMAs onto the inactive list
> when first one is unbound which looks wrong.

Well I've started this discussion by simply asking why we need this. I
think both versions are correct.

> >Aside: In general this is also about reducing the size of the diff to only
> >have essential changes. I'm happy when people throw in more cleanup, but
> >it should be done as follow-up/prep patches. This is because often the
> >only evidence for fixing a bug we have is "it bisects to this commit". So
> >making commits in complex code like gem minimal is the name of the game.
> 
> Sure, as soon as one understands what is essential and what is not. :)
> 
> >>>>diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>index 89a2f3d..77f1bdc 100644
> >>>>--- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>@@ -717,10 +717,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;
> >>>
> >>>Not fully sure about this one, but can't hurt I guess.
> >>
> >>Not sure if it is useful at the moment or at all?
> >
> >Probably not useful right now. Otoh if we ever wire up the display fault
> >registers on modern platforms this migh become useful to cross-check that
> >the current display plane register settings match up with the
> >corresponding buffer. Won't hurt either though.
> >
> >If you feel like make it a separate patch perhaps.
> 
> I don't know, it sounds like an overkill to do that for this short hunk so I
> prefer to leave it in if you don't mind.

Unfortunately "let's just leave this slightly unrelated hunk in the patch
because too much work to split it out" has bitten me countless times in
gem. So if it's really just cleanup (we seem to agree that both old&new
work) but not cleanup enough to justify it's own patch then I'd like to
drop it. Not least because churn for churn's sake just makes everyone's
live more painful (especially backporters).
-Daniel

Tvrtko Ursulin Dec. 1, 2014, 5:24 p.m. UTC | #11

On 12/01/2014 05:16 PM, Daniel Vetter wrote:
> On Mon, Dec 01, 2014 at 04:39:36PM +0000, Tvrtko Ursulin wrote:
>>
>> On 12/01/2014 04:01 PM, Daniel Vetter wrote:
>>> On Mon, Dec 01, 2014 at 02:46:29PM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 12/01/2014 11:32 AM, Tvrtko Ursulin wrote:
>>>>>>> @@ -5430,9 +5434,12 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct
>>>>>>> drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
>>>>>>> +            continue;
>>>>>>> +        if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
>>>>>>> +            return vma;
>>>>>>> +    }
>>>>>>
>>>>>> We fairly put the ggtt vma into the head of the list. Imo better to keep
>>>>>> the head slot reserved for the ggtt normal view, might be some random
>>>>>> code
>>>>>> relying upon this.
>>>>>
>>>>> Ok.
>>>>
>>>> Although on a second thought - I am not sure this makes sense since
>>>> alternative views can exist without the normal one. Thoughts?
>>>
>>> Yeah, hence we need to put the normal ggtt view at the front and
>>> everything else at the back. I'm just somewhat afraid of something
>>> expecting the normal ggtt view to be the first one and which then
>>> accidentally breaks.
>>
>> No, my point was that we can't guarantee that since the first entry will be
>> an alternative view when it is the only item on the list. So in the light of
>> that does it make sense to bother with this special casing at all?
>>
>>> But if you think this is too much fuzz then please split this change out
>>> into a separate patch (i.e. the change to the lookup loop + no longer
>>> inserting ggtt vmas a the front). That way when any regression bisects to
>>> this patch it's clear what's going on.
>>
>> To double check if I understand what you mean, you lost me with "fuzz":
>
> Hm I guess my understanding of fuzz isn't quite in line with common usage.
> I've meant "hairy work". Oh well ...

Since patch(1) sorts out simple mismatches with "fuzz" to me it is a 
synonym for trivial. :)

>> If you agree with my comment above, then the recommendation is for another
>> prep patch to do what you say here? Ie. remove the special case for GGTT VMA
>> list_add ?
>
> Yes.
>
>> i915_gem_obj_ti_ggtt can remain untouched in that prep patch, in my view,
>> since it will be the only GGTT VMA, no?
>
> If you don't insert the ggtt view any more at the front the current
> obj_to_ggtt won't work any more. So you must change that in the same patch
> (otherwise you break bisectability, which is the entire point of the
> split-out).

Ah yes, I overlooked how the current obj_to_ggtt looks. Or to better say 
I only looked at my version and assumed I only added the view type check.

Regards,

Tvrtko

Tvrtko Ursulin Dec. 1, 2014, 5:50 p.m. UTC | #12

On 12/01/2014 05:19 PM, Daniel Vetter wrote:
> On Mon, Dec 01, 2014 at 04:34:16PM +0000, Tvrtko Ursulin wrote:
>>
>> On 12/01/2014 04:07 PM, Daniel Vetter wrote:
>>> On Mon, Dec 01, 2014 at 11:32:42AM +0000, Tvrtko Ursulin wrote:
>>>> On 11/28/2014 05:31 PM, Daniel Vetter wrote:
>>>>> On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <[e-mail]>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>>>> index 86cf428..6213c07 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>>> @@ -2090,8 +2090,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);
>>>>>
>>>>> Why drop the early break if a vma_unbind fails? Looks like a superflous
>>>>> hunk to me.
>>>>
>>>> I wasn't sure about this. (Does it makes sense to try and unbind other VMAs
>>>> if one couldn't be unbound?)
>>>>
>>>> In fact, looking at it now, I am not sure about the unbind flow
>>>> (i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to inactive
>>>> list on first VMA unbind? Retire only on last VMA going away?
>>>
>>> Yeah only the first vma_unbind might fail with the current code. The
>>> problem though is that you ignore all failures.
>>
>> I am not sure what you mean. Why only the first unbind can fail?
>>
>> The part I was unsure about was this break removal in the shrinker. Whether
>> or not it makes sense to go through all VMAs regardless if one failed to
>> unbind? Is there any space to be gained by doing that?
>>
>> Alternatively, I also looked at it as: If it doesn't make sense to go
>> through all of then, then what to do if the first unbind succeeds and some
>> other fails?  End results sounds the same as trying to unbind as much as
>> possible. So I opted for doing that.
>>
>> My second concern is that object retire on 1st VMA unbind. Should that only
>> be done when the last VMA is going away?
>>
>> As it stands (in my v2 patch) it can move all VMAs onto the inactive list
>> when first one is unbound which looks wrong.
>
> Well I've started this discussion by simply asking why we need this. I
> think both versions are correct.

OK I'll try to reverse engineer more before v3 to try and establish the 
answers to the retire question.

>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>>> index 89a2f3d..77f1bdc 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>>>>>> @@ -717,10 +717,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;
>>>>>
>>>>> Not fully sure about this one, but can't hurt I guess.
>>>>
>>>> Not sure if it is useful at the moment or at all?
>>>
>>> Probably not useful right now. Otoh if we ever wire up the display fault
>>> registers on modern platforms this migh become useful to cross-check that
>>> the current display plane register settings match up with the
>>> corresponding buffer. Won't hurt either though.
>>>
>>> If you feel like make it a separate patch perhaps.
>>
>> I don't know, it sounds like an overkill to do that for this short hunk so I
>> prefer to leave it in if you don't mind.
>
> Unfortunately "let's just leave this slightly unrelated hunk in the patch
> because too much work to split it out" has bitten me countless times in
> gem. So if it's really just cleanup (we seem to agree that both old&new
> work) but not cleanup enough to justify it's own patch then I'd like to
> drop it. Not least because churn for churn's sake just makes everyone's
> live more painful (especially backporters).

Overkill wasn't the correct word choice - I did not mean it is too much 
work to split it out if you imply laziness. I really saw it as more than 
just cleanup.  And when you said "if you feel like it" I didn't bother 
explaining in detail why I think it makes sense for it to stay together.

It will "work" with or without, correct. Just won't capture more than 
one VMA in the error state, after the patch adding support for multiple 
VMAs. So it kind of does and doesn't work, depends how you look at it.

I did not think that to be a valuable split for the sake of a trivial 
hunk like the one above. But OK, I can split it out no big deal.

Regards,

Tvrtko

Daniel Vetter Dec. 2, 2014, 9:12 a.m. UTC | #13

On Mon, Dec 01, 2014 at 05:50:24PM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/2014 05:19 PM, Daniel Vetter wrote:
> >On Mon, Dec 01, 2014 at 04:34:16PM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 12/01/2014 04:07 PM, Daniel Vetter wrote:
> >>>On Mon, Dec 01, 2014 at 11:32:42AM +0000, Tvrtko Ursulin wrote:
> >>>>On 11/28/2014 05:31 PM, Daniel Vetter wrote:
> >>>>>On Thu, Nov 27, 2014 at 02:52:44PM +0000, Tvrtko Ursulin wrote:
> >>>>>>From: Tvrtko Ursulin <[e-mail]>
> >>>>>>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>>>>>index 86cf428..6213c07 100644
> >>>>>>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>>>>>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>>>>>@@ -2090,8 +2090,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);
> >>>>>
> >>>>>Why drop the early break if a vma_unbind fails? Looks like a superflous
> >>>>>hunk to me.
> >>>>
> >>>>I wasn't sure about this. (Does it makes sense to try and unbind other VMAs
> >>>>if one couldn't be unbound?)
> >>>>
> >>>>In fact, looking at it now, I am not sure about the unbind flow
> >>>>(i915_vma_unbind). Won't i915_gem_object_retire move all VMAs to inactive
> >>>>list on first VMA unbind? Retire only on last VMA going away?
> >>>
> >>>Yeah only the first vma_unbind might fail with the current code. The
> >>>problem though is that you ignore all failures.
> >>
> >>I am not sure what you mean. Why only the first unbind can fail?
> >>
> >>The part I was unsure about was this break removal in the shrinker. Whether
> >>or not it makes sense to go through all VMAs regardless if one failed to
> >>unbind? Is there any space to be gained by doing that?
> >>
> >>Alternatively, I also looked at it as: If it doesn't make sense to go
> >>through all of then, then what to do if the first unbind succeeds and some
> >>other fails?  End results sounds the same as trying to unbind as much as
> >>possible. So I opted for doing that.
> >>
> >>My second concern is that object retire on 1st VMA unbind. Should that only
> >>be done when the last VMA is going away?
> >>
> >>As it stands (in my v2 patch) it can move all VMAs onto the inactive list
> >>when first one is unbound which looks wrong.
> >
> >Well I've started this discussion by simply asking why we need this. I
> >think both versions are correct.
> 
> OK I'll try to reverse engineer more before v3 to try and establish the
> answers to the retire question.

Just to clarify: This comment was in the same context as the longer
explanation below. So three options imo are:
- We really need this (and then it probably needs a comment in the code or
  explanation in the commit message).
- If it's a cleanup, split it out if it's worth it.
- Or drop the hunk completely.

> >>>>>>diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>>>index 89a2f3d..77f1bdc 100644
> >>>>>>--- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>>>+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >>>>>>@@ -717,10 +717,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;
> >>>>>
> >>>>>Not fully sure about this one, but can't hurt I guess.
> >>>>
> >>>>Not sure if it is useful at the moment or at all?
> >>>
> >>>Probably not useful right now. Otoh if we ever wire up the display fault
> >>>registers on modern platforms this migh become useful to cross-check that
> >>>the current display plane register settings match up with the
> >>>corresponding buffer. Won't hurt either though.
> >>>
> >>>If you feel like make it a separate patch perhaps.
> >>
> >>I don't know, it sounds like an overkill to do that for this short hunk so I
> >>prefer to leave it in if you don't mind.
> >
> >Unfortunately "let's just leave this slightly unrelated hunk in the patch
> >because too much work to split it out" has bitten me countless times in
> >gem. So if it's really just cleanup (we seem to agree that both old&new
> >work) but not cleanup enough to justify it's own patch then I'd like to
> >drop it. Not least because churn for churn's sake just makes everyone's
> >live more painful (especially backporters).
> 
> Overkill wasn't the correct word choice - I did not mean it is too much work
> to split it out if you imply laziness. I really saw it as more than just
> cleanup.  And when you said "if you feel like it" I didn't bother explaining
> in detail why I think it makes sense for it to stay together.
> 
> It will "work" with or without, correct. Just won't capture more than one
> VMA in the error state, after the patch adding support for multiple VMAs. So
> it kind of does and doesn't work, depends how you look at it.
> 
> I did not think that to be a valuable split for the sake of a trivial hunk
> like the one above. But OK, I can split it out no big deal.

Hm, the code around is a bit a mess and probably did break somewhere a
while ago, which is a bit confusing. But yeah if we have platforms with
special ggtt views and not full ppgtt enabled we'll indeed run the risk of
capturing the wrong bo.

So looks good now to me, maybe with a small comment added to the commit
message for dummies like me?
-Daniel

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 319da61..1a9569f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -154,8 +154,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 c4f2cb6..6250a2c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2501,10 +2501,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);
@@ -2656,18 +2669,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 86cf428..6213c07 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2090,8 +2090,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;
@@ -2292,17 +2291,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);
@@ -3043,7 +3039,8 @@  int i915_vma_unbind(struct i915_vma *vma)
 	/* 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 */
@@ -3057,7 +3054,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);
@@ -3476,7 +3474,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;
@@ -3526,7 +3525,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;
 
@@ -3560,7 +3559,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;
@@ -3768,8 +3767,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)
@@ -4123,10 +4122,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;
@@ -4142,7 +4142,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;
@@ -4152,7 +4152,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);
@@ -4165,13 +4166,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;
@@ -4583,12 +4585,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;
@@ -5272,8 +5275,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;
@@ -5281,7 +5285,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;
 
 	}
@@ -5430,9 +5434,12 @@  struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *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 != i915_obj_to_ggtt(obj))
+			continue;
+		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL)
+			return vma;
+	}
 
-	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 d17ff43..4bf9f79 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -580,8 +580,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 e1ed85a..e79caf9 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -358,8 +358,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 bee5b0a..2144738 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:
@@ -2214,14 +2218,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 d0562d0..3534727 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;
 
@@ -161,7 +181,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 89a2f3d..77f1bdc 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -717,10 +717,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];