Re: [PATCH 1/6] drm/ttm: move the LRU into resource handling v4

[Christian König <christian.koenig@xxxxxxx>]

Am 23.03.22 um 11:35 schrieb Daniel Vetter:
> On Mon, Mar 21, 2022 at 02:25:56PM +0100, Christian König wrote:
> > [SNIP]
> >   EXPORT_SYMBOL(ttm_resource_init);
> > @@ -66,15 +172,21 @@ EXPORT_SYMBOL(ttm_resource_init);
> >    * @res: the resource to clean up
> >    *
> >    * Should be used by resource manager backends to clean up the TTM resource
> > - * objects before freeing the underlying structure. Counterpart of
> > - * &ttm_resource_init
> > + * objects before freeing the underlying structure. Makes sure the resource is
> > + * removed from the LRU before destruction.
> > + * Counterpart of &ttm_resource_init.
> ttm_resource_init() or the link wont work correctly. There's a few more
> like this. From earlier patches, but please fix.

Hui what? I've just tried it and the current documentation works fine, 
but when I add the () it doesn't work any more.

> Also in my earlier review I had a bunch more kerneldoc comments that
> arean't addressed here yet:
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2FYfAqtI95aewAb10L%40phenom.ffwll.local%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C43a4517eb6bc4ccda10708da0cb8cbd6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637836285385616052%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=%2BqlN6044DRHaXsYub9pCEVKUu5a0Nfod06lOp%2BaXNP0%3D&reserved=0

What comment exactly do you mean? I thought I've addressed everything 
except for the lockdep checks.

Thanks,
Christian.

> With that addressed Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> still holds.
> -Daniel
>
>
> >    */
> >   void ttm_resource_fini(struct ttm_resource_manager *man,
> >   		       struct ttm_resource *res)
> >   {
> > -	spin_lock(&man->bdev->lru_lock);
> > -	man->usage -= res->bo->base.size;
> > -	spin_unlock(&man->bdev->lru_lock);
> > +	struct ttm_device *bdev = man->bdev;
> > +
> > +	spin_lock(&bdev->lru_lock);
> > +	list_del_init(&res->lru);
> > +	if (res->bo && bdev->funcs->del_from_lru_notify)
> > +		bdev->funcs->del_from_lru_notify(res->bo);
> > +	man->usage -= res->num_pages << PAGE_SHIFT;
> > +	spin_unlock(&bdev->lru_lock);
> >   }
> >   EXPORT_SYMBOL(ttm_resource_fini);
> >   
> > diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> > index c17b2df9178b..3da77fc54552 100644
> > --- a/include/drm/ttm/ttm_bo_api.h
> > +++ b/include/drm/ttm/ttm_bo_api.h
> > @@ -55,8 +55,6 @@ struct ttm_placement;
> >   
> >   struct ttm_place;
> >   
> > -struct ttm_lru_bulk_move;
> > -
> >   /**
> >    * enum ttm_bo_type
> >    *
> > @@ -94,7 +92,6 @@ struct ttm_tt;
> >    * @ttm: TTM structure holding system pages.
> >    * @evicted: Whether the object was evicted without user-space knowing.
> >    * @deleted: True if the object is only a zombie and already deleted.
> > - * @lru: List head for the lru list.
> >    * @ddestroy: List head for the delayed destroy list.
> >    * @swap: List head for swap LRU list.
> >    * @moving: Fence set when BO is moving
> > @@ -143,7 +140,6 @@ struct ttm_buffer_object {
> >   	 * Members protected by the bdev::lru_lock.
> >   	 */
> >   
> > -	struct list_head lru;
> >   	struct list_head ddestroy;
> >   
> >   	/**
> > @@ -295,7 +291,6 @@ void ttm_bo_put(struct ttm_buffer_object *bo);
> >    * ttm_bo_move_to_lru_tail
> >    *
> >    * @bo: The buffer object.
> > - * @mem: Resource object.
> >    * @bulk: optional bulk move structure to remember BO positions
> >    *
> >    * Move this BO to the tail of all lru lists used to lookup and reserve an
> > @@ -303,19 +298,8 @@ void ttm_bo_put(struct ttm_buffer_object *bo);
> >    * held, and is used to make a BO less likely to be considered for eviction.
> >    */
> >   void ttm_bo_move_to_lru_tail(struct ttm_buffer_object *bo,
> > -			     struct ttm_resource *mem,
> >   			     struct ttm_lru_bulk_move *bulk);
> >   
> > -/**
> > - * ttm_bo_bulk_move_lru_tail
> > - *
> > - * @bulk: bulk move structure
> > - *
> > - * Bulk move BOs to the LRU tail, only valid to use when driver makes sure that
> > - * BO order never changes. Should be called with ttm_global::lru_lock held.
> > - */
> > -void ttm_bo_bulk_move_lru_tail(struct ttm_lru_bulk_move *bulk);
> > -
> >   /**
> >    * ttm_bo_lock_delayed_workqueue
> >    *
> > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> > index 5f087575194b..6c7352e13708 100644
> > --- a/include/drm/ttm/ttm_bo_driver.h
> > +++ b/include/drm/ttm/ttm_bo_driver.h
> > @@ -45,33 +45,6 @@
> >   #include "ttm_tt.h"
> >   #include "ttm_pool.h"
> >   
> > -/**
> > - * struct ttm_lru_bulk_move_pos
> > - *
> > - * @first: first BO in the bulk move range
> > - * @last: last BO in the bulk move range
> > - *
> > - * Positions for a lru bulk move.
> > - */
> > -struct ttm_lru_bulk_move_pos {
> > -	struct ttm_buffer_object *first;
> > -	struct ttm_buffer_object *last;
> > -};
> > -
> > -/**
> > - * struct ttm_lru_bulk_move
> > - *
> > - * @tt: first/last lru entry for BOs in the TT domain
> > - * @vram: first/last lru entry for BOs in the VRAM domain
> > - * @swap: first/last lru entry for BOs on the swap list
> > - *
> > - * Helper structure for bulk moves on the LRU list.
> > - */
> > -struct ttm_lru_bulk_move {
> > -	struct ttm_lru_bulk_move_pos tt[TTM_MAX_BO_PRIORITY];
> > -	struct ttm_lru_bulk_move_pos vram[TTM_MAX_BO_PRIORITY];
> > -};
> > -
> >   /*
> >    * ttm_bo.c
> >    */
> > @@ -182,7 +155,7 @@ static inline void
> >   ttm_bo_move_to_lru_tail_unlocked(struct ttm_buffer_object *bo)
> >   {
> >   	spin_lock(&bo->bdev->lru_lock);
> > -	ttm_bo_move_to_lru_tail(bo, bo->resource, NULL);
> > +	ttm_bo_move_to_lru_tail(bo, NULL);
> >   	spin_unlock(&bo->bdev->lru_lock);
> >   }
> >   
> > diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
> > index 323c14a30c6b..181e82e3d806 100644
> > --- a/include/drm/ttm/ttm_resource.h
> > +++ b/include/drm/ttm/ttm_resource.h
> > @@ -26,10 +26,12 @@
> >   #define _TTM_RESOURCE_H_
> >   
> >   #include <linux/types.h>
> > +#include <linux/list.h>
> >   #include <linux/mutex.h>
> >   #include <linux/atomic.h>
> >   #include <linux/dma-buf-map.h>
> >   #include <linux/dma-fence.h>
> > +
> >   #include <drm/drm_print.h>
> >   #include <drm/ttm/ttm_caching.h>
> >   #include <drm/ttm/ttm_kmap_iter.h>
> > @@ -179,6 +181,33 @@ struct ttm_resource {
> >   	uint32_t placement;
> >   	struct ttm_bus_placement bus;
> >   	struct ttm_buffer_object *bo;
> > +	struct list_head lru;
> > +};
> > +
> > +/**
> > + * struct ttm_lru_bulk_move_pos
> > + *
> > + * @first: first res in the bulk move range
> > + * @last: last res in the bulk move range
> > + *
> > + * Positions for a lru bulk move.
> > + */
> > +struct ttm_lru_bulk_move_pos {
> > +	struct ttm_resource *first;
> > +	struct ttm_resource *last;
> > +};
> > +
> > +/**
> > + * struct ttm_lru_bulk_move
> > + *
> > + * @tt: first/last lru entry for resources in the TT domain
> > + * @vram: first/last lru entry for resources in the VRAM domain
> > + *
> > + * Helper structure for bulk moves on the LRU list.
> > + */
> > +struct ttm_lru_bulk_move {
> > +	struct ttm_lru_bulk_move_pos tt[TTM_MAX_BO_PRIORITY];
> > +	struct ttm_lru_bulk_move_pos vram[TTM_MAX_BO_PRIORITY];
> >   };
> >   
> >   /**
> > @@ -267,6 +296,12 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man)
> >   	man->move = NULL;
> >   }
> >   
> > +void ttm_lru_bulk_move_init(struct ttm_lru_bulk_move *bulk);
> > +void ttm_lru_bulk_move_tail(struct ttm_lru_bulk_move *bulk);
> > +
> > +void ttm_resource_move_to_lru_tail(struct ttm_resource *res,
> > +				   struct ttm_lru_bulk_move *bulk);
> > +
> >   void ttm_resource_init(struct ttm_buffer_object *bo,
> >                          const struct ttm_place *place,
> >                          struct ttm_resource *res);
> > --
> > 2.25.1