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 <ge...@linux.alibaba.com>
>> Signed-off-by: Samuel Zhang <guoqing.zh...@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.
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.
>> */
>
