Am 06.02.24 um 13:56 schrieb Christian K?nig:> 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 :(
So I've digged myself out of that hole and would rather like to get this
new feature into 6.9.
Any time to review it? I can also plan some time to review your LRU
changes next week.
Thanks,
Christian.
>
> 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);
>