[PATCH v6 1/6] drm/ttm: Add new callbacks to ttm res mgr
Daniel Vetter
daniel at ffwll.ch
Tue Sep 6 19:58:00 UTC 2022
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 at amd.com>
> > > Signed-off-by: Arunpravin Paneer Selvam
> > > <Arunpravin.PaneerSelvam at 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 at 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
More information about the amd-gfx
mailing list