On 2/7/2025 7:29 PM, Kim, Jonathan wrote:
> [Public]
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
>> Sent: Thursday, February 6, 2025 10:56 PM
>> To: Kim, Jonathan <Jonathan.Kim@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Zhang, Hawking <Hawking.Zhang@xxxxxxx>
>> 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@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.
>
>
> 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
>>>
>
