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

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

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

As you have suggested we can also rename this as force merge or some 
other names.

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