On 2/7/2025 4:56 AM, Kim, Jonathan wrote:
> [Public]
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
>> Sent: Thursday, February 6, 2025 8:13 AM
>> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Lazar, Lijo <Lijo.Lazar@xxxxxxx>
>> Cc: Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Kim, Jonathan
>> <Jonathan.Kim@xxxxxxx>
>> Subject: [PATCH 2/4] drm/amdgpu: Add xgmi speed/width related info
>>
>> Add APIs to initialize XGMI speed, width details and get to max
>> bandwidth supported. It is assumed that a device only supports same
>> generation of XGMI links with uniform width.
>>
>> Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 41 ++++++++++++++++++++++++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h | 11 +++++++
>> 2 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> index 03d909c7b14b..edef5763e2b8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
>> @@ -1679,3 +1679,44 @@ bool amdgpu_xgmi_same_hive(struct amdgpu_device
>> *adev,
>> adev->gmc.xgmi.hive_id &&
>> adev->gmc.xgmi.hive_id == bo_adev->gmc.xgmi.hive_id);
>> }
>> +
>> +static inline uint32_t
>> +__amdgpu_xgmi_get_max_bandwidth(uint8_t link_width,
>> + enum amdgpu_xgmi_link_speed max_speed)
>> +{
>> + /* Returns unidirectional bandwidth in Mbps */
>> + return max_speed * 1000 * link_width;
>
> Instead of wrapping this in a new static, would it work to declare a new member adev->gmc.xgmi.max_bandwidth?
> Then in amdgpu_xgmi_init_info, you can define max_bandwidth directly after the switch.
>
Link Width/Link speed is generally the standard way to represent link
info. Presently, tools like SMI show these for PCIe. Followed the same
here and kept a max bandwidth value calculation.
>> +}
>> +
>> +/**
>> + * amdgpu_xgmi_get_max_bandwidth - Get max possible bandwidth of a single
>> XGMI
>> + * link of the device in Mbps
>> + * @adev: Target device.
>> + *
>> + * Gets the max possible bandwidth of a single XGMI link of the device in Mbps.
>> + * Assumption is all links of the device have the same width and are of the
>> + * same XGMI generation.
>> + */
>> +uint32_t amdgpu_xgmi_get_max_bandwidth(struct amdgpu_device *adev)
>> +{
>> + if (!adev->gmc.xgmi.supported)
>> + return 0;
>> +
>> + return __amdgpu_xgmi_get_max_bandwidth(adev->gmc.xgmi.max_width,
>> adev->gmc.xgmi.max_speed);
>
> If you do the suggested above, both tests can be simplified into 1 line as
> -> return = xgmi_supported ? max_bandwidth : 0;
>
Yes, primarily width/speed is chosen to represent link info.
>> +}
>> +
>> +void amdgpu_xgmi_init_info(struct amdgpu_device *adev)
>> +{
>> + switch (amdgpu_ip_version(adev, XGMI_HWIP, 0)) {
>> + case IP_VERSION(6, 1, 0):
>> + adev->gmc.xgmi.max_speed = XGMI2_5_SPEED_GT;
>> + adev->gmc.xgmi.max_width = 16;
>> + break;
>> + case IP_VERSION(6, 4, 0):
>> + adev->gmc.xgmi.max_speed = XGMI3_SPEED_GT;
>> + adev->gmc.xgmi.max_width = 16;
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>> index 044c4f6be44a..384c4ddef78a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
>> @@ -25,6 +25,12 @@
>> #include <drm/task_barrier.h>
>> #include "amdgpu_ras.h"
>>
>> +enum amdgpu_xgmi_link_speed {
>> + XGMI2_SPEED_GT = 16,
>> + XGMI2_5_SPEED_GT = 25,
>> + XGMI3_SPEED_GT = 32
>
> Why not declare and define them as units of Mbps to begin with so you don't have to do the calculation later?
Actually, this is also something which is followed from PCIe to
represent as GT/s. If bandwidth field alone is kept, then this also
won't be required.
I'm not so sure if that is a good way to keep things.
Thanks,
Lijo
>
> Jon
>
>> +};
>> +
>> struct amdgpu_hive_info {
>> struct kobject kobj;
>> uint64_t hive_id;
>> @@ -75,6 +81,8 @@ struct amdgpu_xgmi {
>> struct ras_common_if *ras_if;
>> bool connected_to_cpu;
>> struct amdgpu_xgmi_ras *ras;
>> + enum amdgpu_xgmi_link_speed max_speed;
>> + uint8_t max_width;
>> };
>>
>> struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev);
>> @@ -102,4 +110,7 @@ int amdgpu_xgmi_request_nps_change(struct
>> amdgpu_device *adev,
>> int amdgpu_get_xgmi_link_status(struct amdgpu_device *adev,
>> int global_link_num);
>>
>> +void amdgpu_xgmi_init_info(struct amdgpu_device *adev);
>> +uint32_t amdgpu_xgmi_get_max_bandwidth(struct amdgpu_device *adev);
>> +
>> #endif
>> --
>> 2.25.1
>
