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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 03.01.22 um 08:41 schrieb Christian König:
> 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@xxxxxxx>
> > <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.

I was mostly concerned about mismatching names for functions and 
structures. If drm_buddy_ (without mm) for all naming is preferred, I 
wouldn't mind.

Best regards
Thomas

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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature