[PATCH v8 3/3] drm/buddy: Add user for defragmentation

[Paneer Selvam, Arunpravin]

Hi Christian,

On 3/5/2024 5:41 PM, Christian König wrote:
> Am 05.03.24 um 12:14 schrieb Paneer Selvam, Arunpravin:
>> On 3/5/2024 4:33 PM, Paneer Selvam, Arunpravin wrote:
>>> Hi Christian,
>>>
>>> On 3/4/2024 10:09 PM, Christian König wrote:
>>>> Am 04.03.24 um 17:32 schrieb Arunpravin Paneer Selvam:
>>>>> Add amdgpu driver as user for the drm buddy
>>>>> defragmentation.
>>>>>
>>>>> Signed-off-by: Arunpravin Paneer Selvam 
>>>>> <Arunpravin.PaneerSelvam at amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 +++++++++++++++--
>>>>>   drivers/gpu/drm/drm_buddy.c                  |  1 +
>>>>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>> index e494f5bf136a..cff8a526c622 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>> @@ -533,8 +533,21 @@ static int amdgpu_vram_mgr_new(struct 
>>>>> ttm_resource_manager *man,
>>>>>                          min_block_size,
>>>>>                          &vres->blocks,
>>>>>                          vres->flags);
>>>>> -        if (unlikely(r))
>>>>> -            goto error_free_blocks;
>>>>> +        if (unlikely(r)) {
>>>>> +            if (r == -ENOSPC) {
>>>>> +                drm_buddy_defrag(mm, min_block_size);
>>>>> +                r = drm_buddy_alloc_blocks(mm, fpfn,
>>>>> +                               lpfn,
>>>>> +                               size,
>>>>> +                               min_block_size,
>>>>> +                               &vres->blocks,
>>>>> +                               vres->flags);
>>>>
>>>> That doesn't looks like something we should do.
>>>>
>>>> We might fallback when contiguous memory is requested, but 
>>>> certainly not on normal allocation failure.
>>> yes, defrag here not useful for normal allocations. But worried 
>>> about the bigger min_block_size normal allocations.
>>> In such cases, I think we should move this drm_buddy_defrag() call 
>>> into buddy allocator file. For example if the required
>>> size is 1024KiB and if min_block_size is 256KiB, the allocator first 
>>> tries to find the 1024KiB block, when there is no single 1024KiB block,
>>> the allocator goes one level below in freelist and tries to search 
>>> for two 512KiB blocks and goes on. At one point of time if we have 
>>> less space,
>>> we might go further levels below to search four 256KiB blocks to 
>>> satisfy the request.
>>>
>>> Assuming if the allocator cannot find the first 256KiB block, that 
>>> time I think we might need to merge the two 128KiB blocks
>>> through defragmentation function. And again for the second 256KiB 
>>> block, we might need to call the defragmentation again to
>>> merge two 128KiB blocks or four 64KiB blocks to form minimum 
>>> alignment size of 256KiB. This goes on for the third and fourth
>>> 256KiB blocks to complete the required size allocation of 1024KiB. 
>>> Please let me know if my understanding is not correct.
>
> I don't think we should do that. We essentially have to support two 
> different use cases:
>
> 1. Non contiguous allocation with 2MiB min_block_size for everything 
> larger than 2MiB. Using a block size as large as possible is 
> desirable, but not something we enforce.
>
> 2. Contiguous allocations for display, firmware etc.. Here we need to 
> enforce a large block size and can live with the additional overhead 
> caused by force merging.
Thanks. I will make the changes accordingly.
>
>>
>> As you have suggested we can also rename this as force merge or some 
>> other names.
>
> Yeah, but just an suggestion. You are way deeper in the code and 
> handling than I'm, so feel free to name it whatever you think fits best.
Sure :)

Thanks,
Arun.
>
> Regards,
> Christian.
>
>
>>
>> Thanks,
>> Arun.
>>>
>>> Thanks,
>>> Arun.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> +                if (unlikely(r))
>>>>> +                    goto error_free_blocks;
>>>>> +            } else {
>>>>> +                goto error_free_blocks;
>>>>> +            }
>>>>> +        }
>>>>>             if (size > remaining_size)
>>>>>               remaining_size = 0;
>>>>> diff --git a/drivers/gpu/drm/drm_buddy.c 
>>>>> b/drivers/gpu/drm/drm_buddy.c
>>>>> index 40131ed9b0cd..19440f8caec0 100644
>>>>> --- a/drivers/gpu/drm/drm_buddy.c
>>>>> +++ b/drivers/gpu/drm/drm_buddy.c
>>>>> @@ -396,6 +396,7 @@ void drm_buddy_defrag(struct drm_buddy *mm,
>>>>>           }
>>>>>       }
>>>>>   }
>>>>> +EXPORT_SYMBOL(drm_buddy_defrag);
>>>>>     /**
>>>>>    * drm_buddy_free_block - free a block
>>>>
>>>
>>
>