| Message ID | 20240705153206.68526-9-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TTM LRU-walk cherry-picks | expand |
Hi Thomas,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-xe/drm-xe-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip next-20240703]
[cannot apply to drm-intel/for-linux-next-fixes drm-misc/drm-misc-next linus/master v6.10-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Hellstr-m/drm-ttm-Allow-TTM-LRU-list-nodes-of-different-types/20240706-050447
base: https://gitlab.freedesktop.org/drm/xe/kernel.git drm-xe-next
patch link: https://lore.kernel.org/r/20240705153206.68526-9-thomas.hellstrom%40linux.intel.com
patch subject: [PATCH v7 8/8] drm/ttm: Balance ttm_resource_cursor_init() and ttm_resource_cursor_fini()
config: i386-buildonly-randconfig-001-20240706 (https://download.01.org/0day-ci/archive/20240706/202407061220.4fu8rFR1-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240706/202407061220.4fu8rFR1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407061220.4fu8rFR1-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/ttm/ttm_resource.c:607: warning: Excess function parameter 'man' description in 'ttm_resource_manager_first'
vim +607 drivers/gpu/drm/ttm/ttm_resource.c
60e2bb1468a0ed Thomas Hellström 2024-07-05 593
5d05b988f1c0fd Christian König 2021-06-08 594 /**
c3fc230689e1af Thomas Hellström 2024-07-05 595 * ttm_resource_manager_first() - Start iterating over the resources
c3fc230689e1af Thomas Hellström 2024-07-05 596 * of a resource manager
5d05b988f1c0fd Christian König 2021-06-08 597 * @man: resource manager to iterate over
5d05b988f1c0fd Christian König 2021-06-08 598 * @cursor: cursor to record the position
5d05b988f1c0fd Christian König 2021-06-08 599 *
c3fc230689e1af Thomas Hellström 2024-07-05 600 * Initializes the cursor and starts iterating. When done iterating,
c3fc230689e1af Thomas Hellström 2024-07-05 601 * the caller must explicitly call ttm_resource_cursor_fini().
c3fc230689e1af Thomas Hellström 2024-07-05 602 *
c3fc230689e1af Thomas Hellström 2024-07-05 603 * Return: The first resource from the resource manager.
5d05b988f1c0fd Christian König 2021-06-08 604 */
5d05b988f1c0fd Christian König 2021-06-08 605 struct ttm_resource *
c052d143fd54cf Thomas Hellström 2024-07-05 606 ttm_resource_manager_first(struct ttm_resource_cursor *cursor)
5d05b988f1c0fd Christian König 2021-06-08 @607 {
c052d143fd54cf Thomas Hellström 2024-07-05 608 struct ttm_resource_manager *man = cursor->man;
5d05b988f1c0fd Christian König 2021-06-08 609
c052d143fd54cf Thomas Hellström 2024-07-05 610 if (WARN_ON_ONCE(!man))
c052d143fd54cf Thomas Hellström 2024-07-05 611 return NULL;
c052d143fd54cf Thomas Hellström 2024-07-05 612
c052d143fd54cf Thomas Hellström 2024-07-05 613 lockdep_assert_held(&man->bdev->lru_lock);
c3fc230689e1af Thomas Hellström 2024-07-05 614
c052d143fd54cf Thomas Hellström 2024-07-05 615 list_move(&cursor->hitch.link, &man->lru[cursor->priority]);
1d19e02e4ccfef Thomas Hellström 2024-07-05 616 return ttm_resource_manager_next(cursor);
5d05b988f1c0fd Christian König 2021-06-08 617 }
5d05b988f1c0fd Christian König 2021-06-08 618
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@linux.intel.com> > --- > 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@intel.com> > > #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 >
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); + 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, - 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> #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 *
Make the interface more symmetric by providing and using a ttm_resource_cursor_init(). Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- 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(-)