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

Christian König ckoenig.leichtzumerken at gmail.com
Fri Feb 23 14:30:35 UTC 2024


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);
>



More information about the Intel-gfx mailing list