> -----Original Message-----
> From: Russell, Kent
> Sent: Friday, October 5, 2018 10:40 AM
> To: Zhu, Rex <Rex.Zhu@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
>
> Sorry, I just saw something with this and had a question. Why does the fan
> control have to be set to manual to read the RPMs? I understand that it
> needs to be manual to set the RPMs, but why would we need that to read
> the current RPMs?
>
Good catch. That part should be dropped.
Alex
> Kent
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Zhu,
> Rex
> Sent: Monday, October 01, 2018 9:34 AM
> To: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; amd-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
>
>
>
> > -----Original Message-----
> > From: Deucher, Alexander
> > Sent: Monday, October 1, 2018 9:16 PM
> > To: Zhu, Rex <Rex.Zhu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Zhu, Rex <Rex.Zhu@xxxxxxx>
> > Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> >
> > > -----Original Message-----
> > > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > > Rex Zhu
> > > Sent: Sunday, September 30, 2018 2:18 AM
> > > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Zhu, Rex <Rex.Zhu@xxxxxxx>
> > > Subject: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> > >
> > > Add fan1_target for get/set fan speed in RPM unit Add
> > > fan1_min/fan1_max for get min, max fan speed in RPM unit Add
> > > fan1_enable to enable/disable the fan1 sensor
> > >
> > > v2: query the min/max rpm gpu support instand of hardcode.
> > >
> > > Signed-off-by: Rex Zhu <Rex.Zhu@xxxxxxx>
> > > ---
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h | 3 +
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 190
> > > ++++++++++++++++++++++++-
> > > drivers/gpu/drm/amd/include/kgd_pp_interface.h | 1 +
> > > drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 19 +++
> > > 4 files changed, 210 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > index 42568ae..f972cd1 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.h
> > > @@ -278,6 +278,9 @@ enum amdgpu_pcie_gen { #define
> > > amdgpu_dpm_get_fan_speed_rpm(adev, s) \
> > > ((adev)->powerplay.pp_funcs-
> > > >get_fan_speed_rpm)((adev)->powerplay.pp_handle, (s))
> > >
> > > +#define amdgpu_dpm_set_fan_speed_rpm(adev, s) \
> > > +
> > > +((adev)->powerplay.pp_funcs->set_fan_speed_rpm)((adev)-
> > > >powerplay.pp_ha
> > > +ndle, (s))
> > > +
> > > #define amdgpu_dpm_get_sclk(adev, l) \
> > > ((adev)->powerplay.pp_funcs->get_sclk((adev)-
> > > >powerplay.pp_handle, (l)))
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > index 18d989e..1d85706 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> > > @@ -1172,6 +1172,11 @@ static ssize_t
> > > amdgpu_hwmon_get_fan1_input(struct device *dev,
> > > struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > int err;
> > > u32 speed = 0;
> > > + u32 pwm_mode;
> > > +
> > > + pwm_mode = amdgpu_dpm_get_fan_control_mode(adev);
> > > + if (pwm_mode != AMD_FAN_CTRL_MANUAL)
> > > + return -ENODATA;
> > >
> > > /* Can't adjust fan when the card is off */
> > > if ((adev->flags & AMD_IS_PX) &&
> > > @@ -1187,6 +1192,153 @@ static ssize_t
> > > amdgpu_hwmon_get_fan1_input(struct device *dev,
> > > return sprintf(buf, "%i\n", speed); }
> > >
> > > +static ssize_t amdgpu_hwmon_get_fan1_min(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > + u32 min_rpm = 0;
> > > + u32 size = sizeof(min_rpm);
> > > + int r;
> > > +
> > > + if (!adev->powerplay.pp_funcs->read_sensor)
> > > + return -EINVAL;
> > > +
> > > + r = amdgpu_dpm_read_sensor(adev,
> > > AMDGPU_PP_SENSOR_MIN_FAN_RPM,
> > > + (void *)&min_rpm, &size);
> > > + if (r)
> > > + return r;
> > > +
> > > + return snprintf(buf, PAGE_SIZE, "%d\n", min_rpm); }
> > > +
> > > +static ssize_t amdgpu_hwmon_get_fan1_max(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > + u32 max_rpm = 0;
> > > + u32 size = sizeof(max_rpm);
> > > + int r;
> > > +
> > > + if (!adev->powerplay.pp_funcs->read_sensor)
> > > + return -EINVAL;
> > > +
> > > + r = amdgpu_dpm_read_sensor(adev,
> > > AMDGPU_PP_SENSOR_MAX_FAN_RPM,
> > > + (void *)&max_rpm, &size);
> > > + if (r)
> > > + return r;
> > > +
> > > + return snprintf(buf, PAGE_SIZE, "%d\n", max_rpm); }
> > > +
> > > +static ssize_t amdgpu_hwmon_get_fan1_target(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > + int err;
> > > + u32 rpm = 0;
> > > + u32 pwm_mode;
> > > +
> > > + pwm_mode = amdgpu_dpm_get_fan_control_mode(adev);
> > > + if (pwm_mode != AMD_FAN_CTRL_MANUAL)
> > > + return -ENODATA;
> > > +
> > > + /* Can't adjust fan when the card is off */
> > > + if ((adev->flags & AMD_IS_PX) &&
> > > + (adev->ddev->switch_power_state !=
> > > DRM_SWITCH_POWER_ON))
> > > + return -EINVAL;
> > > +
> > > + if (adev->powerplay.pp_funcs->get_fan_speed_rpm) {
> > > + err = amdgpu_dpm_get_fan_speed_rpm(adev, &rpm);
> > > + if (err)
> > > + return err;
> > > + }
> > > +
> > > + return sprintf(buf, "%i\n", rpm);
> > > +}
> > > +
> > > +static ssize_t amdgpu_hwmon_set_fan1_target(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count) {
> > > + struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > + int err;
> > > + u32 value;
> > > + u32 pwm_mode;
> > > +
> > > + pwm_mode = amdgpu_dpm_get_fan_control_mode(adev);
> > > + if (pwm_mode != AMD_FAN_CTRL_MANUAL)
> > > + return -ENODATA;
> > > +
> > > + /* Can't adjust fan when the card is off */
> > > + if ((adev->flags & AMD_IS_PX) &&
> > > + (adev->ddev->switch_power_state !=
> > > DRM_SWITCH_POWER_ON))
> > > + return -EINVAL;
> > > +
> > > + err = kstrtou32(buf, 10, &value);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (adev->powerplay.pp_funcs->set_fan_speed_rpm) {
> > > + err = amdgpu_dpm_set_fan_speed_rpm(adev, value);
> > > + if (err)
> > > + return err;
> > > + }
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static ssize_t amdgpu_hwmon_get_fan1_enable(struct device *dev,
> > > + struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > + u32 pwm_mode = 0;
> > > +
> > > + if (!adev->powerplay.pp_funcs->get_fan_control_mode)
> > > + return -EINVAL;
> > > +
> > > + pwm_mode = amdgpu_dpm_get_fan_control_mode(adev);
> > > +
> > > + return sprintf(buf, "%i\n", pwm_mode == AMD_FAN_CTRL_AUTO ?
> > > 0 : 1); }
> > > +
> > > +static ssize_t amdgpu_hwmon_set_fan1_enable(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf,
> > > + size_t count)
> > > +{
> > > + struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > + int err;
> > > + int value;
> > > + u32 pwm_mode;
> > > +
> > > + /* Can't adjust fan when the card is off */
> > > + if ((adev->flags & AMD_IS_PX) &&
> > > + (adev->ddev->switch_power_state !=
> > > DRM_SWITCH_POWER_ON))
> > > + return -EINVAL;
> > > +
> > > + if (!adev->powerplay.pp_funcs->set_fan_control_mode)
> > > + return -EINVAL;
> > > +
> > > + err = kstrtoint(buf, 10, &value);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (value == 0)
> > > + pwm_mode = AMD_FAN_CTRL_AUTO;
> > > + else if (value == 1)
> > > + pwm_mode = AMD_FAN_CTRL_MANUAL;
> > > + else
> > > + return -EINVAL;
> > > +
> > > + amdgpu_dpm_set_fan_control_mode(adev, pwm_mode);
> > > +
> > > + return count;
> > > +}
> > > +
> > > static ssize_t amdgpu_hwmon_show_vddgfx(struct device *dev,
> > > struct device_attribute *attr,
> > > char *buf)
> > > @@ -1406,8 +1558,16 @@ static ssize_t
> > > amdgpu_hwmon_set_power_cap(struct device *dev,
> > > *
> > > * - pwm1_max: pulse width modulation fan control maximum level (255)
> > > *
> > > + * - fan1_min: an minimum value Unit: revolution/min (RPM) (0)
> > > + *
> > > + * - fan1_max: an maxmum value Unit: revolution/max (RPM) (6000)
> > > + *
> >
> > The min and max may not always be 0 and 6000; it depends on the board
> > IIRC. Probably better to drop the (0) and (6000).
>
> I forgot to drop this hardcode value in comments.
> Thanks for pointing out this.
> Will drop the value in V3.
>
> Rex
>
> > Alex
> >
> > > * - fan1_input: fan speed in RPM
> > > *
> > > + * - fan[1-*]_target: Desired fan speed Unit: revolution/min (RPM)
> > > + *
> > > + * - fan[1-*]_enable: Enable or disable the sensors.1: Enable 0:
> > > + Disable
> > > + *
> > > * You can use hwmon tools like sensors to view this information on
> > > your system.
> > > *
> > > */
> > > @@ -1420,6 +1580,10 @@ static ssize_t
> > > amdgpu_hwmon_set_power_cap(struct device *dev, static
> > > SENSOR_DEVICE_ATTR(pwm1_min, S_IRUGO,
> > amdgpu_hwmon_get_pwm1_min, NULL,
> > > 0); static SENSOR_DEVICE_ATTR(pwm1_max, S_IRUGO,
> > > amdgpu_hwmon_get_pwm1_max, NULL, 0); static
> > > SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO,
> > amdgpu_hwmon_get_fan1_input,
> > > NULL, 0);
> > > +static SENSOR_DEVICE_ATTR(fan1_min, S_IRUGO,
> > > amdgpu_hwmon_get_fan1_min,
> > > +NULL, 0); static SENSOR_DEVICE_ATTR(fan1_max, S_IRUGO,
> > > +amdgpu_hwmon_get_fan1_max, NULL, 0); static
> > > +SENSOR_DEVICE_ATTR(fan1_target, S_IRUGO | S_IWUSR,
> > > +amdgpu_hwmon_get_fan1_target, amdgpu_hwmon_set_fan1_target,
> 0);
> > > static
> > > +SENSOR_DEVICE_ATTR(fan1_enable, S_IRUGO | S_IWUSR,
> > > +amdgpu_hwmon_get_fan1_enable,
> amdgpu_hwmon_set_fan1_enable,
> > > 0);
> > > static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO,
> > > amdgpu_hwmon_show_vddgfx, NULL, 0); static
> > > SENSOR_DEVICE_ATTR(in0_label, S_IRUGO,
> > amdgpu_hwmon_show_vddgfx_label,
> > > NULL, 0); static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO,
> > > amdgpu_hwmon_show_vddnb, NULL, 0); @@ -1438,6 +1602,10 @@
> static
> > > ssize_t amdgpu_hwmon_set_power_cap(struct device *dev,
> > > &sensor_dev_attr_pwm1_min.dev_attr.attr,
> > > &sensor_dev_attr_pwm1_max.dev_attr.attr,
> > > &sensor_dev_attr_fan1_input.dev_attr.attr,
> > > + &sensor_dev_attr_fan1_min.dev_attr.attr,
> > > + &sensor_dev_attr_fan1_max.dev_attr.attr,
> > > + &sensor_dev_attr_fan1_target.dev_attr.attr,
> > > + &sensor_dev_attr_fan1_enable.dev_attr.attr,
> > > &sensor_dev_attr_in0_input.dev_attr.attr,
> > > &sensor_dev_attr_in0_label.dev_attr.attr,
> > > &sensor_dev_attr_in1_input.dev_attr.attr,
> > > @@ -1456,13 +1624,16 @@ static umode_t
> > hwmon_attributes_visible(struct
> > > kobject *kobj,
> > > struct amdgpu_device *adev = dev_get_drvdata(dev);
> > > umode_t effective_mode = attr->mode;
> > >
> > > -
> > > /* Skip fan attributes if fan is not present */
> > > if (adev->pm.no_fan && (attr ==
> > > &sensor_dev_attr_pwm1.dev_attr.attr ||
> > > attr == &sensor_dev_attr_pwm1_enable.dev_attr.attr ||
> > > attr == &sensor_dev_attr_pwm1_max.dev_attr.attr ||
> > > attr == &sensor_dev_attr_pwm1_min.dev_attr.attr ||
> > > - attr == &sensor_dev_attr_fan1_input.dev_attr.attr))
> > > + attr == &sensor_dev_attr_fan1_input.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_min.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_max.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_target.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_enable.dev_attr.attr))
> > > return 0;
> > >
> > > /* Skip limit attributes if DPM is not enabled */ @@ -1472,7
> > > +1643,12 @@ static umode_t hwmon_attributes_visible(struct kobject
> > *kobj,
> > > attr == &sensor_dev_attr_pwm1.dev_attr.attr ||
> > > attr == &sensor_dev_attr_pwm1_enable.dev_attr.attr ||
> > > attr == &sensor_dev_attr_pwm1_max.dev_attr.attr ||
> > > - attr == &sensor_dev_attr_pwm1_min.dev_attr.attr))
> > > + attr == &sensor_dev_attr_pwm1_min.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_input.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_min.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_max.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_target.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_enable.dev_attr.attr))
> > > return 0;
> > >
> > > /* mask fan attributes if we have no bindings for this asic to
> > > expose */ @@ -1497,10 +1673,18 @@ static umode_t
> > > hwmon_attributes_visible(struct kobject *kobj,
> > > /* hide max/min values if we can't both query and manage the fan */
> > > if ((!adev->powerplay.pp_funcs->set_fan_speed_percent &&
> > > !adev->powerplay.pp_funcs->get_fan_speed_percent) &&
> > > + (!adev->powerplay.pp_funcs->set_fan_speed_rpm &&
> > > + !adev->powerplay.pp_funcs->get_fan_speed_rpm) &&
> > > (attr == &sensor_dev_attr_pwm1_max.dev_attr.attr ||
> > > attr == &sensor_dev_attr_pwm1_min.dev_attr.attr))
> > > return 0;
> > >
> > > + if ((!adev->powerplay.pp_funcs->set_fan_speed_rpm &&
> > > + !adev->powerplay.pp_funcs->get_fan_speed_rpm) &&
> > > + (attr == &sensor_dev_attr_fan1_max.dev_attr.attr ||
> > > + attr == &sensor_dev_attr_fan1_min.dev_attr.attr))
> > > + return 0;
> > > +
> > > /* only APUs have vddnb */
> > > if (!(adev->flags & AMD_IS_APU) &&
> > > (attr == &sensor_dev_attr_in1_input.dev_attr.attr || diff
> > > --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > index 97001a6..980e696 100644
> > > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > @@ -230,6 +230,7 @@ struct amd_pm_funcs {
> > > enum amd_dpm_forced_level (*get_performance_level)(void
> > *handle);
> > > enum amd_pm_state_type (*get_current_power_state)(void
> > *handle);
> > > int (*get_fan_speed_rpm)(void *handle, uint32_t *rpm);
> > > + int (*set_fan_speed_rpm)(void *handle, uint32_t rpm);
> > > int (*get_pp_num_states)(void *handle, struct pp_states_info
> *data);
> > > int (*get_pp_table)(void *handle, char **table);
> > > int (*set_pp_table)(void *handle, const char *buf, size_t size);
> > > diff -- git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > index 053c485..9d8fa2e 100644
> > > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> > > @@ -576,6 +576,24 @@ static int pp_dpm_get_fan_speed_rpm(void
> > *handle,
> > > uint32_t *rpm)
> > > return ret;
> > > }
> > >
> > > +static int pp_dpm_set_fan_speed_rpm(void *handle, uint32_t rpm) {
> > > + struct pp_hwmgr *hwmgr = handle;
> > > + int ret = 0;
> > > +
> > > + if (!hwmgr || !hwmgr->pm_en)
> > > + return -EINVAL;
> > > +
> > > + if (hwmgr->hwmgr_func->set_fan_speed_rpm == NULL) {
> > > + pr_info("%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);
> > > + return ret;
> > > +}
> > > +
> > > static int pp_dpm_get_pp_num_states(void *handle,
> > > struct pp_states_info *data)
> > > {
> > > @@ -1297,6 +1315,7 @@ static int pp_enable_mgpu_fan_boost(void
> > > *handle)
> > > .set_fan_speed_percent = pp_dpm_set_fan_speed_percent,
> > > .get_fan_speed_percent = pp_dpm_get_fan_speed_percent,
> > > .get_fan_speed_rpm = pp_dpm_get_fan_speed_rpm,
> > > + .set_fan_speed_rpm = pp_dpm_set_fan_speed_rpm,
> > > .get_pp_num_states = pp_dpm_get_pp_num_states,
> > > .get_pp_table = pp_dpm_get_pp_table,
> > > .set_pp_table = pp_dpm_set_pp_table,
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > amd-gfx mailing list
> > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
