[Intel-gfx] [PATCH v3 0/3] Add hwmon support for dgfx selftests

[Intel-gfx] [PATCH v3 0/3] Add hwmon support for dgfx selftests

[Riana Tauro]

Rename librapl library to libpower. Add hwmon support in libpower for
dgfx. 
Use libpower in selftests.

Rev2 : Update commit message
Rev3 : Remove redundant code

Riana Tauro (2):
  drm/i915/selftests: Rename librapl library to libpower
  drm/i915/hwmon: Add helper function to obtain energy values

Tilak Tangudu (1):
  drm/i915/selftests: Add hwmon support in libpower for dgfx

 drivers/gpu/drm/i915/Makefile             |  2 +-
 drivers/gpu/drm/i915/gt/selftest_rc6.c    | 12 ++++----
 drivers/gpu/drm/i915/gt/selftest_rps.c    | 26 ++++++++---------
 drivers/gpu/drm/i915/gt/selftest_slpc.c   |  4 +--
 drivers/gpu/drm/i915/i915_hwmon.c         | 17 +++++++++++
 drivers/gpu/drm/i915/i915_hwmon.h         |  1 +
 drivers/gpu/drm/i915/selftests/libpower.c | 35 +++++++++++++++++++++++
 drivers/gpu/drm/i915/selftests/libpower.h | 19 ++++++++++++
 drivers/gpu/drm/i915/selftests/librapl.c  | 34 ----------------------
 drivers/gpu/drm/i915/selftests/librapl.h  | 17 -----------
 10 files changed, 94 insertions(+), 73 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/selftests/libpower.c
 create mode 100644 drivers/gpu/drm/i915/selftests/libpower.h
 delete mode 100644 drivers/gpu/drm/i915/selftests/librapl.c
 delete mode 100644 drivers/gpu/drm/i915/selftests/librapl.h

-- 
2.25.1

[Intel-gfx] [PATCH v3 1/3] drm/i915/selftests: Rename librapl library to libpower

[Riana Tauro]

Rename librapl files to libpower and replace librapl
with libpower prefix. No functional changes

v2: update commit message (Anshuman)

Signed-off-by: Riana Tauro <riana.tauro@intel.com>
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/Makefile                   |  2 +-
 drivers/gpu/drm/i915/gt/selftest_rc6.c          | 12 ++++++------
 drivers/gpu/drm/i915/gt/selftest_rps.c          |  8 ++++----
 drivers/gpu/drm/i915/gt/selftest_slpc.c         |  2 +-
 .../i915/selftests/{librapl.c => libpower.c}    | 10 +++++-----
 drivers/gpu/drm/i915/selftests/libpower.h       | 17 +++++++++++++++++
 drivers/gpu/drm/i915/selftests/librapl.h        | 17 -----------------
 7 files changed, 34 insertions(+), 34 deletions(-)
 rename drivers/gpu/drm/i915/selftests/{librapl.c => libpower.c} (69%)
 create mode 100644 drivers/gpu/drm/i915/selftests/libpower.h
 delete mode 100644 drivers/gpu/drm/i915/selftests/librapl.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 01974b82d205..ddece53f7c9f 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -342,7 +342,7 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
 	selftests/igt_mmap.o \
 	selftests/igt_reset.o \
 	selftests/igt_spinner.o \
-	selftests/librapl.o
+	selftests/libpower.o
 
 # virtual gpu code
 i915-y += i915_vgpu.o
diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c
index 2ceeadecc639..15b84c428f66 100644
--- a/drivers/gpu/drm/i915/gt/selftest_rc6.c
+++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c
@@ -11,7 +11,7 @@
 #include "selftest_rc6.h"
 
 #include "selftests/i915_random.h"
-#include "selftests/librapl.h"
+#include "selftests/libpower.h"
 
 static u64 rc6_residency(struct intel_rc6 *rc6)
 {
@@ -51,7 +51,7 @@ int live_rc6_manual(void *arg)
 	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915))
 		return 0;
 
-	has_power = librapl_supported(gt->i915);
+	has_power = libpower_supported(gt->i915);
 	wakeref = intel_runtime_pm_get(gt->uncore->rpm);
 
 	/* Force RC6 off for starters */
@@ -61,9 +61,9 @@ int live_rc6_manual(void *arg)
 	res[0] = rc6_residency(rc6);
 
 	dt = ktime_get();
-	rc0_power = librapl_energy_uJ();
+	rc0_power = libpower_get_energy_uJ();
 	msleep(250);
-	rc0_power = librapl_energy_uJ() - rc0_power;
+	rc0_power = libpower_get_energy_uJ() - rc0_power;
 	dt = ktime_sub(ktime_get(), dt);
 	res[1] = rc6_residency(rc6);
 	if ((res[1] - res[0]) >> 10) {
@@ -89,9 +89,9 @@ int live_rc6_manual(void *arg)
 	res[0] = rc6_residency(rc6);
 	intel_uncore_forcewake_flush(rc6_to_uncore(rc6), FORCEWAKE_ALL);
 	dt = ktime_get();
-	rc6_power = librapl_energy_uJ();
+	rc6_power = libpower_get_energy_uJ();
 	msleep(100);
-	rc6_power = librapl_energy_uJ() - rc6_power;
+	rc6_power = libpower_get_energy_uJ() - rc6_power;
 	dt = ktime_sub(ktime_get(), dt);
 	res[1] = rc6_residency(rc6);
 	if (res[1] == res[0]) {
diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c b/drivers/gpu/drm/i915/gt/selftest_rps.c
index 39f1b7564170..b8b0b0c7617e 100644
--- a/drivers/gpu/drm/i915/gt/selftest_rps.c
+++ b/drivers/gpu/drm/i915/gt/selftest_rps.c
@@ -19,7 +19,7 @@
 #include "selftest_rps.h"
 #include "selftests/igt_flush_test.h"
 #include "selftests/igt_spinner.h"
-#include "selftests/librapl.h"
+#include "selftests/libpower.h"
 
 /* Try to isolate the impact of cstates from determing frequency response */
 #define CPU_LATENCY 0 /* -1 to disable pm_qos, 0 to disable cstates */
@@ -1095,9 +1095,9 @@ static u64 __measure_power(int duration_ms)
 	u64 dE, dt;
 
 	dt = ktime_get();
-	dE = librapl_energy_uJ();
+	dE = libpower_get_energy_uJ();
 	usleep_range(1000 * duration_ms, 2000 * duration_ms);
-	dE = librapl_energy_uJ() - dE;
+	dE = libpower_get_energy_uJ() - dE;
 	dt = ktime_get() - dt;
 
 	return div64_u64(1000 * 1000 * dE, dt);
@@ -1143,7 +1143,7 @@ int live_rps_power(void *arg)
 	if (!intel_rps_is_enabled(rps) || GRAPHICS_VER(gt->i915) < 6)
 		return 0;
 
-	if (!librapl_supported(gt->i915))
+	if (!libpower_supported(gt->i915))
 		return 0;
 
 	if (igt_spinner_init(&spin, gt))
diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
index bd44ce73a504..fc1cdda82ec6 100644
--- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
@@ -169,7 +169,7 @@ static int slpc_power(struct intel_gt *gt, struct intel_engine_cs *engine)
 	 * actually saves power. Let's see if our RAPL measurement supports
 	 * that theory.
 	 */
-	if (!librapl_supported(gt->i915))
+	if (!libpower_supported(gt->i915))
 		return 0;
 
 	min.freq = slpc->min_freq;
diff --git a/drivers/gpu/drm/i915/selftests/librapl.c b/drivers/gpu/drm/i915/selftests/libpower.c
similarity index 69%
rename from drivers/gpu/drm/i915/selftests/librapl.c
rename to drivers/gpu/drm/i915/selftests/libpower.c
index eb03b5b28bad..c66e993c5f85 100644
--- a/drivers/gpu/drm/i915/selftests/librapl.c
+++ b/drivers/gpu/drm/i915/selftests/libpower.c
@@ -1,23 +1,23 @@
 // SPDX-License-Identifier: MIT
 /*
- * Copyright © 2020 Intel Corporation
+ * Copyright © 2022 Intel Corporation
  */
 
 #include <asm/msr.h>
 
 #include "i915_drv.h"
-#include "librapl.h"
+#include "libpower.h"
 
-bool librapl_supported(const struct drm_i915_private *i915)
+bool libpower_supported(const struct drm_i915_private *i915)
 {
 	/* Discrete cards require hwmon integration */
 	if (IS_DGFX(i915))
 		return false;
 
-	return librapl_energy_uJ();
+	return libpower_get_energy_uJ();
 }
 
-u64 librapl_energy_uJ(void)
+u64 libpower_get_energy_uJ(void)
 {
 	unsigned long long power;
 	u32 units;
diff --git a/drivers/gpu/drm/i915/selftests/libpower.h b/drivers/gpu/drm/i915/selftests/libpower.h
new file mode 100644
index 000000000000..5352981eb946
--- /dev/null
+++ b/drivers/gpu/drm/i915/selftests/libpower.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef SELFTEST_LIBPOWER_H
+#define SELFTEST_LIBPOWER_H
+
+#include <linux/types.h>
+
+struct drm_i915_private;
+
+bool libpower_supported(const struct drm_i915_private *i915);
+
+u64 libpower_get_energy_uJ(void);
+
+#endif /* SELFTEST_LIBPOWER_H */
diff --git a/drivers/gpu/drm/i915/selftests/librapl.h b/drivers/gpu/drm/i915/selftests/librapl.h
deleted file mode 100644
index e3b24fad0a7a..000000000000
--- a/drivers/gpu/drm/i915/selftests/librapl.h
+++ /dev/null
@@ -1,17 +0,0 @@
-/* SPDX-License-Identifier: MIT */
-/*
- * Copyright © 2020 Intel Corporation
- */
-
-#ifndef SELFTEST_LIBRAPL_H
-#define SELFTEST_LIBRAPL_H
-
-#include <linux/types.h>
-
-struct drm_i915_private;
-
-bool librapl_supported(const struct drm_i915_private *i915);
-
-u64 librapl_energy_uJ(void);
-
-#endif /* SELFTEST_LIBRAPL_H */
-- 
2.25.1

[Intel-gfx] [PATCH v3 2/3] drm/i915/hwmon: Add helper function to obtain energy values

[Riana Tauro]

Add an interface to obtain hwmon energy values. This is used
by selftest to verify power consumption

v2 : use i915_hwmon prefix (Anshuman)
v3 : re-use is_visible function of energy to remove
     redundant code (Anshuman)

Signed-off-by: Riana Tauro <riana.tauro@intel.com>
---
 drivers/gpu/drm/i915/i915_hwmon.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/i915_hwmon.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index c588a17f97e9..d45d1cbce886 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -442,6 +442,23 @@ hwm_energy_read(struct hwm_drvdata *ddat, u32 attr, long *val)
 	}
 }
 
+/*
+ * i915_hwmon_get_energy - obtains energy value
+ * Returns: 0 on success or a negative error code
+ */
+int
+i915_hwmon_get_energy(struct drm_i915_private *i915, long *energy)
+{
+	struct i915_hwmon *hwmon = i915->hwmon;
+	struct hwm_drvdata *ddat = &hwmon->ddat;
+
+	if (!hwm_energy_is_visible(ddat, hwmon_energy_input))
+		return -EOPNOTSUPP;
+
+	hwm_energy(ddat, energy);
+	return 0;
+}
+
 static umode_t
 hwm_curr_is_visible(const struct hwm_drvdata *ddat, u32 attr)
 {
diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h
index 7ca9cf2c34c9..a6c8efeb868d 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.h
+++ b/drivers/gpu/drm/i915/i915_hwmon.h
@@ -12,6 +12,7 @@ struct drm_i915_private;
 #if IS_REACHABLE(CONFIG_HWMON)
 void i915_hwmon_register(struct drm_i915_private *i915);
 void i915_hwmon_unregister(struct drm_i915_private *i915);
+int i915_hwmon_get_energy(struct drm_i915_private *i915, long *energy);
 #else
 static inline void i915_hwmon_register(struct drm_i915_private *i915) { };
 static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { };
-- 
2.25.1

[Intel-gfx] [PATCH v3 3/3] drm/i915/selftests: Add hwmon support in libpower for dgfx

[Riana Tauro]

From: Tilak Tangudu <tilak.tangudu@intel.com>

hwmon provides an interface to read energy values for discrete graphics.
add hwmon support to the existing libpower library so that it can verify
power consumption values in different selftests.

Changed prototype of libpower_get_energy_uJ

Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
Signed-off-by: Riana Tauro <riana.tauro@intel.com>
---
 drivers/gpu/drm/i915/gt/selftest_rc6.c    |  8 +++---
 drivers/gpu/drm/i915/gt/selftest_rps.c    | 22 +++++++--------
 drivers/gpu/drm/i915/gt/selftest_slpc.c   |  2 +-
 drivers/gpu/drm/i915/selftests/libpower.c | 33 ++++++++++++-----------
 drivers/gpu/drm/i915/selftests/libpower.h |  8 +++---
 5 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c
index 15b84c428f66..845058ed83ed 100644
--- a/drivers/gpu/drm/i915/gt/selftest_rc6.c
+++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c
@@ -61,9 +61,9 @@ int live_rc6_manual(void *arg)
 	res[0] = rc6_residency(rc6);
 
 	dt = ktime_get();
-	rc0_power = libpower_get_energy_uJ();
+	rc0_power = libpower_get_energy_uJ(gt->i915);
 	msleep(250);
-	rc0_power = libpower_get_energy_uJ() - rc0_power;
+	rc0_power = libpower_get_energy_uJ(gt->i915) - rc0_power;
 	dt = ktime_sub(ktime_get(), dt);
 	res[1] = rc6_residency(rc6);
 	if ((res[1] - res[0]) >> 10) {
@@ -89,9 +89,9 @@ int live_rc6_manual(void *arg)
 	res[0] = rc6_residency(rc6);
 	intel_uncore_forcewake_flush(rc6_to_uncore(rc6), FORCEWAKE_ALL);
 	dt = ktime_get();
-	rc6_power = libpower_get_energy_uJ();
+	rc6_power = libpower_get_energy_uJ(gt->i915);
 	msleep(100);
-	rc6_power = libpower_get_energy_uJ() - rc6_power;
+	rc6_power = libpower_get_energy_uJ(gt->i915) - rc6_power;
 	dt = ktime_sub(ktime_get(), dt);
 	res[1] = rc6_residency(rc6);
 	if (res[1] == res[0]) {
diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c b/drivers/gpu/drm/i915/gt/selftest_rps.c
index b8b0b0c7617e..6732aa7d4792 100644
--- a/drivers/gpu/drm/i915/gt/selftest_rps.c
+++ b/drivers/gpu/drm/i915/gt/selftest_rps.c
@@ -1090,38 +1090,38 @@ int live_rps_interrupt(void *arg)
 	return err;
 }
 
-static u64 __measure_power(int duration_ms)
+static u64 __measure_power(struct intel_gt *gt, int duration_ms)
 {
 	u64 dE, dt;
 
 	dt = ktime_get();
-	dE = libpower_get_energy_uJ();
+	dE = libpower_get_energy_uJ(gt->i915);
 	usleep_range(1000 * duration_ms, 2000 * duration_ms);
-	dE = libpower_get_energy_uJ() - dE;
+	dE = libpower_get_energy_uJ(gt->i915) - dE;
 	dt = ktime_get() - dt;
 
 	return div64_u64(1000 * 1000 * dE, dt);
 }
 
-static u64 measure_power(struct intel_rps *rps, int *freq)
+static u64 measure_power(struct intel_gt *gt, int *freq)
 {
 	u64 x[5];
 	int i;
 
 	for (i = 0; i < 5; i++)
-		x[i] = __measure_power(5);
+		x[i] = __measure_power(gt, 5);
 
-	*freq = (*freq + intel_rps_read_actual_frequency(rps)) / 2;
+	*freq = (*freq + intel_rps_read_actual_frequency(&gt->rps)) / 2;
 
 	/* A simple triangle filter for better result stability */
 	sort(x, 5, sizeof(*x), cmp_u64, NULL);
 	return div_u64(x[1] + 2 * x[2] + x[3], 4);
 }
 
-static u64 measure_power_at(struct intel_rps *rps, int *freq)
+static u64 measure_power_at(struct intel_gt *gt, int *freq)
 {
-	*freq = rps_set_check(rps, *freq);
-	return measure_power(rps, freq);
+	*freq = rps_set_check(&gt->rps, *freq);
+	return measure_power(gt, freq);
 }
 
 int live_rps_power(void *arg)
@@ -1187,10 +1187,10 @@ int live_rps_power(void *arg)
 		}
 
 		max.freq = rps->max_freq;
-		max.power = measure_power_at(rps, &max.freq);
+		max.power = measure_power_at(gt, &max.freq);
 
 		min.freq = rps->min_freq;
-		min.power = measure_power_at(rps, &min.freq);
+		min.power = measure_power_at(gt, &min.freq);
 
 		igt_spinner_end(&spin);
 		st_engine_heartbeat_enable(engine);
diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
index fc1cdda82ec6..c4b14f2b268c 100644
--- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
@@ -78,7 +78,7 @@ static u64 measure_power_at_freq(struct intel_gt *gt, int *freq, u64 *power)
 	if (err)
 		return err;
 	*freq = intel_rps_read_actual_frequency(&gt->rps);
-	*power = measure_power(&gt->rps, freq);
+	*power = measure_power(gt, freq);
 
 	return err;
 }
diff --git a/drivers/gpu/drm/i915/selftests/libpower.c b/drivers/gpu/drm/i915/selftests/libpower.c
index c66e993c5f85..df37cba30353 100644
--- a/drivers/gpu/drm/i915/selftests/libpower.c
+++ b/drivers/gpu/drm/i915/selftests/libpower.c
@@ -6,29 +6,30 @@
 #include <asm/msr.h>
 
 #include "i915_drv.h"
+#include "i915_hwmon.h"
 #include "libpower.h"
 
-bool libpower_supported(const struct drm_i915_private *i915)
-{
-	/* Discrete cards require hwmon integration */
-	if (IS_DGFX(i915))
-		return false;
-
-	return libpower_get_energy_uJ();
-}
-
-u64 libpower_get_energy_uJ(void)
+u64 libpower_get_energy_uJ(struct drm_i915_private *i915)
 {
 	unsigned long long power;
 	u32 units;
+	long energy_uJ = 0;
 
-	if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
-		return 0;
+	if (IS_DGFX(i915)) {
+#if IS_REACHABLE(CONFIG_HWMON)
+		if (i915_hwmon_get_energy(i915, &energy_uJ))
+#endif
+			return 0;
+	} else {
+		if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
+			return 0;
 
-	units = (power & 0x1f00) >> 8;
+		units = (power & 0x1f00) >> 8;
 
-	if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
-		return 0;
+		if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
+			return 0;
 
-	return (1000000 * power) >> units; /* convert to uJ */
+		energy_uJ = (1000000 * power) >> units; /* convert to uJ */
+	}
+	return energy_uJ;
 }
diff --git a/drivers/gpu/drm/i915/selftests/libpower.h b/drivers/gpu/drm/i915/selftests/libpower.h
index 5352981eb946..03a44611f9e9 100644
--- a/drivers/gpu/drm/i915/selftests/libpower.h
+++ b/drivers/gpu/drm/i915/selftests/libpower.h
@@ -10,8 +10,10 @@
 
 struct drm_i915_private;
 
-bool libpower_supported(const struct drm_i915_private *i915);
-
-u64 libpower_get_energy_uJ(void);
+u64 libpower_get_energy_uJ(struct drm_i915_private *i915);
 
+static inline bool libpower_supported(struct drm_i915_private *i915)
+{
+	return libpower_get_energy_uJ(i915);
+}
 #endif /* SELFTEST_LIBPOWER_H */
-- 
2.25.1

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/hwmon: Add helper function to obtain energy values

[Gupta, Anshuman]

> -----Original Message-----
> From: Tauro, Riana <riana.tauro@intel.com>
> Sent: Monday, November 21, 2022 1:00 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Tauro, Riana <riana.tauro@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>;
> Tangudu, Tilak <tilak.tangudu@intel.com>; Nilawar, Badal
> <badal.nilawar@intel.com>
> Subject: [PATCH v3 2/3] drm/i915/hwmon: Add helper function to obtain
> energy values
> 
> Add an interface to obtain hwmon energy values. This is used by selftest to
> verify power consumption
> 
> v2 : use i915_hwmon prefix (Anshuman)
> v3 : re-use is_visible function of energy to remove
>      redundant code (Anshuman)
> 
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_hwmon.c | 17 +++++++++++++++++
> drivers/gpu/drm/i915/i915_hwmon.h |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index c588a17f97e9..d45d1cbce886 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -442,6 +442,23 @@ hwm_energy_read(struct hwm_drvdata *ddat, u32
> attr, long *val)
>  	}
>  }
> 
> +/*
> + * i915_hwmon_get_energy - obtains energy value
> + * Returns: 0 on success or a negative error code  */ int
Adding comment for exported function should be Kennel Doc comments.
https://docs.kernel.org/doc-guide/kernel-doc.html
With that 
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> +i915_hwmon_get_energy(struct drm_i915_private *i915, long *energy) {
> +	struct i915_hwmon *hwmon = i915->hwmon;
> +	struct hwm_drvdata *ddat = &hwmon->ddat;
> +
> +	if (!hwm_energy_is_visible(ddat, hwmon_energy_input))
> +		return -EOPNOTSUPP;
> +
> +	hwm_energy(ddat, energy);
> +	return 0;
> +}
> +
>  static umode_t
>  hwm_curr_is_visible(const struct hwm_drvdata *ddat, u32 attr)  { diff --git
> a/drivers/gpu/drm/i915/i915_hwmon.h
> b/drivers/gpu/drm/i915/i915_hwmon.h
> index 7ca9cf2c34c9..a6c8efeb868d 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.h
> +++ b/drivers/gpu/drm/i915/i915_hwmon.h
> @@ -12,6 +12,7 @@ struct drm_i915_private;  #if
> IS_REACHABLE(CONFIG_HWMON)  void i915_hwmon_register(struct
> drm_i915_private *i915);  void i915_hwmon_unregister(struct
> drm_i915_private *i915);
> +int i915_hwmon_get_energy(struct drm_i915_private *i915, long *energy);
>  #else
>  static inline void i915_hwmon_register(struct drm_i915_private *i915) { };
> static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { };
> --
> 2.25.1

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/selftests: Add hwmon support in libpower for dgfx

[Gupta, Anshuman]

> -----Original Message-----
> From: Tauro, Riana <riana.tauro@intel.com>
> Sent: Monday, November 21, 2022 1:00 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Tauro, Riana <riana.tauro@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>;
> Tangudu, Tilak <tilak.tangudu@intel.com>; Nilawar, Badal
> <badal.nilawar@intel.com>
> Subject: [PATCH v3 3/3] drm/i915/selftests: Add hwmon support in libpower
> for dgfx
> 
> From: Tilak Tangudu <tilak.tangudu@intel.com>
> 
> hwmon provides an interface to read energy values for discrete graphics.
> add hwmon support to the existing libpower library so that it can verify
> power consumption values in different selftests.
> 
> Changed prototype of libpower_get_energy_uJ
> 
> Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
LGTM
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/selftest_rc6.c    |  8 +++---
>  drivers/gpu/drm/i915/gt/selftest_rps.c    | 22 +++++++--------
>  drivers/gpu/drm/i915/gt/selftest_slpc.c   |  2 +-
>  drivers/gpu/drm/i915/selftests/libpower.c | 33 ++++++++++++-----------
> drivers/gpu/drm/i915/selftests/libpower.h |  8 +++---
>  5 files changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c
> b/drivers/gpu/drm/i915/gt/selftest_rc6.c
> index 15b84c428f66..845058ed83ed 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_rc6.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c
> @@ -61,9 +61,9 @@ int live_rc6_manual(void *arg)
>  	res[0] = rc6_residency(rc6);
> 
>  	dt = ktime_get();
> -	rc0_power = libpower_get_energy_uJ();
> +	rc0_power = libpower_get_energy_uJ(gt->i915);
>  	msleep(250);
> -	rc0_power = libpower_get_energy_uJ() - rc0_power;
> +	rc0_power = libpower_get_energy_uJ(gt->i915) - rc0_power;
>  	dt = ktime_sub(ktime_get(), dt);
>  	res[1] = rc6_residency(rc6);
>  	if ((res[1] - res[0]) >> 10) {
> @@ -89,9 +89,9 @@ int live_rc6_manual(void *arg)
>  	res[0] = rc6_residency(rc6);
>  	intel_uncore_forcewake_flush(rc6_to_uncore(rc6),
> FORCEWAKE_ALL);
>  	dt = ktime_get();
> -	rc6_power = libpower_get_energy_uJ();
> +	rc6_power = libpower_get_energy_uJ(gt->i915);
>  	msleep(100);
> -	rc6_power = libpower_get_energy_uJ() - rc6_power;
> +	rc6_power = libpower_get_energy_uJ(gt->i915) - rc6_power;
>  	dt = ktime_sub(ktime_get(), dt);
>  	res[1] = rc6_residency(rc6);
>  	if (res[1] == res[0]) {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c
> b/drivers/gpu/drm/i915/gt/selftest_rps.c
> index b8b0b0c7617e..6732aa7d4792 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_rps.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_rps.c
> @@ -1090,38 +1090,38 @@ int live_rps_interrupt(void *arg)
>  	return err;
>  }
> 
> -static u64 __measure_power(int duration_ms)
> +static u64 __measure_power(struct intel_gt *gt, int duration_ms)
>  {
>  	u64 dE, dt;
> 
>  	dt = ktime_get();
> -	dE = libpower_get_energy_uJ();
> +	dE = libpower_get_energy_uJ(gt->i915);
>  	usleep_range(1000 * duration_ms, 2000 * duration_ms);
> -	dE = libpower_get_energy_uJ() - dE;
> +	dE = libpower_get_energy_uJ(gt->i915) - dE;
>  	dt = ktime_get() - dt;
> 
>  	return div64_u64(1000 * 1000 * dE, dt);  }
> 
> -static u64 measure_power(struct intel_rps *rps, int *freq)
> +static u64 measure_power(struct intel_gt *gt, int *freq)
>  {
>  	u64 x[5];
>  	int i;
> 
>  	for (i = 0; i < 5; i++)
> -		x[i] = __measure_power(5);
> +		x[i] = __measure_power(gt, 5);
> 
> -	*freq = (*freq + intel_rps_read_actual_frequency(rps)) / 2;
> +	*freq = (*freq + intel_rps_read_actual_frequency(&gt->rps)) / 2;
> 
>  	/* A simple triangle filter for better result stability */
>  	sort(x, 5, sizeof(*x), cmp_u64, NULL);
>  	return div_u64(x[1] + 2 * x[2] + x[3], 4);  }
> 
> -static u64 measure_power_at(struct intel_rps *rps, int *freq)
> +static u64 measure_power_at(struct intel_gt *gt, int *freq)
>  {
> -	*freq = rps_set_check(rps, *freq);
> -	return measure_power(rps, freq);
> +	*freq = rps_set_check(&gt->rps, *freq);
> +	return measure_power(gt, freq);
>  }
> 
>  int live_rps_power(void *arg)
> @@ -1187,10 +1187,10 @@ int live_rps_power(void *arg)
>  		}
> 
>  		max.freq = rps->max_freq;
> -		max.power = measure_power_at(rps, &max.freq);
> +		max.power = measure_power_at(gt, &max.freq);
> 
>  		min.freq = rps->min_freq;
> -		min.power = measure_power_at(rps, &min.freq);
> +		min.power = measure_power_at(gt, &min.freq);
> 
>  		igt_spinner_end(&spin);
>  		st_engine_heartbeat_enable(engine);
> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c
> b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> index fc1cdda82ec6..c4b14f2b268c 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> @@ -78,7 +78,7 @@ static u64 measure_power_at_freq(struct intel_gt *gt,
> int *freq, u64 *power)
>  	if (err)
>  		return err;
>  	*freq = intel_rps_read_actual_frequency(&gt->rps);
> -	*power = measure_power(&gt->rps, freq);
> +	*power = measure_power(gt, freq);
> 
>  	return err;
>  }
> diff --git a/drivers/gpu/drm/i915/selftests/libpower.c
> b/drivers/gpu/drm/i915/selftests/libpower.c
> index c66e993c5f85..df37cba30353 100644
> --- a/drivers/gpu/drm/i915/selftests/libpower.c
> +++ b/drivers/gpu/drm/i915/selftests/libpower.c
> @@ -6,29 +6,30 @@
>  #include <asm/msr.h>
> 
>  #include "i915_drv.h"
> +#include "i915_hwmon.h"
>  #include "libpower.h"
> 
> -bool libpower_supported(const struct drm_i915_private *i915) -{
> -	/* Discrete cards require hwmon integration */
> -	if (IS_DGFX(i915))
> -		return false;
> -
> -	return libpower_get_energy_uJ();
> -}
> -
> -u64 libpower_get_energy_uJ(void)
> +u64 libpower_get_energy_uJ(struct drm_i915_private *i915)
>  {
>  	unsigned long long power;
>  	u32 units;
> +	long energy_uJ = 0;
> 
> -	if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
> -		return 0;
> +	if (IS_DGFX(i915)) {
> +#if IS_REACHABLE(CONFIG_HWMON)
> +		if (i915_hwmon_get_energy(i915, &energy_uJ)) #endif
> +			return 0;
> +	} else {
> +		if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
> +			return 0;
> 
> -	units = (power & 0x1f00) >> 8;
> +		units = (power & 0x1f00) >> 8;
> 
> -	if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
> -		return 0;
> +		if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
> +			return 0;
> 
> -	return (1000000 * power) >> units; /* convert to uJ */
> +		energy_uJ = (1000000 * power) >> units; /* convert to uJ */
> +	}
> +	return energy_uJ;
>  }
> diff --git a/drivers/gpu/drm/i915/selftests/libpower.h
> b/drivers/gpu/drm/i915/selftests/libpower.h
> index 5352981eb946..03a44611f9e9 100644
> --- a/drivers/gpu/drm/i915/selftests/libpower.h
> +++ b/drivers/gpu/drm/i915/selftests/libpower.h
> @@ -10,8 +10,10 @@
> 
>  struct drm_i915_private;
> 
> -bool libpower_supported(const struct drm_i915_private *i915);
> -
> -u64 libpower_get_energy_uJ(void);
> +u64 libpower_get_energy_uJ(struct drm_i915_private *i915);
> 
> +static inline bool libpower_supported(struct drm_i915_private *i915) {
> +	return libpower_get_energy_uJ(i915);
> +}
>  #endif /* SELFTEST_LIBPOWER_H */
> --
> 2.25.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Add hwmon support for dgfx selftests (rev3)

