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

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

 



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@xxxxxxx>
> > > > Reviewed-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux