Re: [PATCH] drm/ttm/amdgpu: consolidate ttm reserve paths

Am 28.07.20 um 21:11 schrieb Dave Airlie:

From: Dave Airlie airlied@redhat.com

Drop the WARN_ON and consolidate the two paths into one.

Use the consolidate slowpath in the execbuf utils code.

Signed-off-by: Dave Airlie airlied@redhat.com

drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 2 +- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 12 +-- include/drm/ttm/ttm_bo_driver.h | 91 ++++------------------ 3 files changed, 20 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h index afa5189dba7d..e01e8903741e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h @@ -160,7 +160,7 @@ static inline int amdgpu_bo_reserve(struct amdgpu_bo *bo, bool no_intr) struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); int r;

• r = __ttm_bo_reserve(&bo->tbo, !no_intr, false, NULL);

• r = ttm_bo_reserve(&bo->tbo, !no_intr, false, NULL); if (unlikely(r != 0)) { if (r != -ERESTARTSYS) dev_err(adev->dev, "%p reserve failed\n", bo);

diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 1797f04c0534..8a8f1a6a83a6 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -93,7 +93,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo;

• ret = __ttm_bo_reserve(bo, intr, (ticket == NULL), ticket);
  

• ret = ttm_bo_reserve(bo, intr, (ticket == NULL), ticket);
  

  if (ret == -EALREADY && dups) { struct ttm_validate_buffer *safe = entry; entry = list_prev_entry(entry, head);

@@ -119,13 +119,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, ttm_eu_backoff_reservation_reverse(list, entry);

if (ret == -EDEADLK) {

• 	if (intr) {
  

• 		ret = dma_resv_lock_slow_interruptible(bo->base.resv,
  

• 								 ticket);
  

• 	} else {
  

• 		dma_resv_lock_slow(bo->base.resv, ticket);
  

• 		ret = 0;
  

• 	}
  

• 	ret = ttm_bo_reserve_slowpath(bo, intr, ticket);
  

  }

The extra {} could be removed here as well, apart from that the patch is Reviewed-by: Christian König christian.koenig@amd.com.

Regards, Christian.

if (!ret && entry->num_shared)

@@ -133,8 +127,6 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, entry->num_shared);

if (unlikely(ret != 0)) {

• 	if (ret == -EINTR)
  

• 		ret = -ERESTARTSYS;
  if (ticket) {
  	ww_acquire_done(ticket);
  	ww_acquire_fini(ticket);
  

diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 5a37f1cc057e..bfa9d146d3d4 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -597,29 +597,30 @@ int ttm_mem_io_lock(struct ttm_mem_type_manager *man, bool interruptible); void ttm_mem_io_unlock(struct ttm_mem_type_manager *man);

/**

• • __ttm_bo_reserve:

• • ttm_bo_reserve:
  • @bo: A pointer to a struct ttm_buffer_object.
  • @interruptible: Sleep interruptible if waiting.
  • @no_wait: Don't sleep while trying to reserve, rather return -EBUSY.
  • @ticket: ticket used to acquire the ww_mutex.

• • Will not remove reserved buffers from the lru lists.
• • Otherwise identical to ttm_bo_reserve.

• • Locks a buffer object for validation. (Or prevents other processes from
• • locking it for validation), while taking a number of measures to prevent
• • deadlocks.
  • Returns:
  • -EDEADLK: The reservation may cause a deadlock.
  • Release all buffer reservations, wait for @bo to become unreserved and

• • try again. (only if use_sequence == 1).

• • try again.
  • -ERESTARTSYS: A wait for the buffer to become unreserved was interrupted by
  • a signal. Release all buffer reservations and return to user-space.
  • -EBUSY: The function needed to sleep, but @no_wait was true
  • -EALREADY: Bo already reserved using @ticket. This error code will only
  • be returned if @use_ticket is set to true.
  */

-static inline int __ttm_bo_reserve(struct ttm_buffer_object *bo,

• 		   bool interruptible, bool no_wait,
  

• 		   struct ww_acquire_ctx *ticket)
  

+static inline int ttm_bo_reserve(struct ttm_buffer_object *bo,

• 		 bool interruptible, bool no_wait,
  

• 		 struct ww_acquire_ctx *ticket)
  

  { int ret = 0;

@@ -641,59 +642,6 @@ static inline int __ttm_bo_reserve(struct ttm_buffer_object *bo, return ret; }

-/**

• • ttm_bo_reserve:
• • @bo: A pointer to a struct ttm_buffer_object.
• • @interruptible: Sleep interruptible if waiting.
• • @no_wait: Don't sleep while trying to reserve, rather return -EBUSY.
• • @ticket: ticket used to acquire the ww_mutex.
• • Locks a buffer object for validation. (Or prevents other processes from
• • locking it for validation) and removes it from lru lists, while taking
• • a number of measures to prevent deadlocks.
• • Deadlocks may occur when two processes try to reserve multiple buffers in
• • different order, either by will or as a result of a buffer being evicted
• • to make room for a buffer already reserved. (Buffers are reserved before
• • they are evicted). The following algorithm prevents such deadlocks from
• • occurring:
• • Processes attempting to reserve multiple buffers other than for eviction,
• • (typically execbuf), should first obtain a unique 32-bit
• • validation sequence number,
• • and call this function with @use_ticket == 1 and @ticket->stamp == the unique
• • sequence number. If upon call of this function, the buffer object is already
• • reserved, the validation sequence is checked against the validation
• • sequence of the process currently reserving the buffer,
• • and if the current validation sequence is greater than that of the process
• • holding the reservation, the function returns -EDEADLK. Otherwise it sleeps
• • waiting for the buffer to become unreserved, after which it retries
• • reserving.
• • The caller should, when receiving an -EDEADLK error
• • release all its buffer reservations, wait for @bo to become unreserved, and
• • then rerun the validation with the same validation sequence. This procedure
• • will always guarantee that the process with the lowest validation sequence
• • will eventually succeed, preventing both deadlocks and starvation.
• • Returns:
• • -EDEADLK: The reservation may cause a deadlock.
• • Release all buffer reservations, wait for @bo to become unreserved and
• • try again. (only if use_sequence == 1).
• • -ERESTARTSYS: A wait for the buffer to become unreserved was interrupted by
• • a signal. Release all buffer reservations and return to user-space.
• • -EBUSY: The function needed to sleep, but @no_wait was true
• • -EALREADY: Bo already reserved using @ticket. This error code will only
• • be returned if @use_ticket is set to true.
• */

-static inline int ttm_bo_reserve(struct ttm_buffer_object *bo,

• 		 bool interruptible, bool no_wait,
  

• 		 struct ww_acquire_ctx *ticket)
  

-{

• WARN_ON(!kref_read(&bo->kref));
• return __ttm_bo_reserve(bo, interruptible, no_wait, ticket);

-}

• /**
  • ttm_bo_reserve_slowpath:
  • @bo: A pointer to a struct ttm_buffer_object.

@@ -708,20 +656,15 @@ static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, bool interruptible, struct ww_acquire_ctx *ticket) {

• int ret = 0;
• WARN_ON(!kref_read(&bo->kref));
• if (interruptible)

• ret = dma_resv_lock_slow_interruptible(bo->base.resv,
  

• 						 ticket);
  

• else

• dma_resv_lock_slow(bo->base.resv, ticket);
  

• if (ret == -EINTR)

• ret = -ERESTARTSYS;
  

• return ret;

• if (interruptible) {

• int ret = dma_resv_lock_slow_interruptible(bo->base.resv,
  

• 					   ticket);
  

• if (ret == -EINTR)
  

• 	ret = -ERESTARTSYS;
  

• return ret;
  

• }

• dma_resv_lock_slow(bo->base.resv, ticket);

• return 0; }

  /**