Re: [PATCH 6/6] drm/i915: Create VMAs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2013-07-17 at 12:19 -0700, Ben Widawsky wrote:
> Formerly: "drm/i915: Create VMAs (part 1)"
> 
> In a previous patch, the notion of a VM was introduced. A VMA describes
> an area of part of the VM address space. A VMA is similar to the concept
> in the linux mm. However, instead of representing regular memory, a VMA
> is backed by a GEM BO. There may be many VMAs for a given object, one
> for each VM the object is to be used in. This may occur through flink,
> dma-buf, or a number of other transient states.
> 
> Currently the code depends on only 1 VMA per object, for the global GTT
> (and aliasing PPGTT). The following patches will address this and make
> the rest of the infrastructure more suited
> 
> v2: s/i915_obj/i915_gem_obj (Chris)
> 
> v3: Only move an object to the now global unbound list if there are no
> more VMAs for the object which are bound into a VM (ie. the list is
> empty).
> 
> v4: killed obj->gtt_space
> some reworks due to rebase
> 
> v5: Free vma on error path (Imre)
> 
> v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
> (Imre)
> Fixed vma freeing in stolen preallocation (Imre)
> 
> Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>

Looks ok, so on patches 5-6:
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>

> ---
>  drivers/gpu/drm/i915/i915_drv.h        | 48 +++++++++++++++++-----
>  drivers/gpu/drm/i915/i915_gem.c        | 74 +++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_gem_evict.c  | 12 ++++--
>  drivers/gpu/drm/i915/i915_gem_gtt.c    |  5 ++-
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 15 +++++--
>  5 files changed, 120 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b3ba428..1a32412 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -533,6 +533,17 @@ struct i915_hw_ppgtt {
>  	int (*enable)(struct drm_device *dev);
>  };
>  
> +/* To make things as simple as possible (ie. no refcounting), a VMA's lifetime
> + * will always be <= an objects lifetime. So object refcounting should cover us.
> + */
> +struct i915_vma {
> +	struct drm_mm_node node;
> +	struct drm_i915_gem_object *obj;
> +	struct i915_address_space *vm;
> +
> +	struct list_head vma_link; /* Link in the object's VMA list */
> +};
> +
>  struct i915_ctx_hang_stats {
>  	/* This context had batch pending when hang was declared */
>  	unsigned batch_pending;
> @@ -1229,8 +1240,9 @@ struct drm_i915_gem_object {
>  
>  	const struct drm_i915_gem_object_ops *ops;
>  
> -	/** Current space allocated to this object in the GTT, if any. */
> -	struct drm_mm_node gtt_space;
> +	/** List of VMAs backed by this object */
> +	struct list_head vma_list;
> +
>  	/** Stolen memory for this object, instead of being backed by shmem. */
>  	struct drm_mm_node *stolen;
>  	struct list_head global_list;
> @@ -1356,18 +1368,32 @@ struct drm_i915_gem_object {
>  
>  #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
>  
> -/* Offset of the first PTE pointing to this object */
> -static inline unsigned long
> -i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +/* This is a temporary define to help transition us to real VMAs. If you see
> + * this, you're either reviewing code, or bisecting it. */
> +static inline struct i915_vma *
> +__i915_gem_obj_to_vma(struct drm_i915_gem_object *obj)
>  {
> -	return o->gtt_space.start;
> +	if (list_empty(&obj->vma_list))
> +		return NULL;
> +	return list_first_entry(&obj->vma_list, struct i915_vma, vma_link);
>  }
>  
>  /* Whether or not this object is currently mapped by the translation tables */
>  static inline bool
>  i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
>  {
> -	return drm_mm_node_allocated(&o->gtt_space);
> +	struct i915_vma *vma = __i915_gem_obj_to_vma(o);
> +	if (vma == NULL)
> +		return false;
> +	return drm_mm_node_allocated(&vma->node);
> +}
> +
> +/* Offset of the first PTE pointing to this object */
> +static inline unsigned long
> +i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
> +{
> +	BUG_ON(list_empty(&o->vma_list));
> +	return __i915_gem_obj_to_vma(o)->node.start;
>  }
>  
>  /* The size used in the translation tables may be larger than the actual size of
> @@ -1377,14 +1403,15 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
>  static inline unsigned long
>  i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
>  {
> -	return o->gtt_space.size;
> +	BUG_ON(list_empty(&o->vma_list));
> +	return __i915_gem_obj_to_vma(o)->node.size;
>  }
>  
>  static inline void
>  i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
>  			    enum i915_cache_level color)
>  {
> -	o->gtt_space.color = color;
> +	__i915_gem_obj_to_vma(o)->node.color = color;
>  }
>  
>  /**
> @@ -1691,6 +1718,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>  struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  						  size_t size);
>  void i915_gem_free_object(struct drm_gem_object *obj);
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> +				     struct i915_address_space *vm);
> +void i915_gem_vma_destroy(struct i915_vma *vma);
>  
>  int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,
>  				     uint32_t alignment,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 812275a..fe7ee32 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2580,6 +2580,7 @@ int
>  i915_gem_object_unbind(struct drm_i915_gem_object *obj)
>  {
>  	drm_i915_private_t *dev_priv = obj->base.dev->dev_private;
> +	struct i915_vma *vma;
>  	int ret;
>  
>  	if (!i915_gem_obj_ggtt_bound(obj))
> @@ -2617,11 +2618,20 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
>  	i915_gem_object_unpin_pages(obj);
>  
>  	list_del(&obj->mm_list);
> -	list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>  	/* Avoid an unnecessary call to unbind on rebind. */
>  	obj->map_and_fenceable = true;
>  
> -	drm_mm_remove_node(&obj->gtt_space);
> +	vma = __i915_gem_obj_to_vma(obj);
> +	list_del(&vma->vma_link);
> +	drm_mm_remove_node(&vma->node);
> +	i915_gem_vma_destroy(vma);
> +
> +	/* Since the unbound list is global, only move to that list if
> +	 * no more VMAs exist.
> +	 * NB: Until we have real VMAs there will only ever be one */
> +	WARN_ON(!list_empty(&obj->vma_list));
> +	if (list_empty(&obj->vma_list))
> +		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>  
>  	return 0;
>  }
> @@ -3051,8 +3061,12 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  	bool mappable, fenceable;
>  	size_t gtt_max = map_and_fenceable ?
>  		dev_priv->gtt.mappable_end : dev_priv->gtt.base.total;
> +	struct i915_vma *vma;
>  	int ret;
>  
> +	if (WARN_ON(!list_empty(&obj->vma_list)))
> +		return -EBUSY;
> +
>  	fence_size = i915_gem_get_gtt_size(dev,
>  					   obj->base.size,
>  					   obj->tiling_mode);
> @@ -3091,9 +3105,15 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  
>  	i915_gem_object_pin_pages(obj);
>  
> +	vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> +	if (vma == NULL) {
> +		i915_gem_object_unpin_pages(obj);
> +		return -ENOMEM;
> +	}
> +
>  search_free:
>  	ret = drm_mm_insert_node_in_range_generic(&dev_priv->gtt.base.mm,
> -						  &obj->gtt_space,
> +						  &vma->node,
>  						  size, alignment,
>  						  obj->cache_level, 0, gtt_max);
>  	if (ret) {
> @@ -3104,25 +3124,21 @@ search_free:
>  		if (ret == 0)
>  			goto search_free;
>  
> -		i915_gem_object_unpin_pages(obj);
> -		return ret;
> +		goto err_out;
>  	}
> -	if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->gtt_space,
> +	if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node,
>  					      obj->cache_level))) {
> -		i915_gem_object_unpin_pages(obj);
> -		drm_mm_remove_node(&obj->gtt_space);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto err_out;
>  	}
>  
>  	ret = i915_gem_gtt_prepare_object(obj);
> -	if (ret) {
> -		i915_gem_object_unpin_pages(obj);
> -		drm_mm_remove_node(&obj->gtt_space);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_out;
>  
>  	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
>  	list_add_tail(&obj->mm_list, &vm->inactive_list);
> +	list_add(&vma->vma_link, &obj->vma_list);
>  
>  	fenceable =
>  		i915_gem_obj_ggtt_size(obj) == fence_size &&
> @@ -3136,6 +3152,12 @@ search_free:
>  	trace_i915_gem_object_bind(obj, map_and_fenceable);
>  	i915_gem_verify_gtt(dev);
>  	return 0;
> +
> +err_out:
> +	i915_gem_vma_destroy(vma);
> +	i915_gem_object_unpin_pages(obj);
> +	drm_mm_remove_node(&vma->node);
> +	return ret;
>  }
>  
>  void
> @@ -3281,6 +3303,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
>  	int ret;
>  
>  	if (obj->cache_level == cache_level)
> @@ -3291,7 +3314,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		return -EBUSY;
>  	}
>  
> -	if (!i915_gem_valid_gtt_space(dev, &obj->gtt_space, cache_level)) {
> +	if (!i915_gem_valid_gtt_space(dev, &vma->node, cache_level)) {
>  		ret = i915_gem_object_unbind(obj);
>  		if (ret)
>  			return ret;
> @@ -3836,6 +3859,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>  	INIT_LIST_HEAD(&obj->global_list);
>  	INIT_LIST_HEAD(&obj->ring_list);
>  	INIT_LIST_HEAD(&obj->exec_list);
> +	INIT_LIST_HEAD(&obj->vma_list);
>  
>  	obj->ops = ops;
>  
> @@ -3956,6 +3980,26 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  	i915_gem_object_free(obj);
>  }
>  
> +struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
> +				     struct i915_address_space *vm)
> +{
> +	struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
> +	if (vma == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&vma->vma_link);
> +	vma->vm = vm;
> +	vma->obj = obj;
> +
> +	return vma;
> +}
> +
> +void i915_gem_vma_destroy(struct i915_vma *vma)
> +{
> +	WARN_ON(vma->node.allocated);
> +	kfree(vma);
> +}
> +
>  int
>  i915_gem_idle(struct drm_device *dev)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 43b8235..df61f33 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -34,11 +34,13 @@
>  static bool
>  mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
>  {
> +	struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
> +
>  	if (obj->pin_count)
>  		return false;
>  
>  	list_add(&obj->exec_list, unwind);
> -	return drm_mm_scan_add_block(&obj->gtt_space);
> +	return drm_mm_scan_add_block(&vma->node);
>  }
>  
>  int
> @@ -49,6 +51,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct i915_address_space *vm = &dev_priv->gtt.base;
>  	struct list_head eviction_list, unwind_list;
> +	struct i915_vma *vma;
>  	struct drm_i915_gem_object *obj;
>  	int ret = 0;
>  
> @@ -106,8 +109,8 @@ none:
>  		obj = list_first_entry(&unwind_list,
>  				       struct drm_i915_gem_object,
>  				       exec_list);
> -
> -		ret = drm_mm_scan_remove_block(&obj->gtt_space);
> +		vma = __i915_gem_obj_to_vma(obj);
> +		ret = drm_mm_scan_remove_block(&vma->node);
>  		BUG_ON(ret);
>  
>  		list_del_init(&obj->exec_list);
> @@ -127,7 +130,8 @@ found:
>  		obj = list_first_entry(&unwind_list,
>  				       struct drm_i915_gem_object,
>  				       exec_list);
> -		if (drm_mm_scan_remove_block(&obj->gtt_space)) {
> +		vma = __i915_gem_obj_to_vma(obj);
> +		if (drm_mm_scan_remove_block(&vma->node)) {
>  			list_move(&obj->exec_list, &eviction_list);
>  			drm_gem_object_reference(&obj->base);
>  			continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 999ecfe..3b639a9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -662,16 +662,17 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
>  
>  	/* Mark any preallocated objects as occupied */
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> +		struct i915_vma *vma = __i915_gem_obj_to_vma(obj);
>  		int ret;
>  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
>  			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
>  
>  		WARN_ON(i915_gem_obj_ggtt_bound(obj));
> -		ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> -					  &obj->gtt_space);
> +		ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
>  		if (ret)
>  			DRM_DEBUG_KMS("Reservation failed\n");
>  		obj->has_global_gtt_mapping = 1;
> +		list_add(&vma->vma_link, &obj->vma_list);
>  	}
>  
>  	dev_priv->gtt.base.start = start;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index a893834..f526136 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -354,6 +354,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	struct i915_address_space *vm = &dev_priv->gtt.base;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_mm_node *stolen;
> +	struct i915_vma *vma;
>  	int ret;
>  
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> @@ -393,18 +394,24 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	if (gtt_offset == I915_GTT_OFFSET_NONE)
>  		return obj;
>  
> +	vma = i915_gem_vma_create(obj, &dev_priv->gtt.base);
> +	if (!vma) {
> +		ret = -ENOMEM;
> +		goto err_out;
> +	}
> +
>  	/* To simplify the initialisation sequence between KMS and GTT,
>  	 * we allow construction of the stolen object prior to
>  	 * setting up the GTT space. The actual reservation will occur
>  	 * later.
>  	 */
> -	obj->gtt_space.start = gtt_offset;
> -	obj->gtt_space.size = size;
> +	vma->node.start = gtt_offset;
> +	vma->node.size = size;
>  	if (drm_mm_initialized(&dev_priv->gtt.base.mm)) {
> -		ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm,
> -					  &obj->gtt_space);
> +		ret = drm_mm_reserve_node(&dev_priv->gtt.base.mm, &vma->node);
>  		if (ret) {
>  			DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
> +			i915_gem_vma_destroy(vma);
>  			goto err_out;
>  		}
>  	}


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux