Re: [PATCH v2] drm: add a check to verify the size alignment

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

On 29/03/22 4:54 pm, Matthew Auld wrote:
> On Tue, 29 Mar 2022 at 12:17, Arunpravin Paneer Selvam
> <arunpravin.paneerselvam@amd.com> wrote:
>>
>>
>>
>> On 23/03/22 1:15 pm, Christian König wrote:
>>> Am 23.03.22 um 08:34 schrieb Arunpravin Paneer Selvam:
>>>> Add a simple check to reject any size not aligned to the
>>>> min_page_size.
>>>>
>>>> handle instances when size is not aligned with the min_page_size.
>>>> Unigine Heaven has allocation requests for example required pages
>>>> are 257 and alignment request is 256. To allocate the left over 1
>>>> page, continues the iteration to find the order value which is 0
>>>> and when it compares with min_order = 8, triggers the BUG_ON(order
>>>> < min_order). To avoid this problem, we added a simple check to
>>>> return -EINVAL if size is not aligned to the min_page_size.
>>>>
>>>> v2: Added more details to the commit description
>>>>
>>>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>>>> Suggested-by: Matthew Auld <matthew.auld@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_buddy.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>>>> index 72f52f293249..b503c88786b0 100644
>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>> @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>>>      if (range_overflows(start, size, mm->size))
>>>>              return -EINVAL;
>>>>
>>>> +    if (WARN_ON(!IS_ALIGNED(size, min_page_size)))
>>>> +            return -EINVAL;
>>>> +
>>>
>>> I'm not that happy with the handling here.
>>>
>>> See a minimum page size larger than the requested size is perfectly
>>> valid, it just means that the remaining pages needs to be trimmed.
>>>
>>> In other words when the request is to allocate 1 page with an alignment
>>> of 256 we just need to give the remaining 255 pages back to the allocator.
>>
>> In one of the previous mail Matthew explained that i915 expects to
>> return -EINVAL error code if size is not aligned to min_page_size.
> 
> We could also move the WARN_ON() into i915 as a separate patch, and
> just change the default buddy behaviour to transparently handle the
> rounding + trim, if you prefer. I don't have a strong opinion.

ok, I sent a patch handling rounding + trim in drm_buddy_alloc_blocks()
function. Please review the patch.
> 
>>
>> can we just modify in amdgpu code where we round_up the size to the
>> min_page_size value and keep this error handling in drm_buddy.c?
>>>
>>> Regards,
>>> Christian.
>>>
>>>>      /* Actual range allocation */
>>>>      if (start + size == end)
>>>>              return __drm_buddy_alloc_range(mm, start, size, blocks);
>>>>
>>>> base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b
>>>