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@xxxxxxx>
> > > > > Signed-off-by: Arunpravin Paneer Selvam
> > > > > <Arunpravin.PaneerSelvam@xxxxxxx>
> > > > 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@xxxxxxx> 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
