[PATCH 0/3] drm/ttm: Reservation object individualization update

[PATCH 0/3] drm/ttm: Reservation object individualization update

[Thomas Hellström]

A couple of patches to modify the individualizing somewhat:

The motivation is that we run into trouble with the xe driver doing the
resv individualization in a clean way, having to grab the vm-wide sleeping
lock in a close- or release path which isn't really optimal. While
addressing that, some fly-by cleanups.

Patch 1: Makes sure the bulk move is removed at individualizing time, which is
convenient since we have all required locks.

Patch 2: Individualize even if fence copying fails.

Patch 3: Use a define instead of open-coded timeout


Thomas Hellström (3):
  drm/ttm: Clear the buffer object bulk move at individualize time
  drm/ttm: Clean up bo individualizing somewhat
  drm/ttm: Use a define for the resv wait timeout

 drivers/gpu/drm/ttm/ttm_bo.c | 73 +++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 31 deletions(-)

-- 
2.39.2

[PATCH 1/3] drm/ttm: Clear the buffer object bulk move at individualize time

[Thomas Hellström]

Clearing the buffer object bulk move is closely tied to individualizing
the resv, since that is when we effectively detach the bo from a vm.

Clearing the bulk move also requires the bo resv, which we have readily
locked at individualizing time without clobbering the much wider vm
lock.

So Clear the buffer object bulk_move at individualizing time, and update
the code comments.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index bd5dae4d1624..57cc9f845adc 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -193,20 +193,33 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
 	BUG_ON(!dma_resv_trylock(&bo->base._resv));
 
 	r = dma_resv_copy_fences(&bo->base._resv, bo->base.resv);
-	dma_resv_unlock(&bo->base._resv);
-	if (r)
-		return r;
 
-	if (bo->type != ttm_bo_type_sg) {
-		/* This works because the BO is about to be destroyed and nobody
-		 * reference it any more. The only tricky case is the trylock on
-		 * the resv object while holding the lru_lock.
+	if (!r && bo->type != ttm_bo_type_sg) {
+		/*
+		 * The TTM bo refcount is now zero and hence nobody will
+		 * therefore try to lock the bo at this point: the LRU
+		 * list lookups will trylock even if the refcount is zero,
+		 * but will only do that under the LRU lock and will
+		 * then immediately back off under the same LRU lock when it
+		 * sees the zero refcount.
 		 */
 		spin_lock(&bo->bdev->lru_lock);
 		bo->base.resv = &bo->base._resv;
+
+		/* Since bulk move is closely tied with the shared resv,
+		 * clear it when we have now individualized, if that was not
+		 * done by the driver already.
+		 */
+		if (bo->bulk_move) {
+			if (bo->resource)
+				ttm_resource_del_bulk_move(bo->resource, bo);
+			bo->bulk_move = NULL;
+		}
 		spin_unlock(&bo->bdev->lru_lock);
 	}
 
+	dma_resv_unlock(&bo->base._resv);
+
 	return r;
 }
 
@@ -324,7 +337,6 @@ static void ttm_bo_release(struct kref *kref)
 	int ret;
 
 	WARN_ON_ONCE(bo->pin_count);
-	WARN_ON_ONCE(bo->bulk_move);
 
 	if (!bo->deleted) {
 		ret = ttm_bo_individualize_resv(bo);
@@ -337,6 +349,8 @@ static void ttm_bo_release(struct kref *kref)
 					      30 * HZ);
 		}
 
+		WARN_ON_ONCE(bo->bulk_move);
+
 		if (bo->bdev->funcs->release_notify)
 			bo->bdev->funcs->release_notify(bo);
 
-- 
2.39.2

[PATCH 2/3] drm/ttm: Clean up bo individualizing somewhat

[Thomas Hellström]

Even if fence copying fails, individualize the resv after the wait.

If fence copying does fail, opportunistically trylock the vm's resv
to attempt to limit the chance of starvation.

Exit individdulizing earlier if the bo type is ttm_bo_type_sg.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 74 +++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 57cc9f845adc..84a512538e45 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -183,44 +183,48 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 	ttm_resource_free(bo, &bo->resource);
 }
 
