RE: [PATCH] drm/amdgpu: simplify vram alloc logic since 2GB limitation removed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux