[PATCH 1/7] drm/ttm: move unlocking out of ttm_bo_cleanup_memtype_use

[PATCH 1/7] drm/ttm: move unlocking out of ttm_bo_cleanup_memtype_use

[Christian König]

Needed for the next patch and makes the code quite a bit easier to
understand.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index c088703777e2..9905cf41cba6 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -390,8 +390,6 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo)
 	ttm_tt_destroy(bo->ttm);
 	bo->ttm = NULL;
 	ttm_bo_mem_put(bo, &bo->mem);
-
-	ww_mutex_unlock (&bo->resv->lock);
 }
 
 static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
@@ -457,6 +455,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 				reservation_object_unlock(&bo->ttm_resv);
 
 			ttm_bo_cleanup_memtype_use(bo);
+			reservation_object_unlock(bo->resv);
 			return;
 		}
 
@@ -559,6 +558,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 
 	spin_unlock(&glob->lru_lock);
 	ttm_bo_cleanup_memtype_use(bo);
+	reservation_object_unlock(bo->resv);
 
 	return 0;
 }
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/7] drm/ttm: consistently use reservation_object_unlock

[Christian König]

Instead of having a confusing wrapper or call the underlying ww_mutex
function directly.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/qxl/qxl_release.c      |  2 +-
 drivers/gpu/drm/ttm/ttm_bo.c           | 13 +++++++------
 drivers/gpu/drm/ttm/ttm_execbuf_util.c |  8 ++++----
 include/drm/ttm/ttm_bo_driver.h        | 14 +-------------
 4 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index e6ec845b5be0..a5acd723be41 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -469,7 +469,7 @@ void qxl_release_fence_buffer_objects(struct qxl_release *release)
 
 		reservation_object_add_shared_fence(bo->resv, &release->base);
 		ttm_bo_add_to_lru(bo);
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 	spin_unlock(&glob->lru_lock);
 	ww_acquire_fini(&release->ticket);
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 9905cf41cba6..6f55310a9d09 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -471,7 +471,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 			ttm_bo_add_to_lru(bo);
 		}
 
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 	if (bo->resv != &bo->ttm_resv)
 		reservation_object_unlock(&bo->ttm_resv);
@@ -517,7 +517,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 
 	if (ret && !no_wait_gpu) {
 		long lret;
-		ww_mutex_unlock(&bo->resv->lock);
+
+		reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 
 		lret = reservation_object_wait_timeout_rcu(resv, true,
@@ -547,7 +548,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 	}
 
 	if (ret || unlikely(list_empty(&bo->ddestroy))) {
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 		return ret;
 	}
@@ -749,7 +750,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 
 			if (place && !bdev->driver->eviction_valuable(bo,
 								      place)) {
-				__ttm_bo_unreserve(bo);
+				reservation_object_unlock(bo->resv);
 				ret = -EBUSY;
 				continue;
 			}
@@ -1788,7 +1789,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	 * already swapped buffer.
 	 */
 
-	__ttm_bo_unreserve(bo);
+	reservation_object_unlock(bo->resv);
 	kref_put(&bo->list_kref, ttm_bo_release_list);
 	return ret;
 }
@@ -1825,7 +1826,7 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo)
 	ret = __ttm_bo_reserve(bo, true, false, NULL);
 	if (unlikely(ret != 0))
 		goto out_unlock;
-	__ttm_bo_unreserve(bo);
+	reservation_object_unlock(bo->resv);
 
 out_unlock:
 	mutex_unlock(&bo->wu_mutex);
diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
index 5e1bcabffef5..373ced0b2fc2 100644
--- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c
+++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
@@ -38,7 +38,7 @@ static void ttm_eu_backoff_reservation_reverse(struct list_head *list,
 	list_for_each_entry_continue_reverse(entry, list, head) {
 		struct ttm_buffer_object *bo = entry->bo;
 
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 }
 
@@ -69,7 +69,7 @@ void ttm_eu_backoff_reservation(struct ww_acquire_ctx *ticket,
 		struct ttm_buffer_object *bo = entry->bo;
 
 		ttm_bo_add_to_lru(bo);
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 	spin_unlock(&glob->lru_lock);
 
@@ -112,7 +112,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket,
 
 		ret = __ttm_bo_reserve(bo, intr, (ticket == NULL), ticket);
 		if (!ret && unlikely(atomic_read(&bo->cpu_writers) > 0)) {
-			__ttm_bo_unreserve(bo);
+			reservation_object_unlock(bo->resv);
 
 			ret = -EBUSY;
 
@@ -203,7 +203,7 @@ void ttm_eu_fence_buffer_objects(struct ww_acquire_ctx *ticket,
 		else
 			reservation_object_add_excl_fence(bo->resv, fence);
 		ttm_bo_add_to_lru(bo);
-		__ttm_bo_unreserve(bo);
+		reservation_object_unlock(bo->resv);
 	}
 	spin_unlock(&glob->lru_lock);
 	if (ticket)
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 5f821a9b3a1f..389359a0006b 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -941,18 +941,6 @@ static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo,
 }
 
 /**
- * __ttm_bo_unreserve
- * @bo: A pointer to a struct ttm_buffer_object.
- *
- * Unreserve a previous reservation of @bo where the buffer object is
- * already on lru lists.
- */
-static inline void __ttm_bo_unreserve(struct ttm_buffer_object *bo)
-{
-	ww_mutex_unlock(&bo->resv->lock);
-}
-
-/**
  * ttm_bo_unreserve
  *
  * @bo: A pointer to a struct ttm_buffer_object.
@@ -966,7 +954,7 @@ static inline void ttm_bo_unreserve(struct ttm_buffer_object *bo)
 		ttm_bo_add_to_lru(bo);
 		spin_unlock(&bo->glob->lru_lock);
 	}
-	__ttm_bo_unreserve(bo);
+	reservation_object_unlock(bo->resv);
 }
 
 /**
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/7] dma-buf: add reservation_object_lock_interruptible()

[Christian König]

That's the only wrapper function missing and necessary to cleanup TTM.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 include/linux/reservation.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index 21fc84d82d41..02166e815afb 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -167,6 +167,29 @@ reservation_object_lock(struct reservation_object *obj,
 }
 
 /**
+ * reservation_object_lock_interruptible - lock the reservation object
+ * @obj: the reservation object
+ * @ctx: the locking context
+ *
+ * Locks the reservation object interruptible for exclusive access and
+ * modification. Note, that the lock is only against other writers, readers
+ * will run concurrently with a writer under RCU. The seqlock is used to
+ * notify readers if they overlap with a writer.
+ *
+ * As the reservation object may be locked by multiple parties in an
+ * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
+ * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation
+ * object may be locked by itself by passing NULL as @ctx.
+ */
+static inline int
+reservation_object_lock_interruptible(struct reservation_object *obj,
+				      struct ww_acquire_ctx *ctx)
+{
+	return ww_mutex_lock_interruptible(&obj->lock, ctx);
+}
+
+
+/**
  * reservation_object_trylock - trylock the reservation object
  * @obj: the reservation object
  *
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm/ttm: user reservation object wrappers

[Christian König]

Consistently use the reservation object wrappers instead of accessing
the ww_mutex directly.

Additional to that use the reservation object wrappers directly instead of
calling __ttm_bo_reserve with fixed parameters.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c    | 16 +++++++++-------
 include/drm/ttm/ttm_bo_driver.h |  6 +++---
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 6f55310a9d09..6f5d18366e6e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -446,7 +446,7 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 	}
 
 	spin_lock(&glob->lru_lock);
-	ret = __ttm_bo_reserve(bo, false, true, NULL);
+	ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
 	if (!ret) {
 		if (reservation_object_test_signaled_rcu(&bo->ttm_resv, true)) {
 			ttm_bo_del_from_lru(bo);
@@ -531,7 +531,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 			return -EBUSY;
 
 		spin_lock(&glob->lru_lock);
-		ret = __ttm_bo_reserve(bo, false, true, NULL);
+		ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
 
 		/*
 		 * We raced, and lost, someone else holds the reservation now,
@@ -592,10 +592,10 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 			kref_get(&nentry->list_kref);
 		}
 
-		ret = __ttm_bo_reserve(entry, false, true, NULL);
+		ret = reservation_object_trylock(entry->resv) ? 0 : -EBUSY;
 		if (remove_all && ret) {
 			spin_unlock(&glob->lru_lock);
-			ret = __ttm_bo_reserve(entry, false, false, NULL);
+			ret = reservation_object_lock(entry->resv, NULL);
 			spin_lock(&glob->lru_lock);
 		}
 
@@ -744,7 +744,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		list_for_each_entry(bo, &man->lru[i], lru) {
-			ret = __ttm_bo_reserve(bo, false, true, NULL);
+			ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
 			if (ret)
 				continue;
 
@@ -1719,7 +1719,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		list_for_each_entry(bo, &glob->swap_lru[i], swap) {
-			ret = __ttm_bo_reserve(bo, false, true, NULL);
+			ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
 			if (!ret)
 				break;
 		}
@@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo)
 		return -ERESTARTSYS;
 	if (!ww_mutex_is_locked(&bo->resv->lock))
 		goto out_unlock;
-	ret = __ttm_bo_reserve(bo, true, false, NULL);
+	ret = reservation_object_lock_interruptible(bo->resv, NULL);
+	if (ret = -EINTR)
+		ret = -ERESTARTSYS;
 	if (unlikely(ret != 0))
 		goto out_unlock;
 	reservation_object_unlock(bo->resv);
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 389359a0006b..3659cf6150d2 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -836,14 +836,14 @@ static inline int __ttm_bo_reserve(struct ttm_buffer_object *bo,
 		if (WARN_ON(ticket))
 			return -EBUSY;
 
-		success = ww_mutex_trylock(&bo->resv->lock);
+		success = reservation_object_trylock(bo->resv);
 		return success ? 0 : -EBUSY;
 	}
 
 	if (interruptible)
-		ret = ww_mutex_lock_interruptible(&bo->resv->lock, ticket);
+		ret = reservation_object_lock_interruptible(bo->resv, ticket);
 	else
-		ret = ww_mutex_lock(&bo->resv->lock, ticket);
+		ret = reservation_object_lock(bo->resv, ticket);
 	if (ret == -EINTR)
 		return -ERESTARTSYS;
 	return ret;
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/7] drm/ttm: remove ttm_bo_unreserve_ticket

[Christian König]

Just another alias for ttm_bo_unreserve.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/nouveau/nouveau_gem.c |  2 +-
 include/drm/ttm/ttm_bo_driver.h       | 13 -------------
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 2170534101ca..dedd58fee67b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -349,7 +349,7 @@ validate_fini_no_ticket(struct validate_op *op, struct nouveau_fence *fence,
 
 		list_del(&nvbo->entry);
 		nvbo->reserved_by = NULL;
-		ttm_bo_unreserve_ticket(&nvbo->bo, &op->ticket);
+		ttm_bo_unreserve(&nvbo->bo);
 		drm_gem_object_unreference_unlocked(&nvbo->gem);
 	}
 }
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 3659cf6150d2..cba1477aa983 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -957,19 +957,6 @@ static inline void ttm_bo_unreserve(struct ttm_buffer_object *bo)
 	reservation_object_unlock(bo->resv);
 }
 
-/**
- * ttm_bo_unreserve_ticket
- * @bo: A pointer to a struct ttm_buffer_object.
- * @ticket: ww_acquire_ctx used for reserving
- *
- * Unreserve a previous reservation of @bo made with @ticket.
- */
-static inline void ttm_bo_unreserve_ticket(struct ttm_buffer_object *bo,
-					   struct ww_acquire_ctx *t)
-{
-	ttm_bo_unreserve(bo);
-}
-
 /*
  * ttm_bo_util.c
  */
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[Christian König]

Needed for the next patch.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 6f5d18366e6e..50a678b504f3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
 }
 
 /**
- * function ttm_bo_cleanup_refs_and_unlock
+ * function ttm_bo_cleanup_refs
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
  * Must be called with lru_lock and reservation held, this function
- * will drop both before returning.
+ * will drop the lru lock and optionally the reservation lock before returning.
  *
  * @interruptible         Any sleeps should occur interruptibly.
  * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
+ * @unlock_resv           Unlock the reservation lock as well.
  */
 
-static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
-					  bool interruptible,
-					  bool no_wait_gpu)
+static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
+			       bool interruptible, bool no_wait_gpu,
+			       bool unlock_resv)
 {
 	struct ttm_bo_global *glob = bo->glob;
 	struct reservation_object *resv;
@@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 	if (ret && !no_wait_gpu) {
 		long lret;
 
-		reservation_object_unlock(bo->resv);
+		if (unlock_resv)
+			reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 
 		lret = reservation_object_wait_timeout_rcu(resv, true,
@@ -531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 			return -EBUSY;
 
 		spin_lock(&glob->lru_lock);
-		ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
-
-		/*
-		 * We raced, and lost, someone else holds the reservation now,
-		 * and is probably busy in ttm_bo_cleanup_memtype_use.
-		 *
-		 * Even if it's not the case, because we finished waiting any
-		 * delayed destruction would succeed, so just return success
-		 * here.
-		 */
-		if (ret) {
-			spin_unlock(&glob->lru_lock);
-			return 0;
+		if (unlock_resv) {
+			ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
+			/*
+			 * We raced, and lost, someone else holds the reservation now,
+			 * and is probably busy in ttm_bo_cleanup_memtype_use.
+			 *
+			 * Even if it's not the case, because we finished waiting any
+			 * delayed destruction would succeed, so just return success
+			 * here.
+			 */
+			if (ret) {
+				spin_unlock(&glob->lru_lock);
+				return 0;
+			}
+		} else {
+			ret = 0;
 		}
 	}
 
@@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 		}
 
 		if (!ret)
-			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
-							     !remove_all);
+			ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
+						  true);
 		else
 			spin_unlock(&glob->lru_lock);
 
