Re: [Intel-gfx] [PATCH v6 1/6] drm/ttm: Add new callbacks to ttm res mgr

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 07, 2022 at 08:45:22AM +0200, Christian König wrote:
> Am 06.09.22 um 21:58 schrieb Daniel Vetter:
> > On Tue, Aug 16, 2022 at 10:33:16AM +0530, Arunpravin Paneer Selvam wrote:
> > > 
> > > On 8/15/2022 4:35 PM, Christian König wrote:
> > > > Am 12.08.22 um 15:30 schrieb Arunpravin Paneer Selvam:
> > > > > We are adding two new callbacks to ttm resource manager
> > > > > function to handle intersection and compatibility of
> > > > > placement and resources.
> > > > > 
> > > > > v2: move the amdgpu and ttm_range_manager changes to
> > > > >       separate patches (Christian)
> > > > > v3: rename "intersect" to "intersects" (Matthew)
> > > > > v4: move !place check to the !res if and return false
> > > > >       in ttm_resource_compatible() function (Christian)
> > > > > v5: move bits of code from patch number 6 to avoid
> > > > >       temporary driver breakup (Christian)
> > > > > 
> > > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > > Signed-off-by: Arunpravin Paneer Selvam
> > > > > <Arunpravin.PaneerSelvam@amd.com>
> > > > Patch #6 could still be cleaned up more now that we have the workaround
> > > > code in patch #1, but that not really a must have.
> > > > 
> > > > Reviewed-by: Christian König <christian.koenig@amd.com> for the entire
> > > > series.
> > > > 
> > > > Do you already have commit rights?
> > > Hi Christian,
> > > I applied for drm-misc commit rights, waiting for the project maintainers to
> > > approve my request.
> > Why do all drivers have to implement the current behaviour? Can we have a
> > default implementation, which gets called if nothing is set instead?
> 
> We do have a default implementation in the range manager which is used by
> radeon, GEM VRAM helpers, VMWGFX and amdgpu (but there only for some
> domains).
> 
> > I'm a bit confused why the bloat here ...
> 
> Drivers do have specialized implementations of the backend, e.g. VMWGFX have
> his handle backend, amdgpu the VRAM backend with special placements, i915 is
> completely special as well.
> 
> Here we only move the decision if resources intersect or are compatible into
> those specialized backends. Previously we had all this in a centralized
> callback for all backends of a driver.
> 
> See the switch in amdgpu_ttm_bo_eviction_valuable() for an example. Final
> goal is to move all this stuff into the specialized backends and remove this
> callback.
> 
> The only driver where I couldn't figure out why we have duplicated all this
> from the standard implementation is Nouveau.

Yeah I didn't read this too carefully, apologies.

> > Also please document new callbacks precisely with inline kerneldoc. I know
> > ttm docs aren't great yet, but they don't get better if we don't start
> > somewhere. I think the in-depth comments for modeset vfuncs (e.g. in
> > drm_modeset_helper_vtables.h) are a good standard here.
> 
> I thought we already did that. Please be a bit more specific.

Yeah rushed this too, but the kerneldoc isn't too great yet. It's
definitely not formatted correctly (you can't do a full function
definition in a struct unfortunately, see the examples I linked). And it
would be good to specificy what the default implementation is, that there
is one (i.e. the hook is optional) and when exactly a driver would want to
overwrite this. Atm it's a one-liner that explains exactly as much as you
can guess from the function interface anyway, that's not super userful.
-Daniel



