Current State of my clk patches

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 <[email protected]>

---
 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(-)

drm/nouveau/include/nvkm/subdev/therm.h +1 -1

index 9841f076..8c84017f 100644
@@ -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);
 

drm/nouveau/nouveau_hwmon.c +9 -6

index 7c965648..f1914d9a 100644
@@ -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;

drm/nouveau/nvkm/subdev/therm/base.c +14 -5

index f27fc6d0..8fa691fb 100644
@@ -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)

drm/nouveau/nvkm/subdev/therm/g84.c +1 -1

index 96f8da40..3bb2d829 100644
@@ -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)

drm/nouveau/nvkm/subdev/therm/g84.c +0 -0

 	if (new_state == NVKM_THERM_THRS_LOWER && cur > thrs->temp)
 		new_state = NVKM_THERM_THRS_HIGHER;
 	else if (new_state == NVKM_THERM_THRS_HIGHER &&

drm/nouveau/nvkm/subdev/therm/gp100.c +5 -4

index 9f0dea3f..d8206748 100644
@@ -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;
 }
 

drm/nouveau/nvkm/subdev/therm/nv40.c +3 -6

index 2c92ffb5..cfd5b215 100644
@@ -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

drm/nouveau/nvkm/subdev/therm/nv50.c +3 -6

index 9b57b433..62ec4063 100644
@@ -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

drm/nouveau/nvkm/subdev/therm/priv.h +2 -2

index 1f46e371..b325ec5f 100644
@@ -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 *);
 

drm/nouveau/nvkm/subdev/therm/temp.c +12 -4

index ddb2b2c6..5ec2dfb3 100644
@@ -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);

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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:

Quoted 4 lines

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.

Quoted 172 lines

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 <[email protected]>

---
 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(-)

drm/nouveau/include/nvkm/subdev/therm.h +1 -1

index 9841f076..8c84017f 100644
@@ -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);
 

drm/nouveau/nouveau_hwmon.c +9 -6

index 7c965648..f1914d9a 100644
@@ -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;

drm/nouveau/nvkm/subdev/therm/base.c +14 -5

index f27fc6d0..8fa691fb 100644
@@ -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)

drm/nouveau/nvkm/subdev/therm/g84.c +1 -1

index 96f8da40..3bb2d829 100644
@@ -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;
 

drm/nouveau/nvkm/subdev/therm/g84.c line 37

Quoted 7 lines

drm/nouveau/nvkm/subdev/therm/g84.c +3 -3

-     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.

drm/nouveau/nvkm/subdev/therm/g84.c g84_therm_threshold_hyst_emulation(struct nvkm_therm *therm, · line 119

Quoted 11 lines

drm/nouveau/nvkm/subdev/therm/g84.c +2 -1

+     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).

Quoted 163 lines

drm/nouveau/nvkm/subdev/therm/g84.c +0 -0

      if (new_state == NVKM_THERM_THRS_LOWER && cur > thrs->temp)
              new_state = NVKM_THERM_THRS_HIGHER;
      else if (new_state == NVKM_THERM_THRS_HIGHER &&

drm/nouveau/nvkm/subdev/therm/gp100.c +5 -4

index 9f0dea3f..d8206748 100644
@@ -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;
 }
 

drm/nouveau/nvkm/subdev/therm/nv40.c +3 -6

index 2c92ffb5..cfd5b215 100644
@@ -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

drm/nouveau/nvkm/subdev/therm/nv50.c +3 -6

index 9b57b433..62ec4063 100644
@@ -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

drm/nouveau/nvkm/subdev/therm/priv.h +2 -2

index 1f46e371..b325ec5f 100644
@@ -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 *);
 

drm/nouveau/nvkm/subdev/therm/temp.c +12 -4

index ddb2b2c6..5ec2dfb3 100644
@@ -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";
 

Quoted 8 lines

drm/nouveau/nvkm/subdev/therm/temp.c +0 -1

      nvkm_debug(&therm->subdev, "internal sensor: %s\n", sensor_avail);
--

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Otherwise hwmon interprets error codes as real values.

Signed-off-by Karol Herbst <[email protected]>

---
 drm/nouveau/nouveau_hwmon.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

drm/nouveau/nouveau_hwmon.c +26 -7

index f1914d9a..0d75c14d 100644
@@ -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:

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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 <[email protected]>

---
 drm/nouveau/nvkm/subdev/volt/gk104.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drm/nouveau/nvkm/subdev/volt/gk104.c +6 -1

index 1c744e02..53a7af9d 100644
@@ -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;
 }

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Quoted 4 lines · 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.

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

Reviewed-by Pierre Moreau <[email protected]>

Quoted 30 lines

Signed-off-by Karol Herbst <[email protected]>

---
 drm/nouveau/nvkm/subdev/volt/gk104.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drm/nouveau/nvkm/subdev/volt/gk104.c +6 -2

index 1c744e02..53a7af9d 100644
@@ -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

Quoted 4 lines

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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 <[email protected]>

---
 drm/nouveau/include/nvkm/subdev/clk.h |  1 +
 drm/nouveau/nvkm/subdev/clk/base.c    | 26 ++++++++++++++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

drm/nouveau/include/nvkm/subdev/clk.h +1 -0

index e5275f74..ce3bbcfe 100644
@@ -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 **);

drm/nouveau/nvkm/subdev/clk/base.c +13 -7

index e4c8d310..ecff3ff3 100644
@@ -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

drm/nouveau/nvkm/subdev/clk/base.c +1 -1

 }
 

@@ -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);
 

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Quoted 93 lines · 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 <[email protected]>

---
 drm/nouveau/include/nvkm/subdev/clk.h |  1 +
 drm/nouveau/nvkm/subdev/clk/base.c    | 26 ++++++++++++++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

drm/nouveau/include/nvkm/subdev/clk.h +1 -0

index e5275f74..ce3bbcfe 100644
@@ -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 **);

drm/nouveau/nvkm/subdev/clk/base.c +13 -7

index e4c8d310..ecff3ff3 100644
@@ -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);
 }
 

drm/nouveau/nvkm/subdev/clk/base.c nvkm_clk_pwrsrc(struct nvkm_notify *notify) · line 573

Quoted 8 lines

drm/nouveau/nvkm/subdev/clk/base.c +1 -1

 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?

drm/nouveau/nvkm/subdev/clk/base.c nvkm_clk_init(struct nvkm_subdev *subdev) · line 628

Quoted 9 lines

drm/nouveau/nvkm/subdev/clk/base.c +1 -1

 }
 

@@ -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?

Quoted 12 lines

drm/nouveau/nvkm/subdev/clk/base.c +1 -1

 }
 

@@ -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);
 

Quoted 7 lines

--
2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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 <[email protected]>

Reviewed-by Martin Peres <[email protected]>

---
 drm/nouveau/include/nvkm/subdev/clk.h |  2 --
 drm/nouveau/nvkm/subdev/clk/base.c    | 16 ++--------------
 2 files changed, 2 insertions(+), 16 deletions(-)

drm/nouveau/include/nvkm/subdev/clk.h +0 -2

index ce3bbcfe..1340f5b8 100644
@@ -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);
 

drm/nouveau/nvkm/subdev/clk/base.c +2 -14

index ecff3ff3..07d530ed 100644
@@ -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);

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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 <[email protected]>

Reviewed-by Martin Peres <[email protected]>

---
 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(-)

drm/nouveau/include/nvkm/subdev/clk.h +3 -1

index 1340f5b8..ec537e08 100644
@@ -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) */

drm/nouveau/nvkm/engine/device/ctrl.c +4 -1

index b0ece71a..da70626c 100644
@@ -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;

drm/nouveau/nvkm/subdev/clk/base.c +4 -1

index 07d530ed..0d4d9fdf 100644
@@ -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();
 

drm/nouveau/nvkm/subdev/clk/base.c +2 -2

 	}
 
 	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;

drm/nouveau/nvkm/subdev/pmu/gk20a.c +7 -11

index 05e81855..de579726 100644
@@ -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);

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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).

Quoted 78 lines

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 <[email protected]>

Reviewed-by Martin Peres <[email protected]>

---
 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(-)

drm/nouveau/include/nvkm/subdev/clk.h +3 -1

index 1340f5b8..ec537e08 100644
@@ -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) */

drm/nouveau/nvkm/engine/device/ctrl.c +4 -1

index b0ece71a..da70626c 100644
@@ -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;

drm/nouveau/nvkm/subdev/clk/base.c +4 -1

index 07d530ed..0d4d9fdf 100644
@@ -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();
 

drm/nouveau/nvkm/subdev/clk/base.c nvkm_clk_update_work(struct work_struct *work) · line 320

Quoted 10 lines

drm/nouveau/nvkm/subdev/clk/base.c +5 -1

+     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.

drm/nouveau/nvkm/subdev/clk/base.c nvkm_clk_update_work(struct work_struct *work) · line 322

Quoted 2 lines

drm/nouveau/nvkm/subdev/clk/base.c +0 -0

@@ -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?

drm/nouveau/nvkm/subdev/clk/base.c nvkm_clk_update_work(struct work_struct *work) · line 325

Quoted 5 lines

drm/nouveau/nvkm/subdev/clk/base.c +1 -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.

Quoted 65 lines

drm/nouveau/nvkm/subdev/clk/base.c +2 -2

      }
 
      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;

drm/nouveau/nvkm/subdev/pmu/gk20a.c +7 -12

index 05e81855..de579726 100644
@@ -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

Quoted 4 lines

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

Signed-off-by Karol Herbst <[email protected]>

Reviewed-by Martin Peres <[email protected]>

---
 drm/nouveau/include/nvkm/subdev/clk.h |  2 ++
 drm/nouveau/nvkm/subdev/clk/base.c    | 32 +++++++++++++++++++++++++-------
 2 files changed, 27 insertions(+), 7 deletions(-)

drm/nouveau/include/nvkm/subdev/clk.h +2 -0

index ec537e08..f35518c3 100644
@@ -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;

drm/nouveau/nvkm/subdev/clk/base.c +25 -7

index 0d4d9fdf..d37c13b7 100644
@@ -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);

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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 <[email protected]>

Reviewed-by Martin Peres <[email protected]>

---
 drm/nouveau/nouveau_debugfs.c      |  6 +--
 drm/nouveau/nvkm/subdev/clk/base.c | 78 +++++++++++++++++++-------------------
 2 files changed, 41 insertions(+), 43 deletions(-)

drm/nouveau/nouveau_debugfs.c +3 -3

index 963a4dba..27281c4e 100644
@@ -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)

drm/nouveau/nvkm/subdev/clk/base.c +9 -31

index d37c13b7..1d71bf09 100644
@@ -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);

drm/nouveau/nvkm/subdev/clk/base.c +6 -0

+
+	list_for_each_entry(pstate, &clk->states, head) {
+		if (pstate->pstate == ret)
+			return ret;
+	}
+	return -EINVAL;
 }
 
 int

drm/nouveau/nvkm/subdev/clk/base.c +18 -2

+
+	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;

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Quoted 3 lines · 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.

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

Quoted 119 lines

v2: reword commit message

Signed-off-by Karol Herbst <[email protected]>

Reviewed-by Martin Peres <[email protected]>

---
 drm/nouveau/nouveau_debugfs.c      |  6 +--
 drm/nouveau/nvkm/subdev/clk/base.c | 78 +++++++++++++++++++-------------------
 2 files changed, 41 insertions(+), 43 deletions(-)

drm/nouveau/nouveau_debugfs.c +3 -3

index 963a4dba..27281c4e 100644
@@ -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)

drm/nouveau/nvkm/subdev/clk/base.c +10 -31

index d37c13b7..1d71bf09 100644
@@ -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;
      }
 

drm/nouveau/nvkm/subdev/clk/base.c nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen) · line 518

Quoted 3 lines

drm/nouveau/nvkm/subdev/clk/base.c +2 -1

-     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()?

Quoted 9 lines

drm/nouveau/nvkm/subdev/clk/base.c +6 -0

+
+     list_for_each_entry(pstate, &clk->states, head) {
+             if (pstate->pstate == ret)
+                     return ret;
+     }
+     return -EINVAL;
 }
 
 int

drm/nouveau/nvkm/subdev/clk/base.c nvkm_clk_nstate(struct nvkm_clk *clk, const char *mode, int arglen) · line 531

Quoted 10 lines

drm/nouveau/nvkm/subdev/clk/base.c +2 -6

 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()?

Quoted 34 lines

drm/nouveau/nvkm/subdev/clk/base.c +18 -3

+
+     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

Quoted 4 lines

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

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 <[email protected]>

Reviewed-by Martin Peres <[email protected]>

---
 drm/nouveau/nvkm/subdev/clk/base.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drm/nouveau/nvkm/subdev/clk/base.c +7 -3

index 1d71bf09..54188d2b 100644
@@ -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);

Signature · 6 lines

2.14.1

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau