[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?
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
> >
