<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=iso-8859-1"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="en-CN" link="blue" vlink="purple" style="word-wrap:break-word"> <div class="WordSection1"> <div> On 2025/5/8 18:56, Christian König wrote: > On 5/8/25 10: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. > > Yeah, that is a rather good idea as well. > > And we should *always* execute that and not just when the physical node id changes. > > Otherwise we won't always test the code path and potentially break it at some point. Hi @Koenig, Christian <<a href="mailto:Christian.Koenig@amd.com">mailto:Christian.Koenig@amd.com</a>>, thank you for the explanation. Hi @Lazar, Lijo <<a href="mailto:Lijo.Lazar@amd.com">mailto:Lijo.Lazar@amd.com</a>>, are you OK with removing the check? I can do reset test using quark in VM to ensure no regression is introduced. Please comment if you still have different opinion. Thank you! Regards Sam > Regards, > Christian. > >> 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(); >> >> 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>