[PATCH] drm/amd/pm: correct the checks for fan attributes support
Lazar, Lijo
lijo.lazar at amd.com
Tue Jan 11 10:52:50 UTC 2022
On 1/11/2022 4:12 PM, Quan, Evan wrote:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Tuesday, January 11, 2022 6:15 PM
>> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Chen, Guchun
>> <Guchun.Chen at amd.com>
>> Subject: Re: [PATCH] drm/amd/pm: correct the checks for fan attributes
>> support
>>
>>
>>
>> On 1/11/2022 3:36 PM, Quan, Evan wrote:
>>> [AMD Official Use Only]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>> Sent: Tuesday, January 11, 2022 4:14 PM
>>>> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Chen, Guchun
>>>> <Guchun.Chen at amd.com>
>>>> Subject: Re: [PATCH] drm/amd/pm: correct the checks for fan
>>>> attributes support
>>>>
>>>>
>>>>
>>>> On 1/11/2022 1:17 PM, Evan Quan wrote:
>>>>> On functionality unsupported, -EOPNOTSUPP will be returned. And we
>>>>> rely on that to determine the fan attributes support.
>>>>>
>>>>> Fixes: 801771de0331 ("drm/amd/pm: do not expose power
>>>> implementation
>>>>> details to
>>>>> amdgpu_pm.c")
>>>>>
>>>>> Signed-off-by: Evan Quan <evan.quan at amd.com>
>>>>> Change-Id: I95e7e0beebd678a446221a72234cd356e14f0fcd
>>>>> ---
>>>>> .../gpu/drm/amd/include/kgd_pp_interface.h | 4 +-
>>>>> drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 31 ++++-
>>>>> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 21 ++--
>>>>> drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 10 +-
>>>>> .../gpu/drm/amd/pm/powerplay/amd_powerplay.c | 59 ++++-----
>>>>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 115
>> +++++++++----
>>>> -----
>>>>> 6 files changed, 132 insertions(+), 108 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>>>>> b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>>>>> index a8eec91c0995..387120099493 100644
>>>>> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>>>>> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
>>>>> @@ -315,8 +315,8 @@ struct amd_pm_funcs {
>>>>> void *rps,
>>>>> bool *equal);
>>>>> /* export for sysfs */
>>>>> - void (*set_fan_control_mode)(void *handle, u32 mode);
>>>>> - u32 (*get_fan_control_mode)(void *handle);
>>>>> + int (*set_fan_control_mode)(void *handle, u32 mode);
>>>>> + int (*get_fan_control_mode)(void *handle, u32 *fan_mode);
>>>>> int (*set_fan_speed_pwm)(void *handle, u32 speed);
>>>>> int (*get_fan_speed_pwm)(void *handle, u32 *speed);
>>>>> int (*force_clock_level)(void *handle, enum pp_clock_type type,
>>>>> uint32_t mask); diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>>>>> b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>>>>> index 68d2e80a673b..fe69785df403 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
>>>>> @@ -1030,15 +1030,20 @@ int
>>>> amdgpu_dpm_get_fan_control_mode(struct amdgpu_device *adev,
>>>>> uint32_t *fan_mode)
>>>>> {
>>>>> const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
>>>>> + int ret = 0;
>>>>>
>>>>> if (!pp_funcs->get_fan_control_mode)
>>>>> return -EOPNOTSUPP;
>>>>>
>>>>> + if (!fan_mode)
>>>>> + return -EINVAL;
>>>>> +
>>>>
>>>> pp_funcs most likely will be there for smu/powerplay subsystem. I
>>>> think the checks should be at one layer down -
>>>> smu_get_fan_control_mode() and
>>>> pp_dpm_get_fan_control_mode()
>>>>
>>>> First one will check if ppt func is implemented and second one will
>>>> check if hwmgr func is implemented for the specific ASIC.
>>> [Quan, Evan] Yes, I agree. And if you go through the changes below, you
>> will see the checks (for the layers down) there.
>>> This patch checks not only those amdgpu_dpm_xxxx APIs, but also those
>> downstream interfaces(smu_xxxx and pp_dpm_xxxx).
>>>
>>
>> Say you call amdgpu_dpm_get_fan_control_mode(adev, NULL) from
>> amdgpu_pm
>>
>> pp_funcs->get_fan_control_mode => this will point to
>> smu_get_fan_control_mode for all swsmu ASICs. So "if (!pp_funcs-
>>> get_fan_control_mode)" will be false.
>>
>> The next statement is NULL check and it will return -EINVAL.
>>
>> What we want to know is ppt_func is implemented or not for the particualr
>> swsmu ASIC. Isn't that the case or am I reading it differently?
>>
> [Quan, Evan] OK, I get your point now. Hmm, that will be a little tricky.
> I suppose the EINVAL check needs to be dispatched in every instance(pp_dpm_xxxxx, smu_xxxx, si_dpm_xxxx) then. Any better idea?
>
Yeah, that is what I meant by "checks should be at one layer down".
Thanks,
Lijo
> BR
> Evan
>> Thanks,
>> Lijo
>>
>>> BR
>>> Evan
>>>>
>>>> Thanks,
>>>> Lijo
>>>>
>>>>> mutex_lock(&adev->pm.mutex);
>>>>> - *fan_mode = pp_funcs->get_fan_control_mode(adev-
>>>>> powerplay.pp_handle);
>>>>> + ret = pp_funcs->get_fan_control_mode(adev-
>>>>> powerplay.pp_handle,
>>>>> + fan_mode);
>>>>> mutex_unlock(&adev->pm.mutex);
>>>>>
>>>>> - return 0;
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> int amdgpu_dpm_set_fan_speed_pwm(struct amdgpu_device *adev,
>>>> @@
>>>>> -1048,6 +1053,9 @@ int amdgpu_dpm_set_fan_speed_pwm(struct
>>>> amdgpu_device *adev,
>>>>> int ret = 0;
>>>>>
>>>>> if (!pp_funcs->set_fan_speed_pwm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (speed == U32_MAX)
>>>>> return -EINVAL;
>>>>>
>>>>> mutex_lock(&adev->pm.mutex);
>>>>> @@ -1065,6 +1073,9 @@ int amdgpu_dpm_get_fan_speed_pwm(struct
>>>> amdgpu_device *adev,
>>>>> int ret = 0;
>>>>>
>>>>> if (!pp_funcs->get_fan_speed_pwm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (!speed)
>>>>> return -EINVAL;
>>>>>
>>>>> mutex_lock(&adev->pm.mutex);
>>>>> @@ -1082,6 +1093,9 @@ int amdgpu_dpm_get_fan_speed_rpm(struct
>>>> amdgpu_device *adev,
>>>>> int ret = 0;
>>>>>
>>>>> if (!pp_funcs->get_fan_speed_rpm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (!speed)
>>>>> return -EINVAL;
>>>>>
>>>>> mutex_lock(&adev->pm.mutex);
>>>>> @@ -1099,6 +1113,9 @@ int amdgpu_dpm_set_fan_speed_rpm(struct
>>>> amdgpu_device *adev,
>>>>> int ret = 0;
>>>>>
>>>>> if (!pp_funcs->set_fan_speed_rpm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (speed == U32_MAX)
>>>>> return -EINVAL;
>>>>>
>>>>> mutex_lock(&adev->pm.mutex);
>>>>> @@ -1113,16 +1130,20 @@ int
>>>> amdgpu_dpm_set_fan_control_mode(struct amdgpu_device *adev,
>>>>> uint32_t mode)
>>>>> {
>>>>> const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
>>>>> + int ret = 0;
>>>>>
>>>>> if (!pp_funcs->set_fan_control_mode)
>>>>> return -EOPNOTSUPP;
>>>>>
>>>>> + if (mode == U32_MAX)
>>>>> + return -EINVAL;
>>>>> +
>>>>> mutex_lock(&adev->pm.mutex);
>>>>> - pp_funcs->set_fan_control_mode(adev->powerplay.pp_handle,
>>>>> - mode);
>>>>> + ret = pp_funcs->set_fan_control_mode(adev-
>>>>> powerplay.pp_handle,
>>>>> + mode);
>>>>> mutex_unlock(&adev->pm.mutex);
>>>>>
>>>>> - return 0;
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> int amdgpu_dpm_get_power_limit(struct amdgpu_device *adev, diff
>>>>> --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>>>> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>>>> index d3eab245e0fe..940cbe751163 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
>>>>> @@ -3197,7 +3197,6 @@ static umode_t
>> hwmon_attributes_visible(struct
>>>> kobject *kobj,
>>>>> struct device *dev = kobj_to_dev(kobj);
>>>>> struct amdgpu_device *adev = dev_get_drvdata(dev);
>>>>> umode_t effective_mode = attr->mode;
>>>>> - uint32_t speed = 0;
>>>>>
>>>>> /* under multi-vf mode, the hwmon attributes are all not
>>>>> supported
>>>> */
>>>>> if (amdgpu_sriov_vf(adev) && !amdgpu_sriov_is_pp_one_vf(adev))
>>>> @@
>>>>> -3263,15 +3262,15 @@ static umode_t hwmon_attributes_visible(struct
>>>> kobject *kobj,
>>>>> return 0;
>>>>>
>>>>> /* mask fan attributes if we have no bindings for this asic to
>>>>> expose
>>>> */
>>>>> - if (((amdgpu_dpm_get_fan_speed_pwm(adev, &speed) == -EINVAL)
>>>> &&
>>>>> + if (((amdgpu_dpm_get_fan_speed_pwm(adev, NULL) == -
>>>> EOPNOTSUPP) &&
>>>>> attr == &sensor_dev_attr_pwm1.dev_attr.attr) || /* can't
>>>>> query
>>>> fan */
>>>>> - ((amdgpu_dpm_get_fan_control_mode(adev, &speed) == -
>>>> EOPNOTSUPP) &&
>>>>> + ((amdgpu_dpm_get_fan_control_mode(adev, NULL) == -
>>>> EOPNOTSUPP) &&
>>>>> attr == &sensor_dev_attr_pwm1_enable.dev_attr.attr)) /*
>>>>> can't
>>>> query state */
>>>>> effective_mode &= ~S_IRUGO;
>>>>>
>>>>> - if (((amdgpu_dpm_set_fan_speed_pwm(adev, speed) == -EINVAL)
>>>> &&
>>>>> + if (((amdgpu_dpm_set_fan_speed_pwm(adev, U32_MAX) == -
>>>> EOPNOTSUPP) &&
>>>>> attr == &sensor_dev_attr_pwm1.dev_attr.attr) || /* can't
>>>> manage fan */
>>>>> - ((amdgpu_dpm_set_fan_control_mode(adev, speed) == -
>>>> EOPNOTSUPP) &&
>>>>> + ((amdgpu_dpm_set_fan_control_mode(adev, U32_MAX) ==
>>>>> +-EOPNOTSUPP) &&
>>>>> attr == &sensor_dev_attr_pwm1_enable.dev_attr.attr)) /*
>>>>> can't
>>>> manage state */
>>>>> effective_mode &= ~S_IWUSR;
>>>>>
>>>>> @@ -3291,16 +3290,16 @@ static umode_t
>>>> hwmon_attributes_visible(struct kobject *kobj,
>>>>> return 0;
>>>>>
>>>>> /* hide max/min values if we can't both query and manage the fan */
>>>>> - if (((amdgpu_dpm_set_fan_speed_pwm(adev, speed) == -EINVAL)
>>>> &&
>>>>> - (amdgpu_dpm_get_fan_speed_pwm(adev, &speed) == -EINVAL)
>>>> &&
>>>>> - (amdgpu_dpm_set_fan_speed_rpm(adev, speed) == -EINVAL)
>>>> &&
>>>>> - (amdgpu_dpm_get_fan_speed_rpm(adev, &speed) == -EINVAL))
>>>> &&
>>>>> + if (((amdgpu_dpm_set_fan_speed_pwm(adev, U32_MAX) == -
>>>> EOPNOTSUPP) &&
>>>>> + (amdgpu_dpm_get_fan_speed_pwm(adev, NULL) == -
>>>> EOPNOTSUPP) &&
>>>>> + (amdgpu_dpm_set_fan_speed_rpm(adev, U32_MAX) == -
>>>> EOPNOTSUPP) &&
>>>>> + (amdgpu_dpm_get_fan_speed_rpm(adev, NULL) == -
>>>> EOPNOTSUPP)) &&
>>>>> (attr == &sensor_dev_attr_pwm1_max.dev_attr.attr ||
>>>>> attr == &sensor_dev_attr_pwm1_min.dev_attr.attr))
>>>>> return 0;
>>>>>
>>>>> - if ((amdgpu_dpm_set_fan_speed_rpm(adev, speed) == -EINVAL)
>>>> &&
>>>>> - (amdgpu_dpm_get_fan_speed_rpm(adev, &speed) == -EINVAL)
>>>> &&
>>>>> + if ((amdgpu_dpm_set_fan_speed_rpm(adev, U32_MAX) == -
>>>> EOPNOTSUPP) &&
>>>>> + (amdgpu_dpm_get_fan_speed_rpm(adev, NULL) == -
>>>> EOPNOTSUPP) &&
>>>>> (attr == &sensor_dev_attr_fan1_max.dev_attr.attr ||
>>>>> attr == &sensor_dev_attr_fan1_min.dev_attr.attr))
>>>>> return 0;
>>>>> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
>>>>> b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
>>>>> index 92b987fb31d4..7677d3a21f8c 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
>>>>> @@ -6669,7 +6669,7 @@ static int si_dpm_set_fan_speed_pwm(void
>>>> *handle,
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static void si_dpm_set_fan_control_mode(void *handle, u32 mode)
>>>>> +static int si_dpm_set_fan_control_mode(void *handle, u32 mode)
>>>>> {
>>>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>>>
>>>>> @@ -6685,9 +6685,11 @@ static void
>> si_dpm_set_fan_control_mode(void
>>>> *handle, u32 mode)
>>>>> else
>>>>> si_fan_ctrl_set_default_mode(adev);
>>>>> }
>>>>> +
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> -static u32 si_dpm_get_fan_control_mode(void *handle)
>>>>> +static int si_dpm_get_fan_control_mode(void *handle, u32
>> *fan_mode)
>>>>> {
>>>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>>> struct si_power_info *si_pi = si_get_pi(adev); @@ -6697,7
>>>>> +6699,9 @@ static u32 si_dpm_get_fan_control_mode(void *handle)
>>>>> return 0;
>>>>>
>>>>> tmp = RREG32(CG_FDO_CTRL2) & FDO_PWM_MODE_MASK;
>>>>> - return (tmp >> FDO_PWM_MODE_SHIFT);
>>>>> + *fan_mode = (tmp >> FDO_PWM_MODE_SHIFT);
>>>>> +
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> #if 0
>>>>> diff --git a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
>>>>> b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
>>>>> index 89341729744d..57bc9405d6a9 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
>>>>> @@ -488,38 +488,37 @@ static enum amd_pm_state_type
>>>> pp_dpm_get_current_power_state(void *handle)
>>>>> return pm_type;
>>>>> }
>>>>>
>>>>> -static void pp_dpm_set_fan_control_mode(void *handle, uint32_t
>>>>> mode)
>>>>> +static int pp_dpm_set_fan_control_mode(void *handle, uint32_t
>> mode)
>>>>> {
>>>>> struct pp_hwmgr *hwmgr = handle;
>>>>>
>>>>> if (!hwmgr || !hwmgr->pm_en)
>>>>> - return;
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (hwmgr->hwmgr_func->set_fan_control_mode == NULL)
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> - if (hwmgr->hwmgr_func->set_fan_control_mode == NULL) {
>>>>> - pr_info_ratelimited("%s was not implemented.\n",
>>>> __func__);
>>>>> - return;
>>>>> - }
>>>>> mutex_lock(&hwmgr->smu_lock);
>>>>> hwmgr->hwmgr_func->set_fan_control_mode(hwmgr, mode);
>>>>> mutex_unlock(&hwmgr->smu_lock);
>>>>> +
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> -static uint32_t pp_dpm_get_fan_control_mode(void *handle)
>>>>> +static int pp_dpm_get_fan_control_mode(void *handle, uint32_t
>>>>> +*fan_mode)
>>>>> {
>>>>> struct pp_hwmgr *hwmgr = handle;
>>>>> - uint32_t mode = 0;
>>>>>
>>>>> if (!hwmgr || !hwmgr->pm_en)
>>>>> - return 0;
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (hwmgr->hwmgr_func->get_fan_control_mode == NULL)
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> - if (hwmgr->hwmgr_func->get_fan_control_mode == NULL) {
>>>>> - pr_info_ratelimited("%s was not implemented.\n",
>>>> __func__);
>>>>> - return 0;
>>>>> - }
>>>>> mutex_lock(&hwmgr->smu_lock);
>>>>> - mode = hwmgr->hwmgr_func->get_fan_control_mode(hwmgr);
>>>>> + *fan_mode = hwmgr->hwmgr_func-
>>>>> get_fan_control_mode(hwmgr);
>>>>> mutex_unlock(&hwmgr->smu_lock);
>>>>> - return mode;
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> static int pp_dpm_set_fan_speed_pwm(void *handle, uint32_t speed)
>>>> @@
>>>>> -528,12 +527,11 @@ static int pp_dpm_set_fan_speed_pwm(void
>> *handle,
>>>> uint32_t speed)
>>>>> int ret = 0;
>>>>>
>>>>> if (!hwmgr || !hwmgr->pm_en)
>>>>> - return -EINVAL;
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (hwmgr->hwmgr_func->set_fan_speed_pwm == NULL)
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> - if (hwmgr->hwmgr_func->set_fan_speed_pwm == NULL) {
>>>>> - pr_info_ratelimited("%s was not implemented.\n",
>>>> __func__);
>>>>> - return 0;
>>>>> - }
>>>>> mutex_lock(&hwmgr->smu_lock);
>>>>> ret = hwmgr->hwmgr_func->set_fan_speed_pwm(hwmgr, speed);
>>>>> mutex_unlock(&hwmgr->smu_lock);
>>>>> @@ -546,12 +544,10 @@ static int pp_dpm_get_fan_speed_pwm(void
>>>> *handle, uint32_t *speed)
>>>>> int ret = 0;
>>>>>
>>>>> if (!hwmgr || !hwmgr->pm_en)
>>>>> - return -EINVAL;
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> - if (hwmgr->hwmgr_func->get_fan_speed_pwm == NULL) {
>>>>> - pr_info_ratelimited("%s was not implemented.\n",
>>>> __func__);
>>>>> - return 0;
>>>>> - }
>>>>> + if (hwmgr->hwmgr_func->get_fan_speed_pwm == NULL)
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> mutex_lock(&hwmgr->smu_lock);
>>>>> ret = hwmgr->hwmgr_func->get_fan_speed_pwm(hwmgr, speed);
>>>> @@
>>>>> -565,10 +561,10 @@ static int pp_dpm_get_fan_speed_rpm(void
>> *handle,
>>>> uint32_t *rpm)
>>>>> int ret = 0;
>>>>>
>>>>> if (!hwmgr || !hwmgr->pm_en)
>>>>> - return -EINVAL;
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> if (hwmgr->hwmgr_func->get_fan_speed_rpm == NULL)
>>>>> - return -EINVAL;
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> mutex_lock(&hwmgr->smu_lock);
>>>>> ret = hwmgr->hwmgr_func->get_fan_speed_rpm(hwmgr, rpm);
>>>> @@ -582,12
>>>>> +578,11 @@ static int pp_dpm_set_fan_speed_rpm(void *handle,
>>>> uint32_t rpm)
>>>>> int ret = 0;
>>>>>
>>>>> if (!hwmgr || !hwmgr->pm_en)
>>>>> - return -EINVAL;
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (hwmgr->hwmgr_func->set_fan_speed_rpm == NULL)
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> - if (hwmgr->hwmgr_func->set_fan_speed_rpm == NULL) {
>>>>> - pr_info_ratelimited("%s was not implemented.\n",
>>>> __func__);
>>>>> - return 0;
>>>>> - }
>>>>> mutex_lock(&hwmgr->smu_lock);
>>>>> ret = hwmgr->hwmgr_func->set_fan_speed_rpm(hwmgr, rpm);
>>>>> mutex_unlock(&hwmgr->smu_lock);
>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>> index c374c3067496..474f1f04cc96 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>> @@ -59,7 +59,7 @@ static int smu_handle_task(struct smu_context
>> *smu,
>>>>> bool lock_needed);
>>>>> static int smu_reset(struct smu_context *smu);
>>>>> static int smu_set_fan_speed_pwm(void *handle, u32 speed);
>>>>> -static int smu_set_fan_control_mode(struct smu_context *smu, int
>>>>> value);
>>>>> +static int smu_set_fan_control_mode(void *handle, u32 value);
>>>>> static int smu_set_power_limit(void *handle, uint32_t limit);
>>>>> static int smu_set_fan_speed_rpm(void *handle, uint32_t speed);
>>>>> static int smu_set_gfx_cgpg(struct smu_context *smu, bool
>>>>> enabled); @@ -407,7 +407,7 @@ static void
>>>>> smu_restore_dpm_user_profile(struct
>>>> smu_context *smu)
>>>>> if (smu->user_dpm_profile.fan_mode ==
>>>> AMD_FAN_CTRL_MANUAL ||
>>>>> smu->user_dpm_profile.fan_mode == AMD_FAN_CTRL_NONE) {
>>>>> ret = smu_set_fan_control_mode(smu, smu-
>>>>> user_dpm_profile.fan_mode);
>>>>> - if (ret) {
>>>>> + if (ret != -EOPNOTSUPP) {
>>>>> smu->user_dpm_profile.fan_speed_pwm = 0;
>>>>> smu->user_dpm_profile.fan_speed_rpm = 0;
>>>>> smu->user_dpm_profile.fan_mode =
>>>> AMD_FAN_CTRL_AUTO; @@ -416,13
>>>>> +416,13 @@ static void smu_restore_dpm_user_profile(struct
>>>> smu_context
>>>>> *smu)
>>>>>
>>>>> if (smu->user_dpm_profile.fan_speed_pwm) {
>>>>> ret = smu_set_fan_speed_pwm(smu, smu-
>>>>> user_dpm_profile.fan_speed_pwm);
>>>>> - if (ret)
>>>>> + if (ret != -EOPNOTSUPP)
>>>>> dev_err(smu->adev->dev, "Failed to set
>>>> manual fan speed in pwm\n");
>>>>> }
>>>>>
>>>>> if (smu->user_dpm_profile.fan_speed_rpm) {
>>>>> ret = smu_set_fan_speed_rpm(smu, smu-
>>>>> user_dpm_profile.fan_speed_rpm);
>>>>> - if (ret)
>>>>> + if (ret != -EOPNOTSUPP)
>>>>> dev_err(smu->adev->dev, "Failed to set
>>>> manual fan speed in rpm\n");
>>>>> }
>>>>> }
>>>>> @@ -2218,18 +2218,19 @@ static int smu_set_fan_speed_rpm(void
>>>> *handle, uint32_t speed)
>>>>> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
>>>>> return -EOPNOTSUPP;
>>>>>
>>>>> + if (!smu->ppt_funcs->set_fan_speed_rpm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> mutex_lock(&smu->mutex);
>>>>>
>>>>> - 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)) {
>>>>> - smu->user_dpm_profile.flags |=
>>>> SMU_CUSTOM_FAN_SPEED_RPM;
>>>>> - smu->user_dpm_profile.fan_speed_rpm = speed;
>>>>> + ret = smu->ppt_funcs->set_fan_speed_rpm(smu, speed);
>>>>> + if (!ret && !(smu->user_dpm_profile.flags &
>>>> SMU_DPM_USER_PROFILE_RESTORE)) {
>>>>> + smu->user_dpm_profile.flags |=
>>>> SMU_CUSTOM_FAN_SPEED_RPM;
>>>>> + smu->user_dpm_profile.fan_speed_rpm = speed;
>>>>>
>>>>> - /* Override custom PWM setting as they cannot co-
>>>> exist */
>>>>> - smu->user_dpm_profile.flags &=
>>>> ~SMU_CUSTOM_FAN_SPEED_PWM;
>>>>> - smu->user_dpm_profile.fan_speed_pwm = 0;
>>>>> - }
>>>>> + /* Override custom PWM setting as they cannot co-exist */
>>>>> + smu->user_dpm_profile.flags &=
>>>> ~SMU_CUSTOM_FAN_SPEED_PWM;
>>>>> + smu->user_dpm_profile.fan_speed_pwm = 0;
>>>>> }
>>>>>
>>>>> mutex_unlock(&smu->mutex);
>>>>> @@ -2562,60 +2563,59 @@ static int
>> smu_set_power_profile_mode(void
>>>> *handle,
>>>>> }
>>>>>
>>>>>
>>>>> -static u32 smu_get_fan_control_mode(void *handle)
>>>>> +static int smu_get_fan_control_mode(void *handle, u32 *fan_mode)
>>>>> {
>>>>> struct smu_context *smu = handle;
>>>>> - u32 ret = 0;
>>>>>
>>>>> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
>>>>> - return AMD_FAN_CTRL_NONE;
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (!smu->ppt_funcs->get_fan_control_mode)
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> mutex_lock(&smu->mutex);
>>>>>
>>>>> - if (smu->ppt_funcs->get_fan_control_mode)
>>>>> - ret = smu->ppt_funcs->get_fan_control_mode(smu);
>>>>> + *fan_mode = smu->ppt_funcs->get_fan_control_mode(smu);
>>>>>
>>>>> mutex_unlock(&smu->mutex);
>>>>>
>>>>> - return ret;
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> -static int smu_set_fan_control_mode(struct smu_context *smu, int
>>>>> value)
>>>>> +static int smu_set_fan_control_mode(void *handle, u32 value)
>>>>> {
>>>>> + struct smu_context *smu = handle;
>>>>> int ret = 0;
>>>>>
>>>>> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
>>>>> - return -EOPNOTSUPP;
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (!smu->ppt_funcs->set_fan_control_mode)
>>>>> + return -EOPNOTSUPP;
>>>>>
>>>>> mutex_lock(&smu->mutex);
>>>>>
>>>>> - if (smu->ppt_funcs->set_fan_control_mode) {
>>>>> - ret = smu->ppt_funcs->set_fan_control_mode(smu, value);
>>>>> - if (!ret && !(smu->user_dpm_profile.flags &
>>>> SMU_DPM_USER_PROFILE_RESTORE))
>>>>> - smu->user_dpm_profile.fan_mode = value;
>>>>> - }
>>>>> + ret = smu->ppt_funcs->set_fan_control_mode(smu, value);
>>>>> + if (ret)
>>>>> + goto out;
>>>>>
>>>>> - mutex_unlock(&smu->mutex);
>>>>> + if (!(smu->user_dpm_profile.flags &
>>>> SMU_DPM_USER_PROFILE_RESTORE)) {
>>>>> + smu->user_dpm_profile.fan_mode = 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.fan_speed_pwm = 0;
>>>>> - smu->user_dpm_profile.fan_speed_rpm = 0;
>>>>> - smu->user_dpm_profile.flags &=
>>>> ~(SMU_CUSTOM_FAN_SPEED_RPM |
>> SMU_CUSTOM_FAN_SPEED_PWM);
>>>>> + /* reset user dpm fan speed */
>>>>> + if (value != AMD_FAN_CTRL_MANUAL) {
>>>>> + smu->user_dpm_profile.fan_speed_pwm = 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;
>>>>> -}
>>>>> -
>>>>> -static void smu_pp_set_fan_control_mode(void *handle, u32 value) -{
>>>>> - struct smu_context *smu = handle;
>>>>> +out:
>>>>> + mutex_unlock(&smu->mutex);
>>>>>
>>>>> - smu_set_fan_control_mode(smu, value);
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> -
>>>>> static int smu_get_fan_speed_pwm(void *handle, u32 *speed)
>>>>> {
>>>>> struct smu_context *smu = handle; @@ -2624,10 +2624,12 @@ static
>>>>> int smu_get_fan_speed_pwm(void
>>>> *handle, u32 *speed)
>>>>> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
>>>>> return -EOPNOTSUPP;
>>>>>
>>>>> + if (!smu->ppt_funcs->get_fan_speed_pwm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> mutex_lock(&smu->mutex);
>>>>>
>>>>> - if (smu->ppt_funcs->get_fan_speed_pwm)
>>>>> - ret = smu->ppt_funcs->get_fan_speed_pwm(smu, speed);
>>>>> + ret = smu->ppt_funcs->get_fan_speed_pwm(smu, speed);
>>>>>
>>>>> mutex_unlock(&smu->mutex);
>>>>>
>>>>> @@ -2642,18 +2644,19 @@ static int smu_set_fan_speed_pwm(void
>>>> *handle, u32 speed)
>>>>> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
>>>>> return -EOPNOTSUPP;
>>>>>
>>>>> + if (!smu->ppt_funcs->set_fan_speed_pwm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> mutex_lock(&smu->mutex);
>>>>>
>>>>> - if (smu->ppt_funcs->set_fan_speed_pwm) {
>>>>> - ret = smu->ppt_funcs->set_fan_speed_pwm(smu, speed);
>>>>> - 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_pwm = speed;
>>>>> + ret = smu->ppt_funcs->set_fan_speed_pwm(smu, speed);
>>>>> + 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_pwm = speed;
>>>>>
>>>>> - /* Override custom RPM setting as they cannot co-
>>>> exist */
>>>>> - smu->user_dpm_profile.flags &=
>>>> ~SMU_CUSTOM_FAN_SPEED_RPM;
>>>>> - smu->user_dpm_profile.fan_speed_rpm = 0;
>>>>> - }
>>>>> + /* Override custom RPM setting as they cannot co-exist */
>>>>> + smu->user_dpm_profile.flags &=
>>>> ~SMU_CUSTOM_FAN_SPEED_RPM;
>>>>> + smu->user_dpm_profile.fan_speed_rpm = 0;
>>>>> }
>>>>>
>>>>> mutex_unlock(&smu->mutex);
>>>>> @@ -2669,10 +2672,12 @@ static int smu_get_fan_speed_rpm(void
>>>> *handle, uint32_t *speed)
>>>>> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
>>>>> return -EOPNOTSUPP;
>>>>>
>>>>> + if (!smu->ppt_funcs->get_fan_speed_rpm)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> mutex_lock(&smu->mutex);
>>>>>
>>>>> - if (smu->ppt_funcs->get_fan_speed_rpm)
>>>>> - ret = smu->ppt_funcs->get_fan_speed_rpm(smu, speed);
>>>>> + ret = smu->ppt_funcs->get_fan_speed_rpm(smu, speed);
>>>>>
>>>>> mutex_unlock(&smu->mutex);
>>>>>
>>>>> @@ -3101,7 +3106,7 @@ static int smu_get_prv_buffer_details(void
>>>>> *handle, void **addr, size_t *size)
>>>>>
>>>>> static const struct amd_pm_funcs swsmu_pm_funcs = {
>>>>> /* export for sysfs */
>>>>> - .set_fan_control_mode = smu_pp_set_fan_control_mode,
>>>>> + .set_fan_control_mode = smu_set_fan_control_mode,
>>>>> .get_fan_control_mode = smu_get_fan_control_mode,
>>>>> .set_fan_speed_pwm = smu_set_fan_speed_pwm,
>>>>> .get_fan_speed_pwm = smu_get_fan_speed_pwm,
>>>>>
More information about the amd-gfx
mailing list