[PATCH v4 1/7] drm/amdgpu: update XGMI info on resume
Zhang, GuoQing (Sam)
GuoQing.Zhang at amd.com
Fri May 9 04:22:00 UTC 2025
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 at linux.alibaba.com>
>>> Signed-off-by: Samuel Zhang <guoqing.zhang at 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 <mailto:Christian.Koenig at amd.com>, thank you for
the explanation.
Hi @Lazar, Lijo <mailto:Lijo.Lazar at amd.com>, 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.
>>> */
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20250509/c1521bfa/attachment.htm>
More information about the amd-gfx
mailing list