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

[Daniel Vetter <daniel@ffwll.ch>]

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?

I'm a bit confused why the bloat here ...

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.
-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