[PATCH] drm/amd/pm: Fix that RPM cannot be obtained for specific GPU

[PATCH] drm/amd/pm: Fix that RPM cannot be obtained for specific GPU

[huangyizhi]

The current mechanism for obtaining RPM is to read tach_period from
the register, and then calculate the RPM together with the frequency.
But we found that on specific GPUs, such as RX 550 and RX 560D,
tach_period always reads as 0 and smu7_fan_ctrl_get_fan_speed_rpm
will returns -EINVAL.

To solve this problem, when reading tach_period as 0, we try
to estimate the current RPM using the percentage of current pwm, the
maximum and minimum RPM.

Signed-off-by: huangyizhi <huangyizhi@hnu.edu.cn>
---
 .../drm/amd/pm/powerplay/hwmgr/smu7_thermal.c | 28 ++++++++++++++++---
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
index a6c3610db23e..307dd87d6882 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
@@ -81,6 +81,11 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
 {
 	uint32_t tach_period;
 	uint32_t crystal_clock_freq;
+	uint32_t duty100;
+	uint32_t duty;
+	uint32_t speed_percent;
+	uint64_t tmp64;
+
 
 	if (hwmgr->thermal_controller.fanInfo.bNoFan ||
 	    !hwmgr->thermal_controller.fanInfo.ucTachometerPulsesPerRevolution)
@@ -89,13 +94,28 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
 	tach_period = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
 			CG_TACH_STATUS, TACH_PERIOD);
 
-	if (tach_period == 0)
-		return -EINVAL;
+	if (tach_period == 0) {
 
-	crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
+		duty100 = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
+				CG_FDO_CTRL1, FMAX_DUTY100);
+		duty = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
+				CG_THERMAL_STATUS, FDO_PWM_DUTY);
 
-	*speed = 60 * crystal_clock_freq * 10000 / tach_period;
+		if (duty100 == 0)
+			return -EINVAL;
 
+		tmp64 = (uint64_t)duty * 100;
+		do_div(tmp64, duty100);
+		speed_percent = MIN((uint32_t)tmp64, 100);
+
+		*speed = speed_percent * (hwmgr->thermal_controller.fanInfo.ulMaxRPM
+			- hwmgr->thermal_controller.fanInfo.ulMinRPM) / 100;
+	} else {
+
+		crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
+
+		*speed = 60 * crystal_clock_freq * 10000 / tach_period;
+	}
 	return 0;
 }
 
-- 
2.30.0

Re: [PATCH] drm/amd/pm: Fix that RPM cannot be obtained for specific GPU

[Christian König]

Am 28.09.21 um 02:49 schrieb huangyizhi:
> The current mechanism for obtaining RPM is to read tach_period from
> the register, and then calculate the RPM together with the frequency.
> But we found that on specific GPUs, such as RX 550 and RX 560D,
> tach_period always reads as 0 and smu7_fan_ctrl_get_fan_speed_rpm
> will returns -EINVAL.
>
> To solve this problem, when reading tach_period as 0, we try
> to estimate the current RPM using the percentage of current pwm, the
> maximum and minimum RPM.

Well that is most likely a bad idea.

When the fan speed is not available faking some value is certainly not 
the right solution, especially when you don't know the topology of the 
DC conversion driven by the PWM.

Christian.

>
> Signed-off-by: huangyizhi <huangyizhi@hnu.edu.cn>
> ---
>   .../drm/amd/pm/powerplay/hwmgr/smu7_thermal.c | 28 ++++++++++++++++---
>   1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
> index a6c3610db23e..307dd87d6882 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
> @@ -81,6 +81,11 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
>   {
>   	uint32_t tach_period;
>   	uint32_t crystal_clock_freq;
> +	uint32_t duty100;
> +	uint32_t duty;
> +	uint32_t speed_percent;
> +	uint64_t tmp64;
> +
>   
>   	if (hwmgr->thermal_controller.fanInfo.bNoFan ||
>   	    !hwmgr->thermal_controller.fanInfo.ucTachometerPulsesPerRevolution)
> @@ -89,13 +94,28 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
>   	tach_period = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
>   			CG_TACH_STATUS, TACH_PERIOD);
>   
> -	if (tach_period == 0)
> -		return -EINVAL;
> +	if (tach_period == 0) {
>   
> -	crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
> +		duty100 = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
> +				CG_FDO_CTRL1, FMAX_DUTY100);
> +		duty = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
> +				CG_THERMAL_STATUS, FDO_PWM_DUTY);
>   
> -	*speed = 60 * crystal_clock_freq * 10000 / tach_period;
> +		if (duty100 == 0)
> +			return -EINVAL;
>   
> +		tmp64 = (uint64_t)duty * 100;
> +		do_div(tmp64, duty100);
> +		speed_percent = MIN((uint32_t)tmp64, 100);
> +
> +		*speed = speed_percent * (hwmgr->thermal_controller.fanInfo.ulMaxRPM
> +			- hwmgr->thermal_controller.fanInfo.ulMinRPM) / 100;
> +	} else {
> +
> +		crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
> +
> +		*speed = 60 * crystal_clock_freq * 10000 / tach_period;
> +	}
>   	return 0;
>   }
>   

Re: [PATCH] drm/amd/pm: Fix that RPM cannot be obtained for specific GPU

[Alex Deucher]

On Tue, Sep 28, 2021 at 2:29 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 28.09.21 um 02:49 schrieb huangyizhi:
> > The current mechanism for obtaining RPM is to read tach_period from
> > the register, and then calculate the RPM together with the frequency.
> > But we found that on specific GPUs, such as RX 550 and RX 560D,
> > tach_period always reads as 0 and smu7_fan_ctrl_get_fan_speed_rpm
> > will returns -EINVAL.
> >
> > To solve this problem, when reading tach_period as 0, we try
> > to estimate the current RPM using the percentage of current pwm, the
> > maximum and minimum RPM.
>
> Well that is most likely a bad idea.
>
> When the fan speed is not available faking some value is certainly not
> the right solution, especially when you don't know the topology of the
> DC conversion driven by the PWM.
>

I think there is a flag in the vbios to determine whether a specific
board supports rpm based fan control.  This used to be an AIB specific
option.  If the flag is not set, the driver should not expose the rpm
interface for fan control, only the PWM interface.  I think at some
point rpm fan control became mandatory, but maybe it was still an
option on polaris and we are missing a check for that flag.

Alex


> Christian.
>
> >
> > Signed-off-by: huangyizhi <huangyizhi@hnu.edu.cn>
> > ---
> >   .../drm/amd/pm/powerplay/hwmgr/smu7_thermal.c | 28 ++++++++++++++++---
> >   1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
> > index a6c3610db23e..307dd87d6882 100644
> > --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
> > +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
> > @@ -81,6 +81,11 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
> >   {
> >       uint32_t tach_period;
> >       uint32_t crystal_clock_freq;
> > +     uint32_t duty100;
> > +     uint32_t duty;
> > +     uint32_t speed_percent;
> > +     uint64_t tmp64;
> > +
> >
> >       if (hwmgr->thermal_controller.fanInfo.bNoFan ||
> >           !hwmgr->thermal_controller.fanInfo.ucTachometerPulsesPerRevolution)
> > @@ -89,13 +94,28 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
> >       tach_period = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
> >                       CG_TACH_STATUS, TACH_PERIOD);
> >
> > -     if (tach_period == 0)
> > -             return -EINVAL;
> > +     if (tach_period == 0) {
> >
> > -     crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
> > +             duty100 = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
> > +                             CG_FDO_CTRL1, FMAX_DUTY100);
> > +             duty = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
> > +                             CG_THERMAL_STATUS, FDO_PWM_DUTY);
> >
> > -     *speed = 60 * crystal_clock_freq * 10000 / tach_period;
> > +             if (duty100 == 0)
> > +                     return -EINVAL;
> >
> > +             tmp64 = (uint64_t)duty * 100;
> > +             do_div(tmp64, duty100);
> > +             speed_percent = MIN((uint32_t)tmp64, 100);
> > +
> > +             *speed = speed_percent * (hwmgr->thermal_controller.fanInfo.ulMaxRPM
> > +                     - hwmgr->thermal_controller.fanInfo.ulMinRPM) / 100;
> > +     } else {
> > +
> > +             crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
> > +
> > +             *speed = 60 * crystal_clock_freq * 10000 / tach_period;
> > +     }
> >       return 0;
> >   }
> >
>

Re: [PATCH] drm/amd/pm: Fix that RPM cannot be obtained for specific GPU

[Christian König]

Am 28.09.21 um 23:50 schrieb Alex Deucher:
> On Tue, Sep 28, 2021 at 2:29 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>> Am 28.09.21 um 02:49 schrieb huangyizhi:
>>> The current mechanism for obtaining RPM is to read tach_period from
>>> the register, and then calculate the RPM together with the frequency.
>>> But we found that on specific GPUs, such as RX 550 and RX 560D,
>>> tach_period always reads as 0 and smu7_fan_ctrl_get_fan_speed_rpm
>>> will returns -EINVAL.
>>>
>>> To solve this problem, when reading tach_period as 0, we try
>>> to estimate the current RPM using the percentage of current pwm, the
>>> maximum and minimum RPM.
>> Well that is most likely a bad idea.
>>
>> When the fan speed is not available faking some value is certainly not
>> the right solution, especially when you don't know the topology of the
>> DC conversion driven by the PWM.
>>
> I think there is a flag in the vbios to determine whether a specific
> board supports rpm based fan control.  This used to be an AIB specific
> option.  If the flag is not set, the driver should not expose the rpm
> interface for fan control, only the PWM interface.  I think at some
> point rpm fan control became mandatory, but maybe it was still an
> option on polaris and we are missing a check for that flag.

Yeah, that sounds totally sane to me as well.

Let's ask for a volunteer for the job on Thursday if not somebody from 
the community speaks up.

Christian.

>
> Alex
>
>
>> Christian.
>>
>>> Signed-off-by: huangyizhi <huangyizhi@hnu.edu.cn>
>>> ---
>>>    .../drm/amd/pm/powerplay/hwmgr/smu7_thermal.c | 28 ++++++++++++++++---
>>>    1 file changed, 24 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
>>> index a6c3610db23e..307dd87d6882 100644
>>> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
>>> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_thermal.c
>>> @@ -81,6 +81,11 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
>>>    {
>>>        uint32_t tach_period;
>>>        uint32_t crystal_clock_freq;
>>> +     uint32_t duty100;
>>> +     uint32_t duty;
>>> +     uint32_t speed_percent;
>>> +     uint64_t tmp64;
>>> +
>>>
>>>        if (hwmgr->thermal_controller.fanInfo.bNoFan ||
>>>            !hwmgr->thermal_controller.fanInfo.ucTachometerPulsesPerRevolution)
>>> @@ -89,13 +94,28 @@ int smu7_fan_ctrl_get_fan_speed_rpm(struct pp_hwmgr *hwmgr, uint32_t *speed)
>>>        tach_period = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
>>>                        CG_TACH_STATUS, TACH_PERIOD);
>>>
>>> -     if (tach_period == 0)
>>> -             return -EINVAL;
>>> +     if (tach_period == 0) {
>>>
>>> -     crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
>>> +             duty100 = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
>>> +                             CG_FDO_CTRL1, FMAX_DUTY100);
>>> +             duty = PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC,
>>> +                             CG_THERMAL_STATUS, FDO_PWM_DUTY);
>>>
>>> -     *speed = 60 * crystal_clock_freq * 10000 / tach_period;
>>> +             if (duty100 == 0)
>>> +                     return -EINVAL;
>>>
>>> +             tmp64 = (uint64_t)duty * 100;
>>> +             do_div(tmp64, duty100);
>>> +             speed_percent = MIN((uint32_t)tmp64, 100);
>>> +
>>> +             *speed = speed_percent * (hwmgr->thermal_controller.fanInfo.ulMaxRPM
>>> +                     - hwmgr->thermal_controller.fanInfo.ulMinRPM) / 100;
>>> +     } else {
>>> +
>>> +             crystal_clock_freq = amdgpu_asic_get_xclk((struct amdgpu_device *)hwmgr->adev);
>>> +
>>> +             *speed = 60 * crystal_clock_freq * 10000 / tach_period;
>>> +     }
>>>        return 0;
>>>    }
>>>