[PATCH v5 1/2] drm/amd/pm: Add support to check SMT state periodically

[Yang, WenYou]

[AMD Official Use Only - General]

Hi Mario,

Thank you  for your review.

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello at amd.com>
> Sent: Thursday, April 6, 2023 10:09 PM
> To: Yang, WenYou <WenYou.Yang at amd.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>; Pan, Xinhui <Xinhui.Pan at amd.com>; Quan,
> Evan <Evan.Quan at amd.com>
> Cc: Yuan, Perry <Perry.Yuan at amd.com>; Liang, Richard qi
> <Richardqi.Liang at amd.com>; Li, Ying <YING.LI at amd.com>; Liu, Kun
> <Kun.Liu2 at amd.com>; amd-gfx at lists.freedesktop.org
> 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 at amd.com>
> > ---
> >   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