Re: [PATCH] drm/xe: Simplify pinned bo iteration

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

On Fri, 2025-03-21 at 12:29 +0100, Thomas Hellström wrote:
> Introduce and use a helper to iterate over the various pinned bo
> lists.
> There are a couple of slight functional changes:
> 
> 1) GGTT maps are now performed with the bo locked.
> 2) If the per-bo callback fails, keep the bo on the original list.
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_bo.c       |  10 +-
>  drivers/gpu/drm/xe/xe_bo_evict.c | 209 ++++++++++++-----------------
> --
>  2 files changed, 86 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 64f9c936eea0..070dafdad66b 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2102,12 +2102,10 @@ int xe_bo_pin_external(struct xe_bo *bo)
>  		if (err)
>  			return err;
>  
> -		if (xe_bo_is_vram(bo)) {
> -			spin_lock(&xe->pinned.lock);
> -			list_add_tail(&bo->pinned_link,
> -				      &xe->pinned.external_vram);
> -			spin_unlock(&xe->pinned.lock);
> -		}
> +		spin_lock(&xe->pinned.lock);
> +		list_add_tail(&bo->pinned_link,
> +			      &xe->pinned.external_vram);
> +		spin_unlock(&xe->pinned.lock);
>  	}

The above belongs to a separate patch, pls ignore when reviewing.
/Thomas


>  
>  	ttm_bo_pin(&bo->ttm);
> diff --git a/drivers/gpu/drm/xe/xe_bo_evict.c
> b/drivers/gpu/drm/xe/xe_bo_evict.c
> index 6a40eedd9db1..1eeb3910450b 100644
> --- a/drivers/gpu/drm/xe/xe_bo_evict.c
> +++ b/drivers/gpu/drm/xe/xe_bo_evict.c
> @@ -10,6 +10,44 @@
>  #include "xe_ggtt.h"
>  #include "xe_tile.h"
>  
> +typedef int (*xe_pinned_fn)(struct xe_bo *bo);
> +
> +static int xe_bo_apply_to_pinned(struct xe_device *xe,
> +				 struct list_head *pinned_list,
> +				 struct list_head *new_list,
> +				 const xe_pinned_fn pinned_fn)
> +{
> +	LIST_HEAD(still_in_list);
> +	struct xe_bo *bo;
> +	int ret = 0;
> +
> +	spin_lock(&xe->pinned.lock);
> +	while (!ret) {
> +		bo = list_first_entry_or_null(pinned_list,
> typeof(*bo),
> +					      pinned_link);
> +		if (!bo)
> +			break;
> +		xe_bo_get(bo);
> +		list_move_tail(&bo->pinned_link, &still_in_list);
> +		spin_unlock(&xe->pinned.lock);
> +
> +		xe_bo_lock(bo, false);
> +		ret = pinned_fn(bo);
> +		if (ret && pinned_list != new_list) {
> +			spin_lock(&xe->pinned.lock);
> +			list_move(&bo->pinned_link, pinned_list);
> +			spin_unlock(&xe->pinned.lock);
> +		}
> +		xe_bo_unlock(bo);
> +		xe_bo_put(bo);
> +		spin_lock(&xe->pinned.lock);
> +	}
> +	list_splice_tail(&still_in_list, new_list);
> +	spin_unlock(&xe->pinned.lock);
> +
> +	return ret;
> +}
> +
>  /**
>   * xe_bo_evict_all - evict all BOs from VRAM
>   *
> @@ -27,9 +65,7 @@
>  int xe_bo_evict_all(struct xe_device *xe)
>  {
>  	struct ttm_device *bdev = &xe->ttm;
> -	struct xe_bo *bo;
>  	struct xe_tile *tile;
> -	struct list_head still_in_list;
>  	u32 mem_type;
>  	u8 id;
>  	int ret;
> @@ -57,34 +93,9 @@ int xe_bo_evict_all(struct xe_device *xe)
>  		}
>  	}
>  
> -	/* Pinned user memory in VRAM */
> -	INIT_LIST_HEAD(&still_in_list);
> -	spin_lock(&xe->pinned.lock);
> -	for (;;) {
> -		bo = list_first_entry_or_null(&xe-
> >pinned.external_vram,
> -					      typeof(*bo),
> pinned_link);
> -		if (!bo)
> -			break;
> -		xe_bo_get(bo);
> -		list_move_tail(&bo->pinned_link, &still_in_list);
> -		spin_unlock(&xe->pinned.lock);
> -
> -		xe_bo_lock(bo, false);
> -		ret = xe_bo_evict_pinned(bo);
> -		xe_bo_unlock(bo);
> -		xe_bo_put(bo);
> -		if (ret) {
> -			spin_lock(&xe->pinned.lock);
> -			list_splice_tail(&still_in_list,
> -					 &xe->pinned.external_vram);
> -			spin_unlock(&xe->pinned.lock);
> -			return ret;
> -		}
> -
> -		spin_lock(&xe->pinned.lock);
> -	}
> -	list_splice_tail(&still_in_list, &xe->pinned.external_vram);
> -	spin_unlock(&xe->pinned.lock);
> +	ret = xe_bo_apply_to_pinned(xe, &xe->pinned.external_vram,
> +				    &xe->pinned.external_vram,
> +				    xe_bo_evict_pinned);
>  
>  	/*
>  	 * Wait for all user BO to be evicted as those evictions
> depend on the
> @@ -93,26 +104,42 @@ int xe_bo_evict_all(struct xe_device *xe)
>  	for_each_tile(tile, xe, id)
>  		xe_tile_migrate_wait(tile);
>  
> -	spin_lock(&xe->pinned.lock);
> -	for (;;) {
> -		bo = list_first_entry_or_null(&xe-
> >pinned.kernel_bo_present,
> -					      typeof(*bo),
> pinned_link);
> -		if (!bo)
> -			break;
> -		xe_bo_get(bo);
> -		list_move_tail(&bo->pinned_link, &xe-
> >pinned.evicted);
> -		spin_unlock(&xe->pinned.lock);
> +	if (ret)
> +		return ret;
>  
> -		xe_bo_lock(bo, false);
> -		ret = xe_bo_evict_pinned(bo);
> -		xe_bo_unlock(bo);
> -		xe_bo_put(bo);
> -		if (ret)
> -			return ret;
> +	return xe_bo_apply_to_pinned(xe, &xe-
> >pinned.kernel_bo_present,
> +				     &xe->pinned.evicted,
> +				     xe_bo_evict_pinned);
> +}
>  
> -		spin_lock(&xe->pinned.lock);
> +static int xe_bo_restore_and_map_ggtt(struct xe_bo *bo)
> +{
> +	struct xe_device *xe = xe_bo_device(bo);
> +	int ret;
> +
> +	ret = xe_bo_restore_pinned(bo);
> +	if (ret)
> +		return ret;
> +
> +	if (bo->flags & XE_BO_FLAG_GGTT) {
> +		struct xe_tile *tile;
> +		u8 id;
> +
> +		for_each_tile(tile, xe_bo_device(bo), id) {
> +			if (tile != bo->tile && !(bo->flags &
> XE_BO_FLAG_GGTTx(tile)))
> +				continue;
> +
> +			mutex_lock(&tile->mem.ggtt->lock);
> +			xe_ggtt_map_bo(tile->mem.ggtt, bo);
> +			mutex_unlock(&tile->mem.ggtt->lock);
> +		}
>  	}
> -	spin_unlock(&xe->pinned.lock);
> +
> +	/*
> +	 * We expect validate to trigger a move VRAM and our move
> code
> +	 * should setup the iosys map.
> +	 */
> +	xe_assert(xe, !iosys_map_is_null(&bo->vmap));
>  
>  	return 0;
>  }
> @@ -130,54 +157,9 @@ int xe_bo_evict_all(struct xe_device *xe)
>   */
>  int xe_bo_restore_kernel(struct xe_device *xe)
>  {
> -	struct xe_bo *bo;
> -	int ret;
> -
> -	spin_lock(&xe->pinned.lock);
> -	for (;;) {
> -		bo = list_first_entry_or_null(&xe->pinned.evicted,
> -					      typeof(*bo),
> pinned_link);
> -		if (!bo)
> -			break;
> -		xe_bo_get(bo);
> -		list_move_tail(&bo->pinned_link, &xe-
> >pinned.kernel_bo_present);
> -		spin_unlock(&xe->pinned.lock);
> -
> -		xe_bo_lock(bo, false);
> -		ret = xe_bo_restore_pinned(bo);
> -		xe_bo_unlock(bo);
> -		if (ret) {
> -			xe_bo_put(bo);
> -			return ret;
> -		}
> -
> -		if (bo->flags & XE_BO_FLAG_GGTT) {
> -			struct xe_tile *tile;
> -			u8 id;
> -
> -			for_each_tile(tile, xe, id) {
> -				if (tile != bo->tile && !(bo->flags
> & XE_BO_FLAG_GGTTx(tile)))
> -					continue;
> -
> -				mutex_lock(&tile->mem.ggtt->lock);
> -				xe_ggtt_map_bo(tile->mem.ggtt, bo);
> -				mutex_unlock(&tile->mem.ggtt->lock);
> -			}
> -		}
> -
> -		/*
> -		 * We expect validate to trigger a move VRAM and our
> move code
> -		 * should setup the iosys map.
> -		 */
> -		xe_assert(xe, !iosys_map_is_null(&bo->vmap));
> -
> -		xe_bo_put(bo);
> -
> -		spin_lock(&xe->pinned.lock);
> -	}
> -	spin_unlock(&xe->pinned.lock);
> -
> -	return 0;
> +	return xe_bo_apply_to_pinned(xe, &xe->pinned.evicted,
> +				     &xe->pinned.kernel_bo_present,
> +				     xe_bo_restore_and_map_ggtt);
>  }
>  
>  /**
> @@ -192,47 +174,20 @@ int xe_bo_restore_kernel(struct xe_device *xe)
>   */
>  int xe_bo_restore_user(struct xe_device *xe)
>  {
> -	struct xe_bo *bo;
>  	struct xe_tile *tile;
> -	struct list_head still_in_list;
> -	u8 id;
> -	int ret;
> +	int ret, id;
>  
>  	if (!IS_DGFX(xe))
>  		return 0;
>  
>  	/* Pinned user memory in VRAM should be validated on resume
> */
> -	INIT_LIST_HEAD(&still_in_list);
> -	spin_lock(&xe->pinned.lock);
> -	for (;;) {
> -		bo = list_first_entry_or_null(&xe-
> >pinned.external_vram,
> -					      typeof(*bo),
> pinned_link);
> -		if (!bo)
> -			break;
> -		list_move_tail(&bo->pinned_link, &still_in_list);
> -		xe_bo_get(bo);
> -		spin_unlock(&xe->pinned.lock);
> -
> -		xe_bo_lock(bo, false);
> -		ret = xe_bo_restore_pinned(bo);
> -		xe_bo_unlock(bo);
> -		xe_bo_put(bo);
> -		if (ret) {
> -			spin_lock(&xe->pinned.lock);
> -			list_splice_tail(&still_in_list,
> -					 &xe->pinned.external_vram);
> -			spin_unlock(&xe->pinned.lock);
> -			return ret;
> -		}
> -
> -		spin_lock(&xe->pinned.lock);
> -	}
> -	list_splice_tail(&still_in_list, &xe->pinned.external_vram);
> -	spin_unlock(&xe->pinned.lock);
> +	ret = xe_bo_apply_to_pinned(xe, &xe->pinned.external_vram,
> +				    &xe->pinned.external_vram,
> +				    xe_bo_restore_pinned);
>  
>  	/* Wait for restore to complete */
>  	for_each_tile(tile, xe, id)
>  		xe_tile_migrate_wait(tile);
>  
> -	return 0;
> +	return ret;
>  }