[Patchwork]

== Series Details ==

Series: Add hwmon support for dgfx selftests (rev3)
URL   : https://patchwork.freedesktop.org/series/109850/
State : warning

== Summary ==

Error: dim checkpatch failed
6270ace209c5 drm/i915/selftests: Rename librapl library to libpower
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 11, in <module>
    import git
ModuleNotFoundError: No module named 'git'
-:122: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#122: 
rename from drivers/gpu/drm/i915/selftests/librapl.c

total: 0 errors, 1 warnings, 0 checks, 126 lines checked
47b6309c4229 drm/i915/hwmon: Add helper function to obtain energy values
8e7b94737d16 drm/i915/selftests: Add hwmon support in libpower for dgfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for Add hwmon support for dgfx selftests (rev3)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6576 bytes --]

== Series Details ==

Series: Add hwmon support for dgfx selftests (rev3)
URL   : https://patchwork.freedesktop.org/series/109850/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12406 -> Patchwork_109850v3
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_109850v3 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_109850v3, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/index.html

Participating hosts (37 -> 22)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (16): fi-ilk-m540 fi-rkl-11600 bat-dg1-7 fi-bdw-samus bat-dg1-6 bat-dg2-8 bat-adlm-1 bat-dg2-9 bat-adlp-6 bat-adlp-4 fi-hsw-4770 bat-adln-1 bat-rplp-1 bat-rpls-2 bat-dg2-11 bat-jsl-1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_109850v3:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_pm:
    - bat-dg1-5:          [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12406/bat-dg1-5/igt@i915_selftest@live@gt_pm.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/bat-dg1-5/igt@i915_selftest@live@gt_pm.html

  
Known issues
------------

  Here are the changes found in Patchwork_109850v3 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_gttfill@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][3] ([fdo#109271]) +9 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-kbl-soraka/igt@gem_exec_gttfill@basic.html

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-apl-guc:         NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#4613]) +3 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-apl-guc/igt@gem_lmem_swapping@basic.html
    - fi-kbl-soraka:      NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#4613]) +3 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@gem_contexts:
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][7] ([i915#7099])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-kbl-soraka/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][8] ([i915#5334])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][9] ([i915#1886])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-apl-guc:         NOTRUN -> [SKIP][10] ([fdo#109271] / [fdo#111827])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-apl-guc/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][11] ([fdo#109271] / [fdo#111827]) +7 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-kbl-soraka/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Possible fixes ####

  * igt@core_hotunplug@unbind-rebind:
    - fi-apl-guc:         [INCOMPLETE][12] ([i915#7073]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12406/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html

  * igt@i915_selftest@live@hangcheck:
    - {fi-ehl-2}:         [INCOMPLETE][14] ([i915#5153] / [i915#6106]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12406/fi-ehl-2/igt@i915_selftest@live@hangcheck.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-ehl-2/igt@i915_selftest@live@hangcheck.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-kefka:       [FAIL][16] ([i915#6298]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12406/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5153]: https://gitlab.freedesktop.org/drm/intel/issues/5153
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6106]: https://gitlab.freedesktop.org/drm/intel/issues/6106
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#7073]: https://gitlab.freedesktop.org/drm/intel/issues/7073
  [i915#7099]: https://gitlab.freedesktop.org/drm/intel/issues/7099


Build changes
-------------

  * Linux: CI_DRM_12406 -> Patchwork_109850v3

  CI-20190529: 20190529
  CI_DRM_12406: 4f2ce91f232852523c9b646db02a770e35ec3362 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7068: 5c0ec905b6bbecfb8df8b8f3315d0470539e6ae3 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_109850v3: 4f2ce91f232852523c9b646db02a770e35ec3362 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

92d12de5c50c drm/i915/selftests: Add hwmon support in libpower for dgfx
954657b7bbd4 drm/i915/hwmon: Add helper function to obtain energy values
92de13f30791 drm/i915/selftests: Rename librapl library to libpower

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109850v3/index.html

[-- Attachment #2: Type: text/html, Size: 7993 bytes --]

Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/hwmon: Add helper function to obtain energy values

[Dixit, Ashutosh]

On Sun, 20 Nov 2022 23:29:45 -0800, Riana Tauro wrote:
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h
> index 7ca9cf2c34c9..a6c8efeb868d 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.h
> +++ b/drivers/gpu/drm/i915/i915_hwmon.h
> @@ -12,6 +12,7 @@ struct drm_i915_private;
>  #if IS_REACHABLE(CONFIG_HWMON)
>  void i915_hwmon_register(struct drm_i915_private *i915);
>  void i915_hwmon_unregister(struct drm_i915_private *i915);
> +int i915_hwmon_get_energy(struct drm_i915_private *i915, long *energy);
>  #else
>  static inline void i915_hwmon_register(struct drm_i915_private *i915) { };
>  static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { };

What about the #else case?

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/selftests: Add hwmon support in libpower for dgfx

[Dixit, Ashutosh]

On Sun, 20 Nov 2022 23:29:46 -0800, Riana Tauro wrote:
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c
> index 15b84c428f66..845058ed83ed 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_rc6.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c
> @@ -61,9 +61,9 @@ int live_rc6_manual(void *arg)
>	res[0] = rc6_residency(rc6);
>
>	dt = ktime_get();
> -	rc0_power = libpower_get_energy_uJ();
> +	rc0_power = libpower_get_energy_uJ(gt->i915);
>	msleep(250);
> -	rc0_power = libpower_get_energy_uJ() - rc0_power;
> +	rc0_power = libpower_get_energy_uJ(gt->i915) - rc0_power;
>	dt = ktime_sub(ktime_get(), dt);
>	res[1] = rc6_residency(rc6);
>	if ((res[1] - res[0]) >> 10) {
> @@ -89,9 +89,9 @@ int live_rc6_manual(void *arg)
>	res[0] = rc6_residency(rc6);
>	intel_uncore_forcewake_flush(rc6_to_uncore(rc6), FORCEWAKE_ALL);
>	dt = ktime_get();
> -	rc6_power = libpower_get_energy_uJ();
> +	rc6_power = libpower_get_energy_uJ(gt->i915);
>	msleep(100);
> -	rc6_power = libpower_get_energy_uJ() - rc6_power;
> +	rc6_power = libpower_get_energy_uJ(gt->i915) - rc6_power;

So:
* arg for live_rps_power and live_rc6_manual is gt
* freq's are per gt
* rc6 residency is per gt

But the power/energy we are using is per device (gt->i915)? And we expect
device level power to be low when only one gt might be in rc6?

Shouldn't all these functions be per gt? Specially when we might have
multiple gt's soon.

Or if per gt functions don't make in all cases have both device and gt
level functions?

i915 hwmon provides both gt and device/package/card level energy but iGFX
till now reads a MSR which does not need gt or i915. So per gt I think
should work for now.

>	dt = ktime_sub(ktime_get(), dt);
>	res[1] = rc6_residency(rc6);
>	if (res[1] == res[0]) {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c b/drivers/gpu/drm/i915/gt/selftest_rps.c
> index b8b0b0c7617e..6732aa7d4792 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_rps.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_rps.c
> @@ -1090,38 +1090,38 @@ int live_rps_interrupt(void *arg)
>	return err;
>  }
>
> -static u64 __measure_power(int duration_ms)
> +static u64 __measure_power(struct intel_gt *gt, int duration_ms)
>  {
>	u64 dE, dt;
>
>	dt = ktime_get();
> -	dE = libpower_get_energy_uJ();
> +	dE = libpower_get_energy_uJ(gt->i915);
>	usleep_range(1000 * duration_ms, 2000 * duration_ms);
> -	dE = libpower_get_energy_uJ() - dE;
> +	dE = libpower_get_energy_uJ(gt->i915) - dE;
>	dt = ktime_get() - dt;
>
>	return div64_u64(1000 * 1000 * dE, dt);
>  }
>
> -static u64 measure_power(struct intel_rps *rps, int *freq)
> +static u64 measure_power(struct intel_gt *gt, int *freq)
>  {
>	u64 x[5];
>	int i;
>
>	for (i = 0; i < 5; i++)
> -		x[i] = __measure_power(5);
> +		x[i] = __measure_power(gt, 5);
>
> -	*freq = (*freq + intel_rps_read_actual_frequency(rps)) / 2;
> +	*freq = (*freq + intel_rps_read_actual_frequency(&gt->rps)) / 2;
>
>	/* A simple triangle filter for better result stability */
>	sort(x, 5, sizeof(*x), cmp_u64, NULL);
>	return div_u64(x[1] + 2 * x[2] + x[3], 4);
>  }
>
> -static u64 measure_power_at(struct intel_rps *rps, int *freq)
> +static u64 measure_power_at(struct intel_gt *gt, int *freq)
>  {
> -	*freq = rps_set_check(rps, *freq);
> -	return measure_power(rps, freq);
> +	*freq = rps_set_check(&gt->rps, *freq);

Hmm looks like this whole live_rps_power stuff is only for host turbo, not
slpc. Anyway that's a future patch.

> +	return measure_power(gt, freq);
>  }
>
>  int live_rps_power(void *arg)
> @@ -1187,10 +1187,10 @@ int live_rps_power(void *arg)
>		}
>
>		max.freq = rps->max_freq;
> -		max.power = measure_power_at(rps, &max.freq);
> +		max.power = measure_power_at(gt, &max.freq);
>
>		min.freq = rps->min_freq;
> -		min.power = measure_power_at(rps, &min.freq);
> +		min.power = measure_power_at(gt, &min.freq);
>
>		igt_spinner_end(&spin);
>		st_engine_heartbeat_enable(engine);
> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> index fc1cdda82ec6..c4b14f2b268c 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
> @@ -78,7 +78,7 @@ static u64 measure_power_at_freq(struct intel_gt *gt, int *freq, u64 *power)
>	if (err)
>		return err;
>	*freq = intel_rps_read_actual_frequency(&gt->rps);
> -	*power = measure_power(&gt->rps, freq);
> +	*power = measure_power(gt, freq);
>
>	return err;
>  }
> diff --git a/drivers/gpu/drm/i915/selftests/libpower.c b/drivers/gpu/drm/i915/selftests/libpower.c
> index c66e993c5f85..df37cba30353 100644
> --- a/drivers/gpu/drm/i915/selftests/libpower.c
> +++ b/drivers/gpu/drm/i915/selftests/libpower.c
> @@ -6,29 +6,30 @@
>  #include <asm/msr.h>
>
>  #include "i915_drv.h"
> +#include "i915_hwmon.h"
>  #include "libpower.h"
>
> -bool libpower_supported(const struct drm_i915_private *i915)
> -{
> -	/* Discrete cards require hwmon integration */
> -	if (IS_DGFX(i915))
> -		return false;
> -
> -	return libpower_get_energy_uJ();
> -}
> -
> -u64 libpower_get_energy_uJ(void)
> +u64 libpower_get_energy_uJ(struct drm_i915_private *i915)
>  {
>	unsigned long long power;
>	u32 units;
> +	long energy_uJ = 0;
>
> -	if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
> -		return 0;
> +	if (IS_DGFX(i915)) {
> +#if IS_REACHABLE(CONFIG_HWMON)
> +		if (i915_hwmon_get_energy(i915, &energy_uJ))
> +#endif

No IS_REACHABLE stuff here. Declare i915_hwmon_get_energy similar to other
functions in i915_hwmon.h.

Thanks.
--
Ashutosh



> +			return 0;
> +	} else {
> +		if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
> +			return 0;
>
> -	units = (power & 0x1f00) >> 8;
> +		units = (power & 0x1f00) >> 8;
>
> -	if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
> -		return 0;
> +		if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
> +			return 0;
>
> -	return (1000000 * power) >> units; /* convert to uJ */
> +		energy_uJ = (1000000 * power) >> units; /* convert to uJ */
> +	}
> +	return energy_uJ;
>  }
> diff --git a/drivers/gpu/drm/i915/selftests/libpower.h b/drivers/gpu/drm/i915/selftests/libpower.h
> index 5352981eb946..03a44611f9e9 100644
> --- a/drivers/gpu/drm/i915/selftests/libpower.h
> +++ b/drivers/gpu/drm/i915/selftests/libpower.h
> @@ -10,8 +10,10 @@
>
>  struct drm_i915_private;
>
> -bool libpower_supported(const struct drm_i915_private *i915);
> -
> -u64 libpower_get_energy_uJ(void);
> +u64 libpower_get_energy_uJ(struct drm_i915_private *i915);
>
> +static inline bool libpower_supported(struct drm_i915_private *i915)
> +{
> +	return libpower_get_energy_uJ(i915);
> +}
>  #endif /* SELFTEST_LIBPOWER_H */
> --
> 2.25.1
>

Re: [Intel-gfx] [PATCH v3 3/3] drm/i915/selftests: Add hwmon support in libpower for dgfx

[Tauro, Riana]

On 11/22/2022 8:10 AM, Dixit, Ashutosh wrote:
> On Sun, 20 Nov 2022 23:29:46 -0800, Riana Tauro wrote:
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c
>> index 15b84c428f66..845058ed83ed 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_rc6.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c
>> @@ -61,9 +61,9 @@ int live_rc6_manual(void *arg)
>> 	res[0] = rc6_residency(rc6);
>>
>> 	dt = ktime_get();
>> -	rc0_power = libpower_get_energy_uJ();
>> +	rc0_power = libpower_get_energy_uJ(gt->i915);
>> 	msleep(250);
>> -	rc0_power = libpower_get_energy_uJ() - rc0_power;
>> +	rc0_power = libpower_get_energy_uJ(gt->i915) - rc0_power;
>> 	dt = ktime_sub(ktime_get(), dt);
>> 	res[1] = rc6_residency(rc6);
>> 	if ((res[1] - res[0]) >> 10) {
>> @@ -89,9 +89,9 @@ int live_rc6_manual(void *arg)
>> 	res[0] = rc6_residency(rc6);
>> 	intel_uncore_forcewake_flush(rc6_to_uncore(rc6), FORCEWAKE_ALL);
>> 	dt = ktime_get();
>> -	rc6_power = libpower_get_energy_uJ();
>> +	rc6_power = libpower_get_energy_uJ(gt->i915);
>> 	msleep(100);
>> -	rc6_power = libpower_get_energy_uJ() - rc6_power;
>> +	rc6_power = libpower_get_energy_uJ(gt->i915) - rc6_power;
> 
> So:
> * arg for live_rps_power and live_rc6_manual is gt
> * freq's are per gt
> * rc6 residency is per gt
> 
> But the power/energy we are using is per device (gt->i915)? And we expect
> device level power to be low when only one gt might be in rc6?
> 
> Shouldn't all these functions be per gt? Specially when we might have
> multiple gt's soon.
> 
> Or if per gt functions don't make in all cases have both device and gt
> level functions?
> 
> i915 hwmon provides both gt and device/package/card level energy but iGFX
> till now reads a MSR which does not need gt or i915. So per gt I think
> should work for now.

Will modify to return per-gt values if present else
the default device level energy.


> 
>> 	dt = ktime_sub(ktime_get(), dt);
>> 	res[1] = rc6_residency(rc6);
>> 	if (res[1] == res[0]) {
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_rps.c b/drivers/gpu/drm/i915/gt/selftest_rps.c
>> index b8b0b0c7617e..6732aa7d4792 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_rps.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_rps.c
>> @@ -1090,38 +1090,38 @@ int live_rps_interrupt(void *arg)
>> 	return err;
>>   }
>>
>> -static u64 __measure_power(int duration_ms)
>> +static u64 __measure_power(struct intel_gt *gt, int duration_ms)
>>   {
>> 	u64 dE, dt;
>>
>> 	dt = ktime_get();
>> -	dE = libpower_get_energy_uJ();
>> +	dE = libpower_get_energy_uJ(gt->i915);
>> 	usleep_range(1000 * duration_ms, 2000 * duration_ms);
>> -	dE = libpower_get_energy_uJ() - dE;
>> +	dE = libpower_get_energy_uJ(gt->i915) - dE;
>> 	dt = ktime_get() - dt;
>>
>> 	return div64_u64(1000 * 1000 * dE, dt);
>>   }
>>
>> -static u64 measure_power(struct intel_rps *rps, int *freq)
>> +static u64 measure_power(struct intel_gt *gt, int *freq)
>>   {
>> 	u64 x[5];
>> 	int i;
>>
>> 	for (i = 0; i < 5; i++)
>> -		x[i] = __measure_power(5);
>> +		x[i] = __measure_power(gt, 5);
>>
>> -	*freq = (*freq + intel_rps_read_actual_frequency(rps)) / 2;
>> +	*freq = (*freq + intel_rps_read_actual_frequency(&gt->rps)) / 2;
>>
>> 	/* A simple triangle filter for better result stability */
>> 	sort(x, 5, sizeof(*x), cmp_u64, NULL);
>> 	return div_u64(x[1] + 2 * x[2] + x[3], 4);
>>   }
>>
>> -static u64 measure_power_at(struct intel_rps *rps, int *freq)
>> +static u64 measure_power_at(struct intel_gt *gt, int *freq)
>>   {
>> -	*freq = rps_set_check(rps, *freq);
>> -	return measure_power(rps, freq);
>> +	*freq = rps_set_check(&gt->rps, *freq);
> 
> Hmm looks like this whole live_rps_power stuff is only for host turbo, not
> slpc. Anyway that's a future patch.
live_slpc_power is present in selftest_slpc.c.

> 
>> +	return measure_power(gt, freq);
>>   }
>>
>>   int live_rps_power(void *arg)
>> @@ -1187,10 +1187,10 @@ int live_rps_power(void *arg)
>> 		}
>>
>> 		max.freq = rps->max_freq;
>> -		max.power = measure_power_at(rps, &max.freq);
>> +		max.power = measure_power_at(gt, &max.freq);
>>
>> 		min.freq = rps->min_freq;
>> -		min.power = measure_power_at(rps, &min.freq);
>> +		min.power = measure_power_at(gt, &min.freq);
>>
>> 		igt_spinner_end(&spin);
>> 		st_engine_heartbeat_enable(engine);
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_slpc.c b/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> index fc1cdda82ec6..c4b14f2b268c 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_slpc.c
>> @@ -78,7 +78,7 @@ static u64 measure_power_at_freq(struct intel_gt *gt, int *freq, u64 *power)
>> 	if (err)
>> 		return err;
>> 	*freq = intel_rps_read_actual_frequency(&gt->rps);
>> -	*power = measure_power(&gt->rps, freq);
>> +	*power = measure_power(gt, freq);
>>
>> 	return err;
>>   }
Changes for slpc present here.
>> diff --git a/drivers/gpu/drm/i915/selftests/libpower.c b/drivers/gpu/drm/i915/selftests/libpower.c
>> index c66e993c5f85..df37cba30353 100644
>> --- a/drivers/gpu/drm/i915/selftests/libpower.c
>> +++ b/drivers/gpu/drm/i915/selftests/libpower.c
>> @@ -6,29 +6,30 @@
>>   #include <asm/msr.h>
>>
>>   #include "i915_drv.h"
>> +#include "i915_hwmon.h"
>>   #include "libpower.h"
>>
>> -bool libpower_supported(const struct drm_i915_private *i915)
>> -{
>> -	/* Discrete cards require hwmon integration */
>> -	if (IS_DGFX(i915))
>> -		return false;
>> -
>> -	return libpower_get_energy_uJ();
>> -}
>> -
>> -u64 libpower_get_energy_uJ(void)
>> +u64 libpower_get_energy_uJ(struct drm_i915_private *i915)
>>   {
>> 	unsigned long long power;
>> 	u32 units;
>> +	long energy_uJ = 0;
>>
>> -	if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
>> -		return 0;
>> +	if (IS_DGFX(i915)) {
>> +#if IS_REACHABLE(CONFIG_HWMON)
>> +		if (i915_hwmon_get_energy(i915, &energy_uJ))
>> +#endif
> 
> No IS_REACHABLE stuff here. Declare i915_hwmon_get_energy similar to other
> functions in i915_hwmon.h.
> 
> Thanks.
> --
> Ashutosh
> 
> 
Thanks for the review comments. Will address in next rev.

Thanks
Riana
> 
>> +			return 0;
>> +	} else {
>> +		if (rdmsrl_safe(MSR_RAPL_POWER_UNIT, &power))
>> +			return 0;
>>
>> -	units = (power & 0x1f00) >> 8;
>> +		units = (power & 0x1f00) >> 8;
>>
>> -	if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
>> -		return 0;
>> +		if (rdmsrl_safe(MSR_PP1_ENERGY_STATUS, &power))
>> +			return 0;
>>
>> -	return (1000000 * power) >> units; /* convert to uJ */
>> +		energy_uJ = (1000000 * power) >> units; /* convert to uJ */
>> +	}
>> +	return energy_uJ;
>>   }
>> diff --git a/drivers/gpu/drm/i915/selftests/libpower.h b/drivers/gpu/drm/i915/selftests/libpower.h
>> index 5352981eb946..03a44611f9e9 100644
>> --- a/drivers/gpu/drm/i915/selftests/libpower.h
>> +++ b/drivers/gpu/drm/i915/selftests/libpower.h
>> @@ -10,8 +10,10 @@
>>
>>   struct drm_i915_private;
>>
>> -bool libpower_supported(const struct drm_i915_private *i915);
>> -
>> -u64 libpower_get_energy_uJ(void);
>> +u64 libpower_get_energy_uJ(struct drm_i915_private *i915);
>>
>> +static inline bool libpower_supported(struct drm_i915_private *i915)
>> +{
>> +	return libpower_get_energy_uJ(i915);
>> +}
>>   #endif /* SELFTEST_LIBPOWER_H */
>> --
>> 2.25.1
>>