Re: [PATCH 15/15] drm/xe: Convert pinned suspend eviction for exhaustive eviction

[Matthew Auld <matthew.auld@intel.com>]

Hi,

On 13/08/2025 11:51, Thomas Hellström wrote:
> Pinned suspend eviction and preparation for eviction validates
> system memory for eviction buffers. Do that under a
> validation exclusive lock to avoid interfering with other
> processes validating system graphics memory.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>   drivers/gpu/drm/xe/xe_bo.c | 205 +++++++++++++++++++------------------
>   1 file changed, 108 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 82bf158426ad..efb9c88b6aa7 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1139,43 +1139,47 @@ long xe_bo_shrink(struct ttm_operation_ctx *ctx, struct ttm_buffer_object *bo,
>   int xe_bo_notifier_prepare_pinned(struct xe_bo *bo)
>   {
>   	struct xe_device *xe = ttm_to_xe_device(bo->ttm.bdev);
> -	struct drm_exec *exec = XE_VALIDATION_UNIMPLEMENTED;
> +	struct xe_validation_ctx ctx;
> +	struct drm_exec exec;
>   	struct xe_bo *backup;
>   	int ret = 0;
>   
> -	xe_bo_lock(bo, false);
> +	xe_validation_guard(&ctx, &xe->val, &exec, 0, ret, true) {

Ah, this reminded me of 
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4288

Could this help with that? If you could maybe keep the exlusive mode 
turned on for the entire prepare/unprepare stage, to ensure other 
execs/validates back off until we are done?

> +		ret = drm_exec_lock_obj(&exec, &bo->ttm.base);
> +		drm_exec_retry_on_contention(&exec);
> +		xe_assert(xe, !ret);
> +		xe_assert(xe, !bo->backup_obj);
>   
> -	xe_assert(xe, !bo->backup_obj);
> +		/*
> +		 * Since this is called from the PM notifier we might have raced with
> +		 * someone unpinning this after we dropped the pinned list lock and
> +		 * grabbing the above bo lock.
> +		 */
> +		if (!xe_bo_is_pinned(bo))
> +			break;
>   
> -	/*
> -	 * Since this is called from the PM notifier we might have raced with
> -	 * someone unpinning this after we dropped the pinned list lock and
> -	 * grabbing the above bo lock.
> -	 */
> -	if (!xe_bo_is_pinned(bo))
> -		goto out_unlock_bo;
> +		if (!xe_bo_is_vram(bo))
> +			break;
>   
> -	if (!xe_bo_is_vram(bo))
> -		goto out_unlock_bo;
> +		if (bo->flags & XE_BO_FLAG_PINNED_NORESTORE)
> +			break;
>   
> -	if (bo->flags & XE_BO_FLAG_PINNED_NORESTORE)
> -		goto out_unlock_bo;
> +		backup = xe_bo_init_locked(xe, NULL, NULL, bo->ttm.base.resv, NULL, xe_bo_size(bo),
> +					   DRM_XE_GEM_CPU_CACHING_WB, ttm_bo_type_kernel,
> +					   XE_BO_FLAG_SYSTEM | XE_BO_FLAG_NEEDS_CPU_ACCESS |
> +					   XE_BO_FLAG_PINNED, &exec);
> +		if (IS_ERR(backup)) {
> +			drm_exec_retry_on_contention(&exec);
> +			ret = PTR_ERR(backup);
> +			xe_validation_retry_on_oom(&ctx, &ret);
> +			break;
> +		}
>   
> -	backup = xe_bo_init_locked(xe, NULL, NULL, bo->ttm.base.resv, NULL, xe_bo_size(bo),
> -				   DRM_XE_GEM_CPU_CACHING_WB, ttm_bo_type_kernel,
> -				   XE_BO_FLAG_SYSTEM | XE_BO_FLAG_NEEDS_CPU_ACCESS |
> -				   XE_BO_FLAG_PINNED, exec);
> -	if (IS_ERR(backup)) {
> -		ret = PTR_ERR(backup);
> -		goto out_unlock_bo;
> +		backup->parent_obj = xe_bo_get(bo); /* Released by bo_destroy */
> +		ttm_bo_pin(&backup->ttm);
> +		bo->backup_obj = backup;
>   	}
>   
> -	backup->parent_obj = xe_bo_get(bo); /* Released by bo_destroy */
> -	ttm_bo_pin(&backup->ttm);
> -	bo->backup_obj = backup;
> -
> -out_unlock_bo:
> -	xe_bo_unlock(bo);
>   	return ret;
>   }
>   
> @@ -1215,99 +1219,106 @@ int xe_bo_notifier_unprepare_pinned(struct xe_bo *bo)
>   int xe_bo_evict_pinned(struct xe_bo *bo)
>   {
>   	struct xe_device *xe = ttm_to_xe_device(bo->ttm.bdev);
> -	struct drm_exec *exec = XE_VALIDATION_UNIMPLEMENTED;
> +	struct xe_validation_ctx ctx;
> +	struct drm_exec exec;
>   	struct xe_bo *backup = bo->backup_obj;
>   	bool backup_created = false;
>   	bool unmap = false;
>   	int ret = 0;
>   
> -	xe_bo_lock(bo, false);
> +	xe_validation_guard(&ctx, &xe->val, &exec, 0, ret, true) {
> +		ret = drm_exec_lock_obj(&exec, &bo->ttm.base);
> +		drm_exec_retry_on_contention(&exec);
> +		xe_assert(xe, !ret);
>   
> -	if (WARN_ON(!bo->ttm.resource)) {
> -		ret = -EINVAL;
> -		goto out_unlock_bo;
> -	}
> +		if (WARN_ON(!bo->ttm.resource)) {
> +			ret = -EINVAL;
> +			break;
> +		}
>   
> -	if (WARN_ON(!xe_bo_is_pinned(bo))) {
> -		ret = -EINVAL;
> -		goto out_unlock_bo;
> -	}
> +		if (WARN_ON(!xe_bo_is_pinned(bo))) {
> +			ret = -EINVAL;
> +			break;
> +		}
>   
> -	if (!xe_bo_is_vram(bo))
> -		goto out_unlock_bo;
> +		if (!xe_bo_is_vram(bo))
> +			break;
>   
> -	if (bo->flags & XE_BO_FLAG_PINNED_NORESTORE)
> -		goto out_unlock_bo;
> +		if (bo->flags & XE_BO_FLAG_PINNED_NORESTORE)
> +			break;
>   
> -	if (!backup) {
> -		backup = xe_bo_init_locked(xe, NULL, NULL, bo->ttm.base.resv, NULL, xe_bo_size(bo),
> -					   DRM_XE_GEM_CPU_CACHING_WB, ttm_bo_type_kernel,
> -					   XE_BO_FLAG_SYSTEM | XE_BO_FLAG_NEEDS_CPU_ACCESS |
> -					   XE_BO_FLAG_PINNED, exec);
> -		if (IS_ERR(backup)) {
> -			ret = PTR_ERR(backup);
> -			goto out_unlock_bo;
> +		if (!backup) {
> +			backup = xe_bo_init_locked(xe, NULL, NULL, bo->ttm.base.resv, NULL,
> +						   xe_bo_size(bo),
> +						   DRM_XE_GEM_CPU_CACHING_WB, ttm_bo_type_kernel,
> +						   XE_BO_FLAG_SYSTEM | XE_BO_FLAG_NEEDS_CPU_ACCESS |
> +						   XE_BO_FLAG_PINNED, &exec);
> +			if (IS_ERR(backup)) {
> +				drm_exec_retry_on_contention(&exec);
> +				ret = PTR_ERR(backup);
> +				xe_validation_retry_on_oom(&ctx, &ret);
> +				break;
> +			}
> +			backup->parent_obj = xe_bo_get(bo); /* Released by bo_destroy */
> +			backup_created = true;
>   		}
> -		backup->parent_obj = xe_bo_get(bo); /* Released by bo_destroy */
> -		backup_created = true;
> -	}
>   
> -	if (xe_bo_is_user(bo) || (bo->flags & XE_BO_FLAG_PINNED_LATE_RESTORE)) {
> -		struct xe_migrate *migrate;
> -		struct dma_fence *fence;
> -
> -		if (bo->tile)
> -			migrate = bo->tile->migrate;
> -		else
> -			migrate = mem_type_to_migrate(xe, bo->ttm.resource->mem_type);
> +		if (xe_bo_is_user(bo) || (bo->flags & XE_BO_FLAG_PINNED_LATE_RESTORE)) {
> +			struct xe_migrate *migrate;
> +			struct dma_fence *fence;
>   
> -		ret = dma_resv_reserve_fences(bo->ttm.base.resv, 1);
> -		if (ret)
> -			goto out_backup;
> +			if (bo->tile)
> +				migrate = bo->tile->migrate;
> +			else
> +				migrate = mem_type_to_migrate(xe, bo->ttm.resource->mem_type);
>   
> -		ret = dma_resv_reserve_fences(backup->ttm.base.resv, 1);
> -		if (ret)
> -			goto out_backup;
> +			ret = dma_resv_reserve_fences(bo->ttm.base.resv, 1);
> +			if (ret)
> +				goto out_backup;
>   
> -		fence = xe_migrate_copy(migrate, bo, backup, bo->ttm.resource,
> -					backup->ttm.resource, false);
> -		if (IS_ERR(fence)) {
> -			ret = PTR_ERR(fence);
> -			goto out_backup;
> -		}
> +			ret = dma_resv_reserve_fences(backup->ttm.base.resv, 1);
> +			if (ret)
> +				goto out_backup;
>   
> -		dma_resv_add_fence(bo->ttm.base.resv, fence,
> -				   DMA_RESV_USAGE_KERNEL);
> -		dma_resv_add_fence(backup->ttm.base.resv, fence,
> -				   DMA_RESV_USAGE_KERNEL);
> -		dma_fence_put(fence);
> -	} else {
> -		ret = xe_bo_vmap(backup);
> -		if (ret)
> -			goto out_backup;
> +			fence = xe_migrate_copy(migrate, bo, backup, bo->ttm.resource,
> +						backup->ttm.resource, false);
> +			if (IS_ERR(fence)) {
> +				ret = PTR_ERR(fence);
> +				goto out_backup;
> +			}
>   
> -		if (iosys_map_is_null(&bo->vmap)) {
> -			ret = xe_bo_vmap(bo);
> +			dma_resv_add_fence(bo->ttm.base.resv, fence,
> +					   DMA_RESV_USAGE_KERNEL);
> +			dma_resv_add_fence(backup->ttm.base.resv, fence,
> +					   DMA_RESV_USAGE_KERNEL);
> +			dma_fence_put(fence);
> +		} else {
> +			ret = xe_bo_vmap(backup);
>   			if (ret)
>   				goto out_backup;
> -			unmap = true;
> -		}
>   
> -		xe_map_memcpy_from(xe, backup->vmap.vaddr, &bo->vmap, 0,
> -				   xe_bo_size(bo));
> -	}
> +			if (iosys_map_is_null(&bo->vmap)) {
> +				ret = xe_bo_vmap(bo);
> +				if (ret)
> +					goto out_vunmap;
> +				unmap = true;
> +			}
>   
> -	if (!bo->backup_obj)
> -		bo->backup_obj = backup;
> +			xe_map_memcpy_from(xe, backup->vmap.vaddr, &bo->vmap, 0,
> +					   xe_bo_size(bo));
> +		}
>   
> +		if (!bo->backup_obj)
> +			bo->backup_obj = backup;
> +out_vunmap:
> +		xe_bo_vunmap(backup);
>   out_backup:
> -	xe_bo_vunmap(backup);
> -	if (ret && backup_created)
> -		xe_bo_put(backup);
> -out_unlock_bo:
> -	if (unmap)
> -		xe_bo_vunmap(bo);
> -	xe_bo_unlock(bo);
> +		if (ret && backup_created)
> +			xe_bo_put(backup);
> +		if (unmap)
> +			xe_bo_vunmap(bo);
> +	}
> +
>   	return ret;
>   }
>