On Fri, Jul 05, 2024 at 05:32:06PM +0200, Thomas Hellström wrote:
> Make the interface more symmetric by providing and using a
> ttm_resource_cursor_init().
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 3 ++-
> drivers/gpu/drm/ttm/ttm_bo_util.c | 4 +++-
> drivers/gpu/drm/ttm/ttm_resource.c | 34 +++++++++++++++++++++---------
> include/drm/ttm/ttm_resource.h | 12 ++++++-----
> 4 files changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 0131ec802066..7fcd9cb0478e 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -449,7 +449,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 3c07f4712d5c..ec6a0482cd94 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;
> @@ -906,6 +907,7 @@ s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev,
>
> ttm_lru_walk_unlock(bo, bo_needs_unlock);
> ttm_bo_put(bo);
> +
Nit: Extra new line.
> if (lret == -EBUSY || lret == -EALREADY)
> lret = 0;
> progress = (lret < 0) ? lret : progress + lret;
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 6d764ba88aab..b300d615e196 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -80,6 +80,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.
> @@ -586,17 +603,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,
The kernel doc here needs to be updated.
> - 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);
> }
>
> @@ -632,8 +648,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..ee0e652328b3 100644
> --- a/include/drm/ttm/ttm_resource.h
> +++ b/include/drm/ttm/ttm_resource.h
> @@ -30,6 +30,7 @@
> #include <linux/mutex.h>
> #include <linux/iosys-map.h>
> #include <linux/dma-fence.h>
> +#include <linux/cleanup.h>
I don't think this needs to be included.
With the above nits fixed:
Reviewed-by: Matthew Brost <matthew.brost@xxxxxxxxx>
>
> #include <drm/drm_print.h>
> #include <drm/ttm/ttm_caching.h>
> @@ -325,6 +326,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 +460,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 +469,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 *
> --
> 2.44.0
>
