[PATCH] drm: remove min_order BUG_ON check

[Matthew Auld]

On 10/03/2022 14:47, Arunpravin wrote:
> 
> 
> On 08/03/22 10:31 pm, Matthew Auld wrote:
>> On 08/03/2022 13:59, Arunpravin wrote:
>>>
>>>
>>> On 07/03/22 10:11 pm, Matthew Auld wrote:
>>>> On 07/03/2022 14:37, Arunpravin wrote:
>>>>> place BUG_ON(order < min_order) outside do..while
>>>>> loop as it fails Unigine Heaven benchmark.
>>>>>
>>>>> Unigine Heaven has buffer allocation requests for
>>>>> example required pages are 161 and alignment request
>>>>> is 128. To allocate the remaining 33 pages, continues
>>>>> the iteration to find the order value which is 5 and
>>>>> when it compares with min_order = 7, enables the
>>>>> BUG_ON(). To avoid this problem, placed the BUG_ON
>>>>> check outside of do..while loop.
>>>>>
>>>>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam at amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/drm_buddy.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>>> index 72f52f293249..ed94c56b720f 100644
>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>> @@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>>     	order = fls(pages) - 1;
>>>>>     	min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>>>>>     
>>>>> +	BUG_ON(order < min_order);
>>>>
>>>> Isn't the issue that we are allowing a size that is not aligned to the
>>>> requested min_page_size? Should we not fix the caller(and throw a normal
>>>> error here), or perhaps add the round_up() here instead?
>>>>
>>> CASE 1:
>>> when size is not aligned to the requested min_page_size, for instance,
>>> required size = 161 pages, min_page_size = 128 pages, here we have 3
>>> possible options,
>>> a. AFAIK,This kind of situation is common in any workload,the first
>>> allocation (i.e) 128 pages is aligned to min_page_size, Should we just
>>> allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
>>> know the left over pages are not in min_page_size alignment?
>>
>> So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
>> arbitrary physical alignment for an object? Is that not meant to apply
>> to every page/chunk? The above example would only have the correct
>> physical alignment guaranteed for the first chunk, or so, is this the
>> expected ABI behaviour?
>>
> I gone through the function amdgpu_gem_create_ioctl(), it reads the
> physical alignment in bytes from userspace, does i915 round up the size
> value to the alignment or does i915 fails the allocation request if size
> is not aligned with min_page_size? If not, I think running unigine
> heaven or similar benchmark triggers BUG_ON() on current version of drm
> buddy

i915 will always round_up the obj->base.size as per the 
default_page_size. But in our case the default_page_size is selected by 
the kernel, which is always either PAGE_SIZE, or 64K on some platforms, 
due to the HW having some minimum GPU page-size for mapping VRAM pages. 
We don't currently have anything similar to 
amdgpu_gem_create_in.alignment, where userspace can request some 
arbitrary physical alignment.

>> Also looking at this some more, the other related bug here is the
>> order-- == min_order check, since it now won't bail when order == 0,
>> leading to order = -1, if we are unlucky...
> will add a fix
>>
>> Originally, if asking for min_page_size > chunk_size, then the
>> allocation was meant to fail if it can't fill the resource request with
>> pages of at least that size(and also alignment). Or at least that was
>> the original meaning in i915 IIRC.
> we can follow the same here too, failing the allocation request if size
> is not aligned with min_page_size?

Yeah, seems reasonable to me.

> 
> I added a debug print for requested num_pages from userspace and its
> alignment request and executed unigine heaven, I see many such instances
> where min_page_size is not aligned to the size, how i915 handles such
> requests?
>>
>>>
>>> b. There are many such instances in unigine heaven workload (there would
>>> be many such workloads), throwing a normal error would lower the FPS? is
>>> it possible to fix at caller application?
>>>
>>> c. adding the round_up() is possible, but in every such instances we end
>>> up allocating extra unused memory. For example, if required pages = 1028
>>> and min_page_size = 1024 pages, we end up round up of left over 4 pages
>>> to the min_page_size, so the total size would be 2048 pages.
>>>
>>>> i.e if someone does:
>>>>
>>>> alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
>>> CASE 2:
>>> I think this case should be detected (i.e) when min_page_size > size,
>>> should we return -EINVAL?
>>>>
>>>> This will still trigger the BUG_ON() even if we move it out of the loop,
>>>> AFAICT.
>>>>
>>>
>>> Should we just allow the CASE 1 proceed for the allocation and return
>>> -EINVAL for the CASE 2?
>>>
>>>>> +
>>>>>     	do {
>>>>>     		order = min(order, (unsigned int)fls(pages) - 1);
>>>>>     		BUG_ON(order > mm->max_order);
>>>>> -		BUG_ON(order < min_order);
>>>>>     
>>>>>     		do {
>>>>>     			if (flags & DRM_BUDDY_RANGE_ALLOCATION)
>>>>>
>>>>> base-commit: 8025c79350b90e5a8029234d433578f12abbae2b