Re: [PATCH 2/2] drm/amdgpu: Add stolen reserved memory for MI25 SRIOV.

[Alex Deucher <alexdeucher@gmail.com>]

On Mon, Mar 14, 2022 at 3:41 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 14.03.22 um 19:54 schrieb Yongqiang Sun:
> > [Why]
> > MI25 SRIOV guest driver loading failed due to allocate
> > memory overlaps with private memory area.
> >
> > [How]
> > 1. Allocate stolen reserved memory for MI25 SRIOV specifically to avoid
> > the memory overlap.
> > 2. Move allocate reserve allocation to vbios allocation since both the
> > two functions are doing similar asic type check and no need to have
> > seperate functions.
> >
> > Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
> > Change-Id: I142127513047a3e81573eb983c510d763b548a24
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 37 ++++++++++++-------------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |  1 -
> >   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  |  1 -
> >   3 files changed, 18 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > index 7c2a9555b7cc..f7f4f00dd2b2 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > @@ -626,6 +626,11 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
> >   {
> >       unsigned size;
> >
> > +     /* Some ASICs need to reserve a region of video memory to avoid access
> > +      * from driver */
> > +     adev->mman.stolen_reserved_offset = 0;
> > +     adev->mman.stolen_reserved_size = 0;
> > +
> >       /*
> >        * TODO:
> >        * Currently there is a bug where some memory client outside
> > @@ -635,11 +640,24 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
> >        * Keep the stolen memory reservation until the while this is not solved.
> >        */
> >       switch (adev->asic_type) {
> > +
> >       case CHIP_VEGA10:
>
> Please don't add empty lines between switch and case. Good practice is
> to check your patches with checkpatch.pl before sending it out.
>
> > +             adev->mman.keep_stolen_vga_memory = true;
> > +             if (amdgpu_sriov_vf(adev)) {
> > +                     adev->mman.stolen_reserved_offset = 0x100000;
> > +                     adev->mman.stolen_reserved_size = 0x600000;
> > +             }
> > +             break;
> >       case CHIP_RAVEN:
> >       case CHIP_RENOIR:
> >               adev->mman.keep_stolen_vga_memory = true;
> >               break;
> > +     case CHIP_YELLOW_CARP:
> > +             if (amdgpu_discovery == 0) {
> > +                     adev->mman.stolen_reserved_offset = 0x1ffb0000;
> > +                     adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
> > +             }
> > +             break;
>
> That looks like this is somehow mixed up. The stolen memory is for VGA
> emulation, but under SRIOV we should not have VGA emulation as far as I
> know.
>
> Alex, what's going on here?

I suggested calling amdgpu_gmc_get_reserved_allocation() from
amdgpu_gmc_get_vbios_allocations() rather calling
amdgpu_gmc_get_reserved_allocation() in every gmc file since it
handles additional reserved areas used allocated by firmware, etc.  My
understanding is that there is some additional reserved area set up in
the hypervisor that we want reserved in the guest.  The idea was to
just use stolen_reserved_offset for that similar to what we do for the
bring up case for yellow carp.

Alex


>
> Regards,
> Christian.
>
> >       default:
> >               adev->mman.keep_stolen_vga_memory = false;
> >               break;
> > @@ -760,25 +778,6 @@ uint64_t amdgpu_gmc_vram_cpu_pa(struct amdgpu_device *adev, struct amdgpu_bo *bo
> >       return amdgpu_bo_gpu_offset(bo) - adev->gmc.vram_start + adev->gmc.aper_base;
> >   }
> >
> > -void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev)
> > -{
> > -     /* Some ASICs need to reserve a region of video memory to avoid access
> > -      * from driver */
> > -     adev->mman.stolen_reserved_offset = 0;
> > -     adev->mman.stolen_reserved_size = 0;
> > -
> > -     switch (adev->asic_type) {
> > -     case CHIP_YELLOW_CARP:
> > -             if (amdgpu_discovery == 0) {
> > -                     adev->mman.stolen_reserved_offset = 0x1ffb0000;
> > -                     adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
> > -             }
> > -             break;
> > -     default:
> > -             break;
> > -     }
> > -}
> > -
> >   int amdgpu_gmc_vram_checking(struct amdgpu_device *adev)
> >   {
> >       struct amdgpu_bo *vram_bo = NULL;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > index 93505bb0a36c..032b0313f277 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > @@ -331,7 +331,6 @@ amdgpu_gmc_set_vm_fault_masks(struct amdgpu_device *adev, int hub_type,
> >                             bool enable);
> >
> >   void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev);
> > -void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev);
> >
> >   void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev);
> >   uint64_t amdgpu_gmc_vram_mc2pa(struct amdgpu_device *adev, uint64_t mc_addr);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> > index f60b7bd4dbf5..3c1d440824a7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> > @@ -948,7 +948,6 @@ static int gmc_v10_0_sw_init(void *handle)
> >               return r;
> >
> >       amdgpu_gmc_get_vbios_allocations(adev);
> > -     amdgpu_gmc_get_reserved_allocation(adev);
> >
> >       /* Memory manager */
> >       r = amdgpu_bo_init(adev);
>