Re: [PATCH v4 1/7] drm/amdgpu: update XGMI info on resume

["Zhang, GuoQing (Sam)" <GuoQing.Zhang@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 4948 bytes --]


On 2025/5/8 17:27, Christian König wrote:
>
> On 5/8/25 07:09, 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) \
>> +    (adev->gmc.xgmi.prev_physical_node_id != adev->gmc.xgmi.physical_node_id)
>
> Please drop that function and the related checks.
>
> If this is necessary we want to update the relevant parameters all the time and not just when something changed.


Hi @Lazar, Lijo <mailto:Lijo.Lazar@amd.com> and @Koenig, Christian
<mailto:Christian.Koenig@amd.com>, you have different opinion on if the
check is needed. Could you align on this? Thank you!

Regards
Sam


>
> Regards,
> Christian.
>
>> +
>>   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.
>>        */

[-- Attachment #2: Type: text/html, Size: 9857 bytes --]