Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Robert Beckett <bob.beckett@collabora.com>]

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
>>>>>
>>>
>