[AMD Official Use Only - General]
Hi Mario,
> -----Original Message-----
> From: Yang, WenYou
> Sent: Friday, April 7, 2023 10:47 AM
> To: Limonciello, Mario <Mario.Limonciello@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
>
> [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
I prefer removing this message statement. The low-level api has a message print if failing to send.
>
> 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)