@@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
-						     no_wait_gpu);
+		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
@@ -1735,7 +1739,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+		ret = ttm_bo_cleanup_refs(bo, false, false, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
-- 
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/7] drm/ttm: optimize ttm_mem_evict_first v2

[Christian König]

Deleted BOs with the same reservation object can be reaped even if they
can't be reserved.

v2: rebase and we still need to remove/add the BO from/to the LRU.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 50a678b504f3..6545c4344684 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -735,20 +735,37 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
-				uint32_t mem_type,
-				const struct ttm_place *place,
-				bool interruptible,
-				bool no_wait_gpu)
+			       struct reservation_object *resv,
+			       uint32_t mem_type,
+			       const struct ttm_place *place,
+			       bool interruptible,
+			       bool no_wait_gpu)
 {
 	struct ttm_bo_global *glob = bdev->glob;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
 	struct ttm_buffer_object *bo;
 	int ret = -EBUSY;
+	bool locked;
 	unsigned i;
 
 	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		list_for_each_entry(bo, &man->lru[i], lru) {
+			if (bo->resv == resv) {
+				if (list_empty(&bo->ddestroy))
+					continue;
+
+				if (place &&
+				    !bdev->driver->eviction_valuable(bo, place))
+					continue;
+
+				ttm_bo_del_from_lru(bo);
+
+				ret = 0;
+				locked = false;
+				break;
+			}
+
 			ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
 			if (ret)
 				continue;
@@ -760,6 +777,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 				continue;
 			}
 
+			locked = true;
 			break;
 		}
 
