On 2/7/2025 5:03 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 3/4] drm/amdgpu: Initialize xgmi info during discovery
>>
>> Initialize xgmi related static information during discovery.
>>
>> Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 20 +++++++++++++------
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> index eca431e52038..d4eade2bd4d3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
>> @@ -2502,6 +2502,19 @@ static int amdgpu_discovery_set_isp_ip_blocks(struct
>> amdgpu_device *adev)
>> return 0;
>> }
>>
>> +static void amdgpu_discovery_set_xgmi_info(struct amdgpu_device *adev)
>> +{
>> + if (amdgpu_ip_version(adev, XGMI_HWIP, 0) == IP_VERSION(4, 8, 0))
>> + adev->gmc.xgmi.supported = true;
>> +
>> + if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
>> + amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4))
>> + adev->ip_versions[XGMI_HWIP][0] = IP_VERSION(6, 4, 0);
>
> Can this stuff get rolled into xgm_init_info and called directly into amdgpu_discovery_set_ip_blocks?
> Breaking up discovery_set_xgmi_info and xgmi_init_info as 2 separate things seems a little confusing.
>
Intent is like this -
Set IP version info. This is the job of discovery and kept inside
amdgpu_discovery.
Set any static information derived out of IP versions and not available
in discovery tables. This is kept outside of discovery file.
Thanks,
Lijo
>> +
>> + if (amdgpu_ip_version(adev, XGMI_HWIP, 0))
>
> Maybe roll this check into xgmi_init_info i.e. void early return if null.
>
>> + amdgpu_xgmi_init_info(adev);
>> +}
>> +
>> int amdgpu_discovery_set_ip_blocks(struct amdgpu_device *adev)
>> {
>> int r;
>> @@ -2769,12 +2782,7 @@ int amdgpu_discovery_set_ip_blocks(struct
>> amdgpu_device *adev)
>> break;
>> }
>>
>> - if (amdgpu_ip_version(adev, XGMI_HWIP, 0) == IP_VERSION(4, 8, 0))
>> - adev->gmc.xgmi.supported = true;
>> -
>> - if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
>> - amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4))
>> - adev->ip_versions[XGMI_HWIP][0] = IP_VERSION(6, 4, 0);
>> + amdgpu_discovery_set_xgmi_info(adev);
>
> If you do the suggestions above, you can just call amdgpu_xgmi_init_info unconditionally.
>
> Jon
>>
>> /* set NBIO version */
>> switch (amdgpu_ip_version(adev, NBIO_HWIP, 0)) {
>> --
>> 2.25.1
>
