[Intel-gfx] [PATCH] drm/i915: Add soft-pinning API for execbuffer

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> Chris Wilson
> Sent: Friday, March 6, 2015 9:44 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH] drm/i915: Add soft-pinning API for execbuffer
> 
> Userspace can pass in an offset that it presumes the object is located
> at. The kernel will then do its utmost to fit the object into that
> location. The assumption is that userspace is handling its own object
> locations (for example along with full-ppgtt) and that the kernel will
> rarely have to make space for the user's requests.	
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h            |  5 +++
>  drivers/gpu/drm/i915/i915_gem.c            | 53 ++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/i915_gem_evict.c      | 52
> +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  9 ++++-
>  include/uapi/drm/i915_drm.h                |  3 +-
>  5 files changed, 106 insertions(+), 16 deletions(-)
> 

> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 9d0df4d85693..b266b31690e4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3592,22 +3592,43 @@ i915_gem_object_bind_to_vm(struct
> drm_i915_gem_object *obj,
>  	if (IS_ERR(vma))
>  		goto err_unpin;
> 
> +	if (flags & PIN_OFFSET_FIXED) {
> +		uint64_t offset = flags & PIN_OFFSET_MASK;
> +		if (offset & (alignment - 1)) {
> +			vma = ERR_PTR(-EINVAL);
> +			goto err_free_vma;
> +		}
> +		vma->node.start = offset;
> +		vma->node.size = size;
> +		vma->node.color = obj->cache_level;
> +		ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> +		if (ret) {
> +			ret = i915_gem_evict_range(dev, vm, start, end);
Did you mean i915_gem_evict_range(dev, vm, offset, offset+size) ?

> +			if (ret == 0)
> +				ret = drm_mm_reserve_node(&vm->mm,
> &vma->node);
> +		}
> +		if (ret) {
> +			vma = ERR_PTR(ret);
> +			goto err_free_vma;
> +		}
> +	} else {
>  search_free:
> -	ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node,
> -						  size, alignment,
> -						  obj->cache_level,
> -						  start, end,
> -
> DRM_MM_SEARCH_DEFAULT,
> -
> DRM_MM_CREATE_DEFAULT);
> -	if (ret) {
> -		ret = i915_gem_evict_something(dev, vm, size, alignment,
> -					       obj->cache_level,
> -					       start, end,
> -					       flags);
> -		if (ret == 0)
> -			goto search_free;
> +		ret = drm_mm_insert_node_in_range_generic(&vm->mm,
> &vma->node,
> +							  size, alignment,
> +							  obj->cache_level,
> +							  start, end,
> +
> DRM_MM_SEARCH_DEFAULT,
> +
> DRM_MM_CREATE_DEFAULT);
> +		if (ret) {
> +			ret = i915_gem_evict_something(dev, vm, size,
> alignment,
> +						       obj->cache_level,
> +						       start, end,
> +						       flags);
> +			if (ret == 0)
> +				goto search_free;
> 
> -		goto err_free_vma;
> +			goto err_free_vma;
> +		}
>  	}
>  	if (WARN_ON(!i915_gem_valid_gtt_space(vma, obj->cache_level))) {
>  		ret = -EINVAL;

> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c
> b/drivers/gpu/drm/i915/i915_gem_evict.c
> index e3a49d94da3a..c4b2ead0d805 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -195,6 +195,58 @@ found:
>  	return ret;
>  }
> 
> +int
> +i915_gem_evict_range(struct drm_device *dev, struct i915_address_space
> *vm,
> +		     unsigned long start, unsigned long end)
> +{
> +	struct drm_mm_node *node;
> +	struct list_head eviction_list;
> +	int ret = 0;
> +
> +	INIT_LIST_HEAD(&eviction_list);
> +	drm_mm_for_each_node(node, &vm->mm) {
> +		struct i915_vma *vma;
> +
> +		if (node->start + node->size <= start)
> +			continue;
> +		if (node->start >= end)
> +			break;
> +
> +		vma = container_of(node, typeof(*vma), node);
> +		if (vma->pin_count) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +
> +		if (WARN_ON(!list_empty(&vma->exec_list))) { 
So if an execbuffer uses both EXEC_OBJECT_PINNED and ordinary buffers in its exec_list then the ordinary buffers cannot be relocated if they are in the range of the pinned buffer.  Was this your intention?

> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		drm_gem_object_reference(&vma->obj->base);
> +		list_add(&vma->exec_list, &eviction_list);
I guess we need another list_head if we want to support both types of object in the same execbuffer call and allow relocation.

> +	}
> +
> +	while (!list_empty(&eviction_list)) {
> +		struct i915_vma *vma;
> +		struct drm_gem_object *obj;
> +
> +		vma = list_first_entry(&eviction_list,
> +				       struct i915_vma,
> +				       exec_list);
> +
> +		obj = &vma->obj->base;
> +
> +		list_del_init(&vma->exec_list);
> +		if (ret == 0)
> +			ret = i915_vma_unbind(vma);
> +
> +		drm_gem_object_unreference(obj);
> +	}
> +
> +	return ret;
> +}

Useful and worthwhile patch though.  Cheers.
Thomas.