[PATCH] drm/ttm/nouveau: consolidate slowpath reserve

[Christian König]

Am 28.07.20 um 08:24 schrieb Dave Airlie:
> From: Dave Airlie <airlied at redhat.com>
>
> The WARN_ON in the non-underscore path is off questionable value
> (can we drop it from the non-slowpath?). At least for nouveau
> where it's just looked up the gem object we know the ttm object
> has a reference always so we can skip the check.

Yeah, agreed. Wanted to look into removing that for quite some time as well.

> It's probably nouveau could use execbut utils here at some point
> but for now align the code between them to always call the __
> versions, and remove the non underscored version.

Can we do it the other way around and remove all uses of the __ versions 
of the functions instead and then merge the __ version into the normal 
one without the WARN_ON()?

Christian.

>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>   drivers/gpu/drm/nouveau/nouveau_gem.c  |  6 ++---
>   drivers/gpu/drm/ttm/ttm_execbuf_util.c | 10 +--------
>   include/drm/ttm/ttm_bo_driver.h        | 31 +++++++++++---------------
>   3 files changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 81f111ad3f4f..d2d535d2ece1 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -422,15 +422,15 @@ validate_init(struct nouveau_channel *chan, struct drm_file *file_priv,
>   			break;
>   		}
>   
> -		ret = ttm_bo_reserve(&nvbo->bo, true, false, &op->ticket);
> +		ret = __ttm_bo_reserve(&nvbo->bo, true, false, &op->ticket);
>   		if (ret) {
>   			list_splice_tail_init(&vram_list, &op->list);
>   			list_splice_tail_init(&gart_list, &op->list);
>   			list_splice_tail_init(&both_list, &op->list);
>   			validate_fini_no_ticket(op, chan, NULL, NULL);
>   			if (unlikely(ret == -EDEADLK)) {
> -				ret = ttm_bo_reserve_slowpath(&nvbo->bo, true,
> -							      &op->ticket);
> +				ret = __ttm_bo_reserve_slowpath(&nvbo->bo, true,
> +								&op->ticket);
>   				if (!ret)
>   					res_bo = nvbo;
>   			}
> diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
> index 1797f04c0534..a24f13bc90fb 100644
> --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
> @@ -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);
>   		}
>   
>   		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..563b970949a1 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -695,7 +695,7 @@ static inline int ttm_bo_reserve(struct ttm_buffer_object *bo,
>   }
>   
>   /**
> - * ttm_bo_reserve_slowpath:
> + * __ttm_bo_reserve_slowpath:
>    * @bo: A pointer to a struct ttm_buffer_object.
>    * @interruptible: Sleep interruptible if waiting.
>    * @sequence: Set (@bo)->sequence to this value after lock
> @@ -704,24 +704,19 @@ static inline int ttm_bo_reserve(struct ttm_buffer_object *bo,
>    * from all our other reservations. Because there are no other reservations
>    * held by us, this function cannot deadlock any more.
>    */
> -static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo,
> -					  bool interruptible,
> -					  struct ww_acquire_ctx *ticket)
> +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;
>   }
>   
>   /**