[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Tuesday, January 11, 2022 6:53 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Chen, Guchun
> <Guchun.Chen@xxxxxxx>
> Subject: Re: [PATCH] drm/amd/pm: correct the checks for fan attributes
> support
>
>
>
> On 1/11/2022 4:12 PM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> >> Sent: Tuesday, January 11, 2022 6:15 PM
> >> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> >> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Chen, Guchun
> >> <Guchun.Chen@xxxxxxx>
> >> 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@xxxxxxx>
> >>>> Sent: Tuesday, January 11, 2022 4:14 PM
> >>>> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> >>>> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Chen,
> Guchun
> >>>> <Guchun.Chen@xxxxxxx>
> >>>> 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@xxxxxxx>
> >>>>> 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".
[Quan, Evan] OK, please check the updated V2.
BR
Evan
>
> 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,
> >>>>>
