Re: [PATCH v2 02/10] drm/ttm: replace TTMs refcount with the DRM refcount v4

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

On 7/6/26 12:07, Natalie Vock wrote:
> Instead of keeping a separate reference count for the TTM object also use
> the reference count for DRM GEM objects inside TTM.
> 
> Apart from avoiding two reference counts for one object this approach has
> the clear advantage of being able to use drm_exec inside TTM.
> 
> v2: adjust XE assert as well and re-enable disabled test
> v3: handle another case in i915
> v4: set GEM driver funcs of transfer BOs to point to the TTM free
> callback (Natalie)
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Since you also made changes to the code you should probably add Co-developed-by: Natalie Vock <nat@pixelcluster.dev>. Apart from that good catch with the transfer BOs.

Can somebody else review this now?

Regards,
Christian.

> Signed-off-by: Natalie Vock <nat@pixelcluster.dev>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c          |  35 +++---
>  drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c |   8 +-
>  drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c    |   2 -
>  drivers/gpu/drm/ttm/ttm_bo.c                     | 135 +++++++++++------------
>  drivers/gpu/drm/ttm/ttm_bo_internal.h            |  17 ++-
>  drivers/gpu/drm/ttm/ttm_bo_util.c                |  15 ++-
>  drivers/gpu/drm/xe/xe_bo.c                       |   2 +-
>  include/drm/ttm/ttm_bo.h                         |   9 --
>  8 files changed, 111 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index df3fcc2b1248e..642296602de69 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -939,7 +939,7 @@ void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj)
>  	 * Don't manipulate the TTM LRUs while in TTM bo destruction.
>  	 * We're called through i915_ttm_delete_mem_notify().
>  	 */
> -	if (!kref_read(&bo->kref))
> +	if (!kref_read(&bo->base.refcount))
>  		return;
>  
>  	/*
> @@ -957,30 +957,21 @@ void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj)
>  	 *
>  	 * TODO: consider maybe also bumping the shrinker list here when we have
>  	 * already unpinned it, which should give us something more like an LRU.
> -	 *
> -	 * TODO: There is a small window of opportunity for this function to
> -	 * get called from eviction after we've dropped the last GEM refcount,
> -	 * but before the TTM deleted flag is set on the object. Avoid
> -	 * adjusting the shrinker list in such cases, since the object is
> -	 * not available to the shrinker anyway due to its zero refcount.
> -	 * To fix this properly we should move to a TTM shrinker LRU list for
> -	 * these objects.
>  	 */
> -	if (kref_get_unless_zero(&obj->base.refcount)) {
> -		if (shrinkable != obj->mm.ttm_shrinkable) {
> -			if (shrinkable) {
> -				if (obj->mm.madv == I915_MADV_WILLNEED)
> -					__i915_gem_object_make_shrinkable(obj);
> -				else
> -					__i915_gem_object_make_purgeable(obj);
> -			} else {
> -				i915_gem_object_make_unshrinkable(obj);
> -			}
> -
> -			obj->mm.ttm_shrinkable = shrinkable;
> +	i915_gem_object_get(obj);
> +	if (shrinkable != obj->mm.ttm_shrinkable) {
> +		if (shrinkable) {
> +			if (obj->mm.madv == I915_MADV_WILLNEED)
> +				__i915_gem_object_make_shrinkable(obj);
> +			else
> +				__i915_gem_object_make_purgeable(obj);
> +		} else {
> +			i915_gem_object_make_unshrinkable(obj);
>  		}
> -		i915_gem_object_put(obj);
> +
> +		obj->mm.ttm_shrinkable = shrinkable;
>  	}
> +	i915_gem_object_put(obj);
>  
>  	/*
>  	 * Put on the correct LRU list depending on the MADV status
> diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
> index 56ad8ef325840..904cb4da6c9b3 100644
> --- a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
> +++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
> @@ -127,7 +127,7 @@ static void ttm_bo_init_reserved_sys_man(struct kunit *test)
>  	dma_resv_unlock(bo->base.resv);
>  
>  	KUNIT_EXPECT_EQ(test, err, 0);
> -	KUNIT_EXPECT_EQ(test, kref_read(&bo->kref), 1);
> +	KUNIT_EXPECT_EQ(test, kref_read(&bo->base.refcount), 1);
>  	KUNIT_EXPECT_PTR_EQ(test, bo->bdev, priv->ttm_dev);
>  	KUNIT_EXPECT_EQ(test, bo->type, bo_type);
>  	KUNIT_EXPECT_EQ(test, bo->page_alignment, PAGE_SIZE);
> @@ -176,7 +176,7 @@ static void ttm_bo_init_reserved_mock_man(struct kunit *test)
>  	dma_resv_unlock(bo->base.resv);
>  
>  	KUNIT_EXPECT_EQ(test, err, 0);
> -	KUNIT_EXPECT_EQ(test, kref_read(&bo->kref), 1);
> +	KUNIT_EXPECT_EQ(test, kref_read(&bo->base.refcount), 1);
>  	KUNIT_EXPECT_PTR_EQ(test, bo->bdev, priv->ttm_dev);
>  	KUNIT_EXPECT_EQ(test, bo->type, bo_type);
>  	KUNIT_EXPECT_EQ(test, ctx.bytes_moved, size);
> @@ -969,6 +969,8 @@ static void ttm_bo_validate_allowed_only_evict(struct kunit *test)
>  	ttm_mock_manager_fini(priv->ttm_dev, mem_multihop);
>  }
>  
> +extern const struct drm_gem_object_funcs ttm_deleted_object_funcs;
> +
>  static void ttm_bo_validate_deleted_evict(struct kunit *test)
>  {
>  	struct ttm_operation_ctx ctx_init = { }, ctx_val  = { };
> @@ -999,7 +1001,7 @@ static void ttm_bo_validate_deleted_evict(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, ttm_resource_manager_usage(man), big);
>  
>  	dma_resv_unlock(bo_big->base.resv);
> -	bo_big->deleted = true;
> +	bo_big->base.funcs = &ttm_deleted_object_funcs;
>  
>  	bo_small = ttm_bo_kunit_init(test, test->priv, small, NULL);
>  	bo_small->type = bo_type;
> diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
> index 5cfe8f3f80d75..b7ab19e0e4b2b 100644
> --- a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
> +++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
> @@ -189,8 +189,6 @@ struct ttm_buffer_object *ttm_bo_kunit_init(struct kunit *test,
>  	bo->bdev = devs->ttm_dev;
>  	bo->destroy = dummy_ttm_bo_destroy;
>  
> -	kref_init(&bo->kref);
> -
>  	return bo;
>  }
>  EXPORT_SYMBOL_GPL(ttm_bo_kunit_init);
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 3980f376e3ba4..2b470c1746f60 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -246,88 +246,84 @@ static void ttm_bo_delayed_delete(struct work_struct *work)
>  	ttm_bo_put(bo);
>  }
>  
> -static void ttm_bo_release(struct kref *kref)
> +/*
> + * All other callbacks should never ever be called on a deleted TTM object.
> + */
> +const struct drm_gem_object_funcs ttm_deleted_object_funcs = {
> +	.free = ttm_bo_free
> +};
> +EXPORT_SYMBOL_FOR_TESTS_ONLY(ttm_deleted_object_funcs);
> +
> +/* Returns true if the BO is about to get deleted */
> +static bool ttm_bo_is_zombie(struct ttm_buffer_object *bo)
> +{
> +	return bo->base.funcs == &ttm_deleted_object_funcs;
> +}
> +
> +void ttm_bo_fini(struct ttm_buffer_object *bo)
>  {
> -	struct ttm_buffer_object *bo =
> -	    container_of(kref, struct ttm_buffer_object, kref);
>  	struct ttm_device *bdev = bo->bdev;
>  	int ret;
>  
>  	WARN_ON_ONCE(bo->pin_count);
>  	WARN_ON_ONCE(bo->bulk_move);
>  
> -	if (!bo->deleted) {
> -		ret = ttm_bo_individualize_resv(bo);
> -		if (ret) {
> -			/* Last resort, if we fail to allocate memory for the
> -			 * fences block for the BO to become idle
> -			 */
> -			dma_resv_wait_timeout(bo->base.resv,
> -					      DMA_RESV_USAGE_BOOKKEEP, false,
> -					      30 * HZ);
> -		}
> +	ret = ttm_bo_individualize_resv(bo);
> +	if (ret) {
> +		/* Last resort, if we fail to allocate memory for the
> +		 * fences block for the BO to become idle
> +		 */
> +		dma_resv_wait_timeout(bo->base.resv, DMA_RESV_USAGE_BOOKKEEP,
> +				      false, 30 * HZ);
> +	}
>  
> -		if (bdev->funcs->release_notify)
> -			bdev->funcs->release_notify(bo);
> +	if (bo->bdev->funcs->release_notify)
> +		bo->bdev->funcs->release_notify(bo);
>  
> -		drm_vma_offset_remove(bdev->vma_manager, &bo->base.vma_node);
> -		ttm_mem_io_free(bdev, bo->resource);
> +	drm_vma_offset_remove(bdev->vma_manager, &bo->base.vma_node);
> +	ttm_mem_io_free(bdev, bo->resource);
>  
> -		if (!dma_resv_test_signaled(&bo->base._resv,
> -					    DMA_RESV_USAGE_BOOKKEEP) ||
> -		    (want_init_on_free() && (bo->ttm != NULL)) ||
> -		    bo->type == ttm_bo_type_sg ||
> -		    !dma_resv_trylock(bo->base.resv)) {
> -			/* The BO is not idle, resurrect it for delayed destroy */
> -			ttm_bo_flush_all_fences(bo);
> -			bo->deleted = true;
> +	if (!dma_resv_test_signaled(&bo->base._resv, DMA_RESV_USAGE_BOOKKEEP) ||
> +	    (want_init_on_free() && (bo->ttm != NULL)) ||
> +	    bo->type == ttm_bo_type_sg ||
> +	    !dma_resv_trylock(bo->base.resv)) {
> +		/* The BO is not idle, resurrect it for delayed destroy */
> +		ttm_bo_flush_all_fences(bo);
>  
> -			spin_lock(&bdev->lru_lock);
> -
> -			/*
> -			 * Make pinned bos immediately available to
> -			 * shrinkers, now that they are queued for
> -			 * destruction.
> -			 *
> -			 * FIXME: QXL is triggering this. Can be removed when the
> -			 * driver is fixed.
> -			 */
> -			if (bo->pin_count) {
> -				bo->pin_count = 0;
> -				ttm_resource_move_to_lru_tail(bo->resource);
> -			}
> +		spin_lock(&bo->bdev->lru_lock);
>  
> -			kref_init(&bo->kref);
> -			spin_unlock(&bdev->lru_lock);
> +		/*
> +		 * Make pinned bos immediately available to
> +		 * shrinkers, now that they are queued for
> +		 * destruction.
> +		 *
> +		 * FIXME: QXL is triggering this. Can be removed when the
> +		 * driver is fixed.
> +		 */
> +		if (bo->pin_count) {
> +			bo->pin_count = 0;
> +			ttm_resource_move_to_lru_tail(bo->resource);
> +		}
>  
> -			INIT_WORK(&bo->delayed_delete, ttm_bo_delayed_delete);
> +		kref_init(&bo->base.refcount);
> +		bo->base.funcs = &ttm_deleted_object_funcs;
> +		spin_unlock(&bo->bdev->lru_lock);
>  
> -			/* Schedule the worker on the closest NUMA node. This
> -			 * improves performance since system memory might be
> -			 * cleared on free and that is best done on a CPU core
> -			 * close to it.
> -			 */
> -			queue_work_node(bdev->pool.nid, bdev->wq, &bo->delayed_delete);
> -			return;
> -		}
> +		INIT_WORK(&bo->delayed_delete, ttm_bo_delayed_delete);
>  
> +		/* Schedule the worker on the closest NUMA node. This
> +		 * improves performance since system memory might be
> +		 * cleared on free and that is best done on a CPU core
> +		 * close to it.
> +		 */
> +		queue_work_node(bdev->pool.nid, bdev->wq, &bo->delayed_delete);
> +	} else {
>  		ttm_bo_cleanup_memtype_use(bo);
>  		dma_resv_unlock(bo->base.resv);
> -	}
>  
> -	atomic_dec(&ttm_glob.bo_count);
> -	bo->destroy(bo);
> -}
> -
> -/* TODO: remove! */
> -void ttm_bo_put(struct ttm_buffer_object *bo)
> -{
> -	kref_put(&bo->kref, ttm_bo_release);
> -}
> -
> -void ttm_bo_fini(struct ttm_buffer_object *bo)
> -{
> -	ttm_bo_put(bo);
> +		atomic_dec(&ttm_glob.bo_count);
> +		bo->destroy(bo);
> +	}
>  }
>  EXPORT_SYMBOL(ttm_bo_fini);
>  
> @@ -470,7 +466,7 @@ int ttm_bo_evict_first(struct ttm_device *bdev, struct ttm_resource_manager *man
>  	if (!bo->resource || bo->resource->mem_type != mem_type)
>  		goto out_bo_moved;
>  
> -	if (bo->deleted) {
> +	if (ttm_bo_is_zombie(bo)) {
>  		ret = ttm_bo_wait_ctx(bo, ctx);
>  		if (!ret)
>  			ttm_bo_cleanup_memtype_use(bo);
> @@ -524,7 +520,7 @@ static s64 ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *
>  	if (bo->pin_count || !bo->bdev->funcs->eviction_valuable(bo, evict_walk->place))
>  		return 0;
>  
> -	if (bo->deleted) {
> +	if (ttm_bo_is_zombie(bo)) {
>  		lret = ttm_bo_wait_ctx(bo, walk->arg.ctx);
>  		if (!lret)
>  			ttm_bo_cleanup_memtype_use(bo);
> @@ -624,7 +620,6 @@ static int ttm_bo_evict_alloc(struct ttm_device *bdev,
>  void ttm_bo_pin(struct ttm_buffer_object *bo)
>  {
>  	dma_resv_assert_held(bo->base.resv);
> -	WARN_ON_ONCE(!kref_read(&bo->kref));
>  	spin_lock(&bo->bdev->lru_lock);
>  	if (bo->resource)
>  		ttm_resource_del_bulk_move(bo->resource, bo);
> @@ -643,7 +638,6 @@ EXPORT_SYMBOL(ttm_bo_pin);
>  void ttm_bo_unpin(struct ttm_buffer_object *bo)
>  {
>  	dma_resv_assert_held(bo->base.resv);
> -	WARN_ON_ONCE(!kref_read(&bo->kref));
>  	if (WARN_ON_ONCE(!bo->pin_count))
>  		return;
>  
> @@ -934,7 +928,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev, struct ttm_buffer_object *bo,
>  {
>  	int ret;
>  
> -	kref_init(&bo->kref);
>  	bo->bdev = bdev;
>  	bo->type = type;
>  	bo->page_alignment = alignment;
> @@ -1131,8 +1124,8 @@ ttm_bo_swapout_cb(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo)
>  		goto out;
>  	}
>  
> -	if (bo->deleted) {
> -		pgoff_t num_pages = tt->num_pages;
> +	if (ttm_bo_is_zombie(bo)) {
> +		pgoff_t num_pages = bo->ttm->num_pages;
>  
>  		ret = ttm_bo_wait_ctx(bo, ctx);
>  		if (ret)
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_internal.h b/drivers/gpu/drm/ttm/ttm_bo_internal.h
> index e0d48eac74b03..ded2a47be0bcb 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_internal.h
> +++ b/drivers/gpu/drm/ttm/ttm_bo_internal.h
> @@ -27,6 +27,14 @@
>  
>  #include <drm/ttm/ttm_bo.h>
>  
> +static inline void ttm_bo_free(struct drm_gem_object *gobj)
> +{
> +	struct ttm_buffer_object *bo = container_of(gobj, typeof(*bo), base);
> +
> +	atomic_dec(&ttm_glob.bo_count);
> +	bo->destroy(bo);
> +}
> +
>  /**
>   * ttm_bo_get - reference a struct ttm_buffer_object
>   *
> @@ -34,7 +42,7 @@
>   */
>  static inline void ttm_bo_get(struct ttm_buffer_object *bo)
>  {
> -	kref_get(&bo->kref);
> +	drm_gem_object_get(&bo->base);
>  }
>  
>  /**
> @@ -50,11 +58,14 @@ static inline void ttm_bo_get(struct ttm_buffer_object *bo)
>  static inline __must_check struct ttm_buffer_object *
>  ttm_bo_get_unless_zero(struct ttm_buffer_object *bo)
>  {
> -	if (!kref_get_unless_zero(&bo->kref))
> +	if (!kref_get_unless_zero(&bo->base.refcount))
>  		return NULL;
>  	return bo;
>  }
>  
> -void ttm_bo_put(struct ttm_buffer_object *bo);
> +static inline void ttm_bo_put(struct ttm_buffer_object *bo)
> +{
> +	drm_gem_object_put(&bo->base);
> +}
>  
>  #endif
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 3e3c201a02226..7ed085adf1c9b 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -41,6 +41,18 @@
>  
>  #include "ttm_bo_internal.h"
>  
> +static void ttm_transfer_object_free(struct drm_gem_object *obj)
> +{
> +	struct ttm_buffer_object *bo =
> +		container_of(obj, struct ttm_buffer_object, base);
> +
> +	ttm_bo_fini(bo);
> +}
> +
> +const struct drm_gem_object_funcs ttm_transfer_object_funcs = {
> +	.free = ttm_transfer_object_free,
> +};
> +
>  struct ttm_transfer_obj {
>  	struct ttm_buffer_object base;
>  	struct ttm_buffer_object *bo;
> @@ -247,7 +259,8 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
>  	atomic_inc(&ttm_glob.bo_count);
>  	drm_vma_node_reset(&fbo->base.base.vma_node);
>  
> -	kref_init(&fbo->base.kref);
> +	kref_init(&fbo->base.base.refcount);
> +	fbo->base.base.funcs = &ttm_transfer_object_funcs;
>  	fbo->base.destroy = &ttm_transfered_destroy;
>  	fbo->base.pin_count = 0;
>  	if (bo->type != ttm_bo_type_sg)
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 85e6d9a0f575b..5843f850339c7 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1651,7 +1651,7 @@ static bool xe_ttm_bo_lock_in_destructor(struct ttm_buffer_object *ttm_bo)
>  	struct xe_device *xe = ttm_to_xe_device(ttm_bo->bdev);
>  	bool locked;
>  
> -	xe_assert(xe, !kref_read(&ttm_bo->kref));
> +	xe_assert(xe, !kref_read(&ttm_bo->base.refcount));
>  
>  	/*
>  	 * We can typically only race with TTM trylocking under the
> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> index 8310bc3d55f90..1eae9eea5ff32 100644
> --- a/include/drm/ttm/ttm_bo.h
> +++ b/include/drm/ttm/ttm_bo.h
> @@ -78,11 +78,8 @@ enum ttm_bo_type {
>   * @type: The bo type.
>   * @page_alignment: Page alignment.
>   * @destroy: Destruction function. If NULL, kfree is used.
> - * @kref: Reference count of this buffer object. When this refcount reaches
> - * zero, the object is destroyed or put on the delayed delete list.
>   * @resource: structure describing current placement.
>   * @ttm: TTM structure holding system pages.
> - * @deleted: True if the object is only a zombie and already deleted.
>   * @bulk_move: The bulk move object.
>   * @priority: Priority for LRU, BOs with lower priority are evicted first.
>   * @pin_count: Pin count.
> @@ -109,17 +106,11 @@ struct ttm_buffer_object {
>  	uint32_t page_alignment;
>  	void (*destroy) (struct ttm_buffer_object *);
>  
> -	/*
> -	* Members not needing protection.
> -	*/
> -	struct kref kref;
> -
>  	/*
>  	 * Members protected by the bo::resv::reserved lock.
>  	 */
>  	struct ttm_resource *resource;
>  	struct ttm_tt *ttm;
> -	bool deleted;
>  	struct ttm_lru_bulk_move *bulk_move;
>  	unsigned priority;
>  	unsigned pin_count;
>