Re: [Intel-gfx] [PATCH] drm: remove min_order BUG_ON check

[Arunpravin <arunpravin.paneerselvam@amd.com>]

On 10/03/22 8:59 pm, Matthew Auld wrote:
> 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@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 had internal discussion with Christian and he suggested to round_up
the size to the alignment and trim the block to the required original
size. I have sent the patch, please review.

Thanks,
Arun
> 
>>
>> 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