Re: [PATCH 61/73] drm/i915: Record allocated vma size

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2016-08-01 at 10:11 +0100, Chris Wilson wrote:
> Tracking the size of the VMA as allocated allows us to dramatically
> reduce the complexity of later functions (like inserting the VMA in to
> the drm_mm range manager).
> 

I assume this new revision only removed the extra hunk of code, right?

If so,

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c     | 103 ++++++++++++++----------------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  67 ++++++++---------------
>  drivers/gpu/drm/i915/i915_gem_gtt.h |   5 +-
>  3 files changed, 63 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f11bf6c4f86a..d89fbee5c48a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2976,53 +2976,36 @@ i915_gem_object_insert_into_vm(struct drm_i915_gem_object *obj,
>  			       u64 alignment,
>  			       u64 flags)
>  {
> -	struct drm_device *dev = obj->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	u64 start, end;
> -	u32 search_flag, alloc_flag;
> +	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>  	struct i915_vma *vma;
> +	u64 start, end;
> +	u64 min_alignment;
>  	int ret;
>  
> -	if (i915_is_ggtt(vm)) {
> -		u32 fence_size, fence_alignment, unfenced_alignment;
> -		u64 view_size;
> -
> -		if (WARN_ON(!ggtt_view))
> -			return ERR_PTR(-EINVAL);
> -
> -		view_size = i915_ggtt_view_size(obj, ggtt_view);
> -
> -		fence_size = i915_gem_get_ggtt_size(dev_priv,
> -						    view_size,
> -						    obj->tiling_mode);
> -		fence_alignment = i915_gem_get_ggtt_alignment(dev_priv,
> -							      view_size,
> -							      obj->tiling_mode,
> -							      true);
> -		unfenced_alignment = i915_gem_get_ggtt_alignment(dev_priv,
> -								 view_size,
> -								 obj->tiling_mode,
> -								 false);
> -		size = max(size, view_size);
> -		if (flags & PIN_MAPPABLE)
> -			size = max_t(u64, size, fence_size);
> -
> -		if (alignment == 0)
> -			alignment = flags & PIN_MAPPABLE ? fence_alignment :
> -				unfenced_alignment;
> -		if (flags & PIN_MAPPABLE && alignment & (fence_alignment - 1)) {
> -			DRM_DEBUG("Invalid object (view type=%u) alignment requested %llx\n",
> -				  ggtt_view ? ggtt_view->type : 0,
> -				  (long long)alignment);
> -			return ERR_PTR(-EINVAL);
> -		}
> -	} else {
> -		size = max_t(u64, size, obj->base.size);
> -		alignment = 4096;
> +	vma = ggtt_view ?
> +		i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view) :
> +		i915_gem_obj_lookup_or_create_vma(obj, vm);
> +	if (IS_ERR(vma))
> +		return vma;
> +
> +	size = max(size, vma->size);
> +	if (flags & PIN_MAPPABLE)
> +		size = i915_gem_get_ggtt_size(dev_priv, size, obj->tiling_mode);
> +
> +	min_alignment =
> +		i915_gem_get_ggtt_alignment(dev_priv, size, obj->tiling_mode,
> +					    flags & PIN_MAPPABLE);
> +	if (alignment == 0)
> +		alignment = min_alignment;
> +	if (alignment & (min_alignment - 1)) {
> +		DRM_DEBUG("Invalid object alignment requested %llu, minimum %llu\n",
> +			  alignment, min_alignment);
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
> -	end = vm->total;
> +
> +	end = vma->vm->total;
>  	if (flags & PIN_MAPPABLE)
>  		end = min_t(u64, end, dev_priv->ggtt.mappable_end);
>  	if (flags & PIN_ZONE_4G)
> @@ -3033,8 +3016,7 @@ i915_gem_object_insert_into_vm(struct drm_i915_gem_object *obj,
>  	 * attempt to find space.
>  	 */
>  	if (size > end) {
> -		DRM_DEBUG("Attempting to bind an object (view type=%u) larger than the aperture: request=%llu [object=%zd] > %s aperture=%llu\n",
> -			  ggtt_view ? ggtt_view->type : 0,
> +		DRM_DEBUG("Attempting to bind an object larger than the aperture: request=%llu [object=%zd] > %s aperture=%llu\n",
>  			  size, obj->base.size,
>  			  flags & PIN_MAPPABLE ? "mappable" : "total",
>  			  end);
> @@ -3047,31 +3029,27 @@ i915_gem_object_insert_into_vm(struct drm_i915_gem_object *obj,
>  
>  	i915_gem_object_pin_pages(obj);
>  
> -	vma = ggtt_view ? i915_gem_obj_lookup_or_create_ggtt_vma(obj, ggtt_view) :
> -			  i915_gem_obj_lookup_or_create_vma(obj, vm);
> -
> -	if (IS_ERR(vma))
> -		goto err_unpin;
> -
>  	if (flags & PIN_OFFSET_FIXED) {
>  		uint64_t offset = flags & PIN_OFFSET_MASK;
> -
> -		if (offset & (alignment - 1) || offset + size > end) {
> +		if (offset & (alignment - 1) || offset > end - size) {
>  			ret = -EINVAL;
> -			goto err_vma;
> +			goto err_unpin;
>  		}
> +
>  		vma->node.start = offset;
>  		vma->node.size = size;
>  		vma->node.color = obj->cache_level;
> -		ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> +		ret = drm_mm_reserve_node(&vma->vm->mm, &vma->node);
>  		if (ret) {
>  			ret = i915_gem_evict_for_vma(vma);
>  			if (ret == 0)
> -				ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> +				ret = drm_mm_reserve_node(&vma->vm->mm, &vma->node);
> +			if (ret)
> +				goto err_unpin;
>  		}
> -		if (ret)
> -			goto err_vma;
>  	} else {
> +		u32 search_flag, alloc_flag;
> +
>  		if (flags & PIN_HIGH) {
>  			search_flag = DRM_MM_SEARCH_BELOW;
>  			alloc_flag = DRM_MM_CREATE_TOP;
> @@ -3090,36 +3068,35 @@ i915_gem_object_insert_into_vm(struct drm_i915_gem_object *obj,
>  			alignment = 0;
>  
>  search_free:
> -		ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node,
> +		ret = drm_mm_insert_node_in_range_generic(&vma->vm->mm,
> +							  &vma->node,
>  							  size, alignment,
>  							  obj->cache_level,
>  							  start, end,
>  							  search_flag,
>  							  alloc_flag);
>  		if (ret) {
> -			ret = i915_gem_evict_something(vm, size, alignment,
> +			ret = i915_gem_evict_something(vma->vm, size, alignment,
>  						       obj->cache_level,
>  						       start, end,
>  						       flags);
>  			if (ret == 0)
>  				goto search_free;
>  
> -			goto err_vma;
> +			goto err_unpin;
>  		}
>  	}
>  	GEM_BUG_ON(!i915_gem_valid_gtt_space(vma, obj->cache_level));
>  
>  	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
> -	list_move_tail(&vma->vm_link, &vm->inactive_list);
> +	list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
>  	obj->bind_count++;
>  
>  	return vma;
>  
> -err_vma:
> -	vma = ERR_PTR(ret);
>  err_unpin:
>  	i915_gem_object_unpin_pages(obj);
> -	return vma;
> +	return ERR_PTR(ret);
>  }
>  
>  bool
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 31d13aea5461..ae6426e83ee0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -184,7 +184,7 @@ static void ppgtt_unbind_vma(struct i915_vma *vma)
>  {
>  	vma->vm->clear_range(vma->vm,
>  			     vma->node.start,
> -			     vma->obj->base.size,
> +			     vma->size,
>  			     true);
>  }
>  
> @@ -2697,28 +2697,18 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
>  
>  static void ggtt_unbind_vma(struct i915_vma *vma)
>  {
> -	struct drm_device *dev = vma->vm->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct drm_i915_gem_object *obj = vma->obj;
> -	const uint64_t size = min_t(uint64_t,
> -				    obj->base.size,
> -				    vma->node.size);
> +	struct i915_hw_ppgtt *appgtt = to_i915(vma->vm->dev)->mm.aliasing_ppgtt;
> +	const u64 size = min(vma->size, vma->node.size);
>  
> -	if (vma->bound & GLOBAL_BIND) {
> +	if (vma->bound & GLOBAL_BIND)
>  		vma->vm->clear_range(vma->vm,
> -				     vma->node.start,
> -				     size,
> +				     vma->node.start, size,
>  				     true);
> -	}
> -
> -	if (dev_priv->mm.aliasing_ppgtt && vma->bound & LOCAL_BIND) {
> -		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
>  
> +	if (vma->bound & LOCAL_BIND && appgtt)
>  		appgtt->base.clear_range(&appgtt->base,
> -					 vma->node.start,
> -					 size,
> +					 vma->node.start, size,
>  					 true);
> -	}
>  }
>  
>  void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
> @@ -3367,14 +3357,14 @@ void i915_vma_close(struct i915_vma *vma)
>  static struct i915_vma *
>  __i915_gem_vma_create(struct drm_i915_gem_object *obj,
>  		      struct i915_address_space *vm,
> -		      const struct i915_ggtt_view *ggtt_view)
> +		      const struct i915_ggtt_view *view)
>  {
>  	struct i915_vma *vma;
>  	int i;
>  
>  	GEM_BUG_ON(vm->closed);
>  
> -	if (WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> +	if (WARN_ON(i915_is_ggtt(vm) != !!view))
>  		return ERR_PTR(-EINVAL);
>  
>  	vma = kmem_cache_zalloc(to_i915(obj->base.dev)->vmas, GFP_KERNEL);
> @@ -3388,12 +3378,22 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj,
>  	list_add(&vma->vm_link, &vm->unbound_list);
>  	vma->vm = vm;
>  	vma->obj = obj;
> +	vma->size = obj->base.size;
>  	vma->is_ggtt = i915_is_ggtt(vm);
>  
> -	if (i915_is_ggtt(vm))
> -		vma->ggtt_view = *ggtt_view;
> -	else
> +	if (i915_is_ggtt(vm)) {
> +		vma->ggtt_view = *view;
> +		if (view->type == I915_GGTT_VIEW_PARTIAL) {
> +			vma->size = view->params.partial.size;
> +			vma->size <<= PAGE_SHIFT;
> +		} else if (view->type == I915_GGTT_VIEW_ROTATED) {
> +			vma->size =
> +				intel_rotation_info_size(&view->params.rotated);
> +			vma->size <<= PAGE_SHIFT;
> +		}
> +	} else {
>  		i915_ppgtt_get(i915_vm_to_ppgtt(vm));
> +	}
>  
>  	list_add_tail(&vma->obj_link, &obj->vma_list);
>  
> @@ -3678,29 +3678,6 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  	return 0;
>  }
>  
> -/**
> - * i915_ggtt_view_size - Get the size of a GGTT view.
> - * @obj: Object the view is of.
> - * @view: The view in question.
> - *
> - * @return The size of the GGTT view in bytes.
> - */
> -size_t
> -i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> -		    const struct i915_ggtt_view *view)
> -{
> -	if (view->type == I915_GGTT_VIEW_NORMAL) {
> -		return obj->base.size;
> -	} else if (view->type == I915_GGTT_VIEW_ROTATED) {
> -		return intel_rotation_info_size(&view->params.rotated) << PAGE_SHIFT;
> -	} else if (view->type == I915_GGTT_VIEW_PARTIAL) {
> -		return view->params.partial.size << PAGE_SHIFT;
> -	} else {
> -		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
> -		return obj->base.size;
> -	}
> -}
> -
>  void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>  {
>  	void __iomem *ptr;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 2ebf07defa4e..2de6f613ac1e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -180,6 +180,7 @@ struct i915_vma {
>  	struct drm_i915_gem_object *obj;
>  	struct i915_address_space *vm;
>  	void __iomem *iomap;
> +	u64 size;
>  
>  	unsigned int active;
>  	struct i915_gem_active last_read[I915_NUM_ENGINES];
> @@ -610,10 +611,6 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
>  	return true;
>  }
>  
> -size_t
> -i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> -		    const struct i915_ggtt_view *view);
> -
>  /**
>   * i915_vma_pin_iomap - calls ioremap_wc to map the GGTT VMA via the aperture
>   * @vma: VMA to iomap
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx