Re: [Intel-gfx] [PATCH v4 2/6] drm: improve drm_buddy_alloc function

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

Hi Thomas

On 16/12/21 5:05 pm, Thomas Zimmermann wrote:
> Hi
> 
> Am 01.12.21 um 17:39 schrieb Arunpravin:
>> - Make drm_buddy_alloc a single function to handle
>>    range allocation and non-range allocation demands
>>
>> - Implemented a new function alloc_range() which allocates
>>    the requested power-of-two block comply with range limitations
>>
>> - Moved order computation and memory alignment logic from
>>    i915 driver to drm buddy
>>
>> v2:
>>    merged below changes to keep the build unbroken
>>     - drm_buddy_alloc_range() becomes obsolete and may be removed
>>     - enable ttm range allocation (fpfn / lpfn) support in i915 driver
>>     - apply enhanced drm_buddy_alloc() function to i915 driver
>>
>> v3(Matthew Auld):
>>    - Fix alignment issues and remove unnecessary list_empty check
>>    - add more validation checks for input arguments
>>    - make alloc_range() block allocations as bottom-up
>>    - optimize order computation logic
>>    - replace uint64_t with u64, which is preferred in the kernel
>>
>> v4(Matthew Auld):
>>    - keep drm_buddy_alloc_range() function implementation for generic
>>      actual range allocations
>>    - keep alloc_range() implementation for end bias allocations
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>

<SNIP>

>> +#define DRM_BUDDY_RANGE_ALLOCATION (1 << 0)
>> +
>>   struct drm_buddy_block {
>>   #define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
>>   #define DRM_BUDDY_HEADER_STATE  GENMASK_ULL(11, 10)
>> @@ -132,12 +139,11 @@ int drm_buddy_init(struct drm_buddy_mm *mm, u64 size, u64 chunk_size);
>>   
>>   void drm_buddy_fini(struct drm_buddy_mm *mm);
>>   
>> -struct drm_buddy_block *
>> -drm_buddy_alloc(struct drm_buddy_mm *mm, unsigned int order);
> 
> Just a style issue. The structure is called drm_buddy_mm. For 
> consistency, I like to suggest to name all the public interfaces and 
> defines drm_buddy_mm_* instead of just drm_buddy_*.
> 
Thanks for the suggestion, I think renaming drm_buddy_* to
drm_buddy_mm_* creates a long name for the public interfaces, for
instance - drm_buddy_mm_alloc_blocks(),
discussing the style issue internally
@Matthew, @christian - please let me know your opinion

> 
>> -
>> -int drm_buddy_alloc_range(struct drm_buddy_mm *mm,
>> -			  struct list_head *blocks,
>> -			  u64 start, u64 size);
>> +int drm_buddy_alloc(struct drm_buddy_mm *mm,
>> +		    u64 start, u64 end, u64 size,
>> +		    u64 min_page_size,
>> +		    struct list_head *blocks,
>> +		    unsigned long flags);
>>   
>>   void drm_buddy_free(struct drm_buddy_mm *mm, struct drm_buddy_block *block);
> 
> I'd call those *_alloc_blocks() and _free_blocks(). Right now it sounds 
> as if they allocate and free instances of drm_buddy_mm.
can we call those drm_buddy_alloc_blocks() and drm_buddy_free_blocks()
Does this make sense?
> 
> Best regards
> Thomas
>>   
>>
>