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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Christian

On Thu, 2024-02-29 at 14:40 +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
> v5: fix bug pointed out by Matthew
> 
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> Reviewed-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx> v3

Now Xe CI passes \o/

Still some checkpatch.pl warnings on both these lines.
For the first line I think it uses From: in the email as the author and
when that doesn't match the SOB, it becomes unhappy.

With that fixed,
Reviewed-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>


> ---
>  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 96a724e8f3ff..e059b1e1b13b 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 && 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);
>  





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux