[AMD Official Use Only - General]
Hi Mario,
Thank you for your review.
> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@xxxxxxx>
> Sent: Thursday, April 6, 2023 10:05 PM
> To: Yang, WenYou <WenYou.Yang@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>; Quan, Evan
> <Evan.Quan@xxxxxxx>
> Cc: Yuan, Perry <Perry.Yuan@xxxxxxx>; Liang, Richard qi
> <Richardqi.Liang@xxxxxxx>; Li, Ying <YING.LI@xxxxxxx>; Liu, Kun
> <Kun.Liu2@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 2/2] drm/amd/pm/vangogh: Send SMT enable message
> to PMFW
>
> On 4/6/2023 07:45, Wenyou Yang wrote:
> > When the SMT state is changed on the fly, sent the SMT enable message
> > to the PMFW to notify it that the SMT state changed.
> >
> > Add the support to send PPSMC_MSG_SetCClkSMTEnable(0x58) message to
> > the PMFW for Vangogh.
> >
> > Signed-off-by: Wenyou Yang <WenYou.Yang@xxxxxxx>
> > ---
> > .../pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h | 3 +-
> > drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h | 3 +-
> > .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 32
> ++++++++++++++++++-
> > 3 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git
> > a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > index 7471e2df2828..a6bfa1912c42 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/pmfw_if/smu_v11_5_ppsmc.h
> > @@ -111,7 +111,8 @@
> > #define PPSMC_MSG_GetGfxOffStatus 0x50
> > #define PPSMC_MSG_GetGfxOffEntryCount 0x51
> > #define PPSMC_MSG_LogGfxOffResidency 0x52
> > -#define PPSMC_Message_Count 0x53
> > +#define PPSMC_MSG_SetCClkSMTEnable 0x58
> > +#define PPSMC_Message_Count 0x59
> >
> > //Argument for PPSMC_MSG_GfxDeviceDriverReset
> > enum {
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > index 297b70b9388f..820812d910bf 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/smu_types.h
> > @@ -245,7 +245,8 @@
> > __SMU_DUMMY_MAP(AllowGpo), \
> > __SMU_DUMMY_MAP(Mode2Reset), \
> > __SMU_DUMMY_MAP(RequestI2cTransaction), \
> > - __SMU_DUMMY_MAP(GetMetricsTable),
> > + __SMU_DUMMY_MAP(GetMetricsTable), \
> > + __SMU_DUMMY_MAP(SetCClkSMTEnable),
> >
> > #undef __SMU_DUMMY_MAP
> > #define __SMU_DUMMY_MAP(type) SMU_MSG_##type
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > index 7433dcaa16e0..ca1ff97f3353 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> > @@ -141,6 +141,7 @@ static struct cmn2asic_msg_mapping
> vangogh_message_map[SMU_MSG_MAX_COUNT] = {
> > MSG_MAP(GetGfxOffStatus,
> PPSMC_MSG_GetGfxOffStatus, 0),
> > MSG_MAP(GetGfxOffEntryCount,
> PPSMC_MSG_GetGfxOffEntryCount, 0),
> > MSG_MAP(LogGfxOffResidency,
> PPSMC_MSG_LogGfxOffResidency, 0),
> > + MSG_MAP(SetCClkSMTEnable,
> PPSMC_MSG_SetCClkSMTEnable,
> 0),
> > };
> >
> > static struct cmn2asic_mapping
> > vangogh_feature_mask_map[SMU_FEATURE_COUNT] = { @@ -460,6 +461,7
> @@
> > static int vangogh_allocate_dpm_context(struct smu_context *smu)
> >
> > static int vangogh_init_smc_tables(struct smu_context *smu)
> > {
> > + uint32_t smu_version;
> > int ret = 0;
> >
> > ret = vangogh_tables_init(smu);
> > @@ -477,9 +479,24 @@ static int vangogh_init_smc_tables(struct
> smu_context *smu)
> > smu->cpu_core_num = 4;
> > #endif
> >
> > + ret = smu_cmn_get_smc_version(smu, NULL, &smu_version);
> > + if (ret)
> > + return ret;
> > +
> > + if (smu_version >= 0x063F0600)
>
> AFAICT the value has already been looked up and you can instead use:
>
> smu->adev->pm.fw_version >= 0x063F0600
smu->adev->pm.fw_version is assigned in smu_v11_0_init_microcode(),
Vangogh doesn't initialize .init_microcode interface.
So, smu->adev->pm.fw_version value is 0.
>
> > + smu_smt_timer_init(smu);
> > +
> > return smu_v11_0_init_smc_tables(smu);
> > }
> >
> > +static int vangogh_fini_smc_tables(struct smu_context *smu) {
> > + smu_smt_timer_fini(smu);
>
> Shouldn't this timer only be deleted if
>
> smu->adev->pm.fw_version >= 0x063F0600
>
Yes, will add version check before delete the timer.
Need to call the smu_cmn_get_smc_version() again, not good.
Look forward to your opinions.
> > + smu_v11_0_fini_smc_tables(smu);
> > +
> > + return 0;
> > +}
> > +
> > static int vangogh_dpm_set_vcn_enable(struct smu_context *smu, bool
> enable)
> > {
> > int ret = 0;
> > @@ -2428,12 +2445,24 @@ static u32 vangogh_get_gfxoff_entrycount(struct
> smu_context *smu, uint64_t *entr
> > return ret;
> > }
> >
> > +static int vangogh_set_cpu_smt_enable(struct smu_context *smu, bool
> > +enable) {
> > + int ret;
> > +
> > + ret = smu_cmn_send_smc_msg_with_param(smu,
> SMU_MSG_SetCClkSMTEnable,
> > + enable ? 1 : 0, NULL);
> > + if (ret)
> > + dev_err(smu->adev->dev, "Set CPU SMT state failed!\n");
>
> Given this is goign to be triggered by a timer, this might be best to be a rate
> limited message to avoid flooding the logs.
Yes, will add, before call dev_err(...)
if (printk_ratelimit())
Best Regards,
Wenyou
>
> > +
> > + return ret;
> > +}
> > +
> > static const struct pptable_funcs vangogh_ppt_funcs = {
> >
> > .check_fw_status = smu_v11_0_check_fw_status,
> > .check_fw_version = smu_v11_0_check_fw_version,
> > .init_smc_tables = vangogh_init_smc_tables,
> > - .fini_smc_tables = smu_v11_0_fini_smc_tables,
> > + .fini_smc_tables = vangogh_fini_smc_tables,
> > .init_power = smu_v11_0_init_power,
> > .fini_power = smu_v11_0_fini_power,
> > .register_irq_handler = smu_v11_0_register_irq_handler, @@ -2474,6
> > +2503,7 @@ static const struct pptable_funcs vangogh_ppt_funcs = {
> > .get_power_limit = vangogh_get_power_limit,
> > .set_power_limit = vangogh_set_power_limit,
> > .get_vbios_bootup_values = smu_v11_0_get_vbios_bootup_values,
> > + .set_cpu_smt_enable = vangogh_set_cpu_smt_enable,
> > };
> >
> > void vangogh_set_ppt_funcs(struct smu_context *smu)
