[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:09 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 1/2] drm/amd/pm: Add support to check SMT state
> periodically
>
> On 4/6/2023 07:45, Wenyou Yang wrote:
> > Add a timer to poll the SMT state periodically, if the SMT state is
> > changed, invoke the interface to notify the PMFW.
> >
> > Signed-off-by: Wenyou Yang <WenYou.Yang@xxxxxxx>
> > ---
> > drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 8 ++++
> > drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 44
> +++++++++++++++++++
> > drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h | 5 +++
> > 3 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > index 09469c750a96..fc571c122e87 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> > @@ -566,6 +566,9 @@ struct smu_context
> >
> > struct firmware pptable_firmware;
> >
> > + bool last_smt_active;
> > + struct timer_list smt_timer;
> > +
> > u32 param_reg;
> > u32 msg_reg;
> > u32 resp_reg;
> > @@ -1354,6 +1357,11 @@ struct pptable_funcs {
> > * @init_pptable_microcode: Prepare the pptable microcode to
> upload via PSP
> > */
> > int (*init_pptable_microcode)(struct smu_context *smu);
> > +
> > + /**
> > + * @set_cpu_smt_enable: Set the CPU SMT status.
> > + */
> > + int (*set_cpu_smt_enable)(struct smu_context *smu, bool
> smt_enable);
> > };
> >
> > typedef enum {
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > index 3ecb900e6ecd..b0e0c6664ac3 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > @@ -26,6 +26,7 @@
> > #include "amdgpu_smu.h"
> > #include "smu_cmn.h"
> > #include "soc15_common.h"
> > +#include <linux/sched/smt.h>
> >
> > /*
> > * DO NOT use these for err/warn/info/debug messages.
> > @@ -1058,3 +1059,46 @@ bool smu_cmn_is_audio_func_enabled(struct
> > amdgpu_device *adev)
> >
> > return snd_driver_loaded;
> > }
> > +
> > +#define TIME_INTERVAL 200
> > +
> > +static int smu_set_cpu_smt_enable(struct smu_context *smu, bool
> > +enable) {
> > + int ret = -EINVAL;
> > +
> > + if (smu->ppt_funcs && smu->ppt_funcs->set_cpu_smt_enable)
> > + ret = smu->ppt_funcs->set_cpu_smt_enable(smu, enable);
> > +
> > + return ret;
> > +}
> > +
> > +static void smu_smt_timer_callback(struct timer_list *timer) {
> > + struct smu_context *smu = container_of(timer,
> > + struct smu_context, smt_timer);
> > + bool smt_active;
> > +
> > + smt_active = sched_smt_active();
> > + if (smt_active != smu->last_smt_active) {
> > + smu->last_smt_active = smt_active;
> > + smu_set_cpu_smt_enable(smu, smt_active);
>
> You're ignoring the return value for smu_set_cpu_smt_enable. If the
> message failed to send that means smu->last_smt_active will have the
> wrong value and the message will never attempt to send again while in this
> SMT state even though the timer triggered again.
>
> I think you should do it like this:
>
> if (!smu_set_cpu_smt_enable(smu, smt_active))
> smu->last_smt_active = smt_active;
>
Yes. Will change in next version.
> > + }
> > +
> > + mod_timer(timer, jiffies + msecs_to_jiffies(TIME_INTERVAL)); }
> > +
> > +void smu_smt_timer_init(struct smu_context *smu) {
> > + struct timer_list *timer = &smu->smt_timer;
> > +
> > + smu->last_smt_active = sched_smt_active();
> > +
> > + timer_setup(timer, smu_smt_timer_callback, 0);
> > +
> > + mod_timer(timer, jiffies + msecs_to_jiffies(TIME_INTERVAL)); }
> > +
> > +void smu_smt_timer_fini(struct smu_context *smu) {
> > + del_timer(&smu->smt_timer);
> > +}
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > index d7cd358a53bd..928dd9e30d83 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > @@ -127,5 +127,10 @@ static inline void smu_cmn_get_sysfs_buf(char
> > **buf, int *offset)
> >
> > bool smu_cmn_is_audio_func_enabled(struct amdgpu_device *adev);
> >
> > +void smu_smt_timer_init(struct smu_context *smu);
> > +
> > #endif
> > +
> > +void smu_smt_timer_fini(struct smu_context *smu);
> > +
> > #endif
