diff mbox series

[v9,25/70] drm/i915: Take reservation lock around i915_vma_pin.

Message ID 20210323155059.628690-26-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Remove obj->mm.lock! | expand

Commit Message

Maarten Lankhorst March 23, 2021, 3:50 p.m. UTC
We previously complained when ww == NULL.

This function is now only used in selftests to pin an object,
and ww locking is now fixed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 .../i915/gem/selftests/i915_gem_coherency.c   | 12 ++++-------
 drivers/gpu/drm/i915/i915_gem.c               |  6 +++++-
 drivers/gpu/drm/i915/i915_vma.c               |  4 +---
 drivers/gpu/drm/i915/i915_vma.h               | 20 +++++++++++++++----
 4 files changed, 26 insertions(+), 16 deletions(-)

Comments

Daniel Vetter March 24, 2021, 12:35 p.m. UTC | #1
On Tue, Mar 23, 2021 at 04:50:14PM +0100, Maarten Lankhorst wrote:
> We previously complained when ww == NULL.
> 
> This function is now only used in selftests to pin an object,
> and ww locking is now fixed.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  .../i915/gem/selftests/i915_gem_coherency.c   | 12 ++++-------
>  drivers/gpu/drm/i915/i915_gem.c               |  6 +++++-
>  drivers/gpu/drm/i915/i915_vma.c               |  4 +---
>  drivers/gpu/drm/i915/i915_vma.h               | 20 +++++++++++++++----
>  4 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
> index b5dbf15570fc..3eec385d43bb 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
> @@ -218,15 +218,13 @@ static int gpu_set(struct context *ctx, unsigned long offset, u32 v)
>  	u32 *cs;
>  	int err;
>  
> +	vma = i915_gem_object_ggtt_pin(ctx->obj, NULL, 0, 0, 0);
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
> +
>  	i915_gem_object_lock(ctx->obj, NULL);
>  	i915_gem_object_set_to_gtt_domain(ctx->obj, false);

I have different context here because of

https://lore.kernel.org/intel-gfx/20210203090205.25818-8-chris@chris-wilson.co.uk/

What's really worrying here is the silent (accidental maybe, commit
message doesn't explain anything) change to the argument of
set_to_gtt_domain(). I've decided to just go with what we have right now,
but please double-check this matches the old version you've had before
this landed in drm-tip. Since I haven't pushed out the branch I've pinged
you with the pastebin on irc for now.
-Daniel

>  
> -	vma = i915_gem_object_ggtt_pin(ctx->obj, NULL, 0, 0, 0);
> -	if (IS_ERR(vma)) {
> -		err = PTR_ERR(vma);
> -		goto out_unlock;
> -	}
> -
>  	rq = intel_engine_create_kernel_request(ctx->engine);
>  	if (IS_ERR(rq)) {
>  		err = PTR_ERR(rq);
> @@ -265,9 +263,7 @@ static int gpu_set(struct context *ctx, unsigned long offset, u32 v)
>  	i915_request_add(rq);
>  out_unpin:
>  	i915_vma_unpin(vma);
> -out_unlock:
>  	i915_gem_object_unlock(ctx->obj);
> -
>  	return err;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3dee4e31fb14..8373662e4b5f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -920,7 +920,11 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>  			return ERR_PTR(ret);
>  	}
>  
> -	ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL);
> +	if (ww)
> +		ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL);
> +	else
> +		ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL);
> +
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 1ffda2aaa7a0..265e3a3079e2 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -863,9 +863,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>  	int err;
>  
>  #ifdef CONFIG_PROVE_LOCKING
> -	if (debug_locks && lockdep_is_held(&vma->vm->i915->drm.struct_mutex))
> -		WARN_ON(!ww);
> -	if (debug_locks && ww && vma->resv)
> +	if (debug_locks && !WARN_ON(!ww) && vma->resv)
>  		assert_vma_held(vma);
>  #endif
>  
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index 6b48f5c42488..8df784a026d2 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -246,10 +246,22 @@ i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>  static inline int __must_check
>  i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>  {
> -#ifdef CONFIG_LOCKDEP
> -	WARN_ON_ONCE(vma->resv && dma_resv_held(vma->resv));
> -#endif
> -	return i915_vma_pin_ww(vma, NULL, size, alignment, flags);
> +	struct i915_gem_ww_ctx ww;
> +	int err;
> +
> +	i915_gem_ww_ctx_init(&ww, true);
> +retry:
> +	err = i915_gem_object_lock(vma->obj, &ww);
> +	if (!err)
> +		err = i915_vma_pin_ww(vma, &ww, size, alignment, flags);
> +	if (err == -EDEADLK) {
> +		err = i915_gem_ww_ctx_backoff(&ww);
> +		if (!err)
> +			goto retry;
> +	}
> +	i915_gem_ww_ctx_fini(&ww);
> +
> +	return err;
>  }
>  
>  int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
> -- 
> 2.31.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
index b5dbf15570fc..3eec385d43bb 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
@@ -218,15 +218,13 @@  static int gpu_set(struct context *ctx, unsigned long offset, u32 v)
 	u32 *cs;
 	int err;
 
+	vma = i915_gem_object_ggtt_pin(ctx->obj, NULL, 0, 0, 0);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
 	i915_gem_object_lock(ctx->obj, NULL);
 	i915_gem_object_set_to_gtt_domain(ctx->obj, false);
 
-	vma = i915_gem_object_ggtt_pin(ctx->obj, NULL, 0, 0, 0);
-	if (IS_ERR(vma)) {
-		err = PTR_ERR(vma);
-		goto out_unlock;
-	}
-
 	rq = intel_engine_create_kernel_request(ctx->engine);
 	if (IS_ERR(rq)) {
 		err = PTR_ERR(rq);
@@ -265,9 +263,7 @@  static int gpu_set(struct context *ctx, unsigned long offset, u32 v)
 	i915_request_add(rq);
 out_unpin:
 	i915_vma_unpin(vma);
-out_unlock:
 	i915_gem_object_unlock(ctx->obj);
-
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3dee4e31fb14..8373662e4b5f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -920,7 +920,11 @@  i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
 			return ERR_PTR(ret);
 	}
 
-	ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL);
+	if (ww)
+		ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL);
+	else
+		ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL);
+
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 1ffda2aaa7a0..265e3a3079e2 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -863,9 +863,7 @@  int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
 	int err;
 
 #ifdef CONFIG_PROVE_LOCKING
-	if (debug_locks && lockdep_is_held(&vma->vm->i915->drm.struct_mutex))
-		WARN_ON(!ww);
-	if (debug_locks && ww && vma->resv)
+	if (debug_locks && !WARN_ON(!ww) && vma->resv)
 		assert_vma_held(vma);
 #endif
 
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 6b48f5c42488..8df784a026d2 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -246,10 +246,22 @@  i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
 static inline int __must_check
 i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 {
-#ifdef CONFIG_LOCKDEP
-	WARN_ON_ONCE(vma->resv && dma_resv_held(vma->resv));
-#endif
-	return i915_vma_pin_ww(vma, NULL, size, alignment, flags);
+	struct i915_gem_ww_ctx ww;
+	int err;
+
+	i915_gem_ww_ctx_init(&ww, true);
+retry:
+	err = i915_gem_object_lock(vma->obj, &ww);
+	if (!err)
+		err = i915_vma_pin_ww(vma, &ww, size, alignment, flags);
+	if (err == -EDEADLK) {
+		err = i915_gem_ww_ctx_backoff(&ww);
+		if (!err)
+			goto retry;
+	}
+	i915_gem_ww_ctx_fini(&ww);
+
+	return err;
 }
 
 int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,