On Thu, Sep 26, 2024 at 10:45 AM Zhang, Yifan <Yifan1.Zhang@xxxxxxx> wrote:
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher@xxxxxxxxx>
> Sent: Thursday, September 26, 2024 1:17 AM
> To: Zhang, Yifan <Yifan1.Zhang@xxxxxxx>
> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Yang, Philip <Philip.Yang@xxxxxxx>; Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> Subject: Re: [PATCH] drm/amdgpu: simplify vram alloc logic since 2GB limitation removed
>
> On Tue, Sep 24, 2024 at 10:22 AM Zhang, Yifan <Yifan1.Zhang@xxxxxxx> wrote:
> >
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > 2GB limitation in VRAM allocation is removed in below patch. My patch is a follow up refine for this. The remaing_size calculation was to address the 2GB limitation in contiguous VRAM allocation, and no longer needed after the limitation is removed.
> >
>
> Thanks. Would be good to add a reference to this commit in the commit message so it's clear what you are talking about and also provide a bit more of a description about why this can be cleaned up (like you did above). One other comment below as well.
>
> Sure, I will send patch V2 to change commit message.
>
> > commit b2dba064c9bdd18c7dd39066d25453af28451dbf
> > Author: Philip Yang <Philip.Yang@xxxxxxx>
> > Date: Fri Apr 19 16:27:00 2024 -0400
> >
> > drm/amdgpu: Handle sg size limit for contiguous allocation
> >
> > Define macro AMDGPU_MAX_SG_SEGMENT_SIZE 2GB, because struct scatterlist
> > length is unsigned int, and some users of it cast to a signed int, so
> > every segment of sg table is limited to size 2GB maximum.
> >
> > For contiguous VRAM allocation, don't limit the max buddy block size in
> > order to get contiguous VRAM memory. To workaround the sg table segment
> > size limit, allocate multiple segments if contiguous size is bigger than
> > AMDGPU_MAX_SG_SEGMENT_SIZE.
> >
> > Signed-off-by: Philip Yang <Philip.Yang@xxxxxxx>
> > Reviewed-by: Christian König <christian.koenig@xxxxxxx>
> > Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
> >
> >
> >
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@xxxxxxxxx>
> > Sent: Tuesday, September 24, 2024 10:07 PM
> > To: Zhang, Yifan <Yifan1.Zhang@xxxxxxx>
> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Yang, Philip <Philip.Yang@xxxxxxx>;
> > Kuehling, Felix <Felix.Kuehling@xxxxxxx>
> > Subject: Re: [PATCH] drm/amdgpu: simplify vram alloc logic since 2GB
> > limitation removed
> >
> > On Mon, Sep 23, 2024 at 4:28 AM Yifan Zhang <yifan1.zhang@xxxxxxx> wrote:
> > >
> > > Make vram alloc loop simpler after 2GB limitation removed.
> >
> > Can you provide more context? What 2GB limitation are you referring to?
> >
> > Alex
> >
> > >
> > > Signed-off-by: Yifan Zhang <yifan1.zhang@xxxxxxx>
> > > ---
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 15 +++++----------
> > > 1 file changed, 5 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > > index 7d26a962f811..3d129fd61fa7 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > > @@ -455,7 +455,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
> > > struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
> > > u64 vis_usage = 0, max_bytes, min_block_size;
> > > struct amdgpu_vram_mgr_resource *vres;
> > > - u64 size, remaining_size, lpfn, fpfn;
> > > + u64 size, lpfn, fpfn;
> > > unsigned int adjust_dcc_size = 0;
> > > struct drm_buddy *mm = &mgr->mm;
> > > struct drm_buddy_block *block; @@ -516,25 +516,23 @@ static
> > > int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
> > > adev->gmc.gmc_funcs->get_dcc_alignment)
> > > adjust_dcc_size =
> > > amdgpu_gmc_get_dcc_alignment(adev);
> > >
> > > - remaining_size = (u64)vres->base.size;
> > > + size = (u64)vres->base.size;
> > > if (bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS && adjust_dcc_size) {
> > > unsigned int dcc_size;
> > >
> > > dcc_size = roundup_pow_of_two(vres->base.size + adjust_dcc_size);
> > > - remaining_size = (u64)dcc_size;
> > > + size = (u64)dcc_size;
> > >
> > > vres->flags |= DRM_BUDDY_TRIM_DISABLE;
> > > }
> > >
> > > mutex_lock(&mgr->lock);
> > > - while (remaining_size) {
> > > + while (true) {
>
> I think you can also drop this while loop now too.
>
> Alex
>
> There is still a "continue" path left in the loop, I think the logic may be broken here if loop dropped.
Ah, yes, sorry I missed that continue.
Alex
>
> r = drm_buddy_alloc_blocks(mm, fpfn,
> lpfn,
> size,
> min_block_size,
> &vres->blocks,
> vres->flags);
>
> if (unlikely(r == -ENOSPC) && pages_per_block == ~0ul &&
> !(place->flags & TTM_PL_FLAG_CONTIGUOUS)) {
> vres->flags &= ~DRM_BUDDY_CONTIGUOUS_ALLOCATION;
> pages_per_block = max_t(u32, 2UL << (20UL - PAGE_SHIFT),
> tbo->page_alignment);
>
> continue; /* continue in the loop */
> }
>
>
> > > if (tbo->page_alignment)
> > > min_block_size = (u64)tbo->page_alignment << PAGE_SHIFT;
> > > else
> > > min_block_size = mgr->default_page_size;
> > >
> > > - size = remaining_size;
> > > -
> > > if (bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS && adjust_dcc_size)
> > > min_block_size = size;
> > > else if ((size >= (u64)pages_per_block <<
> > > PAGE_SHIFT) && @@ -562,10 +560,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
> > > if (unlikely(r))
> > > goto error_free_blocks;
> > >
> > > - if (size > remaining_size)
> > > - remaining_size = 0;
> > > - else
> > > - remaining_size -= size;
> > > + break;
> > > }
> > > mutex_unlock(&mgr->lock);
> > >
> > > --
> > > 2.43.0
> > >
