[AMD Official Use Only]
Yes, that check looks good.
A couple of other things -
PMFW ticket as a comment is not needed.
I remember Satish updating the same struct for smartshift related data. Some additional fields added towards the end. Not sure if Sienna struct also carries those fields. Regardless, you may check if that affects the placement of serial number fields (whether
before or after those fields).
Thanks,
Lijo
Lijo
From: Russell, Kent <Kent.Russell@xxxxxxx>
Sent: Monday, March 28, 2022 9:23:36 PM
To: Lazar, Lijo <Lijo.Lazar@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
Subject: RE: [PATCH 2/2] drm/amdgpu: Add unique_id support for sienna cichlid
Sent: Monday, March 28, 2022 9:23:36 PM
To: Lazar, Lijo <Lijo.Lazar@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
Subject: RE: [PATCH 2/2] drm/amdgpu: Add unique_id support for sienna cichlid
[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Monday, March 28, 2022 11:48 AM
> To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Subject: Re: [PATCH 2/2] drm/amdgpu: Add unique_id support for sienna cichlid
>
>
>
> On 3/28/2022 9:12 PM, Russell, Kent wrote:
> > [AMD Official Use Only]
> >
> > Responses inline
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> >> Sent: Monday, March 28, 2022 11:18 AM
> >> To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> >> Subject: Re: [PATCH 2/2] drm/amdgpu: Add unique_id support for sienna cichlid
> >>
> >>
> >>
> >> On 3/28/2022 8:05 PM, Kent Russell wrote:
> >>> This is being added to SMU Metrics, so add the required tie-ins in the
> >>> kernel. Also create the corresponding unique_id sysfs file.
> >>>
> >>> v2: Add FW version check, remove SMU mutex
> >>> v3: Fix style warning
> >>>
> >>> Signed-off-by: Kent Russell <kent.russell@xxxxxxx>
> >>> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 1 +
> >>> .../pmfw_if/smu11_driver_if_sienna_cichlid.h | 12 +++++--
> >>> .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 36 +++++++++++++++++++
> >>> 3 files changed, 47 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >>> index 4151db2678fb..4a9aabc16fbc 100644
> >>> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >>> @@ -1993,6 +1993,7 @@ static int default_attr_update(struct amdgpu_device *adev,
> >> struct amdgpu_device_
> >>> case IP_VERSION(9, 4, 0):
> >>> case IP_VERSION(9, 4, 1):
> >>> case IP_VERSION(9, 4, 2):
> >>> + case IP_VERSION(10, 3, 0):
> >>> *states = ATTR_STATE_SUPPORTED;
> >>> break;
> >>> default:
> >>> diff --git
> >> a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >> b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >>> index 3e4a314ef925..58f977320d06 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >>> +++
> b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >>> @@ -1419,8 +1419,12 @@ typedef struct {
> >>> uint8_t PcieRate ;
> >>> uint8_t PcieWidth ;
> >>> uint16_t AverageGfxclkFrequencyTarget;
> >>> - uint16_t Padding16_2;
> >>>
> >>> + //PMFW-8711
> >>> + uint32_t PublicSerialNumLower32;
> >>> + uint32_t PublicSerialNumUpper32;
> >>> +
> >>> + uint16_t Padding16_2;
> >>> } SmuMetrics_t;
> >>>
> >>> typedef struct {
> >>> @@ -1476,8 +1480,12 @@ typedef struct {
> >>> uint8_t PcieRate ;
> >>> uint8_t PcieWidth ;
> >>> uint16_t AverageGfxclkFrequencyTarget;
> >>> - uint16_t Padding16_2;
> >>>
> >>> + //PMFW-8711
> >>> + uint32_t PublicSerialNumLower32;
> >>> + uint32_t PublicSerialNumUpper32;
> >>> +
> >>
> >> Is this the case for other ASICs also which share the metrics data with
> >> Sienna?
> >
> > No, only for Sienna Cichlid. The PMFW guys didn't implement it for Navy Flounder or
> Dimgrey Cavefish.
> >
> >>
> >>> + uint16_t Padding16_2;
> >>> } SmuMetrics_V2_t;
> >>>
> >>> typedef struct {
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >>> index 38f04836c82f..550458f6246a 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >>> @@ -481,6 +481,41 @@ static int sienna_cichlid_setup_pptable(struct smu_context
> >> *smu)
> >>> return sienna_cichlid_patch_pptable_quirk(smu);
> >>> }
> >>>
> >>> +static void sienna_cichlid_get_unique_id(struct smu_context *smu)
> >>> +{
> >>> + struct amdgpu_device *adev = smu->adev;
> >>> + struct smu_table_context *smu_table = &smu->smu_table;
> >>> + SmuMetrics_t *metrics =
> >>> + &(((SmuMetricsExternal_t *)(smu_table->metrics_table))->SmuMetrics);
> >>> + SmuMetrics_V2_t *metrics_v2 =
> >>> + &(((SmuMetricsExternal_t *)(smu_table->metrics_table))->SmuMetrics_V2);
> >>> + uint32_t upper32 = 0, lower32 = 0;
> >>> + bool use_metrics_v2;
> >>> + int ret;
> >>> +
> >>> + /* Only supported as of version 0.58.83.0 */
> >>> + if (smu->smc_fw_version < 0x3A5300)
> >>> + return;
> >>> +
> >>
> >> Since this is shared with other ASICs, I guess this check itself may not
> >> be enough. This function may be skipped if it's not MP1 11.0.7 or GC 10.3.0?
> >>
> >
> > Since the sysfs file is only supported on Sienna Cichlid (10.3.0), is it a concern since the tie-
> in won't exist on the other SMU11-based ASICs? And this function is only referenced by
> sienna_cichlid, unless I misunderstood something (and someone else uses
> sienna_cichlid_tables_init).
> >
>
> This function also gets called as part of common init sequence -
> smu_get_unique_id.
> If PMFW version of Navi Flounder/Dimgrey ASIC is greater than Sienna,
> then it may go to the path which is not intended to be executed on that
> ASIC.
Would it be sufficient to just confirm the IP_VERSION here too then?
e.g.
/* Only supported as of version 0.58.83.0 and only on Sienna Cichlid (GC 10.3.0)*/
if (smu->smc_fw_version < 0x3A5300 || adev->ip_versions[GC_HWIP][0] != IP_VERSION(10, 3, 0))
return;
Thus the FW has to be that version, and the IP_VERSION has to be 10.3 ? Or is there a better method to use? Thanks!
Kent
>
> Thanks,
> Lijo
>
> > Kent
> >
> >> Thanks,
> >> Lijo
> >>
> >>> + ret = smu_cmn_get_metrics_table(smu, NULL, false);
> >>> + if (ret)
> >>> + goto out_unlock;
> >>> +
> >>> + use_metrics_v2 = ((smu->adev->ip_versions[MP1_HWIP][0] == IP_VERSION(11, 0,
> >> 7)) &&
> >>> + (smu->smc_fw_version >= 0x3A4300)) ? true : false;
> >>> +
> >>> + upper32 = use_metrics_v2 ? metrics_v2->PublicSerialNumUpper32 :
> >>> + metrics->PublicSerialNumUpper32;
> >>> + lower32 = use_metrics_v2 ? metrics_v2->PublicSerialNumLower32 :
> >>> + metrics->PublicSerialNumLower32;
> >>> +
> >>> +out_unlock:
> >>> +
> >>> + adev->unique_id = ((uint64_t)upper32 << 32) | lower32;
> >>> + if (adev->serial[0] == '\0')
> >>> + sprintf(adev->serial, "%016llx", adev->unique_id);
> >>> +}
> >>> +
> >>> static int sienna_cichlid_tables_init(struct smu_context *smu)
> >>> {
> >>> struct smu_table_context *smu_table = &smu->smu_table;
> >>> @@ -4182,6 +4217,7 @@ static const struct pptable_funcs sienna_cichlid_ppt_funcs = {
> >>> .get_ecc_info = sienna_cichlid_get_ecc_info,
> >>> .get_default_config_table_settings =
> >> sienna_cichlid_get_default_config_table_settings,
> >>> .set_config_table = sienna_cichlid_set_config_table,
> >>> + .get_unique_id = sienna_cichlid_get_unique_id,
> >>> };
> >>>
> >>> void sienna_cichlid_set_ppt_funcs(struct smu_context *smu)
> >>>
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Monday, March 28, 2022 11:48 AM
> To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Subject: Re: [PATCH 2/2] drm/amdgpu: Add unique_id support for sienna cichlid
>
>
>
> On 3/28/2022 9:12 PM, Russell, Kent wrote:
> > [AMD Official Use Only]
> >
> > Responses inline
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> >> Sent: Monday, March 28, 2022 11:18 AM
> >> To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> >> Subject: Re: [PATCH 2/2] drm/amdgpu: Add unique_id support for sienna cichlid
> >>
> >>
> >>
> >> On 3/28/2022 8:05 PM, Kent Russell wrote:
> >>> This is being added to SMU Metrics, so add the required tie-ins in the
> >>> kernel. Also create the corresponding unique_id sysfs file.
> >>>
> >>> v2: Add FW version check, remove SMU mutex
> >>> v3: Fix style warning
> >>>
> >>> Signed-off-by: Kent Russell <kent.russell@xxxxxxx>
> >>> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 1 +
> >>> .../pmfw_if/smu11_driver_if_sienna_cichlid.h | 12 +++++--
> >>> .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 36 +++++++++++++++++++
> >>> 3 files changed, 47 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >>> index 4151db2678fb..4a9aabc16fbc 100644
> >>> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> >>> @@ -1993,6 +1993,7 @@ static int default_attr_update(struct amdgpu_device *adev,
> >> struct amdgpu_device_
> >>> case IP_VERSION(9, 4, 0):
> >>> case IP_VERSION(9, 4, 1):
> >>> case IP_VERSION(9, 4, 2):
> >>> + case IP_VERSION(10, 3, 0):
> >>> *states = ATTR_STATE_SUPPORTED;
> >>> break;
> >>> default:
> >>> diff --git
> >> a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >> b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >>> index 3e4a314ef925..58f977320d06 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >>> +++
> b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu11_driver_if_sienna_cichlid.h
> >>> @@ -1419,8 +1419,12 @@ typedef struct {
> >>> uint8_t PcieRate ;
> >>> uint8_t PcieWidth ;
> >>> uint16_t AverageGfxclkFrequencyTarget;
> >>> - uint16_t Padding16_2;
> >>>
> >>> + //PMFW-8711
> >>> + uint32_t PublicSerialNumLower32;
> >>> + uint32_t PublicSerialNumUpper32;
> >>> +
> >>> + uint16_t Padding16_2;
> >>> } SmuMetrics_t;
> >>>
> >>> typedef struct {
> >>> @@ -1476,8 +1480,12 @@ typedef struct {
> >>> uint8_t PcieRate ;
> >>> uint8_t PcieWidth ;
> >>> uint16_t AverageGfxclkFrequencyTarget;
> >>> - uint16_t Padding16_2;
> >>>
> >>> + //PMFW-8711
> >>> + uint32_t PublicSerialNumLower32;
> >>> + uint32_t PublicSerialNumUpper32;
> >>> +
> >>
> >> Is this the case for other ASICs also which share the metrics data with
> >> Sienna?
> >
> > No, only for Sienna Cichlid. The PMFW guys didn't implement it for Navy Flounder or
> Dimgrey Cavefish.
> >
> >>
> >>> + uint16_t Padding16_2;
> >>> } SmuMetrics_V2_t;
> >>>
> >>> typedef struct {
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >>> index 38f04836c82f..550458f6246a 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> >>> @@ -481,6 +481,41 @@ static int sienna_cichlid_setup_pptable(struct smu_context
> >> *smu)
> >>> return sienna_cichlid_patch_pptable_quirk(smu);
> >>> }
> >>>
> >>> +static void sienna_cichlid_get_unique_id(struct smu_context *smu)
> >>> +{
> >>> + struct amdgpu_device *adev = smu->adev;
> >>> + struct smu_table_context *smu_table = &smu->smu_table;
> >>> + SmuMetrics_t *metrics =
> >>> + &(((SmuMetricsExternal_t *)(smu_table->metrics_table))->SmuMetrics);
> >>> + SmuMetrics_V2_t *metrics_v2 =
> >>> + &(((SmuMetricsExternal_t *)(smu_table->metrics_table))->SmuMetrics_V2);
> >>> + uint32_t upper32 = 0, lower32 = 0;
> >>> + bool use_metrics_v2;
> >>> + int ret;
> >>> +
> >>> + /* Only supported as of version 0.58.83.0 */
> >>> + if (smu->smc_fw_version < 0x3A5300)
> >>> + return;
> >>> +
> >>
> >> Since this is shared with other ASICs, I guess this check itself may not
> >> be enough. This function may be skipped if it's not MP1 11.0.7 or GC 10.3.0?
> >>
> >
> > Since the sysfs file is only supported on Sienna Cichlid (10.3.0), is it a concern since the tie-
> in won't exist on the other SMU11-based ASICs? And this function is only referenced by
> sienna_cichlid, unless I misunderstood something (and someone else uses
> sienna_cichlid_tables_init).
> >
>
> This function also gets called as part of common init sequence -
> smu_get_unique_id.
> If PMFW version of Navi Flounder/Dimgrey ASIC is greater than Sienna,
> then it may go to the path which is not intended to be executed on that
> ASIC.
Would it be sufficient to just confirm the IP_VERSION here too then?
e.g.
/* Only supported as of version 0.58.83.0 and only on Sienna Cichlid (GC 10.3.0)*/
if (smu->smc_fw_version < 0x3A5300 || adev->ip_versions[GC_HWIP][0] != IP_VERSION(10, 3, 0))
return;
Thus the FW has to be that version, and the IP_VERSION has to be 10.3 ? Or is there a better method to use? Thanks!
Kent
>
> Thanks,
> Lijo
>
> > Kent
> >
> >> Thanks,
> >> Lijo
> >>
> >>> + ret = smu_cmn_get_metrics_table(smu, NULL, false);
> >>> + if (ret)
> >>> + goto out_unlock;
> >>> +
> >>> + use_metrics_v2 = ((smu->adev->ip_versions[MP1_HWIP][0] == IP_VERSION(11, 0,
> >> 7)) &&
> >>> + (smu->smc_fw_version >= 0x3A4300)) ? true : false;
> >>> +
> >>> + upper32 = use_metrics_v2 ? metrics_v2->PublicSerialNumUpper32 :
> >>> + metrics->PublicSerialNumUpper32;
> >>> + lower32 = use_metrics_v2 ? metrics_v2->PublicSerialNumLower32 :
> >>> + metrics->PublicSerialNumLower32;
> >>> +
> >>> +out_unlock:
> >>> +
> >>> + adev->unique_id = ((uint64_t)upper32 << 32) | lower32;
> >>> + if (adev->serial[0] == '\0')
> >>> + sprintf(adev->serial, "%016llx", adev->unique_id);
> >>> +}
> >>> +
> >>> static int sienna_cichlid_tables_init(struct smu_context *smu)
> >>> {
> >>> struct smu_table_context *smu_table = &smu->smu_table;
> >>> @@ -4182,6 +4217,7 @@ static const struct pptable_funcs sienna_cichlid_ppt_funcs = {
> >>> .get_ecc_info = sienna_cichlid_get_ecc_info,
> >>> .get_default_config_table_settings =
> >> sienna_cichlid_get_default_config_table_settings,
> >>> .set_config_table = sienna_cichlid_set_config_table,
> >>> + .get_unique_id = sienna_cichlid_get_unique_id,
> >>> };
> >>>
> >>> void sienna_cichlid_set_ppt_funcs(struct smu_context *smu)
> >>>
