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 <[email protected]>
>>>>>> ---
>>>>>> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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
>>>>>>
>>>>
>>