diff mbox series

[v2,3/6] drm/xe/hwmon: Expose card reactive critical power

Message ID 20230627183043.2024530-4-badal.nilawar@intel.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Add HWMON support for DGFX | expand

Commit Message

Nilawar, Badal June 27, 2023, 6:30 p.m. UTC 
Expose the card reactive critical (I1) power. I1 is exposed as
power1_crit in microwatts (typically for client products) or as
curr1_crit in milliamperes (typically for server).
This is port from i915 hwmon.

v2: Move PCODE_MBOX macro to pcode file (Riana)

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  26 +++++
 drivers/gpu/drm/xe/xe_hwmon.c                 | 106 +++++++++++++++++-
 drivers/gpu/drm/xe/xe_pcode.h                 |   5 +
 drivers/gpu/drm/xe/xe_pcode_api.h             |   7 ++
 4 files changed, 143 insertions(+), 1 deletion(-)

Comments

Andi Shyti June 29, 2023, 2:40 p.m. UTC | #1 
Hi Badal,

> Expose the card reactive critical (I1) power. I1 is exposed as
> power1_crit in microwatts (typically for client products) or as
> curr1_crit in milliamperes (typically for server).
> This is port from i915 hwmon.

Should this, then be a more generic framework for more gpu
drivers? Now we are having some code duplication.

[...]

>  hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
>  {
>  	u32 reg_val;
> +	u32 uval;
>  
>  	switch (attr) {
>  	case hwmon_power_max:
> @@ -248,6 +274,9 @@ hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
>  	case hwmon_power_rated_max:
>  		return process_hwmon_reg(ddat, pkg_power_sku,
>  					 reg_read, &reg_val, 0, 0) ? 0 : 0444;
> +	case hwmon_power_crit:
> +		return (hwm_pcode_read_i1(ddat->gt, &uval) ||
> +			!(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;

I like better the form below, with

	err = hwm_pcode_read...()
	if (!err)
		return 0;
	
	return !(uval & ....

>  	default:
>  		return 0;

[...]

> +static umode_t
> +hwm_curr_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> +{
> +	u32 uval;
> +
> +	switch (attr) {
> +	case hwmon_curr_crit:
> +		return (hwm_pcode_read_i1(ddat->gt, &uval) ||
> +			(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;

this is a pattern that is repeated quite often, should this be a
separate function, maybe?

Andi

[...]
Dixit, Ashutosh July 6, 2023, 7:05 p.m. UTC | #2 
On Thu, 29 Jun 2023 07:40:00 -0700, Andi Shyti wrote:
>

Hi Andi,

> > Expose the card reactive critical (I1) power. I1 is exposed as
> > power1_crit in microwatts (typically for client products) or as
> > curr1_crit in milliamperes (typically for server).
> > This is port from i915 hwmon.
>
> Should this, then be a more generic framework for more gpu
> drivers? Now we are having some code duplication.

There are several subsystems where we will see such duplication. These
include things like PMU, OA which are either on the mailing list of in
preparation.

You can argue there is already code duplication between XE and i915, it's
just hidden in xe way of doing things.

The only way to avoid code duplication would be to invent sharing code
between i915 and xe as display has done. It may be possible in some cases
but in others sharing code will make matters worse for both xe and i915.

So in general I am not seeing a way around "code duplication".

Thanks.
--
Ashutosh
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index ff3465195870..bee1d62bfddb 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -20,3 +20,29 @@  Description:	RO. Card default power limit (default TDP setting).
 
 		Only supported for particular Intel xe graphics platforms.
 
+What:		/sys/devices/.../hwmon/hwmon<i>/power1_crit
+Date:		July 2023
+KernelVersion:	6.3
+Contact:	intel-gfx@lists.freedesktop.org
+Description:	RW. Card reactive critical (I1) power limit in microwatts.
+
+		Card reactive critical (I1) power limit in microwatts is exposed
+		for client products. The power controller will throttle the
+		operating frequency if the power averaged over a window exceeds
+		this limit.
+
+		Only supported for particular Intel xe graphics platforms.
+
+What:		/sys/devices/.../hwmon/hwmon<i>/curr1_crit
+Date:		July 2023
+KernelVersion:	6.3
+Contact:	intel-gfx@lists.freedesktop.org
+Description:	RW. Card reactive critical (I1) power limit in milliamperes.
+
+		Card reactive critical (I1) power limit in milliamperes is
+		exposed for server products. The power controller will throttle
+		the operating frequency if the power averaged over a window
+		exceeds this limit.
+
+		Only supported for particular Intel xe graphics platforms.
+
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index a4fba29d5d5a..7068120d9200 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -11,6 +11,9 @@ 
 #include "xe_hwmon.h"
 #include "xe_mmio.h"
 #include "xe_gt.h"
+#include "i915_drv.h"
+#include "xe_pcode.h"
+#include "xe_pcode_api.h"
 
 enum hwm_reg_name {
 	pkg_rapl_limit,
@@ -27,8 +30,10 @@  enum hwm_reg_operation {
 /*
  * SF_* - scale factors for particular quantities according to hwmon spec.
  * - power  - microwatts
+ * - curr   - milliamperes
  */
 #define SF_POWER	1000000
+#define SF_CURR		1000
 
 struct hwm_drvdata {
 	struct xe_hwmon *hwmon;
@@ -232,14 +237,35 @@  static int hwm_power_rated_max_read(struct hwm_drvdata *ddat, long *value)
 }
 
 static const struct hwmon_channel_info *hwm_info[] = {
-	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
+	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
+	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
 	NULL
 };
 
+/* I1 is exposed as power_crit or as curr_crit depending on bit 31 */
+static int hwm_pcode_read_i1(struct xe_gt *gt, u32 *uval)
+{
+	/* Avoid ILLEGAL_SUBCOMMAND "mailbox access failed" warning in snb_pcode_read */
+	if (IS_DG2(gt_to_xe(gt)))
+		return -ENXIO;
+
+	return xe_pcode_read(gt, PCODE_MBOX(PCODE_POWER_SETUP,
+			     POWER_SETUP_SUBCOMMAND_READ_I1, 0),
+			     uval, 0);
+}
+
+static int hwm_pcode_write_i1(struct xe_gt *gt, u32 uval)
+{
+	return xe_pcode_write(gt, PCODE_MBOX(PCODE_POWER_SETUP,
+			      POWER_SETUP_SUBCOMMAND_WRITE_I1, 0),
+			      uval);
+}
+
 static umode_t
 hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
 {
 	u32 reg_val;
+	u32 uval;
 
 	switch (attr) {
 	case hwmon_power_max:
@@ -248,6 +274,9 @@  hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
 	case hwmon_power_rated_max:
 		return process_hwmon_reg(ddat, pkg_power_sku,
 					 reg_read, &reg_val, 0, 0) ? 0 : 0444;
+	case hwmon_power_crit:
+		return (hwm_pcode_read_i1(ddat->gt, &uval) ||
+			!(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
 	default:
 		return 0;
 	}
@@ -256,11 +285,23 @@  hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
 static int
 hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
 {
+	int ret;
+	u32 uval;
+
 	switch (attr) {
 	case hwmon_power_max:
 		return hwm_power_max_read(ddat, val);
 	case hwmon_power_rated_max:
 		return hwm_power_rated_max_read(ddat, val);
+	case hwmon_power_crit:
+		ret = hwm_pcode_read_i1(ddat->gt, &uval);
+		if (ret)
+			return ret;
+		if (!(uval & POWER_SETUP_I1_WATTS))
+			return -ENODEV;
+		*val = mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
+				       SF_POWER, POWER_SETUP_I1_SHIFT);
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -269,9 +310,14 @@  hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
 static int
 hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
 {
+	u32 uval;
+
 	switch (attr) {
 	case hwmon_power_max:
 		return hwm_power_max_write(ddat, val);
+	case hwmon_power_crit:
+		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
+		return hwm_pcode_write_i1(ddat->gt, uval);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -327,6 +373,55 @@  void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
 	xe_device_mem_access_put(gt_to_xe(ddat->gt));
 }
 
+static umode_t
+hwm_curr_is_visible(const struct hwm_drvdata *ddat, u32 attr)
+{
+	u32 uval;
+
+	switch (attr) {
+	case hwmon_curr_crit:
+		return (hwm_pcode_read_i1(ddat->gt, &uval) ||
+			(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwm_curr_read(struct hwm_drvdata *ddat, u32 attr, long *val)
+{
+	int ret;
+	u32 uval;
+
+	switch (attr) {
+	case hwmon_curr_crit:
+		ret = hwm_pcode_read_i1(ddat->gt, &uval);
+		if (ret)
+			return ret;
+		if (uval & POWER_SETUP_I1_WATTS)
+			return -ENODEV;
+		*val = mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
+				       SF_CURR, POWER_SETUP_I1_SHIFT);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int
+hwm_curr_write(struct hwm_drvdata *ddat, u32 attr, long val)
+{
+	u32 uval;
+
+	switch (attr) {
+	case hwmon_curr_crit:
+		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_CURR);
+		return hwm_pcode_write_i1(ddat->gt, uval);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t
 hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	       u32 attr, int channel)
@@ -340,6 +435,9 @@  hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	case hwmon_power:
 		ret = hwm_power_is_visible(ddat, attr, channel);
 		break;
+	case hwmon_curr:
+		ret = hwm_curr_is_visible(ddat, attr);
+		break;
 	default:
 		ret = 0;
 	}
@@ -362,6 +460,9 @@  hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_power:
 		ret = hwm_power_read(ddat, attr, channel, val);
 		break;
+	case hwmon_curr:
+		ret = hwm_curr_read(ddat, attr, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -385,6 +486,9 @@  hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_power:
 		ret = hwm_power_write(ddat, attr, channel, val);
 		break;
+	case hwmon_curr:
+		ret = hwm_curr_write(ddat, attr, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
index 3b4aa8c1a3ba..08cb1d047cba 100644
--- a/drivers/gpu/drm/xe/xe_pcode.h
+++ b/drivers/gpu/drm/xe/xe_pcode.h
@@ -22,4 +22,9 @@  int xe_pcode_write_timeout(struct xe_gt *gt, u32 mbox, u32 val,
 int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
 		     u32 reply_mask, u32 reply, int timeout_ms);
 
+#define PCODE_MBOX(mbcmd, param1, param2)\
+	(FIELD_PREP(PCODE_MB_COMMAND, mbcmd)\
+	| FIELD_PREP(PCODE_MB_PARAM1, param1)\
+	| FIELD_PREP(PCODE_MB_PARAM2, param2))
+
 #endif
diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
index 837ff7c71280..5935cfe30204 100644
--- a/drivers/gpu/drm/xe/xe_pcode_api.h
+++ b/drivers/gpu/drm/xe/xe_pcode_api.h
@@ -35,6 +35,13 @@ 
 #define     DGFX_GET_INIT_STATUS	0x0
 #define     DGFX_INIT_STATUS_COMPLETE	0x1
 
+#define   PCODE_POWER_SETUP			0x7C
+#define     POWER_SETUP_SUBCOMMAND_READ_I1	0x4
+#define     POWER_SETUP_SUBCOMMAND_WRITE_I1	0x5
+#define	    POWER_SETUP_I1_WATTS		REG_BIT(31)
+#define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
+#define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
+
 struct pcode_err_decode {
 	int errno;
 	const char *str;