[Intel-gfx] [PATCH] drm/i915: Double check vma placement upon gaining the vm lock

[Tvrtko Ursulin]

On 26/11/2019 15:26, Chris Wilson wrote:
> The current unbind + retry of i915_gem_object_ggtt_pin() allows for
> someone else to sneak and claim the vma under a different placement
> before the first GGTT bind is complete. Leading to confusion and
> complaints all over.
> 
> Ideally we would pull the evict + rebind under the same lock, but for
> now, simply try again.
> 
> Fixes: 2850748ef876 ("drm/i915: Pull i915_vma_pin under the vm->mutex")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 39 +++++++++++++++++++--------------
>   drivers/gpu/drm/i915/i915_vma.c |  6 +++++
>   2 files changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61395b03443e..b0878d35ed1d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -938,33 +938,38 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>   	if (IS_ERR(vma))
>   		return vma;
>   
> -	if (i915_vma_misplaced(vma, size, alignment, flags)) {
> -		if (flags & PIN_NONBLOCK) {
> -			if (i915_vma_is_pinned(vma) || i915_vma_is_active(vma))
> -				return ERR_PTR(-ENOSPC);
> -
> -			if (flags & PIN_MAPPABLE &&
> -			    vma->fence_size > ggtt->mappable_end / 2)
> -				return ERR_PTR(-ENOSPC);
> +	do {
> +		if (i915_vma_misplaced(vma, size, alignment, flags)) {
> +			if (flags & PIN_NONBLOCK) {
> +				if (i915_vma_is_pinned(vma) ||
> +				    i915_vma_is_active(vma))
> +					return ERR_PTR(-ENOSPC);
> +
> +				if (flags & PIN_MAPPABLE &&
> +				    vma->fence_size > ggtt->mappable_end / 2)
> +					return ERR_PTR(-ENOSPC);
> +			}
> +
> +			ret = i915_vma_unbind(vma);
> +			if (ret)
> +				return ERR_PTR(ret);
>   		}
>   
> -		ret = i915_vma_unbind(vma);
> -		if (ret)
> -			return ERR_PTR(ret);
> -	}
> +		ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL);
> +	} while (ret == -EAGAIN); /* retry if we lost our placement */
> +	if (ret)
> +		return ERR_PTR(ret);
>   
>   	if (vma->fence && !i915_gem_object_is_tiled(obj)) {
>   		mutex_lock(&ggtt->vm.mutex);
>   		ret = i915_vma_revoke_fence(vma);
>   		mutex_unlock(&ggtt->vm.mutex);
> -		if (ret)
> +		if (ret) {
> +			i915_vma_unpin(vma);
>   			return ERR_PTR(ret);
> +		}
>   	}
>   
> -	ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
>   	return vma;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index e5512f26e20a..f6e661428b02 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -905,6 +905,12 @@ int i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>   			__i915_vma_set_map_and_fenceable(vma);
>   	}
>   
> +	/* Somebody else managed to gazump our placement? */
> +	if (i915_vma_misplaced(vma, size, alignment, flags)) {
> +		err = -EAGAIN;
> +		goto err_active;
> +	}
> +

What about other callers? They will not be expecting this.

>   	GEM_BUG_ON(!vma->pages);
>   	err = i915_vma_bind(vma,
>   			    vma->obj ? vma->obj->cache_level : 0,
> 

Regards,

Tvrtko