diff mbox

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

Message ID 1417781506-1730-2-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Dec. 5, 2014, 12:11 p.m. UTC
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(-)

Comments

Daniel Vetter Dec. 5, 2014, 2:25 p.m. UTC | #1
On Fri, Dec 05, 2014 at 12:11:45PM +0000, Tvrtko Ursulin wrote:
> +struct i915_ggtt_view {
> +	enum i915_ggtt_view_type type;
> +
> +	struct sg_table *pages;
> +};

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

Anyway just bikesheds ;-)
-Daniel

> +
> +extern const struct i915_ggtt_view i915_ggtt_view_normal;
> +
>  enum i915_cache_level;
> +
>  /**
>   * A VMA represents a GEM BO that is bound into an address space. Therefore, a
>   * VMA's presence cannot be guaranteed before binding, or after unbinding the
> @@ -129,6 +142,15 @@ struct i915_vma {
>  #define PTE_READ_ONLY	(1<<2)
>  	unsigned int bound : 4;
>  
> +	/**
> +	 * Support different GGTT views into the same object.
> +	 * This means there can be multiple VMA mappings per object and per VM.
> +	 * i915_ggtt_view_type is used to distinguish between those entries.
> +	 * The default one of zero (I915_GGTT_VIEW_NORMAL) is default and also
> +	 * assumed in GEM functions which take no ggtt view parameter.
> +	 */
> +	struct i915_ggtt_view ggtt_view;
> +
>  	/** This object's place on the active/inactive lists */
>  	struct list_head mm_list;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c4536e1..f97479a 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -718,10 +718,8 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
>  			break;
>  
>  		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (vma->vm == vm && vma->pin_count > 0) {
> +			if (vma->vm == vm && vma->pin_count > 0)
>  				capture_bo(err++, vma);
> -				break;
> -			}
>  	}
>  
>  	return err - first;
> @@ -1096,10 +1094,8 @@ static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
>  
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>  		list_for_each_entry(vma, &obj->vma_list, vma_link)
> -			if (vma->vm == vm && vma->pin_count > 0) {
> +			if (vma->vm == vm && vma->pin_count > 0)
>  				i++;
> -				break;
> -			}
>  	}
>  	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
>  
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Michel Thierry Dec. 9, 2014, 3:23 p.m. UTC | #2
On 12/5/2014 12:11 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Things like reliable GGTT mappings and mirrored 2d-on-3d display will need
> to map objects into the same address space multiple times.
>
> Added a GGTT view concept and linked it with the VMA to distinguish between
> multiple instances per address space.
>
> New objects and GEM functions which do not take this new view as a parameter
> assume the default of zero (I915_GGTT_VIEW_NORMAL) which preserves the
> previous behaviour.
>
> This now means that objects can have multiple VMA entries so the code which
> assumed there will only be one also had to be modified.
>
> Alternative GGTT views are supposed to borrow DMA addresses from obj->pages
> which is DMA mapped on first VMA instantiation and unmapped on the last one
> going away.
>
> v2:
>      * Removed per view special casing in i915_gem_ggtt_prepare /
>        finish_object in favour of creating and destroying DMA mappings
>        on first VMA instantiation and last VMA destruction. (Daniel Vetter)
>      * Simplified i915_vma_unbind which does not need to count the GGTT views.
>        (Daniel Vetter)
>      * Also moved obj->map_and_fenceable reset under the same check.
>      * Checkpatch cleanups.
>
> v3:
>      * Only retire objects once the last VMA is unbound.
>
> v4:
>      * Keep scatter-gather table for alternative views persistent for the
>        lifetime of the VMA.
>      * Propagate binding errors to callers and handle appropriately.
>
> For: VIZ-4544
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        |   5 +-
>   drivers/gpu/drm/i915/i915_drv.h            |  46 +++++++++++--
>   drivers/gpu/drm/i915/i915_gem.c            | 101 ++++++++++++++++++-----------
>   drivers/gpu/drm/i915/i915_gem_context.c    |  11 +++-
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   9 ++-
>   drivers/gpu/drm/i915/i915_gem_gtt.c        |  70 +++++++++++++++++---
>   drivers/gpu/drm/i915/i915_gem_gtt.h        |  22 +++++++
>   drivers/gpu/drm/i915/i915_gpu_error.c      |   8 +--
>   8 files changed, 206 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6c16939..bd08289 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -152,8 +152,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>   			seq_puts(m, " (pp");
>   		else
>   			seq_puts(m, " (g");
> -		seq_printf(m, "gtt offset: %08lx, size: %08lx)",
> -			   vma->node.start, vma->node.size);
> +		seq_printf(m, "gtt offset: %08lx, size: %08lx, type: %u)",
> +			   vma->node.start, vma->node.size,
> +			   vma->ggtt_view.type);
>   	}
>   	if (obj->stolen)
>   		seq_printf(m, " (stolen: %08lx)", obj->stolen->start);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 049482f..b2f6f7d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2514,10 +2514,23 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
>   #define PIN_GLOBAL 0x4
>   #define PIN_OFFSET_BIAS 0x8
>   #define PIN_OFFSET_MASK (~4095)
> +int __must_check i915_gem_object_pin_view(struct drm_i915_gem_object *obj,
> +					  struct i915_address_space *vm,
> +					  uint32_t alignment,
> +					  uint64_t flags,
> +					  const struct i915_ggtt_view *view);
> +static inline
>   int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
>   				     struct i915_address_space *vm,
>   				     uint32_t alignment,
> -				     uint64_t flags);
> +				     uint64_t flags)
> +{
> +	return i915_gem_object_pin_view(obj, vm, alignment, flags,
> +						&i915_ggtt_view_normal);
> +}
> +
> +int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> +		  u32 flags);
>   int __must_check i915_vma_unbind(struct i915_vma *vma);
>   int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
>   void i915_gem_release_all_mmaps(struct drm_i915_private *dev_priv);
> @@ -2679,18 +2692,43 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>   
>   void i915_gem_restore_fences(struct drm_device *dev);
>   
> +unsigned long i915_gem_obj_offset_view(struct drm_i915_gem_object *o,
> +				       struct i915_address_space *vm,
> +				       enum i915_ggtt_view_type view);
> +static inline
>   unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
> -				  struct i915_address_space *vm);
> +				  struct i915_address_space *vm)
> +{
> +	return i915_gem_obj_offset_view(o, vm, I915_GGTT_VIEW_NORMAL);
> +}
>   bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o);
>   bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
>   			struct i915_address_space *vm);
>   unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
>   				struct i915_address_space *vm);
> +struct i915_vma *i915_gem_obj_to_vma_view(struct drm_i915_gem_object *obj,
> +					  struct i915_address_space *vm,
> +					  const struct i915_ggtt_view *view);
> +static inline
>   struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
> -				     struct i915_address_space *vm);
> +				     struct i915_address_space *vm)
> +{
> +	return i915_gem_obj_to_vma_view(obj, vm, &i915_ggtt_view_normal);
> +}
> +
> +struct i915_vma *
> +i915_gem_obj_lookup_or_create_vma_view(struct drm_i915_gem_object *obj,
> +				       struct i915_address_space *vm,
> +				       const struct i915_ggtt_view *view);
> +
> +static inline
>   struct i915_vma *
>   i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> -				  struct i915_address_space *vm);
> +				  struct i915_address_space *vm)
> +{
> +	return i915_gem_obj_lookup_or_create_vma_view(obj, vm,
> +						&i915_ggtt_view_normal);
> +}
Hi,

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

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

Everything else looks good to me.

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

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

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

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

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

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

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

Regards,

Tvrtko
diff mbox

Patch

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