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

["Christian König" <christian.koenig@amd.com>]

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.

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.
>>  	 */
>