[PATCH 1/5] drm/amdgpu: Refine uvd_v6/7_0_enc_get_destroy_msg

[PATCH 1/5] drm/amdgpu: Refine uvd_v6/7_0_enc_get_destroy_msg

[Rex Zhu]

1. make uvd_v7_0_enc_get_destroy_msg static
2. drop a function variable that always true

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 10 +++-------
 drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c | 12 ++++--------
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
index 8ef4a53..7a5b402 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
@@ -274,7 +274,7 @@ static int uvd_v6_0_enc_get_create_msg(struct amdgpu_ring *ring, uint32_t handle
  */
 static int uvd_v6_0_enc_get_destroy_msg(struct amdgpu_ring *ring,
 					uint32_t handle,
-					bool direct, struct dma_fence **fence)
+					struct dma_fence **fence)
 {
 	const unsigned ib_size_dw = 16;
 	struct amdgpu_job *job;
@@ -310,11 +310,7 @@ static int uvd_v6_0_enc_get_destroy_msg(struct amdgpu_ring *ring,
 	for (i = ib->length_dw; i < ib_size_dw; ++i)
 		ib->ptr[i] = 0x0;
 
-	if (direct)
-		r = amdgpu_job_submit_direct(job, ring, &f);
-	else
-		r = amdgpu_job_submit(job, &ring->adev->vce.entity,
-				      AMDGPU_FENCE_OWNER_UNDEFINED, &f);
+	r = amdgpu_job_submit_direct(job, ring, &f);
 	if (r)
 		goto err;
 
@@ -345,7 +341,7 @@ static int uvd_v6_0_enc_ring_test_ib(struct amdgpu_ring *ring, long timeout)
 		goto error;
 	}
 
-	r = uvd_v6_0_enc_get_destroy_msg(ring, 1, true, &fence);
+	r = uvd_v6_0_enc_get_destroy_msg(ring, 1, &fence);
 	if (r) {
 		DRM_ERROR("amdgpu: failed to get destroy ib (%ld).\n", r);
 		goto error;
diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
index a289f6a..58b39af 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
@@ -280,8 +280,8 @@ static int uvd_v7_0_enc_get_create_msg(struct amdgpu_ring *ring, uint32_t handle
  *
  * Close up a stream for HW test or if userspace failed to do so
  */
-int uvd_v7_0_enc_get_destroy_msg(struct amdgpu_ring *ring, uint32_t handle,
-				 bool direct, struct dma_fence **fence)
+static int uvd_v7_0_enc_get_destroy_msg(struct amdgpu_ring *ring, uint32_t handle,
+				struct dma_fence **fence)
 {
 	const unsigned ib_size_dw = 16;
 	struct amdgpu_job *job;
@@ -317,11 +317,7 @@ int uvd_v7_0_enc_get_destroy_msg(struct amdgpu_ring *ring, uint32_t handle,
 	for (i = ib->length_dw; i < ib_size_dw; ++i)
 		ib->ptr[i] = 0x0;
 
-	if (direct)
-		r = amdgpu_job_submit_direct(job, ring, &f);
-	else
-		r = amdgpu_job_submit(job, &ring->adev->vce.entity,
-				      AMDGPU_FENCE_OWNER_UNDEFINED, &f);
+	r = amdgpu_job_submit_direct(job, ring, &f);
 	if (r)
 		goto err;
 
@@ -352,7 +348,7 @@ static int uvd_v7_0_enc_ring_test_ib(struct amdgpu_ring *ring, long timeout)
 		goto error;
 	}
 
-	r = uvd_v7_0_enc_get_destroy_msg(ring, 1, true, &fence);
+	r = uvd_v7_0_enc_get_destroy_msg(ring, 1, &fence);
 	if (r) {
 		DRM_ERROR("amdgpu: (%d)failed to get destroy ib (%ld).\n", ring->me, r);
 		goto error;
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/5] drm/amdgpu: Add new AMDGPU_PP_SENSOR_MIN/MAX_FAN_RPM sensor

[Rex Zhu]

For getting the min/max fan speed in RPM units.

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/include/kgd_pp_interface.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
index 8593850..97001a6 100644
--- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
+++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
@@ -114,6 +114,8 @@ enum amd_pp_sensors {
 	AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK,
 	AMDGPU_PP_SENSOR_STABLE_PSTATE_MCLK,
 	AMDGPU_PP_SENSOR_ENABLED_SMC_FEATURES_MASK,
+	AMDGPU_PP_SENSOR_MIN_FAN_RPM,
+	AMDGPU_PP_SENSOR_MAX_FAN_RPM,
 };
 
 enum amd_pp_task {
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/5] drm/amd/pp: Implement AMDGPU_PP_SENSOR_MIN/MAX_FAN_RPM

[Rex Zhu]

so user can query the RPM range

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/powerplay/amd_powerplay.c                | 6 ++++++
 drivers/gpu/drm/amd/powerplay/hwmgr/vega20_processpptables.c | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
index 32f475e..053c485 100644
--- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
+++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
@@ -813,6 +813,12 @@ static int pp_dpm_read_sensor(void *handle, int idx,
 	case AMDGPU_PP_SENSOR_STABLE_PSTATE_MCLK:
 		*((uint32_t *)value) = hwmgr->pstate_mclk;
 		return 0;
+	case AMDGPU_PP_SENSOR_MIN_FAN_RPM:
+		*((uint32_t *)value) = hwmgr->thermal_controller.fanInfo.ulMinRPM;
+		return 0;
+	case AMDGPU_PP_SENSOR_MAX_FAN_RPM:
+		*((uint32_t *)value) = hwmgr->thermal_controller.fanInfo.ulMaxRPM;
+		return 0;
 	default:
 		mutex_lock(&hwmgr->smu_lock);
 		ret = hwmgr->hwmgr_func->read_sensor(hwmgr, idx, value, size);
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_processpptables.c b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_processpptables.c
index 5f1f7a3..c9b93e6 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_processpptables.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_processpptables.c
@@ -834,6 +834,8 @@ static int init_powerplay_table_information(
 
 	hwmgr->thermal_controller.ucType = powerplay_table->ucThermalControllerType;
 	pptable_information->uc_thermal_controller_type = powerplay_table->ucThermalControllerType;
+	hwmgr->thermal_controller.fanInfo.ulMinRPM = 0;
+	hwmgr->thermal_controller.fanInfo.ulMaxRPM = powerplay_table->smcPPTable.FanMaximumRpm;
 
 	set_hw_cap(hwmgr,
 		ATOM_VEGA20_PP_THERMALCONTROLLER_NONE != hwmgr->thermal_controller.ucType,
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Rex Zhu]

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@amd.com>
---
 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_handle, (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)
+ *
  * - 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Rex
> Zhu
> Sent: Sunday, September 30, 2018 2:18 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex <Rex.Zhu@amd.com>
> 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@amd.com>
> ---
>  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).

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Zhu, Rex]

> -----Original Message-----
> From: Deucher, Alexander
> Sent: Monday, October 1, 2018 9:16 PM
> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex <Rex.Zhu@amd.com>
> Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> 
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Rex
> > Zhu
> > Sent: Sunday, September 30, 2018 2:18 AM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > 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@amd.com>
> > ---
> >  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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Russell, Kent]

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?

 Kent

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhu, Rex
Sent: Monday, October 01, 2018 9:34 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
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@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex <Rex.Zhu@amd.com>
> Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> 
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of 
> > Rex Zhu
> > Sent: Sunday, September 30, 2018 2:18 AM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > 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@amd.com>
> > ---
> >  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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Zhu, Rex]

In order to follow the standard sysfs interfaces for hwmon:

fan[1-*]_enable
		Enable or disable the sensors.
		When disabled the sensor read will return -ENODATA.
		1: Enable
		0: Disable
		RW

When enable, we set the fan control mode to Manual.
Disable, we set the fan control mode to Auto.

Regards
Rex

> -----Original Message-----
> From: Russell, Kent
> Sent: Friday, October 5, 2018 10:40 PM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
> 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?
> 
>  Kent
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhu,
> Rex
> Sent: Monday, October 01, 2018 9:34 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-
> gfx@lists.freedesktop.org
> 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@amd.com>; amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> >
> > > -----Original Message-----
> > > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > > Rex Zhu
> > > Sent: Sunday, September 30, 2018 2:18 AM
> > > To: amd-gfx@lists.freedesktop.org
> > > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > > 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@amd.com>
> > > ---
> > >  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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Russell, Kent]

Thanks, I understand. This threw off one of our automated tests that suddenly couldn't read the value of the RPMs because the fan control wasn't set to manual at the time.  
 
 Kent 

-----Original Message-----
From: Zhu, Rex 
Sent: Friday, October 05, 2018 11:53 AM
To: Russell, Kent <Kent.Russell@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

In order to follow the standard sysfs interfaces for hwmon:

fan[1-*]_enable
		Enable or disable the sensors.
		When disabled the sensor read will return -ENODATA.
		1: Enable
		0: Disable
		RW

When enable, we set the fan control mode to Manual.
Disable, we set the fan control mode to Auto.

Regards
Rex

> -----Original Message-----
> From: Russell, Kent
> Sent: Friday, October 5, 2018 10:40 PM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander 
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
> 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?
> 
>  Kent
> 
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of 
> Zhu, Rex
> Sent: Monday, October 01, 2018 9:34 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>; amd- 
> gfx@lists.freedesktop.org
> 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@amd.com>; amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> >
> > > -----Original Message-----
> > > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of 
> > > Rex Zhu
> > > Sent: Sunday, September 30, 2018 2:18 AM
> > > To: amd-gfx@lists.freedesktop.org
> > > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > > 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@amd.com>
> > > ---
> > >  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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Zhu, Rex]

I may misunderstood. 
If for read, we should not  change the mode to manual.
I will revert the change for fan1_input.

Best Regards
Rex

> -----Original Message-----
> From: Russell, Kent
> Sent: Friday, October 5, 2018 11:55 PM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> 
> Thanks, I understand. This threw off one of our automated tests that
> suddenly couldn't read the value of the RPMs because the fan control wasn't
> set to manual at the time.
> 
>  Kent
> 
> -----Original Message-----
> From: Zhu, Rex
> Sent: Friday, October 05, 2018 11:53 AM
> To: Russell, Kent <Kent.Russell@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> 
> In order to follow the standard sysfs interfaces for hwmon:
> 
> fan[1-*]_enable
> 		Enable or disable the sensors.
> 		When disabled the sensor read will return -ENODATA.
> 		1: Enable
> 		0: Disable
> 		RW
> 
> When enable, we set the fan control mode to Manual.
> Disable, we set the fan control mode to Auto.
> 
> Regards
> Rex
> 
> > -----Original Message-----
> > From: Russell, Kent
> > Sent: Friday, October 5, 2018 10:40 PM
> > To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
> > 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?
> >
> >  Kent
> >
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Zhu, Rex
> > Sent: Monday, October 01, 2018 9:34 AM
> > To: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-
> > gfx@lists.freedesktop.org
> > 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@amd.com>; amd-gfx@lists.freedesktop.org
> > > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > > Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> > >
> > > > -----Original Message-----
> > > > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > > > Rex Zhu
> > > > Sent: Sunday, September 30, 2018 2:18 AM
> > > > To: amd-gfx@lists.freedesktop.org
> > > > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > > > 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@amd.com>
> > > > ---
> > > >  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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs

[Deucher, Alexander]

> -----Original Message-----
> From: Russell, Kent
> Sent: Friday, October 5, 2018 10:40 AM
> To: Zhu, Rex <Rex.Zhu@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org
> 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@lists.freedesktop.org> On Behalf Of Zhu,
> Rex
> Sent: Monday, October 01, 2018 9:34 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-
> gfx@lists.freedesktop.org
> 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@amd.com>; amd-gfx@lists.freedesktop.org
> > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > Subject: RE: [PATCH 4/5] drm/amdgpu: Add fan RPM setting via sysfs
> >
> > > -----Original Message-----
> > > From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > > Rex Zhu
> > > Sent: Sunday, September 30, 2018 2:18 AM
> > > To: amd-gfx@lists.freedesktop.org
> > > Cc: Zhu, Rex <Rex.Zhu@amd.com>
> > > 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@amd.com>
> > > ---
> > >  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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/5] drm/amdgpu: Disable sysfs pwm1 if not in manual fan control

[Rex Zhu]

Following lm-sensors 3.0.0,
Only enable pwm1 sysfs when fan control mode(pwm1_enable)
in manual

Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 1d85706..d6bd5ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1120,12 +1120,19 @@ static ssize_t amdgpu_hwmon_set_pwm1(struct device *dev,
 	struct amdgpu_device *adev = dev_get_drvdata(dev);
 	int err;
 	u32 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;
 
+	pwm_mode = amdgpu_dpm_get_fan_control_mode(adev);
+	if (pwm_mode != AMD_FAN_CTRL_MANUAL) {
+		pr_info("manual fan speed control should be enabled first\n");
+		return -EINVAL;
+	}
+
 	err = kstrtou32(buf, 10, &value);
 	if (err)
 		return err;
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 5/5] drm/amdgpu: Disable sysfs pwm1 if not in manual fan control

[Quan, Evan]

Series is Reviewed-by: Evan Quan <evan.quan@amd.com>

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Rex
> Zhu
> Sent: 2018年9月30日 14:18
> To: amd-gfx@lists.freedesktop.org
> Cc: Zhu, Rex <Rex.Zhu@amd.com>
> Subject: [PATCH 5/5] drm/amdgpu: Disable sysfs pwm1 if not in manual fan
> control
> 
> Following lm-sensors 3.0.0,
> Only enable pwm1 sysfs when fan control mode(pwm1_enable) in manual
> 
> Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 1d85706..d6bd5ab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1120,12 +1120,19 @@ static ssize_t amdgpu_hwmon_set_pwm1(struct
> device *dev,
>  	struct amdgpu_device *adev = dev_get_drvdata(dev);
>  	int err;
>  	u32 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;
> 
> +	pwm_mode = amdgpu_dpm_get_fan_control_mode(adev);
> +	if (pwm_mode != AMD_FAN_CTRL_MANUAL) {
> +		pr_info("manual fan speed control should be enabled
> first\n");
> +		return -EINVAL;
> +	}
> +
>  	err = kstrtou32(buf, 10, &value);
>  	if (err)
>  		return err;
> --
> 1.9.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx