Re: [PATCH 04/12] drm/ttm: Switch LRU cursor to track dma_resv instead of buffer objects

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

On Mon, Jul 13, 2026 at 03:40:55PM +0200, Thomas Hellström wrote:
> On Fri, 2026-07-10 at 20:52 +0200, Christian König wrote:
> > Refactor the LRU cursor to hold references to dma_resv directly while
> > locking it rather than the buffer object.
> > 
> > This avoid the need to grab a reference to the BO and so allows
> > handling
> > of BOs with zero reference count.
> > 
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > ---
> >  drivers/gpu/drm/ttm/ttm_bo_util.c  | 124 +++++++++++++++------------
> > --
> >  drivers/gpu/drm/ttm/ttm_resource.c |  18 +++++
> >  include/drm/ttm/ttm_bo.h           |  10 +--
> >  include/drm/ttm/ttm_resource.h     |   2 +
> >  4 files changed, 87 insertions(+), 67 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > index 029c218f9fb47..1f7361604b552 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > @@ -819,19 +819,17 @@ int ttm_bo_pipeline_gutting(struct
> > ttm_buffer_object *bo)
> >  }
> >  
> >  static bool ttm_lru_walk_trylock(struct ttm_bo_lru_cursor *curs,
> > -				 struct ttm_buffer_object *bo)
> > +				 struct dma_resv *resv)
> >  {
> >  	struct ttm_operation_ctx *ctx = curs->arg->ctx;
> >  
> > -	curs->needs_unlock = false;
> > -
> > -	if (dma_resv_trylock(bo->base.resv)) {
> > -		curs->needs_unlock = true;
> > +	if (dma_resv_trylock(resv)) {
> > +		curs->resv = dma_resv_get(resv);
> >  		return true;
> >  	}
> >  
> > -	if (bo->base.resv == ctx->resv && ctx->allow_res_evict) {
> > -		dma_resv_assert_held(bo->base.resv);
> > +	if (resv == ctx->resv && ctx->allow_res_evict) {
> > +		dma_resv_assert_held(resv);
> >  		return true;
> >  	}
> >  
> > @@ -839,18 +837,18 @@ static bool ttm_lru_walk_trylock(struct
> > ttm_bo_lru_cursor *curs,
> >  }
> >  
> >  static int ttm_lru_walk_ticketlock(struct ttm_bo_lru_cursor *curs,
> > -				   struct ttm_buffer_object *bo)
> > +				   struct dma_resv *resv)
> >  {
> >  	struct ttm_lru_walk_arg *arg = curs->arg;
> >  	int ret;
> >  
> >  	if (arg->ctx->interruptible)
> > -		ret = dma_resv_lock_interruptible(bo->base.resv,
> > arg->ticket);
> > +		ret = dma_resv_lock_interruptible(resv, arg-
> > >ticket);
> >  	else
> > -		ret = dma_resv_lock(bo->base.resv, arg->ticket);
> > +		ret = dma_resv_lock(resv, arg->ticket);
> >  
> >  	if (!ret) {
> > -		curs->needs_unlock = true;
> > +		curs->resv = dma_resv_get(resv);
> >  		/*
> >  		 * Only a single ticketlock per loop. Ticketlocks
> > are prone
> >  		 * to return -EDEADLK causing the eviction to fail,
> > so
> > @@ -920,14 +918,16 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk
> > *walk, struct ttm_device *bdev,
> >  }
> >  EXPORT_SYMBOL(ttm_lru_walk_for_evict);
> >  
> > -static void ttm_bo_lru_cursor_cleanup_bo(struct ttm_bo_lru_cursor
> > *curs)
> > +static void ttm_bo_lru_cursor_cleanup(struct ttm_bo_lru_cursor
> > *curs)
> >  {
> > -	struct ttm_buffer_object *bo = curs->bo;
> > +	if (curs->resv) {
> > +		dma_resv_unlock(curs->resv);
> > +		dma_resv_put(curs->resv);
> > +		curs->resv = NULL;
> > +	}
> >  
> > -	if (bo) {
> > -		if (curs->needs_unlock)
> > -			dma_resv_unlock(bo->base.resv);
> > -		ttm_bo_put(bo);
> > +	if (curs->bo) {
> > +		drm_gem_object_put(&curs->bo->base);
> >  		curs->bo = NULL;
> >  	}
> >  }
> > @@ -941,7 +941,7 @@ void ttm_bo_lru_cursor_fini(struct
> > ttm_bo_lru_cursor *curs)
> >  {
> >  	spinlock_t *lru_lock = &curs->res_curs.man->bdev->lru_lock;
> >  
> > -	ttm_bo_lru_cursor_cleanup_bo(curs);
> > +	ttm_bo_lru_cursor_cleanup(curs);
> >  	spin_lock(lru_lock);
> >  	ttm_resource_cursor_fini(&curs->res_curs);
> >  	spin_unlock(lru_lock);
> > @@ -972,21 +972,18 @@ ttm_bo_lru_cursor_init(struct ttm_bo_lru_cursor
> > *curs,
> >  EXPORT_SYMBOL(ttm_bo_lru_cursor_init);
> >  
> >  static struct ttm_buffer_object *
> > -__ttm_bo_lru_cursor_next(struct ttm_bo_lru_cursor *curs)
> > +__ttm_bo_lru_cursor_iter(struct ttm_bo_lru_cursor *curs, bool first)
> >  {
> >  	spinlock_t *lru_lock = &curs->res_curs.man->bdev->lru_lock;
> > -	struct ttm_resource *res = NULL;
> > -	struct ttm_buffer_object *bo;
> >  	struct ttm_lru_walk_arg *arg = curs->arg;
> > -	bool first = !curs->bo;
> > -
> > -	ttm_bo_lru_cursor_cleanup_bo(curs);
> > +	int ret;
> >  
> > -	spin_lock(lru_lock);
> >  	for (;;) {
> > -		int mem_type, ret = 0;
> > -		bool bo_locked = false;
> > +		struct ttm_resource *res;
> > +
> > +		ttm_bo_lru_cursor_cleanup(curs);
> >  
> > +		spin_lock(lru_lock);
> >  		if (first) {
> >  			res = ttm_resource_manager_first(&curs-
> > >res_curs);
> >  			first = false;
> > @@ -996,43 +993,48 @@ __ttm_bo_lru_cursor_next(struct
> > ttm_bo_lru_cursor *curs)
> >  		if (!res)
> >  			break;
> >  
> > -		bo = res->bo;
> > -		if (ttm_lru_walk_trylock(curs, bo))
> > -			bo_locked = true;
> > -		else if (!arg->ticket || arg->ctx->no_wait_gpu ||
> > arg->trylock_only)
> > -			continue;
> > -
> > -		if (!ttm_bo_get_unless_zero(bo)) {
> > -			if (curs->needs_unlock)
> > -				dma_resv_unlock(bo->base.resv);
> > -			continue;
> > +		if (!ttm_lru_walk_trylock(curs, res->bo->base.resv))
> > {
> > +			struct dma_resv *resv;
> > +
> > +			if (!arg->ticket || arg->ctx->no_wait_gpu ||
> > +			    arg->trylock_only) {
> > +				spin_unlock(lru_lock);
> > +				continue;
> > +			}
> > +
> > +			resv = dma_resv_get(res->bo->base.resv);
> > +			spin_unlock(lru_lock);
> > +
> > +			ret = ttm_lru_walk_ticketlock(curs, resv);
> > +			if (ret && ret != -EALREADY)
> > +				return ERR_PTR(ret);
> > +
> > +			/*
> > +			 * We need to double check that we still
> > have the same
> > +			 * dma_resv object.
> 
> While I think this would actually work, we need documentation and
> asserts of what exactly we're relying on here to keep the bo alive.
> Otherwise future work will most likely break something.
> 

As a general comment, we should build some TTM assertion macros akin to
xe_assert. TTM has a lot of subtle invariants that make the whole thing
work, and many of them are not guarded by assertions. Assertions, of
course, help catch bugs at runtime, but they also serve as
self-documenting invariants, which leads to a codebase that is easier to
understand and maintain.

Matt 

> Thanks,
> Thomas
> 
> 
> > +			 */
> > +			spin_lock(lru_lock);
> > +			res = ttm_resource_manager_current(&curs-
> > >res_curs);
> > +			if (ret || !res || res->bo->base.resv !=
> > resv) {
> > +				spin_unlock(lru_lock);
> > +				dma_resv_put(resv);
> > +				continue;
> > +			}
> > +			dma_resv_put(resv);
> >  		}
> > -
> > -		mem_type = res->mem_type;
> >  		spin_unlock(lru_lock);
> > -		if (!bo_locked)
> > -			ret = ttm_lru_walk_ticketlock(curs, bo);
> > +
> > +		/* Grab a GEM reference to the BO if it isn't
> > already deleted */
> 
> 
> 
> > +		if (kref_get_unless_zero(&res->bo->base.refcount))
> > +			curs->bo = res->bo;
> >  
> >  		/*
> > -		 * Note that in between the release of the lru lock
> > and the
> > -		 * ticketlock, the bo may have switched resource,
> > -		 * and also memory type, since the resource may have
> > been
> > -		 * freed and allocated again with a different memory
> > type.
> > -		 * In that case, just skip it.
> > +		 * The BO is now locked so it can't be released any
> > more until
> > +		 * we drop both the lock and the eventual GEM
> > reference.
> >  		 */
> > -		curs->bo = bo;
> > -		if (!ret && bo->resource && bo->resource->mem_type
> > == mem_type)
> > -			return bo;
> > -
> > -		ttm_bo_lru_cursor_cleanup_bo(curs);
> > -		if (ret && ret != -EALREADY)
> > -			return ERR_PTR(ret);
> > -
> > -		spin_lock(lru_lock);
> > +		return res->bo;
> >  	}
> > -
> > -	spin_unlock(lru_lock);
> > -	return res ? bo : NULL;
> > +	return NULL;
> >  }
> >  
> >  /**
> > @@ -1046,7 +1048,7 @@ __ttm_bo_lru_cursor_next(struct
> > ttm_bo_lru_cursor *curs)
> >   */
> >  struct ttm_buffer_object *ttm_bo_lru_cursor_next(struct
> > ttm_bo_lru_cursor *curs)
> >  {
> > -	return __ttm_bo_lru_cursor_next(curs);
> > +	return __ttm_bo_lru_cursor_iter(curs, false);
> >  }
> >  EXPORT_SYMBOL(ttm_bo_lru_cursor_next);
> >  
> > @@ -1060,8 +1062,8 @@ EXPORT_SYMBOL(ttm_bo_lru_cursor_next);
> >   */
> >  struct ttm_buffer_object *ttm_bo_lru_cursor_first(struct
> > ttm_bo_lru_cursor *curs)
> >  {
> > -	ttm_bo_lru_cursor_cleanup_bo(curs);
> > -	return __ttm_bo_lru_cursor_next(curs);
> > +	ttm_bo_lru_cursor_cleanup(curs);
> > +	return __ttm_bo_lru_cursor_iter(curs, true);
> >  }
> >  EXPORT_SYMBOL(ttm_bo_lru_cursor_first);
> >  
> > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
> > b/drivers/gpu/drm/ttm/ttm_resource.c
> > index 154d6739256f8..4a765b25472c3 100644
> > --- a/drivers/gpu/drm/ttm/ttm_resource.c
> > +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> > @@ -714,6 +714,24 @@ ttm_resource_manager_next(struct
> > ttm_resource_cursor *cursor)
> >  	return NULL;
> >  }
> >  
> > +/* TODO */
> > +struct ttm_resource *
> > +ttm_resource_manager_current(struct ttm_resource_cursor *cursor)
> > +{
> > +	struct ttm_resource_manager *man = cursor->man;
> > +	struct ttm_lru_item *lru;
> > +
> > +	lockdep_assert_held(&man->bdev->lru_lock);
> > +
> > +	lru = &cursor->hitch;
> > +	list_for_each_entry_continue_reverse(lru, &man->lru[cursor-
> > >priority],
> > +					     link) {
> > +		if (ttm_lru_item_is_res(lru))
> > +			return ttm_lru_item_to_res(lru);
> > +	}
> > +	return NULL;
> > +}
> > +
> >  /**
> >   * ttm_lru_first_res_or_null() - Return the first resource on an lru
> > list
> >   * @head: The list head of the lru list.
> > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > index 8310bc3d55f90..30e835414e721 100644
> > --- a/include/drm/ttm/ttm_bo.h
> > +++ b/include/drm/ttm/ttm_bo.h
> > @@ -488,15 +488,13 @@ struct ttm_bo_lru_cursor {
> >  	/** @res_curs: Embedded struct ttm_resource_cursor. */
> >  	struct ttm_resource_cursor res_curs;
> >  	/**
> > -	 * @bo: Buffer object pointer if a buffer object is
> > refcounted,
> > -	 * NULL otherwise.
> > +	 * @resv: reference to the locked dma_resv
> >  	 */
> > -	struct ttm_buffer_object *bo;
> > +	struct dma_resv *resv;
> >  	/**
> > -	 * @needs_unlock: Valid iff @bo != NULL. The bo resv needs
> > -	 * unlock before the next iteration or after loop exit.
> > +	 * @bo: TTM BO with GEM reference, NULL for deleted BOs
> >  	 */
> > -	bool needs_unlock;
> > +	struct ttm_buffer_object *bo;
> >  	/** @arg: Pointer to common BO LRU walk arguments. */
> >  	struct ttm_lru_walk_arg *arg;
> >  };
> > diff --git a/include/drm/ttm/ttm_resource.h
> > b/include/drm/ttm/ttm_resource.h
> > index a5d386583fb6e..e8e9c8b81ce4b 100644
> > --- a/include/drm/ttm/ttm_resource.h
> > +++ b/include/drm/ttm/ttm_resource.h
> > @@ -488,6 +488,8 @@ struct ttm_resource *
> >  ttm_resource_manager_first(struct ttm_resource_cursor *cursor);
> >  struct ttm_resource *
> >  ttm_resource_manager_next(struct ttm_resource_cursor *cursor);
> > +struct ttm_resource *
> > +ttm_resource_manager_current(struct ttm_resource_cursor *cursor);
> >  
> >  struct ttm_resource *
> >  ttm_lru_first_res_or_null(struct list_head *head);