Hi, Christian
On Fri, 2024-02-23 at 15:30 +0100, Christian K?nig
wrote:> 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.
Sorry for the late response. Was planning to review but saw that there
was still an xe CI failure.
https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-129579v1/bat-atsm-2/igt at
xe_evict_ccs@evict-overcommit-parallel-nofree-samefd.html
I haven't really had time to look into what might be causing this,
though.
/Thomas
>
> >
> > 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);
> >
>