| Message ID | 20220812133048.2814-1-Arunpravin.PaneerSelvam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/6] drm/ttm: Add new callbacks to ttm res mgr | expand |
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? 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
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. 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 >
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 > > >
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. > 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. 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
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 >
Am 07.09.22 um 19:00 schrieb Daniel Vetter: > [SNIP] >>> 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 Arun can you take care of improving this? Thanks, Christian. > > > >> Thanks, >> Christian. >> >>> -Daniel >>> >>>> Thanks, >>>> Arun >>>>> Regards, >>>>> Christian. >>>>>
On 9/8/2022 11:34 AM, Christian König wrote: > Am 07.09.22 um 19:00 schrieb Daniel Vetter: >> [SNIP] >>>> 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 > > Arun can you take care of improving this? Hi Christian, Yes, I will check on this. Thanks, Arun > > Thanks, > Christian. > >> >> >> >>> Thanks, >>> Christian. >>> >>>> -Daniel >>>> >>>>> Thanks, >>>>> Arun >>>>>> Regards, >>>>>> Christian. >>>>>> >
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,
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> --- 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(-) base-commit: 730c2bf4ad395acf0aa0820535fdb8ea6abe5df1