[RFC PATCH 00/29] Current State of my clk patches

[RFC PATCH 00/29] Current State of my clk patches

[Karol Herbst]

Just wanted to post updated versions of my last series/patches. Reviews
welcomed.

It would be also nice if we agree on features I should focus upstreaming, so
that this work can be better splitted or reordered.

Sadly most of my patches depend on the rather big clk subdev rework and I think
those patches shows best, why I think this rework is actually needed and makes
things much easier to add later on.

Feature overview:
  1- 3: Hwmon fixes on suspended device
  4- 8: Clk subdev rework to decouple current and expected clock states
        Fix reclocking while suspended
     9: Restore clocks on Suspend
    10: Fix reclocking while going into suspend
    11: Fix reclocking while suspending
 12-14: Thermal daemon triggers reclocks on temperature change when needed
          includes volt updates and dropping cstates according to the vbios
 15-17: Thermal throttling
    18: Don't do full reclocks when only parts need to be updated
 19-20: Hacky Workaround for enabling Maxwell2 reclocking (hidden behind module parameter)
 21-23: debugfs file to change boost mode
 24-29: Parse battery vpstate to throttle clocks when system is running on battery

Karol Herbst (29):
  therm: split return code and value in nvkm_get_temp
  hwmon: properly check for errors
  subdev/volt/gk104: return error when read fails
  clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it
  clk: Remove dstate
  clk: Make pstate a pointer to nvkm_pstate
  clk: Hold information about the current cstate status
  clk: We should pass the pstate id around not the index in the list
  clk: Set clocks to pre suspend state after suspend
  core/device: Move therm behind clk
  debugfs: Wake up GPU before doing any reclocking
  therm: Don't cancel the timer
  therm: Move the temp readout into the alarm
  therm: Trigger reclock in temperature daemon
  bios: add thermal policies table
  clk: parse thermal policies for throttling thresholds
  clk: thermal throttling
  clk: Only do partial reclocks as required
  secboot/acr352: reset PMU after secboot
  device: enable clk for Maxwell2
  clk: Save the max clock we can set
  nvif: Add boost info and set operations
  debugfs: Add boost interface to change the boost_mode
  bios/vpstate: there are some fermi vbios with no boost or tdp entry
  bios/vpstate: parse max battery id
  clk: refactor the base and boost clock limits so that we can limit
    pstates as well
  clk: implement limiting pstates just like we do for cstates
  clk: move the switch out of the loop in nvkm_cstate_valid
  clk: limit clocks on battery

 drm/nouveau/include/nvif/if0001.h                  |  15 +
 drm/nouveau/include/nvkm/core/device.h             |   2 +-
 .../include/nvkm/subdev/bios/thermal_policies.h    |  27 ++
 drm/nouveau/include/nvkm/subdev/bios/vpstate.h     |   1 +
 drm/nouveau/include/nvkm/subdev/clk.h              |  23 +-
 drm/nouveau/include/nvkm/subdev/therm.h            |   3 +-
 drm/nouveau/nouveau_debugfs.c                      |  90 ++++-
 drm/nouveau/nouveau_hwmon.c                        |  48 ++-
 drm/nouveau/nvkm/engine/device/base.c              |   3 +
 drm/nouveau/nvkm/engine/device/ctrl.c              |  66 +++-
 drm/nouveau/nvkm/subdev/bios/Kbuild                |   1 +
 drm/nouveau/nvkm/subdev/bios/thermal_policies.c    |  81 +++++
 drm/nouveau/nvkm/subdev/bios/vpstate.c             |  13 +-
 drm/nouveau/nvkm/subdev/clk/base.c                 | 375 +++++++++++++++------
 drm/nouveau/nvkm/subdev/clk/gk104.c                |  10 +-
 drm/nouveau/nvkm/subdev/pmu/gk20a.c                |  18 +-
 drm/nouveau/nvkm/subdev/secboot/acr_r352.c         |  13 +
 drm/nouveau/nvkm/subdev/therm/base.c               |  59 ++--
 drm/nouveau/nvkm/subdev/therm/g84.c                |  11 +-
 drm/nouveau/nvkm/subdev/therm/gp100.c              |   9 +-
 drm/nouveau/nvkm/subdev/therm/nv40.c               |   9 +-
 drm/nouveau/nvkm/subdev/therm/nv50.c               |   9 +-
 drm/nouveau/nvkm/subdev/therm/priv.h               |   4 +-
 drm/nouveau/nvkm/subdev/therm/temp.c               |  16 +-
 drm/nouveau/nvkm/subdev/volt/base.c                |   3 +
 drm/nouveau/nvkm/subdev/volt/gk104.c               |   7 +-
 26 files changed, 723 insertions(+), 193 deletions(-)
 create mode 100644 drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
 create mode 100644 drm/nouveau/nvkm/subdev/bios/thermal_policies.c

-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 01/29] therm: split return code and value in nvkm_get_temp

[Karol Herbst]

The current hwmon code doesn't check if the returned value was actually an
error.

Since Kepler temperature sensors are able to report negative values.
Since Pascal (and maybe earlier) we have sensors with improved precision.

Adjust the nvkm_get_temp method to be able to deal with those changes
and let hwmon return an error properly.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/therm.h |  2 +-
 drm/nouveau/nouveau_hwmon.c             | 15 +++++++++------
 drm/nouveau/nvkm/subdev/therm/base.c    | 19 ++++++++++++++-----
 drm/nouveau/nvkm/subdev/therm/g84.c     | 11 ++++++-----
 drm/nouveau/nvkm/subdev/therm/gp100.c   |  9 +++++----
 drm/nouveau/nvkm/subdev/therm/nv40.c    |  9 +++------
 drm/nouveau/nvkm/subdev/therm/nv50.c    |  9 +++------
 drm/nouveau/nvkm/subdev/therm/priv.h    |  4 ++--
 drm/nouveau/nvkm/subdev/therm/temp.c    | 16 ++++++++++++----
 9 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/therm.h b/drm/nouveau/include/nvkm/subdev/therm.h
index 9841f076..8c84017f 100644
--- a/drm/nouveau/include/nvkm/subdev/therm.h
+++ b/drm/nouveau/include/nvkm/subdev/therm.h
@@ -86,7 +86,7 @@ struct nvkm_therm {
 	int (*attr_set)(struct nvkm_therm *, enum nvkm_therm_attr_type, int);
 };
 
-int nvkm_therm_temp_get(struct nvkm_therm *);
+int nvkm_therm_temp_get(struct nvkm_therm *, int *);
 int nvkm_therm_fan_sense(struct nvkm_therm *);
 int nvkm_therm_cstate(struct nvkm_therm *, int, int);
 
diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
index 7c965648..f1914d9a 100644
--- a/drm/nouveau/nouveau_hwmon.c
+++ b/drm/nouveau/nouveau_hwmon.c
@@ -326,8 +326,9 @@ nouveau_temp_is_visible(const void *data, u32 attr, int channel)
 {
 	struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
+	int val;
 
-	if (therm && therm->attr_get && nvkm_therm_temp_get(therm) < 0)
+	if (therm && therm->attr_get && nvkm_therm_temp_get(therm, &val) < 0)
 		return 0;
 
 	switch (attr) {
@@ -421,15 +422,16 @@ nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
-	int ret;
+	int ret = 0;
+	int temp;
 
 	if (!therm || !therm->attr_get)
 		return -EOPNOTSUPP;
 
 	switch (attr) {
 	case hwmon_temp_input:
-		ret = nvkm_therm_temp_get(therm);
-		*val = ret < 0 ? ret : (ret * 1000);
+		ret = nvkm_therm_temp_get(therm, &temp);
+		*val = temp * 1000;
 		break;
 	case hwmon_temp_max:
 		*val = therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK)
@@ -459,7 +461,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)
 		return -EOPNOTSUPP;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int
@@ -713,6 +715,7 @@ nouveau_hwmon_init(struct drm_device *dev)
 	struct device *hwmon_dev;
 	int ret = 0;
 	int i = 0;
+	int val;
 
 	hwmon = drm->hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
 	if (!hwmon)
@@ -720,7 +723,7 @@ nouveau_hwmon_init(struct drm_device *dev)
 	hwmon->dev = dev;
 
 	if (therm && therm->attr_get && therm->attr_set) {
-		if (nvkm_therm_temp_get(therm) >= 0)
+		if (nvkm_therm_temp_get(therm, &val) >= 0)
 			special_groups[i++] = &temp1_auto_point_sensor_group;
 		if (therm->fan_get && therm->fan_get(therm) >= 0)
 			special_groups[i++] = &pwm_fan_sensor_group;
diff --git a/drm/nouveau/nvkm/subdev/therm/base.c b/drm/nouveau/nvkm/subdev/therm/base.c
index f27fc6d0..8fa691fb 100644
--- a/drm/nouveau/nvkm/subdev/therm/base.c
+++ b/drm/nouveau/nvkm/subdev/therm/base.c
@@ -24,22 +24,26 @@
 #include "priv.h"
 
 int
-nvkm_therm_temp_get(struct nvkm_therm *therm)
+nvkm_therm_temp_get(struct nvkm_therm *therm, int *val)
 {
 	if (therm->func->temp_get)
-		return therm->func->temp_get(therm);
+		return therm->func->temp_get(therm, val);
 	return -ENODEV;
 }
 
 static int
 nvkm_therm_update_trip(struct nvkm_therm *therm)
 {
+	int temp, ret;
 	struct nvbios_therm_trip_point *trip = therm->fan->bios.trip,
 				       *cur_trip = NULL,
 				       *last_trip = therm->last_trip;
-	u8  temp = therm->func->temp_get(therm);
 	u16 duty, i;
 
+	ret = therm->func->temp_get(therm, &temp);
+	if (ret < 0)
+		return ret;
+
 	/* look for the trip point corresponding to the current temperature */
 	cur_trip = NULL;
 	for (i = 0; i < therm->fan->bios.nr_fan_trip; i++) {
@@ -67,9 +71,13 @@ static int
 nvkm_therm_compute_linear_duty(struct nvkm_therm *therm, u8 linear_min_temp,
                                u8 linear_max_temp)
 {
-	u8  temp = therm->func->temp_get(therm);
+	int temp, ret;
 	u16 duty;
 
+	ret = therm->func->temp_get(therm, &temp);
+	if (ret < 0)
+		return ret;
+
 	/* handle the non-linear part first */
 	if (temp < linear_min_temp)
 		return therm->fan->bios.min_duty;
@@ -182,6 +190,7 @@ nvkm_therm_fan_mode(struct nvkm_therm *therm, int mode)
 {
 	struct nvkm_subdev *subdev = &therm->subdev;
 	struct nvkm_device *device = subdev->device;
+	int val;
 	static const char *name[] = {
 		"disabled",
 		"manual",
@@ -197,7 +206,7 @@ nvkm_therm_fan_mode(struct nvkm_therm *therm, int mode)
 	/* do not allow automatic fan management if the thermal sensor is
 	 * not available */
 	if (mode == NVKM_THERM_CTRL_AUTO &&
-	    therm->func->temp_get(therm) < 0)
+	    therm->func->temp_get(therm, &val) < 0)
 		return -EINVAL;
 
 	if (therm->mode == mode)
diff --git a/drm/nouveau/nvkm/subdev/therm/g84.c b/drm/nouveau/nvkm/subdev/therm/g84.c
index 96f8da40..3bb2d829 100644
--- a/drm/nouveau/nvkm/subdev/therm/g84.c
+++ b/drm/nouveau/nvkm/subdev/therm/g84.c
@@ -27,14 +27,15 @@
 #include <subdev/fuse.h>
 
 int
-g84_temp_get(struct nvkm_therm *therm)
+g84_temp_get(struct nvkm_therm *therm, int *val)
 {
 	struct nvkm_device *device = therm->subdev.device;
 
-	if (nvkm_fuse_read(device->fuse, 0x1a8) == 1)
-		return nvkm_rd32(device, 0x20400);
-	else
+	if (nvkm_fuse_read(device->fuse, 0x1a8) != 1)
 		return -ENODEV;
+
+	*val = nvkm_rd32(device, 0x20400);
+	return 0;
 }
 
 void
@@ -115,7 +116,7 @@ g84_therm_threshold_hyst_emulation(struct nvkm_therm *therm,
 	}
 
 	/* fix the state (in case someone reprogrammed the alarms) */
-	cur = therm->func->temp_get(therm);
+	therm->func->temp_get(therm, &cur);
 	if (new_state == NVKM_THERM_THRS_LOWER && cur > thrs->temp)
 		new_state = NVKM_THERM_THRS_HIGHER;
 	else if (new_state == NVKM_THERM_THRS_HIGHER &&
diff --git a/drm/nouveau/nvkm/subdev/therm/gp100.c b/drm/nouveau/nvkm/subdev/therm/gp100.c
index 9f0dea3f..d8206748 100644
--- a/drm/nouveau/nvkm/subdev/therm/gp100.c
+++ b/drm/nouveau/nvkm/subdev/therm/gp100.c
@@ -24,7 +24,7 @@
 #include "priv.h"
 
 static int
-gp100_temp_get(struct nvkm_therm *therm)
+gp100_temp_get(struct nvkm_therm *therm, int *val)
 {
 	struct nvkm_device *device = therm->subdev.device;
 	struct nvkm_subdev *subdev = &therm->subdev;
@@ -36,9 +36,10 @@ gp100_temp_get(struct nvkm_therm *therm)
 		nvkm_trace(subdev, "reading temperature from SHADOWed sensor\n");
 
 	/* device valid */
-	if (tsensor & 0x20000000)
-		return (inttemp >> 8);
-	else
+	if (tsensor & 0x20000000) {
+		*val = inttemp >> 8;
+		return 0;
+	} else
 		return -ENODEV;
 }
 
diff --git a/drm/nouveau/nvkm/subdev/therm/nv40.c b/drm/nouveau/nvkm/subdev/therm/nv40.c
index 2c92ffb5..cfd5b215 100644
--- a/drm/nouveau/nvkm/subdev/therm/nv40.c
+++ b/drm/nouveau/nvkm/subdev/therm/nv40.c
@@ -70,7 +70,7 @@ nv40_sensor_setup(struct nvkm_therm *therm)
 }
 
 static int
-nv40_temp_get(struct nvkm_therm *therm)
+nv40_temp_get(struct nvkm_therm *therm, int *val)
 {
 	struct nvkm_device *device = therm->subdev.device;
 	struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
@@ -95,11 +95,8 @@ nv40_temp_get(struct nvkm_therm *therm)
 	core_temp = core_temp + sensor->offset_num / sensor->offset_den;
 	core_temp = core_temp + sensor->offset_constant - 8;
 
-	/* reserve negative temperatures for errors */
-	if (core_temp < 0)
-		core_temp = 0;
-
-	return core_temp;
+	*val = core_temp;
+	return 0;
 }
 
 static int
diff --git a/drm/nouveau/nvkm/subdev/therm/nv50.c b/drm/nouveau/nvkm/subdev/therm/nv50.c
index 9b57b433..62ec4063 100644
--- a/drm/nouveau/nvkm/subdev/therm/nv50.c
+++ b/drm/nouveau/nvkm/subdev/therm/nv50.c
@@ -126,7 +126,7 @@ nv50_sensor_setup(struct nvkm_therm *therm)
 }
 
 static int
-nv50_temp_get(struct nvkm_therm *therm)
+nv50_temp_get(struct nvkm_therm *therm, int *val)
 {
 	struct nvkm_device *device = therm->subdev.device;
 	struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
@@ -143,11 +143,8 @@ nv50_temp_get(struct nvkm_therm *therm)
 	core_temp = core_temp + sensor->offset_num / sensor->offset_den;
 	core_temp = core_temp + sensor->offset_constant - 8;
 
-	/* reserve negative temperatures for errors */
-	if (core_temp < 0)
-		core_temp = 0;
-
-	return core_temp;
+	*val = core_temp;
+	return 0;
 }
 
 static void
diff --git a/drm/nouveau/nvkm/subdev/therm/priv.h b/drm/nouveau/nvkm/subdev/therm/priv.h
index 1f46e371..b325ec5f 100644
--- a/drm/nouveau/nvkm/subdev/therm/priv.h
+++ b/drm/nouveau/nvkm/subdev/therm/priv.h
@@ -91,7 +91,7 @@ struct nvkm_therm_func {
 	int (*pwm_set)(struct nvkm_therm *, int line, u32, u32);
 	int (*pwm_clock)(struct nvkm_therm *, int line);
 
-	int (*temp_get)(struct nvkm_therm *);
+	int (*temp_get)(struct nvkm_therm *, int *);
 
 	int (*fan_sense)(struct nvkm_therm *);
 
@@ -105,7 +105,7 @@ int  nv50_fan_pwm_get(struct nvkm_therm *, int, u32 *, u32 *);
 int  nv50_fan_pwm_set(struct nvkm_therm *, int, u32, u32);
 int  nv50_fan_pwm_clock(struct nvkm_therm *, int);
 
-int  g84_temp_get(struct nvkm_therm *);
+int  g84_temp_get(struct nvkm_therm *, int *);
 void g84_sensor_setup(struct nvkm_therm *);
 void g84_therm_fini(struct nvkm_therm *);
 
diff --git a/drm/nouveau/nvkm/subdev/therm/temp.c b/drm/nouveau/nvkm/subdev/therm/temp.c
index ddb2b2c6..5ec2dfb3 100644
--- a/drm/nouveau/nvkm/subdev/therm/temp.c
+++ b/drm/nouveau/nvkm/subdev/therm/temp.c
@@ -86,7 +86,10 @@ nvkm_therm_sensor_event(struct nvkm_therm *therm, enum nvkm_therm_thrs thrs,
 	static const char * const thresholds[] = {
 		"fanboost", "downclock", "critical", "shutdown"
 	};
-	int temperature = therm->func->temp_get(therm);
+	int temperature;
+
+	if (therm->func->temp_get(therm, &temperature) < 0)
+		return;
 
 	if (thrs < 0 || thrs > 3)
 		return;
@@ -140,7 +143,10 @@ nvkm_therm_threshold_hyst_polling(struct nvkm_therm *therm,
 {
 	enum nvkm_therm_thrs_direction direction;
 	enum nvkm_therm_thrs_state prev_state, new_state;
-	int temp = therm->func->temp_get(therm);
+	int temp;
+
+	if (therm->func->temp_get(therm, &temp) < 0)
+		return;
 
 	prev_state = nvkm_therm_sensor_get_threshold_state(therm, thrs_name);
 
@@ -166,6 +172,7 @@ alarm_timer_callback(struct nvkm_alarm *alarm)
 	struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
 	struct nvkm_timer *tmr = therm->subdev.device->timer;
 	unsigned long flags;
+	int val;
 
 	spin_lock_irqsave(&therm->sensor.alarm_program_lock, flags);
 
@@ -185,7 +192,7 @@ alarm_timer_callback(struct nvkm_alarm *alarm)
 	spin_unlock_irqrestore(&therm->sensor.alarm_program_lock, flags);
 
 	/* schedule the next poll in one second */
-	if (therm->func->temp_get(therm) >= 0)
+	if (therm->func->temp_get(therm, &val) >= 0)
 		nvkm_timer_alarm(tmr, 1000000000ULL, alarm);
 }
 
@@ -227,9 +234,10 @@ nvkm_therm_sensor_fini(struct nvkm_therm *therm, bool suspend)
 void
 nvkm_therm_sensor_preinit(struct nvkm_therm *therm)
 {
+	int val;
 	const char *sensor_avail = "yes";
 
-	if (therm->func->temp_get(therm) < 0)
+	if (therm->func->temp_get(therm, &val) < 0)
 		sensor_avail = "no";
 
 	nvkm_debug(&therm->subdev, "internal sensor: %s\n", sensor_avail);
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 02/29] hwmon: properly check for errors

[Karol Herbst]

Otherwise hwmon interprets error codes as real values.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nouveau_hwmon.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
index f1914d9a..0d75c14d 100644
--- a/drm/nouveau/nouveau_hwmon.c
+++ b/drm/nouveau/nouveau_hwmon.c
@@ -470,18 +470,23 @@ nouveau_fan_read(struct device *dev, u32 attr, int channel, long *val)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
+	int ret;
 
 	if (!therm)
 		return -EOPNOTSUPP;
 
 	switch (attr) {
 	case hwmon_fan_input:
-		*val = nvkm_therm_fan_sense(therm);
+		ret = nvkm_therm_fan_sense(therm);
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
 	return 0;
 }
 
@@ -491,7 +496,7 @@ nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
-	int ret;
+	int ret = 0;
 
 	if (!volt)
 		return -EOPNOTSUPP;
@@ -499,7 +504,8 @@ nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
 	switch (attr) {
 	case hwmon_in_input:
 		ret = nvkm_volt_get(volt);
-		*val = ret < 0 ? ret : (ret / 1000);
+		if (ret >= 0)
+			*val = ret / 1000;
 		break;
 	case hwmon_in_min:
 		*val = volt->min_uv > 0 ? (volt->min_uv / 1000) : -ENODEV;
@@ -511,7 +517,7 @@ nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
 		return -EOPNOTSUPP;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int
@@ -520,21 +526,26 @@ nouveau_pwm_read(struct device *dev, u32 attr, int channel, long *val)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
+	int ret;
 
 	if (!therm || !therm->attr_get || !therm->fan_get)
 		return -EOPNOTSUPP;
 
 	switch (attr) {
 	case hwmon_pwm_enable:
-		*val = therm->attr_get(therm, NVKM_THERM_ATTR_FAN_MODE);
+		ret = therm->attr_get(therm, NVKM_THERM_ATTR_FAN_MODE);
 		break;
 	case hwmon_pwm_input:
-		*val = therm->fan_get(therm);
+		ret = therm->fan_get(therm);
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
 	return 0;
 }
 
@@ -544,18 +555,26 @@ nouveau_power_read(struct device *dev, u32 attr, int channel, long *val)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct nouveau_drm *drm = nouveau_drm(drm_dev);
 	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
+	int ret;
 
 	if (!iccsense)
 		return -EOPNOTSUPP;
 
 	switch (attr) {
 	case hwmon_power_input:
-		*val = nvkm_iccsense_read_all(iccsense);
+		ret = nvkm_iccsense_read_all(iccsense);
+		if (ret < 0)
+			return ret;
+		*val = ret;
 		break;
 	case hwmon_power_max:
+		if (iccsense->power_w_max <= 0)
+			return -ENODEV;
 		*val = iccsense->power_w_max;
 		break;
 	case hwmon_power_crit:
+		if (iccsense->power_w_crit <= 0)
+			return -ENODEV;
 		*val = iccsense->power_w_crit;
 		break;
 	default:
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 03/29] subdev/volt/gk104: return error when read fails

[Karol Herbst]

While my gpu was powered off, hwmon returned 0.6V as the current voltage.
If nvkm_rd32 fails for any reason, return the error.

With that sensors will display a "N/A" instead of 0.6V.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nvkm/subdev/volt/gk104.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drm/nouveau/nvkm/subdev/volt/gk104.c b/drm/nouveau/nvkm/subdev/volt/gk104.c
index 1c744e02..53a7af9d 100644
--- a/drm/nouveau/nvkm/subdev/volt/gk104.c
+++ b/drm/nouveau/nvkm/subdev/volt/gk104.c
@@ -40,10 +40,15 @@ gk104_volt_get(struct nvkm_volt *base)
 {
 	struct nvbios_volt *bios = &gk104_volt(base)->bios;
 	struct nvkm_device *device = base->subdev.device;
-	u32 div, duty;
+	int div, duty;
 
 	div  = nvkm_rd32(device, 0x20340);
+	if (div < 0)
+		return div;
+
 	duty = nvkm_rd32(device, 0x20344);
+	if (duty < 0)
+		return duty;
 
 	return bios->base + bios->pwm_range * duty / div;
 }
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it

[Karol Herbst]

This function will be used to update the current clock state.

This will happen for various reasons:
  * Temperature changes
  * User changes clocking state
  * Load changes

v2: remove parameter name

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  1 +
 drm/nouveau/nvkm/subdev/clk/base.c    | 26 ++++++++++++++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index e5275f74..ce3bbcfe 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -123,6 +123,7 @@ int nvkm_clk_ustate(struct nvkm_clk *, int req, int pwr);
 int nvkm_clk_astate(struct nvkm_clk *, int req, int rel, bool wait);
 int nvkm_clk_dstate(struct nvkm_clk *, int req, int rel);
 int nvkm_clk_tstate(struct nvkm_clk *, u8 temperature);
+int nvkm_clk_update(struct nvkm_clk *, bool wait);
 
 int nv04_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
 int nv40_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index e4c8d310..ecff3ff3 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -296,7 +296,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
 }
 
 static void
-nvkm_pstate_work(struct work_struct *work)
+nvkm_clk_update_work(struct work_struct *work)
 {
 	struct nvkm_clk *clk = container_of(work, typeof(*clk), work);
 	struct nvkm_subdev *subdev = &clk->subdev;
@@ -332,9 +332,15 @@ nvkm_pstate_work(struct work_struct *work)
 	nvkm_notify_get(&clk->pwrsrc_ntfy);
 }
 
-static int
-nvkm_pstate_calc(struct nvkm_clk *clk, bool wait)
+int
+nvkm_clk_update(struct nvkm_clk *clk, bool wait)
 {
+	if (!clk)
+		return -EINVAL;
+
+	if (!clk->allow_reclock)
+		return -ENODEV;
+
 	atomic_set(&clk->waiting, 1);
 	schedule_work(&clk->work);
 	if (wait)
@@ -524,7 +530,7 @@ nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
 	if (ret >= 0) {
 		if (ret -= 2, pwr) clk->ustate_ac = ret;
 		else		   clk->ustate_dc = ret;
-		return nvkm_pstate_calc(clk, true);
+		return nvkm_clk_update(clk, true);
 	}
 	return ret;
 }
@@ -536,7 +542,7 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
 	if ( rel) clk->astate += rel;
 	clk->astate = min(clk->astate, clk->state_nr - 1);
 	clk->astate = max(clk->astate, 0);
-	return nvkm_pstate_calc(clk, wait);
+	return nvkm_clk_update(clk, wait);
 }
 
 int
@@ -545,7 +551,7 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
 	if (clk->temp == temp)
 		return 0;
 	clk->temp = temp;
-	return nvkm_pstate_calc(clk, false);
+	return nvkm_clk_update(clk, false);
 }
 
 int
@@ -555,7 +561,7 @@ nvkm_clk_dstate(struct nvkm_clk *clk, int req, int rel)
 	if ( rel) clk->dstate += rel;
 	clk->dstate = min(clk->dstate, clk->state_nr - 1);
 	clk->dstate = max(clk->dstate, 0);
-	return nvkm_pstate_calc(clk, true);
+	return nvkm_clk_update(clk, true);
 }
 
 static int
@@ -563,7 +569,7 @@ nvkm_clk_pwrsrc(struct nvkm_notify *notify)
 {
 	struct nvkm_clk *clk =
 		container_of(notify, typeof(*clk), pwrsrc_ntfy);
-	nvkm_pstate_calc(clk, false);
+	nvkm_clk_update(clk, false);
 	return NVKM_NOTIFY_DROP;
 }
 
@@ -618,7 +624,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
 	clk->dstate = 0;
 	clk->pstate = -1;
 	clk->temp = 90; /* reasonable default value */
-	nvkm_pstate_calc(clk, true);
+	nvkm_clk_update(clk, true);
 	return 0;
 }
 
@@ -675,7 +681,7 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 	clk->ustate_dc = -1;
 	clk->allow_reclock = allow_reclock;
 
-	INIT_WORK(&clk->work, nvkm_pstate_work);
+	INIT_WORK(&clk->work, nvkm_clk_update_work);
 	init_waitqueue_head(&clk->wait);
 	atomic_set(&clk->waiting, 0);
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 05/29] clk: Remove dstate

[Karol Herbst]

We won't need it now, because we will adjust the clocks depending on engine
loads later on anyway or a static lookup table. It also simplifies the
clocking logic.

This code was nowhere used anyway and just a mock up.

v2: fixed typo in commit message

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  2 --
 drm/nouveau/nvkm/subdev/clk/base.c    | 16 ++--------------
 2 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index ce3bbcfe..1340f5b8 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -99,7 +99,6 @@ struct nvkm_clk {
 	int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
 	int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
 	int astate; /* perfmon adjustment (base) */
-	int dstate; /* display adjustment (min+) */
 	u8  temp;
 
 	bool allow_reclock;
@@ -121,7 +120,6 @@ struct nvkm_clk {
 int nvkm_clk_read(struct nvkm_clk *, enum nv_clk_src);
 int nvkm_clk_ustate(struct nvkm_clk *, int req, int pwr);
 int nvkm_clk_astate(struct nvkm_clk *, int req, int rel, bool wait);
-int nvkm_clk_dstate(struct nvkm_clk *, int req, int rel);
 int nvkm_clk_tstate(struct nvkm_clk *, u8 temperature);
 int nvkm_clk_update(struct nvkm_clk *, bool wait);
 
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index ecff3ff3..07d530ed 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -306,15 +306,14 @@ nvkm_clk_update_work(struct work_struct *work)
 		return;
 	clk->pwrsrc = power_supply_is_system_supplied();
 
-	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C D %d\n",
+	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
 		   clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
-		   clk->astate, clk->temp, clk->dstate);
+		   clk->astate, clk->temp);
 
 	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
 	if (clk->state_nr && pstate != -1) {
 		pstate = (pstate < 0) ? clk->astate : pstate;
 		pstate = min(pstate, clk->state_nr - 1);
-		pstate = max(pstate, clk->dstate);
 	} else {
 		pstate = clk->pstate = -1;
 	}
@@ -554,16 +553,6 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
 	return nvkm_clk_update(clk, false);
 }
 
-int
-nvkm_clk_dstate(struct nvkm_clk *clk, int req, int rel)
-{
-	if (!rel) clk->dstate  = req;
-	if ( rel) clk->dstate += rel;
-	clk->dstate = min(clk->dstate, clk->state_nr - 1);
-	clk->dstate = max(clk->dstate, 0);
-	return nvkm_clk_update(clk, true);
-}
-
 static int
 nvkm_clk_pwrsrc(struct nvkm_notify *notify)
 {
@@ -621,7 +610,6 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
 		return clk->func->init(clk);
 
 	clk->astate = clk->state_nr - 1;
-	clk->dstate = 0;
 	clk->pstate = -1;
 	clk->temp = 90; /* reasonable default value */
 	nvkm_clk_update(clk, true);
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

[Karol Herbst]

We will access the current cstate at least every second and this saves us
some CPU cycles looking them up every second.

v2: Rewording commit message.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  4 +++-
 drm/nouveau/nvkm/engine/device/ctrl.c |  5 ++++-
 drm/nouveau/nvkm/subdev/clk/base.c    | 17 ++++++++++++-----
 drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 18 +++++++-----------
 4 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index 1340f5b8..ec537e08 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -10,6 +10,8 @@ struct nvkm_pll_vals;
 #define NVKM_CLK_CSTATE_BASE    -2 /* pstate base */
 #define NVKM_CLK_CSTATE_HIGHEST -3 /* highest possible */
 
+#define NVKM_CLK_PSTATE_DEFAULT -1
+
 enum nv_clk_src {
 	nv_clk_src_crystal,
 	nv_clk_src_href,
@@ -95,7 +97,7 @@ struct nvkm_clk {
 
 	struct nvkm_notify pwrsrc_ntfy;
 	int pwrsrc;
-	int pstate; /* current */
+	struct nvkm_pstate *pstate; /* current */
 	int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
 	int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
 	int astate; /* perfmon adjustment (base) */
diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c b/drm/nouveau/nvkm/engine/device/ctrl.c
index b0ece71a..da70626c 100644
--- a/drm/nouveau/nvkm/engine/device/ctrl.c
+++ b/drm/nouveau/nvkm/engine/device/ctrl.c
@@ -52,7 +52,10 @@ nvkm_control_mthd_pstate_info(struct nvkm_control *ctrl, void *data, u32 size)
 		args->v0.ustate_ac = clk->ustate_ac;
 		args->v0.ustate_dc = clk->ustate_dc;
 		args->v0.pwrsrc = clk->pwrsrc;
-		args->v0.pstate = clk->pstate;
+		if (clk->pstate)
+			args->v0.pstate = clk->pstate->pstate;
+		else
+			args->v0.pstate = NVKM_CLK_PSTATE_DEFAULT;
 	} else {
 		args->v0.count = 0;
 		args->v0.ustate_ac = NVIF_CONTROL_PSTATE_INFO_V0_USTATE_DISABLE;
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 07d530ed..0d4d9fdf 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -271,13 +271,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
 	struct nvkm_pstate *pstate;
 	int ret, idx = 0;
 
+	if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
+		return 0;
+
 	list_for_each_entry(pstate, &clk->states, head) {
 		if (idx++ == pstatei)
 			break;
 	}
 
 	nvkm_debug(subdev, "setting performance state %d\n", pstatei);
-	clk->pstate = pstatei;
+	clk->pstate = pstate;
 
 	nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
 
@@ -306,8 +309,12 @@ nvkm_clk_update_work(struct work_struct *work)
 		return;
 	clk->pwrsrc = power_supply_is_system_supplied();
 
+	if (clk->pstate)
+		pstate = clk->pstate->pstate;
+	else
+		pstate = NVKM_CLK_PSTATE_DEFAULT;
 	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
-		   clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
+		   pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
 		   clk->astate, clk->temp);
 
 	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
@@ -315,11 +322,11 @@ nvkm_clk_update_work(struct work_struct *work)
 		pstate = (pstate < 0) ? clk->astate : pstate;
 		pstate = min(pstate, clk->state_nr - 1);
 	} else {
-		pstate = clk->pstate = -1;
+		pstate = NVKM_CLK_PSTATE_DEFAULT;
 	}
 
 	nvkm_trace(subdev, "-> %d\n", pstate);
-	if (pstate != clk->pstate) {
+	if (!clk->pstate || pstate != clk->pstate->pstate) {
 		int ret = nvkm_pstate_prog(clk, pstate);
 		if (ret) {
 			nvkm_error(subdev, "error setting pstate %d: %d\n",
@@ -610,7 +617,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
 		return clk->func->init(clk);
 
 	clk->astate = clk->state_nr - 1;
-	clk->pstate = -1;
+	clk->pstate = NULL;
 	clk->temp = 90; /* reasonable default value */
 	nvkm_clk_update(clk, true);
 	return 0;
diff --git a/drm/nouveau/nvkm/subdev/pmu/gk20a.c b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
index 05e81855..de579726 100644
--- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
+++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
@@ -55,24 +55,22 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int *state)
 	return nvkm_clk_astate(clk, *state, 0, false);
 }
 
-static void
-gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
-{
-	struct nvkm_clk *clk = pmu->base.subdev.device->clk;
-
-	*state = clk->pstate;
-}
-
 static int
 gk20a_pmu_dvfs_get_target_state(struct gk20a_pmu *pmu,
 				int *state, int load)
 {
 	struct gk20a_pmu_dvfs_data *data = pmu->data;
 	struct nvkm_clk *clk = pmu->base.subdev.device->clk;
+	struct nvkm_pstate *pstate = clk->pstate;
 	int cur_level, level;
 
+	if (!pstate) {
+		*state = 0;
+		return 1;
+	}
+
 	/* For GK20A, the performance level is directly mapped to pstate */
-	level = cur_level = clk->pstate;
+	level = cur_level = clk->pstate->pstate;
 
 	if (load > data->p_load_max) {
 		level = min(clk->state_nr - 1, level + (clk->state_nr / 3));
@@ -142,8 +140,6 @@ gk20a_pmu_dvfs_work(struct nvkm_alarm *alarm)
 	nvkm_trace(subdev, "utilization = %d %%, avg_load = %d %%\n",
 		   utilization, data->avg_load);
 
-	gk20a_pmu_dvfs_get_cur_state(pmu, &state);
-
 	if (gk20a_pmu_dvfs_get_target_state(pmu, &state, data->avg_load)) {
 		nvkm_trace(subdev, "set new state to %d\n", state);
 		gk20a_pmu_dvfs_target(pmu, &state);
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 07/29] clk: Hold information about the current cstate status

[Karol Herbst]

Later we will have situations where the expected and the current state
isn't the same.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  2 ++
 drm/nouveau/nvkm/subdev/clk/base.c    | 32 +++++++++++++++++++++++++-------
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index ec537e08..f35518c3 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -101,6 +101,8 @@ struct nvkm_clk {
 	int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
 	int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
 	int astate; /* perfmon adjustment (base) */
+	struct nvkm_cstate *cstate;
+	int exp_cstateid;
 	u8  temp;
 
 	bool allow_reclock;
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 0d4d9fdf..d37c13b7 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -146,9 +146,14 @@ static struct nvkm_cstate *
 nvkm_cstate_get(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
 {
 	struct nvkm_cstate *cstate;
-	if (cstatei == NVKM_CLK_CSTATE_HIGHEST)
+	switch (cstatei) {
+	case NVKM_CLK_CSTATE_HIGHEST:
 		return list_last_entry(&pstate->list, typeof(*cstate), head);
-	else {
+	case NVKM_CLK_CSTATE_BASE:
+		return &pstate->base;
+	case NVKM_CLK_CSTATE_DEFAULT:
+		return NULL;
+	default:
 		list_for_each_entry(cstate, &pstate->list, head) {
 			if (cstate->id == cstatei)
 				return cstate;
@@ -167,6 +172,9 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
 	struct nvkm_cstate *cstate;
 	int ret;
 
+	if (cstatei == NVKM_CLK_CSTATE_DEFAULT)
+		return 0;
+
 	if (!list_empty(&pstate->list)) {
 		cstate = nvkm_cstate_get(clk, pstate, cstatei);
 		cstate = nvkm_cstate_find_best(clk, pstate, cstate);
@@ -193,6 +201,7 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
 
 	ret = clk->func->calc(clk, cstate);
 	if (ret == 0) {
+		clk->cstate = cstate;
 		ret = clk->func->prog(clk);
 		clk->func->tidy(clk);
 	}
@@ -295,7 +304,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
 		ram->func->tidy(ram);
 	}
 
-	return nvkm_cstate_prog(clk, pstate, NVKM_CLK_CSTATE_HIGHEST);
+	return nvkm_cstate_prog(clk, pstate, clk->exp_cstateid);
 }
 
 static void
@@ -313,9 +322,9 @@ nvkm_clk_update_work(struct work_struct *work)
 		pstate = clk->pstate->pstate;
 	else
 		pstate = NVKM_CLK_PSTATE_DEFAULT;
-	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
+	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d C %d T %d°C\n",
 		   pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
-		   clk->astate, clk->temp);
+		   clk->astate, clk->exp_cstateid, clk->temp);
 
 	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
 	if (clk->state_nr && pstate != -1) {
@@ -536,6 +545,7 @@ nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
 	if (ret >= 0) {
 		if (ret -= 2, pwr) clk->ustate_ac = ret;
 		else		   clk->ustate_dc = ret;
+		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
 		return nvkm_clk_update(clk, true);
 	}
 	return ret;
@@ -548,6 +558,7 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
 	if ( rel) clk->astate += rel;
 	clk->astate = min(clk->astate, clk->state_nr - 1);
 	clk->astate = max(clk->astate, 0);
+	clk->exp_cstateid = NVKM_CLK_CSTATE_BASE;
 	return nvkm_clk_update(clk, wait);
 }
 
@@ -618,6 +629,8 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
 
 	clk->astate = clk->state_nr - 1;
 	clk->pstate = NULL;
+	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
+	clk->cstate = NULL;
 	clk->temp = 90; /* reasonable default value */
 	nvkm_clk_update(clk, true);
 	return 0;
@@ -701,15 +714,20 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 	if (mode) {
 		clk->ustate_ac = nvkm_clk_nstate(clk, mode, arglen);
 		clk->ustate_dc = nvkm_clk_nstate(clk, mode, arglen);
+		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
 	}
 
 	mode = nvkm_stropt(device->cfgopt, "NvClkModeAC", &arglen);
-	if (mode)
+	if (mode) {
 		clk->ustate_ac = nvkm_clk_nstate(clk, mode, arglen);
+		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
+	}
 
 	mode = nvkm_stropt(device->cfgopt, "NvClkModeDC", &arglen);
-	if (mode)
+	if (mode) {
 		clk->ustate_dc = nvkm_clk_nstate(clk, mode, arglen);
+		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
+	}
 
 	clk->boost_mode = nvkm_longopt(device->cfgopt, "NvBoost",
 				       NVKM_CLK_BOOST_NONE);
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list

[Karol Herbst]

This makes the code easier, because we can compare the id with
pstate->pstate and saves us from the trouble of iterating over the pstates
to match the index.

v2: reword commit message

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/nouveau_debugfs.c      |  6 +--
 drm/nouveau/nvkm/subdev/clk/base.c | 78 +++++++++++++++++++-------------------
 2 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/drm/nouveau/nouveau_debugfs.c b/drm/nouveau/nouveau_debugfs.c
index 963a4dba..27281c4e 100644
--- a/drm/nouveau/nouveau_debugfs.c
+++ b/drm/nouveau/nouveau_debugfs.c
@@ -96,11 +96,11 @@ nouveau_debugfs_pstate_get(struct seq_file *m, void *data)
 		} while (attr.index);
 
 		if (state >= 0) {
-			if (info.ustate_ac == state)
+			if (info.ustate_ac == attr.state)
 				seq_printf(m, " AC");
-			if (info.ustate_dc == state)
+			if (info.ustate_dc == attr.state)
 				seq_printf(m, " DC");
-			if (info.pstate == state)
+			if (info.pstate == attr.state)
 				seq_printf(m, " *");
 		} else {
 			if (info.ustate_ac < -1)
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index d37c13b7..1d71bf09 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -272,23 +272,26 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
  * P-States
  *****************************************************************************/
 static int
-nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
+nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
 {
 	struct nvkm_subdev *subdev = &clk->subdev;
 	struct nvkm_fb *fb = subdev->device->fb;
 	struct nvkm_pci *pci = subdev->device->pci;
 	struct nvkm_pstate *pstate;
-	int ret, idx = 0;
+	int ret;
 
-	if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
+	if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
 		return 0;
 
 	list_for_each_entry(pstate, &clk->states, head) {
-		if (idx++ == pstatei)
+		if (pstate->pstate == pstateid)
 			break;
 	}
 
-	nvkm_debug(subdev, "setting performance state %d\n", pstatei);
+	if (!pstate)
+		return -EINVAL;
+
+	nvkm_debug(subdev, "setting performance state %x\n", pstateid);
 	clk->pstate = pstate;
 
 	nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
@@ -329,7 +332,6 @@ nvkm_clk_update_work(struct work_struct *work)
 	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
 	if (clk->state_nr && pstate != -1) {
 		pstate = (pstate < 0) ? clk->astate : pstate;
-		pstate = min(pstate, clk->state_nr - 1);
 	} else {
 		pstate = NVKM_CLK_PSTATE_DEFAULT;
 	}
@@ -490,33 +492,10 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx)
 /******************************************************************************
  * Adjustment triggers
  *****************************************************************************/
-static int
-nvkm_clk_ustate_update(struct nvkm_clk *clk, int req)
-{
-	struct nvkm_pstate *pstate;
-	int i = 0;
-
-	if (!clk->allow_reclock)
-		return -ENOSYS;
-
-	if (req != -1 && req != -2) {
-		list_for_each_entry(pstate, &clk->states, head) {
-			if (pstate->pstate == req)
-				break;
-			i++;
-		}
-
-		if (pstate->pstate != req)
-			return -EINVAL;
-		req = i;
-	}
-
-	return req + 2;
-}
-
 static int
 nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
 {
+	struct nvkm_pstate *pstate;
 	int ret = 1;
 
 	if (clk->allow_reclock && !strncasecmpz(mode, "auto", arglen))
@@ -528,27 +507,46 @@ nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
 
 		((char *)mode)[arglen] = '\0';
 		if (!kstrtol(mode, 0, &v)) {
-			ret = nvkm_clk_ustate_update(clk, v);
+			ret = v;
 			if (ret < 0)
 				ret = 1;
 		}
 		((char *)mode)[arglen] = save;
 	}
 
-	return ret - 2;
+	if (ret < 0)
+		return ret;
+
+	list_for_each_entry(pstate, &clk->states, head) {
+		if (pstate->pstate == ret)
+			return ret;
+	}
+	return -EINVAL;
 }
 
 int
 nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
 {
-	int ret = nvkm_clk_ustate_update(clk, req);
-	if (ret >= 0) {
-		if (ret -= 2, pwr) clk->ustate_ac = ret;
-		else		   clk->ustate_dc = ret;
-		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
-		return nvkm_clk_update(clk, true);
+	struct nvkm_pstate *pstate;
+	bool valid = false;
+
+	list_for_each_entry(pstate, &clk->states, head) {
+		if (pstate->pstate == req) {
+			valid = true;
+			break;
+		}
 	}
-	return ret;
+
+	if (!valid)
+		return -EINVAL;
+
+	if (pwr)
+		clk->ustate_ac = req;
+	else
+		clk->ustate_dc = req;
+
+	clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
+	return nvkm_clk_update(clk, true);
 }
 
 int
@@ -627,7 +625,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
 	if (clk->func->init)
 		return clk->func->init(clk);
 
-	clk->astate = clk->state_nr - 1;
+	clk->astate = NVKM_CLK_PSTATE_DEFAULT;
 	clk->pstate = NULL;
 	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
 	clk->cstate = NULL;
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 09/29] clk: Set clocks to pre suspend state after suspend

[Karol Herbst]

The idea is to clear out the saved state, because after a resume we can't
know what the GPU is clocked to. The reclock is triggered by the call to
nvkm_clk_update later in nvkm_clk_init.

v2: convert to C style comments

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/nvkm/subdev/clk/base.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 1d71bf09..54188d2b 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -625,11 +625,10 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
 	if (clk->func->init)
 		return clk->func->init(clk);
 
-	clk->astate = NVKM_CLK_PSTATE_DEFAULT;
+	/* after a resume we have no idea what clocks are set, reset the state
+	 */
 	clk->pstate = NULL;
-	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
 	clk->cstate = NULL;
-	clk->temp = 90; /* reasonable default value */
 	nvkm_clk_update(clk, true);
 	return 0;
 }
@@ -683,8 +682,13 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 	clk->func = func;
 	INIT_LIST_HEAD(&clk->states);
 	clk->domains = func->domains;
+
+	clk->astate = NVKM_CLK_PSTATE_DEFAULT;
 	clk->ustate_ac = -1;
 	clk->ustate_dc = -1;
+	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
+	clk->temp = 90; /* reasonable default value */
+
 	clk->allow_reclock = allow_reclock;
 
 	INIT_WORK(&clk->work, nvkm_clk_update_work);
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 10/29] core/device: Move therm behind clk

[Karol Herbst]

Later therm will depend on clk reporting new temperatures and triggereing
reclocks for thermal throttling or therm related voltage/clock adjustments.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/core/device.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drm/nouveau/include/nvkm/core/device.h b/drm/nouveau/include/nvkm/core/device.h
index bb4c214f..44a99c3c 100644
--- a/drm/nouveau/include/nvkm/core/device.h
+++ b/drm/nouveau/include/nvkm/core/device.h
@@ -24,8 +24,8 @@ enum nvkm_devidx {
 	NVKM_SUBDEV_PMU,
 	NVKM_SUBDEV_VOLT,
 	NVKM_SUBDEV_ICCSENSE,
-	NVKM_SUBDEV_THERM,
 	NVKM_SUBDEV_CLK,
+	NVKM_SUBDEV_THERM,
 	NVKM_SUBDEV_SECBOOT,
 
 	NVKM_ENGINE_BSP,
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 11/29] debugfs: Wake up GPU before doing any reclocking

[Karol Herbst]

Fixes various reclocking related issues on prime systems

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nouveau_debugfs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drm/nouveau/nouveau_debugfs.c b/drm/nouveau/nouveau_debugfs.c
index 27281c4e..b0a598f9 100644
--- a/drm/nouveau/nouveau_debugfs.c
+++ b/drm/nouveau/nouveau_debugfs.c
@@ -160,7 +160,11 @@ nouveau_debugfs_pstate_set(struct file *file, const char __user *ubuf,
 		args.ustate = value;
 	}
 
+	ret = pm_runtime_get_sync(drm->dev);
+	if (IS_ERR_VALUE(ret) && ret != -EACCES)
+		return ret;
 	ret = nvif_mthd(ctrl, NVIF_CONTROL_PSTATE_USER, &args, sizeof(args));
+	pm_runtime_put_autosuspend(drm->dev);
 	if (ret < 0)
 		return ret;
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 12/29] therm: Don't cancel the timer

[Karol Herbst]

We will need a always running therm daemon to adjust the voltage/clocks on
the fly.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/nvkm/subdev/therm/base.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/therm/base.c b/drm/nouveau/nvkm/subdev/therm/base.c
index 8fa691fb..4e69cd8e 100644
--- a/drm/nouveau/nvkm/subdev/therm/base.c
+++ b/drm/nouveau/nvkm/subdev/therm/base.c
@@ -114,7 +114,6 @@ nvkm_therm_update(struct nvkm_therm *therm, int mode)
 	struct nvkm_timer *tmr = subdev->device->timer;
 	unsigned long flags;
 	bool immd = true;
-	bool poll = true;
 	int duty = -1;
 
 	spin_lock_irqsave(&therm->lock, flags);
@@ -124,11 +123,9 @@ nvkm_therm_update(struct nvkm_therm *therm, int mode)
 
 	switch (mode) {
 	case NVKM_THERM_CTRL_MANUAL:
-		nvkm_timer_alarm(tmr, 0, &therm->alarm);
 		duty = nvkm_therm_fan_get(therm);
 		if (duty < 0)
 			duty = 100;
-		poll = false;
 		break;
 	case NVKM_THERM_CTRL_AUTO:
 		switch(therm->fan->bios.fan_mode) {
@@ -143,18 +140,16 @@ nvkm_therm_update(struct nvkm_therm *therm, int mode)
 				duty = therm->cstate;
 			else
 				duty = nvkm_therm_update_linear_fallback(therm);
-			poll = false;
 			break;
 		}
 		immd = false;
 		break;
 	case NVKM_THERM_CTRL_NONE:
 	default:
-		nvkm_timer_alarm(tmr, 0, &therm->alarm);
-		poll = false;
+		break;
 	}
 
-	if (poll)
+	if (list_empty(&therm->alarm.head))
 		nvkm_timer_alarm(tmr, 1000000000ULL, &therm->alarm);
 	spin_unlock_irqrestore(&therm->lock, flags);
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 13/29] therm: Move the temp readout into the alarm

[Karol Herbst]

It makes more sense to read out the temperature in the alarm, because we
want to do various things with it:
 1. adjust the fans
 2. notify the clk subdev about the changed temperature

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/therm.h |  1 +
 drm/nouveau/nvkm/subdev/therm/base.c    | 43 +++++++++++++++------------------
 2 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/therm.h b/drm/nouveau/include/nvkm/subdev/therm.h
index 8c84017f..27670497 100644
--- a/drm/nouveau/include/nvkm/subdev/therm.h
+++ b/drm/nouveau/include/nvkm/subdev/therm.h
@@ -56,6 +56,7 @@ struct nvkm_therm {
 	int mode;
 	int cstate;
 	int suspend;
+	u8  last_temp;
 
 	/* bios */
 	struct nvbios_therm_sensor bios_sensor;
diff --git a/drm/nouveau/nvkm/subdev/therm/base.c b/drm/nouveau/nvkm/subdev/therm/base.c
index 4e69cd8e..59f01fec 100644
--- a/drm/nouveau/nvkm/subdev/therm/base.c
+++ b/drm/nouveau/nvkm/subdev/therm/base.c
@@ -32,18 +32,13 @@ nvkm_therm_temp_get(struct nvkm_therm *therm, int *val)
 }
 
 static int
-nvkm_therm_update_trip(struct nvkm_therm *therm)
+nvkm_therm_update_trip(struct nvkm_therm *therm, u8 temp)
 {
-	int temp, ret;
 	struct nvbios_therm_trip_point *trip = therm->fan->bios.trip,
 				       *cur_trip = NULL,
 				       *last_trip = therm->last_trip;
 	u16 duty, i;
 
-	ret = therm->func->temp_get(therm, &temp);
-	if (ret < 0)
-		return ret;
-
 	/* look for the trip point corresponding to the current temperature */
 	cur_trip = NULL;
 	for (i = 0; i < therm->fan->bios.nr_fan_trip; i++) {
@@ -69,15 +64,10 @@ nvkm_therm_update_trip(struct nvkm_therm *therm)
 
 static int
 nvkm_therm_compute_linear_duty(struct nvkm_therm *therm, u8 linear_min_temp,
-                               u8 linear_max_temp)
+                               u8 linear_max_temp, u8 temp)
 {
-	int temp, ret;
 	u16 duty;
 
-	ret = therm->func->temp_get(therm, &temp);
-	if (ret < 0)
-		return ret;
-
 	/* handle the non-linear part first */
 	if (temp < linear_min_temp)
 		return therm->fan->bios.min_duty;
@@ -93,22 +83,22 @@ nvkm_therm_compute_linear_duty(struct nvkm_therm *therm, u8 linear_min_temp,
 }
 
 static int
-nvkm_therm_update_linear(struct nvkm_therm *therm)
+nvkm_therm_update_linear(struct nvkm_therm *therm, u8 temp)
 {
 	u8  min = therm->fan->bios.linear_min_temp;
 	u8  max = therm->fan->bios.linear_max_temp;
-	return nvkm_therm_compute_linear_duty(therm, min, max);
+	return nvkm_therm_compute_linear_duty(therm, min, max, temp);
 }
 
 static int
-nvkm_therm_update_linear_fallback(struct nvkm_therm *therm)
+nvkm_therm_update_linear_fallback(struct nvkm_therm *therm, u8 temp)
 {
 	u8 max = therm->bios_sensor.thrs_fan_boost.temp;
-	return nvkm_therm_compute_linear_duty(therm, 30, max);
+	return nvkm_therm_compute_linear_duty(therm, 30, max, temp);
 }
 
 static void
-nvkm_therm_update(struct nvkm_therm *therm, int mode)
+nvkm_therm_update(struct nvkm_therm *therm, u8 temp, int mode)
 {
 	struct nvkm_subdev *subdev = &therm->subdev;
 	struct nvkm_timer *tmr = subdev->device->timer;
@@ -130,16 +120,16 @@ nvkm_therm_update(struct nvkm_therm *therm, int mode)
 	case NVKM_THERM_CTRL_AUTO:
 		switch(therm->fan->bios.fan_mode) {
 		case NVBIOS_THERM_FAN_TRIP:
-			duty = nvkm_therm_update_trip(therm);
+			duty = nvkm_therm_update_trip(therm, temp);
 			break;
 		case NVBIOS_THERM_FAN_LINEAR:
-			duty = nvkm_therm_update_linear(therm);
+			duty = nvkm_therm_update_linear(therm, temp);
 			break;
 		case NVBIOS_THERM_FAN_OTHER:
 			if (therm->cstate)
 				duty = therm->cstate;
 			else
-				duty = nvkm_therm_update_linear_fallback(therm);
+				duty = nvkm_therm_update_linear_fallback(therm, temp);
 			break;
 		}
 		immd = false;
@@ -167,7 +157,7 @@ nvkm_therm_cstate(struct nvkm_therm *therm, int fan, int dir)
 		    (dir > 0 && fan > therm->cstate)) {
 		nvkm_debug(subdev, "default fan speed -> %d%%\n", fan);
 		therm->cstate = fan;
-		nvkm_therm_update(therm, -1);
+		nvkm_therm_update(therm, therm->last_temp, -1);
 	}
 	return 0;
 }
@@ -175,9 +165,12 @@ nvkm_therm_cstate(struct nvkm_therm *therm, int fan, int dir)
 static void
 nvkm_therm_alarm(struct nvkm_alarm *alarm)
 {
+	int temp;
 	struct nvkm_therm *therm =
 	       container_of(alarm, struct nvkm_therm, alarm);
-	nvkm_therm_update(therm, -1);
+	if (nvkm_therm_temp_get(therm, &temp) >= 0)
+		therm->last_temp = temp;
+	nvkm_therm_update(therm, therm->last_temp, -1);
 }
 
 int
@@ -208,7 +201,7 @@ nvkm_therm_fan_mode(struct nvkm_therm *therm, int mode)
 		return 0;
 
 	nvkm_debug(subdev, "fan management: %s\n", name[mode]);
-	nvkm_therm_update(therm, mode);
+	nvkm_therm_update(therm, therm->last_temp, mode);
 	return 0;
 }
 
@@ -343,11 +336,15 @@ nvkm_therm_oneinit(struct nvkm_subdev *subdev)
 static int
 nvkm_therm_init(struct nvkm_subdev *subdev)
 {
+	int temp;
 	struct nvkm_therm *therm = nvkm_therm(subdev);
 
 	if (therm->func->init)
 		therm->func->init(therm);
 
+	nvkm_therm_temp_get(therm, &temp);
+	therm->last_temp = temp;
+
 	if (therm->suspend >= 0) {
 		/* restore the pwm value only when on manual or auto mode */
 		if (therm->suspend > 0)
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 14/29] therm: Trigger reclock in temperature daemon

[Karol Herbst]

Depending on the temperature, cstates might become unreachable or the maped
voltage of a cstate changes. We want to adjust to that.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/nvkm/subdev/therm/base.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drm/nouveau/nvkm/subdev/therm/base.c b/drm/nouveau/nvkm/subdev/therm/base.c
index 59f01fec..c90b9cfc 100644
--- a/drm/nouveau/nvkm/subdev/therm/base.c
+++ b/drm/nouveau/nvkm/subdev/therm/base.c
@@ -23,6 +23,8 @@
  */
 #include "priv.h"
 
+#include <subdev/clk.h>
+
 int
 nvkm_therm_temp_get(struct nvkm_therm *therm, int *val)
 {
@@ -168,9 +170,15 @@ nvkm_therm_alarm(struct nvkm_alarm *alarm)
 	int temp;
 	struct nvkm_therm *therm =
 	       container_of(alarm, struct nvkm_therm, alarm);
+	struct nvkm_clk *clk = therm->subdev.device->clk;
+
 	if (nvkm_therm_temp_get(therm, &temp) >= 0)
 		therm->last_temp = temp;
+
 	nvkm_therm_update(therm, therm->last_temp, -1);
+
+	if (clk)
+		nvkm_clk_tstate(clk, therm->last_temp);
 }
 
 int
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 15/29] bios: add thermal policies table

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 .../include/nvkm/subdev/bios/thermal_policies.h    | 27 ++++++++
 drm/nouveau/nvkm/subdev/bios/Kbuild                |  1 +
 drm/nouveau/nvkm/subdev/bios/thermal_policies.c    | 81 ++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
 create mode 100644 drm/nouveau/nvkm/subdev/bios/thermal_policies.c

diff --git a/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h b/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
new file mode 100644
index 00000000..c9215fdd
--- /dev/null
+++ b/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
@@ -0,0 +1,27 @@
+#ifndef __NVBIOS_THERMAL_POLICIES_H__
+#define __NVBIOS_THERMAL_POLICIES_H__
+
+struct nvbios_thermal_policies_header {
+	u32 offset;
+
+	u8 version;
+	u8 hlen;
+	u8 ecount;
+	u8 elen;
+};
+struct nvbios_thermal_policies_entry {
+	u8  mode;
+	u16 t0;
+	u16 t1;
+	u16 t2;
+	s16 down_offset;
+	s16 up_offset;
+};
+
+int nvbios_thermal_policies_parse(struct nvkm_bios *,
+				  struct nvbios_thermal_policies_header *);
+int nvbios_thermal_policies_entry(struct nvkm_bios *,
+				  struct nvbios_thermal_policies_header *,
+				  u8 idx,
+				  struct nvbios_thermal_policies_entry *);
+#endif
diff --git a/drm/nouveau/nvkm/subdev/bios/Kbuild b/drm/nouveau/nvkm/subdev/bios/Kbuild
index 6b4f1e06..38f31dd0 100644
--- a/drm/nouveau/nvkm/subdev/bios/Kbuild
+++ b/drm/nouveau/nvkm/subdev/bios/Kbuild
@@ -30,6 +30,7 @@ nvkm-y += nvkm/subdev/bios/shadowramin.o
 nvkm-y += nvkm/subdev/bios/shadowrom.o
 nvkm-y += nvkm/subdev/bios/timing.o
 nvkm-y += nvkm/subdev/bios/therm.o
+nvkm-y += nvkm/subdev/bios/thermal_policies.o
 nvkm-y += nvkm/subdev/bios/vmap.o
 nvkm-y += nvkm/subdev/bios/volt.o
 nvkm-y += nvkm/subdev/bios/vpstate.o
diff --git a/drm/nouveau/nvkm/subdev/bios/thermal_policies.c b/drm/nouveau/nvkm/subdev/bios/thermal_policies.c
new file mode 100644
index 00000000..5105194e
--- /dev/null
+++ b/drm/nouveau/nvkm/subdev/bios/thermal_policies.c
@@ -0,0 +1,81 @@
+/*
+ * Copyright 2017 Karol Herbst
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Karol Herbst
+ */
+#include <subdev/bios.h>
+#include <subdev/bios/bit.h>
+#include <subdev/bios/thermal_policies.h>
+
+static u32
+nvbios_thermal_policies_offset(struct nvkm_bios *b)
+{
+	struct bit_entry bit_P;
+
+	if (!bit_entry(b, 'P', &bit_P)) {
+		if (bit_P.version == 2 && bit_P.length >= 0x50)
+			return nvbios_rd32(b, bit_P.offset + 0x50);
+	}
+
+	return 0;
+}
+
+int
+nvbios_thermal_policies_parse(struct nvkm_bios *b, struct nvbios_thermal_policies_header *h)
+{
+	if (!h)
+		return -EINVAL;
+
+	h->offset = nvbios_thermal_policies_offset(b);
+	if (!h->offset)
+		return -ENODEV;
+
+	h->version = nvbios_rd08(b, h->offset);
+	switch (h->version) {
+	case 0x10:
+		h->hlen     = nvbios_rd08(b, h->offset + 0x1);
+		h->elen     = nvbios_rd08(b, h->offset + 0x2);
+		h->ecount   = nvbios_rd08(b, h->offset + 0x3);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+int
+nvbios_thermal_policies_entry(struct nvkm_bios *b, struct nvbios_thermal_policies_header *h,
+		     u8 idx, struct nvbios_thermal_policies_entry *e)
+{
+	u32 offset;
+
+	if (!e || !h || idx > h->ecount)
+		return -EINVAL;
+
+	offset = h->offset + h->hlen + idx * h->elen;
+	e->mode = nvbios_rd08(b, offset);
+	e->t0 = nvbios_rd16(b, offset + 0x2);
+	e->t1 = nvbios_rd16(b, offset + 0x4);
+	e->t2 = nvbios_rd16(b, offset + 0x6);
+	e->down_offset = nvbios_rd16(b, offset + 0x12);
+	e->up_offset = nvbios_rd16(b, offset + 0x14);
+
+	return 0;
+}
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 16/29] clk: parse thermal policies for throttling thresholds

[Karol Herbst]

v2: use min_t

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  2 ++
 drm/nouveau/nvkm/subdev/clk/base.c    | 42 +++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index f35518c3..f5ff1fd9 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -104,6 +104,8 @@ struct nvkm_clk {
 	struct nvkm_cstate *cstate;
 	int exp_cstateid;
 	u8  temp;
+	u8  max_temp;
+	u8  relax_temp;
 
 	bool allow_reclock;
 #define NVKM_CLK_BOOST_NONE 0x0
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 54188d2b..54e14936 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -27,6 +27,7 @@
 #include <subdev/bios/boost.h>
 #include <subdev/bios/cstep.h>
 #include <subdev/bios/perf.h>
+#include <subdev/bios/thermal_policies.h>
 #include <subdev/bios/vpstate.h>
 #include <subdev/fb.h>
 #include <subdev/therm.h>
@@ -659,6 +660,44 @@ nvkm_clk = {
 	.fini = nvkm_clk_fini,
 };
 
+static void
+nvkm_clk_parse_max_temp(struct nvkm_clk *clk)
+{
+	struct nvkm_subdev *subdev = &clk->subdev;
+	struct nvkm_bios *bios = subdev->device->bios;
+	struct nvbios_thermal_policies_header header;
+	struct nvbios_thermal_policies_entry entry;
+	u8 i;
+	s16 mt = 0xff;
+	s16 rt = 0xff;
+
+	if (nvbios_thermal_policies_parse(bios, &header))
+		return;
+
+	if (!header.ecount)
+		return;
+
+	for (i = 0; i < header.ecount; i++) {
+		if (nvbios_thermal_policies_entry(bios, &header, i, &entry))
+			return;
+
+		if (entry.mode != 1)
+			continue;
+
+		mt = min_t(s16, mt, (entry.t0 + entry.down_offset) / 32);
+		rt = min_t(s16, rt, (entry.t0 + entry.up_offset) / 32);
+	}
+
+	if (mt == 0xff || rt == 0xff)
+		return;
+
+	clk->max_temp = mt;
+	clk->relax_temp = rt;
+
+	nvkm_debug(subdev, "setting up sw throttling thresholds (%u/%u°C)\n",
+		   clk->max_temp, clk->relax_temp);
+}
+
 int
 nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 	      int index, bool allow_reclock, struct nvkm_clk *clk)
@@ -733,6 +772,9 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 
 	clk->boost_mode = nvkm_longopt(device->cfgopt, "NvBoost",
 				       NVKM_CLK_BOOST_NONE);
+
+	nvkm_clk_parse_max_temp(clk);
+
 	return 0;
 }
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 17/29] clk: thermal throttling

[Karol Herbst]

v2: make message about relaxed throttling an info
    rework reporting about current clk state

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  1 +
 drm/nouveau/nvkm/subdev/clk/base.c    | 44 +++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index f5ff1fd9..b79bf657 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -106,6 +106,7 @@ struct nvkm_clk {
 	u8  temp;
 	u8  max_temp;
 	u8  relax_temp;
+	bool throttled;
 
 	bool allow_reclock;
 #define NVKM_CLK_BOOST_NONE 0x0
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 54e14936..72ca5e0c 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -279,7 +279,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
 	struct nvkm_fb *fb = subdev->device->fb;
 	struct nvkm_pci *pci = subdev->device->pci;
 	struct nvkm_pstate *pstate;
-	int ret;
+	int ret, cstate;
 
 	if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
 		return 0;
@@ -308,7 +308,12 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
 		ram->func->tidy(ram);
 	}
 
-	return nvkm_cstate_prog(clk, pstate, clk->exp_cstateid);
+	if (clk->throttled)
+		cstate = list_first_entry(&pstate->list, struct nvkm_cstate, head)->id;
+	else
+		cstate = clk->exp_cstateid;
+
+	return nvkm_cstate_prog(clk, pstate, cstate);
 }
 
 static void
@@ -322,22 +327,20 @@ nvkm_clk_update_work(struct work_struct *work)
 		return;
 	clk->pwrsrc = power_supply_is_system_supplied();
 
-	if (clk->pstate)
-		pstate = clk->pstate->pstate;
-	else
-		pstate = NVKM_CLK_PSTATE_DEFAULT;
-	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d C %d T %d°C\n",
-		   pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
-		   clk->astate, clk->exp_cstateid, clk->temp);
-
 	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
 	if (clk->state_nr && pstate != -1) {
-		pstate = (pstate < 0) ? clk->astate : pstate;
+		if (clk->throttled)
+			pstate = list_first_entry(&clk->states, struct nvkm_pstate, head)->pstate;
+		else
+			pstate = (pstate < 0) ? clk->astate : pstate;
 	} else {
 		pstate = NVKM_CLK_PSTATE_DEFAULT;
 	}
 
-	nvkm_trace(subdev, "-> %d\n", pstate);
+	nvkm_trace(subdev, "PWR %d U(AC) %d U(DC) %d A %d C %d T %d°C\n",
+		   clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
+		   clk->astate, clk->exp_cstateid, clk->temp);
+
 	if (!clk->pstate || pstate != clk->pstate->pstate) {
 		int ret = nvkm_pstate_prog(clk, pstate);
 		if (ret) {
@@ -564,9 +567,25 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
 int
 nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
 {
+	struct nvkm_subdev *subdev = &clk->subdev;
 	if (clk->temp == temp)
 		return 0;
 	clk->temp = temp;
+	if (clk->max_temp && clk->relax_temp) {
+		if (!clk->throttled && temp > clk->max_temp) {
+			nvkm_warn(subdev,
+				  "temperature (%d C) hit the 'downclock' "
+				  "threshold\n",
+				  temp);
+			clk->throttled = true;
+		} else if (clk->throttled && temp < clk->relax_temp) {
+			nvkm_info(subdev,
+				  "temperature (%d C) went below the "
+				  "'relax' threshold\n",
+				  temp);
+			clk->throttled = false;
+		}
+	}
 	return nvkm_clk_update(clk, false);
 }
 
@@ -727,6 +746,7 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 	clk->ustate_dc = -1;
 	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
 	clk->temp = 90; /* reasonable default value */
+	clk->throttled = false;
 
 	clk->allow_reclock = allow_reclock;
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 18/29] clk: Only do partial reclocks as required

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nvkm/subdev/clk/base.c  | 26 ++++++++++++++++++--------
 drm/nouveau/nvkm/subdev/volt/base.c |  3 +++
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 72ca5e0c..a154a425 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -171,7 +171,7 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
 	struct nvkm_therm *therm = device->therm;
 	struct nvkm_volt *volt = device->volt;
 	struct nvkm_cstate *cstate;
-	int ret;
+	int ret = 0;
 
 	if (cstatei == NVKM_CLK_CSTATE_DEFAULT)
 		return 0;
@@ -183,6 +183,12 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
 		cstate = &pstate->base;
 	}
 
+	// if the cstate matches, just update the voltage
+	if (clk->cstate && clk->cstate == cstate)
+		return nvkm_volt_set_id(volt, clk->cstate->voltage,
+					clk->pstate->base.voltage, clk->temp,
+					0);
+
 	if (therm) {
 		ret = nvkm_therm_cstate(therm, pstate->fanspeed, +1);
 		if (ret && ret != -ENODEV) {
@@ -284,6 +290,11 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
 	if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
 		return 0;
 
+	if (clk->pstate && clk->pstate->pstate == pstateid) {
+		pstate = clk->pstate;
+		goto cstate;
+	}
+
 	list_for_each_entry(pstate, &clk->states, head) {
 		if (pstate->pstate == pstateid)
 			break;
@@ -308,6 +319,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
 		ram->func->tidy(ram);
 	}
 
+cstate:
 	if (clk->throttled)
 		cstate = list_first_entry(&pstate->list, struct nvkm_cstate, head)->id;
 	else
@@ -321,7 +333,7 @@ nvkm_clk_update_work(struct work_struct *work)
 {
 	struct nvkm_clk *clk = container_of(work, typeof(*clk), work);
 	struct nvkm_subdev *subdev = &clk->subdev;
-	int pstate;
+	int pstate, ret;
 
 	if (!atomic_xchg(&clk->waiting, 0))
 		return;
@@ -341,12 +353,10 @@ nvkm_clk_update_work(struct work_struct *work)
 		   clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
 		   clk->astate, clk->exp_cstateid, clk->temp);
 
-	if (!clk->pstate || pstate != clk->pstate->pstate) {
-		int ret = nvkm_pstate_prog(clk, pstate);
-		if (ret) {
-			nvkm_error(subdev, "error setting pstate %d: %d\n",
-				   pstate, ret);
-		}
+	ret = nvkm_pstate_prog(clk, pstate);
+	if (ret) {
+		nvkm_error(subdev, "error setting pstate %d: %d\n",
+			   pstate, ret);
 	}
 
 	wake_up_all(&clk->wait);
diff --git a/drm/nouveau/nvkm/subdev/volt/base.c b/drm/nouveau/nvkm/subdev/volt/base.c
index e344901c..56a0f379 100644
--- a/drm/nouveau/nvkm/subdev/volt/base.c
+++ b/drm/nouveau/nvkm/subdev/volt/base.c
@@ -162,6 +162,9 @@ nvkm_volt_set_id(struct nvkm_volt *volt, u8 id, u8 min_id, u8 temp,
 {
 	int ret;
 
+	if (!volt)
+		return -ENODEV;
+
 	if (volt->func->set_id)
 		return volt->func->set_id(volt, id, condition);
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 19/29] secboot/acr352: reset PMU after secboot

[Karol Herbst]

This is needed for using Nouveaus PMU image after performing secboot. This will
be helpfull for Maxwell2 reclocking on boards without externally controlled
fans like on most laptops or fanless boards.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nvkm/subdev/secboot/acr_r352.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drm/nouveau/nvkm/subdev/secboot/acr_r352.c b/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
index a7213542..00095ef8 100644
--- a/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
+++ b/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
@@ -924,6 +924,19 @@ acr_r352_bootstrap(struct acr_r352 *acr, struct nvkm_secboot *sb)
 		}
 	}
 
+	/* reset the PMU if needed */
+	if (acr->base.boot_falcon == NVKM_SECBOOT_FALCON_PMU &&
+	    !nvkm_secboot_is_managed(sb, NVKM_SECBOOT_FALCON_PMU)) {
+		struct nvkm_pmu *pmu = subdev->device->pmu;
+		if (pmu) {
+			ret = nvkm_subdev_init(&pmu->subdev);
+			if (ret < 0) {
+				nvkm_error(subdev, "Failed to reset PMU\n");
+				return ret;
+			}
+		}
+	}
+
 	return 0;
 }
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 20/29] device: enable clk for Maxwell2

[Karol Herbst]

Reclokcing will only be enabled by setting NvFanless to true.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nvkm/engine/device/base.c |  3 +++
 drm/nouveau/nvkm/subdev/clk/gk104.c   | 10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drm/nouveau/nvkm/engine/device/base.c b/drm/nouveau/nvkm/engine/device/base.c
index 28fd4fa9..c1ede28a 100644
--- a/drm/nouveau/nvkm/engine/device/base.c
+++ b/drm/nouveau/nvkm/engine/device/base.c
@@ -2029,6 +2029,7 @@ nv120_chipset = {
 	.bar = gf100_bar_new,
 	.bios = nvkm_bios_new,
 	.bus = gf100_bus_new,
+	.clk = gk104_clk_new,
 	.devinit = gm200_devinit_new,
 	.fb = gm200_fb_new,
 	.fuse = gm107_fuse_new,
@@ -2064,6 +2065,7 @@ nv124_chipset = {
 	.bar = gf100_bar_new,
 	.bios = nvkm_bios_new,
 	.bus = gf100_bus_new,
+	.clk = gk104_clk_new,
 	.devinit = gm200_devinit_new,
 	.fb = gm200_fb_new,
 	.fuse = gm107_fuse_new,
@@ -2099,6 +2101,7 @@ nv126_chipset = {
 	.bar = gf100_bar_new,
 	.bios = nvkm_bios_new,
 	.bus = gf100_bus_new,
+	.clk = gk104_clk_new,
 	.devinit = gm200_devinit_new,
 	.fb = gm200_fb_new,
 	.fuse = gm107_fuse_new,
diff --git a/drm/nouveau/nvkm/subdev/clk/gk104.c b/drm/nouveau/nvkm/subdev/clk/gk104.c
index 0b37e3da..be04463e 100644
--- a/drm/nouveau/nvkm/subdev/clk/gk104.c
+++ b/drm/nouveau/nvkm/subdev/clk/gk104.c
@@ -25,6 +25,7 @@
 #include "priv.h"
 #include "pll.h"
 
+#include <core/option.h>
 #include <subdev/timer.h>
 #include <subdev/bios.h>
 #include <subdev/bios/pll.h>
@@ -507,10 +508,17 @@ int
 gk104_clk_new(struct nvkm_device *device, int index, struct nvkm_clk **pclk)
 {
 	struct gk104_clk *clk;
+	bool reclocking;
 
 	if (!(clk = kzalloc(sizeof(*clk), GFP_KERNEL)))
 		return -ENOMEM;
 	*pclk = &clk->base;
 
-	return nvkm_clk_ctor(&gk104_clk, device, index, true, &clk->base);
+	if (device->chipset >= 0x120)
+		reclocking = nvkm_boolopt(device->cfgopt, "NvFanless", false);
+	else
+		reclocking = true;
+
+	return nvkm_clk_ctor(&gk104_clk, device, index, reclocking,
+			     &clk->base);
 }
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 21/29] clk: Save the max clock we can set

[Karol Herbst]

Saving the highest possible clock from the vpstate domain makes it easier
to read it out whenever we want.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/include/nvkm/subdev/clk.h | 1 +
 drm/nouveau/nvkm/subdev/clk/base.c    | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index b79bf657..4c225dcb 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -115,6 +115,7 @@ struct nvkm_clk {
 	u8  boost_mode;
 	u32 base_khz;
 	u32 boost_khz;
+	u32 max_khz;
 
 	/*XXX: die, these are here *only* to support the completely
 	 *     bat-shit insane what-was-nouveau_hw.c code
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index a154a425..0fd92790 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -267,6 +267,8 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
 			u32 freq = nvkm_clk_adjust(clk, true, pstate->pstate,
 						   domain->bios, cstepX.freq);
 			cstate->domain[domain->name] = freq;
+			if (domain->flags & NVKM_CLK_DOM_FLAG_VPSTATE)
+				clk->max_khz = max(clk->max_khz, freq);
 		}
 		domain++;
 	}
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 22/29] nvif: Add boost info and set operations

[Karol Herbst]

v5: Return ENODEV on devices without any vpstates.
    Fail earlier if not supported.

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/include/nvif/if0001.h     | 15 +++++++++
 drm/nouveau/nvkm/engine/device/ctrl.c | 61 +++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

diff --git a/drm/nouveau/include/nvif/if0001.h b/drm/nouveau/include/nvif/if0001.h
index bd5b6412..e4acd195 100644
--- a/drm/nouveau/include/nvif/if0001.h
+++ b/drm/nouveau/include/nvif/if0001.h
@@ -4,6 +4,8 @@
 #define NVIF_CONTROL_PSTATE_INFO                                           0x00
 #define NVIF_CONTROL_PSTATE_ATTR                                           0x01
 #define NVIF_CONTROL_PSTATE_USER                                           0x02
+#define NVIF_CONTROL_BOOST_INFO                                            0x03
+#define NVIF_CONTROL_BOOST_SET                                             0x04
 
 struct nvif_control_pstate_info_v0 {
 	__u8  version;
@@ -43,4 +45,17 @@ struct nvif_control_pstate_user_v0 {
 	__s8  pwrsrc; /*  in: target power source */
 	__u8  pad03[5];
 };
+
+struct nvif_control_boost_info_v0 {
+	__u8  version;
+	__u8  mode;
+	__u16 base_mhz;
+	__u16 boost_mhz;
+	__u16 max_mhz;
+};
+
+struct nvif_control_boost_set_v0 {
+	__u8  version;
+	__u8  mode;
+};
 #endif
diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c b/drm/nouveau/nvkm/engine/device/ctrl.c
index da70626c..0c44e4d2 100644
--- a/drm/nouveau/nvkm/engine/device/ctrl.c
+++ b/drm/nouveau/nvkm/engine/device/ctrl.c
@@ -169,6 +169,63 @@ nvkm_control_mthd_pstate_user(struct nvkm_control *ctrl, void *data, u32 size)
 	return ret;
 }
 
+static int
+nvkm_control_mthd_boost_info(struct nvkm_control *ctrl, void *data, u32 size)
+{
+	union {
+		struct nvif_control_boost_info_v0 v0;
+	} *args = data;
+	struct nvkm_clk *clk = ctrl->device->clk;
+	int ret = -ENOSYS;
+
+	if (!clk)
+		return -ENODEV;
+
+	if (!clk->base_khz && !clk->boost_khz)
+		return -ENODEV;
+
+	nvif_ioctl(&ctrl->object, "control boost info size %d\n", size);
+	if (!(ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, false))) {
+		nvif_ioctl(&ctrl->object, "control boost info vers %d\n",
+			   args->v0.version);
+	} else
+		return ret;
+
+	args->v0.mode = clk->boost_mode;
+	args->v0.base_mhz = clk->base_khz / 2000;
+	args->v0.boost_mhz = clk->boost_khz / 2000;
+	args->v0.max_mhz = clk->max_khz / 2000;
+	return 0;
+}
+
+static int
+nvkm_control_mthd_boost_set(struct nvkm_control *ctrl, void *data, u32 size)
+{
+	union {
+		struct nvif_control_boost_set_v0 v0;
+	} *args = data;
+	struct nvkm_clk *clk = ctrl->device->clk;
+	int ret = -ENOSYS;
+
+	if (!clk)
+		return -ENODEV;
+
+	if (!clk->base_khz && !clk->boost_khz)
+		return -ENODEV;
+
+	nvif_ioctl(&ctrl->object, "control boost set size %d\n", size);
+	if (!(ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, false))) {
+		nvif_ioctl(&ctrl->object, "control boost set vers %d\n",
+			   args->v0.version);
+	} else
+		return ret;
+
+	if (args->v0.mode > 2)
+		return -EINVAL;
+	clk->boost_mode = args->v0.mode;
+	return nvkm_clk_update(clk, true);
+}
+
 static int
 nvkm_control_mthd(struct nvkm_object *object, u32 mthd, void *data, u32 size)
 {
@@ -180,6 +237,10 @@ nvkm_control_mthd(struct nvkm_object *object, u32 mthd, void *data, u32 size)
 		return nvkm_control_mthd_pstate_attr(ctrl, data, size);
 	case NVIF_CONTROL_PSTATE_USER:
 		return nvkm_control_mthd_pstate_user(ctrl, data, size);
+	case NVIF_CONTROL_BOOST_INFO:
+		return nvkm_control_mthd_boost_info(ctrl, data, size);
+	case NVIF_CONTROL_BOOST_SET:
+		return nvkm_control_mthd_boost_set(ctrl, data, size);
 	default:
 		break;
 	}
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 23/29] debugfs: Add boost interface to change the boost_mode

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
Reviewed-by: Martin Peres <martin.peres@free.fr>
---
 drm/nouveau/nouveau_debugfs.c | 80 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drm/nouveau/nouveau_debugfs.c b/drm/nouveau/nouveau_debugfs.c
index b0a598f9..c19cc535 100644
--- a/drm/nouveau/nouveau_debugfs.c
+++ b/drm/nouveau/nouveau_debugfs.c
@@ -184,6 +184,85 @@ static const struct file_operations nouveau_pstate_fops = {
 	.write = nouveau_debugfs_pstate_set,
 };
 
+static void
+nouveau_debugfs_boost_get_entry(struct seq_file *m, u8 mode, u8 entry, u16 value)
+{
+	if (value) {
+		if (mode == entry)
+			seq_printf(m, "*%i", entry);
+		else
+			seq_printf(m, " %i", entry);
+		seq_printf(m, ": %u MHz\n", value);
+	}
+}
+
+static int
+nouveau_debugfs_boost_get(struct seq_file *m, void *data)
+{
+	struct drm_device *drm = m->private;
+	struct nouveau_debugfs *debugfs = nouveau_debugfs(drm);
+	struct nvif_object *ctrl = &debugfs->ctrl;
+	struct nvif_control_boost_info_v0 info = {};
+	int ret;
+
+	ret = nvif_mthd(ctrl, NVIF_CONTROL_BOOST_INFO, &info, sizeof(info));
+	if (ret)
+		return ret;
+
+	nouveau_debugfs_boost_get_entry(m, info.mode, 0, info.base_mhz);
+	nouveau_debugfs_boost_get_entry(m, info.mode, 1, info.boost_mhz);
+	nouveau_debugfs_boost_get_entry(m, info.mode, 2, info.max_mhz);
+	return 0;
+}
+
+static ssize_t
+nouveau_debugfs_boost_set(struct file *file, const char __user *ubuf,
+			  size_t len, loff_t *offp)
+{
+	struct seq_file *m = file->private_data;
+	struct drm_device *drm = m->private;
+	struct nouveau_debugfs *debugfs = nouveau_debugfs(drm);
+	struct nvif_object *ctrl = &debugfs->ctrl;
+	struct nvif_control_boost_set_v0 args = {};
+	char buf[3] = {};
+	long ret;
+	u8 value;
+
+	if (len >= sizeof(buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, ubuf, len))
+		return -EFAULT;
+
+	ret = kstrtou8(buf, 10, &value);
+	if (ret)
+		return ret;
+
+	args.mode = value;
+	ret = pm_runtime_get_sync(drm->dev);
+	if (IS_ERR_VALUE(ret) && ret != -EACCES)
+		return ret;
+	ret = nvif_mthd(ctrl, NVIF_CONTROL_BOOST_SET, &args, sizeof(args));
+	pm_runtime_put_autosuspend(drm->dev);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static int
+nouveau_debugfs_boost_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, nouveau_debugfs_boost_get, inode->i_private);
+}
+
+static const struct file_operations nouveau_boost_fops = {
+	.owner = THIS_MODULE,
+	.open = nouveau_debugfs_boost_open,
+	.read = seq_read,
+	.write = nouveau_debugfs_boost_set,
+};
+
 static struct drm_info_list nouveau_debugfs_list[] = {
 	{ "vbios.rom", nouveau_debugfs_vbios_image, 0, NULL },
 };
@@ -193,6 +272,7 @@ static const struct nouveau_debugfs_files {
 	const char *name;
 	const struct file_operations *fops;
 } nouveau_debugfs_files[] = {
+	{"boost", &nouveau_boost_fops},
 	{"pstate", &nouveau_pstate_fops},
 };
 
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 24/29] bios/vpstate: there are some fermi vbios with no boost or tdp entry

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nvkm/subdev/bios/vpstate.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/bios/vpstate.c b/drm/nouveau/nvkm/subdev/bios/vpstate.c
index 20b6fc82..71524548 100644
--- a/drm/nouveau/nvkm/subdev/bios/vpstate.c
+++ b/drm/nouveau/nvkm/subdev/bios/vpstate.c
@@ -58,8 +58,14 @@ nvbios_vpstate_parse(struct nvkm_bios *b, struct nvbios_vpstate_header *h)
 		h->ecount   = nvbios_rd08(b, h->offset + 0x5);
 
 		h->base_id  = nvbios_rd08(b, h->offset + 0x0f);
-		h->boost_id = nvbios_rd08(b, h->offset + 0x10);
-		h->tdp_id   = nvbios_rd08(b, h->offset + 0x11);
+		if (h->hlen > 0x10)
+			h->boost_id = nvbios_rd08(b, h->offset + 0x10);
+		else
+			h->boost_id = 0xff;
+		if (h->hlen > 0x11)
+			h->tdp_id = nvbios_rd08(b, h->offset + 0x11);
+		else
+			h->tdp_id = 0xff;
 		return 0;
 	default:
 		return -EINVAL;
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 25/29] bios/vpstate: parse max battery id

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/bios/vpstate.h | 1 +
 drm/nouveau/nvkm/subdev/bios/vpstate.c         | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drm/nouveau/include/nvkm/subdev/bios/vpstate.h b/drm/nouveau/include/nvkm/subdev/bios/vpstate.h
index 87f804fc..181de47b 100644
--- a/drm/nouveau/include/nvkm/subdev/bios/vpstate.h
+++ b/drm/nouveau/include/nvkm/subdev/bios/vpstate.h
@@ -10,6 +10,7 @@ struct nvbios_vpstate_header {
 	u8 scount;
 	u8 slen;
 
+	u8 battery_id;
 	u8 base_id;
 	u8 boost_id;
 	u8 tdp_id;
diff --git a/drm/nouveau/nvkm/subdev/bios/vpstate.c b/drm/nouveau/nvkm/subdev/bios/vpstate.c
index 71524548..c1de6421 100644
--- a/drm/nouveau/nvkm/subdev/bios/vpstate.c
+++ b/drm/nouveau/nvkm/subdev/bios/vpstate.c
@@ -57,7 +57,8 @@ nvbios_vpstate_parse(struct nvkm_bios *b, struct nvbios_vpstate_header *h)
 		h->scount   = nvbios_rd08(b, h->offset + 0x4);
 		h->ecount   = nvbios_rd08(b, h->offset + 0x5);
 
-		h->base_id  = nvbios_rd08(b, h->offset + 0x0f);
+		h->battery_id = nvbios_rd08(b, h->offset + 0x0c);
+		h->base_id    = nvbios_rd08(b, h->offset + 0x0f);
 		if (h->hlen > 0x10)
 			h->boost_id = nvbios_rd08(b, h->offset + 0x10);
 		else
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 26/29] clk: refactor the base and boost clock limits so that we can limit pstates as well

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  9 +++++++--
 drm/nouveau/nvkm/engine/device/ctrl.c |  8 ++++----
 drm/nouveau/nvkm/subdev/clk/base.c    | 34 +++++++++++++++++++++++++---------
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index 4c225dcb..70505c9b 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -81,6 +81,11 @@ struct nvkm_domain {
 	int mdiv;
 };
 
+struct nvkm_clk_limit {
+	u8  pstate;
+	u32 max_khz;
+};
+
 struct nvkm_clk {
 	const struct nvkm_clk_func *func;
 	struct nvkm_subdev subdev;
@@ -113,8 +118,8 @@ struct nvkm_clk {
 #define NVKM_CLK_BOOST_BIOS 0x1
 #define NVKM_CLK_BOOST_FULL 0x2
 	u8  boost_mode;
-	u32 base_khz;
-	u32 boost_khz;
+	struct nvkm_clk_limit base_limit;
+	struct nvkm_clk_limit boost_limit;
 	u32 max_khz;
 
 	/*XXX: die, these are here *only* to support the completely
diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c b/drm/nouveau/nvkm/engine/device/ctrl.c
index 0c44e4d2..d521cd05 100644
--- a/drm/nouveau/nvkm/engine/device/ctrl.c
+++ b/drm/nouveau/nvkm/engine/device/ctrl.c
@@ -181,7 +181,7 @@ nvkm_control_mthd_boost_info(struct nvkm_control *ctrl, void *data, u32 size)
 	if (!clk)
 		return -ENODEV;
 
-	if (!clk->base_khz && !clk->boost_khz)
+	if (!clk->base_limit.max_khz && !clk->boost_limit.max_khz)
 		return -ENODEV;
 
 	nvif_ioctl(&ctrl->object, "control boost info size %d\n", size);
@@ -192,8 +192,8 @@ nvkm_control_mthd_boost_info(struct nvkm_control *ctrl, void *data, u32 size)
 		return ret;
 
 	args->v0.mode = clk->boost_mode;
-	args->v0.base_mhz = clk->base_khz / 2000;
-	args->v0.boost_mhz = clk->boost_khz / 2000;
+	args->v0.base_mhz = clk->base_limit.max_khz / 2000;
+	args->v0.boost_mhz = clk->boost_limit.max_khz / 2000;
 	args->v0.max_mhz = clk->max_khz / 2000;
 	return 0;
 }
@@ -210,7 +210,7 @@ nvkm_control_mthd_boost_set(struct nvkm_control *ctrl, void *data, u32 size)
 	if (!clk)
 		return -ENODEV;
 
-	if (!clk->base_khz && !clk->boost_khz)
+	if (!clk->base_limit.max_khz && !clk->boost_limit.max_khz)
 		return -ENODEV;
 
 	nvif_ioctl(&ctrl->object, "control boost set size %d\n", size);
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 0fd92790..9f44ace0 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -87,14 +87,22 @@ nvkm_cstate_valid(struct nvkm_clk *clk, struct nvkm_cstate *cstate,
 	while (domain && domain->name != nv_clk_src_max) {
 		if (domain->flags & NVKM_CLK_DOM_FLAG_VPSTATE) {
 			u32 freq = cstate->domain[domain->name];
+			u32 limit;
 			switch (clk->boost_mode) {
 			case NVKM_CLK_BOOST_NONE:
-				if (clk->base_khz && freq > clk->base_khz)
-					return false;
+				limit = clk->base_limit.max_khz;
+				if (limit)
+					break;
 			case NVKM_CLK_BOOST_BIOS:
-				if (clk->boost_khz && freq > clk->boost_khz)
-					return false;
+				limit = clk->boost_limit.max_khz;
+				break;
+			default:
+				limit = 0;
+				break;
 			}
+
+			if (limit && freq > limit)
+				return false;
 		}
 		domain++;
 	}
@@ -729,6 +737,14 @@ nvkm_clk_parse_max_temp(struct nvkm_clk *clk)
 		   clk->max_temp, clk->relax_temp);
 }
 
+void static
+nvkm_clk_fill_limit(struct nvkm_clk_limit *l,
+		    const struct nvbios_vpstate_entry *e)
+{
+	l->pstate = e->pstate;
+	l->max_khz = e->clock_mhz * 1000;
+}
+
 int
 nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 	      int index, bool allow_reclock, struct nvkm_clk *clk)
@@ -742,11 +758,11 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 	nvkm_subdev_ctor(&nvkm_clk, device, index, subdev);
 
 	if (bios && !nvbios_vpstate_parse(bios, &h)) {
-		struct nvbios_vpstate_entry base, boost;
-		if (!nvbios_vpstate_entry(bios, &h, h.boost_id, &boost))
-			clk->boost_khz = boost.clock_mhz * 1000;
-		if (!nvbios_vpstate_entry(bios, &h, h.base_id, &base))
-			clk->base_khz = base.clock_mhz * 1000;
+		struct nvbios_vpstate_entry vpe;
+		if (!nvbios_vpstate_entry(bios, &h, h.boost_id, &vpe))
+			nvkm_clk_fill_limit(&clk->boost_limit, &vpe);
+		if (!nvbios_vpstate_entry(bios, &h, h.base_id, &vpe))
+			nvkm_clk_fill_limit(&clk->base_limit, &vpe);
 	}
 
 	clk->func = func;
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 27/29] clk: implement limiting pstates just like we do for cstates

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nvkm/subdev/clk/base.c | 62 ++++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 9f44ace0..2a51c078 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -288,6 +288,52 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
 /******************************************************************************
  * P-States
  *****************************************************************************/
+static struct nvkm_pstate *
+nvkm_pstate_find_best(struct nvkm_clk *clk, struct nvkm_pstate *start)
+{
+	struct nvkm_pstate *pstate;
+	u8 limit;
+
+	switch (clk->boost_mode) {
+	case NVKM_CLK_BOOST_NONE:
+		limit = clk->base_limit.pstate;
+		if (limit)
+			break;
+	case NVKM_CLK_BOOST_BIOS:
+		limit = clk->boost_limit.pstate;
+		break;
+	default:
+		limit = 0;
+		break;
+	}
+
+	if (!limit)
+		return start;
+
+	for (pstate = start; &pstate->head != &clk->states;
+	     pstate = list_entry(pstate->head.prev, typeof(*pstate), head)) {
+		if (limit >= pstate->pstate)
+			break;
+	}
+	return pstate;
+}
+
+static struct nvkm_pstate *
+nvkm_pstate_get(struct nvkm_clk *clk, int pstateid)
+{
+	struct nvkm_pstate *pstate;
+	switch (pstateid) {
+	case NVKM_CLK_PSTATE_DEFAULT:
+		return NULL;
+	default:
+		list_for_each_entry(pstate, &clk->states, head) {
+			if (pstate->pstate == pstateid)
+				return pstate;
+		}
+	}
+	return NULL;
+}
+
 static int
 nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
 {
@@ -300,20 +346,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
 	if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
 		return 0;
 
-	if (clk->pstate && clk->pstate->pstate == pstateid) {
-		pstate = clk->pstate;
-		goto cstate;
-	}
-
-	list_for_each_entry(pstate, &clk->states, head) {
-		if (pstate->pstate == pstateid)
-			break;
-	}
+	pstate = nvkm_pstate_get(clk, pstateid);
+	pstate = nvkm_pstate_find_best(clk, pstate);
 
 	if (!pstate)
 		return -EINVAL;
 
-	nvkm_debug(subdev, "setting performance state %x\n", pstateid);
+	if (clk->pstate && clk->pstate == pstate)
+		goto cstate;
+
+	nvkm_debug(subdev, "setting performance state %x\n", pstate->pstate);
 	clk->pstate = pstate;
 
 	nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 28/29] clk: move the switch out of the loop in nvkm_cstate_valid

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/nvkm/subdev/clk/base.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 2a51c078..994d32b9 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -83,28 +83,30 @@ nvkm_cstate_valid(struct nvkm_clk *clk, struct nvkm_cstate *cstate,
 	const struct nvkm_domain *domain = clk->domains;
 	struct nvkm_volt *volt = clk->subdev.device->volt;
 	int voltage;
+	u32 limit;
 
-	while (domain && domain->name != nv_clk_src_max) {
-		if (domain->flags & NVKM_CLK_DOM_FLAG_VPSTATE) {
-			u32 freq = cstate->domain[domain->name];
-			u32 limit;
-			switch (clk->boost_mode) {
-			case NVKM_CLK_BOOST_NONE:
-				limit = clk->base_limit.max_khz;
-				if (limit)
-					break;
-			case NVKM_CLK_BOOST_BIOS:
-				limit = clk->boost_limit.max_khz;
-				break;
-			default:
-				limit = 0;
-				break;
-			}
+	switch (clk->boost_mode) {
+	case NVKM_CLK_BOOST_NONE:
+		limit = clk->base_limit.max_khz;
+		if (limit)
+			break;
+	case NVKM_CLK_BOOST_BIOS:
+		limit = clk->boost_limit.max_khz;
+		break;
+	default:
+		limit = 0;
+		break;
+	}
 
+	if (limit) {
+		for (; domain && domain->name != nv_clk_src_max; domain++) {
+			u32 freq;
+			if (!(domain->flags & NVKM_CLK_DOM_FLAG_VPSTATE))
+				continue;
+			freq = cstate->domain[domain->name];
 			if (limit && freq > limit)
 				return false;
 		}
-		domain++;
 	}
 
 	if (!volt)
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[RFC PATCH 29/29] clk: limit clocks on battery

[Karol Herbst]

Signed-off-by: Karol Herbst <karolherbst@gmail.com>
---
 drm/nouveau/include/nvkm/subdev/clk.h |  1 +
 drm/nouveau/nvkm/subdev/clk/base.c    | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
index 70505c9b..fae45e6e 100644
--- a/drm/nouveau/include/nvkm/subdev/clk.h
+++ b/drm/nouveau/include/nvkm/subdev/clk.h
@@ -120,6 +120,7 @@ struct nvkm_clk {
 	u8  boost_mode;
 	struct nvkm_clk_limit base_limit;
 	struct nvkm_clk_limit boost_limit;
+	struct nvkm_clk_limit batt_limit;
 	u32 max_khz;
 
 	/*XXX: die, these are here *only* to support the completely
diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
index 994d32b9..0de5b3dc 100644
--- a/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drm/nouveau/nvkm/subdev/clk/base.c
@@ -98,6 +98,14 @@ nvkm_cstate_valid(struct nvkm_clk *clk, struct nvkm_cstate *cstate,
 		break;
 	}
 
+	if (!clk->pwrsrc) {
+		u32 blimit = clk->batt_limit.max_khz;
+		if (!limit)
+			limit = blimit;
+		else if (blimit)
+			limit = min(blimit, limit);
+	}
+
 	if (limit) {
 		for (; domain && domain->name != nv_clk_src_max; domain++) {
 			u32 freq;
@@ -309,6 +317,14 @@ nvkm_pstate_find_best(struct nvkm_clk *clk, struct nvkm_pstate *start)
 		break;
 	}
 
+	if (!clk->pwrsrc) {
+		u8 blimit = clk->batt_limit.pstate;
+		if (!limit)
+			limit = blimit;
+		else if (blimit)
+			limit = min(blimit, limit);
+	}
+
 	if (!limit)
 		return start;
 
@@ -807,6 +823,8 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
 			nvkm_clk_fill_limit(&clk->boost_limit, &vpe);
 		if (!nvbios_vpstate_entry(bios, &h, h.base_id, &vpe))
 			nvkm_clk_fill_limit(&clk->base_limit, &vpe);
+		if (!nvbios_vpstate_entry(bios, &h, h.battery_id, &vpe))
+			nvkm_clk_fill_limit(&clk->batt_limit, &vpe);
 	}
 
 	clk->func = func;
-- 
2.14.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 01/29] therm: split return code and value in nvkm_get_temp

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 14691 bytes --]

As you changed the return value of `temp_get()` to solely be the error code, or
absence of an error, I would change all those tests that checked whether the
returned value was strictly less, or greater than, 0 to now only compare
against 0 (no error). For example,

  if (therm && therm->attr_get && nvkm_therm_temp_get(therm, &val) < 0)
  if (therm->func->temp_get(therm, &val) >= 0)

could become

  if (therm && therm->attr_get && nvkm_therm_temp_get(therm, &val))
  if (!therm->func->temp_get(therm, &val))

to match other error checking code.

More comments below

On 2017-09-15 — 17:11, Karol Herbst wrote:
> The current hwmon code doesn't check if the returned value was actually an
> error.
> 
> Since Kepler temperature sensors are able to report negative values.

I assume those negative values are not for error reporting, but more if you
buried your GPU in the snow somewhere in the Antarctic and still want a valid
temperature to be reported.

> Since Pascal (and maybe earlier) we have sensors with improved precision.
> 
> Adjust the nvkm_get_temp method to be able to deal with those changes
> and let hwmon return an error properly.
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drm/nouveau/include/nvkm/subdev/therm.h |  2 +-
>  drm/nouveau/nouveau_hwmon.c             | 15 +++++++++------
>  drm/nouveau/nvkm/subdev/therm/base.c    | 19 ++++++++++++++-----
>  drm/nouveau/nvkm/subdev/therm/g84.c     | 11 ++++++-----
>  drm/nouveau/nvkm/subdev/therm/gp100.c   |  9 +++++----
>  drm/nouveau/nvkm/subdev/therm/nv40.c    |  9 +++------
>  drm/nouveau/nvkm/subdev/therm/nv50.c    |  9 +++------
>  drm/nouveau/nvkm/subdev/therm/priv.h    |  4 ++--
>  drm/nouveau/nvkm/subdev/therm/temp.c    | 16 ++++++++++++----
>  9 files changed, 55 insertions(+), 39 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/therm.h b/drm/nouveau/include/nvkm/subdev/therm.h
> index 9841f076..8c84017f 100644
> --- a/drm/nouveau/include/nvkm/subdev/therm.h
> +++ b/drm/nouveau/include/nvkm/subdev/therm.h
> @@ -86,7 +86,7 @@ struct nvkm_therm {
>  	int (*attr_set)(struct nvkm_therm *, enum nvkm_therm_attr_type, int);
>  };
>  
> -int nvkm_therm_temp_get(struct nvkm_therm *);
> +int nvkm_therm_temp_get(struct nvkm_therm *, int *);
>  int nvkm_therm_fan_sense(struct nvkm_therm *);
>  int nvkm_therm_cstate(struct nvkm_therm *, int, int);
>  
> diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
> index 7c965648..f1914d9a 100644
> --- a/drm/nouveau/nouveau_hwmon.c
> +++ b/drm/nouveau/nouveau_hwmon.c
> @@ -326,8 +326,9 @@ nouveau_temp_is_visible(const void *data, u32 attr, int channel)
>  {
>  	struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>  	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +	int val;
>  
> -	if (therm && therm->attr_get && nvkm_therm_temp_get(therm) < 0)
> +	if (therm && therm->attr_get && nvkm_therm_temp_get(therm, &val) < 0)
>  		return 0;
>  
>  	switch (attr) {
> @@ -421,15 +422,16 @@ nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct nouveau_drm *drm = nouveau_drm(drm_dev);
>  	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> -	int ret;
> +	int ret = 0;
> +	int temp;
>  
>  	if (!therm || !therm->attr_get)
>  		return -EOPNOTSUPP;
>  
>  	switch (attr) {
>  	case hwmon_temp_input:
> -		ret = nvkm_therm_temp_get(therm);
> -		*val = ret < 0 ? ret : (ret * 1000);
> +		ret = nvkm_therm_temp_get(therm, &temp);
> +		*val = temp * 1000;
>  		break;
>  	case hwmon_temp_max:
>  		*val = therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK)
> @@ -459,7 +461,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)
>  		return -EOPNOTSUPP;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int
> @@ -713,6 +715,7 @@ nouveau_hwmon_init(struct drm_device *dev)
>  	struct device *hwmon_dev;
>  	int ret = 0;
>  	int i = 0;
> +	int val;
>  
>  	hwmon = drm->hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
>  	if (!hwmon)
> @@ -720,7 +723,7 @@ nouveau_hwmon_init(struct drm_device *dev)
>  	hwmon->dev = dev;
>  
>  	if (therm && therm->attr_get && therm->attr_set) {
> -		if (nvkm_therm_temp_get(therm) >= 0)
> +		if (nvkm_therm_temp_get(therm, &val) >= 0)
>  			special_groups[i++] = &temp1_auto_point_sensor_group;
>  		if (therm->fan_get && therm->fan_get(therm) >= 0)
>  			special_groups[i++] = &pwm_fan_sensor_group;
> diff --git a/drm/nouveau/nvkm/subdev/therm/base.c b/drm/nouveau/nvkm/subdev/therm/base.c
> index f27fc6d0..8fa691fb 100644
> --- a/drm/nouveau/nvkm/subdev/therm/base.c
> +++ b/drm/nouveau/nvkm/subdev/therm/base.c
> @@ -24,22 +24,26 @@
>  #include "priv.h"
>  
>  int
> -nvkm_therm_temp_get(struct nvkm_therm *therm)
> +nvkm_therm_temp_get(struct nvkm_therm *therm, int *val)
>  {
>  	if (therm->func->temp_get)
> -		return therm->func->temp_get(therm);
> +		return therm->func->temp_get(therm, val);
>  	return -ENODEV;
>  }
>  
>  static int
>  nvkm_therm_update_trip(struct nvkm_therm *therm)
>  {
> +	int temp, ret;
>  	struct nvbios_therm_trip_point *trip = therm->fan->bios.trip,
>  				       *cur_trip = NULL,
>  				       *last_trip = therm->last_trip;
> -	u8  temp = therm->func->temp_get(therm);
>  	u16 duty, i;
>  
> +	ret = therm->func->temp_get(therm, &temp);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* look for the trip point corresponding to the current temperature */
>  	cur_trip = NULL;
>  	for (i = 0; i < therm->fan->bios.nr_fan_trip; i++) {
> @@ -67,9 +71,13 @@ static int
>  nvkm_therm_compute_linear_duty(struct nvkm_therm *therm, u8 linear_min_temp,
>                                 u8 linear_max_temp)
>  {
> -	u8  temp = therm->func->temp_get(therm);
> +	int temp, ret;
>  	u16 duty;
>  
> +	ret = therm->func->temp_get(therm, &temp);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* handle the non-linear part first */
>  	if (temp < linear_min_temp)
>  		return therm->fan->bios.min_duty;
> @@ -182,6 +190,7 @@ nvkm_therm_fan_mode(struct nvkm_therm *therm, int mode)
>  {
>  	struct nvkm_subdev *subdev = &therm->subdev;
>  	struct nvkm_device *device = subdev->device;
> +	int val;
>  	static const char *name[] = {
>  		"disabled",
>  		"manual",
> @@ -197,7 +206,7 @@ nvkm_therm_fan_mode(struct nvkm_therm *therm, int mode)
>  	/* do not allow automatic fan management if the thermal sensor is
>  	 * not available */
>  	if (mode == NVKM_THERM_CTRL_AUTO &&
> -	    therm->func->temp_get(therm) < 0)
> +	    therm->func->temp_get(therm, &val) < 0)
>  		return -EINVAL;
>  
>  	if (therm->mode == mode)
> diff --git a/drm/nouveau/nvkm/subdev/therm/g84.c b/drm/nouveau/nvkm/subdev/therm/g84.c
> index 96f8da40..3bb2d829 100644
> --- a/drm/nouveau/nvkm/subdev/therm/g84.c
> +++ b/drm/nouveau/nvkm/subdev/therm/g84.c
> @@ -27,14 +27,15 @@
>  #include <subdev/fuse.h>
>  
>  int
> -g84_temp_get(struct nvkm_therm *therm)
> +g84_temp_get(struct nvkm_therm *therm, int *val)
>  {
>  	struct nvkm_device *device = therm->subdev.device;
>  
> -	if (nvkm_fuse_read(device->fuse, 0x1a8) == 1)
> -		return nvkm_rd32(device, 0x20400);
> -	else
> +	if (nvkm_fuse_read(device->fuse, 0x1a8) != 1)
>  		return -ENODEV;
> +
> +	*val = nvkm_rd32(device, 0x20400);

You could allow passing NULL values for `val` for when you are not interested
by the results, as you did in some places further up.
I would probably check for NULL values for `val`, and for `therm` though it
seems to be an implicit rule that the caller should be the one doing the check.

> +	return 0;
>  }
>  
>  void
> @@ -115,7 +116,7 @@ g84_therm_threshold_hyst_emulation(struct nvkm_therm *therm,
>  	}
>  
>  	/* fix the state (in case someone reprogrammed the alarms) */
> -	cur = therm->func->temp_get(therm);
> +	therm->func->temp_get(therm, &cur);

Should there be a check for the return value of the `temp_get()` function here,
and error out if an error code was returned? In any case, you are the behaviour
here, as if `temp_get()` previously returned an error code, the if-statement
would always be false, whereas now, it’s undecided as `cur` will not even be
initialised in that case!

I am not sure whether it should be the caller of `temp_get()` or `temp_get()`
itself which should initialise a default value for the `val` argument (probably
the latter).

>  	if (new_state == NVKM_THERM_THRS_LOWER && cur > thrs->temp)
>  		new_state = NVKM_THERM_THRS_HIGHER;
>  	else if (new_state == NVKM_THERM_THRS_HIGHER &&
> diff --git a/drm/nouveau/nvkm/subdev/therm/gp100.c b/drm/nouveau/nvkm/subdev/therm/gp100.c
> index 9f0dea3f..d8206748 100644
> --- a/drm/nouveau/nvkm/subdev/therm/gp100.c
> +++ b/drm/nouveau/nvkm/subdev/therm/gp100.c
> @@ -24,7 +24,7 @@
>  #include "priv.h"
>  
>  static int
> -gp100_temp_get(struct nvkm_therm *therm)
> +gp100_temp_get(struct nvkm_therm *therm, int *val)
>  {
>  	struct nvkm_device *device = therm->subdev.device;
>  	struct nvkm_subdev *subdev = &therm->subdev;
> @@ -36,9 +36,10 @@ gp100_temp_get(struct nvkm_therm *therm)
>  		nvkm_trace(subdev, "reading temperature from SHADOWed sensor\n");
>  
>  	/* device valid */
> -	if (tsensor & 0x20000000)
> -		return (inttemp >> 8);
> -	else
> +	if (tsensor & 0x20000000) {
> +		*val = inttemp >> 8;
> +		return 0;
> +	} else
>  		return -ENODEV;
>  }
>  
> diff --git a/drm/nouveau/nvkm/subdev/therm/nv40.c b/drm/nouveau/nvkm/subdev/therm/nv40.c
> index 2c92ffb5..cfd5b215 100644
> --- a/drm/nouveau/nvkm/subdev/therm/nv40.c
> +++ b/drm/nouveau/nvkm/subdev/therm/nv40.c
> @@ -70,7 +70,7 @@ nv40_sensor_setup(struct nvkm_therm *therm)
>  }
>  
>  static int
> -nv40_temp_get(struct nvkm_therm *therm)
> +nv40_temp_get(struct nvkm_therm *therm, int *val)
>  {
>  	struct nvkm_device *device = therm->subdev.device;
>  	struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
> @@ -95,11 +95,8 @@ nv40_temp_get(struct nvkm_therm *therm)
>  	core_temp = core_temp + sensor->offset_num / sensor->offset_den;
>  	core_temp = core_temp + sensor->offset_constant - 8;
>  
> -	/* reserve negative temperatures for errors */
> -	if (core_temp < 0)
> -		core_temp = 0;
> -
> -	return core_temp;
> +	*val = core_temp;
> +	return 0;
>  }
>  
>  static int
> diff --git a/drm/nouveau/nvkm/subdev/therm/nv50.c b/drm/nouveau/nvkm/subdev/therm/nv50.c
> index 9b57b433..62ec4063 100644
> --- a/drm/nouveau/nvkm/subdev/therm/nv50.c
> +++ b/drm/nouveau/nvkm/subdev/therm/nv50.c
> @@ -126,7 +126,7 @@ nv50_sensor_setup(struct nvkm_therm *therm)
>  }
>  
>  static int
> -nv50_temp_get(struct nvkm_therm *therm)
> +nv50_temp_get(struct nvkm_therm *therm, int *val)
>  {
>  	struct nvkm_device *device = therm->subdev.device;
>  	struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
> @@ -143,11 +143,8 @@ nv50_temp_get(struct nvkm_therm *therm)
>  	core_temp = core_temp + sensor->offset_num / sensor->offset_den;
>  	core_temp = core_temp + sensor->offset_constant - 8;
>  
> -	/* reserve negative temperatures for errors */
> -	if (core_temp < 0)
> -		core_temp = 0;
> -
> -	return core_temp;
> +	*val = core_temp;
> +	return 0;
>  }
>  
>  static void
> diff --git a/drm/nouveau/nvkm/subdev/therm/priv.h b/drm/nouveau/nvkm/subdev/therm/priv.h
> index 1f46e371..b325ec5f 100644
> --- a/drm/nouveau/nvkm/subdev/therm/priv.h
> +++ b/drm/nouveau/nvkm/subdev/therm/priv.h
> @@ -91,7 +91,7 @@ struct nvkm_therm_func {
>  	int (*pwm_set)(struct nvkm_therm *, int line, u32, u32);
>  	int (*pwm_clock)(struct nvkm_therm *, int line);
>  
> -	int (*temp_get)(struct nvkm_therm *);
> +	int (*temp_get)(struct nvkm_therm *, int *);
>  
>  	int (*fan_sense)(struct nvkm_therm *);
>  
> @@ -105,7 +105,7 @@ int  nv50_fan_pwm_get(struct nvkm_therm *, int, u32 *, u32 *);
>  int  nv50_fan_pwm_set(struct nvkm_therm *, int, u32, u32);
>  int  nv50_fan_pwm_clock(struct nvkm_therm *, int);
>  
> -int  g84_temp_get(struct nvkm_therm *);
> +int  g84_temp_get(struct nvkm_therm *, int *);
>  void g84_sensor_setup(struct nvkm_therm *);
>  void g84_therm_fini(struct nvkm_therm *);
>  
> diff --git a/drm/nouveau/nvkm/subdev/therm/temp.c b/drm/nouveau/nvkm/subdev/therm/temp.c
> index ddb2b2c6..5ec2dfb3 100644
> --- a/drm/nouveau/nvkm/subdev/therm/temp.c
> +++ b/drm/nouveau/nvkm/subdev/therm/temp.c
> @@ -86,7 +86,10 @@ nvkm_therm_sensor_event(struct nvkm_therm *therm, enum nvkm_therm_thrs thrs,
>  	static const char * const thresholds[] = {
>  		"fanboost", "downclock", "critical", "shutdown"
>  	};
> -	int temperature = therm->func->temp_get(therm);
> +	int temperature;
> +
> +	if (therm->func->temp_get(therm, &temperature) < 0)
> +		return;
>  
>  	if (thrs < 0 || thrs > 3)
>  		return;
> @@ -140,7 +143,10 @@ nvkm_therm_threshold_hyst_polling(struct nvkm_therm *therm,
>  {
>  	enum nvkm_therm_thrs_direction direction;
>  	enum nvkm_therm_thrs_state prev_state, new_state;
> -	int temp = therm->func->temp_get(therm);
> +	int temp;
> +
> +	if (therm->func->temp_get(therm, &temp) < 0)
> +		return;
>  
>  	prev_state = nvkm_therm_sensor_get_threshold_state(therm, thrs_name);
>  
> @@ -166,6 +172,7 @@ alarm_timer_callback(struct nvkm_alarm *alarm)
>  	struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
>  	struct nvkm_timer *tmr = therm->subdev.device->timer;
>  	unsigned long flags;
> +	int val;
>  
>  	spin_lock_irqsave(&therm->sensor.alarm_program_lock, flags);
>  
> @@ -185,7 +192,7 @@ alarm_timer_callback(struct nvkm_alarm *alarm)
>  	spin_unlock_irqrestore(&therm->sensor.alarm_program_lock, flags);
>  
>  	/* schedule the next poll in one second */
> -	if (therm->func->temp_get(therm) >= 0)
> +	if (therm->func->temp_get(therm, &val) >= 0)
>  		nvkm_timer_alarm(tmr, 1000000000ULL, alarm);
>  }
>  
> @@ -227,9 +234,10 @@ nvkm_therm_sensor_fini(struct nvkm_therm *therm, bool suspend)
>  void
>  nvkm_therm_sensor_preinit(struct nvkm_therm *therm)
>  {
> +	int val;
>  	const char *sensor_avail = "yes";
>  
> -	if (therm->func->temp_get(therm) < 0)
> +	if (therm->func->temp_get(therm, &val) < 0)
>  		sensor_avail = "no";
>  
>  	nvkm_debug(&therm->subdev, "internal sensor: %s\n", sensor_avail);
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 02/29] hwmon: properly check for errors

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 3978 bytes --]

Reviewed-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

On 2017-09-15 — 17:11, Karol Herbst wrote:
> Otherwise hwmon interprets error codes as real values.
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drm/nouveau/nouveau_hwmon.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
> index f1914d9a..0d75c14d 100644
> --- a/drm/nouveau/nouveau_hwmon.c
> +++ b/drm/nouveau/nouveau_hwmon.c
> @@ -470,18 +470,23 @@ nouveau_fan_read(struct device *dev, u32 attr, int channel, long *val)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct nouveau_drm *drm = nouveau_drm(drm_dev);
>  	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +	int ret;
>  
>  	if (!therm)
>  		return -EOPNOTSUPP;
>  
>  	switch (attr) {
>  	case hwmon_fan_input:
> -		*val = nvkm_therm_fan_sense(therm);
> +		ret = nvkm_therm_fan_sense(therm);
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
>  	return 0;
>  }
>  
> @@ -491,7 +496,7 @@ nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct nouveau_drm *drm = nouveau_drm(drm_dev);
>  	struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
> -	int ret;
> +	int ret = 0;
>  
>  	if (!volt)
>  		return -EOPNOTSUPP;
> @@ -499,7 +504,8 @@ nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
>  	switch (attr) {
>  	case hwmon_in_input:
>  		ret = nvkm_volt_get(volt);
> -		*val = ret < 0 ? ret : (ret / 1000);
> +		if (ret >= 0)
> +			*val = ret / 1000;
>  		break;
>  	case hwmon_in_min:
>  		*val = volt->min_uv > 0 ? (volt->min_uv / 1000) : -ENODEV;
> @@ -511,7 +517,7 @@ nouveau_in_read(struct device *dev, u32 attr, int channel, long *val)
>  		return -EOPNOTSUPP;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int
> @@ -520,21 +526,26 @@ nouveau_pwm_read(struct device *dev, u32 attr, int channel, long *val)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct nouveau_drm *drm = nouveau_drm(drm_dev);
>  	struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
> +	int ret;
>  
>  	if (!therm || !therm->attr_get || !therm->fan_get)
>  		return -EOPNOTSUPP;
>  
>  	switch (attr) {
>  	case hwmon_pwm_enable:
> -		*val = therm->attr_get(therm, NVKM_THERM_ATTR_FAN_MODE);
> +		ret = therm->attr_get(therm, NVKM_THERM_ATTR_FAN_MODE);
>  		break;
>  	case hwmon_pwm_input:
> -		*val = therm->fan_get(therm);
> +		ret = therm->fan_get(therm);
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
>  	return 0;
>  }
>  
> @@ -544,18 +555,26 @@ nouveau_power_read(struct device *dev, u32 attr, int channel, long *val)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct nouveau_drm *drm = nouveau_drm(drm_dev);
>  	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
> +	int ret;
>  
>  	if (!iccsense)
>  		return -EOPNOTSUPP;
>  
>  	switch (attr) {
>  	case hwmon_power_input:
> -		*val = nvkm_iccsense_read_all(iccsense);
> +		ret = nvkm_iccsense_read_all(iccsense);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
>  		break;
>  	case hwmon_power_max:
> +		if (iccsense->power_w_max <= 0)
> +			return -ENODEV;
>  		*val = iccsense->power_w_max;
>  		break;
>  	case hwmon_power_crit:
> +		if (iccsense->power_w_crit <= 0)
> +			return -ENODEV;
>  		*val = iccsense->power_w_crit;
>  		break;
>  	default:
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 03/29] subdev/volt/gk104: return error when read fails

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 1487 bytes --]

On 2017-09-15 — 17:11, Karol Herbst wrote:
> While my gpu was powered off, hwmon returned 0.6V as the current voltage.
> If nvkm_rd32 fails for any reason, return the error.
> 
> With that sensors will display a "N/A" instead of 0.6V.

Small nitpick, add a comma between “that” and “sensors”.
Otherwise,

Reviewed-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drm/nouveau/nvkm/subdev/volt/gk104.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drm/nouveau/nvkm/subdev/volt/gk104.c b/drm/nouveau/nvkm/subdev/volt/gk104.c
> index 1c744e02..53a7af9d 100644
> --- a/drm/nouveau/nvkm/subdev/volt/gk104.c
> +++ b/drm/nouveau/nvkm/subdev/volt/gk104.c
> @@ -40,10 +40,15 @@ gk104_volt_get(struct nvkm_volt *base)
>  {
>  	struct nvbios_volt *bios = &gk104_volt(base)->bios;
>  	struct nvkm_device *device = base->subdev.device;
> -	u32 div, duty;
> +	int div, duty;
>  
>  	div  = nvkm_rd32(device, 0x20340);
> +	if (div < 0)
> +		return div;
> +
>  	duty = nvkm_rd32(device, 0x20344);
> +	if (duty < 0)
> +		return duty;
>  
>  	return bios->base + bios->pwm_range * duty / div;
>  }
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 01/29] therm: split return code and value in nvkm_get_temp

[Karol Herbst]

On Sun, Oct 8, 2017 at 12:16 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> As you changed the return value of `temp_get()` to solely be the error code, or
> absence of an error, I would change all those tests that checked whether the
> returned value was strictly less, or greater than, 0 to now only compare
> against 0 (no error). For example,
>
>   if (therm && therm->attr_get && nvkm_therm_temp_get(therm, &val) < 0)
>   if (therm->func->temp_get(therm, &val) >= 0)
>
> could become
>
>   if (therm && therm->attr_get && nvkm_therm_temp_get(therm, &val))
>   if (!therm->func->temp_get(therm, &val))
>

right, makes sense.

> to match other error checking code.
>
> More comments below
>
> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> The current hwmon code doesn't check if the returned value was actually an
>> error.
>>
>> Since Kepler temperature sensors are able to report negative values.
>
> I assume those negative values are not for error reporting, but more if you
> buried your GPU in the snow somewhere in the Antarctic and still want a valid
> temperature to be reported.

well, nobody ever tested that, but at least on Kepler technically we
have two new registers related to therm, one offset and one value. If
the offset is 0x60, value needs to be substracted by that offset to
get the real temperature. No idea if that works for real though, but
why would they add it anyhow if not?

>
>> Since Pascal (and maybe earlier) we have sensors with improved precision.
>>
>> Adjust the nvkm_get_temp method to be able to deal with those changes
>> and let hwmon return an error properly.
>>
>> Signed-off-by: Karol Herbst <karolherbst@gmail.com>
>> ---
>>  drm/nouveau/include/nvkm/subdev/therm.h |  2 +-
>>  drm/nouveau/nouveau_hwmon.c             | 15 +++++++++------
>>  drm/nouveau/nvkm/subdev/therm/base.c    | 19 ++++++++++++++-----
>>  drm/nouveau/nvkm/subdev/therm/g84.c     | 11 ++++++-----
>>  drm/nouveau/nvkm/subdev/therm/gp100.c   |  9 +++++----
>>  drm/nouveau/nvkm/subdev/therm/nv40.c    |  9 +++------
>>  drm/nouveau/nvkm/subdev/therm/nv50.c    |  9 +++------
>>  drm/nouveau/nvkm/subdev/therm/priv.h    |  4 ++--
>>  drm/nouveau/nvkm/subdev/therm/temp.c    | 16 ++++++++++++----
>>  9 files changed, 55 insertions(+), 39 deletions(-)
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/therm.h b/drm/nouveau/include/nvkm/subdev/therm.h
>> index 9841f076..8c84017f 100644
>> --- a/drm/nouveau/include/nvkm/subdev/therm.h
>> +++ b/drm/nouveau/include/nvkm/subdev/therm.h
>> @@ -86,7 +86,7 @@ struct nvkm_therm {
>>       int (*attr_set)(struct nvkm_therm *, enum nvkm_therm_attr_type, int);
>>  };
>>
>> -int nvkm_therm_temp_get(struct nvkm_therm *);
>> +int nvkm_therm_temp_get(struct nvkm_therm *, int *);
>>  int nvkm_therm_fan_sense(struct nvkm_therm *);
>>  int nvkm_therm_cstate(struct nvkm_therm *, int, int);
>>
>> diff --git a/drm/nouveau/nouveau_hwmon.c b/drm/nouveau/nouveau_hwmon.c
>> index 7c965648..f1914d9a 100644
>> --- a/drm/nouveau/nouveau_hwmon.c
>> +++ b/drm/nouveau/nouveau_hwmon.c
>> @@ -326,8 +326,9 @@ nouveau_temp_is_visible(const void *data, u32 attr, int channel)
>>  {
>>       struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>>       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> +     int val;
>>
>> -     if (therm && therm->attr_get && nvkm_therm_temp_get(therm) < 0)
>> +     if (therm && therm->attr_get && nvkm_therm_temp_get(therm, &val) < 0)
>>               return 0;
>>
>>       switch (attr) {
>> @@ -421,15 +422,16 @@ nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)
>>       struct drm_device *drm_dev = dev_get_drvdata(dev);
>>       struct nouveau_drm *drm = nouveau_drm(drm_dev);
>>       struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> -     int ret;
>> +     int ret = 0;
>> +     int temp;
>>
>>       if (!therm || !therm->attr_get)
>>               return -EOPNOTSUPP;
>>
>>       switch (attr) {
>>       case hwmon_temp_input:
>> -             ret = nvkm_therm_temp_get(therm);
>> -             *val = ret < 0 ? ret : (ret * 1000);
>> +             ret = nvkm_therm_temp_get(therm, &temp);
>> +             *val = temp * 1000;
>>               break;
>>       case hwmon_temp_max:
>>               *val = therm->attr_get(therm, NVKM_THERM_ATTR_THRS_DOWN_CLK)
>> @@ -459,7 +461,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)
>>               return -EOPNOTSUPP;
>>       }
>>
>> -     return 0;
>> +     return ret;
>>  }
>>
>>  static int
>> @@ -713,6 +715,7 @@ nouveau_hwmon_init(struct drm_device *dev)
>>       struct device *hwmon_dev;
>>       int ret = 0;
>>       int i = 0;
>> +     int val;
>>
>>       hwmon = drm->hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
>>       if (!hwmon)
>> @@ -720,7 +723,7 @@ nouveau_hwmon_init(struct drm_device *dev)
>>       hwmon->dev = dev;
>>
>>       if (therm && therm->attr_get && therm->attr_set) {
>> -             if (nvkm_therm_temp_get(therm) >= 0)
>> +             if (nvkm_therm_temp_get(therm, &val) >= 0)
>>                       special_groups[i++] = &temp1_auto_point_sensor_group;
>>               if (therm->fan_get && therm->fan_get(therm) >= 0)
>>                       special_groups[i++] = &pwm_fan_sensor_group;
>> diff --git a/drm/nouveau/nvkm/subdev/therm/base.c b/drm/nouveau/nvkm/subdev/therm/base.c
>> index f27fc6d0..8fa691fb 100644
>> --- a/drm/nouveau/nvkm/subdev/therm/base.c
>> +++ b/drm/nouveau/nvkm/subdev/therm/base.c
>> @@ -24,22 +24,26 @@
>>  #include "priv.h"
>>
>>  int
>> -nvkm_therm_temp_get(struct nvkm_therm *therm)
>> +nvkm_therm_temp_get(struct nvkm_therm *therm, int *val)
>>  {
>>       if (therm->func->temp_get)
>> -             return therm->func->temp_get(therm);
>> +             return therm->func->temp_get(therm, val);
>>       return -ENODEV;
>>  }
>>
>>  static int
>>  nvkm_therm_update_trip(struct nvkm_therm *therm)
>>  {
>> +     int temp, ret;
>>       struct nvbios_therm_trip_point *trip = therm->fan->bios.trip,
>>                                      *cur_trip = NULL,
>>                                      *last_trip = therm->last_trip;
>> -     u8  temp = therm->func->temp_get(therm);
>>       u16 duty, i;
>>
>> +     ret = therm->func->temp_get(therm, &temp);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       /* look for the trip point corresponding to the current temperature */
>>       cur_trip = NULL;
>>       for (i = 0; i < therm->fan->bios.nr_fan_trip; i++) {
>> @@ -67,9 +71,13 @@ static int
>>  nvkm_therm_compute_linear_duty(struct nvkm_therm *therm, u8 linear_min_temp,
>>                                 u8 linear_max_temp)
>>  {
>> -     u8  temp = therm->func->temp_get(therm);
>> +     int temp, ret;
>>       u16 duty;
>>
>> +     ret = therm->func->temp_get(therm, &temp);
>> +     if (ret < 0)
>> +             return ret;
>> +
>>       /* handle the non-linear part first */
>>       if (temp < linear_min_temp)
>>               return therm->fan->bios.min_duty;
>> @@ -182,6 +190,7 @@ nvkm_therm_fan_mode(struct nvkm_therm *therm, int mode)
>>  {
>>       struct nvkm_subdev *subdev = &therm->subdev;
>>       struct nvkm_device *device = subdev->device;
>> +     int val;
>>       static const char *name[] = {
>>               "disabled",
>>               "manual",
>> @@ -197,7 +206,7 @@ nvkm_therm_fan_mode(struct nvkm_therm *therm, int mode)
>>       /* do not allow automatic fan management if the thermal sensor is
>>        * not available */
>>       if (mode == NVKM_THERM_CTRL_AUTO &&
>> -         therm->func->temp_get(therm) < 0)
>> +         therm->func->temp_get(therm, &val) < 0)
>>               return -EINVAL;
>>
>>       if (therm->mode == mode)
>> diff --git a/drm/nouveau/nvkm/subdev/therm/g84.c b/drm/nouveau/nvkm/subdev/therm/g84.c
>> index 96f8da40..3bb2d829 100644
>> --- a/drm/nouveau/nvkm/subdev/therm/g84.c
>> +++ b/drm/nouveau/nvkm/subdev/therm/g84.c
>> @@ -27,14 +27,15 @@
>>  #include <subdev/fuse.h>
>>
>>  int
>> -g84_temp_get(struct nvkm_therm *therm)
>> +g84_temp_get(struct nvkm_therm *therm, int *val)
>>  {
>>       struct nvkm_device *device = therm->subdev.device;
>>
>> -     if (nvkm_fuse_read(device->fuse, 0x1a8) == 1)
>> -             return nvkm_rd32(device, 0x20400);
>> -     else
>> +     if (nvkm_fuse_read(device->fuse, 0x1a8) != 1)
>>               return -ENODEV;
>> +
>> +     *val = nvkm_rd32(device, 0x20400);
>
> You could allow passing NULL values for `val` for when you are not interested
> by the results, as you did in some places further up.
> I would probably check for NULL values for `val`, and for `therm` though it
> seems to be an implicit rule that the caller should be the one doing the check.

well, I think we should do all NULL checks inside the "public" subdev
functions. g84_temp_get is internal to the subdev, so we can assume
therm never to be NULL.
Will go through those and see where some are missing related to my changes.

>
>> +     return 0;
>>  }
>>
>>  void
>> @@ -115,7 +116,7 @@ g84_therm_threshold_hyst_emulation(struct nvkm_therm *therm,
>>       }
>>
>>       /* fix the state (in case someone reprogrammed the alarms) */
>> -     cur = therm->func->temp_get(therm);
>> +     therm->func->temp_get(therm, &cur);
>
> Should there be a check for the return value of the `temp_get()` function here,
> and error out if an error code was returned? In any case, you are the behaviour
> here, as if `temp_get()` previously returned an error code, the if-statement
> would always be false, whereas now, it’s undecided as `cur` will not even be
> initialised in that case!
>
> I am not sure whether it should be the caller of `temp_get()` or `temp_get()`
> itself which should initialise a default value for the `val` argument (probably
> the latter).

I think error checking is the right way to go here, then we don't need
a default value. Values in an error case are usually not trustworthy
anyhow.

>
>>       if (new_state == NVKM_THERM_THRS_LOWER && cur > thrs->temp)
>>               new_state = NVKM_THERM_THRS_HIGHER;
>>       else if (new_state == NVKM_THERM_THRS_HIGHER &&
>> diff --git a/drm/nouveau/nvkm/subdev/therm/gp100.c b/drm/nouveau/nvkm/subdev/therm/gp100.c
>> index 9f0dea3f..d8206748 100644
>> --- a/drm/nouveau/nvkm/subdev/therm/gp100.c
>> +++ b/drm/nouveau/nvkm/subdev/therm/gp100.c
>> @@ -24,7 +24,7 @@
>>  #include "priv.h"
>>
>>  static int
>> -gp100_temp_get(struct nvkm_therm *therm)
>> +gp100_temp_get(struct nvkm_therm *therm, int *val)
>>  {
>>       struct nvkm_device *device = therm->subdev.device;
>>       struct nvkm_subdev *subdev = &therm->subdev;
>> @@ -36,9 +36,10 @@ gp100_temp_get(struct nvkm_therm *therm)
>>               nvkm_trace(subdev, "reading temperature from SHADOWed sensor\n");
>>
>>       /* device valid */
>> -     if (tsensor & 0x20000000)
>> -             return (inttemp >> 8);
>> -     else
>> +     if (tsensor & 0x20000000) {
>> +             *val = inttemp >> 8;
>> +             return 0;
>> +     } else
>>               return -ENODEV;
>>  }
>>
>> diff --git a/drm/nouveau/nvkm/subdev/therm/nv40.c b/drm/nouveau/nvkm/subdev/therm/nv40.c
>> index 2c92ffb5..cfd5b215 100644
>> --- a/drm/nouveau/nvkm/subdev/therm/nv40.c
>> +++ b/drm/nouveau/nvkm/subdev/therm/nv40.c
>> @@ -70,7 +70,7 @@ nv40_sensor_setup(struct nvkm_therm *therm)
>>  }
>>
>>  static int
>> -nv40_temp_get(struct nvkm_therm *therm)
>> +nv40_temp_get(struct nvkm_therm *therm, int *val)
>>  {
>>       struct nvkm_device *device = therm->subdev.device;
>>       struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
>> @@ -95,11 +95,8 @@ nv40_temp_get(struct nvkm_therm *therm)
>>       core_temp = core_temp + sensor->offset_num / sensor->offset_den;
>>       core_temp = core_temp + sensor->offset_constant - 8;
>>
>> -     /* reserve negative temperatures for errors */
>> -     if (core_temp < 0)
>> -             core_temp = 0;
>> -
>> -     return core_temp;
>> +     *val = core_temp;
>> +     return 0;
>>  }
>>
>>  static int
>> diff --git a/drm/nouveau/nvkm/subdev/therm/nv50.c b/drm/nouveau/nvkm/subdev/therm/nv50.c
>> index 9b57b433..62ec4063 100644
>> --- a/drm/nouveau/nvkm/subdev/therm/nv50.c
>> +++ b/drm/nouveau/nvkm/subdev/therm/nv50.c
>> @@ -126,7 +126,7 @@ nv50_sensor_setup(struct nvkm_therm *therm)
>>  }
>>
>>  static int
>> -nv50_temp_get(struct nvkm_therm *therm)
>> +nv50_temp_get(struct nvkm_therm *therm, int *val)
>>  {
>>       struct nvkm_device *device = therm->subdev.device;
>>       struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
>> @@ -143,11 +143,8 @@ nv50_temp_get(struct nvkm_therm *therm)
>>       core_temp = core_temp + sensor->offset_num / sensor->offset_den;
>>       core_temp = core_temp + sensor->offset_constant - 8;
>>
>> -     /* reserve negative temperatures for errors */
>> -     if (core_temp < 0)
>> -             core_temp = 0;
>> -
>> -     return core_temp;
>> +     *val = core_temp;
>> +     return 0;
>>  }
>>
>>  static void
>> diff --git a/drm/nouveau/nvkm/subdev/therm/priv.h b/drm/nouveau/nvkm/subdev/therm/priv.h
>> index 1f46e371..b325ec5f 100644
>> --- a/drm/nouveau/nvkm/subdev/therm/priv.h
>> +++ b/drm/nouveau/nvkm/subdev/therm/priv.h
>> @@ -91,7 +91,7 @@ struct nvkm_therm_func {
>>       int (*pwm_set)(struct nvkm_therm *, int line, u32, u32);
>>       int (*pwm_clock)(struct nvkm_therm *, int line);
>>
>> -     int (*temp_get)(struct nvkm_therm *);
>> +     int (*temp_get)(struct nvkm_therm *, int *);
>>
>>       int (*fan_sense)(struct nvkm_therm *);
>>
>> @@ -105,7 +105,7 @@ int  nv50_fan_pwm_get(struct nvkm_therm *, int, u32 *, u32 *);
>>  int  nv50_fan_pwm_set(struct nvkm_therm *, int, u32, u32);
>>  int  nv50_fan_pwm_clock(struct nvkm_therm *, int);
>>
>> -int  g84_temp_get(struct nvkm_therm *);
>> +int  g84_temp_get(struct nvkm_therm *, int *);
>>  void g84_sensor_setup(struct nvkm_therm *);
>>  void g84_therm_fini(struct nvkm_therm *);
>>
>> diff --git a/drm/nouveau/nvkm/subdev/therm/temp.c b/drm/nouveau/nvkm/subdev/therm/temp.c
>> index ddb2b2c6..5ec2dfb3 100644
>> --- a/drm/nouveau/nvkm/subdev/therm/temp.c
>> +++ b/drm/nouveau/nvkm/subdev/therm/temp.c
>> @@ -86,7 +86,10 @@ nvkm_therm_sensor_event(struct nvkm_therm *therm, enum nvkm_therm_thrs thrs,
>>       static const char * const thresholds[] = {
>>               "fanboost", "downclock", "critical", "shutdown"
>>       };
>> -     int temperature = therm->func->temp_get(therm);
>> +     int temperature;
>> +
>> +     if (therm->func->temp_get(therm, &temperature) < 0)
>> +             return;
>>
>>       if (thrs < 0 || thrs > 3)
>>               return;
>> @@ -140,7 +143,10 @@ nvkm_therm_threshold_hyst_polling(struct nvkm_therm *therm,
>>  {
>>       enum nvkm_therm_thrs_direction direction;
>>       enum nvkm_therm_thrs_state prev_state, new_state;
>> -     int temp = therm->func->temp_get(therm);
>> +     int temp;
>> +
>> +     if (therm->func->temp_get(therm, &temp) < 0)
>> +             return;
>>
>>       prev_state = nvkm_therm_sensor_get_threshold_state(therm, thrs_name);
>>
>> @@ -166,6 +172,7 @@ alarm_timer_callback(struct nvkm_alarm *alarm)
>>       struct nvbios_therm_sensor *sensor = &therm->bios_sensor;
>>       struct nvkm_timer *tmr = therm->subdev.device->timer;
>>       unsigned long flags;
>> +     int val;
>>
>>       spin_lock_irqsave(&therm->sensor.alarm_program_lock, flags);
>>
>> @@ -185,7 +192,7 @@ alarm_timer_callback(struct nvkm_alarm *alarm)
>>       spin_unlock_irqrestore(&therm->sensor.alarm_program_lock, flags);
>>
>>       /* schedule the next poll in one second */
>> -     if (therm->func->temp_get(therm) >= 0)
>> +     if (therm->func->temp_get(therm, &val) >= 0)
>>               nvkm_timer_alarm(tmr, 1000000000ULL, alarm);
>>  }
>>
>> @@ -227,9 +234,10 @@ nvkm_therm_sensor_fini(struct nvkm_therm *therm, bool suspend)
>>  void
>>  nvkm_therm_sensor_preinit(struct nvkm_therm *therm)
>>  {
>> +     int val;
>>       const char *sensor_avail = "yes";
>>
>> -     if (therm->func->temp_get(therm) < 0)
>> +     if (therm->func->temp_get(therm, &val) < 0)
>>               sensor_avail = "no";
>>
>>       nvkm_debug(&therm->subdev, "internal sensor: %s\n", sensor_avail);
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 4841 bytes --]

On 2017-09-15 — 17:11, Karol Herbst wrote:
> This function will be used to update the current clock state.
> 
> This will happen for various reasons:
>   * Temperature changes
>   * User changes clocking state
>   * Load changes
> 
> v2: remove parameter name
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h |  1 +
>  drm/nouveau/nvkm/subdev/clk/base.c    | 26 ++++++++++++++++----------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index e5275f74..ce3bbcfe 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -123,6 +123,7 @@ int nvkm_clk_ustate(struct nvkm_clk *, int req, int pwr);
>  int nvkm_clk_astate(struct nvkm_clk *, int req, int rel, bool wait);
>  int nvkm_clk_dstate(struct nvkm_clk *, int req, int rel);
>  int nvkm_clk_tstate(struct nvkm_clk *, u8 temperature);
> +int nvkm_clk_update(struct nvkm_clk *, bool wait);
>  
>  int nv04_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
>  int nv40_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index e4c8d310..ecff3ff3 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -296,7 +296,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>  }
>  
>  static void
> -nvkm_pstate_work(struct work_struct *work)
> +nvkm_clk_update_work(struct work_struct *work)
>  {
>  	struct nvkm_clk *clk = container_of(work, typeof(*clk), work);
>  	struct nvkm_subdev *subdev = &clk->subdev;
> @@ -332,9 +332,15 @@ nvkm_pstate_work(struct work_struct *work)
>  	nvkm_notify_get(&clk->pwrsrc_ntfy);
>  }
>  
> -static int
> -nvkm_pstate_calc(struct nvkm_clk *clk, bool wait)
> +int
> +nvkm_clk_update(struct nvkm_clk *clk, bool wait)
>  {
> +	if (!clk)
> +		return -EINVAL;
> +
> +	if (!clk->allow_reclock)
> +		return -ENODEV;
> +
>  	atomic_set(&clk->waiting, 1);
>  	schedule_work(&clk->work);
>  	if (wait)
> @@ -524,7 +530,7 @@ nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
>  	if (ret >= 0) {
>  		if (ret -= 2, pwr) clk->ustate_ac = ret;
>  		else		   clk->ustate_dc = ret;
> -		return nvkm_pstate_calc(clk, true);
> +		return nvkm_clk_update(clk, true);
>  	}
>  	return ret;
>  }
> @@ -536,7 +542,7 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
>  	if ( rel) clk->astate += rel;
>  	clk->astate = min(clk->astate, clk->state_nr - 1);
>  	clk->astate = max(clk->astate, 0);
> -	return nvkm_pstate_calc(clk, wait);
> +	return nvkm_clk_update(clk, wait);
>  }
>  
>  int
> @@ -545,7 +551,7 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
>  	if (clk->temp == temp)
>  		return 0;
>  	clk->temp = temp;
> -	return nvkm_pstate_calc(clk, false);
> +	return nvkm_clk_update(clk, false);
>  }
>  
>  int
> @@ -555,7 +561,7 @@ nvkm_clk_dstate(struct nvkm_clk *clk, int req, int rel)
>  	if ( rel) clk->dstate += rel;
>  	clk->dstate = min(clk->dstate, clk->state_nr - 1);
>  	clk->dstate = max(clk->dstate, 0);
> -	return nvkm_pstate_calc(clk, true);
> +	return nvkm_clk_update(clk, true);
>  }
>  
>  static int
> @@ -563,7 +569,7 @@ nvkm_clk_pwrsrc(struct nvkm_notify *notify)
>  {
>  	struct nvkm_clk *clk =
>  		container_of(notify, typeof(*clk), pwrsrc_ntfy);
> -	nvkm_pstate_calc(clk, false);
> +	nvkm_clk_update(clk, false);
>  	return NVKM_NOTIFY_DROP;

Same here as below, shouldn’t there be some error checking on what
`nvkm_clk_update()` returned, as it can fail?

>  }
>  
> @@ -618,7 +624,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>  	clk->dstate = 0;
>  	clk->pstate = -1;
>  	clk->temp = 90; /* reasonable default value */
> -	nvkm_pstate_calc(clk, true);
> +	nvkm_clk_update(clk, true);
>  	return 0;

`nvkm_pstate_calc()` did not return any error code, but `nvkm_clk_update()` now
can, so shouldn’t the function return the return value of `nvkm_clk_update()`
instead? Or at least do some error checking on what `nvkm_clk_update()`
returned?

>  }
>  
> @@ -675,7 +681,7 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>  	clk->ustate_dc = -1;
>  	clk->allow_reclock = allow_reclock;
>  
> -	INIT_WORK(&clk->work, nvkm_pstate_work);
> +	INIT_WORK(&clk->work, nvkm_clk_update_work);
>  	init_waitqueue_head(&clk->wait);
>  	atomic_set(&clk->waiting, 0);
>  
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 03/29] subdev/volt/gk104: return error when read fails

[Karol Herbst]

On Sun, Oct 8, 2017 at 12:29 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> While my gpu was powered off, hwmon returned 0.6V as the current voltage.
>> If nvkm_rd32 fails for any reason, return the error.
>>
>> With that sensors will display a "N/A" instead of 0.6V.
>
> Small nitpick, add a comma between “that” and “sensors”.
> Otherwise,
>
> Reviewed-by: Pierre Moreau <pierre.morrow@free.fr>
>

I think in the end I am not _that_ happy with that change. nvkm_rd32
can't return an error code, but as we interpret the full register as
the value, values above 0x7fffffff are most likely an error on either
our side or the hardware side. When the GPU is suspended we get
0xffffffff out, but this code shouldn't be called in the first place
when the GPU is suspended. We can't really trust runpm as well due to
race conditions, waking up the GPU is another terrible idea.

>>
>> Signed-off-by: Karol Herbst <karolherbst@gmail.com>
>> ---
>>  drm/nouveau/nvkm/subdev/volt/gk104.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drm/nouveau/nvkm/subdev/volt/gk104.c b/drm/nouveau/nvkm/subdev/volt/gk104.c
>> index 1c744e02..53a7af9d 100644
>> --- a/drm/nouveau/nvkm/subdev/volt/gk104.c
>> +++ b/drm/nouveau/nvkm/subdev/volt/gk104.c
>> @@ -40,10 +40,15 @@ gk104_volt_get(struct nvkm_volt *base)
>>  {
>>       struct nvbios_volt *bios = &gk104_volt(base)->bios;
>>       struct nvkm_device *device = base->subdev.device;
>> -     u32 div, duty;
>> +     int div, duty;
>>
>>       div  = nvkm_rd32(device, 0x20340);
>> +     if (div < 0)
>> +             return div;
>> +
>>       duty = nvkm_rd32(device, 0x20344);
>> +     if (duty < 0)
>> +             return duty;
>>
>>       return bios->base + bios->pwm_range * duty / div;
>>  }
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 05/29] clk: Remove dstate

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 3781 bytes --]

This patch could be applied before patch 04, that way patch 04 does not need to
update a function (`nvkm_clk_dstate()`) that will be removed in the next patch.

Reviewed-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

On 2017-09-15 — 17:11, Karol Herbst wrote:
> We won't need it now, because we will adjust the clocks depending on engine
> loads later on anyway or a static lookup table. It also simplifies the
> clocking logic.
> 
> This code was nowhere used anyway and just a mock up.
> 
> v2: fixed typo in commit message
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h |  2 --
>  drm/nouveau/nvkm/subdev/clk/base.c    | 16 ++--------------
>  2 files changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index ce3bbcfe..1340f5b8 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -99,7 +99,6 @@ struct nvkm_clk {
>  	int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
>  	int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
>  	int astate; /* perfmon adjustment (base) */
> -	int dstate; /* display adjustment (min+) */
>  	u8  temp;
>  
>  	bool allow_reclock;
> @@ -121,7 +120,6 @@ struct nvkm_clk {
>  int nvkm_clk_read(struct nvkm_clk *, enum nv_clk_src);
>  int nvkm_clk_ustate(struct nvkm_clk *, int req, int pwr);
>  int nvkm_clk_astate(struct nvkm_clk *, int req, int rel, bool wait);
> -int nvkm_clk_dstate(struct nvkm_clk *, int req, int rel);
>  int nvkm_clk_tstate(struct nvkm_clk *, u8 temperature);
>  int nvkm_clk_update(struct nvkm_clk *, bool wait);
>  
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index ecff3ff3..07d530ed 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -306,15 +306,14 @@ nvkm_clk_update_work(struct work_struct *work)
>  		return;
>  	clk->pwrsrc = power_supply_is_system_supplied();
>  
> -	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C D %d\n",
> +	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
>  		   clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> -		   clk->astate, clk->temp, clk->dstate);
> +		   clk->astate, clk->temp);
>  
>  	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>  	if (clk->state_nr && pstate != -1) {
>  		pstate = (pstate < 0) ? clk->astate : pstate;
>  		pstate = min(pstate, clk->state_nr - 1);
> -		pstate = max(pstate, clk->dstate);
>  	} else {
>  		pstate = clk->pstate = -1;
>  	}
> @@ -554,16 +553,6 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
>  	return nvkm_clk_update(clk, false);
>  }
>  
> -int
> -nvkm_clk_dstate(struct nvkm_clk *clk, int req, int rel)
> -{
> -	if (!rel) clk->dstate  = req;
> -	if ( rel) clk->dstate += rel;
> -	clk->dstate = min(clk->dstate, clk->state_nr - 1);
> -	clk->dstate = max(clk->dstate, 0);
> -	return nvkm_clk_update(clk, true);
> -}
> -
>  static int
>  nvkm_clk_pwrsrc(struct nvkm_notify *notify)
>  {
> @@ -621,7 +610,6 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>  		return clk->func->init(clk);
>  
>  	clk->astate = clk->state_nr - 1;
> -	clk->dstate = 0;
>  	clk->pstate = -1;
>  	clk->temp = 90; /* reasonable default value */
>  	nvkm_clk_update(clk, true);
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 6653 bytes --]

The patch seems fine but I found it super confusing that sometimes `pstate` is
a pointer (for example `clk->pstate`), sometimes it is an int (for example
`args->v0.pstate`).

On 2017-09-15 — 17:11, Karol Herbst wrote:
> We will access the current cstate at least every second and this saves us
> some CPU cycles looking them up every second.
> 
> v2: Rewording commit message.
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h |  4 +++-
>  drm/nouveau/nvkm/engine/device/ctrl.c |  5 ++++-
>  drm/nouveau/nvkm/subdev/clk/base.c    | 17 ++++++++++++-----
>  drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 18 +++++++-----------
>  4 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index 1340f5b8..ec537e08 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -10,6 +10,8 @@ struct nvkm_pll_vals;
>  #define NVKM_CLK_CSTATE_BASE    -2 /* pstate base */
>  #define NVKM_CLK_CSTATE_HIGHEST -3 /* highest possible */
>  
> +#define NVKM_CLK_PSTATE_DEFAULT -1
> +
>  enum nv_clk_src {
>  	nv_clk_src_crystal,
>  	nv_clk_src_href,
> @@ -95,7 +97,7 @@ struct nvkm_clk {
>  
>  	struct nvkm_notify pwrsrc_ntfy;
>  	int pwrsrc;
> -	int pstate; /* current */
> +	struct nvkm_pstate *pstate; /* current */
>  	int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
>  	int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
>  	int astate; /* perfmon adjustment (base) */
> diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c b/drm/nouveau/nvkm/engine/device/ctrl.c
> index b0ece71a..da70626c 100644
> --- a/drm/nouveau/nvkm/engine/device/ctrl.c
> +++ b/drm/nouveau/nvkm/engine/device/ctrl.c
> @@ -52,7 +52,10 @@ nvkm_control_mthd_pstate_info(struct nvkm_control *ctrl, void *data, u32 size)
>  		args->v0.ustate_ac = clk->ustate_ac;
>  		args->v0.ustate_dc = clk->ustate_dc;
>  		args->v0.pwrsrc = clk->pwrsrc;
> -		args->v0.pstate = clk->pstate;
> +		if (clk->pstate)
> +			args->v0.pstate = clk->pstate->pstate;
> +		else
> +			args->v0.pstate = NVKM_CLK_PSTATE_DEFAULT;
>  	} else {
>  		args->v0.count = 0;
>  		args->v0.ustate_ac = NVIF_CONTROL_PSTATE_INFO_V0_USTATE_DISABLE;
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index 07d530ed..0d4d9fdf 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -271,13 +271,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>  	struct nvkm_pstate *pstate;
>  	int ret, idx = 0;
>  
> +	if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
> +		return 0;
> +
>  	list_for_each_entry(pstate, &clk->states, head) {
>  		if (idx++ == pstatei)
>  			break;
>  	}
>  
>  	nvkm_debug(subdev, "setting performance state %d\n", pstatei);
> -	clk->pstate = pstatei;
> +	clk->pstate = pstate;
>  
>  	nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>  
> @@ -306,8 +309,12 @@ nvkm_clk_update_work(struct work_struct *work)
>  		return;
>  	clk->pwrsrc = power_supply_is_system_supplied();
>  
> +	if (clk->pstate)
> +		pstate = clk->pstate->pstate;
> +	else
> +		pstate = NVKM_CLK_PSTATE_DEFAULT;
>  	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
> -		   clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> +		   pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
>  		   clk->astate, clk->temp);
>  
>  	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;

In the if-statement following the previous line, the -1 could be replaced by
NVKM_CLK_PSTATE_DEFAULT.

> @@ -315,11 +322,11 @@ nvkm_clk_update_work(struct work_struct *work)
>  		pstate = (pstate < 0) ? clk->astate : pstate;

Can `pstate` have negative values other than -1?

>  		pstate = min(pstate, clk->state_nr - 1);
>  	} else {
> -		pstate = clk->pstate = -1;
> +		pstate = NVKM_CLK_PSTATE_DEFAULT;

Shouldn’t you also reset `clk->pstate` to NULL? Or maybe it is
`nvkm_pstate_prog()` which should do it if
`pstatei == NVKM_CLK_PSTATE_DEFAULT`.

>  	}
>  
>  	nvkm_trace(subdev, "-> %d\n", pstate);
> -	if (pstate != clk->pstate) {
> +	if (!clk->pstate || pstate != clk->pstate->pstate) {
>  		int ret = nvkm_pstate_prog(clk, pstate);
>  		if (ret) {
>  			nvkm_error(subdev, "error setting pstate %d: %d\n",
> @@ -610,7 +617,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>  		return clk->func->init(clk);
>  
>  	clk->astate = clk->state_nr - 1;
> -	clk->pstate = -1;
> +	clk->pstate = NULL;
>  	clk->temp = 90; /* reasonable default value */
>  	nvkm_clk_update(clk, true);
>  	return 0;
> diff --git a/drm/nouveau/nvkm/subdev/pmu/gk20a.c b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> index 05e81855..de579726 100644
> --- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> +++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> @@ -55,24 +55,22 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int *state)
>  	return nvkm_clk_astate(clk, *state, 0, false);
>  }
>  
> -static void
> -gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
> -{
> -	struct nvkm_clk *clk = pmu->base.subdev.device->clk;
> -
> -	*state = clk->pstate;
> -}
> -
>  static int
>  gk20a_pmu_dvfs_get_target_state(struct gk20a_pmu *pmu,
>  				int *state, int load)
>  {
>  	struct gk20a_pmu_dvfs_data *data = pmu->data;
>  	struct nvkm_clk *clk = pmu->base.subdev.device->clk;
> +	struct nvkm_pstate *pstate = clk->pstate;
>  	int cur_level, level;
>  
> +	if (!pstate) {
> +		*state = 0;
> +		return 1;
> +	}
> +
>  	/* For GK20A, the performance level is directly mapped to pstate */
> -	level = cur_level = clk->pstate;
> +	level = cur_level = clk->pstate->pstate;
>  
>  	if (load > data->p_load_max) {
>  		level = min(clk->state_nr - 1, level + (clk->state_nr / 3));
> @@ -142,8 +140,6 @@ gk20a_pmu_dvfs_work(struct nvkm_alarm *alarm)
>  	nvkm_trace(subdev, "utilization = %d %%, avg_load = %d %%\n",
>  		   utilization, data->avg_load);
>  
> -	gk20a_pmu_dvfs_get_cur_state(pmu, &state);
> -
>  	if (gk20a_pmu_dvfs_get_target_state(pmu, &state, data->avg_load)) {
>  		nvkm_trace(subdev, "set new state to %d\n", state);
>  		gk20a_pmu_dvfs_target(pmu, &state);
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 07/29] clk: Hold information about the current cstate status

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 5380 bytes --]

Acked-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

On 2017-09-15 — 17:11, Karol Herbst wrote:
> Later we will have situations where the expected and the current state
> isn't the same.
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h |  2 ++
>  drm/nouveau/nvkm/subdev/clk/base.c    | 32 +++++++++++++++++++++++++-------
>  2 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index ec537e08..f35518c3 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -101,6 +101,8 @@ struct nvkm_clk {
>  	int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
>  	int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
>  	int astate; /* perfmon adjustment (base) */
> +	struct nvkm_cstate *cstate;
> +	int exp_cstateid;
>  	u8  temp;
>  
>  	bool allow_reclock;
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index 0d4d9fdf..d37c13b7 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -146,9 +146,14 @@ static struct nvkm_cstate *
>  nvkm_cstate_get(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
>  {
>  	struct nvkm_cstate *cstate;
> -	if (cstatei == NVKM_CLK_CSTATE_HIGHEST)
> +	switch (cstatei) {
> +	case NVKM_CLK_CSTATE_HIGHEST:
>  		return list_last_entry(&pstate->list, typeof(*cstate), head);
> -	else {
> +	case NVKM_CLK_CSTATE_BASE:
> +		return &pstate->base;
> +	case NVKM_CLK_CSTATE_DEFAULT:
> +		return NULL;
> +	default:
>  		list_for_each_entry(cstate, &pstate->list, head) {
>  			if (cstate->id == cstatei)
>  				return cstate;
> @@ -167,6 +172,9 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
>  	struct nvkm_cstate *cstate;
>  	int ret;
>  
> +	if (cstatei == NVKM_CLK_CSTATE_DEFAULT)
> +		return 0;
> +
>  	if (!list_empty(&pstate->list)) {
>  		cstate = nvkm_cstate_get(clk, pstate, cstatei);
>  		cstate = nvkm_cstate_find_best(clk, pstate, cstate);
> @@ -193,6 +201,7 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct nvkm_pstate *pstate, int cstatei)
>  
>  	ret = clk->func->calc(clk, cstate);
>  	if (ret == 0) {
> +		clk->cstate = cstate;
>  		ret = clk->func->prog(clk);
>  		clk->func->tidy(clk);
>  	}
> @@ -295,7 +304,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>  		ram->func->tidy(ram);
>  	}
>  
> -	return nvkm_cstate_prog(clk, pstate, NVKM_CLK_CSTATE_HIGHEST);
> +	return nvkm_cstate_prog(clk, pstate, clk->exp_cstateid);
>  }
>  
>  static void
> @@ -313,9 +322,9 @@ nvkm_clk_update_work(struct work_struct *work)
>  		pstate = clk->pstate->pstate;
>  	else
>  		pstate = NVKM_CLK_PSTATE_DEFAULT;
> -	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
> +	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d C %d T %d°C\n",
>  		   pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> -		   clk->astate, clk->temp);
> +		   clk->astate, clk->exp_cstateid, clk->temp);
>  
>  	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>  	if (clk->state_nr && pstate != -1) {
> @@ -536,6 +545,7 @@ nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
>  	if (ret >= 0) {
>  		if (ret -= 2, pwr) clk->ustate_ac = ret;
>  		else		   clk->ustate_dc = ret;
> +		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
>  		return nvkm_clk_update(clk, true);
>  	}
>  	return ret;
> @@ -548,6 +558,7 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
>  	if ( rel) clk->astate += rel;
>  	clk->astate = min(clk->astate, clk->state_nr - 1);
>  	clk->astate = max(clk->astate, 0);
> +	clk->exp_cstateid = NVKM_CLK_CSTATE_BASE;
>  	return nvkm_clk_update(clk, wait);
>  }
>  
> @@ -618,6 +629,8 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>  
>  	clk->astate = clk->state_nr - 1;
>  	clk->pstate = NULL;
> +	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
> +	clk->cstate = NULL;
>  	clk->temp = 90; /* reasonable default value */
>  	nvkm_clk_update(clk, true);
>  	return 0;
> @@ -701,15 +714,20 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>  	if (mode) {
>  		clk->ustate_ac = nvkm_clk_nstate(clk, mode, arglen);
>  		clk->ustate_dc = nvkm_clk_nstate(clk, mode, arglen);
> +		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
>  	}
>  
>  	mode = nvkm_stropt(device->cfgopt, "NvClkModeAC", &arglen);
> -	if (mode)
> +	if (mode) {
>  		clk->ustate_ac = nvkm_clk_nstate(clk, mode, arglen);
> +		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
> +	}
>  
>  	mode = nvkm_stropt(device->cfgopt, "NvClkModeDC", &arglen);
> -	if (mode)
> +	if (mode) {
>  		clk->ustate_dc = nvkm_clk_nstate(clk, mode, arglen);
> +		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
> +	}
>  
>  	clk->boost_mode = nvkm_longopt(device->cfgopt, "NvBoost",
>  				       NVKM_CLK_BOOST_NONE);
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 04/29] clk: Rename nvkm_pstate_calc to nvkm_clk_update and export it

[Karol Herbst]

On Sun, Oct 8, 2017 at 12:41 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> This function will be used to update the current clock state.
>>
>> This will happen for various reasons:
>>   * Temperature changes
>>   * User changes clocking state
>>   * Load changes
>>
>> v2: remove parameter name
>>
>> Signed-off-by: Karol Herbst <karolherbst@gmail.com>
>> ---
>>  drm/nouveau/include/nvkm/subdev/clk.h |  1 +
>>  drm/nouveau/nvkm/subdev/clk/base.c    | 26 ++++++++++++++++----------
>>  2 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
>> index e5275f74..ce3bbcfe 100644
>> --- a/drm/nouveau/include/nvkm/subdev/clk.h
>> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
>> @@ -123,6 +123,7 @@ int nvkm_clk_ustate(struct nvkm_clk *, int req, int pwr);
>>  int nvkm_clk_astate(struct nvkm_clk *, int req, int rel, bool wait);
>>  int nvkm_clk_dstate(struct nvkm_clk *, int req, int rel);
>>  int nvkm_clk_tstate(struct nvkm_clk *, u8 temperature);
>> +int nvkm_clk_update(struct nvkm_clk *, bool wait);
>>
>>  int nv04_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
>>  int nv40_clk_new(struct nvkm_device *, int, struct nvkm_clk **);
>> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
>> index e4c8d310..ecff3ff3 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -296,7 +296,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>>  }
>>
>>  static void
>> -nvkm_pstate_work(struct work_struct *work)
>> +nvkm_clk_update_work(struct work_struct *work)
>>  {
>>       struct nvkm_clk *clk = container_of(work, typeof(*clk), work);
>>       struct nvkm_subdev *subdev = &clk->subdev;
>> @@ -332,9 +332,15 @@ nvkm_pstate_work(struct work_struct *work)
>>       nvkm_notify_get(&clk->pwrsrc_ntfy);
>>  }
>>
>> -static int
>> -nvkm_pstate_calc(struct nvkm_clk *clk, bool wait)
>> +int
>> +nvkm_clk_update(struct nvkm_clk *clk, bool wait)
>>  {
>> +     if (!clk)
>> +             return -EINVAL;
>> +
>> +     if (!clk->allow_reclock)
>> +             return -ENODEV;
>> +
>>       atomic_set(&clk->waiting, 1);
>>       schedule_work(&clk->work);
>>       if (wait)
>> @@ -524,7 +530,7 @@ nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
>>       if (ret >= 0) {
>>               if (ret -= 2, pwr) clk->ustate_ac = ret;
>>               else               clk->ustate_dc = ret;
>> -             return nvkm_pstate_calc(clk, true);
>> +             return nvkm_clk_update(clk, true);
>>       }
>>       return ret;
>>  }
>> @@ -536,7 +542,7 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
>>       if ( rel) clk->astate += rel;
>>       clk->astate = min(clk->astate, clk->state_nr - 1);
>>       clk->astate = max(clk->astate, 0);
>> -     return nvkm_pstate_calc(clk, wait);
>> +     return nvkm_clk_update(clk, wait);
>>  }
>>
>>  int
>> @@ -545,7 +551,7 @@ nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
>>       if (clk->temp == temp)
>>               return 0;
>>       clk->temp = temp;
>> -     return nvkm_pstate_calc(clk, false);
>> +     return nvkm_clk_update(clk, false);
>>  }
>>
>>  int
>> @@ -555,7 +561,7 @@ nvkm_clk_dstate(struct nvkm_clk *clk, int req, int rel)
>>       if ( rel) clk->dstate += rel;
>>       clk->dstate = min(clk->dstate, clk->state_nr - 1);
>>       clk->dstate = max(clk->dstate, 0);
>> -     return nvkm_pstate_calc(clk, true);
>> +     return nvkm_clk_update(clk, true);
>>  }
>>
>>  static int
>> @@ -563,7 +569,7 @@ nvkm_clk_pwrsrc(struct nvkm_notify *notify)
>>  {
>>       struct nvkm_clk *clk =
>>               container_of(notify, typeof(*clk), pwrsrc_ntfy);
>> -     nvkm_pstate_calc(clk, false);
>> +     nvkm_clk_update(clk, false);
>>       return NVKM_NOTIFY_DROP;
>
> Same here as below, shouldn’t there be some error checking on what
> `nvkm_clk_update()` returned, as it can fail?
>

it doesn't matter here, because it shouldn't change the return value
of that function.

>>  }
>>
>> @@ -618,7 +624,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>>       clk->dstate = 0;
>>       clk->pstate = -1;
>>       clk->temp = 90; /* reasonable default value */
>> -     nvkm_pstate_calc(clk, true);
>> +     nvkm_clk_update(clk, true);
>>       return 0;
>
> `nvkm_pstate_calc()` did not return any error code, but `nvkm_clk_update()` now
> can, so shouldn’t the function return the return value of `nvkm_clk_update()`
> instead? Or at least do some error checking on what `nvkm_clk_update()`
> returned?

I don't want nvkm_clk_init to fail just because we don't allow
reclocking (which is basically the only reason it may return !0 here)

>
>>  }
>>
>> @@ -675,7 +681,7 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>>       clk->ustate_dc = -1;
>>       clk->allow_reclock = allow_reclock;
>>
>> -     INIT_WORK(&clk->work, nvkm_pstate_work);
>> +     INIT_WORK(&clk->work, nvkm_clk_update_work);
>>       init_waitqueue_head(&clk->wait);
>>       atomic_set(&clk->waiting, 0);
>>
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 6188 bytes --]

On 2017-09-15 — 17:11, Karol Herbst wrote:
> This makes the code easier, because we can compare the id with
> pstate->pstate and saves us from the trouble of iterating over the pstates
> to match the index.

I don’t remember whether I have already done  this comment before, but I am not
sure where your iterating over the pstates savings are, as in this patch at
least, you iterate as much as you did before.

A few more comments below

> v2: reword commit message
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>
> ---
>  drm/nouveau/nouveau_debugfs.c      |  6 +--
>  drm/nouveau/nvkm/subdev/clk/base.c | 78 +++++++++++++++++++-------------------
>  2 files changed, 41 insertions(+), 43 deletions(-)
> 
> diff --git a/drm/nouveau/nouveau_debugfs.c b/drm/nouveau/nouveau_debugfs.c
> index 963a4dba..27281c4e 100644
> --- a/drm/nouveau/nouveau_debugfs.c
> +++ b/drm/nouveau/nouveau_debugfs.c
> @@ -96,11 +96,11 @@ nouveau_debugfs_pstate_get(struct seq_file *m, void *data)
>  		} while (attr.index);
>  
>  		if (state >= 0) {
> -			if (info.ustate_ac == state)
> +			if (info.ustate_ac == attr.state)
>  				seq_printf(m, " AC");
> -			if (info.ustate_dc == state)
> +			if (info.ustate_dc == attr.state)
>  				seq_printf(m, " DC");
> -			if (info.pstate == state)
> +			if (info.pstate == attr.state)
>  				seq_printf(m, " *");
>  		} else {
>  			if (info.ustate_ac < -1)
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index d37c13b7..1d71bf09 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -272,23 +272,26 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
>   * P-States
>   *****************************************************************************/
>  static int
> -nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
> +nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
>  {
>  	struct nvkm_subdev *subdev = &clk->subdev;
>  	struct nvkm_fb *fb = subdev->device->fb;
>  	struct nvkm_pci *pci = subdev->device->pci;
>  	struct nvkm_pstate *pstate;
> -	int ret, idx = 0;
> +	int ret;
>  
> -	if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
> +	if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
>  		return 0;
>  
>  	list_for_each_entry(pstate, &clk->states, head) {
> -		if (idx++ == pstatei)
> +		if (pstate->pstate == pstateid)
>  			break;
>  	}
>  
> -	nvkm_debug(subdev, "setting performance state %d\n", pstatei);
> +	if (!pstate)
> +		return -EINVAL;
> +
> +	nvkm_debug(subdev, "setting performance state %x\n", pstateid);
>  	clk->pstate = pstate;
>  
>  	nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
> @@ -329,7 +332,6 @@ nvkm_clk_update_work(struct work_struct *work)
>  	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>  	if (clk->state_nr && pstate != -1) {
>  		pstate = (pstate < 0) ? clk->astate : pstate;
> -		pstate = min(pstate, clk->state_nr - 1);
>  	} else {
>  		pstate = NVKM_CLK_PSTATE_DEFAULT;
>  	}
> @@ -490,33 +492,10 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx)
>  /******************************************************************************
>   * Adjustment triggers
>   *****************************************************************************/
> -static int
> -nvkm_clk_ustate_update(struct nvkm_clk *clk, int req)
> -{
> -	struct nvkm_pstate *pstate;
> -	int i = 0;
> -
> -	if (!clk->allow_reclock)
> -		return -ENOSYS;
> -
> -	if (req != -1 && req != -2) {
> -		list_for_each_entry(pstate, &clk->states, head) {
> -			if (pstate->pstate == req)
> -				break;
> -			i++;
> -		}
> -
> -		if (pstate->pstate != req)
> -			return -EINVAL;
> -		req = i;
> -	}
> -
> -	return req + 2;
> -}
> -
>  static int
>  nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
>  {
> +	struct nvkm_pstate *pstate;
>  	int ret = 1;
>  
>  	if (clk->allow_reclock && !strncasecmpz(mode, "auto", arglen))
> @@ -528,27 +507,46 @@ nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
>  
>  		((char *)mode)[arglen] = '\0';
>  		if (!kstrtol(mode, 0, &v)) {
> -			ret = nvkm_clk_ustate_update(clk, v);
> +			ret = v;
>  			if (ret < 0)
>  				ret = 1;
>  		}
>  		((char *)mode)[arglen] = save;
>  	}
>  
> -	return ret - 2;
> +	if (ret < 0)
> +		return ret;

Don’t you need to check for `clk->allow_reclock`, as it was done in
`nvkm_clk_ustate_update()`?

> +
> +	list_for_each_entry(pstate, &clk->states, head) {
> +		if (pstate->pstate == ret)
> +			return ret;
> +	}
> +	return -EINVAL;
>  }
>  
>  int
>  nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
>  {
> -	int ret = nvkm_clk_ustate_update(clk, req);
> -	if (ret >= 0) {
> -		if (ret -= 2, pwr) clk->ustate_ac = ret;
> -		else		   clk->ustate_dc = ret;
> -		clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
> -		return nvkm_clk_update(clk, true);
> +	struct nvkm_pstate *pstate;
> +	bool valid = false;

Same here, don’t you need to check for `clk->allow_reclock`, as it was done in
`nvkm_clk_ustate_update()`?

> +
> +	list_for_each_entry(pstate, &clk->states, head) {
> +		if (pstate->pstate == req) {
> +			valid = true;
> +			break;
> +		}
>  	}
> -	return ret;
> +
> +	if (!valid)
> +		return -EINVAL;
> +
> +	if (pwr)
> +		clk->ustate_ac = req;
> +	else
> +		clk->ustate_dc = req;
> +
> +	clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
> +	return nvkm_clk_update(clk, true);
>  }
>  
>  int
> @@ -627,7 +625,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>  	if (clk->func->init)
>  		return clk->func->init(clk);
>  
> -	clk->astate = clk->state_nr - 1;
> +	clk->astate = NVKM_CLK_PSTATE_DEFAULT;
>  	clk->pstate = NULL;
>  	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
>  	clk->cstate = NULL;
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 06/29] clk: Make pstate a pointer to nvkm_pstate

[Karol Herbst]

On Sun, Oct 8, 2017 at 1:47 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> The patch seems fine but I found it super confusing that sometimes `pstate` is
> a pointer (for example `clk->pstate`), sometimes it is an int (for example
> `args->v0.pstate`).
>

yeah I still plan to clean the names up when I find some time to
finish those patches for real

> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> We will access the current cstate at least every second and this saves us
>> some CPU cycles looking them up every second.
>>
>> v2: Rewording commit message.
>>
>> Signed-off-by: Karol Herbst <karolherbst@gmail.com>
>> Reviewed-by: Martin Peres <martin.peres@free.fr>
>> ---
>>  drm/nouveau/include/nvkm/subdev/clk.h |  4 +++-
>>  drm/nouveau/nvkm/engine/device/ctrl.c |  5 ++++-
>>  drm/nouveau/nvkm/subdev/clk/base.c    | 17 ++++++++++++-----
>>  drm/nouveau/nvkm/subdev/pmu/gk20a.c   | 18 +++++++-----------
>>  4 files changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
>> index 1340f5b8..ec537e08 100644
>> --- a/drm/nouveau/include/nvkm/subdev/clk.h
>> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
>> @@ -10,6 +10,8 @@ struct nvkm_pll_vals;
>>  #define NVKM_CLK_CSTATE_BASE    -2 /* pstate base */
>>  #define NVKM_CLK_CSTATE_HIGHEST -3 /* highest possible */
>>
>> +#define NVKM_CLK_PSTATE_DEFAULT -1
>> +
>>  enum nv_clk_src {
>>       nv_clk_src_crystal,
>>       nv_clk_src_href,
>> @@ -95,7 +97,7 @@ struct nvkm_clk {
>>
>>       struct nvkm_notify pwrsrc_ntfy;
>>       int pwrsrc;
>> -     int pstate; /* current */
>> +     struct nvkm_pstate *pstate; /* current */
>>       int ustate_ac; /* user-requested (-1 disabled, -2 perfmon) */
>>       int ustate_dc; /* user-requested (-1 disabled, -2 perfmon) */
>>       int astate; /* perfmon adjustment (base) */
>> diff --git a/drm/nouveau/nvkm/engine/device/ctrl.c b/drm/nouveau/nvkm/engine/device/ctrl.c
>> index b0ece71a..da70626c 100644
>> --- a/drm/nouveau/nvkm/engine/device/ctrl.c
>> +++ b/drm/nouveau/nvkm/engine/device/ctrl.c
>> @@ -52,7 +52,10 @@ nvkm_control_mthd_pstate_info(struct nvkm_control *ctrl, void *data, u32 size)
>>               args->v0.ustate_ac = clk->ustate_ac;
>>               args->v0.ustate_dc = clk->ustate_dc;
>>               args->v0.pwrsrc = clk->pwrsrc;
>> -             args->v0.pstate = clk->pstate;
>> +             if (clk->pstate)
>> +                     args->v0.pstate = clk->pstate->pstate;
>> +             else
>> +                     args->v0.pstate = NVKM_CLK_PSTATE_DEFAULT;
>>       } else {
>>               args->v0.count = 0;
>>               args->v0.ustate_ac = NVIF_CONTROL_PSTATE_INFO_V0_USTATE_DISABLE;
>> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
>> index 07d530ed..0d4d9fdf 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -271,13 +271,16 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>>       struct nvkm_pstate *pstate;
>>       int ret, idx = 0;
>>
>> +     if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
>> +             return 0;
>> +
>>       list_for_each_entry(pstate, &clk->states, head) {
>>               if (idx++ == pstatei)
>>                       break;
>>       }
>>
>>       nvkm_debug(subdev, "setting performance state %d\n", pstatei);
>> -     clk->pstate = pstatei;
>> +     clk->pstate = pstate;
>>
>>       nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>>
>> @@ -306,8 +309,12 @@ nvkm_clk_update_work(struct work_struct *work)
>>               return;
>>       clk->pwrsrc = power_supply_is_system_supplied();
>>
>> +     if (clk->pstate)
>> +             pstate = clk->pstate->pstate;
>> +     else
>> +             pstate = NVKM_CLK_PSTATE_DEFAULT;
>>       nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d T %d°C\n",
>> -                clk->pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
>> +                pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
>>                  clk->astate, clk->temp);
>>
>>       pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>
> In the if-statement following the previous line, the -1 could be replaced by
> NVKM_CLK_PSTATE_DEFAULT.
>
>> @@ -315,11 +322,11 @@ nvkm_clk_update_work(struct work_struct *work)
>>               pstate = (pstate < 0) ? clk->astate : pstate;
>
> Can `pstate` have negative values other than -1?

not yet, but >=0 means a pstate is selected by an id, <0 but a type,
but it might be that we'll never need more than the default one.

>
>>               pstate = min(pstate, clk->state_nr - 1);
>>       } else {
>> -             pstate = clk->pstate = -1;
>> +             pstate = NVKM_CLK_PSTATE_DEFAULT;
>
> Shouldn’t you also reset `clk->pstate` to NULL? Or maybe it is
> `nvkm_pstate_prog()` which should do it if
> `pstatei == NVKM_CLK_PSTATE_DEFAULT`.
>

no. We can't go back to the default clocks except we run devinit
again. Default means: state after powering on the GPU. and clk->pstate
points to the current pstate we reclocked to last. Which means if we
hit the else branch here, clk->pstate is already NULL.

>>       }
>>
>>       nvkm_trace(subdev, "-> %d\n", pstate);
>> -     if (pstate != clk->pstate) {
>> +     if (!clk->pstate || pstate != clk->pstate->pstate) {
>>               int ret = nvkm_pstate_prog(clk, pstate);
>>               if (ret) {
>>                       nvkm_error(subdev, "error setting pstate %d: %d\n",
>> @@ -610,7 +617,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>>               return clk->func->init(clk);
>>
>>       clk->astate = clk->state_nr - 1;
>> -     clk->pstate = -1;
>> +     clk->pstate = NULL;
>>       clk->temp = 90; /* reasonable default value */
>>       nvkm_clk_update(clk, true);
>>       return 0;
>> diff --git a/drm/nouveau/nvkm/subdev/pmu/gk20a.c b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
>> index 05e81855..de579726 100644
>> --- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
>> +++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
>> @@ -55,24 +55,22 @@ gk20a_pmu_dvfs_target(struct gk20a_pmu *pmu, int *state)
>>       return nvkm_clk_astate(clk, *state, 0, false);
>>  }
>>
>> -static void
>> -gk20a_pmu_dvfs_get_cur_state(struct gk20a_pmu *pmu, int *state)
>> -{
>> -     struct nvkm_clk *clk = pmu->base.subdev.device->clk;
>> -
>> -     *state = clk->pstate;
>> -}
>> -
>>  static int
>>  gk20a_pmu_dvfs_get_target_state(struct gk20a_pmu *pmu,
>>                               int *state, int load)
>>  {
>>       struct gk20a_pmu_dvfs_data *data = pmu->data;
>>       struct nvkm_clk *clk = pmu->base.subdev.device->clk;
>> +     struct nvkm_pstate *pstate = clk->pstate;
>>       int cur_level, level;
>>
>> +     if (!pstate) {
>> +             *state = 0;
>> +             return 1;
>> +     }
>> +
>>       /* For GK20A, the performance level is directly mapped to pstate */
>> -     level = cur_level = clk->pstate;
>> +     level = cur_level = clk->pstate->pstate;
>>
>>       if (load > data->p_load_max) {
>>               level = min(clk->state_nr - 1, level + (clk->state_nr / 3));
>> @@ -142,8 +140,6 @@ gk20a_pmu_dvfs_work(struct nvkm_alarm *alarm)
>>       nvkm_trace(subdev, "utilization = %d %%, avg_load = %d %%\n",
>>                  utilization, data->avg_load);
>>
>> -     gk20a_pmu_dvfs_get_cur_state(pmu, &state);
>> -
>>       if (gk20a_pmu_dvfs_get_target_state(pmu, &state, data->avg_load)) {
>>               nvkm_trace(subdev, "set new state to %d\n", state);
>>               gk20a_pmu_dvfs_target(pmu, &state);
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 08/29] clk: We should pass the pstate id around not the index in the list

[Karol Herbst]

On Sun, Oct 8, 2017 at 4:23 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> This makes the code easier, because we can compare the id with
>> pstate->pstate and saves us from the trouble of iterating over the pstates
>> to match the index.
>
> I don’t remember whether I have already done  this comment before, but I am not
> sure where your iterating over the pstates savings are, as in this patch at
> least, you iterate as much as you did before.
>

yeah I think I ended up adding one loop again to check whether the
input was actual an existing pstate or not, but it shouldn't be needed
anymore. It was required before, now it's only there for error
checking.

> A few more comments below
>
>> v2: reword commit message
>>
>> Signed-off-by: Karol Herbst <karolherbst@gmail.com>
>> Reviewed-by: Martin Peres <martin.peres@free.fr>
>> ---
>>  drm/nouveau/nouveau_debugfs.c      |  6 +--
>>  drm/nouveau/nvkm/subdev/clk/base.c | 78 +++++++++++++++++++-------------------
>>  2 files changed, 41 insertions(+), 43 deletions(-)
>>
>> diff --git a/drm/nouveau/nouveau_debugfs.c b/drm/nouveau/nouveau_debugfs.c
>> index 963a4dba..27281c4e 100644
>> --- a/drm/nouveau/nouveau_debugfs.c
>> +++ b/drm/nouveau/nouveau_debugfs.c
>> @@ -96,11 +96,11 @@ nouveau_debugfs_pstate_get(struct seq_file *m, void *data)
>>               } while (attr.index);
>>
>>               if (state >= 0) {
>> -                     if (info.ustate_ac == state)
>> +                     if (info.ustate_ac == attr.state)
>>                               seq_printf(m, " AC");
>> -                     if (info.ustate_dc == state)
>> +                     if (info.ustate_dc == attr.state)
>>                               seq_printf(m, " DC");
>> -                     if (info.pstate == state)
>> +                     if (info.pstate == attr.state)
>>                               seq_printf(m, " *");
>>               } else {
>>                       if (info.ustate_ac < -1)
>> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
>> index d37c13b7..1d71bf09 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -272,23 +272,26 @@ nvkm_cstate_new(struct nvkm_clk *clk, int idx, struct nvkm_pstate *pstate)
>>   * P-States
>>   *****************************************************************************/
>>  static int
>> -nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
>> +nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
>>  {
>>       struct nvkm_subdev *subdev = &clk->subdev;
>>       struct nvkm_fb *fb = subdev->device->fb;
>>       struct nvkm_pci *pci = subdev->device->pci;
>>       struct nvkm_pstate *pstate;
>> -     int ret, idx = 0;
>> +     int ret;
>>
>> -     if (pstatei == NVKM_CLK_PSTATE_DEFAULT)
>> +     if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
>>               return 0;
>>
>>       list_for_each_entry(pstate, &clk->states, head) {
>> -             if (idx++ == pstatei)
>> +             if (pstate->pstate == pstateid)
>>                       break;
>>       }
>>
>> -     nvkm_debug(subdev, "setting performance state %d\n", pstatei);
>> +     if (!pstate)
>> +             return -EINVAL;
>> +
>> +     nvkm_debug(subdev, "setting performance state %x\n", pstateid);
>>       clk->pstate = pstate;
>>
>>       nvkm_pcie_set_link(pci, pstate->pcie_speed, pstate->pcie_width);
>> @@ -329,7 +332,6 @@ nvkm_clk_update_work(struct work_struct *work)
>>       pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>>       if (clk->state_nr && pstate != -1) {
>>               pstate = (pstate < 0) ? clk->astate : pstate;
>> -             pstate = min(pstate, clk->state_nr - 1);
>>       } else {
>>               pstate = NVKM_CLK_PSTATE_DEFAULT;
>>       }
>> @@ -490,33 +492,10 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx)
>>  /******************************************************************************
>>   * Adjustment triggers
>>   *****************************************************************************/
>> -static int
>> -nvkm_clk_ustate_update(struct nvkm_clk *clk, int req)
>> -{
>> -     struct nvkm_pstate *pstate;
>> -     int i = 0;
>> -
>> -     if (!clk->allow_reclock)
>> -             return -ENOSYS;
>> -
>> -     if (req != -1 && req != -2) {
>> -             list_for_each_entry(pstate, &clk->states, head) {
>> -                     if (pstate->pstate == req)
>> -                             break;
>> -                     i++;
>> -             }
>> -
>> -             if (pstate->pstate != req)
>> -                     return -EINVAL;
>> -             req = i;
>> -     }
>> -
>> -     return req + 2;
>> -}
>> -
>>  static int
>>  nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
>>  {
>> +     struct nvkm_pstate *pstate;
>>       int ret = 1;
>>
>>       if (clk->allow_reclock && !strncasecmpz(mode, "auto", arglen))
>> @@ -528,27 +507,46 @@ nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen)
>>
>>               ((char *)mode)[arglen] = '\0';
>>               if (!kstrtol(mode, 0, &v)) {
>> -                     ret = nvkm_clk_ustate_update(clk, v);
>> +                     ret = v;
>>                       if (ret < 0)
>>                               ret = 1;
>>               }
>>               ((char *)mode)[arglen] = save;
>>       }
>>
>> -     return ret - 2;
>> +     if (ret < 0)
>> +             return ret;
>
> Don’t you need to check for `clk->allow_reclock`, as it was done in
> `nvkm_clk_ustate_update()`?
>

well ret ended up being 1 due to that check inside nvkm_clk_ustate_update.

>> +
>> +     list_for_each_entry(pstate, &clk->states, head) {
>> +             if (pstate->pstate == ret)
>> +                     return ret;
>> +     }
>> +     return -EINVAL;
>>  }
>>
>>  int
>>  nvkm_clk_ustate(struct nvkm_clk *clk, int req, int pwr)
>>  {
>> -     int ret = nvkm_clk_ustate_update(clk, req);
>> -     if (ret >= 0) {
>> -             if (ret -= 2, pwr) clk->ustate_ac = ret;
>> -             else               clk->ustate_dc = ret;
>> -             clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
>> -             return nvkm_clk_update(clk, true);
>> +     struct nvkm_pstate *pstate;
>> +     bool valid = false;
>
> Same here, don’t you need to check for `clk->allow_reclock`, as it was done in
> `nvkm_clk_ustate_update()`?
>
>> +
>> +     list_for_each_entry(pstate, &clk->states, head) {
>> +             if (pstate->pstate == req) {
>> +                     valid = true;
>> +                     break;
>> +             }
>>       }
>> -     return ret;
>> +
>> +     if (!valid)
>> +             return -EINVAL;
>> +
>> +     if (pwr)
>> +             clk->ustate_ac = req;
>> +     else
>> +             clk->ustate_dc = req;
>> +
>> +     clk->exp_cstateid = NVKM_CLK_CSTATE_HIGHEST;
>> +     return nvkm_clk_update(clk, true);
>>  }
>>
>>  int
>> @@ -627,7 +625,7 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>>       if (clk->func->init)
>>               return clk->func->init(clk);
>>
>> -     clk->astate = clk->state_nr - 1;
>> +     clk->astate = NVKM_CLK_PSTATE_DEFAULT;
>>       clk->pstate = NULL;
>>       clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
>>       clk->cstate = NULL;
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 09/29] clk: Set clocks to pre suspend state after suspend

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 2027 bytes --]

Reviewed-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

On 2017-09-15 — 17:11, Karol Herbst wrote:
> The idea is to clear out the saved state, because after a resume we can't
> know what the GPU is clocked to. The reclock is triggered by the call to
> nvkm_clk_update later in nvkm_clk_init.
> 
> v2: convert to C style comments
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>
> ---
>  drm/nouveau/nvkm/subdev/clk/base.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index 1d71bf09..54188d2b 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -625,11 +625,10 @@ nvkm_clk_init(struct nvkm_subdev *subdev)
>  	if (clk->func->init)
>  		return clk->func->init(clk);
>  
> -	clk->astate = NVKM_CLK_PSTATE_DEFAULT;
> +	/* after a resume we have no idea what clocks are set, reset the state
> +	 */
>  	clk->pstate = NULL;
> -	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
>  	clk->cstate = NULL;
> -	clk->temp = 90; /* reasonable default value */
>  	nvkm_clk_update(clk, true);
>  	return 0;
>  }
> @@ -683,8 +682,13 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>  	clk->func = func;
>  	INIT_LIST_HEAD(&clk->states);
>  	clk->domains = func->domains;
> +
> +	clk->astate = NVKM_CLK_PSTATE_DEFAULT;
>  	clk->ustate_ac = -1;
>  	clk->ustate_dc = -1;
> +	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
> +	clk->temp = 90; /* reasonable default value */
> +
>  	clk->allow_reclock = allow_reclock;
>  
>  	INIT_WORK(&clk->work, nvkm_clk_update_work);
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 14/29] therm: Trigger reclock in temperature daemon

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 1574 bytes --]

Reviewed-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

On 2017-09-15 — 17:11, Karol Herbst wrote:
> Depending on the temperature, cstates might become unreachable or the maped
> voltage of a cstate changes. We want to adjust to that.
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Martin Peres <martin.peres-GANU6spQydw@public.gmane.org>
> ---
>  drm/nouveau/nvkm/subdev/therm/base.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drm/nouveau/nvkm/subdev/therm/base.c b/drm/nouveau/nvkm/subdev/therm/base.c
> index 59f01fec..c90b9cfc 100644
> --- a/drm/nouveau/nvkm/subdev/therm/base.c
> +++ b/drm/nouveau/nvkm/subdev/therm/base.c
> @@ -23,6 +23,8 @@
>   */
>  #include "priv.h"
>  
> +#include <subdev/clk.h>
> +
>  int
>  nvkm_therm_temp_get(struct nvkm_therm *therm, int *val)
>  {
> @@ -168,9 +170,15 @@ nvkm_therm_alarm(struct nvkm_alarm *alarm)
>  	int temp;
>  	struct nvkm_therm *therm =
>  	       container_of(alarm, struct nvkm_therm, alarm);
> +	struct nvkm_clk *clk = therm->subdev.device->clk;
> +
>  	if (nvkm_therm_temp_get(therm, &temp) >= 0)
>  		therm->last_temp = temp;
> +
>  	nvkm_therm_update(therm, therm->last_temp, -1);
> +
> +	if (clk)
> +		nvkm_clk_tstate(clk, therm->last_temp);
>  }
>  
>  int
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 16/29] clk: parse thermal policies for throttling thresholds

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 3173 bytes --]

On 2017-09-15 — 17:11, Karol Herbst wrote:
> v2: use min_t
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h |  2 ++
>  drm/nouveau/nvkm/subdev/clk/base.c    | 42 +++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index f35518c3..f5ff1fd9 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -104,6 +104,8 @@ struct nvkm_clk {
>  	struct nvkm_cstate *cstate;
>  	int exp_cstateid;
>  	u8  temp;
> +	u8  max_temp;
> +	u8  relax_temp;

I guess those should be int, similar to the changes you made in patch 1.

>  
>  	bool allow_reclock;
>  #define NVKM_CLK_BOOST_NONE 0x0
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index 54188d2b..54e14936 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -27,6 +27,7 @@
>  #include <subdev/bios/boost.h>
>  #include <subdev/bios/cstep.h>
>  #include <subdev/bios/perf.h>
> +#include <subdev/bios/thermal_policies.h>
>  #include <subdev/bios/vpstate.h>
>  #include <subdev/fb.h>
>  #include <subdev/therm.h>
> @@ -659,6 +660,44 @@ nvkm_clk = {
>  	.fini = nvkm_clk_fini,
>  };
>  
> +static void
> +nvkm_clk_parse_max_temp(struct nvkm_clk *clk)
> +{
> +	struct nvkm_subdev *subdev = &clk->subdev;
> +	struct nvkm_bios *bios = subdev->device->bios;
> +	struct nvbios_thermal_policies_header header;
> +	struct nvbios_thermal_policies_entry entry;
> +	u8 i;
> +	s16 mt = 0xff;
> +	s16 rt = 0xff;

Why 0xff? I mean, sure, there is a high possibility that the GPU won’t like
temperatures above 255℃, but still.

> +
> +	if (nvbios_thermal_policies_parse(bios, &header))
> +		return;
> +
> +	if (!header.ecount)
> +		return;
> +
> +	for (i = 0; i < header.ecount; i++) {
> +		if (nvbios_thermal_policies_entry(bios, &header, i, &entry))
> +			return;
> +
> +		if (entry.mode != 1)
> +			continue;
> +
> +		mt = min_t(s16, mt, (entry.t0 + entry.down_offset) / 32);
> +		rt = min_t(s16, rt, (entry.t0 + entry.up_offset) / 32);
> +	}
> +
> +	if (mt == 0xff || rt == 0xff)
> +		return;
> +
> +	clk->max_temp = mt;
> +	clk->relax_temp = rt;
> +
> +	nvkm_debug(subdev, "setting up sw throttling thresholds (%u/%u°C)\n",
> +		   clk->max_temp, clk->relax_temp);
> +}
> +
>  int
>  nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>  	      int index, bool allow_reclock, struct nvkm_clk *clk)
> @@ -733,6 +772,9 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>  
>  	clk->boost_mode = nvkm_longopt(device->cfgopt, "NvBoost",
>  				       NVKM_CLK_BOOST_NONE);
> +
> +	nvkm_clk_parse_max_temp(clk);
> +
>  	return 0;
>  }
>  
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 15/29] bios: add thermal policies table

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 5671 bytes --]

Looking good. It would be nice to have some defines/enums for the different
modes. Some comments about t0, t1 and t2 would be nice. I saw you are using t0
in patch 16, but I have no idea why use t0 rather than t1 or t2.

Otherwise,
Acked-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

On 2017-09-15 — 17:11, Karol Herbst wrote:
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../include/nvkm/subdev/bios/thermal_policies.h    | 27 ++++++++
>  drm/nouveau/nvkm/subdev/bios/Kbuild                |  1 +
>  drm/nouveau/nvkm/subdev/bios/thermal_policies.c    | 81 ++++++++++++++++++++++
>  3 files changed, 109 insertions(+)
>  create mode 100644 drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
>  create mode 100644 drm/nouveau/nvkm/subdev/bios/thermal_policies.c
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h b/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
> new file mode 100644
> index 00000000..c9215fdd
> --- /dev/null
> +++ b/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
> @@ -0,0 +1,27 @@
> +#ifndef __NVBIOS_THERMAL_POLICIES_H__
> +#define __NVBIOS_THERMAL_POLICIES_H__
> +
> +struct nvbios_thermal_policies_header {
> +	u32 offset;
> +
> +	u8 version;
> +	u8 hlen;
> +	u8 ecount;
> +	u8 elen;
> +};
> +struct nvbios_thermal_policies_entry {
> +	u8  mode;
> +	u16 t0;
> +	u16 t1;
> +	u16 t2;
> +	s16 down_offset;
> +	s16 up_offset;
> +};
> +
> +int nvbios_thermal_policies_parse(struct nvkm_bios *,
> +				  struct nvbios_thermal_policies_header *);
> +int nvbios_thermal_policies_entry(struct nvkm_bios *,
> +				  struct nvbios_thermal_policies_header *,
> +				  u8 idx,
> +				  struct nvbios_thermal_policies_entry *);
> +#endif
> diff --git a/drm/nouveau/nvkm/subdev/bios/Kbuild b/drm/nouveau/nvkm/subdev/bios/Kbuild
> index 6b4f1e06..38f31dd0 100644
> --- a/drm/nouveau/nvkm/subdev/bios/Kbuild
> +++ b/drm/nouveau/nvkm/subdev/bios/Kbuild
> @@ -30,6 +30,7 @@ nvkm-y += nvkm/subdev/bios/shadowramin.o
>  nvkm-y += nvkm/subdev/bios/shadowrom.o
>  nvkm-y += nvkm/subdev/bios/timing.o
>  nvkm-y += nvkm/subdev/bios/therm.o
> +nvkm-y += nvkm/subdev/bios/thermal_policies.o
>  nvkm-y += nvkm/subdev/bios/vmap.o
>  nvkm-y += nvkm/subdev/bios/volt.o
>  nvkm-y += nvkm/subdev/bios/vpstate.o
> diff --git a/drm/nouveau/nvkm/subdev/bios/thermal_policies.c b/drm/nouveau/nvkm/subdev/bios/thermal_policies.c
> new file mode 100644
> index 00000000..5105194e
> --- /dev/null
> +++ b/drm/nouveau/nvkm/subdev/bios/thermal_policies.c
> @@ -0,0 +1,81 @@
> +/*
> + * Copyright 2017 Karol Herbst
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Karol Herbst
> + */
> +#include <subdev/bios.h>
> +#include <subdev/bios/bit.h>
> +#include <subdev/bios/thermal_policies.h>
> +
> +static u32
> +nvbios_thermal_policies_offset(struct nvkm_bios *b)
> +{
> +	struct bit_entry bit_P;
> +
> +	if (!bit_entry(b, 'P', &bit_P)) {
> +		if (bit_P.version == 2 && bit_P.length >= 0x50)
> +			return nvbios_rd32(b, bit_P.offset + 0x50);
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +nvbios_thermal_policies_parse(struct nvkm_bios *b, struct nvbios_thermal_policies_header *h)
> +{
> +	if (!h)
> +		return -EINVAL;
> +
> +	h->offset = nvbios_thermal_policies_offset(b);
> +	if (!h->offset)
> +		return -ENODEV;
> +
> +	h->version = nvbios_rd08(b, h->offset);
> +	switch (h->version) {
> +	case 0x10:
> +		h->hlen     = nvbios_rd08(b, h->offset + 0x1);
> +		h->elen     = nvbios_rd08(b, h->offset + 0x2);
> +		h->ecount   = nvbios_rd08(b, h->offset + 0x3);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +int
> +nvbios_thermal_policies_entry(struct nvkm_bios *b, struct nvbios_thermal_policies_header *h,
> +		     u8 idx, struct nvbios_thermal_policies_entry *e)
> +{
> +	u32 offset;
> +
> +	if (!e || !h || idx > h->ecount)
> +		return -EINVAL;
> +
> +	offset = h->offset + h->hlen + idx * h->elen;
> +	e->mode = nvbios_rd08(b, offset);
> +	e->t0 = nvbios_rd16(b, offset + 0x2);
> +	e->t1 = nvbios_rd16(b, offset + 0x4);
> +	e->t2 = nvbios_rd16(b, offset + 0x6);
> +	e->down_offset = nvbios_rd16(b, offset + 0x12);
> +	e->up_offset = nvbios_rd16(b, offset + 0x14);
> +
> +	return 0;
> +}
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 17/29] clk: thermal throttling

[Pierre Moreau]

[-- Attachment #1.1: Type: text/plain, Size: 4356 bytes --]

Reviewed-by: Pierre Moreau <pierre.morrow-GANU6spQydw@public.gmane.org>

On 2017-09-15 — 17:11, Karol Herbst wrote:
> v2: make message about relaxed throttling an info
>     rework reporting about current clk state
> 
> Signed-off-by: Karol Herbst <karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drm/nouveau/include/nvkm/subdev/clk.h |  1 +
>  drm/nouveau/nvkm/subdev/clk/base.c    | 44 +++++++++++++++++++++++++----------
>  2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
> index f5ff1fd9..b79bf657 100644
> --- a/drm/nouveau/include/nvkm/subdev/clk.h
> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
> @@ -106,6 +106,7 @@ struct nvkm_clk {
>  	u8  temp;
>  	u8  max_temp;
>  	u8  relax_temp;
> +	bool throttled;
>  
>  	bool allow_reclock;
>  #define NVKM_CLK_BOOST_NONE 0x0
> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
> index 54e14936..72ca5e0c 100644
> --- a/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -279,7 +279,7 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
>  	struct nvkm_fb *fb = subdev->device->fb;
>  	struct nvkm_pci *pci = subdev->device->pci;
>  	struct nvkm_pstate *pstate;
> -	int ret;
> +	int ret, cstate;
>  
>  	if (pstateid == NVKM_CLK_PSTATE_DEFAULT)
>  		return 0;
> @@ -308,7 +308,12 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstateid)
>  		ram->func->tidy(ram);
>  	}
>  
> -	return nvkm_cstate_prog(clk, pstate, clk->exp_cstateid);
> +	if (clk->throttled)
> +		cstate = list_first_entry(&pstate->list, struct nvkm_cstate, head)->id;
> +	else
> +		cstate = clk->exp_cstateid;
> +
> +	return nvkm_cstate_prog(clk, pstate, cstate);
>  }
>  
>  static void
> @@ -322,22 +327,20 @@ nvkm_clk_update_work(struct work_struct *work)
>  		return;
>  	clk->pwrsrc = power_supply_is_system_supplied();
>  
> -	if (clk->pstate)
> -		pstate = clk->pstate->pstate;
> -	else
> -		pstate = NVKM_CLK_PSTATE_DEFAULT;
> -	nvkm_trace(subdev, "P %d PWR %d U(AC) %d U(DC) %d A %d C %d T %d°C\n",
> -		   pstate, clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> -		   clk->astate, clk->exp_cstateid, clk->temp);
> -
>  	pstate = clk->pwrsrc ? clk->ustate_ac : clk->ustate_dc;
>  	if (clk->state_nr && pstate != -1) {
> -		pstate = (pstate < 0) ? clk->astate : pstate;
> +		if (clk->throttled)
> +			pstate = list_first_entry(&clk->states, struct nvkm_pstate, head)->pstate;
> +		else
> +			pstate = (pstate < 0) ? clk->astate : pstate;
>  	} else {
>  		pstate = NVKM_CLK_PSTATE_DEFAULT;
>  	}
>  
> -	nvkm_trace(subdev, "-> %d\n", pstate);
> +	nvkm_trace(subdev, "PWR %d U(AC) %d U(DC) %d A %d C %d T %d°C\n",
> +		   clk->pwrsrc, clk->ustate_ac, clk->ustate_dc,
> +		   clk->astate, clk->exp_cstateid, clk->temp);
> +
>  	if (!clk->pstate || pstate != clk->pstate->pstate) {
>  		int ret = nvkm_pstate_prog(clk, pstate);
>  		if (ret) {
> @@ -564,9 +567,25 @@ nvkm_clk_astate(struct nvkm_clk *clk, int req, int rel, bool wait)
>  int
>  nvkm_clk_tstate(struct nvkm_clk *clk, u8 temp)
>  {
> +	struct nvkm_subdev *subdev = &clk->subdev;
>  	if (clk->temp == temp)
>  		return 0;
>  	clk->temp = temp;
> +	if (clk->max_temp && clk->relax_temp) {
> +		if (!clk->throttled && temp > clk->max_temp) {
> +			nvkm_warn(subdev,
> +				  "temperature (%d C) hit the 'downclock' "
> +				  "threshold\n",
> +				  temp);
> +			clk->throttled = true;
> +		} else if (clk->throttled && temp < clk->relax_temp) {
> +			nvkm_info(subdev,
> +				  "temperature (%d C) went below the "
> +				  "'relax' threshold\n",
> +				  temp);
> +			clk->throttled = false;
> +		}
> +	}
>  	return nvkm_clk_update(clk, false);
>  }
>  
> @@ -727,6 +746,7 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>  	clk->ustate_dc = -1;
>  	clk->exp_cstateid = NVKM_CLK_CSTATE_DEFAULT;
>  	clk->temp = 90; /* reasonable default value */
> +	clk->throttled = false;
>  
>  	clk->allow_reclock = allow_reclock;
>  
> -- 
> 2.14.1
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 16/29] clk: parse thermal policies for throttling thresholds

[Karol Herbst]

On Sun, Oct 8, 2017 at 8:36 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> v2: use min_t
>>
>> Signed-off-by: Karol Herbst <karolherbst@gmail.com>
>> ---
>>  drm/nouveau/include/nvkm/subdev/clk.h |  2 ++
>>  drm/nouveau/nvkm/subdev/clk/base.c    | 42 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/clk.h b/drm/nouveau/include/nvkm/subdev/clk.h
>> index f35518c3..f5ff1fd9 100644
>> --- a/drm/nouveau/include/nvkm/subdev/clk.h
>> +++ b/drm/nouveau/include/nvkm/subdev/clk.h
>> @@ -104,6 +104,8 @@ struct nvkm_clk {
>>       struct nvkm_cstate *cstate;
>>       int exp_cstateid;
>>       u8  temp;
>> +     u8  max_temp;
>> +     u8  relax_temp;
>
> I guess those should be int, similar to the changes you made in patch 1.
>
>>
>>       bool allow_reclock;
>>  #define NVKM_CLK_BOOST_NONE 0x0
>> diff --git a/drm/nouveau/nvkm/subdev/clk/base.c b/drm/nouveau/nvkm/subdev/clk/base.c
>> index 54188d2b..54e14936 100644
>> --- a/drm/nouveau/nvkm/subdev/clk/base.c
>> +++ b/drm/nouveau/nvkm/subdev/clk/base.c
>> @@ -27,6 +27,7 @@
>>  #include <subdev/bios/boost.h>
>>  #include <subdev/bios/cstep.h>
>>  #include <subdev/bios/perf.h>
>> +#include <subdev/bios/thermal_policies.h>
>>  #include <subdev/bios/vpstate.h>
>>  #include <subdev/fb.h>
>>  #include <subdev/therm.h>
>> @@ -659,6 +660,44 @@ nvkm_clk = {
>>       .fini = nvkm_clk_fini,
>>  };
>>
>> +static void
>> +nvkm_clk_parse_max_temp(struct nvkm_clk *clk)
>> +{
>> +     struct nvkm_subdev *subdev = &clk->subdev;
>> +     struct nvkm_bios *bios = subdev->device->bios;
>> +     struct nvbios_thermal_policies_header header;
>> +     struct nvbios_thermal_policies_entry entry;
>> +     u8 i;
>> +     s16 mt = 0xff;
>> +     s16 rt = 0xff;
>
> Why 0xff? I mean, sure, there is a high possibility that the GPU won’t like
> temperatures above 255℃, but still.

just a hack to properly check if we set any values we can use and make
available.

>
>> +
>> +     if (nvbios_thermal_policies_parse(bios, &header))
>> +             return;
>> +
>> +     if (!header.ecount)
>> +             return;
>> +
>> +     for (i = 0; i < header.ecount; i++) {
>> +             if (nvbios_thermal_policies_entry(bios, &header, i, &entry))
>> +                     return;
>> +
>> +             if (entry.mode != 1)
>> +                     continue;
>> +
>> +             mt = min_t(s16, mt, (entry.t0 + entry.down_offset) / 32);
>> +             rt = min_t(s16, rt, (entry.t0 + entry.up_offset) / 32);
>> +     }
>> +
>> +     if (mt == 0xff || rt == 0xff)
>> +             return;
>> +
>> +     clk->max_temp = mt;
>> +     clk->relax_temp = rt;
>> +
>> +     nvkm_debug(subdev, "setting up sw throttling thresholds (%u/%u°C)\n",
>> +                clk->max_temp, clk->relax_temp);
>> +}
>> +
>>  int
>>  nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>>             int index, bool allow_reclock, struct nvkm_clk *clk)
>> @@ -733,6 +772,9 @@ nvkm_clk_ctor(const struct nvkm_clk_func *func, struct nvkm_device *device,
>>
>>       clk->boost_mode = nvkm_longopt(device->cfgopt, "NvBoost",
>>                                      NVKM_CLK_BOOST_NONE);
>> +
>> +     nvkm_clk_parse_max_temp(clk);
>> +
>>       return 0;
>>  }
>>
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [RFC PATCH 15/29] bios: add thermal policies table

[Karol Herbst]

On Sun, Oct 8, 2017 at 8:40 PM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> Looking good. It would be nice to have some defines/enums for the different
> modes. Some comments about t0, t1 and t2 would be nice. I saw you are using t0
> in patch 16, but I have no idea why use t0 rather than t1 or t2.
>

I think I will remove t1 and t2 for now, because I have no idea when
they are used. I also have no idea what those modes are doing except
changing things in a strange way. The only thing I know for sure is
the mode == 1 case and the throttle limits. This table also defines
how fast we should throttle, but for now, until we figure out more, we
go with the jump to min immediately approach.

> Otherwise,
> Acked-by: Pierre Moreau <pierre.morrow@free.fr>
>
> On 2017-09-15 — 17:11, Karol Herbst wrote:
>> Signed-off-by: Karol Herbst <karolherbst@gmail.com>
>> ---
>>  .../include/nvkm/subdev/bios/thermal_policies.h    | 27 ++++++++
>>  drm/nouveau/nvkm/subdev/bios/Kbuild                |  1 +
>>  drm/nouveau/nvkm/subdev/bios/thermal_policies.c    | 81 ++++++++++++++++++++++
>>  3 files changed, 109 insertions(+)
>>  create mode 100644 drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
>>  create mode 100644 drm/nouveau/nvkm/subdev/bios/thermal_policies.c
>>
>> diff --git a/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h b/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
>> new file mode 100644
>> index 00000000..c9215fdd
>> --- /dev/null
>> +++ b/drm/nouveau/include/nvkm/subdev/bios/thermal_policies.h
>> @@ -0,0 +1,27 @@
>> +#ifndef __NVBIOS_THERMAL_POLICIES_H__
>> +#define __NVBIOS_THERMAL_POLICIES_H__
>> +
>> +struct nvbios_thermal_policies_header {
>> +     u32 offset;
>> +
>> +     u8 version;
>> +     u8 hlen;
>> +     u8 ecount;
>> +     u8 elen;
>> +};
>> +struct nvbios_thermal_policies_entry {
>> +     u8  mode;
>> +     u16 t0;
>> +     u16 t1;
>> +     u16 t2;
>> +     s16 down_offset;
>> +     s16 up_offset;
>> +};
>> +
>> +int nvbios_thermal_policies_parse(struct nvkm_bios *,
>> +                               struct nvbios_thermal_policies_header *);
>> +int nvbios_thermal_policies_entry(struct nvkm_bios *,
>> +                               struct nvbios_thermal_policies_header *,
>> +                               u8 idx,
>> +                               struct nvbios_thermal_policies_entry *);
>> +#endif
>> diff --git a/drm/nouveau/nvkm/subdev/bios/Kbuild b/drm/nouveau/nvkm/subdev/bios/Kbuild
>> index 6b4f1e06..38f31dd0 100644
>> --- a/drm/nouveau/nvkm/subdev/bios/Kbuild
>> +++ b/drm/nouveau/nvkm/subdev/bios/Kbuild
>> @@ -30,6 +30,7 @@ nvkm-y += nvkm/subdev/bios/shadowramin.o
>>  nvkm-y += nvkm/subdev/bios/shadowrom.o
>>  nvkm-y += nvkm/subdev/bios/timing.o
>>  nvkm-y += nvkm/subdev/bios/therm.o
>> +nvkm-y += nvkm/subdev/bios/thermal_policies.o
>>  nvkm-y += nvkm/subdev/bios/vmap.o
>>  nvkm-y += nvkm/subdev/bios/volt.o
>>  nvkm-y += nvkm/subdev/bios/vpstate.o
>> diff --git a/drm/nouveau/nvkm/subdev/bios/thermal_policies.c b/drm/nouveau/nvkm/subdev/bios/thermal_policies.c
>> new file mode 100644
>> index 00000000..5105194e
>> --- /dev/null
>> +++ b/drm/nouveau/nvkm/subdev/bios/thermal_policies.c
>> @@ -0,0 +1,81 @@
>> +/*
>> + * Copyright 2017 Karol Herbst
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + * Authors: Karol Herbst
>> + */
>> +#include <subdev/bios.h>
>> +#include <subdev/bios/bit.h>
>> +#include <subdev/bios/thermal_policies.h>
>> +
>> +static u32
>> +nvbios_thermal_policies_offset(struct nvkm_bios *b)
>> +{
>> +     struct bit_entry bit_P;
>> +
>> +     if (!bit_entry(b, 'P', &bit_P)) {
>> +             if (bit_P.version == 2 && bit_P.length >= 0x50)
>> +                     return nvbios_rd32(b, bit_P.offset + 0x50);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +int
>> +nvbios_thermal_policies_parse(struct nvkm_bios *b, struct nvbios_thermal_policies_header *h)
>> +{
>> +     if (!h)
>> +             return -EINVAL;
>> +
>> +     h->offset = nvbios_thermal_policies_offset(b);
>> +     if (!h->offset)
>> +             return -ENODEV;
>> +
>> +     h->version = nvbios_rd08(b, h->offset);
>> +     switch (h->version) {
>> +     case 0x10:
>> +             h->hlen     = nvbios_rd08(b, h->offset + 0x1);
>> +             h->elen     = nvbios_rd08(b, h->offset + 0x2);
>> +             h->ecount   = nvbios_rd08(b, h->offset + 0x3);
>> +             return 0;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +}
>> +
>> +int
>> +nvbios_thermal_policies_entry(struct nvkm_bios *b, struct nvbios_thermal_policies_header *h,
>> +                  u8 idx, struct nvbios_thermal_policies_entry *e)
>> +{
>> +     u32 offset;
>> +
>> +     if (!e || !h || idx > h->ecount)
>> +             return -EINVAL;
>> +
>> +     offset = h->offset + h->hlen + idx * h->elen;
>> +     e->mode = nvbios_rd08(b, offset);
>> +     e->t0 = nvbios_rd16(b, offset + 0x2);
>> +     e->t1 = nvbios_rd16(b, offset + 0x4);
>> +     e->t2 = nvbios_rd16(b, offset + 0x6);
>> +     e->down_offset = nvbios_rd16(b, offset + 0x12);
>> +     e->up_offset = nvbios_rd16(b, offset + 0x14);
>> +
>> +     return 0;
>> +}
>> --
>> 2.14.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau