| Message ID | 20220315180444.3327283-6-bob.beckett@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: ttm for stolen | expand |
Am 15.03.22 um 19:04 schrieb Robert Beckett: > RFC: do we want this to become a generic interface in > ttm_resource_manager_func? > > RFC: would we prefer a different interface? e.g. > for_each_resource_in_range or for_each_bo_in_range Well completely NAK to that. Why do you need that? The long term goal is to completely remove the range checks from TTM instead. Regards, Christian. > > Signed-off-by: Robert Beckett <bob.beckett@collabora.com> > --- > drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++ > include/drm/ttm/ttm_range_manager.h | 3 +++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c > index 8cd4f3fb9f79..5662627bb933 100644 > --- a/drivers/gpu/drm/ttm/ttm_range_manager.c > +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c > @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct ttm_device *bdev, > return 0; > } > EXPORT_SYMBOL(ttm_range_man_fini_nocheck); > + > +/** > + * ttm_range_man_range_busy - Check whether anything is allocated with a range > + * > + * @man: memory manager to check > + * @fpfn: first page number to check > + * @lpfn: last page number to check > + * > + * Return: true if anything allocated within the range, false otherwise. > + */ > +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, > + unsigned fpfn, unsigned lpfn) > +{ > + struct ttm_range_manager *rman = to_range_manager(man); > + struct drm_mm *mm = &rman->mm; > + > + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 1) - 1)) > + return true; > + return false; > +} > +EXPORT_SYMBOL(ttm_range_man_range_busy); > diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h > index 7963b957e9ef..86794a3f9101 100644 > --- a/include/drm/ttm/ttm_range_manager.h > +++ b/include/drm/ttm/ttm_range_manager.h > @@ -53,4 +53,7 @@ static __always_inline int ttm_range_man_fini(struct ttm_device *bdev, > BUILD_BUG_ON(__builtin_constant_p(type) && type >= TTM_NUM_MEM_TYPES); > return ttm_range_man_fini_nocheck(bdev, type); > } > + > +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, > + unsigned fpfn, unsigned lpfn); > #endif
On 16/03/2022 09:54, Christian König wrote: > Am 15.03.22 um 19:04 schrieb Robert Beckett: >> RFC: do we want this to become a generic interface in >> ttm_resource_manager_func? >> >> RFC: would we prefer a different interface? e.g. >> for_each_resource_in_range or for_each_bo_in_range > > Well completely NAK to that. Why do you need that? > > The long term goal is to completely remove the range checks from TTM > instead. ah, I did not know that. I wanted it just to enable parity with a selftest that checks whether a range is allocated before initializing a given range with test data behind the allocator's back. It needs to check the range so that it doesn't destroy in use data. I suppose we could add another drm_mm range tracker just for testing and shadow track each allocation in the range, but that seemed like a lot of extra infrastructure for no general runtime use. would you mind explaining the rationale for removing range checks? It seems to me like a natural fit for a memory manager > > Regards, > Christian. > >> >> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >> --- >> drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++ >> include/drm/ttm/ttm_range_manager.h | 3 +++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c >> b/drivers/gpu/drm/ttm/ttm_range_manager.c >> index 8cd4f3fb9f79..5662627bb933 100644 >> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c >> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c >> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct ttm_device >> *bdev, >> return 0; >> } >> EXPORT_SYMBOL(ttm_range_man_fini_nocheck); >> + >> +/** >> + * ttm_range_man_range_busy - Check whether anything is allocated >> with a range >> + * >> + * @man: memory manager to check >> + * @fpfn: first page number to check >> + * @lpfn: last page number to check >> + * >> + * Return: true if anything allocated within the range, false otherwise. >> + */ >> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >> + unsigned fpfn, unsigned lpfn) >> +{ >> + struct ttm_range_manager *rman = to_range_manager(man); >> + struct drm_mm *mm = &rman->mm; >> + >> + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + >> 1) - 1)) >> + return true; >> + return false; >> +} >> +EXPORT_SYMBOL(ttm_range_man_range_busy); >> diff --git a/include/drm/ttm/ttm_range_manager.h >> b/include/drm/ttm/ttm_range_manager.h >> index 7963b957e9ef..86794a3f9101 100644 >> --- a/include/drm/ttm/ttm_range_manager.h >> +++ b/include/drm/ttm/ttm_range_manager.h >> @@ -53,4 +53,7 @@ static __always_inline int ttm_range_man_fini(struct >> ttm_device *bdev, >> BUILD_BUG_ON(__builtin_constant_p(type) && type >= >> TTM_NUM_MEM_TYPES); >> return ttm_range_man_fini_nocheck(bdev, type); >> } >> + >> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >> + unsigned fpfn, unsigned lpfn); >> #endif >
Am 16.03.22 um 14:19 schrieb Robert Beckett: > > > On 16/03/2022 09:54, Christian König wrote: >> Am 15.03.22 um 19:04 schrieb Robert Beckett: >>> RFC: do we want this to become a generic interface in >>> ttm_resource_manager_func? >>> >>> RFC: would we prefer a different interface? e.g. >>> for_each_resource_in_range or for_each_bo_in_range >> >> Well completely NAK to that. Why do you need that? >> >> The long term goal is to completely remove the range checks from TTM >> instead. > > ah, I did not know that. > I wanted it just to enable parity with a selftest that checks whether > a range is allocated before initializing a given range with test data > behind the allocator's back. It needs to check the range so that it > doesn't destroy in use data. Mhm, of hand that doesn't sounds like a valid test case. Do you have the code at hand? > > I suppose we could add another drm_mm range tracker just for testing > and shadow track each allocation in the range, but that seemed like a > lot of extra infrastructure for no general runtime use. I have no idea what you mean with that. > > would you mind explaining the rationale for removing range checks? It > seems to me like a natural fit for a memory manager TTM manages buffer objects and resources, not address space. The lpfn/fpfn parameter for the resource allocators are actually used as just two independent parameters and not define any range. We just keep the names for historical reasons. The only places we still use and compare them as ranges are ttm_resource_compat() and ttm_bo_eviction_valuable() and I already have patches to clean up those and move them into the backend resource handling. Regards, Christian. > >> >> Regards, >> Christian. >> >>> >>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>> --- >>> drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++ >>> include/drm/ttm/ttm_range_manager.h | 3 +++ >>> 2 files changed, 24 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c >>> b/drivers/gpu/drm/ttm/ttm_range_manager.c >>> index 8cd4f3fb9f79..5662627bb933 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c >>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c >>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct >>> ttm_device *bdev, >>> return 0; >>> } >>> EXPORT_SYMBOL(ttm_range_man_fini_nocheck); >>> + >>> +/** >>> + * ttm_range_man_range_busy - Check whether anything is allocated >>> with a range >>> + * >>> + * @man: memory manager to check >>> + * @fpfn: first page number to check >>> + * @lpfn: last page number to check >>> + * >>> + * Return: true if anything allocated within the range, false >>> otherwise. >>> + */ >>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>> + unsigned fpfn, unsigned lpfn) >>> +{ >>> + struct ttm_range_manager *rman = to_range_manager(man); >>> + struct drm_mm *mm = &rman->mm; >>> + >>> + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + >>> 1) - 1)) >>> + return true; >>> + return false; >>> +} >>> +EXPORT_SYMBOL(ttm_range_man_range_busy); >>> diff --git a/include/drm/ttm/ttm_range_manager.h >>> b/include/drm/ttm/ttm_range_manager.h >>> index 7963b957e9ef..86794a3f9101 100644 >>> --- a/include/drm/ttm/ttm_range_manager.h >>> +++ b/include/drm/ttm/ttm_range_manager.h >>> @@ -53,4 +53,7 @@ static __always_inline int >>> ttm_range_man_fini(struct ttm_device *bdev, >>> BUILD_BUG_ON(__builtin_constant_p(type) && type >= >>> TTM_NUM_MEM_TYPES); >>> return ttm_range_man_fini_nocheck(bdev, type); >>> } >>> + >>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>> + unsigned fpfn, unsigned lpfn); >>> #endif >>
On 16/03/2022 13:43, Christian König wrote: > Am 16.03.22 um 14:19 schrieb Robert Beckett: >> >> >> On 16/03/2022 09:54, Christian König wrote: >>> Am 15.03.22 um 19:04 schrieb Robert Beckett: >>>> RFC: do we want this to become a generic interface in >>>> ttm_resource_manager_func? >>>> >>>> RFC: would we prefer a different interface? e.g. >>>> for_each_resource_in_range or for_each_bo_in_range >>> >>> Well completely NAK to that. Why do you need that? >>> >>> The long term goal is to completely remove the range checks from TTM >>> instead. >> >> ah, I did not know that. >> I wanted it just to enable parity with a selftest that checks whether >> a range is allocated before initializing a given range with test data >> behind the allocator's back. It needs to check the range so that it >> doesn't destroy in use data. > > Mhm, of hand that doesn't sounds like a valid test case. Do you have the > code at hand? https://patchwork.freedesktop.org/patch/478347/?series=101396&rev=1 this is where I replace an existing range check via drm_mm with the range check I added in this patch. > >> >> I suppose we could add another drm_mm range tracker just for testing >> and shadow track each allocation in the range, but that seemed like a >> lot of extra infrastructure for no general runtime use. > > I have no idea what you mean with that. I meant as a potential solution to tracking allocations without a range check, we would need to add something external. e.g. adding a shadow drm_mm range tracker, or a bitmask across the range, or stick objects in a list etc. > >> >> would you mind explaining the rationale for removing range checks? It >> seems to me like a natural fit for a memory manager > > TTM manages buffer objects and resources, not address space. The > lpfn/fpfn parameter for the resource allocators are actually used as > just two independent parameters and not define any range. We just keep > the names for historical reasons. > > The only places we still use and compare them as ranges are > ttm_resource_compat() and ttm_bo_eviction_valuable() and I already have > patches to clean up those and move them into the backend resource handling. except the ttm_range_manager seems to still use them as a range specifier. If the general design going forward is to not consider ranges, how would you recommend constructing buffers around pre-allocated regions e.g. uefi frame buffers who's range is dictated externally? > > Regards, > Christian. > >> >>> >>> Regards, >>> Christian. >>> >>>> >>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>>> --- >>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++ >>>> include/drm/ttm/ttm_range_manager.h | 3 +++ >>>> 2 files changed, 24 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>> index 8cd4f3fb9f79..5662627bb933 100644 >>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct >>>> ttm_device *bdev, >>>> return 0; >>>> } >>>> EXPORT_SYMBOL(ttm_range_man_fini_nocheck); >>>> + >>>> +/** >>>> + * ttm_range_man_range_busy - Check whether anything is allocated >>>> with a range >>>> + * >>>> + * @man: memory manager to check >>>> + * @fpfn: first page number to check >>>> + * @lpfn: last page number to check >>>> + * >>>> + * Return: true if anything allocated within the range, false >>>> otherwise. >>>> + */ >>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>> + unsigned fpfn, unsigned lpfn) >>>> +{ >>>> + struct ttm_range_manager *rman = to_range_manager(man); >>>> + struct drm_mm *mm = &rman->mm; >>>> + >>>> + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + >>>> 1) - 1)) >>>> + return true; >>>> + return false; >>>> +} >>>> +EXPORT_SYMBOL(ttm_range_man_range_busy); >>>> diff --git a/include/drm/ttm/ttm_range_manager.h >>>> b/include/drm/ttm/ttm_range_manager.h >>>> index 7963b957e9ef..86794a3f9101 100644 >>>> --- a/include/drm/ttm/ttm_range_manager.h >>>> +++ b/include/drm/ttm/ttm_range_manager.h >>>> @@ -53,4 +53,7 @@ static __always_inline int >>>> ttm_range_man_fini(struct ttm_device *bdev, >>>> BUILD_BUG_ON(__builtin_constant_p(type) && type >= >>>> TTM_NUM_MEM_TYPES); >>>> return ttm_range_man_fini_nocheck(bdev, type); >>>> } >>>> + >>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>> + unsigned fpfn, unsigned lpfn); >>>> #endif >>> >
Am 16.03.22 um 15:26 schrieb Robert Beckett: > > [SNIP] > this is where I replace an existing range check via drm_mm with the > range check I added in this patch. Mhm, I still don't get the use case from the code, but I don't think it matters any more. >>> I suppose we could add another drm_mm range tracker just for testing >>> and shadow track each allocation in the range, but that seemed like >>> a lot of extra infrastructure for no general runtime use. >> >> I have no idea what you mean with that. > > I meant as a potential solution to tracking allocations without a > range check, we would need to add something external. e.g. adding a > shadow drm_mm range tracker, or a bitmask across the range, or stick > objects in a list etc. Ah! So you are trying to get access to the drm_mm inside the ttm_range_manager and not add some additional range check function! Now I got your use case. >>> would you mind explaining the rationale for removing range checks? >>> It seems to me like a natural fit for a memory manager >> >> TTM manages buffer objects and resources, not address space. The >> lpfn/fpfn parameter for the resource allocators are actually used as >> just two independent parameters and not define any range. We just >> keep the names for historical reasons. >> >> The only places we still use and compare them as ranges are >> ttm_resource_compat() and ttm_bo_eviction_valuable() and I already >> have patches to clean up those and move them into the backend >> resource handling. > > except the ttm_range_manager seems to still use them as a range specifier. Yeah, because the range manager is the backend which handles ranges using the drm_mm :) > If the general design going forward is to not consider ranges, how > would you recommend constructing buffers around pre-allocated regions > e.g. uefi frame buffers who's range is dictated externally? Call ttm_bo_mem_space() with the fpfn/lpfn filled in as required. See function amdgpu_bo_create_kernel_at() for an example. Regards, Christian. > >> >> Regards, >> Christian. >> >>> >>>> >>>> Regards, >>>> Christian. >>>> >>>>> >>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>>>> --- >>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++ >>>>> include/drm/ttm/ttm_range_manager.h | 3 +++ >>>>> 2 files changed, 24 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>> index 8cd4f3fb9f79..5662627bb933 100644 >>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct >>>>> ttm_device *bdev, >>>>> return 0; >>>>> } >>>>> EXPORT_SYMBOL(ttm_range_man_fini_nocheck); >>>>> + >>>>> +/** >>>>> + * ttm_range_man_range_busy - Check whether anything is allocated >>>>> with a range >>>>> + * >>>>> + * @man: memory manager to check >>>>> + * @fpfn: first page number to check >>>>> + * @lpfn: last page number to check >>>>> + * >>>>> + * Return: true if anything allocated within the range, false >>>>> otherwise. >>>>> + */ >>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>>> + unsigned fpfn, unsigned lpfn) >>>>> +{ >>>>> + struct ttm_range_manager *rman = to_range_manager(man); >>>>> + struct drm_mm *mm = &rman->mm; >>>>> + >>>>> + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn >>>>> + 1) - 1)) >>>>> + return true; >>>>> + return false; >>>>> +} >>>>> +EXPORT_SYMBOL(ttm_range_man_range_busy); >>>>> diff --git a/include/drm/ttm/ttm_range_manager.h >>>>> b/include/drm/ttm/ttm_range_manager.h >>>>> index 7963b957e9ef..86794a3f9101 100644 >>>>> --- a/include/drm/ttm/ttm_range_manager.h >>>>> +++ b/include/drm/ttm/ttm_range_manager.h >>>>> @@ -53,4 +53,7 @@ static __always_inline int >>>>> ttm_range_man_fini(struct ttm_device *bdev, >>>>> BUILD_BUG_ON(__builtin_constant_p(type) && type >= >>>>> TTM_NUM_MEM_TYPES); >>>>> return ttm_range_man_fini_nocheck(bdev, type); >>>>> } >>>>> + >>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>>> + unsigned fpfn, unsigned lpfn); >>>>> #endif >>>> >>
On 16/03/2022 14:39, Christian König wrote: > Am 16.03.22 um 15:26 schrieb Robert Beckett: >> >> [SNIP] >> this is where I replace an existing range check via drm_mm with the >> range check I added in this patch. > > Mhm, I still don't get the use case from the code, but I don't think it > matters any more. > >>>> I suppose we could add another drm_mm range tracker just for testing >>>> and shadow track each allocation in the range, but that seemed like >>>> a lot of extra infrastructure for no general runtime use. >>> >>> I have no idea what you mean with that. >> >> I meant as a potential solution to tracking allocations without a >> range check, we would need to add something external. e.g. adding a >> shadow drm_mm range tracker, or a bitmask across the range, or stick >> objects in a list etc. > > Ah! So you are trying to get access to the drm_mm inside the > ttm_range_manager and not add some additional range check function! Now > I got your use case. well, specifically I was trying to avoid having to get access to the drm_mm. I wanted to maintain an abstract interface at the resource manager level, hence the rfc to ask if we could add a range check to ttm_resource_manager_func. I don't like the idea of code external to ttm having to poke in to the implementation details of the manager to get it's underlying drm_mm. > >>>> would you mind explaining the rationale for removing range checks? >>>> It seems to me like a natural fit for a memory manager >>> >>> TTM manages buffer objects and resources, not address space. The >>> lpfn/fpfn parameter for the resource allocators are actually used as >>> just two independent parameters and not define any range. We just >>> keep the names for historical reasons. >>> >>> The only places we still use and compare them as ranges are >>> ttm_resource_compat() and ttm_bo_eviction_valuable() and I already >>> have patches to clean up those and move them into the backend >>> resource handling. >> >> except the ttm_range_manager seems to still use them as a range >> specifier. > > Yeah, because the range manager is the backend which handles ranges > using the drm_mm :) > >> If the general design going forward is to not consider ranges, how >> would you recommend constructing buffers around pre-allocated regions >> e.g. uefi frame buffers who's range is dictated externally? > > Call ttm_bo_mem_space() with the fpfn/lpfn filled in as required. See > function amdgpu_bo_create_kernel_at() for an example. ah, I see, thanks. To allow similar code to before, which was conceptually just trying to see if a range was currently free, would you be okay with a new ttm_bo_mem_try_space, which does not do the force to evict, but instead returns -EBUSY? If so, the test can try to alloc, and immediately free if successful which would imply it was free. > > Regards, > Christian. > >> >>> >>> Regards, >>> Christian. >>> >>>> >>>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> >>>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>>>>> --- >>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++ >>>>>> include/drm/ttm/ttm_range_manager.h | 3 +++ >>>>>> 2 files changed, 24 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>> index 8cd4f3fb9f79..5662627bb933 100644 >>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct >>>>>> ttm_device *bdev, >>>>>> return 0; >>>>>> } >>>>>> EXPORT_SYMBOL(ttm_range_man_fini_nocheck); >>>>>> + >>>>>> +/** >>>>>> + * ttm_range_man_range_busy - Check whether anything is allocated >>>>>> with a range >>>>>> + * >>>>>> + * @man: memory manager to check >>>>>> + * @fpfn: first page number to check >>>>>> + * @lpfn: last page number to check >>>>>> + * >>>>>> + * Return: true if anything allocated within the range, false >>>>>> otherwise. >>>>>> + */ >>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>>>> + unsigned fpfn, unsigned lpfn) >>>>>> +{ >>>>>> + struct ttm_range_manager *rman = to_range_manager(man); >>>>>> + struct drm_mm *mm = &rman->mm; >>>>>> + >>>>>> + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn >>>>>> + 1) - 1)) >>>>>> + return true; >>>>>> + return false; >>>>>> +} >>>>>> +EXPORT_SYMBOL(ttm_range_man_range_busy); >>>>>> diff --git a/include/drm/ttm/ttm_range_manager.h >>>>>> b/include/drm/ttm/ttm_range_manager.h >>>>>> index 7963b957e9ef..86794a3f9101 100644 >>>>>> --- a/include/drm/ttm/ttm_range_manager.h >>>>>> +++ b/include/drm/ttm/ttm_range_manager.h >>>>>> @@ -53,4 +53,7 @@ static __always_inline int >>>>>> ttm_range_man_fini(struct ttm_device *bdev, >>>>>> BUILD_BUG_ON(__builtin_constant_p(type) && type >= >>>>>> TTM_NUM_MEM_TYPES); >>>>>> return ttm_range_man_fini_nocheck(bdev, type); >>>>>> } >>>>>> + >>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>>>> + unsigned fpfn, unsigned lpfn); >>>>>> #endif >>>>> >>> >
Am 16.03.22 um 16:28 schrieb Robert Beckett: > > > On 16/03/2022 14:39, Christian König wrote: >> Am 16.03.22 um 15:26 schrieb Robert Beckett: >>> >>> [SNIP] >>> this is where I replace an existing range check via drm_mm with the >>> range check I added in this patch. >> >> Mhm, I still don't get the use case from the code, but I don't think >> it matters any more. >> >>>>> I suppose we could add another drm_mm range tracker just for >>>>> testing and shadow track each allocation in the range, but that >>>>> seemed like a lot of extra infrastructure for no general runtime use. >>>> >>>> I have no idea what you mean with that. >>> >>> I meant as a potential solution to tracking allocations without a >>> range check, we would need to add something external. e.g. adding a >>> shadow drm_mm range tracker, or a bitmask across the range, or stick >>> objects in a list etc. >> >> Ah! So you are trying to get access to the drm_mm inside the >> ttm_range_manager and not add some additional range check function! >> Now I got your use case. > > well, specifically I was trying to avoid having to get access to the > drm_mm. > I wanted to maintain an abstract interface at the resource manager > level, hence the rfc to ask if we could add a range check to > ttm_resource_manager_func. > > I don't like the idea of code external to ttm having to poke in to the > implementation details of the manager to get it's underlying drm_mm. The purpose of the ttm_range_manager is to implement a base class which is then extended by the drivers with more explicit functionality. I have it on my TODO list to properly export the ttm_range_manager functions and use them to simplify the amdgpu_gtt_mgr.c implementation. So accessing the drm_mm for a test case sounds perfectly fine to me as long as you document what is happening. E.g. maybe add a wrapper function to get a pointer to the drm_mm. > >> >>>>> would you mind explaining the rationale for removing range checks? >>>>> It seems to me like a natural fit for a memory manager >>>> >>>> TTM manages buffer objects and resources, not address space. The >>>> lpfn/fpfn parameter for the resource allocators are actually used >>>> as just two independent parameters and not define any range. We >>>> just keep the names for historical reasons. >>>> >>>> The only places we still use and compare them as ranges are >>>> ttm_resource_compat() and ttm_bo_eviction_valuable() and I already >>>> have patches to clean up those and move them into the backend >>>> resource handling. >>> >>> except the ttm_range_manager seems to still use them as a range >>> specifier. >> >> Yeah, because the range manager is the backend which handles ranges >> using the drm_mm :) >> >>> If the general design going forward is to not consider ranges, how >>> would you recommend constructing buffers around pre-allocated >>> regions e.g. uefi frame buffers who's range is dictated externally? >> >> Call ttm_bo_mem_space() with the fpfn/lpfn filled in as required. See >> function amdgpu_bo_create_kernel_at() for an example. > > ah, I see, thanks. > > To allow similar code to before, which was conceptually just trying to > see if a range was currently free, would you be okay with a new > ttm_bo_mem_try_space, which does not do the force to evict, but > instead returns -EBUSY? You can already do that by setting the num_busy_placement to zero. That should prevent any eviction. Regards, Christian. > > If so, the test can try to alloc, and immediately free if successful > which would imply it was free. > >> >> Regards, >> Christian. >> >>> >>>> >>>> Regards, >>>> Christian. >>>> >>>>> >>>>>> >>>>>> Regards, >>>>>> Christian. >>>>>> >>>>>>> >>>>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/ttm/ttm_range_manager.c | 21 >>>>>>> +++++++++++++++++++++ >>>>>>> include/drm/ttm/ttm_range_manager.h | 3 +++ >>>>>>> 2 files changed, 24 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>>> index 8cd4f3fb9f79..5662627bb933 100644 >>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c >>>>>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct >>>>>>> ttm_device *bdev, >>>>>>> return 0; >>>>>>> } >>>>>>> EXPORT_SYMBOL(ttm_range_man_fini_nocheck); >>>>>>> + >>>>>>> +/** >>>>>>> + * ttm_range_man_range_busy - Check whether anything is >>>>>>> allocated with a range >>>>>>> + * >>>>>>> + * @man: memory manager to check >>>>>>> + * @fpfn: first page number to check >>>>>>> + * @lpfn: last page number to check >>>>>>> + * >>>>>>> + * Return: true if anything allocated within the range, false >>>>>>> otherwise. >>>>>>> + */ >>>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>>>>> + unsigned fpfn, unsigned lpfn) >>>>>>> +{ >>>>>>> + struct ttm_range_manager *rman = to_range_manager(man); >>>>>>> + struct drm_mm *mm = &rman->mm; >>>>>>> + >>>>>>> + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), >>>>>>> PFN_PHYS(lpfn + 1) - 1)) >>>>>>> + return true; >>>>>>> + return false; >>>>>>> +} >>>>>>> +EXPORT_SYMBOL(ttm_range_man_range_busy); >>>>>>> diff --git a/include/drm/ttm/ttm_range_manager.h >>>>>>> b/include/drm/ttm/ttm_range_manager.h >>>>>>> index 7963b957e9ef..86794a3f9101 100644 >>>>>>> --- a/include/drm/ttm/ttm_range_manager.h >>>>>>> +++ b/include/drm/ttm/ttm_range_manager.h >>>>>>> @@ -53,4 +53,7 @@ static __always_inline int >>>>>>> ttm_range_man_fini(struct ttm_device *bdev, >>>>>>> BUILD_BUG_ON(__builtin_constant_p(type) && type >= >>>>>>> TTM_NUM_MEM_TYPES); >>>>>>> return ttm_range_man_fini_nocheck(bdev, type); >>>>>>> } >>>>>>> + >>>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, >>>>>>> + unsigned fpfn, unsigned lpfn); >>>>>>> #endif >>>>>> >>>> >>
diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c index 8cd4f3fb9f79..5662627bb933 100644 --- a/drivers/gpu/drm/ttm/ttm_range_manager.c +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct ttm_device *bdev, return 0; } EXPORT_SYMBOL(ttm_range_man_fini_nocheck); + +/** + * ttm_range_man_range_busy - Check whether anything is allocated with a range + * + * @man: memory manager to check + * @fpfn: first page number to check + * @lpfn: last page number to check + * + * Return: true if anything allocated within the range, false otherwise. + */ +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, + unsigned fpfn, unsigned lpfn) +{ + struct ttm_range_manager *rman = to_range_manager(man); + struct drm_mm *mm = &rman->mm; + + if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 1) - 1)) + return true; + return false; +} +EXPORT_SYMBOL(ttm_range_man_range_busy); diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h index 7963b957e9ef..86794a3f9101 100644 --- a/include/drm/ttm/ttm_range_manager.h +++ b/include/drm/ttm/ttm_range_manager.h @@ -53,4 +53,7 @@ static __always_inline int ttm_range_man_fini(struct ttm_device *bdev, BUILD_BUG_ON(__builtin_constant_p(type) && type >= TTM_NUM_MEM_TYPES); return ttm_range_man_fini_nocheck(bdev, type); } + +bool ttm_range_man_range_busy(struct ttm_resource_manager *man, + unsigned fpfn, unsigned lpfn); #endif
RFC: do we want this to become a generic interface in ttm_resource_manager_func? RFC: would we prefer a different interface? e.g. for_each_resource_in_range or for_each_bo_in_range Signed-off-by: Robert Beckett <bob.beckett@collabora.com> --- drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++ include/drm/ttm/ttm_range_manager.h | 3 +++ 2 files changed, 24 insertions(+)