Re: [PATCH 4/4] drm/xe/bo: add some annotations in bo_put()

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Sep 10, 2024 at 02:11:49PM +0100, Matthew Auld wrote:
> If the put() triggers bo destroy then there is at least one potential
> sleeping lock. Also annotate bos_lock and ggtt lock.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Tejas Upadhyay <tejas.upadhyay@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_bo.c | 14 ++++++++++++++
>  drivers/gpu/drm/xe/xe_bo.h |  6 +-----
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 53948a965404..4176b302c38d 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2320,6 +2320,20 @@ void xe_bo_put_commit(struct llist_head *deferred)
>  		drm_gem_object_free(&bo->ttm.base.refcount);
>  }
>  
> +void xe_bo_put(struct xe_bo *bo)

These are helpful but thinking we might need a version without asserts
that can be used in code paths where we know that we have another ref to
the BO. I can already think of a case in the SVM code where this would
be needed. In that version maybe we assert the ref count > 1, granted
that races but probably the best we can there.

What do you think?

Asserts look good though.

Matt

> +{
> +	might_sleep();
> +	if (bo) {
> +#ifdef CONFIG_PROC_FS
> +		if (bo->client)
> +			might_lock(&bo->client->bos_lock);
> +#endif
> +		if (bo->ggtt_node && bo->ggtt_node->ggtt)
> +			might_lock(&bo->ggtt_node->ggtt->lock);
> +		drm_gem_object_put(&bo->ttm.base);
> +	}
> +}
> +
>  /**
>   * xe_bo_dumb_create - Create a dumb bo as backing for a fb
>   * @file_priv: ...
> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> index dbfb3209615d..6e4be52306df 100644
> --- a/drivers/gpu/drm/xe/xe_bo.h
> +++ b/drivers/gpu/drm/xe/xe_bo.h
> @@ -126,11 +126,7 @@ static inline struct xe_bo *xe_bo_get(struct xe_bo *bo)
>  	return bo;
>  }
>  
> -static inline void xe_bo_put(struct xe_bo *bo)
> -{
> -	if (bo)
> -		drm_gem_object_put(&bo->ttm.base);
> -}
> +void xe_bo_put(struct xe_bo *bo);
>  
>  static inline void __xe_bo_unset_bulk_move(struct xe_bo *bo)
>  {
> -- 
> 2.46.0
>