Re: [PATCH] drm/buddy: Add start address support to trim function

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 17/07/2024 16:02, Paneer Selvam, Arunpravin wrote:
> On 7/16/2024 3:34 PM, Matthew Auld wrote:
> > On 16/07/2024 10:50, Paneer Selvam, Arunpravin wrote:
> > > Hi Matthew,
> > >
> > > On 7/10/2024 6:20 PM, Matthew Auld wrote:
> > > > On 10/07/2024 07:03, Paneer Selvam, Arunpravin wrote:
> > > > > Thanks Alex.
> > > > >
> > > > > Hi Matthew,
> > > > > Any comments?
> > > >
> > > > Do we not pass the required address alignment when allocating the 
> > > > pages in the first place?
> > > If address alignment is really useful, we can add that in the 
> > > drm_buddy_alloc_blocks() function.
> >
> > I mean don't we already pass the min page size, which should give us 
> > matching physical address alignment?
> I think we don't need to align the address to the passed min_block_size 
> value for all the contiguous
> buffers, so I thought that decision we can leave it to the drivers and 
> they can achieve that through trim function
> in this kind of a specific request.

I would have assumed it would be simpler to use min_block_size and then 
trim the size, if it's too big? That would then also take care of the 
try_harder case?

Also how are we dealing with the multi-block try_harder case? AFAICT we 
only allow trimming single block atm, or is it not possible to trigger 
that path here? Or are we handling that somehow?

> https://patchwork.freedesktop.org/series/136150/
> We are getting this sparse error from the Intel CI. Do you think these 
> errors are introduced with this patches?

I think it's safe to ignore, there seem to be other series with the same 
thing.

> Thanks,
> Arun.
> > > Thanks,
> > > Arun.
> > > > > Thanks,
> > > > > Arun.
> > > > >
> > > > > On 7/9/2024 1:42 AM, Alex Deucher wrote:
> > > > > > On Thu, Jul 4, 2024 at 4:40 AM Arunpravin Paneer Selvam
> > > > > > <Arunpravin.PaneerSelvam@xxxxxxx> wrote:
> > > > > > > - Add a new start parameter in trim function to specify exact
> > > > > > >    address from where to start the trimming. This would help us
> > > > > > >    in situations like if drivers would like to do address alignment
> > > > > > >    for specific requirements.
> > > > > > >
> > > > > > > - Add a new flag DRM_BUDDY_TRIM_DISABLE. Drivers can use this
> > > > > > >    flag to disable the allocator trimming part. This patch enables
> > > > > > >    the drivers control trimming and they can do it themselves
> > > > > > >    based on the application requirements.
> > > > > > >
> > > > > > > v1:(Matthew)
> > > > > > >    - check new_start alignment with min chunk_size
> > > > > > >    - use range_overflows()
> > > > > > >
> > > > > > > Signed-off-by: Arunpravin Paneer Selvam 
> > > > > > > <Arunpravin.PaneerSelvam@xxxxxxx>
> > > > > > Series is:
> > > > > > Acked-by: Alex Deucher <alexander.deucher@xxxxxxx>
> > > > > >
> > > > > > I'd like to take this series through the amdgpu tree if there are no
> > > > > > objections as it's required for display buffers on some chips and I'd
> > > > > > like to make sure it lands in 6.11.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Alex
> > > > > >
> > > > > > > ---
> > > > > > >   drivers/gpu/drm/drm_buddy.c          | 25 
> > > > > > > +++++++++++++++++++++++--
> > > > > > >   drivers/gpu/drm/xe/xe_ttm_vram_mgr.c |  2 +-
> > > > > > >   include/drm/drm_buddy.h              |  2 ++
> > > > > > >   3 files changed, 26 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/drm_buddy.c 
> > > > > > > b/drivers/gpu/drm/drm_buddy.c
> > > > > > > index 94f8c34fc293..8cebe1fa4e9d 100644
> > > > > > > --- a/drivers/gpu/drm/drm_buddy.c
> > > > > > > +++ b/drivers/gpu/drm/drm_buddy.c
> > > > > > > @@ -851,6 +851,7 @@ static int __alloc_contig_try_harder(struct 
> > > > > > > drm_buddy *mm,
> > > > > > >    * drm_buddy_block_trim - free unused pages
> > > > > > >    *
> > > > > > >    * @mm: DRM buddy manager
> > > > > > > + * @start: start address to begin the trimming.
> > > > > > >    * @new_size: original size requested
> > > > > > >    * @blocks: Input and output list of allocated blocks.
> > > > > > >    * MUST contain single block as input to be trimmed.
> > > > > > > @@ -866,11 +867,13 @@ static int __alloc_contig_try_harder(struct 
> > > > > > > drm_buddy *mm,
> > > > > > >    * 0 on success, error code on failure.
> > > > > > >    */
> > > > > > >   int drm_buddy_block_trim(struct drm_buddy *mm,
> > > > > > > +                        u64 *start,
> > > > > > >                           u64 new_size,
> > > > > > >                           struct list_head *blocks)
> > > > > > >   {
> > > > > > >          struct drm_buddy_block *parent;
> > > > > > >          struct drm_buddy_block *block;
> > > > > > > +       u64 block_start, block_end;
> > > > > > >          LIST_HEAD(dfs);
> > > > > > >          u64 new_start;
> > > > > > >          int err;
> > > > > > > @@ -882,6 +885,9 @@ int drm_buddy_block_trim(struct drm_buddy *mm,
> > > > > > >                                   struct drm_buddy_block,
> > > > > > >                                   link);
> > > > > > >
> > > > > > > +       block_start = drm_buddy_block_offset(block);
> > > > > > > +       block_end = block_start + drm_buddy_block_size(mm, block);
> > > > > > > +
> > > > > > >          if (WARN_ON(!drm_buddy_block_is_allocated(block)))
> > > > > > >                  return -EINVAL;
> > > > > > >
> > > > > > > @@ -894,6 +900,20 @@ int drm_buddy_block_trim(struct drm_buddy *mm,
> > > > > > >          if (new_size == drm_buddy_block_size(mm, block))
> > > > > > >                  return 0;
> > > > > > >
> > > > > > > +       new_start = block_start;
> > > > > > > +       if (start) {
> > > > > > > +               new_start = *start;
> > > > > > > +
> > > > > > > +               if (new_start < block_start)
> > > > > > > +                       return -EINVAL;
> > > > > > > +
> > > > > > > +               if (!IS_ALIGNED(new_start, mm->chunk_size))
> > > > > > > +                       return -EINVAL;
> > > > > > > +
> > > > > > > +               if (range_overflows(new_start, new_size, block_end))
> > > > > > > +                       return -EINVAL;
> > > > > > > +       }
> > > > > > > +
> > > > > > >          list_del(&block->link);
> > > > > > >          mark_free(mm, block);
> > > > > > >          mm->avail += drm_buddy_block_size(mm, block);
> > > > > > > @@ -904,7 +924,6 @@ int drm_buddy_block_trim(struct drm_buddy *mm,
> > > > > > >          parent = block->parent;
> > > > > > >          block->parent = NULL;
> > > > > > >
> > > > > > > -       new_start = drm_buddy_block_offset(block);
> > > > > > >          list_add(&block->tmp_link, &dfs);
> > > > > > >          err =  __alloc_range(mm, &dfs, new_start, new_size, 
> > > > > > > blocks, NULL);
> > > > > > >          if (err) {
> > > > > > > @@ -1066,7 +1085,8 @@ int drm_buddy_alloc_blocks(struct drm_buddy 
> > > > > > > *mm,
> > > > > > >          } while (1);
> > > > > > >
> > > > > > >          /* Trim the allocated block to the required size */
> > > > > > > -       if (original_size != size) {
> > > > > > > +       if (!(flags & DRM_BUDDY_TRIM_DISABLE) &&
> > > > > > > +           original_size != size) {
> > > > > > >                  struct list_head *trim_list;
> > > > > > >                  LIST_HEAD(temp);
> > > > > > >                  u64 trim_size;
> > > > > > > @@ -1083,6 +1103,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy 
> > > > > > > *mm,
> > > > > > >                  }
> > > > > > >
> > > > > > >                  drm_buddy_block_trim(mm,
> > > > > > > +                                    NULL,
> > > > > > >                                       trim_size,
> > > > > > >                                       trim_list);
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c 
> > > > > > > b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > > > > > > index fe3779fdba2c..423b261ea743 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > > > > > > @@ -150,7 +150,7 @@ static int xe_ttm_vram_mgr_new(struct 
> > > > > > > ttm_resource_manager *man,
> > > > > > >          } while (remaining_size);
> > > > > > >
> > > > > > >          if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
> > > > > > > -               if (!drm_buddy_block_trim(mm, vres->base.size, 
> > > > > > > &vres->blocks))
> > > > > > > +               if (!drm_buddy_block_trim(mm, NULL, 
> > > > > > > vres->base.size, &vres->blocks))
> > > > > > >                          size = vres->base.size;
> > > > > > >          }
> > > > > > >
> > > > > > > diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
> > > > > > > index 82570f77e817..0c2f735f0265 100644
> > > > > > > --- a/include/drm/drm_buddy.h
> > > > > > > +++ b/include/drm/drm_buddy.h
> > > > > > > @@ -27,6 +27,7 @@
> > > > > > >   #define DRM_BUDDY_CONTIGUOUS_ALLOCATION BIT(2)
> > > > > > >   #define DRM_BUDDY_CLEAR_ALLOCATION             BIT(3)
> > > > > > >   #define DRM_BUDDY_CLEARED                      BIT(4)
> > > > > > > +#define DRM_BUDDY_TRIM_DISABLE                 BIT(5)
> > > > > > >
> > > > > > >   struct drm_buddy_block {
> > > > > > >   #define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
> > > > > > > @@ -155,6 +156,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
> > > > > > >                             unsigned long flags);
> > > > > > >
> > > > > > >   int drm_buddy_block_trim(struct drm_buddy *mm,
> > > > > > > +                        u64 *start,
> > > > > > >                           u64 new_size,
> > > > > > >                           struct list_head *blocks);
> > > > > > >
> > > > > > > --
> > > > > > > 2.25.1