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 03:26:42PM +0000, Matthew Brost wrote:
> On Tue, Sep 10, 2024 at 04:03:58PM +0100, Matthew Auld wrote:
> > On 10/09/2024 15:49, Matthew Brost wrote:
> > > 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)
> > > > +{
> > > > +	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);
> > > 
> > > Missed this. We can take dma-resv lock too if we get into the TTM code
> > > upon this put too, right? We should probably add that annotate too.
> > 
> > Yeah, I was also wondering the same thing, but it looks to only be trylock
> > in ttm_bo_release(). AFAICT it will only grab the full dma-resv lock from a
> > worker, which it does if the lock is contended, or if there are unsignalled
> > fences and we need to wait.
> > 
> 
> Ah ok. I was thinking about this in the path of reclaim, the does
> trylocking make it ok to take a dma-resv lock? If it doesn't then I
> think we need some sort additional annotation so the final put is not in
> the path of reclaim.
> 

Ignore this. I was confusing myself, dma-resv is outside of reclaim.
Annotations look good.

Matt

> Matt
> 
> > > 
> > > Matt
> > > 
> > > > +		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
> > > >