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

["Christian König" <christian.koenig@amd.com>]

Am 26.12.21 um 21:59 schrieb Arunpravin:
> 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

I would prefer drm_buddy as prefix as well and I think we could rather 
drop the _mm postfix from the structure here.

Cause what we try to manage is not necessary memory, but rather address 
space.

Christian.

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