-static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
+static void ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
 {
-	int r;
-
-	if (bo->base.resv == &bo->base._resv)
-		return 0;
+	if (bo->base.resv == &bo->base._resv || bo->type == ttm_bo_type_sg)
+		return;
 
 	BUG_ON(!dma_resv_trylock(&bo->base._resv));
 
-	r = dma_resv_copy_fences(&bo->base._resv, bo->base.resv);
+	if (dma_resv_copy_fences(&bo->base._resv, bo->base.resv)) {
+		/* Opportunistic trylock to block new fence additions */
+		bool locked = dma_resv_trylock(bo->base.resv);
 
-	if (!r && bo->type != ttm_bo_type_sg) {
-		/*
-		 * The TTM bo refcount is now zero and hence nobody will
-		 * therefore try to lock the bo at this point: the LRU
-		 * list lookups will trylock even if the refcount is zero,
-		 * but will only do that under the LRU lock and will
-		 * then immediately back off under the same LRU lock when it
-		 * sees the zero refcount.
-		 */
-		spin_lock(&bo->bdev->lru_lock);
-		bo->base.resv = &bo->base._resv;
+		/* Last resort if memory allocation failed for fence copying */
+		dma_resv_wait_timeout(bo->base.resv,
+				      DMA_RESV_USAGE_BOOKKEEP, false,
+				      30 * HZ);
+		if (locked)
+			dma_resv_unlock(bo->base.resv);
+	}
 
-		/* Since bulk move is closely tied with the shared resv,
-		 * clear it when we have now individualized, if that was not
-		 * done by the driver already.
-		 */
-		if (bo->bulk_move) {
-			if (bo->resource)
-				ttm_resource_del_bulk_move(bo->resource, bo);
-			bo->bulk_move = NULL;
-		}
-		spin_unlock(&bo->bdev->lru_lock);
+	/*
+	 * The TTM bo refcount is now zero and hence nobody will
+	 * therefore try to lock the bo at this point: the LRU
+	 * list lookups will trylock even if the refcount is zero,
+	 * but will only do that under the LRU lock and will
+	 * then immediately back off under the same LRU lock when it
+	 * sees the zero refcount.
+	 */
+	spin_lock(&bo->bdev->lru_lock);
+	bo->base.resv = &bo->base._resv;
+
+	/* Since bulk move is closely tied with the shared resv,
+	 * clear it when we have now individualized, if that was not
+	 * done by the driver already.
+	 */
+	if (bo->bulk_move) {
+		if (bo->resource)
+			ttm_resource_del_bulk_move(bo->resource, bo);
+		bo->bulk_move = NULL;
 	}
+	spin_unlock(&bo->bdev->lru_lock);
 
 	dma_resv_unlock(&bo->base._resv);
-
-	return r;
 }
 
 static void ttm_bo_flush_all_fences(struct ttm_buffer_object *bo)
@@ -334,21 +338,11 @@ static void ttm_bo_release(struct kref *kref)
 	struct ttm_buffer_object *bo =
 	    container_of(kref, struct ttm_buffer_object, kref);
 	struct ttm_device *bdev = bo->bdev;
-	int ret;
 
 	WARN_ON_ONCE(bo->pin_count);
 
 	if (!bo->deleted) {
-		ret = ttm_bo_individualize_resv(bo);
-		if (ret) {
-			/* Last resort, if we fail to allocate memory for the
-			 * fences block for the BO to become idle
-			 */
-			dma_resv_wait_timeout(bo->base.resv,
-					      DMA_RESV_USAGE_BOOKKEEP, false,
-					      30 * HZ);
-		}
-
+		ttm_bo_individualize_resv(bo);
 		WARN_ON_ONCE(bo->bulk_move);
 
 		if (bo->bdev->funcs->release_notify)
-- 
2.39.2

[PATCH 3/3] drm/ttm: Use a define for the resv wait timeout

[Thomas Hellström]

Rather than coding different delays here and there, use a define for the
resv timeout delay.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 84a512538e45..bacaed78ae55 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -31,6 +31,9 @@
 
 #define pr_fmt(fmt) "[TTM] " fmt
 
+/* The "Reasonable fence signal time" used by TTM. */
+#define TTM_RESV_TIMEOUT (30 * HZ)
+
 #include <drm/ttm/ttm_bo.h>
 #include <drm/ttm/ttm_placement.h>
 #include <drm/ttm/ttm_tt.h>
@@ -197,7 +200,7 @@ static void ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
 		/* Last resort if memory allocation failed for fence copying */
 		dma_resv_wait_timeout(bo->base.resv,
 				      DMA_RESV_USAGE_BOOKKEEP, false,
-				      30 * HZ);
+				      TTM_RESV_TIMEOUT);
 		if (locked)
 			dma_resv_unlock(bo->base.resv);
 	}
@@ -276,7 +279,7 @@ static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
 
 		lret = dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP,
 					     interruptible,
-					     30 * HZ);
+					     TTM_RESV_TIMEOUT);
 
 		if (lret < 0)
 			return lret;
@@ -1113,7 +1116,7 @@ int ttm_bo_wait_ctx(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx)
 	}
 
 	ret = dma_resv_wait_timeout(bo->base.resv, DMA_RESV_USAGE_BOOKKEEP,
-				    ctx->interruptible, 15 * HZ);
+				    ctx->interruptible, TTM_RESV_TIMEOUT);
 	if (unlikely(ret < 0))
 		return ret;
 	if (unlikely(ret == 0))
-- 
2.39.2

Re: [PATCH 1/3] drm/ttm: Clear the buffer object bulk move at individualize time

[Christian König]

Am 25.05.23 um 17:02 schrieb Thomas Hellström:
> Clearing the buffer object bulk move is closely tied to individualizing
> the resv, since that is when we effectively detach the bo from a vm.
>
> Clearing the bulk move also requires the bo resv, which we have readily
> locked at individualizing time without clobbering the much wider vm
> lock.
>
> So Clear the buffer object bulk_move at individualizing time, and update
> the code comments.

WOW, there are some big misunderstandings here. First of all the 
assumption that the reservation lock is taken at individualization time 
is completely incorrect.

Instead this is called during release with an unknown state of the 
reservation lock, this can be locked or not depending who is dropping 
the last reference.

Then when you use bulk move, the area covered by the bulk is protected 
by the common reservation lock. So when a BO is to be removed from the 
bulk it's a must have to remove it from the bulk *before* dropping the 
last reference.

Regards,
Christian.

>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 30 ++++++++++++++++++++++--------
>   1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index bd5dae4d1624..57cc9f845adc 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -193,20 +193,33 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
>   	BUG_ON(!dma_resv_trylock(&bo->base._resv));
>   
>   	r = dma_resv_copy_fences(&bo->base._resv, bo->base.resv);
> -	dma_resv_unlock(&bo->base._resv);
> -	if (r)
> -		return r;
>   
> -	if (bo->type != ttm_bo_type_sg) {
> -		/* This works because the BO is about to be destroyed and nobody
> -		 * reference it any more. The only tricky case is the trylock on
> -		 * the resv object while holding the lru_lock.
> +	if (!r && bo->type != ttm_bo_type_sg) {
> +		/*
> +		 * The TTM bo refcount is now zero and hence nobody will
> +		 * therefore try to lock the bo at this point: the LRU
> +		 * list lookups will trylock even if the refcount is zero,
> +		 * but will only do that under the LRU lock and will
> +		 * then immediately back off under the same LRU lock when it
> +		 * sees the zero refcount.
>   		 */
>   		spin_lock(&bo->bdev->lru_lock);
>   		bo->base.resv = &bo->base._resv;
> +
> +		/* Since bulk move is closely tied with the shared resv,
> +		 * clear it when we have now individualized, if that was not
> +		 * done by the driver already.
> +		 */
> +		if (bo->bulk_move) {
> +			if (bo->resource)
> +				ttm_resource_del_bulk_move(bo->resource, bo);
> +			bo->bulk_move = NULL;
> +		}
>   		spin_unlock(&bo->bdev->lru_lock);
>   	}
>   
> +	dma_resv_unlock(&bo->base._resv);
> +
>   	return r;
>   }
>   
> @@ -324,7 +337,6 @@ static void ttm_bo_release(struct kref *kref)
>   	int ret;
>   
>   	WARN_ON_ONCE(bo->pin_count);
> -	WARN_ON_ONCE(bo->bulk_move);
>   
>   	if (!bo->deleted) {
>   		ret = ttm_bo_individualize_resv(bo);
> @@ -337,6 +349,8 @@ static void ttm_bo_release(struct kref *kref)
>   					      30 * HZ);
>   		}
>   
> +		WARN_ON_ONCE(bo->bulk_move);
> +
>   		if (bo->bdev->funcs->release_notify)
>   			bo->bdev->funcs->release_notify(bo);
>