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

["Quan, Evan" <Evan.Quan@amd.com>]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Thursday, August 12, 2021 3:53 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: nils.wallmenius@gmail.com; Chen, Guchun <Guchun.Chen@amd.com>
> Subject: Re: [PATCH V2 2/7] drm/amd/pm: record the RPM and PWM based
> fan speed settings
> 
> 
> 
> On 8/12/2021 12:21 PM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> >> Sent: Thursday, August 12, 2021 2:05 PM
> >> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> >> Cc: nils.wallmenius@gmail.com; Chen, Guchun <Guchun.Chen@amd.com>
> >> 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@amd.com>
> >>> --
> >>> 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.
[Quan, Evan] Hmm. How about the followings? A notice in the description part for related APIs?

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index 04c7d82f8b89..112ee5f5d855 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -3174,6 +3174,9 @@ static ssize_t amdgpu_hwmon_show_mclk_label(struct device *dev,
  *
  * - fan[1-\*]_enable: Enable or disable the sensors.1: Enable 0: Disable
  *
+ * NOTE: DO NOT set the fan speed via "pwm1" and "fan[1-\*]_target" interfaces at the same time.
+ *       That will get the former one overridden.
+ *
  * hwmon interfaces for GPU clocks:
  *
  * - freq1_input: the gfx/compute clock in hertz
> 
> 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);
> >>>