| Message ID | 20220728143315.968590-4-Arunpravin.PaneerSelvam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] drm/ttm: Add new callbacks to ttm res mgr | expand |
On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote: > Implemented a new intersect and compatible callback function > fetching start offset from drm buddy allocator. > > v2: move the bits that are specific to buddy_man (Matthew) > > Signed-off-by: Christian König <christian.koenig@amd.com> > Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 39 +----------- > drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 62 +++++++++++++++++++ > 2 files changed, 64 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 70e2ed4e99df..54eead15d74b 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct ttm_buffer_object *bo, > if (!i915_gem_object_evictable(obj)) > return false; > > - switch (res->mem_type) { > - case I915_PL_LMEM0: { > - struct ttm_resource_manager *man = > - ttm_manager_type(bo->bdev, res->mem_type); > - struct i915_ttm_buddy_resource *bman_res = > - to_ttm_buddy_resource(res); > - struct drm_buddy *mm = bman_res->mm; > - struct drm_buddy_block *block; > - > - if (!place->fpfn && !place->lpfn) > - return true; > - > - GEM_BUG_ON(!place->lpfn); > - > - /* > - * If we just want something mappable then we can quickly check > - * if the current victim resource is using any of the CPU > - * visible portion. > - */ > - if (!place->fpfn && > - place->lpfn == i915_ttm_buddy_man_visible_size(man)) > - return bman_res->used_visible_size > 0; > - > - /* Real range allocation */ > - list_for_each_entry(block, &bman_res->blocks, link) { > - unsigned long fpfn = > - drm_buddy_block_offset(block) >> PAGE_SHIFT; > - unsigned long lpfn = fpfn + > - (drm_buddy_block_size(mm, block) >> PAGE_SHIFT); > - > - if (place->fpfn < lpfn && place->lpfn > fpfn) > - return true; > - } > - return false; > - } default: > - break; > - } > + if (res->mem_type == I915_PL_LMEM0) > + return ttm_bo_eviction_valuable(bo, place); We should be able to drop the mem_type == I915_PL_LMEM0 check here I think, and just unconditionally do: return ttm_bo_eviction_valuable(bo, place); > > return true; > } > diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c > index a5109548abc0..9d2a31154d58 100644 > --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c > +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c > @@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct ttm_resource_manager *man, > kfree(bman_res); > } > > +static bool i915_ttm_buddy_man_intersect(struct ttm_resource_manager *man, Nit: intersects > + struct ttm_resource *res, > + const struct ttm_place *place, > + size_t size) > +{ > + struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res); > + u32 start, num_pages = PFN_UP(size); > + struct drm_buddy_block *block; > + > + if (!place->fpfn && !place->lpfn) > + return true; > + > + /* > + * If we just want something mappable then we can quickly check > + * if the current victim resource is using any of the CP > + * visible portion. > + */ > + if (!place->fpfn && > + place->lpfn == i915_ttm_buddy_man_visible_size(man)) > + return bman_res->used_visible_size > 0; > + > + /* Check each drm buddy block individually */ > + list_for_each_entry(block, &bman_res->blocks, link) { > + start = drm_buddy_block_offset(block) >> PAGE_SHIFT; > + /* Don't evict BOs outside of the requested placement range */ > + if (place->fpfn >= (start + num_pages) || > + (place->lpfn && place->lpfn <= start)) > + return false; > + } > + > + return true; We need to account for the block size somewhere. Also same bug in the amdgpu patch it seems. We also need to do this the other way around and keep checking until we find something that overlaps, for example if the first block doesn't intersect/overlap we will incorrectly return false here, even if one of the other blocks does intersect. list_for_each_entry() { fpfn = drm_buddy_block_size(mm, block) >> PAGE_SHIFT; lpfn = fpfn + drm_buddy_block_size(mm, block) >> PAGE_SHIFT); if (place->fpfn < lpfn && place->lpfn > fpfn) return true; } return false; > +} > + > +static bool i915_ttm_buddy_man_compatible(struct ttm_resource_manager *man, > + struct ttm_resource *res, > + const struct ttm_place *place, > + size_t size) > +{ > + struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res); > + u32 start, num_pages = PFN_UP(size); > + struct drm_buddy_block *block; > + > + if (!place->fpfn && !place->lpfn) > + return true; > + > + if (!place->fpfn && > + place->lpfn == i915_ttm_buddy_man_visible_size(man)) > + return bman_res->used_visible_size == res->num_pages; > + > + /* Check each drm buddy block individually */ > + list_for_each_entry(block, &bman_res->blocks, link) { > + start = drm_buddy_block_offset(block) >> PAGE_SHIFT; > + if (start < place->fpfn || > + (place->lpfn && (start + num_pages) > place->lpfn)) Same here. We need to consider the block size/range. Otherwise I think looks good. > + return false; > + } > + > + return true; > +} > + > static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man, > struct drm_printer *printer) > { > @@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man, > static const struct ttm_resource_manager_func i915_ttm_buddy_manager_func = { > .alloc = i915_ttm_buddy_man_alloc, > .free = i915_ttm_buddy_man_free, > + .intersects = i915_ttm_buddy_man_intersect, > + .compatible = i915_ttm_buddy_man_compatible, > .debug = i915_ttm_buddy_man_debug, > }; >
On 7/28/2022 8:57 PM, Matthew Auld wrote: > On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote: >> Implemented a new intersect and compatible callback function >> fetching start offset from drm buddy allocator. >> >> v2: move the bits that are specific to buddy_man (Matthew) >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> Signed-off-by: Arunpravin Paneer Selvam >> <Arunpravin.PaneerSelvam@amd.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 39 +----------- >> drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 62 +++++++++++++++++++ >> 2 files changed, 64 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> index 70e2ed4e99df..54eead15d74b 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> @@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct >> ttm_buffer_object *bo, >> if (!i915_gem_object_evictable(obj)) >> return false; >> - switch (res->mem_type) { >> - case I915_PL_LMEM0: { >> - struct ttm_resource_manager *man = >> - ttm_manager_type(bo->bdev, res->mem_type); >> - struct i915_ttm_buddy_resource *bman_res = >> - to_ttm_buddy_resource(res); >> - struct drm_buddy *mm = bman_res->mm; >> - struct drm_buddy_block *block; >> - >> - if (!place->fpfn && !place->lpfn) >> - return true; >> - >> - GEM_BUG_ON(!place->lpfn); >> - >> - /* >> - * If we just want something mappable then we can quickly check >> - * if the current victim resource is using any of the CPU >> - * visible portion. >> - */ >> - if (!place->fpfn && >> - place->lpfn == i915_ttm_buddy_man_visible_size(man)) >> - return bman_res->used_visible_size > 0; >> - >> - /* Real range allocation */ >> - list_for_each_entry(block, &bman_res->blocks, link) { >> - unsigned long fpfn = >> - drm_buddy_block_offset(block) >> PAGE_SHIFT; >> - unsigned long lpfn = fpfn + >> - (drm_buddy_block_size(mm, block) >> PAGE_SHIFT); >> - >> - if (place->fpfn < lpfn && place->lpfn > fpfn) >> - return true; >> - } >> - return false; >> - } default: >> - break; >> - } >> + if (res->mem_type == I915_PL_LMEM0) >> + return ttm_bo_eviction_valuable(bo, place); > > We should be able to drop the mem_type == I915_PL_LMEM0 check here I > think, and just unconditionally do: > > return ttm_bo_eviction_valuable(bo, place); okay > >> return true; >> } >> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c >> b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c >> index a5109548abc0..9d2a31154d58 100644 >> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c >> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c >> @@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct >> ttm_resource_manager *man, >> kfree(bman_res); >> } >> +static bool i915_ttm_buddy_man_intersect(struct >> ttm_resource_manager *man, > > Nit: intersects ok > >> + struct ttm_resource *res, >> + const struct ttm_place *place, >> + size_t size) >> +{ >> + struct i915_ttm_buddy_resource *bman_res = >> to_ttm_buddy_resource(res); >> + u32 start, num_pages = PFN_UP(size); >> + struct drm_buddy_block *block; >> + >> + if (!place->fpfn && !place->lpfn) >> + return true; I place bug_on check here GEM_BUG_ON(!place->lpfn); >> + >> + /* >> + * If we just want something mappable then we can quickly check >> + * if the current victim resource is using any of the CP >> + * visible portion. >> + */ >> + if (!place->fpfn && >> + place->lpfn == i915_ttm_buddy_man_visible_size(man)) >> + return bman_res->used_visible_size > 0; >> + >> + /* Check each drm buddy block individually */ >> + list_for_each_entry(block, &bman_res->blocks, link) { >> + start = drm_buddy_block_offset(block) >> PAGE_SHIFT; >> + /* Don't evict BOs outside of the requested placement range */ >> + if (place->fpfn >= (start + num_pages) || >> + (place->lpfn && place->lpfn <= start)) >> + return false; >> + } >> + >> + return true; > > We need to account for the block size somewhere. Also same bug in the > amdgpu patch it seems. We also need to do this the other way around > and keep checking until we find something that overlaps, for example > if the first block doesn't intersect/overlap we will incorrectly > return false here, even if one of the other blocks does intersect. > > list_for_each_entry() { > fpfn = drm_buddy_block_size(mm, block) >> PAGE_SHIFT; > lpfn = fpfn + drm_buddy_block_size(mm, block) >> PAGE_SHIFT); > > if (place->fpfn < lpfn && place->lpfn > fpfn) > return true; > } > > return false; yes, here the final code looks like, list_for_each_entry(block, &bman_res->blocks, link) { unsigned long fpfn = drm_buddy_block_offset(block) >> PAGE_SHIFT; unsigned long lpfn = fpfn + (drm_buddy_block_size(mm, block) >> PAGE_SHIFT); /* Don't evict BOs outside of the requested placement range */ if (place->fpfn < lpfn && place->lpfn > fpfn) return true; } return false; > >> +} >> + >> +static bool i915_ttm_buddy_man_compatible(struct >> ttm_resource_manager *man, >> + struct ttm_resource *res, >> + const struct ttm_place *place, >> + size_t size) >> +{ >> + struct i915_ttm_buddy_resource *bman_res = >> to_ttm_buddy_resource(res); >> + u32 start, num_pages = PFN_UP(size); >> + struct drm_buddy_block *block; >> + >> + if (!place->fpfn && !place->lpfn) >> + return true; I place bug_on check here GEM_BUG_ON(!place->lpfn); >> + >> + if (!place->fpfn && >> + place->lpfn == i915_ttm_buddy_man_visible_size(man)) >> + return bman_res->used_visible_size == res->num_pages; >> + >> + /* Check each drm buddy block individually */ >> + list_for_each_entry(block, &bman_res->blocks, link) { >> + start = drm_buddy_block_offset(block) >> PAGE_SHIFT; >> + if (start < place->fpfn || >> + (place->lpfn && (start + num_pages) > place->lpfn)) > > Same here. We need to consider the block size/range. ahh somehow missed the block size, here the final code looks like, /* Check each drm buddy block individually */ list_for_each_entry(block, &bman_res->blocks, link) { unsigned long fpfn = drm_buddy_block_offset(block) >> PAGE_SHIFT; unsigned long lpfn = fpfn + (drm_buddy_block_size(mm, block) >> PAGE_SHIFT); if (fpfn < place->fpfn || lpfn > place->lpfn) return false; } return true; > > Otherwise I think looks good. > >> + return false; >> + } >> + >> + return true; >> +} >> + >> static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man, >> struct drm_printer *printer) >> { >> @@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct >> ttm_resource_manager *man, >> static const struct ttm_resource_manager_func >> i915_ttm_buddy_manager_func = { >> .alloc = i915_ttm_buddy_man_alloc, >> .free = i915_ttm_buddy_man_free, >> + .intersects = i915_ttm_buddy_man_intersect, >> + .compatible = i915_ttm_buddy_man_compatible, >> .debug = i915_ttm_buddy_man_debug, >> };
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 70e2ed4e99df..54eead15d74b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct ttm_buffer_object *bo, if (!i915_gem_object_evictable(obj)) return false; - switch (res->mem_type) { - case I915_PL_LMEM0: { - struct ttm_resource_manager *man = - ttm_manager_type(bo->bdev, res->mem_type); - struct i915_ttm_buddy_resource *bman_res = - to_ttm_buddy_resource(res); - struct drm_buddy *mm = bman_res->mm; - struct drm_buddy_block *block; - - if (!place->fpfn && !place->lpfn) - return true; - - GEM_BUG_ON(!place->lpfn); - - /* - * If we just want something mappable then we can quickly check - * if the current victim resource is using any of the CPU - * visible portion. - */ - if (!place->fpfn && - place->lpfn == i915_ttm_buddy_man_visible_size(man)) - return bman_res->used_visible_size > 0; - - /* Real range allocation */ - list_for_each_entry(block, &bman_res->blocks, link) { - unsigned long fpfn = - drm_buddy_block_offset(block) >> PAGE_SHIFT; - unsigned long lpfn = fpfn + - (drm_buddy_block_size(mm, block) >> PAGE_SHIFT); - - if (place->fpfn < lpfn && place->lpfn > fpfn) - return true; - } - return false; - } default: - break; - } + if (res->mem_type == I915_PL_LMEM0) + return ttm_bo_eviction_valuable(bo, place); return true; } diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c index a5109548abc0..9d2a31154d58 100644 --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c @@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct ttm_resource_manager *man, kfree(bman_res); } +static bool i915_ttm_buddy_man_intersect(struct ttm_resource_manager *man, + struct ttm_resource *res, + const struct ttm_place *place, + size_t size) +{ + struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res); + u32 start, num_pages = PFN_UP(size); + struct drm_buddy_block *block; + + if (!place->fpfn && !place->lpfn) + return true; + + /* + * If we just want something mappable then we can quickly check + * if the current victim resource is using any of the CP + * visible portion. + */ + if (!place->fpfn && + place->lpfn == i915_ttm_buddy_man_visible_size(man)) + return bman_res->used_visible_size > 0; + + /* Check each drm buddy block individually */ + list_for_each_entry(block, &bman_res->blocks, link) { + start = drm_buddy_block_offset(block) >> PAGE_SHIFT; + /* Don't evict BOs outside of the requested placement range */ + if (place->fpfn >= (start + num_pages) || + (place->lpfn && place->lpfn <= start)) + return false; + } + + return true; +} + +static bool i915_ttm_buddy_man_compatible(struct ttm_resource_manager *man, + struct ttm_resource *res, + const struct ttm_place *place, + size_t size) +{ + struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res); + u32 start, num_pages = PFN_UP(size); + struct drm_buddy_block *block; + + if (!place->fpfn && !place->lpfn) + return true; + + if (!place->fpfn && + place->lpfn == i915_ttm_buddy_man_visible_size(man)) + return bman_res->used_visible_size == res->num_pages; + + /* Check each drm buddy block individually */ + list_for_each_entry(block, &bman_res->blocks, link) { + start = drm_buddy_block_offset(block) >> PAGE_SHIFT; + if (start < place->fpfn || + (place->lpfn && (start + num_pages) > place->lpfn)) + return false; + } + + return true; +} + static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man, struct drm_printer *printer) { @@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man, static const struct ttm_resource_manager_func i915_ttm_buddy_manager_func = { .alloc = i915_ttm_buddy_man_alloc, .free = i915_ttm_buddy_man_free, + .intersects = i915_ttm_buddy_man_intersect, + .compatible = i915_ttm_buddy_man_compatible, .debug = i915_ttm_buddy_man_debug, };