[Intel-gfx] [PATCH 14/37] drm/i915: Serialise i915_vma_pin_inplace() with i915_vma_unbind()

[Tvrtko Ursulin]

On 05/08/2020 13:22, Chris Wilson wrote:
> Directly seralise the atomic pinning with evicting the vma from unbind
> with a pair of coupled cmpxchg to avoid fighting over vm->mutex.

Assumption being bind/unbind should never contend and create a 
busy-spinny section? And motivation being.. ?

> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_vma.c | 45 ++++++++++-----------------------
>   1 file changed, 14 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index dbe11b349175..17ce0bce318e 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -742,12 +742,10 @@ i915_vma_detach(struct i915_vma *vma)
>   
>   bool i915_vma_pin_inplace(struct i915_vma *vma, unsigned int flags)
>   {
> -	unsigned int bound;
> -	bool pinned = true;
> +	unsigned int bound = atomic_read(&vma->flags);
>   
>   	GEM_BUG_ON(flags & ~I915_VMA_BIND_MASK);
>   
> -	bound = atomic_read(&vma->flags);
>   	do {
>   		if (unlikely(flags & ~bound))
>   			return false;
> @@ -755,34 +753,10 @@ bool i915_vma_pin_inplace(struct i915_vma *vma, unsigned int flags)
>   		if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR)))
>   			return false;
>   
> -		if (!(bound & I915_VMA_PIN_MASK))
> -			goto unpinned;
> -
>   		GEM_BUG_ON(((bound + 1) & I915_VMA_PIN_MASK) == 0);
>   	} while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1));
>   
>   	return true;
> -
> -unpinned:
> -	/*
> -	 * If pin_count==0, but we are bound, check under the lock to avoid
> -	 * racing with a concurrent i915_vma_unbind().
> -	 */
> -	mutex_lock(&vma->vm->mutex);
> -	do {
> -		if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR))) {
> -			pinned = false;
> -			break;
> -		}
> -
> -		if (unlikely(flags & ~bound)) {
> -			pinned = false;
> -			break;
> -		}
> -	} while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1));
> -	mutex_unlock(&vma->vm->mutex);
> -
> -	return pinned;
>   }
>   
>   static int vma_get_pages(struct i915_vma *vma)
> @@ -1292,6 +1266,7 @@ void __i915_vma_evict(struct i915_vma *vma)
>   
>   int __i915_vma_unbind(struct i915_vma *vma)
>   {
> +	unsigned int bound;
>   	int ret;
>   
>   	lockdep_assert_held(&vma->vm->mutex);
> @@ -1299,10 +1274,18 @@ int __i915_vma_unbind(struct i915_vma *vma)
>   	if (!drm_mm_node_allocated(&vma->node))
>   		return 0;
>   
> -	if (i915_vma_is_pinned(vma)) {
> -		vma_print_allocator(vma, "is pinned");
> -		return -EAGAIN;
> -	}
> +	/* Serialise with i915_vma_pin_inplace() */
> +	bound = atomic_read(&vma->flags);
> +	do {
> +		if (unlikely(bound & I915_VMA_PIN_MASK)) {
> +			vma_print_allocator(vma, "is pinned");
> +			return -EAGAIN;
> +		}
> +
> +		if (unlikely(bound & I915_VMA_ERROR))
> +			break;
> +	} while (!atomic_try_cmpxchg(&vma->flags,
> +				     &bound, bound | I915_VMA_ERROR));
Using the error flag is somehow critical for this scheme to work? Can 
you please explain in the comment and/or commit message?

>   
>   	/*
>   	 * After confirming that no one else is pinning this vma, wait for
> 

Regards,

Tvrtko