[PATCH 1/2] drm/ttm: improve idle/busy handling v4
Thomas Hellström
thomas.hellstrom at linux.intel.com
Mon Feb 26 20:21:42 UTC 2024
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@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);
> >
>
More information about the dri-devel
mailing list