Re: [PATCH v14 1/8] drm/ttm: Balance ttm_resource_cursor_init() and ttm_resource_cursor_fini()

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2024-11-20 at 11:51 +0100, Christian König wrote:
> Am 15.11.24 um 16:01 schrieb Thomas Hellström:
> > Make the interface more symmetric by providing and using a
> > ttm_resource_cursor_init().
> > 
> > v10:
> > - Fix a stray newline (Matthew Brost)
> > - Update kerneldoc (Matthew Brost)
> > 
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > Reviewed-by: Christian König <christian.koenig@amd.com>
> 
> Did you plan to merge this through drm-misc-next or the XE branch?
> 
> If through drm-misc-next then I would go ahead and push this patch
> since 
> that is really a stand alone cleanup.

I was planning to merge it all through drm-xe-next, so I'll hold off
merging that patch.

Thanks,
Thomas

> 
> Regards,
> Christian.
> 
> > ---
> >   drivers/gpu/drm/ttm/ttm_bo.c       |  3 ++-
> >   drivers/gpu/drm/ttm/ttm_bo_util.c  |  3 ++-
> >   drivers/gpu/drm/ttm/ttm_resource.c | 35 ++++++++++++++++++++-----
> > -----
> >   include/drm/ttm/ttm_resource.h     | 11 +++++-----
> >   4 files changed, 34 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> > b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 48c5365efca1..06d6a452c4f4 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -450,7 +450,8 @@ int ttm_bo_evict_first(struct ttm_device *bdev,
> > struct ttm_resource_manager *man
> >   	int ret = 0;
> >   
> >   	spin_lock(&bdev->lru_lock);
> > -	res = ttm_resource_manager_first(man, &cursor);
> > +	ttm_resource_cursor_init(&cursor, man);
> > +	res = ttm_resource_manager_first(&cursor);
> >   	ttm_resource_cursor_fini(&cursor);
> >   	if (!res) {
> >   		ret = -ENOENT;
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > index d939925efa81..917096bd5f68 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > @@ -865,7 +865,8 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk
> > *walk, struct ttm_device *bdev,
> >   	s64 lret;
> >   
> >   	spin_lock(&bdev->lru_lock);
> > -	ttm_resource_manager_for_each_res(man, &cursor, res) {
> > +	ttm_resource_cursor_init(&cursor, man);
> > +	ttm_resource_manager_for_each_res(&cursor, res) {
> >   		struct ttm_buffer_object *bo = res->bo;
> >   		bool bo_needs_unlock = false;
> >   		bool bo_locked = false;
> > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
> > b/drivers/gpu/drm/ttm/ttm_resource.c
> > index a87665eb28a6..e19360cc7930 100644
> > --- a/drivers/gpu/drm/ttm/ttm_resource.c
> > +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> > @@ -81,6 +81,23 @@ static void ttm_bulk_move_drop_cursors(struct
> > ttm_lru_bulk_move *bulk)
> >   		ttm_resource_cursor_clear_bulk(cursor);
> >   }
> >   
> > +/**
> > + * ttm_resource_cursor_init() - Initialize a struct
> > ttm_resource_cursor
> > + * @cursor: The cursor to initialize.
> > + * @man: The resource manager.
> > + *
> > + * Initialize the cursor before using it for iteration.
> > + */
> > +void ttm_resource_cursor_init(struct ttm_resource_cursor *cursor,
> > +			      struct ttm_resource_manager *man)
> > +{
> > +	cursor->priority = 0;
> > +	cursor->man = man;
> > +	ttm_lru_item_init(&cursor->hitch, TTM_LRU_HITCH);
> > +	INIT_LIST_HEAD(&cursor->bulk_link);
> > +	INIT_LIST_HEAD(&cursor->hitch.link);
> > +}
> > +
> >   /**
> >    * ttm_resource_cursor_fini() - Finalize the LRU list cursor
> > usage
> >    * @cursor: The struct ttm_resource_cursor to finalize.
> > @@ -593,7 +610,6 @@ ttm_resource_cursor_check_bulk(struct
> > ttm_resource_cursor *cursor,
> >   /**
> >    * ttm_resource_manager_first() - Start iterating over the
> > resources
> >    * of a resource manager
> > - * @man: resource manager to iterate over
> >    * @cursor: cursor to record the position
> >    *
> >    * Initializes the cursor and starts iterating. When done
> > iterating,
> > @@ -602,17 +618,16 @@ ttm_resource_cursor_check_bulk(struct
> > ttm_resource_cursor *cursor,
> >    * Return: The first resource from the resource manager.
> >    */
> >   struct ttm_resource *
> > -ttm_resource_manager_first(struct ttm_resource_manager *man,
> > -			   struct ttm_resource_cursor *cursor)
> > +ttm_resource_manager_first(struct ttm_resource_cursor *cursor)
> >   {
> > -	lockdep_assert_held(&man->bdev->lru_lock);
> > +	struct ttm_resource_manager *man = cursor->man;
> >   
> > -	cursor->priority = 0;
> > -	cursor->man = man;
> > -	ttm_lru_item_init(&cursor->hitch, TTM_LRU_HITCH);
> > -	INIT_LIST_HEAD(&cursor->bulk_link);
> > -	list_add(&cursor->hitch.link, &man->lru[cursor-
> > >priority]);
> > +	if (WARN_ON_ONCE(!man))
> > +		return NULL;
> > +
> > +	lockdep_assert_held(&man->bdev->lru_lock);
> >   
> > +	list_move(&cursor->hitch.link, &man->lru[cursor-
> > >priority]);
> >   	return ttm_resource_manager_next(cursor);
> >   }
> >   
> > @@ -648,8 +663,6 @@ ttm_resource_manager_next(struct
> > ttm_resource_cursor *cursor)
> >   		ttm_resource_cursor_clear_bulk(cursor);
> >   	}
> >   
> > -	ttm_resource_cursor_fini(cursor);
> > -
> >   	return NULL;
> >   }
> >   
> > diff --git a/include/drm/ttm/ttm_resource.h
> > b/include/drm/ttm/ttm_resource.h
> > index be034be56ba1..e1f3b95d73b6 100644
> > --- a/include/drm/ttm/ttm_resource.h
> > +++ b/include/drm/ttm/ttm_resource.h
> > @@ -325,6 +325,9 @@ struct ttm_resource_cursor {
> >   	unsigned int priority;
> >   };
> >   
> > +void ttm_resource_cursor_init(struct ttm_resource_cursor *cursor,
> > +			      struct ttm_resource_manager *man);
> > +
> >   void ttm_resource_cursor_fini(struct ttm_resource_cursor
> > *cursor);
> >   
> >   /**
> > @@ -456,8 +459,7 @@ void ttm_resource_manager_debug(struct
> > ttm_resource_manager *man,
> >   				struct drm_printer *p);
> >   
> >   struct ttm_resource *
> > -ttm_resource_manager_first(struct ttm_resource_manager *man,
> > -			   struct ttm_resource_cursor *cursor);
> > +ttm_resource_manager_first(struct ttm_resource_cursor *cursor);
> >   struct ttm_resource *
> >   ttm_resource_manager_next(struct ttm_resource_cursor *cursor);
> >   
> > @@ -466,14 +468,13 @@ ttm_lru_first_res_or_null(struct list_head
> > *head);
> >   
> >   /**
> >    * ttm_resource_manager_for_each_res - iterate over all resources
> > - * @man: the resource manager
> >    * @cursor: struct ttm_resource_cursor for the current position
> >    * @res: the current resource
> >    *
> >    * Iterate over all the evictable resources in a resource
> > manager.
> >    */
> > -#define ttm_resource_manager_for_each_res(man, cursor,
> > res)		\
> > -	for (res = ttm_resource_manager_first(man, cursor);
> > res;	\
> > +#define ttm_resource_manager_for_each_res(cursor, res)	\
> > +	for (res = ttm_resource_manager_first(cursor); res;	\
> >   	     res = ttm_resource_manager_next(cursor))
> >   
> >   struct ttm_kmap_iter *
>