diff mbox series

[14/37] drm/i915: Serialise i915_vma_pin_inplace() with i915_vma_unbind()

Message ID 20200805122231.23313-15-chris@chris-wilson.co.uk
State New, archived
Headers show
Series Replace obj->mm.lock with reservation_ww_class | expand

Commit Message

Chris Wilson Aug. 5, 2020, 12:22 p.m. UTC
Directly seralise the atomic pinning with evicting the vma from unbind
with a pair of coupled cmpxchg to avoid fighting over vm->mutex.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_vma.c | 45 ++++++++++-----------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

Comments

Tvrtko Ursulin Aug. 5, 2020, 1:56 p.m. UTC | #1
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@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
diff mbox series

Patch

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));
 
 	/*
 	 * After confirming that no one else is pinning this vma, wait for