Re: [PATCH V2 2/7] drm/amd/pm: record the RPM and PWM based fan speed settings

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 8/12/2021 12:21 PM, Quan, Evan wrote:
> [AMD Official Use Only]
>
>
>
> > -----Original Message-----
> > From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> > Sent: Thursday, August 12, 2021 2:05 PM
> > To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: nils.wallmenius@xxxxxxxxx; Chen, Guchun <Guchun.Chen@xxxxxxx>
> > Subject: Re: [PATCH V2 2/7] drm/amd/pm: record the RPM and PWM based
> > fan speed settings
> >
> >
> >
> > On 8/12/2021 9:31 AM, Evan Quan wrote:
> > > As the relationship "PWM = RPM / smu->fan_max_rpm" between fan
> > speed
> > > PWM and RPM is not true for SMU11 ASICs. So, both the RPM and PWM
> > > settings need to be saved.
> > >
> > > Change-Id: I318c134d442273d518b805339cdf383e151b935d
> > > Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> > > --
> > > v1->v2:
> > >     - coding style and logging prints optimization (Guchun)
> > >     - reuse existing flags (Lijo)
> > > ---
> > >    drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h   |  3 +++
> > >    drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 22
> > ++++++++++++++++------
> > >    2 files changed, 19 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > index 183654f8b564..29934a5af44e 100644
> > > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > @@ -34,6 +34,8 @@
> > >    #define SMU_FW_NAME_LEN			0x24
> > >
> > >    #define SMU_DPM_USER_PROFILE_RESTORE (1 << 0)
> > > +#define SMU_CUSTOM_FAN_SPEED_RPM     (1 << 1)
> > > +#define SMU_CUSTOM_FAN_SPEED_PWM     (1 << 2)
> > >
> > >    // Power Throttlers
> > >    #define SMU_THROTTLER_PPT0_BIT			0
> > > @@ -230,6 +232,7 @@ struct smu_user_dpm_profile {
> > >    	uint32_t fan_mode;
> > >    	uint32_t power_limit;
> > >    	uint32_t fan_speed_percent;
> > > +	uint32_t fan_speed_rpm;
> > >    	uint32_t flags;
> > >    	uint32_t user_od;
> > >
> > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > index e33e67310030..131ad0dfefbe 100644
> > > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > @@ -413,7 +413,13 @@ static void smu_restore_dpm_user_profile(struct
> > smu_context *smu)
> > >    		if (!ret && smu->user_dpm_profile.fan_speed_percent) {
> > >    			ret = smu_set_fan_speed_percent(smu, smu-
> > > user_dpm_profile.fan_speed_percent);
> > >    			if (ret)
> > > -				dev_err(smu->adev->dev, "Failed to set
> > manual fan speed\n");
> > > +				dev_err(smu->adev->dev, "Failed to set
> > manual fan speed in percent\n");
> > > +		}
> > > +
> > > +		if (!ret && smu->user_dpm_profile.fan_speed_rpm) {
> > > +			ret = smu_set_fan_speed_rpm(smu, smu-
> > > user_dpm_profile.fan_speed_rpm);
> > > +			if (ret)
> > > +				dev_err(smu->adev->dev, "Failed to set
> > manual fan speed in
> > > +rpm\n");
> > >    		}
> > >    	}
> > >
> > > @@ -2179,7 +2185,6 @@ static int smu_set_gfx_cgpg(struct smu_context
> > *smu, bool enabled)
> > >    static int smu_set_fan_speed_rpm(void *handle, uint32_t speed)
> > >    {
> > >    	struct smu_context *smu = handle;
> > > -	u32 percent;
> > >    	int ret = 0;
> > >
> > >    	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled) @@ -
> > 2190,8
> > > +2195,8 @@ static int smu_set_fan_speed_rpm(void *handle, uint32_t
> > speed)
> > >    	if (smu->ppt_funcs->set_fan_speed_rpm) {
> > >    		ret = smu->ppt_funcs->set_fan_speed_rpm(smu, speed);
> > >    		if (!ret && smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE) {
> > > -			percent = speed * 100 / smu->fan_max_rpm;
> > > -			smu->user_dpm_profile.fan_speed_percent =
> > percent;
> > > +			smu->user_dpm_profile.flags |=
> > SMU_CUSTOM_FAN_SPEED_RPM;
> > > +			smu->user_dpm_profile.fan_speed_rpm = speed;
> >
> > Sorry, missed this when you posted v1. Either RPM or PWM mode is allowed
> > at a time, is that right? If so, shall we make the pwm to 0 and reset PWM flag
> > when RPM is set and vice versa? This helps during restore where one is not
> > overwritten with the other.
> [Quan, Evan] Sounds reasonable to me. But I suppose we also need to prompt some warnings when user trying to set these two modes at the same time.
> Instead of performing these silently. Right?

Not sure on the warnings part. For ex: user may set the manual mode, 
choose a pwm based fan speed and may later switch to a precise rpm based 
speed. Since it's driven by user, we may not need to warn.

Thanks,
Lijo

> > Thanks,
> > Lijo
> >
> > >    		}
> > >    	}
> > >
> > > @@ -2552,8 +2557,11 @@ static int smu_set_fan_control_mode(struct
> > > smu_context *smu, int value)
> > >
> > >    	/* reset user dpm fan speed */
> > >    	if (!ret && value != AMD_FAN_CTRL_MANUAL &&
> > > -			!(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE))
> > > +			!(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > >    		smu->user_dpm_profile.fan_speed_percent = 0;
> > > +		smu->user_dpm_profile.fan_speed_rpm = 0;
> > > +		smu->user_dpm_profile.flags &=
> > ~(SMU_CUSTOM_FAN_SPEED_RPM | SMU_CUSTOM_FAN_SPEED_PWM);
> > > +	}
> > >
> > >    	return ret;
> > >    }
> > > @@ -2604,8 +2612,10 @@ static int smu_set_fan_speed_percent(void
> > *handle, u32 speed)
> > >    		if (speed > 100)
> > >    			speed = 100;
> > >    		ret = smu->ppt_funcs->set_fan_speed_percent(smu,
> > speed);
> > > -		if (!ret && !(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE))
> > > +		if (!ret && !(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > > +			smu->user_dpm_profile.flags |=
> > SMU_CUSTOM_FAN_SPEED_PWM;
> > >    			smu->user_dpm_profile.fan_speed_percent =
> > speed;
> > > +		}
> > >    	}
> > >
> > >    	mutex_unlock(&smu->mutex);