> 
> Thanks,
> Christian.
> 
> > -Daniel
> > 
> > > Thanks,
> > > Arun
> > > > Regards,
> > > > Christian.
> > > > 
> > > > > ---
> > > > >    drivers/gpu/drm/ttm/ttm_bo.c       |  9 ++--
> > > > >    drivers/gpu/drm/ttm/ttm_resource.c | 77 +++++++++++++++++++++++++++++-
> > > > >    include/drm/ttm/ttm_resource.h     | 40 ++++++++++++++++
> > > > >    3 files changed, 119 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > > > > index c1bd006a5525..f066e8124c50 100644
> > > > > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > > > > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > > > > @@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object
> > > > > *bo,
> > > > >    bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
> > > > >                      const struct ttm_place *place)
> > > > >    {
> > > > > +    struct ttm_resource *res = bo->resource;
> > > > > +    struct ttm_device *bdev = bo->bdev;
> > > > > +
> > > > >        dma_resv_assert_held(bo->base.resv);
> > > > >        if (bo->resource->mem_type == TTM_PL_SYSTEM)
> > > > >            return true;
> > > > > @@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct
> > > > > ttm_buffer_object *bo,
> > > > >        /* Don't evict this BO if it's outside of the
> > > > >         * requested placement range
> > > > >         */
> > > > > -    if (place->fpfn >= (bo->resource->start +
> > > > > bo->resource->num_pages) ||
> > > > > -        (place->lpfn && place->lpfn <= bo->resource->start))
> > > > > -        return false;
> > > > > -
> > > > > -    return true;
> > > > > +    return ttm_resource_intersects(bdev, res, place, bo->base.size);
> > > > >    }
> > > > >    EXPORT_SYMBOL(ttm_bo_eviction_valuable);
> > > > >    diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
> > > > > b/drivers/gpu/drm/ttm/ttm_resource.c
> > > > > index 20f9adcc3235..0d1f862a582b 100644
> > > > > --- a/drivers/gpu/drm/ttm/ttm_resource.c
> > > > > +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> > > > > @@ -253,10 +253,84 @@ void ttm_resource_free(struct
> > > > > ttm_buffer_object *bo, struct ttm_resource **res)
> > > > >    }
> > > > >    EXPORT_SYMBOL(ttm_resource_free);
> > > > >    +/**
> > > > > + * ttm_resource_intersects - test for intersection
> > > > > + *
> > > > > + * @bdev: TTM device structure
> > > > > + * @res: The resource to test
> > > > > + * @place: The placement to test
> > > > > + * @size: How many bytes the new allocation needs.
> > > > > + *
> > > > > + * Test if @res intersects with @place and @size. Used for testing
> > > > > if evictions
> > > > > + * are valueable or not.
> > > > > + *
> > > > > + * Returns true if the res placement intersects with @place and @size.
> > > > > + */
> > > > > +bool ttm_resource_intersects(struct ttm_device *bdev,
> > > > > +                 struct ttm_resource *res,
> > > > > +                 const struct ttm_place *place,
> > > > > +                 size_t size)
> > > > > +{
> > > > > +    struct ttm_resource_manager *man;
> > > > > +
> > > > > +    if (!res)
> > > > > +        return false;
> > > > > +
> > > > > +    if (!place)
> > > > > +        return true;
> > > > > +
> > > > > +    man = ttm_manager_type(bdev, res->mem_type);
> > > > > +    if (!man->func->intersects) {
> > > > > +        if (place->fpfn >= (res->start + res->num_pages) ||
> > > > > +            (place->lpfn && place->lpfn <= res->start))
> > > > > +            return false;
> > > > > +
> > > > > +        return true;
> > > > > +    }
> > > > > +
> > > > > +    return man->func->intersects(man, res, place, size);
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * ttm_resource_compatible - test for compatibility
> > > > > + *
> > > > > + * @bdev: TTM device structure
> > > > > + * @res: The resource to test
> > > > > + * @place: The placement to test
> > > > > + * @size: How many bytes the new allocation needs.
> > > > > + *
> > > > > + * Test if @res compatible with @place and @size.
> > > > > + *
> > > > > + * Returns true if the res placement compatible with @place and @size.
> > > > > + */
> > > > > +bool ttm_resource_compatible(struct ttm_device *bdev,
> > > > > +                 struct ttm_resource *res,
> > > > > +                 const struct ttm_place *place,
> > > > > +                 size_t size)
> > > > > +{
> > > > > +    struct ttm_resource_manager *man;
> > > > > +
> > > > > +    if (!res || !place)
> > > > > +        return false;
> > > > > +
> > > > > +    man = ttm_manager_type(bdev, res->mem_type);
> > > > > +    if (!man->func->compatible) {
> > > > > +        if (res->start < place->fpfn ||
> > > > > +            (place->lpfn && (res->start + res->num_pages) >
> > > > > place->lpfn))
> > > > > +            return false;
> > > > > +
> > > > > +        return true;
> > > > > +    }
> > > > > +
> > > > > +    return man->func->compatible(man, res, place, size);
> > > > > +}
> > > > > +
> > > > >    static bool ttm_resource_places_compat(struct ttm_resource *res,
> > > > >                           const struct ttm_place *places,
> > > > >                           unsigned num_placement)
> > > > >    {
> > > > > +    struct ttm_buffer_object *bo = res->bo;
> > > > > +    struct ttm_device *bdev = bo->bdev;
> > > > >        unsigned i;
> > > > >          if (res->placement & TTM_PL_FLAG_TEMPORARY)
> > > > > @@ -265,8 +339,7 @@ static bool ttm_resource_places_compat(struct
> > > > > ttm_resource *res,
> > > > >        for (i = 0; i < num_placement; i++) {
> > > > >            const struct ttm_place *heap = &places[i];
> > > > >    -        if (res->start < heap->fpfn || (heap->lpfn &&
> > > > > -            (res->start + res->num_pages) > heap->lpfn))
> > > > > +        if (!ttm_resource_compatible(bdev, res, heap, bo->base.size))
> > > > >                continue;
> > > > >              if ((res->mem_type == heap->mem_type) &&
> > > > > diff --git a/include/drm/ttm/ttm_resource.h
> > > > > b/include/drm/ttm/ttm_resource.h
> > > > > index ca89a48c2460..5afc6d664fde 100644
> > > > > --- a/include/drm/ttm/ttm_resource.h
> > > > > +++ b/include/drm/ttm/ttm_resource.h
> > > > > @@ -88,6 +88,38 @@ struct ttm_resource_manager_func {
> > > > >        void (*free)(struct ttm_resource_manager *man,
> > > > >                 struct ttm_resource *res);
> > > > >    +    /**
> > > > > +     * struct ttm_resource_manager_func member intersects
> > > > > +     *
> > > > > +     * @man: Pointer to a memory type manager.
> > > > > +     * @res: Pointer to a struct ttm_resource to be checked.
> > > > > +     * @place: Placement to check against.
> > > > > +     * @size: Size of the check.
> > > > > +     *
> > > > > +     * Test if @res intersects with @place + @size. Used to judge if
> > > > > +     * evictions are valueable or not.
> > > > > +     */
> > > > > +    bool (*intersects)(struct ttm_resource_manager *man,
> > > > > +               struct ttm_resource *res,
> > > > > +               const struct ttm_place *place,
> > > > > +               size_t size);
> > > > > +
> > > > > +    /**
> > > > > +     * struct ttm_resource_manager_func member compatible
> > > > > +     *
> > > > > +     * @man: Pointer to a memory type manager.
> > > > > +     * @res: Pointer to a struct ttm_resource to be checked.
> > > > > +     * @place: Placement to check against.
> > > > > +     * @size: Size of the check.
> > > > > +     *
> > > > > +     * Test if @res compatible with @place + @size. Used to check of
> > > > > +     * the need to move the backing store or not.
> > > > > +     */
> > > > > +    bool (*compatible)(struct ttm_resource_manager *man,
> > > > > +               struct ttm_resource *res,
> > > > > +               const struct ttm_place *place,
> > > > > +               size_t size);
> > > > > +
> > > > >        /**
> > > > >         * struct ttm_resource_manager_func member debug
> > > > >         *
> > > > > @@ -329,6 +361,14 @@ int ttm_resource_alloc(struct ttm_buffer_object
> > > > > *bo,
> > > > >                   const struct ttm_place *place,
> > > > >                   struct ttm_resource **res);
> > > > >    void ttm_resource_free(struct ttm_buffer_object *bo, struct
> > > > > ttm_resource **res);
> > > > > +bool ttm_resource_intersects(struct ttm_device *bdev,
> > > > > +                 struct ttm_resource *res,
> > > > > +                 const struct ttm_place *place,
> > > > > +                 size_t size);
> > > > > +bool ttm_resource_compatible(struct ttm_device *bdev,
> > > > > +                 struct ttm_resource *res,
> > > > > +                 const struct ttm_place *place,
> > > > > +                 size_t size);
> > > > >    bool ttm_resource_compat(struct ttm_resource *res,
> > > > >                 struct ttm_placement *placement);
> > > > >    void ttm_resource_set_bo(struct ttm_resource *res,
> > > > > 
> > > > > base-commit: 730c2bf4ad395acf0aa0820535fdb8ea6abe5df1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch