On 5/22/25 05:43, 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 <emily.d...@amd.com>
> Signed-off-by: Samuel Zhang <guoqing.zh...@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 23 ++++++++++++++++++++--
> drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 13 ++++++++----
> 4 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index e5bb46effb6c..97389168c90f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5065,6 +5065,10 @@ static inline int amdgpu_virt_resume(struct
> amdgpu_device *adev)
> adev->vm_manager.vram_base_offset +=
> adev->gmc.xgmi.physical_node_id *
> adev->gmc.xgmi.node_segment_size;
>
> + /* fb_start/fb_end may be reset in get_fb_location(), set it again on
> resume */
> + adev->gmc.fb_start = 0;
> + adev->gmc.fb_end = adev->gmc.xgmi.node_segment_size *
> adev->gmc.xgmi.num_physical_nodes - 1;
Why do we need that in the first place? Updating the fb_start/end in
get_fb_location() is actually fine.
Or are the fb_start/fb_end values still used anywhere they shouldn't?
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index d1fa5e8e3937..35df4be6ef2a 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
> *
> @@ -251,7 +253,17 @@ void amdgpu_gmc_sysvm_location(struct amdgpu_device
> *adev, struct amdgpu_gmc *mc
> 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.
> + */
> + mc->vram_start = 0;
> + mc->vram_end = mc->vram_start + mc->mc_vram_size - 1;
> + if (mc->real_vram_size < mc->visible_vram_size)
> + mc->visible_vram_size = mc->real_vram_size;
Make that here mc->visible_vram_size = min(mc->visible_vram_size,
mc->real_vram_size).
> + }
> + /* 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;
> @@ -276,7 +288,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 +1079,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;
> + }
> +
Please make an else branch here for the XGMI connected GPU case, or maybe code
it like this:
/*
* ....
*/
if (!amdgpu_virt_xgmi_migrate_enabled(adev))
vram_addr -= adev->gmc.xgmi.physical_node_id *
adev->gmc.xgmi.node_segment_size
vram_end = vram_start + vram_size;
> + 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/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)) {
Better check for adev->gmc.xgmi.connected_to_cpu here.
Regards,
Christian.
> 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..23d02965ad65 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -413,6 +413,11 @@ static const uint32_t ecc_umc_mcumc_ctrl_mask_addrs[] = {
> (0x001d43e0 + 0x00001800),
> };
>
> +static inline bool gmc_v9_0_is_pdb0_enabled(struct amdgpu_device *adev)
> +{
> + return adev->gmc.xgmi.connected_to_cpu ||
> amdgpu_virt_xgmi_migrate_enabled(adev);
> +}
> +
> static inline bool gmc_v9_0_is_multi_chiplet(struct amdgpu_device *adev)
> {
> return !!adev->aid_mask;
> @@ -1726,7 +1731,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 (gmc_v9_0_is_pdb0_enabled(adev)) {
> amdgpu_gmc_sysvm_location(adev, mc);
> } else {
> amdgpu_gmc_vram_location(adev, mc, base);
> @@ -1841,7 +1846,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device
> *adev)
> return 0;
> }
>
> - if (adev->gmc.xgmi.connected_to_cpu) {
> + if (gmc_v9_0_is_pdb0_enabled(adev)) {
> adev->gmc.vmid0_page_table_depth = 1;
> adev->gmc.vmid0_page_table_block_size = 12;
> } else {
> @@ -1867,7 +1872,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device
> *adev)
> if (r)
> return r;
>
> - if (adev->gmc.xgmi.connected_to_cpu)
> + if (gmc_v9_0_is_pdb0_enabled(adev))
> r = amdgpu_gmc_pdb0_alloc(adev);
> }
>
> @@ -2372,7 +2377,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device
> *adev)
> {
> int r;
>
> - if (adev->gmc.xgmi.connected_to_cpu)
> + if (gmc_v9_0_is_pdb0_enabled(adev))
> amdgpu_gmc_init_pdb0(adev);
>
> if (adev->gart.bo == NULL) {
