<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="en-CN" link="#467886" vlink="#96607D" style="word-wrap:break-word"> <div class="WordSection1"> <div> On 2025/5/8 16:12, Lazar, Lijo wrote: > On 5/8/2025 10:39 AM, Samuel Zhang wrote: >> For virtual machine with vGPUs in SRIOV single device mode and XGMI >> is enabled, XGMI physical node ids may change when waking up from >> hiberation with different vGPU devices. So update XGMI info on resume. >> >> Signed-off-by: Jiang Liu<gerry@linux.alibaba.com> >> Signed-off-by: Samuel Zhang<guoqing.zhang@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 24 ++++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 4 ++++ >> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 6 ++++++ >> 3 files changed, 34 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index d477a901af84..843a3b0a9a07 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -4478,6 +4478,7 @@ int amdgpu_device_init(struct amdgpu_device *adev, >> r = adev->gfxhub.funcs->get_xgmi_info(adev); >> if (r) >> return r; >> + adev->gmc.xgmi.prev_physical_node_id = adev->gmc.xgmi.physical_node_id; >> } >> >> /* enable PCIE atomic ops */ >> @@ -5040,6 +5041,26 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients) >> return 0; >> } >> >> +static int amdgpu_device_update_xgmi_info(struct amdgpu_device *adev) >> +{ >> + int r; >> + >> + /* Get xgmi info again for sriov to detect device changes */ >> + if (amdgpu_sriov_vf(adev) && >> + !(adev->flags & AMD_IS_APU) && >> + adev->gmc.xgmi.supported && >> + !adev->gmc.xgmi.connected_to_cpu) { >> + adev->gmc.xgmi.prev_physical_node_id = adev->gmc.xgmi.physical_node_id; >> + r = adev->gfxhub.funcs->get_xgmi_info(adev); >> + if (r) >> + return r; >> + >> + dev_info(adev->dev, "xgmi node, old id %d, new id %d\n", >> + adev->gmc.xgmi.prev_physical_node_id, adev->gmc.xgmi.physical_node_id); >> + } >> + return 0; >> +} >> + >> /** >> * amdgpu_device_resume - initiate device resume >> * >> @@ -5059,6 +5080,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool notify_clients) >> r = amdgpu_virt_request_full_gpu(adev, true); >> if (r) >> return r; >> + r = amdgpu_device_update_xgmi_info(adev); >> + if (r) >> + return r; >> } >> >> if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h >> index 32dabba4062f..1387901576f1 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h >> @@ -89,6 +89,7 @@ struct amdgpu_xgmi { >> u64 node_segment_size; >> /* physical node (0-3) */ >> unsigned physical_node_id; >> + unsigned prev_physical_node_id; >> /* number of nodes (0-4) */ >> unsigned num_physical_nodes; >> /* gpu list in the same hive */ >> @@ -101,6 +102,9 @@ struct amdgpu_xgmi { >> uint8_t max_width; >> }; >> >> +#define amdgpu_xmgi_is_node_changed(adev) \ > Typo - xgmi > >> + (adev->gmc.xgmi.prev_physical_node_id != adev->gmc.xgmi.physical_node_id) > Since prev_physical_node_id is updated only for VF, the check should be > there here as well. > > Otherwise, you may have something like below in > amdgpu_device_update_xgmi_info() > > amdgpu_xgmi.node_changed = false; > if (check_condition) { > prev_node = adev->gmc.xgmi.physical_node_id; > adev->gfxhub.funcs->get_xgmi_info(adev) > amdgpu_xgmi.node_changed = (prev_node != adev->gmc.xgmi.physical_node_id); > } > > To make it clearer - > > Would still prefer to wrap under amdgpu_virt_migration_xyz() to make it > clear that this is done for node migration. > > Ex: > > bool amdgpu_virt_migration_detected() > { > return amdgpu_xgmi.node_changed; // And any other combination checks > which could up in future. > } > > The check needs to be done for any further changes down the series like > > if (amdgpu_virt_migration_detected()) > psp_update_gpu_addresses(); psp_update_gpu_addresses() and other gpu address updating logic will always needed when pdb0 is enabled, not just when detecting xgmi node changed. Because when pdb0 is enabled, the returned gpu addr from amdgpu_bo_create_reserved() will be in GART aperture, which is not compatible with PSP and SMU. They need to updated to FB aperture addr using the new `amdgpu_bo_fb_aper_addr()`. That's reason of the last 4 refactoring patches, to remove the cached gpu address completely and convert to local variables. Regards Sam > Thanks, > Lijo > >> + >> struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev); >> void amdgpu_put_xgmi_hive(struct amdgpu_hive_info *hive); >> int amdgpu_xgmi_update_topology(struct amdgpu_hive_info *hive, struct amdgpu_device *adev); >> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c >> index 59385da80185..7c0ca2721eb3 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c >> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c >> @@ -2533,6 +2533,12 @@ static int gmc_v9_0_resume(struct amdgpu_ip_block *ip_block) >> struct amdgpu_device *adev = ip_block->adev; >> int r; >> >> + if (amdgpu_xmgi_is_node_changed(adev)) { >> + adev->vm_manager.vram_base_offset = adev->gfxhub.funcs->get_mc_fb_offset(adev); >> + adev->vm_manager.vram_base_offset += >> + adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size; >> + } >> + >> /* If a reset is done for NPS mode switch, read the memory range >> * information again. >> */<o:p></o:p> </div> </div> </body> </html>