<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#467886" vlink="#96607D" style="word-wrap:break-word"> [AMD Official Use Only - AMD Internal Distribution Only] <div> <div class="WordSection1"> Ping… <a id="OWAAME01E8261B08740759D04BBF623CC8D2E" href="mailto:Christian.Koenig@amd.com"> @Koenig, Christian</a> kindly pls review and feedback… thanks you very much!<o:p></o:p> <o:p> </o:p> <div> <o:p> </o:p> Rgds/Owen<o:p></o:p> </div> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com> Sent: Tuesday, May 20, 2025 1:11 PM To: Koenig, Christian <Christian.Koenig@amd.com>; Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>; amd-gfx@lists.freedesktop.org Cc: Zhao, Victor <Victor.Zhao@amd.com>; Chang, HaiJun <HaiJun.Chang@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>; Ma, Qing (Mark) <Qing.Ma@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Deng, Emily <Emily.Deng@amd.com> Subject: Re: [PATCH v6 3/4] drm/amdgpu: enable pdb0 for hibernation on SRIOV<o:p></o:p> </div> </div> <o:p> </o:p> <div> On 2025/5/19 21:57, Christian König wrote: > On 5/19/25 10:20, Samuel Zhang wrote: >> When switching to new GPU index after hibernation and then resume, >> VRAM offset of each VRAM BO will be changed, and the cached gpu >> addresses needed to updated. >> >> This is to enable pdb0 and switch to use pdb0-based virtual gpu >> address by default in amdgpu_bo_create_reserved(). since the virtual >> addresses do not change, this can avoid the need to update all >> cached gpu addresses all over the codebase. >> >> Signed-off-by: Emily Deng <<a href="mailto:Emily.Deng@amd.com">Emily.Deng@amd.com</a>> >> Signed-off-by: Samuel Zhang <<a href="mailto:guoqing.zhang@amd.com">guoqing.zhang@amd.com</a>> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 32 ++++++++++++++++++------ >> drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h | 1 + >> drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c | 2 +- >> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 10 +++++--- >> drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c | 6 +++-- >> 5 files changed, 37 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c >> index d1fa5e8e3937..265d6c777af5 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c >> @@ -38,6 +38,8 @@ >> #include <drm/drm_drv.h> >> #include <drm/ttm/ttm_tt.h> >> >> +static const u64 four_gb = 0x100000000ULL; >> + >> /** >> * amdgpu_gmc_pdb0_alloc - allocate vram for pdb0 >> * >> @@ -249,15 +251,24 @@ void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc >> { >> u64 hive_vram_start = 0; >> u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1; >> - mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id; >> - mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1; >> - mc->gart_start = hive_vram_end + 1; >> + >> + if (amdgpu_virt_xgmi_migrate_enabled(adev)) { >> + /* set mc->vram_start to 0 to switch the returned GPU address of >> + * amdgpu_bo_create_reserved() from FB aperture to GART aperture. >> + */ >> + amdgpu_gmc_vram_location(adev, mc, 0); > This function does a lot more than just setting mc->vram_start and mc->vram_end. > > You should probably just update the two setting and not call amdgpu_gmc_vram_location() at all. I tried only setting mc->vram_start and mc->vram_end. But KMD load will fail with following error logs. [ 329.314346] amdgpu 0000:09:00.0: amdgpu: VRAM: 196288M 0x0000000000000000 - 0x0000002FEBFFFFFF (196288M used) [ 329.314348] amdgpu 0000:09:00.0: amdgpu: GART: 512M 0x0000018000000000 - 0x000001801FFFFFFF [ 329.314385] [drm] Detected VRAM RAM=196288M, BAR=262144M [ 329.314386] [drm] RAM width 8192bits HBM [ 329.314546] amdgpu 0000:09:00.0: amdgpu: (-22) failed to allocate kernel bo [ 329.315013] [drm:amdgpu_device_init [amdgpu]] *ERROR* sw_init of IP block <gmc_v9_0> failed -22 [ 329.315846] amdgpu 0000:09:00.0: amdgpu: amdgpu_device_ip_init failed It seems like setting mc->visible_vram_size and mc->visible_vram_size fields are also needed. In this case call amdgpu_gmc_vram_location() is better than inline the logic, I think. > >> + } else { >> + mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id; >> + mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1; >> + dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n", >> + mc->mc_vram_size >> 20, mc->vram_start, >> + mc->vram_end, mc->real_vram_size >> 20); >> + } >> + /* node_segment_size may not 4GB aligned on SRIOV, align up is needed. */ >> + mc->gart_start = ALIGN(hive_vram_end + 1, four_gb); >> mc->gart_end = mc->gart_start + mc->gart_size - 1; >> mc->fb_start = hive_vram_start; >> mc->fb_end = hive_vram_end; >> - dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n", >> - mc->mc_vram_size >> 20, mc->vram_start, >> - mc->vram_end, mc->real_vram_size >> 20); >> dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n", >> mc->gart_size >> 20, mc->gart_start, mc->gart_end); >> } >> @@ -276,7 +287,6 @@ void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc >> void amdgpu_gmc_gart_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc, >> enum amdgpu_gart_placement gart_placement) >> { >> - const uint64_t four_gb = 0x100000000ULL; >> u64 size_af, size_bf; >> /*To avoid the hole, limit the max mc address to AMDGPU_GMC_HOLE_START*/ >> u64 max_mc_address = min(adev->gmc.mc_mask, AMDGPU_GMC_HOLE_START - 1); >> @@ -1068,6 +1078,14 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev) >> flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1)); >> flags |= AMDGPU_PDE_PTE_FLAG(adev); >> >> + if (amdgpu_virt_xgmi_migrate_enabled(adev)) { >> + /* always start from current device so that the GART address can keep >> + * consistent when hibernate-resume with different GPUs. >> + */ >> + vram_addr = adev->vm_manager.vram_base_offset; >> + vram_end = vram_addr + vram_size; >> + } >> + >> /* The first n PDE0 entries are used as PTE, >> * pointing to vram >> */ >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h >> index bd7fc123b8f9..46fac7ca7dfa 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h >> @@ -307,6 +307,7 @@ struct amdgpu_gmc { >> struct amdgpu_bo *pdb0_bo; >> /* CPU kmapped address of pdb0*/ >> void *ptr_pdb0; >> + bool pdb0_enabled; > This isn't needed, just always check (adev->gmc.xgmi.connected_to_cpu || amdgpu_virt_xgmi_migrate_enabled(adev)), make a function for that if necessary. Ok, I will update it in the next patch version. > >> >> /* MALL size */ >> u64 mall_size; >> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c >> index cb25f7f0dfc1..e6165f6d0763 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c >> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c >> @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev, >> /* In the case squeezing vram into GART aperture, we don't use >> * FB aperture and AGP aperture. Disable them. >> */ >> - if (adev->gmc.pdb0_bo) { >> + if (adev->gmc.pdb0_bo && !amdgpu_virt_xgmi_migrate_enabled(adev)) { >> WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0); >> WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF); >> WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0); >> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c >> index 59385da80185..04fb99c64b37 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c >> @@ -1682,6 +1682,8 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block) >> adev->gmc.private_aperture_start + (4ULL << 30) - 1; >> adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF; >> >> + adev->gmc.pdb0_enabled = adev->gmc.xgmi.connected_to_cpu || >> + amdgpu_virt_xgmi_migrate_enabled(adev); >> return 0; >> } >> >> @@ -1726,7 +1728,7 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev, >> >> /* add the xgmi offset of the physical node */ >> base += adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size; >> - if (adev->gmc.xgmi.connected_to_cpu) { >> + if (adev->gmc.pdb0_enabled) { >> amdgpu_gmc_sysvm_location(adev, mc); >> } else { >> amdgpu_gmc_vram_location(adev, mc, base); >> @@ -1841,7 +1843,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev) >> return 0; >> } >> >> - if (adev->gmc.xgmi.connected_to_cpu) { >> + if (adev->gmc.pdb0_enabled) { >> adev->gmc.vmid0_page_table_depth = 1; >> adev->gmc.vmid0_page_table_block_size = 12; >> } else { >> @@ -1867,7 +1869,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev) >> if (r) >> return r; >> >> - if (adev->gmc.xgmi.connected_to_cpu) >> + if (adev->gmc.pdb0_enabled) >> r = amdgpu_gmc_pdb0_alloc(adev); >> } >> >> @@ -2372,7 +2374,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev) >> { >> int r; >> >> - if (adev->gmc.xgmi.connected_to_cpu) >> + if (adev->gmc.pdb0_enabled) >> amdgpu_gmc_init_pdb0(adev); >> >> if (adev->gart.bo == NULL) { >> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c >> index 84cde1239ee4..18e80aa78aff 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c >> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_8.c >> @@ -45,8 +45,10 @@ static u64 mmhub_v1_8_get_fb_location(struct amdgpu_device *adev) >> top &= MC_VM_FB_LOCATION_TOP__FB_TOP_MASK; >> top <<= 24; >> >> - adev->gmc.fb_start = base; >> - adev->gmc.fb_end = top; >> + if (!amdgpu_virt_xgmi_migrate_enabled(adev)) { >> + adev->gmc.fb_start = base; >> + adev->gmc.fb_end = top; >> + } > We should probably avoid calling this in the first place. > > The function gmc_v9_0_vram_gtt_location() should probably be adjusted. mmhub_v1_8_get_fb_location() is called by the new amdgpu_bo_fb_aper_addr() as well, not just gmc_v9_0_vram_gtt_location(). mmhub_v1_8_get_fb_location() is supposed to be a query api according to its name. having such side effect is very surprising. Another approach is set the right fb_start and fb_end in the new amdgpu_virt_resume(), like updating vram_base_offset. Which approach do you prefer? Or any better suggestions? Thank you. Regards Sam > > Regards, > Christian. > >> >> return base; >> }<o:p></o:p> </div> </div> </div> </body> </html>