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