[PATCH 1/2] drm/ttm: improve idle/busy handling v4

Christian König ckoenig.leichtzumerken at gmail.com
Tue Feb 6 12:56:27 UTC 2024


Am 06.02.24 um 13:53 schrieb Thomas Hellström:
> Hi, Christian,
>
> On Fri, 2024-01-26 at 15:09 +0100, Christian König wrote:
>> Previously we would never try to move a BO into the preferred
>> placements
>> when it ever landed in a busy placement since those were considered
>> compatible.
>>
>> Rework the whole handling and finally unify the idle and busy
>> handling.
>> ttm_bo_validate() is now responsible to try idle placement first and
>> then
>> use the busy placement if that didn't worked.
>>
>> Drawback is that we now always try the idle placement first for each
>> validation which might cause some additional CPU overhead on
>> overcommit.
>>
>> v2: fix kerneldoc warning and coding style
>> v3: take care of XE as well
>> v4: keep the ttm_bo_mem_space functionality as it is for now, only
>> add
>>      new handling for ttm_bo_validate as suggested by Thomas
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> Reviewed-by: Zack Rusin <zack.rusin at broadcom.com> v3
> Sending this through xe CI, will try to review asap.

Take your time. At the moment people are bombarding me with work and I 
have only two hands and one head as well :(

Christian.

>
> /Thomas
>
>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c       | 231 +++++++++++++--------------
>> --
>>   drivers/gpu/drm/ttm/ttm_resource.c |  16 +-
>>   include/drm/ttm/ttm_resource.h     |   3 +-
>>   3 files changed, 121 insertions(+), 129 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
>> b/drivers/gpu/drm/ttm/ttm_bo.c
>> index ba3f09e2d7e6..b12f435542a9 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -724,64 +724,36 @@ static int ttm_bo_add_move_fence(struct
>> ttm_buffer_object *bo,
>>   	return ret;
>>   }
>>   
>> -/*
>> - * Repeatedly evict memory from the LRU for @mem_type until we
>> create enough
>> - * space, or we've evicted everything and there isn't enough space.
>> - */
>> -static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>> -				  const struct ttm_place *place,
>> -				  struct ttm_resource **mem,
>> -				  struct ttm_operation_ctx *ctx)
>> -{
>> -	struct ttm_device *bdev = bo->bdev;
>> -	struct ttm_resource_manager *man;
>> -	struct ww_acquire_ctx *ticket;
>> -	int ret;
>> -
>> -	man = ttm_manager_type(bdev, place->mem_type);
>> -	ticket = dma_resv_locking_ctx(bo->base.resv);
>> -	do {
>> -		ret = ttm_resource_alloc(bo, place, mem);
>> -		if (likely(!ret))
>> -			break;
>> -		if (unlikely(ret != -ENOSPC))
>> -			return ret;
>> -		ret = ttm_mem_evict_first(bdev, man, place, ctx,
>> -					  ticket);
>> -		if (unlikely(ret != 0))
>> -			return ret;
>> -	} while (1);
>> -
>> -	return ttm_bo_add_move_fence(bo, man, *mem, ctx-
>>> no_wait_gpu);
>> -}
>> -
>>   /**
>> - * ttm_bo_mem_space
>> + * ttm_bo_alloc_resource - Allocate backing store for a BO
>>    *
>> - * @bo: Pointer to a struct ttm_buffer_object. the data of which
>> - * we want to allocate space for.
>> - * @placement: Proposed new placement for the buffer object.
>> - * @mem: A struct ttm_resource.
>> + * @bo: Pointer to a struct ttm_buffer_object of which we want a
>> resource for
>> + * @placement: Proposed new placement for the buffer object
>>    * @ctx: if and how to sleep, lock buffers and alloc memory
>> + * @force_space: If we should evict buffers to force space
>> + * @res: The resulting struct ttm_resource.
>>    *
>> - * Allocate memory space for the buffer object pointed to by @bo,
>> using
>> - * the placement flags in @placement, potentially evicting other
>> idle buffer objects.
>> - * This function may sleep while waiting for space to become
>> available.
>> + * Allocates a resource for the buffer object pointed to by @bo,
>> using the
>> + * placement flags in @placement, potentially evicting other buffer
>> objects when
>> + * @force_space is true.
>> + * This function may sleep while waiting for resources to become
>> available.
>>    * Returns:
>> - * -EBUSY: No space available (only if no_wait == 1).
>> + * -EBUSY: No space available (only if no_wait == true).
>>    * -ENOSPC: Could not allocate space for the buffer object, either
>> due to
>>    * fragmentation or concurrent allocators.
>>    * -ERESTARTSYS: An interruptible sleep was interrupted by a signal.
>>    */
>> -int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>> -			struct ttm_placement *placement,
>> -			struct ttm_resource **mem,
>> -			struct ttm_operation_ctx *ctx)
>> +static int ttm_bo_alloc_resource(struct ttm_buffer_object *bo,
>> +				 struct ttm_placement *placement,
>> +				 struct ttm_operation_ctx *ctx,
>> +				 bool force_space,
>> +				 struct ttm_resource **res)
>>   {
>>   	struct ttm_device *bdev = bo->bdev;
>> -	bool type_found = false;
>> +	struct ww_acquire_ctx *ticket;
>>   	int i, ret;
>>   
>> +	ticket = dma_resv_locking_ctx(bo->base.resv);
>>   	ret = dma_resv_reserve_fences(bo->base.resv, 1);
>>   	if (unlikely(ret))
>>   		return ret;
>> @@ -790,98 +762,73 @@ int ttm_bo_mem_space(struct ttm_buffer_object
>> *bo,
>>   		const struct ttm_place *place = &placement-
>>> placement[i];
>>   		struct ttm_resource_manager *man;
>>   
>> -		if (place->flags & TTM_PL_FLAG_FALLBACK)
>> -			continue;
>> -
>>   		man = ttm_manager_type(bdev, place->mem_type);
>>   		if (!man || !ttm_resource_manager_used(man))
>>   			continue;
>>   
>> -		type_found = true;
>> -		ret = ttm_resource_alloc(bo, place, mem);
>> -		if (ret == -ENOSPC)
>> +		if (place->flags & (force_space ?
>> TTM_PL_FLAG_DESIRED :
>> +				    TTM_PL_FLAG_FALLBACK))
>> +			continue;
>> +
>> +		do {
>> +			ret = ttm_resource_alloc(bo, place, res);
>> +			if (unlikely(ret != -ENOSPC))
>> +				return ret;
>> +			if (likely(!ret) || !force_space)
>> +				break;
>> +
>> +			ret = ttm_mem_evict_first(bdev, man, place,
>> ctx,
>> +						  ticket);
>> +			if (unlikely(ret == -EBUSY))
>> +				break;
>> +			if (unlikely(ret))
>> +				return ret;
>> +		} while (1);
>> +		if (ret)
>>   			continue;
>> -		if (unlikely(ret))
>> -			goto error;
>>   
>> -		ret = ttm_bo_add_move_fence(bo, man, *mem, ctx-
>>> no_wait_gpu);
>> +		ret = ttm_bo_add_move_fence(bo, man, *res, ctx-
>>> no_wait_gpu);
>>   		if (unlikely(ret)) {
>> -			ttm_resource_free(bo, mem);
>> +			ttm_resource_free(bo, res);
>>   			if (ret == -EBUSY)
>>   				continue;
>>   
>> -			goto error;
>> +			return ret;
>>   		}
>>   		return 0;
>>   	}
>>   
>> -	for (i = 0; i < placement->num_placement; ++i) {
>> -		const struct ttm_place *place = &placement-
>>> placement[i];
>> -		struct ttm_resource_manager *man;
>> -
>> -		if (place->flags & TTM_PL_FLAG_DESIRED)
>> -			continue;
>> -
>> -		man = ttm_manager_type(bdev, place->mem_type);
>> -		if (!man || !ttm_resource_manager_used(man))
>> -			continue;
>> -
>> -		type_found = true;
>> -		ret = ttm_bo_mem_force_space(bo, place, mem, ctx);
>> -		if (likely(!ret))
>> -			return 0;
>> -
>> -		if (ret && ret != -EBUSY)
>> -			goto error;
>> -	}
>> -
>> -	ret = -ENOSPC;
>> -	if (!type_found) {
>> -		pr_err(TTM_PFX "No compatible memory type found\n");
>> -		ret = -EINVAL;
>> -	}
>> -
>> -error:
>> -	return ret;
>> +	return -ENOSPC;
>>   }
>> -EXPORT_SYMBOL(ttm_bo_mem_space);
>>   
>> -static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
>> -			      struct ttm_placement *placement,
>> -			      struct ttm_operation_ctx *ctx)
>> +/*
>> + * ttm_bo_mem_space - Wrapper around ttm_bo_alloc_resource
>> + *
>> + * @bo: Pointer to a struct ttm_buffer_object of which we want a
>> resource for
>> + * @placement: Proposed new placement for the buffer object
>> + * @res: The resulting struct ttm_resource.
>> + * @ctx: if and how to sleep, lock buffers and alloc memory
>> + *
>> + * Tries both idle allocation and forcefully eviction of buffers.
>> See
>> + * ttm_bo_alloc_resource for details.
>> + */
>> +int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>> +		     struct ttm_placement *placement,
>> +		     struct ttm_resource **res,
>> +		     struct ttm_operation_ctx *ctx)
>>   {
>> -	struct ttm_resource *mem;
>> -	struct ttm_place hop;
>> +	bool force_space = false;
>>   	int ret;
>>   
>> -	dma_resv_assert_held(bo->base.resv);
>> +	do {
>> +		ret = ttm_bo_alloc_resource(bo, placement, ctx,
>> +					    force_space, res);
>> +		force_space = !force_space;
>> +	} while (ret == -ENOSPC && force_space);
>>   
>> -	/*
>> -	 * Determine where to move the buffer.
>> -	 *
>> -	 * If driver determines move is going to need
>> -	 * an extra step then it will return -EMULTIHOP
>> -	 * and the buffer will be moved to the temporary
>> -	 * stop and the driver will be called to make
>> -	 * the second hop.
>> -	 */
>> -	ret = ttm_bo_mem_space(bo, placement, &mem, ctx);
>> -	if (ret)
>> -		return ret;
>> -bounce:
>> -	ret = ttm_bo_handle_move_mem(bo, mem, false, ctx, &hop);
>> -	if (ret == -EMULTIHOP) {
>> -		ret = ttm_bo_bounce_temp_buffer(bo, &mem, ctx,
>> &hop);
>> -		if (ret)
>> -			goto out;
>> -		/* try and move to final place now. */
>> -		goto bounce;
>> -	}
>> -out:
>> -	if (ret)
>> -		ttm_resource_free(bo, &mem);
>>   	return ret;
>>   }
>> +EXPORT_SYMBOL(ttm_bo_mem_space);
>>   
>>   /**
>>    * ttm_bo_validate
>> @@ -902,6 +849,9 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
>>   		    struct ttm_placement *placement,
>>   		    struct ttm_operation_ctx *ctx)
>>   {
>> +	struct ttm_resource *res;
>> +	struct ttm_place hop;
>> +	bool force_space;
>>   	int ret;
>>   
>>   	dma_resv_assert_held(bo->base.resv);
>> @@ -912,20 +862,53 @@ int ttm_bo_validate(struct ttm_buffer_object
>> *bo,
>>   	if (!placement->num_placement)
>>   		return ttm_bo_pipeline_gutting(bo);
>>   
>> -	/* Check whether we need to move buffer. */
>> -	if (bo->resource && ttm_resource_compatible(bo->resource,
>> placement))
>> -		return 0;
>> +	force_space = false;
>> +	do {
>> +		/* Check whether we need to move buffer. */
>> +		if (bo->resource &&
>> +		    ttm_resource_compatible(bo->resource, placement,
>> +					    force_space))
>> +			return 0;
>>   
>> -	/* Moving of pinned BOs is forbidden */
>> -	if (bo->pin_count)
>> -		return -EINVAL;
>> +		/* Moving of pinned BOs is forbidden */
>> +		if (bo->pin_count)
>> +			return -EINVAL;
>> +
>> +		/*
>> +		 * Determine where to move the buffer.
>> +		 *
>> +		 * If driver determines move is going to need
>> +		 * an extra step then it will return -EMULTIHOP
>> +		 * and the buffer will be moved to the temporary
>> +		 * stop and the driver will be called to make
>> +		 * the second hop.
>> +		 */
>> +		ret = ttm_bo_alloc_resource(bo, placement, ctx,
>> force_space,
>> +					    &res);
>> +		force_space = !force_space;
>> +		if (ret == -ENOSPC)
>> +			continue;
>> +		if (ret)
>> +			return ret;
>> +
>> +bounce:
>> +		ret = ttm_bo_handle_move_mem(bo, res, false, ctx,
>> &hop);
>> +		if (ret == -EMULTIHOP) {
>> +			ret = ttm_bo_bounce_temp_buffer(bo, &res,
>> ctx, &hop);
>> +			/* try and move to final place now. */
>> +			if (!ret)
>> +				goto bounce;
>> +		}
>> +		if (ret) {
>> +			ttm_resource_free(bo, &res);
>> +			return ret;
>> +		}
>> +
>> +	} while (ret && force_space);
>>   
>> -	ret = ttm_bo_move_buffer(bo, placement, ctx);
>>   	/* For backward compatibility with userspace */
>>   	if (ret == -ENOSPC)
>>   		return -ENOMEM;
>> -	if (ret)
>> -		return ret;
>>   
>>   	/*
>>   	 * We might need to add a TTM.
>> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
>> b/drivers/gpu/drm/ttm/ttm_resource.c
>> index fb14f7716cf8..65155f2013ca 100644
>> --- a/drivers/gpu/drm/ttm/ttm_resource.c
>> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
>> @@ -295,11 +295,13 @@ bool ttm_resource_intersects(struct ttm_device
>> *bdev,
>>    *
>>    * @res: the resource to check
>>    * @placement: the placement to check against
>> + * @evicting: true if the caller is doing evictions
>>    *
>>    * Returns true if the placement is compatible.
>>    */
>>   bool ttm_resource_compatible(struct ttm_resource *res,
>> -			     struct ttm_placement *placement)
>> +			     struct ttm_placement *placement,
>> +			     bool evicting)
>>   {
>>   	struct ttm_buffer_object *bo = res->bo;
>>   	struct ttm_device *bdev = bo->bdev;
>> @@ -315,14 +317,20 @@ bool ttm_resource_compatible(struct
>> ttm_resource *res,
>>   		if (res->mem_type != place->mem_type)
>>   			continue;
>>   
>> +		if (place->flags & (evicting ? TTM_PL_FLAG_DESIRED :
>> +				    TTM_PL_FLAG_FALLBACK))
>> +			continue;
>> +
>> +		if (place->flags & TTM_PL_FLAG_CONTIGUOUS &&
>> +		    !(res->placement & TTM_PL_FLAG_CONTIGUOUS))
>> +			continue;
>> +
>>   		man = ttm_manager_type(bdev, res->mem_type);
>>   		if (man->func->compatible &&
>>   		    !man->func->compatible(man, res, place, bo-
>>> base.size))
>>   			continue;
>>   
>> -		if ((!(place->flags & TTM_PL_FLAG_CONTIGUOUS) ||
>> -		     (res->placement & TTM_PL_FLAG_CONTIGUOUS)))
>> -			return true;
>> +		return true;
>>   	}
>>   	return false;
>>   }
>> diff --git a/include/drm/ttm/ttm_resource.h
>> b/include/drm/ttm/ttm_resource.h
>> index 1afa13f0c22b..7561023db43d 100644
>> --- a/include/drm/ttm/ttm_resource.h
>> +++ b/include/drm/ttm/ttm_resource.h
>> @@ -366,7 +366,8 @@ bool ttm_resource_intersects(struct ttm_device
>> *bdev,
>>   			     const struct ttm_place *place,
>>   			     size_t size);
>>   bool ttm_resource_compatible(struct ttm_resource *res,
>> -			     struct ttm_placement *placement);
>> +			     struct ttm_placement *placement,
>> +			     bool evicting);
>>   void ttm_resource_set_bo(struct ttm_resource *res,
>>   			 struct ttm_buffer_object *bo);
>>   



More information about the Intel-gfx mailing list