[PATCH 3/4] drm/amdgpu: Initialize xgmi info during discovery

[Lazar, Lijo]

On 2/7/2025 7:29 PM, Kim, Jonathan wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Thursday, February 6, 2025 10:56 PM
>> To: Kim, Jonathan <Jonathan.Kim at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Zhang, Hawking <Hawking.Zhang at amd.com>
>> Subject: Re: [PATCH 3/4] drm/amdgpu: Initialize xgmi info during discovery
>>
>>
>>
>> On 2/7/2025 5:03 AM, Kim, Jonathan wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>> Sent: Thursday, February 6, 2025 8:13 AM
>>>> To: amd-gfx at lists.freedesktop.org; Lazar, Lijo <Lijo.Lazar at amd.com>
>>>> Cc: Zhang, Hawking <Hawking.Zhang at amd.com>; Kim, Jonathan
>>>> <Jonathan.Kim at amd.com>
>>>> 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 at amd.com>
>>>> ---
>>>>  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.
> 
> 
> Then why are you proposing to set up static information in the discovery file in the first place?

I didn't understand that statement. The function - amdgpu_xgmi_init_info
- called from discovery sets up the derived information. Only IP version
info is set inside discovery.

Thanks,
Lijo

> 
> Jon
> 
>>
>> 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
>>>
>