@@ -775,7 +793,8 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
+		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu,
+					  locked);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
@@ -786,7 +805,10 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	BUG_ON(ret != 0);
 
 	ret = ttm_bo_evict(bo, interruptible, no_wait_gpu);
-	ttm_bo_unreserve(bo);
+	if (locked)
+		ttm_bo_unreserve(bo);
+	else
+		ttm_bo_add_to_lru(bo);
 
 	kref_put(&bo->list_kref, ttm_bo_release_list);
 	return ret;
@@ -850,7 +872,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 			return ret;
 		if (mem->mm_node)
 			break;
-		ret = ttm_mem_evict_first(bdev, mem_type, place,
+		ret = ttm_mem_evict_first(bdev, bo->resv, mem_type, place,
 					  interruptible, no_wait_gpu);
 		if (unlikely(ret != 0))
 			return ret;
@@ -1353,7 +1375,8 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		while (!list_empty(&man->lru[i])) {
 			spin_unlock(&glob->lru_lock);
-			ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
+			ret = ttm_mem_evict_first(bdev, NULL, mem_type, NULL,
+						  false, false);
 			if (ret)
 				return ret;
 			spin_lock(&glob->lru_lock);
-- 
2.11.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/ttm: user reservation object wrappers

[Michel Dänzer]

On 09/11/17 09:59 AM, Christian König wrote:
> Consistently use the reservation object wrappers instead of accessing
> the ww_mutex directly.
> 
> Additional to that use the reservation object wrappers directly instead of
> calling __ttm_bo_reserve with fixed parameters.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

[...]

> @@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo)
>  		return -ERESTARTSYS;
>  	if (!ww_mutex_is_locked(&bo->resv->lock))
>  		goto out_unlock;
> -	ret = __ttm_bo_reserve(bo, true, false, NULL);
> +	ret = reservation_object_lock_interruptible(bo->resv, NULL);
> +	if (ret = -EINTR)
> +		ret = -ERESTARTSYS;

Typo in the test, must be

    if (ret == -EINTR)


This bug caused the Xorg process to hang for me when trying to run
glxgears, requiring a hard reboot. Did you accidentally send an untested
version of this patch?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/7] drm/ttm: user reservation object wrappers

[Christian König]

Am 09.11.2017 um 17:50 schrieb Michel Dänzer:
> On 09/11/17 09:59 AM, Christian König wrote:
>> Consistently use the reservation object wrappers instead of accessing
>> the ww_mutex directly.
>>
>> Additional to that use the reservation object wrappers directly instead of
>> calling __ttm_bo_reserve with fixed parameters.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> [...]
>
>> @@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo)
>>   		return -ERESTARTSYS;
>>   	if (!ww_mutex_is_locked(&bo->resv->lock))
>>   		goto out_unlock;
>> -	ret = __ttm_bo_reserve(bo, true, false, NULL);
>> +	ret = reservation_object_lock_interruptible(bo->resv, NULL);
>> +	if (ret = -EINTR)
>> +		ret = -ERESTARTSYS;
> Typo in the test, must be
>
>      if (ret == -EINTR)
>
>
> This bug caused the Xorg process to hang for me when trying to run
> glxgears, requiring a hard reboot. Did you accidentally send an untested
> version of this patch?
Yeah, just stumbled over this as well. I accidentally merged the fix for 
this into a later patch which I didn't send out yet.

Consider it fixed,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/7] drm/ttm: optimize ttm_mem_evict_first v2

[Michel Dänzer]

On 09/11/17 09:59 AM, Christian König wrote:
> Deleted BOs with the same reservation object can be reaped even if they
> can't be reserved.
> 
> v2: rebase and we still need to remove/add the BO from/to the LRU.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 39 +++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 50a678b504f3..6545c4344684 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -735,20 +735,37 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>  
>  static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> -				uint32_t mem_type,
> -				const struct ttm_place *place,
> -				bool interruptible,
> -				bool no_wait_gpu)
> +			       struct reservation_object *resv,
> +			       uint32_t mem_type,
> +			       const struct ttm_place *place,
> +			       bool interruptible,
> +			       bool no_wait_gpu)
>  {
>  	struct ttm_bo_global *glob = bdev->glob;
>  	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>  	struct ttm_buffer_object *bo;
>  	int ret = -EBUSY;
> +	bool locked;
>  	unsigned i;
>  
>  	spin_lock(&glob->lru_lock);
>  	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>  		list_for_each_entry(bo, &man->lru[i], lru) {
> +			if (bo->resv == resv) {
> +				if (list_empty(&bo->ddestroy))
> +					continue;
> +
> +				if (place &&
> +				    !bdev->driver->eviction_valuable(bo, place))
> +					continue;
> +
> +				ttm_bo_del_from_lru(bo);

Is this necessary, despite the existing ttm_bo_del_from_lru call before
unlocking the LRU lock? If yes, why isn't this necessary in the bo->resv
!= resv case?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 7/7] drm/ttm: optimize ttm_mem_evict_first v2

[Chunming Zhou]

On 2017年11月09日 16:59, Christian König wrote:
> Deleted BOs with the same reservation object can be reaped even if they
> can't be reserved.
>
> v2: rebase and we still need to remove/add the BO from/to the LRU.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 39 +++++++++++++++++++++++++++++++--------
>   1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 50a678b504f3..6545c4344684 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -735,20 +735,37 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> -				uint32_t mem_type,
> -				const struct ttm_place *place,
> -				bool interruptible,
> -				bool no_wait_gpu)
> +			       struct reservation_object *resv,
> +			       uint32_t mem_type,
> +			       const struct ttm_place *place,
> +			       bool interruptible,
> +			       bool no_wait_gpu)
>   {
>   	struct ttm_bo_global *glob = bdev->glob;
>   	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>   	struct ttm_buffer_object *bo;
>   	int ret = -EBUSY;
> +	bool locked;
>   	unsigned i;
>   
>   	spin_lock(&glob->lru_lock);
>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   		list_for_each_entry(bo, &man->lru[i], lru) {
> +			if (bo->resv == resv) {
> +				if (list_empty(&bo->ddestroy))
> +					continue;
I don't think only destroying BO can be evicted under per-vm-bo case, 
but also normal BO should as well.
I'd give an example:
1. vm-A allocates all vram;
2. vm-B also try to allocate full vram, so the BOs of vram in vm-A will 
be evicted to GTT.
3. vm-A is trying to allocate all GTT, if we don't allow eviction or 
swap, then will fail here.

As above, we shouldn't disallow eviction and swap during allocation, we 
aren't able to predict what case happen.
For over limit allocation, at worst, they will be returned with failed 
status while doing its CS.
If you think the allocation shouldn't be over limitation of memory, we 
can add the checking condition before allocation every time, but not 
disallow eviction and swap in allocation, which really breaks the used TTM.

Regards,
David Zhou

> +
> +				if (place &&
> +				    !bdev->driver->eviction_valuable(bo, place))
> +					continue;
> +
> +				ttm_bo_del_from_lru(bo);
> +
> +				ret = 0;
> +				locked = false;
> +				break;
> +			}
> +
>   			ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
>   			if (ret)
>   				continue;
> @@ -760,6 +777,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   				continue;
>   			}
>   
> +			locked = true;
>   			break;
>   		}
>   
> @@ -775,7 +793,8 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   	kref_get(&bo->list_kref);
>   
>   	if (!list_empty(&bo->ddestroy)) {
> -		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
> +		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu,
> +					  locked);
>   		kref_put(&bo->list_kref, ttm_bo_release_list);
>   		return ret;
>   	}
> @@ -786,7 +805,10 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   	BUG_ON(ret != 0);
>   
>   	ret = ttm_bo_evict(bo, interruptible, no_wait_gpu);
> -	ttm_bo_unreserve(bo);
> +	if (locked)
> +		ttm_bo_unreserve(bo);
> +	else
> +		ttm_bo_add_to_lru(bo);
>   
>   	kref_put(&bo->list_kref, ttm_bo_release_list);
>   	return ret;
> @@ -850,7 +872,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   			return ret;
>   		if (mem->mm_node)
>   			break;
> -		ret = ttm_mem_evict_first(bdev, mem_type, place,
> +		ret = ttm_mem_evict_first(bdev, bo->resv, mem_type, place,
>   					  interruptible, no_wait_gpu);
>   		if (unlikely(ret != 0))
>   			return ret;
> @@ -1353,7 +1375,8 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
>   	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>   		while (!list_empty(&man->lru[i])) {
>   			spin_unlock(&glob->lru_lock);
> -			ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
> +			ret = ttm_mem_evict_first(bdev, NULL, mem_type, NULL,
> +						  false, false);
>   			if (ret)
>   				return ret;
>   			spin_lock(&glob->lru_lock);

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[He, Roger]

static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
			       bool interruptible, bool no_wait_gpu,
			       bool unlock_resv)
{
	......
	ttm_bo_del_from_lru(bo);
	list_del_init(&bo->ddestroy);
	kref_put(&bo->list_kref, ttm_bo_ref_bug);

	spin_unlock(&glob->lru_lock);
	ttm_bo_cleanup_memtype_use(bo);

	if (unlock_resv)                                                              //[Roger]: not sure we should add condition here as well. If not, for per-vm-bo, eviction would unlock the VM root BO resv obj which is not we want I think.
	reservation_object_unlock(bo->resv);

	return 0;
}


Thanks
Roger(Hongbo.He)

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com] 
Sent: Thursday, November 09, 2017 4:59 PM
To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; He, Roger <Hongbo.He@amd.com>; michel@daenzer.net
Subject: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

