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: 6222 bytes --]


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 <mailto:Christian.Koenig@amd.com>, thank you for
the explanation.

Hi @Lazar, Lijo <mailto:Lijo.Lazar@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.
>>>       */

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