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

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

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.

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);
>>>