Needed for the next patch.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 6f5d18366e6e..50a678b504f3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)  }
 
 /**
- * function ttm_bo_cleanup_refs_and_unlock
+ * function ttm_bo_cleanup_refs
  * If bo idle, remove from delayed- and lru lists, and unref.
  * If not idle, do nothing.
  *
  * Must be called with lru_lock and reservation held, this function
- * will drop both before returning.
+ * will drop the lru lock and optionally the reservation lock before returning.
  *
  * @interruptible         Any sleeps should occur interruptibly.
  * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
+ * @unlock_resv           Unlock the reservation lock as well.
  */
 
-static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
-					  bool interruptible,
-					  bool no_wait_gpu)
+static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
+			       bool interruptible, bool no_wait_gpu,
+			       bool unlock_resv)
 {
 	struct ttm_bo_global *glob = bo->glob;
 	struct reservation_object *resv;
@@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 	if (ret && !no_wait_gpu) {
 		long lret;
 
-		reservation_object_unlock(bo->resv);
+		if (unlock_resv)
+			reservation_object_unlock(bo->resv);
 		spin_unlock(&glob->lru_lock);
 
 		lret = reservation_object_wait_timeout_rcu(resv, true, @@ -531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
 			return -EBUSY;
 
 		spin_lock(&glob->lru_lock);
-		ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
-
-		/*
-		 * We raced, and lost, someone else holds the reservation now,
-		 * and is probably busy in ttm_bo_cleanup_memtype_use.
-		 *
-		 * Even if it's not the case, because we finished waiting any
-		 * delayed destruction would succeed, so just return success
-		 * here.
-		 */
-		if (ret) {
-			spin_unlock(&glob->lru_lock);
-			return 0;
+		if (unlock_resv) {
+			ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
+			/*
+			 * We raced, and lost, someone else holds the reservation now,
+			 * and is probably busy in ttm_bo_cleanup_memtype_use.
+			 *
+			 * Even if it's not the case, because we finished waiting any
+			 * delayed destruction would succeed, so just return success
+			 * here.
+			 */
+			if (ret) {
+				spin_unlock(&glob->lru_lock);
+				return 0;
+			}
+		} else {
+			ret = 0;
 		}
 	}
 
@@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
 		}
 
 		if (!ret)
-			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
-							     !remove_all);
+			ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
+						  true);
 		else
 			spin_unlock(&glob->lru_lock);
 
@@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
-						     no_wait_gpu);
+		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
@@ -1735,7 +1739,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
 	kref_get(&bo->list_kref);
 
 	if (!list_empty(&bo->ddestroy)) {
-		ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false);
+		ret = ttm_bo_cleanup_refs(bo, false, false, true);
 		kref_put(&bo->list_kref, ttm_bo_release_list);
 		return ret;
 	}
--
2.11.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/7] drm/ttm: user reservation object wrappers

[Michel Dänzer]

On 09/11/17 06:13 PM, Christian König wrote:
> Am 09.11.2017 um 17:50 schrieb Michel Dänzer:
>> On 09/11/17 09:59 AM, Christian König wrote:
>>> Consistently use the reservation object wrappers instead of accessing
>>> the ww_mutex directly.
>>>
>>> Additional to that use the reservation object wrappers directly
>>> instead of
>>> calling __ttm_bo_reserve with fixed parameters.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> [...]
>>
>>> @@ -1823,7 +1823,9 @@ int ttm_bo_wait_unreserved(struct
>>> ttm_buffer_object *bo)
>>>           return -ERESTARTSYS;
>>>       if (!ww_mutex_is_locked(&bo->resv->lock))
>>>           goto out_unlock;
>>> -    ret = __ttm_bo_reserve(bo, true, false, NULL);
>>> +    ret = reservation_object_lock_interruptible(bo->resv, NULL);
>>> +    if (ret = -EINTR)
>>> +        ret = -ERESTARTSYS;
>> Typo in the test, must be
>>
>>      if (ret == -EINTR)
>>
>>
>> This bug caused the Xorg process to hang for me when trying to run
>> glxgears, requiring a hard reboot. Did you accidentally send an untested
>> version of this patch?
> Yeah, just stumbled over this as well. I accidentally merged the fix for
> this into a later patch which I didn't send out yet.
> 
> Consider it fixed,

Thanks. With that, patches 1-6 are

Reviewed-and-Tested-by: Michel Dänzer <michel.daenzer@amd.com>


I sent another comment on patch 7.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional

[Christian König]

Good catch and yeah that is actually the real problem why my original 
patch didn't worked as expected.

Going to fix this in v2,
Christian.

Am 10.11.2017 um 10:55 schrieb He, Roger:
> static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> 			       bool interruptible, bool no_wait_gpu,
> 			       bool unlock_resv)
> {
> 	......
> 	ttm_bo_del_from_lru(bo);
> 	list_del_init(&bo->ddestroy);
> 	kref_put(&bo->list_kref, ttm_bo_ref_bug);
>
> 	spin_unlock(&glob->lru_lock);
> 	ttm_bo_cleanup_memtype_use(bo);
>
> 	if (unlock_resv)                                                              //[Roger]: not sure we should add condition here as well. If not, for per-vm-bo, eviction would unlock the VM root BO resv obj which is not we want I think.
> 	reservation_object_unlock(bo->resv);
>
> 	return 0;
> }
>
>
> Thanks
> Roger(Hongbo.He)
>
> -----Original Message-----
> From: Christian König [mailto:ckoenig.leichtzumerken@gmail.com]
> Sent: Thursday, November 09, 2017 4:59 PM
> To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; He, Roger <Hongbo.He@amd.com>; michel@daenzer.net
> Subject: [PATCH 6/7] drm/ttm: make unlocking in ttm_bo_cleanup_refs optional
>
> Needed for the next patch.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 52 ++++++++++++++++++++++++--------------------
>   1 file changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 6f5d18366e6e..50a678b504f3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -486,20 +486,21 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)  }
>   
>   /**
> - * function ttm_bo_cleanup_refs_and_unlock
> + * function ttm_bo_cleanup_refs
>    * If bo idle, remove from delayed- and lru lists, and unref.
>    * If not idle, do nothing.
>    *
>    * Must be called with lru_lock and reservation held, this function
> - * will drop both before returning.
> + * will drop the lru lock and optionally the reservation lock before returning.
>    *
>    * @interruptible         Any sleeps should occur interruptibly.
>    * @no_wait_gpu           Never wait for gpu. Return -EBUSY instead.
> + * @unlock_resv           Unlock the reservation lock as well.
>    */
>   
> -static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
> -					  bool interruptible,
> -					  bool no_wait_gpu)
> +static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> +			       bool interruptible, bool no_wait_gpu,
> +			       bool unlock_resv)
>   {
>   	struct ttm_bo_global *glob = bo->glob;
>   	struct reservation_object *resv;
> @@ -518,7 +519,8 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
>   	if (ret && !no_wait_gpu) {
>   		long lret;
>   
> -		reservation_object_unlock(bo->resv);
> +		if (unlock_resv)
> +			reservation_object_unlock(bo->resv);
>   		spin_unlock(&glob->lru_lock);
>   
>   		lret = reservation_object_wait_timeout_rcu(resv, true, @@ -531,19 +533,22 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo,
>   			return -EBUSY;
>   
>   		spin_lock(&glob->lru_lock);
> -		ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
> -
> -		/*
> -		 * We raced, and lost, someone else holds the reservation now,
> -		 * and is probably busy in ttm_bo_cleanup_memtype_use.
> -		 *
> -		 * Even if it's not the case, because we finished waiting any
> -		 * delayed destruction would succeed, so just return success
> -		 * here.
> -		 */
> -		if (ret) {
> -			spin_unlock(&glob->lru_lock);
> -			return 0;
> +		if (unlock_resv) {
> +			ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
> +			/*
> +			 * We raced, and lost, someone else holds the reservation now,
> +			 * and is probably busy in ttm_bo_cleanup_memtype_use.
> +			 *
> +			 * Even if it's not the case, because we finished waiting any
> +			 * delayed destruction would succeed, so just return success
> +			 * here.
> +			 */
> +			if (ret) {
> +				spin_unlock(&glob->lru_lock);
> +				return 0;
> +			}
> +		} else {
> +			ret = 0;
>   		}
>   	}
>   
> @@ -600,8 +605,8 @@ static int ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
>   		}
>   
>   		if (!ret)
> -			ret = ttm_bo_cleanup_refs_and_unlock(entry, false,
> -							     !remove_all);
> +			ret = ttm_bo_cleanup_refs(entry, false, !remove_all,
> +						  true);
>   		else
>   			spin_unlock(&glob->lru_lock);
>   
> @@ -770,8 +775,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   	kref_get(&bo->list_kref);
>   
>   	if (!list_empty(&bo->ddestroy)) {
> -		ret = ttm_bo_cleanup_refs_and_unlock(bo, interruptible,
> -						     no_wait_gpu);
> +		ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, true);
>   		kref_put(&bo->list_kref, ttm_bo_release_list);
>   		return ret;
>   	}
> @@ -1735,7 +1739,7 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
>   	kref_get(&bo->list_kref);
>   
>   	if (!list_empty(&bo->ddestroy)) {
> -		ret = ttm_bo_cleanup_refs_and_unlock(bo, false, false);
> +		ret = ttm_bo_cleanup_refs(bo, false, false, true);
>   		kref_put(&bo->list_kref, ttm_bo_release_list);
>   		return ret;
>   	}
> --
> 2.11.0
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm/ttm: optimize ttm_mem_evict_first v2

[Christian König]

Am 10.11.2017 um 08:22 schrieb Chunming Zhou:
>
>
> On 2017年11月09日 16:59, Christian König wrote:
>> Deleted BOs with the same reservation object can be reaped even if they
>> can't be reserved.
>>
>> v2: rebase and we still need to remove/add the BO from/to the LRU.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c | 39 
>> +++++++++++++++++++++++++++++++--------
>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 50a678b504f3..6545c4344684 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -735,20 +735,37 @@ bool ttm_bo_eviction_valuable(struct 
>> ttm_buffer_object *bo,
>>   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>>     static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>> -                uint32_t mem_type,
>> -                const struct ttm_place *place,
>> -                bool interruptible,
>> -                bool no_wait_gpu)
>> +                   struct reservation_object *resv,
>> +                   uint32_t mem_type,
>> +                   const struct ttm_place *place,
>> +                   bool interruptible,
>> +                   bool no_wait_gpu)
>>   {
>>       struct ttm_bo_global *glob = bdev->glob;
>>       struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>       struct ttm_buffer_object *bo;
>>       int ret = -EBUSY;
>> +    bool locked;
>>       unsigned i;
>>         spin_lock(&glob->lru_lock);
>>       for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>>           list_for_each_entry(bo, &man->lru[i], lru) {
>> +            if (bo->resv == resv) {
>> +                if (list_empty(&bo->ddestroy))
>> +                    continue;
> I don't think only destroying BO can be evicted under per-vm-bo case, 
> but also normal BO should as well.
> I'd give an example:
> 1. vm-A allocates all vram;
> 2. vm-B also try to allocate full vram, so the BOs of vram in vm-A 
> will be evicted to GTT.
> 3. vm-A is trying to allocate all GTT, if we don't allow eviction or 
> swap, then will fail here.

That is a really good example, thanks.

>
> As above, we shouldn't disallow eviction and swap during allocation, 
> we aren't able to predict what case happen.
> For over limit allocation, at worst, they will be returned with failed 
> status while doing its CS.
> If you think the allocation shouldn't be over limitation of memory, we 
> can add the checking condition before allocation every time, but not 
> disallow eviction and swap in allocation, which really breaks the used 
> TTM.

Ok, you convinced me. The case above indeed needs a better handling.

I will reactivate my operation context patch set for TTM. Shouldn't be 
to much work to get this going.

Regards,
Christian.

>
> Regards,
> David Zhou
>
>> +
>> +                if (place &&
>> +                    !bdev->driver->eviction_valuable(bo, place))
>> +                    continue;
>> +
>> +                ttm_bo_del_from_lru(bo);
>> +
>> +                ret = 0;
>> +                locked = false;
>> +                break;
>> +            }
>> +
>>               ret = reservation_object_trylock(bo->resv) ? 0 : -EBUSY;
>>               if (ret)
>>                   continue;
>> @@ -760,6 +777,7 @@ static int ttm_mem_evict_first(struct 
>> ttm_bo_device *bdev,
>>                   continue;
>>               }
>>   +            locked = true;
>>               break;
>>           }
>>   @@ -775,7 +793,8 @@ static int ttm_mem_evict_first(struct 
>> ttm_bo_device *bdev,
>>       kref_get(&bo->list_kref);
>>         if (!list_empty(&bo->ddestroy)) {
>> -        ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu, 
>> true);
>> +        ret = ttm_bo_cleanup_refs(bo, interruptible, no_wait_gpu,
>> +                      locked);
>>           kref_put(&bo->list_kref, ttm_bo_release_list);
>>           return ret;
>>       }
>> @@ -786,7 +805,10 @@ static int ttm_mem_evict_first(struct 
>> ttm_bo_device *bdev,
>>       BUG_ON(ret != 0);
>>         ret = ttm_bo_evict(bo, interruptible, no_wait_gpu);
>> -    ttm_bo_unreserve(bo);
>> +    if (locked)
>> +        ttm_bo_unreserve(bo);
>> +    else
>> +        ttm_bo_add_to_lru(bo);
>>         kref_put(&bo->list_kref, ttm_bo_release_list);
>>       return ret;
>> @@ -850,7 +872,7 @@ static int ttm_bo_mem_force_space(struct 
>> ttm_buffer_object *bo,
>>               return ret;
>>           if (mem->mm_node)
>>               break;
>> -        ret = ttm_mem_evict_first(bdev, mem_type, place,
>> +        ret = ttm_mem_evict_first(bdev, bo->resv, mem_type, place,
>>                         interruptible, no_wait_gpu);
>>           if (unlikely(ret != 0))
>>               return ret;
>> @@ -1353,7 +1375,8 @@ static int ttm_bo_force_list_clean(struct 
>> ttm_bo_device *bdev,
>>       for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>>           while (!list_empty(&man->lru[i])) {
>>               spin_unlock(&glob->lru_lock);
>> -            ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, 
>> false);
>> +            ret = ttm_mem_evict_first(bdev, NULL, mem_type, NULL,
>> +                          false, false);
>>               if (ret)
>>                   return ret;
>>               spin_lock(&glob->lru_lock);
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx