Skip to contentSkip to conversation

RFC [PATCH 0/3] Expose power budget cap via hwmon

PATCH In review Karol Herbst 2 participants 11 messages Oct 24, 2016 – Oct 25, 2016 7 messages not in the archive

lore
Share & tools
lore

Karol Herbst

author
#0 lore raw

There is an optinal header field in the power budget table we can use to read out the power cap of the GPU.

Sadly it is optional and if that field isn't sad, things beome complicated.

Anyhow, this is good enough for most cards and we can use it later for capping the power consumption of the GPUs, but first, just export those values through hwmon.

First design, will change stuff, want comments. Thanks.

Karol Herbst (3):
nvbios/power_budget: Add basic power budget parsing subdev/iccsense: Parse max and crit power level
hwmon: expose power_max and power_crit

 .../include/nvkm/subdev/bios/power_budget.h        |  20 ++++
 drm/nouveau/include/nvkm/subdev/iccsense.h         |   3 +
 drm/nouveau/nouveau_hwmon.c                        |  44 +++++++++
 drm/nouveau/nvkm/subdev/bios/Kbuild                |   1 +
 drm/nouveau/nvkm/subdev/bios/power_budget.c        | 108 +++++++++++++++++++++
 drm/nouveau/nvkm/subdev/iccsense/base.c            |  13 ++-
 6 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 drm/nouveau/include/nvkm/subdev/bios/power_budget.h
 create mode 100644 drm/nouveau/nvkm/subdev/bios/power_budget.c
Signature · 6 lines
2.10.0

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

A message that is not in the archive (sent off-list or not yet received): 20161024211113.5876-1-karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

author
#2 lore raw

PATCH 1/3 nvbios/power_budget: Add basic power budget parsing

Signed-off-by Karol Herbst <[email protected]>
---
 .../include/nvkm/subdev/bios/power_budget.h        |  20 ++++
 drm/nouveau/nvkm/subdev/bios/Kbuild                |   1 +
 drm/nouveau/nvkm/subdev/bios/power_budget.c        | 108 +++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drm/nouveau/include/nvkm/subdev/bios/power_budget.h
 create mode 100644 drm/nouveau/nvkm/subdev/bios/power_budget.c
drm/nouveau/include/nvkm/subdev/bios/power_budget.h new file +20 -0
new file mode 100644
index 0000000..dd65c08
@@ -0,0 +1,20 @@
+#ifndef __NVBIOS_POWER_BUDGET_H__
+#define __NVBIOS_POWER_BUDGET_H__
+
+#include <nvkm/subdev/bios.h>
+
+struct nvbios_power_budget_entry {
+	u32 min_w;
+	u32 avg_w;
+	u32 max_w;
+};
+
+struct nvbios_power_budget {
+	u8  nr_entry;
+	u8  cap_entry;
+	struct nvbios_power_budget_entry *entries;
+};
+
+int nvbios_power_budget_parse(struct nvkm_bios *, struct nvbios_power_budget *);
+
+#endif
drm/nouveau/nvkm/subdev/bios/Kbuild +1 -0
index be57220..6b4f1e0 100644
@@ -19,6 +19,7 @@ nvkm-y += nvkm/subdev/bios/pcir.o
 nvkm-y += nvkm/subdev/bios/perf.o
 nvkm-y += nvkm/subdev/bios/pll.o
 nvkm-y += nvkm/subdev/bios/pmu.o
+nvkm-y += nvkm/subdev/bios/power_budget.o
 nvkm-y += nvkm/subdev/bios/ramcfg.o
 nvkm-y += nvkm/subdev/bios/rammap.o
 nvkm-y += nvkm/subdev/bios/shadow.o
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +108 -0

108 lines not on this page · raw message

Signature · 6 lines
2.10.0

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

A message that is not in the archive (sent off-list or not yet received): 20161024211113.5876-2-karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

#4 lore raw

Re: [PATCH 1/3] nvbios/power_budget: Add basic power budget parsing

Quoted 123 lines · Karol Herbst
Signed-off-by Karol Herbst <[email protected]>
---
 .../include/nvkm/subdev/bios/power_budget.h        |  20 ++++
 drm/nouveau/nvkm/subdev/bios/Kbuild                |   1 +
 drm/nouveau/nvkm/subdev/bios/power_budget.c        | 108 +++++++++++++++++++++
 3 files changed, 129 insertions(+)
 create mode 100644 drm/nouveau/include/nvkm/subdev/bios/power_budget.h
 create mode 100644 drm/nouveau/nvkm/subdev/bios/power_budget.c
drm/nouveau/include/nvkm/subdev/bios/power_budget.h new file +20 -0
new file mode 100644
index 0000000..dd65c08
@@ -0,0 +1,20 @@
+#ifndef __NVBIOS_POWER_BUDGET_H__
+#define __NVBIOS_POWER_BUDGET_H__
+
+#include <nvkm/subdev/bios.h>
+
+struct nvbios_power_budget_entry {
+	u32 min_w;
+	u32 avg_w;
+	u32 max_w;
+};
+
+struct nvbios_power_budget {
+	u8  nr_entry;
+	u8  cap_entry;
+	struct nvbios_power_budget_entry *entries;
+};
+
+int nvbios_power_budget_parse(struct nvkm_bios *, struct nvbios_power_budget *);
+
+#endif
drm/nouveau/nvkm/subdev/bios/Kbuild +1 -0
index be57220..6b4f1e0 100644
@@ -19,6 +19,7 @@ nvkm-y += nvkm/subdev/bios/pcir.o
 nvkm-y += nvkm/subdev/bios/perf.o
 nvkm-y += nvkm/subdev/bios/pll.o
 nvkm-y += nvkm/subdev/bios/pmu.o
+nvkm-y += nvkm/subdev/bios/power_budget.o
 nvkm-y += nvkm/subdev/bios/ramcfg.o
 nvkm-y += nvkm/subdev/bios/rammap.o
 nvkm-y += nvkm/subdev/bios/shadow.o
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +70 -0
new file mode 100644
index 0000000..538497b
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2015 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/power_budget.h>
+
+static u32
+nvbios_power_budget_table(struct nvkm_bios *bios, u8 *ver, u8 *hdr, u8 *cnt,
+			  u8 *len)
+{
+	struct bit_entry bit_P;
+	u32 power_budget;
+
+	if (bit_entry(bios, 'P', &bit_P) || bit_P.version != 2 ||
+	    bit_P.length < 0x2c)
+		return 0;
+
+	power_budget = nvbios_rd32(bios, bit_P.offset + 0x2c);
+	if (!power_budget)
+		return 0;
+
+	*ver = nvbios_rd08(bios, power_budget);
+	switch (*ver) {
+	case 0x10:
+	case 0x20:
+	case 0x30:
+		*hdr = nvbios_rd08(bios, power_budget + 0x1);
+		*len = nvbios_rd08(bios, power_budget + 0x2);
+		*cnt = nvbios_rd08(bios, power_budget + 0x3);
+		return power_budget;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+int
+nvbios_power_budget_parse(struct nvkm_bios *bios, struct nvbios_power_budget *budget)
+{
+	struct nvkm_subdev *subdev = &bios->subdev;
+	u8 ver, hdr, cnt, len, i, cap_entry;
+	u32 header;
+
+	header = nvbios_power_budget_table(bios, &ver, &hdr, &cnt, &len);
+	if (!header || !cnt)
+		return -ENODEV;
+
+	budget->entries = kmalloc_array(cnt, sizeof(*budget->entries), GFP_KERNEL);

drm/nouveau/nvkm/subdev/bios/power_budget.c line 80

Quoted 10 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +10 -0
+	if (!budget->entries)
+		return -ENOMEM;
+
+	budget->nr_entry = cnt;
+	switch (ver) {
+	case 0x20:
+		cap_entry = nvbios_rd08(bios, header + 0x9);
+		break;
+	default:
+		cap_entry = 0;

Are you sure about this? How about setting it to 0xff instead?

Quoted 19 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +18 -0
+	}
+
+	if (cap_entry < cnt)
+		budget->cap_entry = cap_entry;
+	else {
+		if (cap_entry != 0xff)
+			nvkm_warn(subdev,
+				  "invalid cap_entry in power budget table found\n");
+		budget->cap_entry = 0xff;
+	}
+
+	for (i = 0; i < cnt; ++i) {
+		u32 entry_offset = header + hdr + i * len;
+		struct nvbios_power_budget_entry *entry = &budget->entries[i];
+
+		if (len >= 0xe) {
+			entry->min_w = nvbios_rd32(bios, entry_offset + 0x2);
+			entry->avg_w = nvbios_rd32(bios, entry_offset + 0x6);

drm/nouveau/nvkm/subdev/bios/power_budget.c line 108

Quoted 10 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +10 -0
+			entry->max_w = nvbios_rd32(bios, entry_offset + 0xa);
+		} else {
+			entry->min_w = 0;
+			entry->max_w = nvbios_rd32(bios, entry_offset + 0x2);
+			entry->avg_w = entry->max_w;
+		}
+	}
+
+	return 0;
+}

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

A message that is not in the archive (sent off-list or not yet received): 992054de-6e21-0edb-ade2-ab135787d461-GANU6spQydw@public.gmane.org

author
#6 lore raw

Re: [PATCH 1/3] nvbios/power_budget: Add basic power budget parsing

Quoted 146 lines · Martin Peres
Quoted 5 lines · Karol Herbst
Signed-off-by Karol Herbst <[email protected]>
---
 .../include/nvkm/subdev/bios/power_budget.h        |  20 ++++
 drm/nouveau/nvkm/subdev/bios/Kbuild                |   1 +
 drm/nouveau/nvkm/subdev/bios/power_budget.c        | 108

+++++++++++++++++++++

Quoted 2 lines
 3 files changed, 129 insertions(+)
 create mode 100644

drm/nouveau/include/nvkm/subdev/bios/power_budget.h

Review

Quoted 3 lines
 create mode 100644 drm/nouveau/nvkm/subdev/bios/power_budget.c
(quoted diff) +0 -0

b/drm/nouveau/include/nvkm/subdev/bios/power_budget.h

Quoted 13 lines

new file mode 100644
index 0000000..dd65c08

drm/nouveau/include/nvkm/subdev/bios/power_budget.h new file +8 -0
@@ -0,0 +1,20 @@
+#ifndef __NVBIOS_POWER_BUDGET_H__
+#define __NVBIOS_POWER_BUDGET_H__
+
+#include <nvkm/subdev/bios.h>
+
+struct nvbios_power_budget_entry {
+	u32 min_w;
+	u32 avg_w;

drm/nouveau/include/nvkm/subdev/bios/power_budget.h line 18

Quoted 10 lines
drm/nouveau/include/nvkm/subdev/bios/power_budget.h new file +10 -0
+	u32 max_w;
+};
+
+struct nvbios_power_budget {
+	u8  nr_entry;
+	u8  cap_entry;
+	struct nvbios_power_budget_entry *entries;
+};
+
+int nvbios_power_budget_parse(struct nvkm_bios *, struct

nvbios_power_budget *);

Review

Quoted 3 lines
drm/nouveau/include/nvkm/subdev/bios/power_budget.h new file +2 -0
+
+#endif
(quoted diff) +0 -0

b/drm/nouveau/nvkm/subdev/bios/Kbuild

Review

Quoted 12 lines

index be57220..6b4f1e0 100644

drm/nouveau/nvkm/subdev/bios/Kbuild +1 -0
@@ -19,6 +19,7 @@ nvkm-y += nvkm/subdev/bios/pcir.o
 nvkm-y += nvkm/subdev/bios/perf.o
 nvkm-y += nvkm/subdev/bios/pll.o
 nvkm-y += nvkm/subdev/bios/pmu.o
+nvkm-y += nvkm/subdev/bios/power_budget.o
 nvkm-y += nvkm/subdev/bios/ramcfg.o
 nvkm-y += nvkm/subdev/bios/rammap.o
 nvkm-y += nvkm/subdev/bios/shadow.o
(quoted diff) +0 -0

b/drm/nouveau/nvkm/subdev/bios/power_budget.c

drm/nouveau/nvkm/subdev/bios/power_budget.c line 4

Quoted 9 lines

new file mode 100644
index 0000000..538497b

drm/nouveau/nvkm/subdev/bios/power_budget.c new file +4 -0
@@ -0,0 +1,108 @@
+/*
+ * Copyright 2015 Karol Herbst
+ *
+ * Permission is hereby granted, free of charge, to any person

obtaining a

drm/nouveau/nvkm/subdev/bios/power_budget.c line 5

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * copy of this software and associated documentation files (the

"Software"),

drm/nouveau/nvkm/subdev/bios/power_budget.c line 6

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * to deal in the Software without restriction, including without

limitation

drm/nouveau/nvkm/subdev/bios/power_budget.c line 7

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * the rights to use, copy, modify, merge, publish, distribute,

sublicense,

drm/nouveau/nvkm/subdev/bios/power_budget.c line 8

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * and/or sell copies of the Software, and to permit persons to whom

the

drm/nouveau/nvkm/subdev/bios/power_budget.c line 9

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * Software is furnished to do so, subject to the following

conditions:

drm/nouveau/nvkm/subdev/bios/power_budget.c line 11

Quoted 2 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +2 -0
+ *
+ * The above copyright notice and this permission notice shall be

included in

drm/nouveau/nvkm/subdev/bios/power_budget.c

Quoted 3 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +3 -0
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,

EXPRESS OR

drm/nouveau/nvkm/subdev/bios/power_budget.c line 15

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF

MERCHANTABILITY,

drm/nouveau/nvkm/subdev/bios/power_budget.c line 16

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO

EVENT SHALL

drm/nouveau/nvkm/subdev/bios/power_budget.c line 17

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM,

DAMAGES OR

drm/nouveau/nvkm/subdev/bios/power_budget.c line 18

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR

OTHERWISE,

drm/nouveau/nvkm/subdev/bios/power_budget.c line 19

Quoted 1 line
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +1 -0
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE

USE OR

drm/nouveau/nvkm/subdev/bios/power_budget.c

Quoted 10 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +10 -0
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: Karol Herbst
+ */
+#include <subdev/bios.h>
+#include <subdev/bios/bit.h>
+#include <subdev/bios/power_budget.h>
+
+static u32
+nvbios_power_budget_table(struct nvkm_bios *bios, u8 *ver, u8 *hdr,

u8 *cnt,

Quoted 21 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +21 -0
+			  u8 *len)
+{
+	struct bit_entry bit_P;
+	u32 power_budget;
+
+	if (bit_entry(bios, 'P', &bit_P) || bit_P.version != 2 ||
+	    bit_P.length < 0x2c)
+		return 0;
+
+	power_budget = nvbios_rd32(bios, bit_P.offset + 0x2c);
+	if (!power_budget)
+		return 0;
+
+	*ver = nvbios_rd08(bios, power_budget);
+	switch (*ver) {
+	case 0x10:
+	case 0x20:
+	case 0x30:
+		*hdr = nvbios_rd08(bios, power_budget + 0x1);
+		*len = nvbios_rd08(bios, power_budget + 0x2);
+		*cnt = nvbios_rd08(bios, power_budget + 0x3);

drm/nouveau/nvkm/subdev/bios/power_budget.c

Quoted 10 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +10 -0
+		return power_budget;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+int
+nvbios_power_budget_parse(struct nvkm_bios *bios, struct

nvbios_power_budget *budget)

drm/nouveau/nvkm/subdev/bios/power_budget.c

Quoted 10 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +10 -0
+{
+	struct nvkm_subdev *subdev = &bios->subdev;
+	u8 ver, hdr, cnt, len, i, cap_entry;
+	u32 header;
+
+	header = nvbios_power_budget_table(bios, &ver, &hdr, &cnt, &len);
+	if (!header || !cnt)
+		return -ENODEV;
+
+	budget->entries = kmalloc_array(cnt, sizeof(*budget->entries),

GFP_KERNEL);

Quoted 13 lines

drm/nouveau/nvkm/subdev/bios/power_budget.c line 80

Quoted 10 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +10 -0
+	if (!budget->entries)
+		return -ENOMEM;
+
+	budget->nr_entry = cnt;
+	switch (ver) {
+	case 0x20:
+		cap_entry = nvbios_rd08(bios, header + 0x9);
+		break;
+	default:
+		cap_entry = 0;

Are you sure about this? How about setting it to 0xff instead?

right, has to be 0xff

Quoted 21 lines
Quoted 21 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +21 -0
+	}
+
+	if (cap_entry < cnt)
+		budget->cap_entry = cap_entry;
+	else {
+		if (cap_entry != 0xff)
+			nvkm_warn(subdev,
+				  "invalid cap_entry in power budget table found\n");
+		budget->cap_entry = 0xff;
+	}
+
+	for (i = 0; i < cnt; ++i) {
+		u32 entry_offset = header + hdr + i * len;
+		struct nvbios_power_budget_entry *entry = &budget->entries[i];
+
+		if (len >= 0xe) {
+			entry->min_w = nvbios_rd32(bios, entry_offset + 0x2);
+			entry->avg_w = nvbios_rd32(bios, entry_offset + 0x6);
+			entry->max_w = nvbios_rd32(bios, entry_offset + 0xa);
+		} else {
+			entry->min_w = 0;
Quoted 8 lines

drm/nouveau/nvkm/subdev/bios/power_budget.c line 108

Quoted 8 lines
drm/nouveau/nvkm/subdev/bios/power_budget.c new file +7 -0
+			entry->max_w = nvbios_rd32(bios, entry_offset + 0x2);
+			entry->avg_w = entry->max_w;
+		}
+	}
+
+	return 0;
+}

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

author
#7 lore raw

PATCH 2/3 subdev/iccsense: Parse max and crit power level

In reply to a message not in the archive
Signed-off-by Karol Herbst <[email protected]>
---
 drm/nouveau/include/nvkm/subdev/iccsense.h |  3 +++
 drm/nouveau/nvkm/subdev/iccsense/base.c    | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)
drm/nouveau/include/nvkm/subdev/iccsense.h +3 -0
index 3c2ddd9..b7a9b04 100644
@@ -8,6 +8,9 @@ struct nvkm_iccsense {
 	bool data_valid;
 	struct list_head sensors;
 	struct list_head rails;
+
+	u32 power_w_max;
+	u32 power_w_crit;
 };
 
 int gf100_iccsense_new(struct nvkm_device *, int index, struct nvkm_iccsense **);
drm/nouveau/nvkm/subdev/iccsense/base.c +12 -1
index 658355f..4e2b074 100644
@@ -26,6 +26,7 @@
 #include <subdev/bios.h>
 #include <subdev/bios/extdev.h>
 #include <subdev/bios/iccsense.h>
+#include <subdev/bios/power_budget.h>
 #include <subdev/i2c.h>
 
 static bool

@@ -216,10 +217,20 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 {
 	struct nvkm_iccsense *iccsense = nvkm_iccsense(subdev);
 	struct nvkm_bios *bios = subdev->device->bios;
+	struct nvbios_power_budget budget;
 	struct nvbios_iccsense stbl;
 	int i;
 
-	if (!bios || nvbios_iccsense_parse(bios, &stbl) || !stbl.nr_entry)
+	if (!bios)
+		return 0;
+
+	if (!nvbios_power_budget_parse(bios, &budget) && budget.nr_entry &&
+	    budget.cap_entry != 0xff) {
+		iccsense->power_w_max  = budget.entries[budget.cap_entry].avg_w;
+		iccsense->power_w_crit = budget.entries[budget.cap_entry].max_w;
+	}
+
+	if (nvbios_iccsense_parse(bios, &stbl) || !stbl.nr_entry)
 		return 0;
 
 	iccsense->data_valid = true;
Signature · 6 lines
2.10.0

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

A message that is not in the archive (sent off-list or not yet received): 20161024211113.5876-3-karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

#9 lore raw

Re: [PATCH 2/3] subdev/iccsense: Parse max and crit power level

Quoted 5 lines · Karol Herbst
Signed-off-by Karol Herbst <[email protected]>
---
 drm/nouveau/include/nvkm/subdev/iccsense.h |  3 +++
 drm/nouveau/nvkm/subdev/iccsense/base.c    | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

What is the point of duplicating values here? Just store the parsed bios table, like we do for fan management.

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

A message that is not in the archive (sent off-list or not yet received): e62aa5a5-d856-4e6c-7fba-d56043ba498f-GANU6spQydw@public.gmane.org

author
#11 lore raw

Re: [PATCH 2/3] subdev/iccsense: Parse max and crit power level

Quoted 10 lines · Martin Peres
Quoted 5 lines · Karol Herbst
Signed-off-by Karol Herbst <[email protected]>
---
 drm/nouveau/include/nvkm/subdev/iccsense.h |  3 +++
 drm/nouveau/nvkm/subdev/iccsense/base.c    | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

What is the point of duplicating values here? Just store the parsed bios
table, like we do for fan management.

will have a look. I was just thinking that thise fields may mean something else depending on the context. There are already some weird things, like we only get one value for very short entries.

But I guess it makes sense to keep it simple for now.

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

author
#12 lore raw

PATCH 3/3 hwmon: expose power_max and power_crit

In reply to a message not in the archive
Signed-off-by Karol Herbst <[email protected]>
---
 drm/nouveau/nouveau_hwmon.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
drm/nouveau/nouveau_hwmon.c +44 -0
index 71f764b..3d4672a 100644
@@ -596,6 +596,32 @@ nouveau_hwmon_get_power1_input(struct device *d, struct device_attribute *a,
 static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO,
 			  nouveau_hwmon_get_power1_input, NULL, 0);
 
+static ssize_t
+nouveau_hwmon_get_power1_max(struct device *d, struct device_attribute *a,
+			     char *buf)
+{
+	struct drm_device *dev = dev_get_drvdata(d);
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
+	return sprintf(buf, "%i\n", iccsense->power_w_max);
+}
+
+static SENSOR_DEVICE_ATTR(power1_max, S_IRUGO,
+			  nouveau_hwmon_get_power1_max, NULL, 0);
+
+static ssize_t
+nouveau_hwmon_get_power1_crit(struct device *d, struct device_attribute *a,
+			      char *buf)
+{
+	struct drm_device *dev = dev_get_drvdata(d);
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
+	return sprintf(buf, "%i\n", iccsense->power_w_crit);
+}
+
+static SENSOR_DEVICE_ATTR(power1_crit, S_IRUGO,
+			  nouveau_hwmon_get_power1_crit, NULL, 0);
+
 static struct attribute *hwmon_default_attributes[] = {
 	&sensor_dev_attr_name.dev_attr.attr,
 	&sensor_dev_attr_update_rate.dev_attr.attr,

@@ -639,6 +665,12 @@ static struct attribute *hwmon_power_attributes[] = {
 	NULL
 };
 
+static struct attribute *hwmon_power_caps_attributes[] = {
+	&sensor_dev_attr_power1_max.dev_attr.attr,
+	&sensor_dev_attr_power1_crit.dev_attr.attr,
+	NULL
+};
+
 static const struct attribute_group hwmon_default_attrgroup = {
 	.attrs = hwmon_default_attributes,
 };

@@ -657,6 +689,9 @@ static const struct attribute_group hwmon_in0_attrgroup = {
 static const struct attribute_group hwmon_power_attrgroup = {
 	.attrs = hwmon_power_attributes,
 };
+static const struct attribute_group hwmon_power_caps_attrgroup = {
+	.attrs = hwmon_power_caps_attributes,
+};
 #endif
 
 int

@@ -728,8 +763,16 @@ nouveau_hwmon_init(struct drm_device *dev)
 	if (iccsense && iccsense->data_valid && !list_empty(&iccsense->rails)) {
 		ret = sysfs_create_group(&hwmon_dev->kobj,
 					 &hwmon_power_attrgroup);
+
 		if (ret)
 			goto error;
+
+		if (iccsense->power_w_max && iccsense->power_w_crit) {
+			ret = sysfs_create_group(&hwmon_dev->kobj,
+						 &hwmon_power_caps_attrgroup);
+			if (ret)
+				goto error;
+		}
 	}
 
 	hwmon->hwmon = hwmon_dev;

@@ -759,6 +802,7 @@ nouveau_hwmon_fini(struct drm_device *dev)
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_fan_rpm_attrgroup);
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_in0_attrgroup);
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_attrgroup);
+		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_caps_attrgroup);
 
 		hwmon_device_unregister(hwmon->hwmon);
 	}
Signature · 6 lines
2.10.0

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

A message that is not in the archive (sent off-list or not yet received): 20161024211113.5876-4-karolherbst-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

#14 lore raw

Re: [PATCH 3/3] hwmon: expose power_max and power_crit

Quoted 84 lines · Karol Herbst
Signed-off-by Karol Herbst <[email protected]>
---
 drm/nouveau/nouveau_hwmon.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
drm/nouveau/nouveau_hwmon.c +43 -0
index 71f764b..3d4672a 100644
@@ -596,6 +596,32 @@ nouveau_hwmon_get_power1_input(struct device *d, struct device_attribute *a,
 static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO,
 			  nouveau_hwmon_get_power1_input, NULL, 0);
 
+static ssize_t
+nouveau_hwmon_get_power1_max(struct device *d, struct device_attribute *a,
+			     char *buf)
+{
+	struct drm_device *dev = dev_get_drvdata(d);
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
+	return sprintf(buf, "%i\n", iccsense->power_w_max);
+}
+
+static SENSOR_DEVICE_ATTR(power1_max, S_IRUGO,
+			  nouveau_hwmon_get_power1_max, NULL, 0);
+
+static ssize_t
+nouveau_hwmon_get_power1_crit(struct device *d, struct device_attribute *a,
+			      char *buf)
+{
+	struct drm_device *dev = dev_get_drvdata(d);
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
+	return sprintf(buf, "%i\n", iccsense->power_w_crit);
+}
+
+static SENSOR_DEVICE_ATTR(power1_crit, S_IRUGO,
+			  nouveau_hwmon_get_power1_crit, NULL, 0);
+
 static struct attribute *hwmon_default_attributes[] = {
 	&sensor_dev_attr_name.dev_attr.attr,
 	&sensor_dev_attr_update_rate.dev_attr.attr,

@@ -639,6 +665,12 @@ static struct attribute *hwmon_power_attributes[] = {
 	NULL
 };
 
+static struct attribute *hwmon_power_caps_attributes[] = {
+	&sensor_dev_attr_power1_max.dev_attr.attr,
+	&sensor_dev_attr_power1_crit.dev_attr.attr,
+	NULL
+};
+
 static const struct attribute_group hwmon_default_attrgroup = {
 	.attrs = hwmon_default_attributes,
 };

@@ -657,6 +689,9 @@ static const struct attribute_group hwmon_in0_attrgroup = {
 static const struct attribute_group hwmon_power_attrgroup = {
 	.attrs = hwmon_power_attributes,
 };
+static const struct attribute_group hwmon_power_caps_attrgroup = {
+	.attrs = hwmon_power_caps_attributes,
+};
 #endif
 
 int

@@ -728,8 +763,16 @@ nouveau_hwmon_init(struct drm_device *dev)
 	if (iccsense && iccsense->data_valid && !list_empty(&iccsense->rails)) {
 		ret = sysfs_create_group(&hwmon_dev->kobj,
 					 &hwmon_power_attrgroup);
+
 		if (ret)
 			goto error;
+
+		if (iccsense->power_w_max && iccsense->power_w_crit) {
+			ret = sysfs_create_group(&hwmon_dev->kobj,
+						 &hwmon_power_caps_attrgroup);
+			if (ret)
+				goto error;
+		}
 	}
 
 	hwmon->hwmon = hwmon_dev;

drm/nouveau/nouveau_hwmon.c nouveau_hwmon_fini(struct drm_device *dev) · line 808

Quoted 7 lines
drm/nouveau/nouveau_hwmon.c +1 -0
@@ -759,6 +802,7 @@ nouveau_hwmon_fini(struct drm_device *dev)
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_fan_rpm_attrgroup);
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_in0_attrgroup);
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_attrgroup);
+		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_caps_attrgroup);
 
 		hwmon_device_unregister(hwmon->hwmon);
 	}

So, you wanted to make a cache because you felt you may want to let the user edit the values?

Not sure we should ever allow this :s At least, a RO file is a very good idea to help debugging and just letting the user know about such limitations.

So, in my opinion, I think we should just kill patch 2 and use directly the output of the bios table here. Maybe you can write a helper function in patch 1 to get the min, avg and max values and return -1 if it is not valid? This way, you can access this info very easily in multiple places without duplicating this 0xff check.

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

A message that is not in the archive (sent off-list or not yet received): a270c96c-d792-8dd1-eaea-b63fe3a61e3a-GANU6spQydw@public.gmane.org

author
#16 lore raw

Re: [PATCH 3/3] hwmon: expose power_max and power_crit

Quoted 113 lines · Martin Peres
Quoted 3 lines · Karol Herbst
Signed-off-by Karol Herbst <[email protected]>
---
 drm/nouveau/nouveau_hwmon.c | 44

++++++++++++++++++++++++++++++++++++++++++++

Review

Quoted 3 lines
 1 file changed, 44 insertions(+)
(quoted diff) +0 -0

b/drm/nouveau/nouveau_hwmon.c

drm/nouveau/nouveau_hwmon.c nouveau_hwmon_get_power1_input(struct device *d,

Quoted 4 lines

index 71f764b..3d4672a 100644

drm/nouveau/nouveau_hwmon.c +0 -0
@@ -596,6 +596,32 @@ nouveau_hwmon_get_power1_input(struct device *d,

struct device_attribute *a,

drm/nouveau/nouveau_hwmon.c nouveau_hwmon_get_power1_input(struct device *d, struct device_attribute *a,

Quoted 5 lines
drm/nouveau/nouveau_hwmon.c +2 -0
 static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO,
 			  nouveau_hwmon_get_power1_input, NULL, 0);
 
+static ssize_t
+nouveau_hwmon_get_power1_max(struct device *d, struct

device_attribute *a,

drm/nouveau/nouveau_hwmon.c nouveau_hwmon_get_power1_input(struct device *d, struct device_attribute *a,

Quoted 13 lines
drm/nouveau/nouveau_hwmon.c +13 -0
+			     char *buf)
+{
+	struct drm_device *dev = dev_get_drvdata(d);
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
+	return sprintf(buf, "%i\n", iccsense->power_w_max);
+}
+
+static SENSOR_DEVICE_ATTR(power1_max, S_IRUGO,
+			  nouveau_hwmon_get_power1_max, NULL, 0);
+
+static ssize_t
+nouveau_hwmon_get_power1_crit(struct device *d, struct

device_attribute *a,

drm/nouveau/nouveau_hwmon.c static struct attribute

Quoted 15 lines
drm/nouveau/nouveau_hwmon.c +11 -0
+			      char *buf)
+{
+	struct drm_device *dev = dev_get_drvdata(d);
+	struct nouveau_drm *drm = nouveau_drm(dev);
+	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
+	return sprintf(buf, "%i\n", iccsense->power_w_crit);
+}
+
+static SENSOR_DEVICE_ATTR(power1_crit, S_IRUGO,
+			  nouveau_hwmon_get_power1_crit, NULL, 0);
+
 static struct attribute *hwmon_default_attributes[] = {
 	&sensor_dev_attr_name.dev_attr.attr,
 	&sensor_dev_attr_update_rate.dev_attr.attr,

@@ -639,6 +665,12 @@ static struct attribute

*hwmon_power_attributes[] = {

drm/nouveau/nouveau_hwmon.c static const struct attribute_group

Quoted 13 lines
drm/nouveau/nouveau_hwmon.c +6 -0
 	NULL
 };
 
+static struct attribute *hwmon_power_caps_attributes[] = {
+	&sensor_dev_attr_power1_max.dev_attr.attr,
+	&sensor_dev_attr_power1_crit.dev_attr.attr,
+	NULL
+};
+
 static const struct attribute_group hwmon_default_attrgroup = {
 	.attrs = hwmon_default_attributes,
 };

@@ -657,6 +689,9 @@ static const struct attribute_group

hwmon_in0_attrgroup = {

drm/nouveau/nouveau_hwmon.c nouveau_hwmon_init(struct drm_device *dev) · line 763

Quoted 11 lines
drm/nouveau/nouveau_hwmon.c +3 -0
 static const struct attribute_group hwmon_power_attrgroup = {
 	.attrs = hwmon_power_attributes,
 };
+static const struct attribute_group hwmon_power_caps_attrgroup = {
+	.attrs = hwmon_power_caps_attributes,
+};
 #endif
 
 int

@@ -728,8 +763,16 @@ nouveau_hwmon_init(struct drm_device *dev)
 	if (iccsense && iccsense->data_valid &&

!list_empty(&iccsense->rails)) {

Quoted 16 lines
drm/nouveau/nouveau_hwmon.c +8 -0
 		ret = sysfs_create_group(&hwmon_dev->kobj,
 					 &hwmon_power_attrgroup);
+
 		if (ret)
 			goto error;
+
+		if (iccsense->power_w_max && iccsense->power_w_crit) {
+			ret = sysfs_create_group(&hwmon_dev->kobj,
+						 &hwmon_power_caps_attrgroup);
+			if (ret)
+				goto error;
+		}
 	}
 
 	hwmon->hwmon = hwmon_dev;

drm/nouveau/nouveau_hwmon.c nouveau_hwmon_fini(struct drm_device *dev) · line 805

Quoted 4 lines
drm/nouveau/nouveau_hwmon.c +1 -0
@@ -759,6 +802,7 @@ nouveau_hwmon_fini(struct drm_device *dev)
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_fan_rpm_attrgroup);
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_in0_attrgroup);
 		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_attrgroup);
+		sysfs_remove_group(&hwmon->hwmon->kobj,

&hwmon_power_caps_attrgroup);

drm/nouveau/nouveau_hwmon.c nouveau_hwmon_fini(struct drm_device *dev)

Quoted 4 lines
drm/nouveau/nouveau_hwmon.c +0 -0
 		hwmon_device_unregister(hwmon->hwmon);
 	}

So, you wanted to make a cache because you felt you may want to let the

user edit the values?

Not sure we should ever allow this :s At least, a RO file is a very good
idea to help debugging and just letting the user know about such limitations.

So, in my opinion, I think we should just kill patch 2 and use directly

Quoted 9 lines

the output of the bios table here. Maybe you can write a helper function
in patch 1 to get the min, avg and max values and return -1 if it is not
valid? This way, you can access this info very easily in multiple places
without duplicating this 0xff check.

no, my intention was to parse the vbios once and be done with it, otherwise we would spread the logic how to read the values. But maybe the entries are indeed always min/max/crit and there is no avg value. But the min of the cap entry means something like min max consumption.

Aditinally the other ebtries have really weird affects, so I would rather not want to parse the table every time.

There are some flags in the entries which affect what is the max allowed consumption and I alrwady see it will get a little complicated.

Quoted 4 lines

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

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

#17 lore raw
In reply to a message not in the archive
Quoted 4 lines · Karol Herbst

There is an optinal header field in the power budget table we can use to read out the power cap of the GPU.

Sadly it is optional and if that field isn't sad, things beome

oh.... why would it be sad? Poor little one :p

Quoted 7 lines

complicated.

Anyhow, this is good enough for most cards and we can use it later for capping the power consumption of the GPUs, but first, just export those values through hwmon.

First design, will change stuff, want comments. Thanks.

Thanks for doing this!

Quoted 6 lines

Karol Herbst (3):
nvbios/power_budget: Add basic power budget parsing subdev/iccsense: Parse max and crit power level
hwmon: expose power_max and power_crit

Quoted 10 lines
 .../include/nvkm/subdev/bios/power_budget.h        |  20 ++++
 drm/nouveau/include/nvkm/subdev/iccsense.h         |   3 +
 drm/nouveau/nouveau_hwmon.c                        |  44 +++++++++
 drm/nouveau/nvkm/subdev/bios/Kbuild                |   1 +
 drm/nouveau/nvkm/subdev/bios/power_budget.c        | 108 +++++++++++++++++++++
 drm/nouveau/nvkm/subdev/iccsense/base.c            |  13 ++-
 6 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 drm/nouveau/include/nvkm/subdev/bios/power_budget.h
 create mode 100644 drm/nouveau/nvkm/subdev/bios/power_budget.c

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