[Public]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Friday, February 7, 2025 9:20 AM
> 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 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.
Snip from you're last response:
> >> Set any static information derived out of IP versions and not available
> >> in discovery tables. This is kept outside of discovery file.
You're calling amdgpu_discovery_set_xgmi_info which calls amdgpu_xgmi_init_info which is setting static derived information in the discovery file.
A wrapper called in a wrapper is still doing the opposite of what you're saying int the snip above.
If you're trying to avoid this and keep discovery clean, call xgmi_init_info in amdgpu_device.c somewhere after the IP blocks are set.
And put xgmi_supported definitions in xgmi_init_info since that doesn't count as IP version setting.
Jon
>
> 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
> >>>
> >
