[PATCH v3 0/6] Add HWMON support for DGFX

[PATCH v3 0/6] Add HWMON support for DGFX

[Badal Nilawar]

This series adds the hwmon support on xe driver for 
DGFX. This is ported from i915 hwmon. 

v3: Fix review comments (Matt Brost/Andi) 
  

Badal Nilawar (6):
  drm/xe/hwmon: Add HWMON infrastructure
  drm/xe/hwmon: Expose power attributes
  drm/xe/hwmon: Expose card reactive critical power
  drm/xe/hwmon: Expose input voltage attribute
  drm/xe/hwmon: Expose hwmon energy attribute
  drm/xe/hwmon: Expose power1_max_interval

 .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  77 ++
 drivers/gpu/drm/xe/Makefile                   |   3 +
 drivers/gpu/drm/xe/regs/xe_gt_regs.h          |   9 +
 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h      |  45 +
 drivers/gpu/drm/xe/xe_device.c                |   5 +
 drivers/gpu/drm/xe/xe_device_types.h          |   2 +
 drivers/gpu/drm/xe/xe_hwmon.c                 | 958 ++++++++++++++++++
 drivers/gpu/drm/xe/xe_hwmon.h                 |  26 +
 drivers/gpu/drm/xe/xe_macros.h                |   3 +
 drivers/gpu/drm/xe/xe_pcode.h                 |   5 +
 drivers/gpu/drm/xe/xe_pcode_api.h             |   7 +
 drivers/gpu/drm/xe/xe_uc.c                    |   6 +
 12 files changed, 1146 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
 create mode 100644 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
 create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
 create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h

-- 
2.25.1

[PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Badal Nilawar]

The xe HWMON module will be used to expose voltage, power and energy
values for dGfx. Here we set up xe hwmon infrastructure including xe
hwmon registration, basic data structures and functions.

v2:
  - Fix review comments (Riana)
v3:
  - %s/hwm_/hwmon/ (Matt Brost)
  - Use drmm_mutex_init (Matt Brost)
  - Print error value (Matt Brost)
  - %s/hwmon_drvdata/xe_hwmon_data/
  - Move rpm (xe_device_mem_access_get/put) calls
    to this patch (Matt Brost)

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 drivers/gpu/drm/xe/Makefile          |   3 +
 drivers/gpu/drm/xe/xe_device.c       |   5 +
 drivers/gpu/drm/xe/xe_device_types.h |   2 +
 drivers/gpu/drm/xe/xe_hwmon.c        | 150 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_hwmon.h        |  22 ++++
 5 files changed, 182 insertions(+)
 create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
 create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 4ea9e3150c20..831be23e000b 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -116,6 +116,9 @@ xe-y += xe_bb.o \
 	xe_wa.o \
 	xe_wopcm.o
 
+# graphics hardware monitoring (HWMON) support
+xe-$(CONFIG_HWMON) += xe_hwmon.o
+
 # i915 Display compat #defines and #includes
 subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
 	-I$(srctree)/$(src)/display/ext \
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 5409cf7895d3..01bd08812514 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -34,6 +34,7 @@
 #include "xe_vm.h"
 #include "xe_vm_madvise.h"
 #include "xe_wait_user_fence.h"
+#include "xe_hwmon.h"
 
 #ifdef CONFIG_LOCKDEP
 struct lockdep_map xe_device_mem_access_lockdep_map = {
@@ -335,6 +336,8 @@ int xe_device_probe(struct xe_device *xe)
 
 	xe_debugfs_register(xe);
 
+	xe_hwmon_register(xe);
+
 	err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
 	if (err)
 		return err;
@@ -361,6 +364,8 @@ static void xe_device_remove_display(struct xe_device *xe)
 
 void xe_device_remove(struct xe_device *xe)
 {
+	xe_hwmon_unregister(xe);
+
 	xe_device_remove_display(xe);
 
 	xe_display_unlink(xe);
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index b156f69d7320..dd06eba815ec 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -376,6 +376,8 @@ struct xe_device {
 	 */
 	struct task_struct *pm_callback_task;
 
+	struct xe_hwmon *hwmon;
+
 	/* private: */
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
new file mode 100644
index 000000000000..5e35128a61a8
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include <linux/hwmon.h>
+
+#include <drm/drm_managed.h>
+#include "regs/xe_gt_regs.h"
+#include "xe_device.h"
+#include "xe_hwmon.h"
+
+struct xe_hwmon_data {
+	struct device *hwmon_dev;
+	struct xe_gt *gt;
+	char name[12];
+};
+
+struct xe_hwmon {
+	struct xe_hwmon_data ddat;
+	struct mutex hwmon_lock;
+};
+
+static const struct hwmon_channel_info *hwmon_info[] = {
+	NULL
+};
+
+static umode_t
+hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+		 u32 attr, int channel)
+{
+	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	switch (type) {
+	default:
+		ret = 0;
+		break;
+	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
+}
+
+static int
+hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	   int channel, long *val)
+{
+	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	switch (type) {
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
+}
+
+static int
+hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	    int channel, long val)
+{
+	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	switch (type) {
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
+}
+
+static const struct hwmon_ops hwmon_ops = {
+	.is_visible = hwmon_is_visible,
+	.read = hwmon_read,
+	.write = hwmon_write,
+};
+
+static const struct hwmon_chip_info hwmon_chip_info = {
+	.ops = &hwmon_ops,
+	.info = hwmon_info,
+};
+
+static void
+hwmon_get_preregistration_info(struct xe_device *xe)
+{
+}
+
+void xe_hwmon_register(struct xe_device *xe)
+{
+	struct device *dev = xe->drm.dev;
+	struct xe_hwmon *hwmon;
+	struct device *hwmon_dev;
+	struct xe_hwmon_data *ddat;
+
+	/* hwmon is available only for dGfx */
+	if (!IS_DGFX(xe))
+		return;
+
+	hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+	if (!hwmon)
+		return;
+
+	xe->hwmon = hwmon;
+	drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
+
+	ddat = &hwmon->ddat;
+
+	/* primary GT to access device level properties */
+	ddat->gt = xe->tiles[0].primary_gt;
+
+	snprintf(ddat->name, sizeof(ddat->name), "xe");
+
+	hwmon_get_preregistration_info(xe);
+
+	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
+
+	/*  hwmon_dev points to device hwmon<i> */
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
+							 ddat,
+							 &hwmon_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev)) {
+		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
+		xe->hwmon = NULL;
+		return;
+	}
+
+	ddat->hwmon_dev = hwmon_dev;
+}
+
+void xe_hwmon_unregister(struct xe_device *xe)
+{
+	xe->hwmon = NULL;
+}
diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
new file mode 100644
index 000000000000..a078eeb0a68b
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_hwmon.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef __XE_HWMON_H__
+#define __XE_HWMON_H__
+
+#include <linux/types.h>
+
+struct xe_device;
+
+#if IS_REACHABLE(CONFIG_HWMON)
+void xe_hwmon_register(struct xe_device *xe);
+void xe_hwmon_unregister(struct xe_device *xe);
+#else
+static inline void xe_hwmon_register(struct xe_device *xe) { };
+static inline void xe_hwmon_unregister(struct xe_device *xe) { };
+#endif
+
+#endif /* __XE_HWMON_H__ */
-- 
2.25.1

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/2/23 06:52, Badal Nilawar wrote:
> The xe HWMON module will be used to expose voltage, power and energy
> values for dGfx. Here we set up xe hwmon infrastructure including xe
> hwmon registration, basic data structures and functions.
> 
> v2:
>    - Fix review comments (Riana)
> v3:
>    - %s/hwm_/hwmon/ (Matt Brost)
>    - Use drmm_mutex_init (Matt Brost)
>    - Print error value (Matt Brost)
>    - %s/hwmon_drvdata/xe_hwmon_data/
>    - Move rpm (xe_device_mem_access_get/put) calls
>      to this patch (Matt Brost)
> 
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   drivers/gpu/drm/xe/Makefile          |   3 +
>   drivers/gpu/drm/xe/xe_device.c       |   5 +
>   drivers/gpu/drm/xe/xe_device_types.h |   2 +
>   drivers/gpu/drm/xe/xe_hwmon.c        | 150 +++++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_hwmon.h        |  22 ++++
>   5 files changed, 182 insertions(+)
>   create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
>   create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 4ea9e3150c20..831be23e000b 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -116,6 +116,9 @@ xe-y += xe_bb.o \
>   	xe_wa.o \
>   	xe_wopcm.o
>   
> +# graphics hardware monitoring (HWMON) support
> +xe-$(CONFIG_HWMON) += xe_hwmon.o
> +
>   # i915 Display compat #defines and #includes
>   subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
>   	-I$(srctree)/$(src)/display/ext \
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 5409cf7895d3..01bd08812514 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -34,6 +34,7 @@
>   #include "xe_vm.h"
>   #include "xe_vm_madvise.h"
>   #include "xe_wait_user_fence.h"
> +#include "xe_hwmon.h"
>   
>   #ifdef CONFIG_LOCKDEP
>   struct lockdep_map xe_device_mem_access_lockdep_map = {
> @@ -335,6 +336,8 @@ int xe_device_probe(struct xe_device *xe)
>   
>   	xe_debugfs_register(xe);
>   
> +	xe_hwmon_register(xe);
> +
>   	err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
>   	if (err)
>   		return err;
> @@ -361,6 +364,8 @@ static void xe_device_remove_display(struct xe_device *xe)
>   
>   void xe_device_remove(struct xe_device *xe)
>   {
> +	xe_hwmon_unregister(xe);
> +
>   	xe_device_remove_display(xe);
>   
>   	xe_display_unlink(xe);
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index b156f69d7320..dd06eba815ec 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -376,6 +376,8 @@ struct xe_device {
>   	 */
>   	struct task_struct *pm_callback_task;
>   
> +	struct xe_hwmon *hwmon;
> +
>   	/* private: */
>   
>   #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> new file mode 100644
> index 000000000000..5e35128a61a8
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/hwmon.h>
> +
> +#include <drm/drm_managed.h>
> +#include "regs/xe_gt_regs.h"
> +#include "xe_device.h"
> +#include "xe_hwmon.h"
> +
> +struct xe_hwmon_data {
> +	struct device *hwmon_dev;
> +	struct xe_gt *gt;
> +	char name[12];
> +};
> +
> +struct xe_hwmon {
> +	struct xe_hwmon_data ddat;
> +	struct mutex hwmon_lock;
> +};
> +
> +static const struct hwmon_channel_info *hwmon_info[] = {
> +	NULL
> +};
> +
> +static umode_t
> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> +		 u32 attr, int channel)
> +{
> +	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
> +	int ret;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> +	switch (type) {
> +	default:
> +		ret = 0;
> +		break;
> +	}
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	return ret;
> +}
> +
> +static int
> +hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +	   int channel, long *val)
> +{
> +	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
> +	int ret;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> +	switch (type) {
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	return ret;
> +}
> +
> +static int
> +hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +	    int channel, long val)
> +{
> +	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
> +	int ret;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> +	switch (type) {
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	return ret;
> +}
> +
> +static const struct hwmon_ops hwmon_ops = {
> +	.is_visible = hwmon_is_visible,
> +	.read = hwmon_read,
> +	.write = hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info hwmon_chip_info = {
> +	.ops = &hwmon_ops,
> +	.info = hwmon_info,
> +};
> +
> +static void
> +hwmon_get_preregistration_info(struct xe_device *xe)
> +{
> +}
> +
> +void xe_hwmon_register(struct xe_device *xe)
> +{
> +	struct device *dev = xe->drm.dev;
> +	struct xe_hwmon *hwmon;
> +	struct device *hwmon_dev;
> +	struct xe_hwmon_data *ddat;
> +
> +	/* hwmon is available only for dGfx */
> +	if (!IS_DGFX(xe))
> +		return;
> +
> +	hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> +	if (!hwmon)
> +		return;
> +
> +	xe->hwmon = hwmon;
> +	drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
> +
> +	ddat = &hwmon->ddat;
> +
> +	/* primary GT to access device level properties */
> +	ddat->gt = xe->tiles[0].primary_gt;
> +
> +	snprintf(ddat->name, sizeof(ddat->name), "xe");

Why not just pass "xe" as string to devm_hwmon_device_register_with_info() ?

> +
> +	hwmon_get_preregistration_info(xe);
> +
> +	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
> +
> +	/*  hwmon_dev points to device hwmon<i> */
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> +							 ddat,
> +							 &hwmon_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev)) {
> +		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
> +		xe->hwmon = NULL;
> +		return;
> +	}
> +

What is xe->hwmon used for other than for setting it to NULL
in xe_hwmon_unregister() ?

> +	ddat->hwmon_dev = hwmon_dev;
> +}
> +
> +void xe_hwmon_unregister(struct xe_device *xe)
> +{
> +	xe->hwmon = NULL;
> +}

Not sure I understand why this function is needed. Please explain.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
> new file mode 100644
> index 000000000000..a078eeb0a68b
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __XE_HWMON_H__
> +#define __XE_HWMON_H__
> +
> +#include <linux/types.h>
> +
> +struct xe_device;
> +
> +#if IS_REACHABLE(CONFIG_HWMON)
> +void xe_hwmon_register(struct xe_device *xe);
> +void xe_hwmon_unregister(struct xe_device *xe);
> +#else
> +static inline void xe_hwmon_register(struct xe_device *xe) { };
> +static inline void xe_hwmon_unregister(struct xe_device *xe) { };
> +#endif
> +
> +#endif /* __XE_HWMON_H__ */

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Andi Shyti]

Hi Badal,

[...]

> +struct xe_hwmon_data {
> +	struct device *hwmon_dev;
> +	struct xe_gt *gt;
> +	char name[12];
> +};
> +
> +struct xe_hwmon {
> +	struct xe_hwmon_data ddat;
> +	struct mutex hwmon_lock;
> +};

why do we need two structures here? Can we merge them?

> +static const struct hwmon_channel_info *hwmon_info[] = {
> +	NULL
> +};

just:

  static const struct hwmon_channel_info *hwmon_info[] = { };

would do.

> +static umode_t
> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> +		 u32 attr, int channel)
> +{
> +	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
> +	int ret;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> +	switch (type) {
> +	default:
> +		ret = 0;
> +		break;
> +	}
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	return ret;

OK... we are forced to go through the switch and initialize ret.
Would be nicer to initialize ret to '0'... but it's not
important, feel free to ignore this comment if the compiler
doesn't complain.

> +}

[...]

> +	/*  hwmon_dev points to device hwmon<i> */
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> +							 ddat,
> +							 &hwmon_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev)) {
> +		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));

I think this is better:

   drm_warn(&xe->drm, "Fail to register xe hwmon (%pe)\n", hwmon_dev);

> +		xe->hwmon = NULL;
> +		return;
> +	}
> +
> +	ddat->hwmon_dev = hwmon_dev;
> +}
> +
> +void xe_hwmon_unregister(struct xe_device *xe)
> +{
> +	xe->hwmon = NULL;

I think this is not necessary. Will xe check for hwmon at some
point?

Andi

> +}

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/2/23 15:40, Andi Shyti wrote:
> Hi Badal,
> 
> [...]
> 
>> +struct xe_hwmon_data {
>> +	struct device *hwmon_dev;
>> +	struct xe_gt *gt;
>> +	char name[12];
>> +};
>> +
>> +struct xe_hwmon {
>> +	struct xe_hwmon_data ddat;
>> +	struct mutex hwmon_lock;
>> +};
> 
> why do we need two structures here? Can we merge them?
> 
>> +static const struct hwmon_channel_info *hwmon_info[] = {
>> +	NULL
>> +};
> 
> just:
> 
>    static const struct hwmon_channel_info *hwmon_info[] = { };
> 
> would do.
> 

What would be the point ? This is just a placeholder, and we need
a NULL entry at the end.

>> +static umode_t
>> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>> +		 u32 attr, int channel)
>> +{
>> +	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
>> +	int ret;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
>> +	switch (type) {
>> +	default:
>> +		ret = 0;
>> +		break;
>> +	}
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +
>> +	return ret;
> 
> OK... we are forced to go through the switch and initialize ret.
> Would be nicer to initialize ret to '0'... but it's not
> important, feel free to ignore this comment if the compiler
> doesn't complain.
> 
>> +}
> 
> [...]
> 
>> +	/*  hwmon_dev points to device hwmon<i> */
>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>> +							 ddat,
>> +							 &hwmon_chip_info,
>> +							 NULL);
>> +	if (IS_ERR(hwmon_dev)) {
>> +		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
> 
> I think this is better:
> 
>     drm_warn(&xe->drm, "Fail to register xe hwmon (%pe)\n", hwmon_dev);
> 
>> +		xe->hwmon = NULL;
>> +		return;
>> +	}
>> +
>> +	ddat->hwmon_dev = hwmon_dev;
>> +}
>> +
>> +void xe_hwmon_unregister(struct xe_device *xe)
>> +{
>> +	xe->hwmon = NULL;
> 
> I think this is not necessary. Will xe check for hwmon at some
> point?
> 
> Andi
> 
>> +}

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Andi Shyti]

Hi Guenter,

[...]

> > > +static const struct hwmon_channel_info *hwmon_info[] = {
> > > +	NULL
> > > +};
> > 
> > just:
> > 
> >    static const struct hwmon_channel_info *hwmon_info[] = { };
> > 
> > would do.
> > 
> 
> What would be the point ? This is just a placeholder, and we need
> a NULL entry at the end.

right... this series needs to be read from the end to the
beginning :)

Andi

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/2/23 16:34, Andi Shyti wrote:
> Hi Guenter,
> 
> [...]
> 
>>>> +static const struct hwmon_channel_info *hwmon_info[] = {
>>>> +	NULL
>>>> +};
>>>
>>> just:
>>>
>>>     static const struct hwmon_channel_info *hwmon_info[] = { };
>>>
>>> would do.
>>>
>>
>> What would be the point ? This is just a placeholder, and we need
>> a NULL entry at the end.
> 
> right... this series needs to be read from the end to the
> beginning :)
> 

Or applied completely, review the result, and then dig back to
the individual patches to provide feedback. I got severely confused
when trying to review the series. I am not really sure if the split
into multiple patches is really making the review easier; it seems
to me it does the opposite.

Guenter

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/2/23 15:40, Andi Shyti wrote:
> Hi Badal,
> 
> [...]
> 
>> +struct xe_hwmon_data {
>> +	struct device *hwmon_dev;
>> +	struct xe_gt *gt;
>> +	char name[12];
>> +};
>> +
>> +struct xe_hwmon {
>> +	struct xe_hwmon_data ddat;
>> +	struct mutex hwmon_lock;
>> +};
> 
> why do we need two structures here? Can we merge them?
> 

A later patch adds multiple hwmon devices which makes use of it.
I think that is flawed, and I am not inclined to accept it.

Guenter

>> +static const struct hwmon_channel_info *hwmon_info[] = {
>> +	NULL
>> +};
> 
> just:
> 
>    static const struct hwmon_channel_info *hwmon_info[] = { };
> 
> would do.
> 
>> +static umode_t
>> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>> +		 u32 attr, int channel)
>> +{
>> +	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
>> +	int ret;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
>> +	switch (type) {
>> +	default:
>> +		ret = 0;
>> +		break;
>> +	}
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +
>> +	return ret;
> 
> OK... we are forced to go through the switch and initialize ret.
> Would be nicer to initialize ret to '0'... but it's not
> important, feel free to ignore this comment if the compiler
> doesn't complain.
> 
>> +}
> 
> [...]
> 
>> +	/*  hwmon_dev points to device hwmon<i> */
>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>> +							 ddat,
>> +							 &hwmon_chip_info,
>> +							 NULL);
>> +	if (IS_ERR(hwmon_dev)) {
>> +		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
> 
> I think this is better:
> 
>     drm_warn(&xe->drm, "Fail to register xe hwmon (%pe)\n", hwmon_dev);
> 
>> +		xe->hwmon = NULL;
>> +		return;
>> +	}
>> +
>> +	ddat->hwmon_dev = hwmon_dev;
>> +}
>> +
>> +void xe_hwmon_unregister(struct xe_device *xe)
>> +{
>> +	xe->hwmon = NULL;
> 
> I think this is not necessary. Will xe check for hwmon at some
> point?
> 
> Andi
> 
>> +}

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Nilawar, Badal]

Hi Guenter,
On 03-08-2023 04:42, Guenter Roeck wrote:
> On 8/2/23 15:40, Andi Shyti wrote:
>> Hi Badal,
>>
>> [...]
>>
>>> +struct xe_hwmon_data {
>>> +    struct device *hwmon_dev;
>>> +    struct xe_gt *gt;
>>> +    char name[12];
>>> +};
>>> +
>>> +struct xe_hwmon {
>>> +    struct xe_hwmon_data ddat;
>>> +    struct mutex hwmon_lock;
>>> +};
>>
>> why do we need two structures here? Can we merge them?
>>
> 
> A later patch adds multiple hwmon devices which makes use of it.
> I think that is flawed, and I am not inclined to accept it.
Is there any obvious reason that there shouldn't be multiple devices? In 
i915 we are doing the same. 
https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3

Regards,
Badal
> 
> Guenter
> 
>>> +static const struct hwmon_channel_info *hwmon_info[] = {
>>> +    NULL
>>> +};
>>
>> just:
>>
>>    static const struct hwmon_channel_info *hwmon_info[] = { };
>>
>> would do.
>>
>>> +static umode_t
>>> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>>> +         u32 attr, int channel)
>>> +{
>>> +    struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
>>> +    int ret;
>>> +
>>> +    xe_device_mem_access_get(gt_to_xe(ddat->gt));
>>> +
>>> +    switch (type) {
>>> +    default:
>>> +        ret = 0;
>>> +        break;
>>> +    }
>>> +
>>> +    xe_device_mem_access_put(gt_to_xe(ddat->gt));
>>> +
>>> +    return ret;
>>
>> OK... we are forced to go through the switch and initialize ret.
>> Would be nicer to initialize ret to '0'... but it's not
>> important, feel free to ignore this comment if the compiler
>> doesn't complain.
>>
>>> +}
>>
>> [...]
>>
>>> +    /*  hwmon_dev points to device hwmon<i> */
>>> +    hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>>> +                             ddat,
>>> +                             &hwmon_chip_info,
>>> +                             NULL);
>>> +    if (IS_ERR(hwmon_dev)) {
>>> +        drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", 
>>> PTR_ERR(hwmon_dev));
>>
>> I think this is better:
>>
>>     drm_warn(&xe->drm, "Fail to register xe hwmon (%pe)\n", hwmon_dev);
>>
>>> +        xe->hwmon = NULL;
>>> +        return;
>>> +    }
>>> +
>>> +    ddat->hwmon_dev = hwmon_dev;
>>> +}
>>> +
>>> +void xe_hwmon_unregister(struct xe_device *xe)
>>> +{
>>> +    xe->hwmon = NULL;
>>
>> I think this is not necessary. Will xe check for hwmon at some
>> point?
>>
>> Andi
>>
>>> +}
> 

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/4/23 06:19, Nilawar, Badal wrote:
> 
> Hi Guenter,
> On 03-08-2023 04:42, Guenter Roeck wrote:
>> On 8/2/23 15:40, Andi Shyti wrote:
>>> Hi Badal,
>>>
>>> [...]
>>>
>>>> +struct xe_hwmon_data {
>>>> +    struct device *hwmon_dev;
>>>> +    struct xe_gt *gt;
>>>> +    char name[12];
>>>> +};
>>>> +
>>>> +struct xe_hwmon {
>>>> +    struct xe_hwmon_data ddat;
>>>> +    struct mutex hwmon_lock;
>>>> +};
>>>
>>> why do we need two structures here? Can we merge them?
>>>
>>
>> A later patch adds multiple hwmon devices which makes use of it.
>> I think that is flawed, and I am not inclined to accept it.
> Is there any obvious reason that there shouldn't be multiple devices? In i915 we are doing the same. https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
> 

Technically you can do whatever you like as long as the code doesn't reside
in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
either. i915 shouldn't do it, but I didn't realize what they are doing
at the time. Other drivers doing it wrong is not an argument. You can't
argue that you may drive faster than the speed limit because others do it
or because police didn't stop you last time you did either.

One chip, one hwmon device. Do you have separate parent devices for
all your hwmon devices ? If yes, you can argue that having multiple hwmon
devices make sense. If not, you can't.

Guenter

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Nilawar, Badal]

On 04-08-2023 19:56, Guenter Roeck wrote:
> On 8/4/23 06:19, Nilawar, Badal wrote:
>>
>> Hi Guenter,
>> On 03-08-2023 04:42, Guenter Roeck wrote:
>>> On 8/2/23 15:40, Andi Shyti wrote:
>>>> Hi Badal,
>>>>
>>>> [...]
>>>>
>>>>> +struct xe_hwmon_data {
>>>>> +    struct device *hwmon_dev;
>>>>> +    struct xe_gt *gt;
>>>>> +    char name[12];
>>>>> +};
>>>>> +
>>>>> +struct xe_hwmon {
>>>>> +    struct xe_hwmon_data ddat;
>>>>> +    struct mutex hwmon_lock;
>>>>> +};
>>>>
>>>> why do we need two structures here? Can we merge them?
>>>>
>>>
>>> A later patch adds multiple hwmon devices which makes use of it.
>>> I think that is flawed, and I am not inclined to accept it.
>> Is there any obvious reason that there shouldn't be multiple devices? 
>> In i915 we are doing the same. 
>> https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
>>
> 
> Technically you can do whatever you like as long as the code doesn't reside
> in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
> either. i915 shouldn't do it, but I didn't realize what they are doing
> at the time. Other drivers doing it wrong is not an argument. You can't
> argue that you may drive faster than the speed limit because others do it
> or because police didn't stop you last time you did either.
> 
> One chip, one hwmon device. Do you have separate parent devices for
> all your hwmon devices ? If yes, you can argue that having multiple hwmon
> devices make sense. If not, you can't.
Thanks for clarification. There is only one parent device. So will try 
to accommodate single hwmon device.

Regards,
Badal
> 
> Guenter
> 

Re: [Intel-xe] [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Rodrigo Vivi]

On Fri, Aug 04, 2023 at 08:06:22PM +0530, Nilawar, Badal wrote:
> 
> 
> On 04-08-2023 19:56, Guenter Roeck wrote:
> > On 8/4/23 06:19, Nilawar, Badal wrote:
> > > 
> > > Hi Guenter,
> > > On 03-08-2023 04:42, Guenter Roeck wrote:
> > > > On 8/2/23 15:40, Andi Shyti wrote:
> > > > > Hi Badal,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > +struct xe_hwmon_data {
> > > > > > +    struct device *hwmon_dev;
> > > > > > +    struct xe_gt *gt;
> > > > > > +    char name[12];
> > > > > > +};
> > > > > > +
> > > > > > +struct xe_hwmon {
> > > > > > +    struct xe_hwmon_data ddat;
> > > > > > +    struct mutex hwmon_lock;
> > > > > > +};
> > > > > 
> > > > > why do we need two structures here? Can we merge them?
> > > > > 
> > > > 
> > > > A later patch adds multiple hwmon devices which makes use of it.
> > > > I think that is flawed, and I am not inclined to accept it.
> > > Is there any obvious reason that there shouldn't be multiple
> > > devices? In i915 we are doing the same.
> > > https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
> > > 
> > 
> > Technically you can do whatever you like as long as the code doesn't reside
> > in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
> > either. i915 shouldn't do it, but I didn't realize what they are doing
> > at the time. Other drivers doing it wrong is not an argument. You can't
> > argue that you may drive faster than the speed limit because others do it
> > or because police didn't stop you last time you did either.
> > 
> > One chip, one hwmon device. Do you have separate parent devices for
> > all your hwmon devices ? If yes, you can argue that having multiple hwmon
> > devices make sense. If not, you can't.
> Thanks for clarification. There is only one parent device. So will try to
> accommodate single hwmon device.

Well, it is one PCI device, but under 1 pci device we can have multiple "tiles"
that can duplicate many components. Inside each tile we can even have multiple
"gt"s.

But back to the tile, each tile has its own metrics. It's own power delivery,
own sensors and all. They can even be seen as independent devices from this
angle.

I'm afraid that the attempt to put everything as one device, but all the
entries duplicated per tile/gt we might end up with a messed api.

> 
> Regards,
> Badal
> > 
> > Guenter
> > 

Re: [Intel-xe] [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/8/23 14:31, Rodrigo Vivi wrote:
> On Fri, Aug 04, 2023 at 08:06:22PM +0530, Nilawar, Badal wrote:
>>
>>
>> On 04-08-2023 19:56, Guenter Roeck wrote:
>>> On 8/4/23 06:19, Nilawar, Badal wrote:
>>>>
>>>> Hi Guenter,
>>>> On 03-08-2023 04:42, Guenter Roeck wrote:
>>>>> On 8/2/23 15:40, Andi Shyti wrote:
>>>>>> Hi Badal,
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +struct xe_hwmon_data {
>>>>>>> +    struct device *hwmon_dev;
>>>>>>> +    struct xe_gt *gt;
>>>>>>> +    char name[12];
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct xe_hwmon {
>>>>>>> +    struct xe_hwmon_data ddat;
>>>>>>> +    struct mutex hwmon_lock;
>>>>>>> +};
>>>>>>
>>>>>> why do we need two structures here? Can we merge them?
>>>>>>
>>>>>
>>>>> A later patch adds multiple hwmon devices which makes use of it.
>>>>> I think that is flawed, and I am not inclined to accept it.
>>>> Is there any obvious reason that there shouldn't be multiple
>>>> devices? In i915 we are doing the same.
>>>> https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
>>>>
>>>
>>> Technically you can do whatever you like as long as the code doesn't reside
>>> in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
>>> either. i915 shouldn't do it, but I didn't realize what they are doing
>>> at the time. Other drivers doing it wrong is not an argument. You can't
>>> argue that you may drive faster than the speed limit because others do it
>>> or because police didn't stop you last time you did either.
>>>
>>> One chip, one hwmon device. Do you have separate parent devices for
>>> all your hwmon devices ? If yes, you can argue that having multiple hwmon
>>> devices make sense. If not, you can't.
>> Thanks for clarification. There is only one parent device. So will try to
>> accommodate single hwmon device.
> 
> Well, it is one PCI device, but under 1 pci device we can have multiple "tiles"
> that can duplicate many components. Inside each tile we can even have multiple
> "gt"s.
> 
> But back to the tile, each tile has its own metrics. It's own power delivery,
> own sensors and all. They can even be seen as independent devices from this
> angle.
> 
> I'm afraid that the attempt to put everything as one device, but all the
> entries duplicated per tile/gt we might end up with a messed api.
> 

Your argument does not make sense. I am not asking to duplicate anything.

Guenter

Re: [Intel-xe] [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Rodrigo Vivi]

On Tue, Aug 08, 2023 at 03:07:43PM -0700, Guenter Roeck wrote:
> On 8/8/23 14:31, Rodrigo Vivi wrote:
> > On Fri, Aug 04, 2023 at 08:06:22PM +0530, Nilawar, Badal wrote:
> > > 
> > > 
> > > On 04-08-2023 19:56, Guenter Roeck wrote:
> > > > On 8/4/23 06:19, Nilawar, Badal wrote:
> > > > > 
> > > > > Hi Guenter,
> > > > > On 03-08-2023 04:42, Guenter Roeck wrote:
> > > > > > On 8/2/23 15:40, Andi Shyti wrote:
> > > > > > > Hi Badal,
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > +struct xe_hwmon_data {
> > > > > > > > +    struct device *hwmon_dev;
> > > > > > > > +    struct xe_gt *gt;
> > > > > > > > +    char name[12];
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +struct xe_hwmon {
> > > > > > > > +    struct xe_hwmon_data ddat;
> > > > > > > > +    struct mutex hwmon_lock;
> > > > > > > > +};
> > > > > > > 
> > > > > > > why do we need two structures here? Can we merge them?
> > > > > > > 
> > > > > > 
> > > > > > A later patch adds multiple hwmon devices which makes use of it.
> > > > > > I think that is flawed, and I am not inclined to accept it.
> > > > > Is there any obvious reason that there shouldn't be multiple
> > > > > devices? In i915 we are doing the same.
> > > > > https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
> > > > > 
> > > > 
> > > > Technically you can do whatever you like as long as the code doesn't reside
> > > > in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
> > > > either. i915 shouldn't do it, but I didn't realize what they are doing
> > > > at the time. Other drivers doing it wrong is not an argument. You can't
> > > > argue that you may drive faster than the speed limit because others do it
> > > > or because police didn't stop you last time you did either.
> > > > 
> > > > One chip, one hwmon device. Do you have separate parent devices for
> > > > all your hwmon devices ? If yes, you can argue that having multiple hwmon
> > > > devices make sense. If not, you can't.
> > > Thanks for clarification. There is only one parent device. So will try to
> > > accommodate single hwmon device.
> > 
> > Well, it is one PCI device, but under 1 pci device we can have multiple "tiles"
> > that can duplicate many components. Inside each tile we can even have multiple
> > "gt"s.
> > 
> > But back to the tile, each tile has its own metrics. It's own power delivery,
> > own sensors and all. They can even be seen as independent devices from this
> > angle.
> > 
> > I'm afraid that the attempt to put everything as one device, but all the
> > entries duplicated per tile/gt we might end up with a messed api.
> > 
> 
> Your argument does not make sense. I am not asking to duplicate anything.

Okay, I'm sorry, maybe 'duplication' was a bad choice of words from my part.

You had told that having multiple hwmon device for a single chip was not
acceptable.

But I'm trying to explain that we have a hardware architecture where the graphics
is duplicated in 'tiles' inside the same PCI card. Each tile with its
own sensors and monitoring systems. And also an extra sensors monitoring the
entire 'package' that includes the tiles and the SoC.
So 1 hwmon device per gt-tile and package sound the appropriated way to me.

Your lines had convinced Badal to get them all and merge in a single hwmon
device. If we do this, the API will get messed up.

And this is what I meant by 'messed up':
quoting Badal:
"""
With single device energy entries will look like hwmonxx/energy1_input,
energy2_input, energy3_input.
To identify which entry for what need to expose additional entry energyX_lable
which will contain ("package", "gtN")
"""

I am arguing that for this tiled ('sub-device') hw architecture the
initial patch from Badal, that is the same way implemented in i915 is
absolutely the right way to go with the current infrastructure.

One idea that I had the first time that I saw this was if it wouldn't
be acceptable for instance in hwmon an infra with named (numbered?)
sub-directories inside the the hwmon device. But I couldn't not find any
other hwmon usage out there that could justify a big change in the core of
hwmon like that.

Well, but maybe we are indeed missing some other better way of handling
this sub-device case with the current hwmon infrastructure. If so, I'd
like to hear more about the suggestions and get some initial pointers.

Thanks so much for your time and help here,
Rodrigo.

> 
> Guenter
> 

Re: [Intel-xe] [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Guenter Roeck]

On 8/11/23 09:01, Rodrigo Vivi wrote:
> On Tue, Aug 08, 2023 at 03:07:43PM -0700, Guenter Roeck wrote:
>> On 8/8/23 14:31, Rodrigo Vivi wrote:
>>> On Fri, Aug 04, 2023 at 08:06:22PM +0530, Nilawar, Badal wrote:
>>>>
>>>>
>>>> On 04-08-2023 19:56, Guenter Roeck wrote:
>>>>> On 8/4/23 06:19, Nilawar, Badal wrote:
>>>>>>
>>>>>> Hi Guenter,
>>>>>> On 03-08-2023 04:42, Guenter Roeck wrote:
>>>>>>> On 8/2/23 15:40, Andi Shyti wrote:
>>>>>>>> Hi Badal,
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> +struct xe_hwmon_data {
>>>>>>>>> +    struct device *hwmon_dev;
>>>>>>>>> +    struct xe_gt *gt;
>>>>>>>>> +    char name[12];
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +struct xe_hwmon {
>>>>>>>>> +    struct xe_hwmon_data ddat;
>>>>>>>>> +    struct mutex hwmon_lock;
>>>>>>>>> +};
>>>>>>>>
>>>>>>>> why do we need two structures here? Can we merge them?
>>>>>>>>
>>>>>>>
>>>>>>> A later patch adds multiple hwmon devices which makes use of it.
>>>>>>> I think that is flawed, and I am not inclined to accept it.
>>>>>> Is there any obvious reason that there shouldn't be multiple
>>>>>> devices? In i915 we are doing the same.
>>>>>> https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
>>>>>>
>>>>>
>>>>> Technically you can do whatever you like as long as the code doesn't reside
>>>>> in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
>>>>> either. i915 shouldn't do it, but I didn't realize what they are doing
>>>>> at the time. Other drivers doing it wrong is not an argument. You can't
>>>>> argue that you may drive faster than the speed limit because others do it
>>>>> or because police didn't stop you last time you did either.
>>>>>
>>>>> One chip, one hwmon device. Do you have separate parent devices for
>>>>> all your hwmon devices ? If yes, you can argue that having multiple hwmon
>>>>> devices make sense. If not, you can't.
>>>> Thanks for clarification. There is only one parent device. So will try to
>>>> accommodate single hwmon device.
>>>
>>> Well, it is one PCI device, but under 1 pci device we can have multiple "tiles"
>>> that can duplicate many components. Inside each tile we can even have multiple
>>> "gt"s.
>>>
>>> But back to the tile, each tile has its own metrics. It's own power delivery,
>>> own sensors and all. They can even be seen as independent devices from this
>>> angle.
>>>
>>> I'm afraid that the attempt to put everything as one device, but all the
>>> entries duplicated per tile/gt we might end up with a messed api.
>>>
>>
>> Your argument does not make sense. I am not asking to duplicate anything.
> 
> Okay, I'm sorry, maybe 'duplication' was a bad choice of words from my part.
> 
> You had told that having multiple hwmon device for a single chip was not
> acceptable.
> 
> But I'm trying to explain that we have a hardware architecture where the graphics
> is duplicated in 'tiles' inside the same PCI card. Each tile with its
> own sensors and monitoring systems. And also an extra sensors monitoring the
> entire 'package' that includes the tiles and the SoC.
> So 1 hwmon device per gt-tile and package sound the appropriated way to me.
> 

No, it isn't. Next you are going to tell me to split CPU temperature devices
in the same way because they are split in "tiles" on the same CPU core.

> Your lines had convinced Badal to get them all and merge in a single hwmon
> device. If we do this, the API will get messed up.
> 
> And this is what I meant by 'messed up':
> quoting Badal:
> """
> With single device energy entries will look like hwmonxx/energy1_input,
> energy2_input, energy3_input.
> To identify which entry for what need to expose additional entry energyX_lable
> which will contain ("package", "gtN")

So what is the problem with that ? That is a description and not "messed up".

Guenter

Re: [Intel-xe] [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Rodrigo Vivi]

On Fri, Aug 11, 2023 at 10:39:00AM -0700, Guenter Roeck wrote:
> On 8/11/23 09:01, Rodrigo Vivi wrote:
> > On Tue, Aug 08, 2023 at 03:07:43PM -0700, Guenter Roeck wrote:
> > > On 8/8/23 14:31, Rodrigo Vivi wrote:
> > > > On Fri, Aug 04, 2023 at 08:06:22PM +0530, Nilawar, Badal wrote:
> > > > > 
> > > > > 
> > > > > On 04-08-2023 19:56, Guenter Roeck wrote:
> > > > > > On 8/4/23 06:19, Nilawar, Badal wrote:
> > > > > > > 
> > > > > > > Hi Guenter,
> > > > > > > On 03-08-2023 04:42, Guenter Roeck wrote:
> > > > > > > > On 8/2/23 15:40, Andi Shyti wrote:
> > > > > > > > > Hi Badal,
> > > > > > > > > 
> > > > > > > > > [...]
> > > > > > > > > 
> > > > > > > > > > +struct xe_hwmon_data {
> > > > > > > > > > +    struct device *hwmon_dev;
> > > > > > > > > > +    struct xe_gt *gt;
> > > > > > > > > > +    char name[12];
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +struct xe_hwmon {
> > > > > > > > > > +    struct xe_hwmon_data ddat;
> > > > > > > > > > +    struct mutex hwmon_lock;
> > > > > > > > > > +};
> > > > > > > > > 
> > > > > > > > > why do we need two structures here? Can we merge them?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > A later patch adds multiple hwmon devices which makes use of it.
> > > > > > > > I think that is flawed, and I am not inclined to accept it.
> > > > > > > Is there any obvious reason that there shouldn't be multiple
> > > > > > > devices? In i915 we are doing the same.
> > > > > > > https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
> > > > > > > 
> > > > > > 
> > > > > > Technically you can do whatever you like as long as the code doesn't reside
> > > > > > in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
> > > > > > either. i915 shouldn't do it, but I didn't realize what they are doing
> > > > > > at the time. Other drivers doing it wrong is not an argument. You can't
> > > > > > argue that you may drive faster than the speed limit because others do it
> > > > > > or because police didn't stop you last time you did either.
> > > > > > 
> > > > > > One chip, one hwmon device. Do you have separate parent devices for
> > > > > > all your hwmon devices ? If yes, you can argue that having multiple hwmon
> > > > > > devices make sense. If not, you can't.
> > > > > Thanks for clarification. There is only one parent device. So will try to
> > > > > accommodate single hwmon device.
> > > > 
> > > > Well, it is one PCI device, but under 1 pci device we can have multiple "tiles"
> > > > that can duplicate many components. Inside each tile we can even have multiple
> > > > "gt"s.
> > > > 
> > > > But back to the tile, each tile has its own metrics. It's own power delivery,
> > > > own sensors and all. They can even be seen as independent devices from this
> > > > angle.
> > > > 
> > > > I'm afraid that the attempt to put everything as one device, but all the
> > > > entries duplicated per tile/gt we might end up with a messed api.
> > > > 
> > > 
> > > Your argument does not make sense. I am not asking to duplicate anything.
> > 
> > Okay, I'm sorry, maybe 'duplication' was a bad choice of words from my part.
> > 
> > You had told that having multiple hwmon device for a single chip was not
> > acceptable.
> > 
> > But I'm trying to explain that we have a hardware architecture where the graphics
> > is duplicated in 'tiles' inside the same PCI card. Each tile with its
> > own sensors and monitoring systems. And also an extra sensors monitoring the
> > entire 'package' that includes the tiles and the SoC.
> > So 1 hwmon device per gt-tile and package sound the appropriated way to me.
> > 
> 
> No, it isn't. Next you are going to tell me to split CPU temperature devices
> in the same way because they are split in "tiles" on the same CPU core.

okay, let's align with coretemp then.

> 
> > Your lines had convinced Badal to get them all and merge in a single hwmon
> > device. If we do this, the API will get messed up.
> > 
> > And this is what I meant by 'messed up':
> > quoting Badal:
> > """
> > With single device energy entries will look like hwmonxx/energy1_input,
> > energy2_input, energy3_input.
> > To identify which entry for what need to expose additional entry energyX_lable
> > which will contain ("package", "gtN")
> 
> So what is the problem with that ? That is a description and not "messed up".

From the user space perspective it would be easier to get an unique handle
for the subdevice (directory) and then inspect each property (files) with single
and direct access, without having to inspect every single 'label' file of that
property in the directory to match the desired sub-device.

But nevermind. Let's have a single hwmon per device with multiple label files
and aligning with cputemp.

> 
> Guenter
> 

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Nilawar, Badal]

On 04-08-2023 19:56, Guenter Roeck wrote:
> On 8/4/23 06:19, Nilawar, Badal wrote:
>>
>> Hi Guenter,
>> On 03-08-2023 04:42, Guenter Roeck wrote:
>>> On 8/2/23 15:40, Andi Shyti wrote:
>>>> Hi Badal,
>>>>
>>>> [...]
>>>>
>>>>> +struct xe_hwmon_data {
>>>>> +    struct device *hwmon_dev;
>>>>> +    struct xe_gt *gt;
>>>>> +    char name[12];
>>>>> +};
>>>>> +
>>>>> +struct xe_hwmon {
>>>>> +    struct xe_hwmon_data ddat;
>>>>> +    struct mutex hwmon_lock;
>>>>> +};
>>>>
>>>> why do we need two structures here? Can we merge them?
>>>>
>>>
>>> A later patch adds multiple hwmon devices which makes use of it.
>>> I think that is flawed, and I am not inclined to accept it.
>> Is there any obvious reason that there shouldn't be multiple devices? 
>> In i915 we are doing the same. 
>> https://patchwork.freedesktop.org/patch/497324/?series=104278&rev=3
>>
> 
> Technically you can do whatever you like as long as the code doesn't reside
> in drivers/hwmon. I won't NACK it, but I won't give it a Reviewed-by:
> either. i915 shouldn't do it, but I didn't realize what they are doing
> at the time. Other drivers doing it wrong is not an argument. You can't
> argue that you may drive faster than the speed limit because others do it
> or because police didn't stop you last time you did either.
> 
> One chip, one hwmon device. Do you have separate parent devices for
> all your hwmon devices ? If yes, you can argue that having multiple hwmon
> devices make sense. If not, you can't.
Thanks for clarification. There is only one parent device, so will try 
to accommodate one hwmon device approach.

Regards,
Badal
> 
> Guenter
> 

Re: [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Nilawar, Badal]

On 03-08-2023 04:10, Andi Shyti wrote:
> Hi Badal,
> 
> [...]
> 
>> +struct xe_hwmon_data {
>> +	struct device *hwmon_dev;
>> +	struct xe_gt *gt;
>> +	char name[12];
>> +};
>> +
>> +struct xe_hwmon {
>> +	struct xe_hwmon_data ddat;
>> +	struct mutex hwmon_lock;
>> +};
> 
> why do we need two structures here? Can we merge them?
In my previous series I mentioned its require to hold multiple hwmon 
devices.
> 
>> +static const struct hwmon_channel_info *hwmon_info[] = {
>> +	NULL
>> +};
> 
> just:
> 
>    static const struct hwmon_channel_info *hwmon_info[] = { };
> 
> would do.
sure
> 
>> +static umode_t
>> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>> +		 u32 attr, int channel)
>> +{
>> +	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
>> +	int ret;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
>> +	switch (type) {
>> +	default:
>> +		ret = 0;
>> +		break;
>> +	}
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +
>> +	return ret;
> 
> OK... we are forced to go through the switch and initialize ret.
> Would be nicer to initialize ret to '0'... but it's not
> important, feel free to ignore this comment if the compiler
> doesn't complain.
> 
>> +}
> 
> [...]
> 
>> +	/*  hwmon_dev points to device hwmon<i> */
>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>> +							 ddat,
>> +							 &hwmon_chip_info,
>> +							 NULL);
>> +	if (IS_ERR(hwmon_dev)) {
>> +		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
> 
> I think this is better:
> 
>     drm_warn(&xe->drm, "Fail to register xe hwmon (%pe)\n", hwmon_dev);
Sure
> 
>> +		xe->hwmon = NULL;
>> +		return;
>> +	}
>> +
>> +	ddat->hwmon_dev = hwmon_dev;
>> +}
>> +
>> +void xe_hwmon_unregister(struct xe_device *xe)
>> +{
>> +	xe->hwmon = NULL;
> 
> I think this is not necessary. Will xe check for hwmon at some
> point?
Yes this not needed as we are using devm_hwmon* function to register 
hwmon but in i915 patches this was suggested for sanity. I will remove 
this function.

Regards,
Badal
> 
> Andi
> 
>> +}

[PATCH v3 2/6] drm/xe/hwmon: Expose power attributes

[Badal Nilawar]

Expose power_max (pl1) and power_rated_max (tdp) attributes.

v2:
  - Fix review comments (Riana)
v3:
  - Convert enums to uppercase (Matt Brost)
  - Avoid extra reg read in hwmon_is_visible function (Riana)
  - Add XE_MISSING_CASE macro to warn on default case (Andi)
  - Serialize locking (Matt Brost, Andi)
  - Use xe_device_assert_mem_access when applicable (Matt Brost)
  - Add intel-xe@lists.freedesktop.org in Documentation (Matt Brost)

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  22 ++
 drivers/gpu/drm/xe/regs/xe_gt_regs.h          |   4 +
 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h      |  34 ++
 drivers/gpu/drm/xe/xe_hwmon.c                 | 304 +++++++++++++++++-
 drivers/gpu/drm/xe/xe_hwmon.h                 |   4 +
 drivers/gpu/drm/xe/xe_macros.h                |   3 +
 drivers/gpu/drm/xe/xe_uc.c                    |   6 +
 7 files changed, 375 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
 create mode 100644 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
new file mode 100644
index 000000000000..d48d98f903ed
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -0,0 +1,22 @@
+What:		/sys/devices/.../hwmon/hwmon<i>/power1_max
+Date:		August 2023
+KernelVersion:	6.4
+Contact:	intel-xe@lists.freedesktop.org
+Description:	RW. Card reactive sustained  (PL1/Tau) power limit in microwatts.
+
+		The power controller will throttle the operating frequency
+		if the power averaged over a window (typically seconds)
+		exceeds this limit. A read value of 0 means that the PL1
+		power limit is disabled, writing 0 disables the
+		limit. Writing values > 0 will enable the power limit.
+
+		Only supported for particular Intel xe graphics platforms.
+
+What:		/sys/devices/.../hwmon/hwmon<i>/power1_rated_max
+Date:		August 2023
+KernelVersion:	6.4
+Contact:	intel-xe@lists.freedesktop.org
+Description:	RO. Card default power limit (default TDP setting).
+
+		Only supported for particular Intel xe graphics platforms.
+
diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
index d654f3311351..eb7210afbd2c 100644
--- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
@@ -397,4 +397,8 @@
 #define XEHPC_BCS5_BCS6_INTR_MASK		XE_REG(0x190118)
 #define XEHPC_BCS7_BCS8_INTR_MASK		XE_REG(0x19011c)
 
+#define PVC_GT0_PACKAGE_RAPL_LIMIT		XE_REG(0x281008)
+#define PVC_GT0_PACKAGE_POWER_SKU_UNIT		XE_REG(0x281068)
+#define PVC_GT0_PACKAGE_POWER_SKU		XE_REG(0x281080)
+
 #endif
diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
new file mode 100644
index 000000000000..cb2d49b5c8a9
--- /dev/null
+++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef _XE_MCHBAR_REGS_H__
+#define _XE_MCHBAR_REGS_H_
+
+#include "regs/xe_reg_defs.h"
+
+/*
+ * MCHBAR mirror.
+ *
+ * This mirrors the MCHBAR MMIO space whose location is determined by
+ * device 0 function 0's pci config register 0x44 or 0x48 and matches it in
+ * every way.
+ */
+
+#define MCHBAR_MIRROR_BASE_SNB			0x140000
+
+#define PCU_CR_PACKAGE_POWER_SKU		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5930)
+#define   PKG_PKG_TDP				GENMASK_ULL(14, 0)
+#define   PKG_MIN_PWR				GENMASK_ULL(30, 16)
+#define   PKG_MAX_PWR				GENMASK_ULL(46, 32)
+
+#define PCU_CR_PACKAGE_POWER_SKU_UNIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
+#define   PKG_PWR_UNIT				REG_GENMASK(3, 0)
+
+#define PCU_CR_PACKAGE_RAPL_LIMIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
+#define   PKG_PWR_LIM_1				REG_GENMASK(14, 0)
+#define   PKG_PWR_LIM_1_EN			REG_BIT(15)
+
+#endif
+
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index 5e35128a61a8..ce8dac2168f6 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -6,9 +6,30 @@
 #include <linux/hwmon.h>
 
 #include <drm/drm_managed.h>
+#include "regs/xe_mchbar_regs.h"
 #include "regs/xe_gt_regs.h"
 #include "xe_device.h"
 #include "xe_hwmon.h"
+#include "xe_mmio.h"
+#include "xe_gt.h"
+
+enum hwmon_reg_name {
+	REG_PKG_RAPL_LIMIT,
+	REG_PKG_POWER_SKU,
+	REG_PKG_POWER_SKU_UNIT,
+};
+
+enum hwmon_reg_operation {
+	REG_READ,
+	REG_WRITE,
+	REG_RMW,
+};
+
+/*
+ * SF_* - scale factors for particular quantities according to hwmon spec.
+ * - power  - microwatts
+ */
+#define SF_POWER	1000000
 
 struct xe_hwmon_data {
 	struct device *hwmon_dev;
@@ -18,13 +39,268 @@ struct xe_hwmon_data {
 
 struct xe_hwmon {
 	struct xe_hwmon_data ddat;
-	struct mutex hwmon_lock;
+	struct mutex hwmon_lock; /* rmw operations*/
+	bool reset_in_progress;
+	wait_queue_head_t waitq;
+	int scl_shift_power;
 };
 
+#define ddat_to_xe_hwmon(ddat)	({ container_of(ddat, struct xe_hwmon, ddat); })
+
+static u32 hwmon_get_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_name)
+{
+	struct xe_device *xe = gt_to_xe(ddat->gt);
+	struct xe_reg reg = XE_REG(0);
+
+	switch (reg_name) {
+	case REG_PKG_RAPL_LIMIT:
+		if (xe->info.platform == XE_DG2)
+			reg = PCU_CR_PACKAGE_RAPL_LIMIT;
+		else if (xe->info.platform == XE_PVC)
+			reg = PVC_GT0_PACKAGE_RAPL_LIMIT;
+		break;
+	case REG_PKG_POWER_SKU:
+		if (xe->info.platform == XE_DG2)
+			reg = PCU_CR_PACKAGE_POWER_SKU;
+		else if (xe->info.platform == XE_PVC)
+			reg = PVC_GT0_PACKAGE_POWER_SKU;
+		break;
+	case REG_PKG_POWER_SKU_UNIT:
+		if (xe->info.platform == XE_DG2)
+			reg = PCU_CR_PACKAGE_POWER_SKU_UNIT;
+		else if (xe->info.platform == XE_PVC)
+			reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
+		break;
+	default:
+		XE_MISSING_CASE(reg_name);
+		break;
+	}
+
+	return reg.raw;
+}
+
+static int process_hwmon_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_name,
+			     enum hwmon_reg_operation operation, u32 *value,
+			     u32 clr, u32 set)
+{
+	struct xe_reg reg;
+	int ret = 0;
+
+	reg.raw = hwmon_get_reg(ddat, reg_name);
+
+	if (!reg.raw)
+		return -EOPNOTSUPP;
+
+	switch (operation) {
+	case REG_READ:
+		*value = xe_mmio_read32(ddat->gt, reg);
+		break;
+	case REG_WRITE:
+		xe_mmio_write32(ddat->gt, reg, *value);
+		break;
+	case REG_RMW:
+		*value = xe_mmio_rmw32(ddat->gt, reg, clr, set);
+		break;
+	default:
+		XE_MISSING_CASE(operation);
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+	return ret;
+}
+
+int process_hwmon_reg_read64(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_name, u64 *value)
+{
+	struct xe_reg reg;
+
+	reg.raw = hwmon_get_reg(ddat, reg_name);
+
+	if (!reg.raw)
+		return -EOPNOTSUPP;
+
+	*value = xe_mmio_read64(ddat->gt, reg);
+
+	return 0;
+}
+
+#define PL1_DISABLE 0
+
+/*
+ * HW allows arbitrary PL1 limits to be set but silently clamps these values to
+ * "typical but not guaranteed" min/max values in rg.PKG_POWER_SKU. Follow the
+ * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
+ * clamped values when read.
+ */
+static int hwmon_power_max_read(struct xe_hwmon_data *ddat, long *value)
+{
+	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
+	u32 reg_val;
+	u64 r, min, max;
+
+	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val, 0, 0);
+	/* Check if PL1 limit is disabled */
+	if (!(reg_val & PKG_PWR_LIM_1_EN)) {
+		*value = PL1_DISABLE;
+		return 0;
+	}
+
+	reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
+	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
+
+	process_hwmon_reg_read64(ddat, REG_PKG_POWER_SKU, &r);
+	min = REG_FIELD_GET(PKG_MIN_PWR, r);
+	min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
+	max = REG_FIELD_GET(PKG_MAX_PWR, r);
+	max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
+
+	if (min && max)
+		*value = clamp_t(u64, *value, min, max);
+
+	return 0;
+}
+
+static inline bool check_reset_in_progress(struct xe_hwmon *hwmon)
+{
+	mutex_lock(&hwmon->hwmon_lock);
+	if (!hwmon->reset_in_progress)
+		return true;
+	mutex_unlock(&hwmon->hwmon_lock);
+		return false;
+}
+
+static int hwmon_power_max_write(struct xe_hwmon_data *ddat, long value)
+{
+	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
+	DEFINE_WAIT(wait);
+	int ret = 0;
+	u32 nval;
+
+	/* hwmon->hwmon_lock remain held till rmw operation is over */
+	wait_event(hwmon->waitq, check_reset_in_progress(hwmon));
+
+	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
+	if (value == PL1_DISABLE) {
+		process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &nval,
+				  PKG_PWR_LIM_1_EN, 0);
+		process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_READ, &nval,
+				  PKG_PWR_LIM_1_EN, 0);
+
+		if (nval & PKG_PWR_LIM_1_EN)
+			ret = -ENODEV;
+		goto unlock;
+	}
+
+	/* Computation in 64-bits to avoid overflow. Round to nearest. */
+	nval = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
+	nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
+
+	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &nval,
+			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
+unlock:
+	mutex_unlock(&hwmon->hwmon_lock);
+
+	return 0;
+}
+
+static int hwmon_power_rated_max_read(struct xe_hwmon_data *ddat, long *value)
+{
+	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
+	u32 reg_val;
+
+	process_hwmon_reg(ddat, REG_PKG_POWER_SKU, REG_READ, &reg_val, 0, 0);
+	reg_val = REG_FIELD_GET(PKG_PKG_TDP, reg_val);
+	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
+
+	return 0;
+}
+
 static const struct hwmon_channel_info *hwmon_info[] = {
+	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
 	NULL
 };
 
+static umode_t
+hwmon_power_is_visible(struct xe_hwmon_data *ddat, u32 attr, int chan)
+{
+	switch (attr) {
+	case hwmon_power_max:
+		return hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT) ? 0664 : 0;
+	case hwmon_power_rated_max:
+		return hwmon_get_reg(ddat, REG_PKG_POWER_SKU) ? 0444 : 0;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwmon_power_read(struct xe_hwmon_data *ddat, u32 attr, int chan, long *val)
+{
+	switch (attr) {
+	case hwmon_power_max:
+		return hwmon_power_max_read(ddat, val);
+	case hwmon_power_rated_max:
+		return hwmon_power_rated_max_read(ddat, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int
+hwmon_power_write(struct xe_hwmon_data *ddat, u32 attr, int chan, long val)
+{
+	switch (attr) {
+	case hwmon_power_max:
+		return hwmon_power_max_write(ddat, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old)
+{
+	struct xe_hwmon *hwmon = xe->hwmon;
+	struct xe_hwmon_data *ddat = &hwmon->ddat;
+	u32 r;
+
+	if (!(hwmon && hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT)))
+		return;
+
+	xe_device_assert_mem_access(gt_to_xe(ddat->gt));
+
+	mutex_lock(&hwmon->hwmon_lock);
+
+	hwmon->reset_in_progress = true;
+
+	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &r,
+			  PKG_PWR_LIM_1_EN, 0);
+	*old = !!(r & PKG_PWR_LIM_1_EN);
+
+	mutex_unlock(&hwmon->hwmon_lock);
+}
+
+void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
+{
+	struct xe_hwmon *hwmon = xe->hwmon;
+	struct xe_hwmon_data *ddat = &hwmon->ddat;
+	u32 r;
+
+	if (!(hwmon && hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT)))
+		return;
+
+	xe_device_assert_mem_access(gt_to_xe(ddat->gt));
+
+	mutex_lock(&hwmon->hwmon_lock);
+
+	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &r,
+			  PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0);
+
+	hwmon->reset_in_progress = false;
+	wake_up_all(&hwmon->waitq);
+
+	mutex_unlock(&hwmon->hwmon_lock);
+}
+
 static umode_t
 hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 		 u32 attr, int channel)
@@ -35,6 +311,9 @@ hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	xe_device_mem_access_get(gt_to_xe(ddat->gt));
 
 	switch (type) {
+	case hwmon_power:
+		ret = hwmon_power_is_visible(ddat, attr, channel);
+		break;
 	default:
 		ret = 0;
 		break;
@@ -55,6 +334,9 @@ hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	xe_device_mem_access_get(gt_to_xe(ddat->gt));
 
 	switch (type) {
+	case hwmon_power:
+		ret = hwmon_power_read(ddat, attr, channel, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -75,6 +357,9 @@ hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	xe_device_mem_access_get(gt_to_xe(ddat->gt));
 
 	switch (type) {
+	case hwmon_power:
+		ret = hwmon_power_write(ddat, attr, channel, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -99,6 +384,18 @@ static const struct hwmon_chip_info hwmon_chip_info = {
 static void
 hwmon_get_preregistration_info(struct xe_device *xe)
 {
+	struct xe_hwmon *hwmon = xe->hwmon;
+	struct xe_hwmon_data *ddat = &hwmon->ddat;
+	u32 val_sku_unit = 0;
+	int ret;
+
+	ret = process_hwmon_reg(ddat, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
+	/*
+	 * The contents of register PKG_POWER_SKU_UNIT do not change,
+	 * so read it once and store the shift values.
+	 */
+	if (!ret)
+		hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
 }
 
 void xe_hwmon_register(struct xe_device *xe)
@@ -128,13 +425,16 @@ void xe_hwmon_register(struct xe_device *xe)
 
 	hwmon_get_preregistration_info(xe);
 
+	init_waitqueue_head(&hwmon->waitq);
+
 	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
 
-	/*  hwmon_dev points to device hwmon<i> */
+	/* hwmon_dev points to device hwmon<i> */
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
 							 ddat,
 							 &hwmon_chip_info,
 							 NULL);
+
 	if (IS_ERR(hwmon_dev)) {
 		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
 		xe->hwmon = NULL;
diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
index a078eeb0a68b..a5dc693569c5 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.h
+++ b/drivers/gpu/drm/xe/xe_hwmon.h
@@ -14,9 +14,13 @@ struct xe_device;
 #if IS_REACHABLE(CONFIG_HWMON)
 void xe_hwmon_register(struct xe_device *xe);
 void xe_hwmon_unregister(struct xe_device *xe);
+void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old);
+void xe_hwmon_power_max_restore(struct xe_device *xe, bool old);
 #else
 static inline void xe_hwmon_register(struct xe_device *xe) { };
 static inline void xe_hwmon_unregister(struct xe_device *xe) { };
+static inline void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old) { };
+static inline void xe_hwmon_power_max_restore(struct xe_device *xe, bool old) { };
 #endif
 
 #endif /* __XE_HWMON_H__ */
diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
index daf56c846d03..030296f8f863 100644
--- a/drivers/gpu/drm/xe/xe_macros.h
+++ b/drivers/gpu/drm/xe/xe_macros.h
@@ -15,4 +15,7 @@
 			    "Ioctl argument check failed at %s:%d: %s", \
 			    __FILE__, __LINE__, #cond), 1))
 
+#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
+			     __stringify(x), (long)(x))
+
 #endif
diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
index addd6f2681b9..2e9c915ac707 100644
--- a/drivers/gpu/drm/xe/xe_uc.c
+++ b/drivers/gpu/drm/xe/xe_uc.c
@@ -13,6 +13,7 @@
 #include "xe_huc.h"
 #include "xe_uc_fw.h"
 #include "xe_wopcm.h"
+#include "xe_hwmon.h"
 
 static struct xe_gt *
 uc_to_gt(struct xe_uc *uc)
@@ -127,11 +128,15 @@ int xe_uc_init_hwconfig(struct xe_uc *uc)
 int xe_uc_init_hw(struct xe_uc *uc)
 {
 	int ret;
+	bool pl1en;
 
 	/* GuC submission not enabled, nothing to do */
 	if (!xe_device_guc_submission_enabled(uc_to_xe(uc)))
 		return 0;
 
+	/* Disable a potentially low PL1 power limit to allow freq to be raised */
+	xe_hwmon_power_max_disable(uc_to_xe(uc), &pl1en);
+
 	ret = xe_uc_sanitize_reset(uc);
 	if (ret)
 		return ret;
@@ -160,6 +165,7 @@ int xe_uc_init_hw(struct xe_uc *uc)
 	if (ret)
 		return ret;
 
+	xe_hwmon_power_max_restore(uc_to_xe(uc), pl1en);
 	/* We don't fail the driver load if HuC fails to auth, but let's warn */
 	ret = xe_huc_auth(&uc->huc);
 	XE_WARN_ON(ret);
-- 
2.25.1

Re: [PATCH v3 2/6] drm/xe/hwmon: Expose power attributes

[Andi Shyti]

Hi Badal,

On Wed, Aug 02, 2023 at 07:22:37PM +0530, Badal Nilawar wrote:
> Expose power_max (pl1) and power_rated_max (tdp) attributes.

can you please write a few words more here to explain the
interface being exposed and what these powers are?

> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_MCHBAR_REGS_H__
> +#define _XE_MCHBAR_REGS_H_

there is an extra '_' in the ifndef

> +

[...]

>  #include <linux/hwmon.h>
>  
>  #include <drm/drm_managed.h>
> +#include "regs/xe_mchbar_regs.h"
>  #include "regs/xe_gt_regs.h"
>  #include "xe_device.h"
>  #include "xe_hwmon.h"
> +#include "xe_mmio.h"
> +#include "xe_gt.h"

can we keep these in alphabetical order?

> +enum hwmon_reg_name {
> +	REG_PKG_RAPL_LIMIT,
> +	REG_PKG_POWER_SKU,
> +	REG_PKG_POWER_SKU_UNIT,
> +};

Are these names or id's? With name I understand string/

> +enum hwmon_reg_operation {
> +	REG_READ,
> +	REG_WRITE,
> +	REG_RMW,
> +};

I'm not checking on the prefixes here... I let someone more
experienced than me comment if there anything wrong.

> +/*
> + * SF_* - scale factors for particular quantities according to hwmon spec.
> + * - power  - microwatts
> + */

this comment looks a bit off to me, what does
" - power  - microwatts" stand for?

> +#define SF_POWER	1000000
>  
>  struct xe_hwmon_data {
>  	struct device *hwmon_dev;
> @@ -18,13 +39,268 @@ struct xe_hwmon_data {
>  
>  struct xe_hwmon {
>  	struct xe_hwmon_data ddat;
> -	struct mutex hwmon_lock;
> +	struct mutex hwmon_lock; /* rmw operations*/

please put this change in the previous patch.

> +	bool reset_in_progress;
> +	wait_queue_head_t waitq;
> +	int scl_shift_power;
>  };
>  
> +#define ddat_to_xe_hwmon(ddat)	({ container_of(ddat, struct xe_hwmon, ddat); })

Any particular reason for the ({ ... }) ?

> +static int process_hwmon_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_name,
> +			     enum hwmon_reg_operation operation, u32 *value,
> +			     u32 clr, u32 set)
> +{
> +	struct xe_reg reg;
> +	int ret = 0;
> +
> +	reg.raw = hwmon_get_reg(ddat, reg_name);
> +
> +	if (!reg.raw)
> +		return -EOPNOTSUPP;
> +
> +	switch (operation) {
> +	case REG_READ:
> +		*value = xe_mmio_read32(ddat->gt, reg);
> +		break;
> +	case REG_WRITE:
> +		xe_mmio_write32(ddat->gt, reg, *value);
> +		break;
> +	case REG_RMW:
> +		*value = xe_mmio_rmw32(ddat->gt, reg, clr, set);
> +		break;
> +	default:
> +		XE_MISSING_CASE(operation);
> +		ret = -EOPNOTSUPP;

you could just return 0 or return -EOPNOTSUPP everywhere and save
"ret" and a return (maybe not needed).

Just a personal preference, feel free to ignro and do as you like
it.

> +		break;
> +	}
> +
> +	return ret;
> +}

[...]

> +static int hwmon_power_max_read(struct xe_hwmon_data *ddat, long *value)
> +{
> +	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
> +	u32 reg_val;
> +	u64 r, min, max;
> +
> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val, 0, 0);
> +	/* Check if PL1 limit is disabled */
> +	if (!(reg_val & PKG_PWR_LIM_1_EN)) {
> +		*value = PL1_DISABLE;
> +		return 0;
> +	}
> +
> +	reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
> +	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
> +
> +	process_hwmon_reg_read64(ddat, REG_PKG_POWER_SKU, &r);
> +	min = REG_FIELD_GET(PKG_MIN_PWR, r);
> +	min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
> +	max = REG_FIELD_GET(PKG_MAX_PWR, r);
> +	max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
> +
> +	if (min && max)
> +		*value = clamp_t(u64, *value, min, max);
> +
> +	return 0;

you are returning '0' in any case, can we make this void?

> +}
> +
> +static inline bool check_reset_in_progress(struct xe_hwmon *hwmon)
> +{
> +	mutex_lock(&hwmon->hwmon_lock);
> +	if (!hwmon->reset_in_progress)
> +		return true;
> +	mutex_unlock(&hwmon->hwmon_lock);
> +		return false;

This is a bit scary (apart from the indentation) and without a
strong explanation I can't let this go.

I'm pretty sure that we don't need this... can you explain?

> +}
> +
> +static int hwmon_power_max_write(struct xe_hwmon_data *ddat, long value)
> +{
> +	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
> +	DEFINE_WAIT(wait);
> +	int ret = 0;
> +	u32 nval;
> +
> +	/* hwmon->hwmon_lock remain held till rmw operation is over */
> +	wait_event(hwmon->waitq, check_reset_in_progress(hwmon));
> +
> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> +	if (value == PL1_DISABLE) {
> +		process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &nval,
> +				  PKG_PWR_LIM_1_EN, 0);
> +		process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_READ, &nval,
> +				  PKG_PWR_LIM_1_EN, 0);
> +
> +		if (nval & PKG_PWR_LIM_1_EN)
> +			ret = -ENODEV;
> +		goto unlock;
> +	}
> +
> +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> +	nval = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
> +	nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
> +
> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &nval,
> +			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
> +unlock:
> +	mutex_unlock(&hwmon->hwmon_lock);

Where is this lock taken? Are you relying on the fact that this
lock might not be taken? In any case it is not allowed to unlock
a without previously locking.

It's very error prone when you lock in a function and unlock in
another function and in the rare cases when this is done it has
to be written in the function name.

> +	return 0;
> +}
> +
> +static int hwmon_power_rated_max_read(struct xe_hwmon_data *ddat, long *value)
> +{
> +	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
> +	u32 reg_val;
> +
> +	process_hwmon_reg(ddat, REG_PKG_POWER_SKU, REG_READ, &reg_val, 0, 0);
> +	reg_val = REG_FIELD_GET(PKG_PKG_TDP, reg_val);
> +	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
> +
> +	return 0;

Can this function be void?

> +}

[...]

> +void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old)
> +{
> +	struct xe_hwmon *hwmon = xe->hwmon;
> +	struct xe_hwmon_data *ddat = &hwmon->ddat;
> +	u32 r;
> +
> +	if (!(hwmon && hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT)))
> +		return;
> +
> +	xe_device_assert_mem_access(gt_to_xe(ddat->gt));
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	hwmon->reset_in_progress = true;
> +
> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &r,
> +			  PKG_PWR_LIM_1_EN, 0);
> +	*old = !!(r & PKG_PWR_LIM_1_EN);

do we need to place under lock these last to lines?

> +	mutex_unlock(&hwmon->hwmon_lock);
> +}
> +
> +void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
> +{
> +	struct xe_hwmon *hwmon = xe->hwmon;
> +	struct xe_hwmon_data *ddat = &hwmon->ddat;
> +	u32 r;
> +
> +	if (!(hwmon && hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT)))
> +		return;
> +
> +	xe_device_assert_mem_access(gt_to_xe(ddat->gt));
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &r,
> +			  PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0);
> +
> +	hwmon->reset_in_progress = false;
> +	wake_up_all(&hwmon->waitq);

does the wake up need to be under lock?

Now... does it eve happen that "check_reset_in_progress()"
returns false and therefore unlocks the mutex?

> +
> +	mutex_unlock(&hwmon->hwmon_lock);
> +}

[...]

>  void xe_hwmon_register(struct xe_device *xe)
> @@ -128,13 +425,16 @@ void xe_hwmon_register(struct xe_device *xe)
>  
>  	hwmon_get_preregistration_info(xe);
>  
> +	init_waitqueue_head(&hwmon->waitq);
> +
>  	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>  
> -	/*  hwmon_dev points to device hwmon<i> */
> +	/* hwmon_dev points to device hwmon<i> */

Please this change needs to go in the previous patch.
What is <i>?

>  	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>  							 ddat,
>  							 &hwmon_chip_info,
>  							 NULL);
> +

This change in the previous patch.

>  	if (IS_ERR(hwmon_dev)) {
>  		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
>  		xe->hwmon = NULL;
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
> index a078eeb0a68b..a5dc693569c5 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.h
> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
> @@ -14,9 +14,13 @@ struct xe_device;
>  #if IS_REACHABLE(CONFIG_HWMON)
>  void xe_hwmon_register(struct xe_device *xe);
>  void xe_hwmon_unregister(struct xe_device *xe);
> +void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old);
> +void xe_hwmon_power_max_restore(struct xe_device *xe, bool old);
>  #else
>  static inline void xe_hwmon_register(struct xe_device *xe) { };
>  static inline void xe_hwmon_unregister(struct xe_device *xe) { };
> +static inline void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old) { };
> +static inline void xe_hwmon_power_max_restore(struct xe_device *xe, bool old) { };
>  #endif
>  
>  #endif /* __XE_HWMON_H__ */
> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
> index daf56c846d03..030296f8f863 100644
> --- a/drivers/gpu/drm/xe/xe_macros.h
> +++ b/drivers/gpu/drm/xe/xe_macros.h
> @@ -15,4 +15,7 @@
>  			    "Ioctl argument check failed at %s:%d: %s", \
>  			    __FILE__, __LINE__, #cond), 1))
>  
> +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> +			     __stringify(x), (long)(x))
> +

Should this have its own patch?

Andi

>  #endif

Re: [PATCH v3 2/6] drm/xe/hwmon: Expose power attributes

[Nilawar, Badal]

On 03-08-2023 04:53, Andi Shyti wrote:
> Hi Badal,
> 
> On Wed, Aug 02, 2023 at 07:22:37PM +0530, Badal Nilawar wrote:
>> Expose power_max (pl1) and power_rated_max (tdp) attributes.
> 
> can you please write a few words more here to explain the
> interface being exposed and what these powers are?
> 
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_MCHBAR_REGS_H__
>> +#define _XE_MCHBAR_REGS_H_
> 
> there is an extra '_' in the ifndef
Sure I will fix this.
> 
>> +
> 
> [...]
> 
>>   #include <linux/hwmon.h>
>>   
>>   #include <drm/drm_managed.h>
>> +#include "regs/xe_mchbar_regs.h"
>>   #include "regs/xe_gt_regs.h"
>>   #include "xe_device.h"
>>   #include "xe_hwmon.h"
>> +#include "xe_mmio.h"
>> +#include "xe_gt.h"
> 
> can we keep these in alphabetical order?
Sure
> 
>> +enum hwmon_reg_name {
>> +	REG_PKG_RAPL_LIMIT,
>> +	REG_PKG_POWER_SKU,
>> +	REG_PKG_POWER_SKU_UNIT,
>> +};
> 
> Are these names or id's? With name I understand string/Can't say ids. I will remove _name prefix to avoid confusion.
> 
>> +enum hwmon_reg_operation {
>> +	REG_READ,
>> +	REG_WRITE,
>> +	REG_RMW,
>> +};
> 
> I'm not checking on the prefixes here... I let someone more
> experienced than me comment if there anything wrong.
> 
>> +/*
>> + * SF_* - scale factors for particular quantities according to hwmon spec.
>> + * - power  - microwatts
>> + */
> 
> this comment looks a bit off to me, what does
> " - power  - microwatts" stand for?
unit of power is microwatts as per hwmon spec.
> 
>> +#define SF_POWER	1000000
>>   
>>   struct xe_hwmon_data {
>>   	struct device *hwmon_dev;
>> @@ -18,13 +39,268 @@ struct xe_hwmon_data {
>>   
>>   struct xe_hwmon {
>>   	struct xe_hwmon_data ddat;
>> -	struct mutex hwmon_lock;
>> +	struct mutex hwmon_lock; /* rmw operations*/
> 
> please put this change in the previous patch.
Sure
> 
>> +	bool reset_in_progress;
>> +	wait_queue_head_t waitq;
>> +	int scl_shift_power;
>>   };
>>   
>> +#define ddat_to_xe_hwmon(ddat)	({ container_of(ddat, struct xe_hwmon, ddat); })
> 
> Any particular reason for the ({ ... }) ?
> 
>> +static int process_hwmon_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_name,
>> +			     enum hwmon_reg_operation operation, u32 *value,
>> +			     u32 clr, u32 set)
>> +{
>> +	struct xe_reg reg;
>> +	int ret = 0;
>> +
>> +	reg.raw = hwmon_get_reg(ddat, reg_name);
>> +
>> +	if (!reg.raw)
>> +		return -EOPNOTSUPP;
>> +
>> +	switch (operation) {
>> +	case REG_READ:
>> +		*value = xe_mmio_read32(ddat->gt, reg);
>> +		break;
>> +	case REG_WRITE:
>> +		xe_mmio_write32(ddat->gt, reg, *value);
>> +		break;
>> +	case REG_RMW:
>> +		*value = xe_mmio_rmw32(ddat->gt, reg, clr, set);
>> +		break;
>> +	default:
>> +		XE_MISSING_CASE(operation);
>> +		ret = -EOPNOTSUPP;
> 
> you could just return 0 or return -EOPNOTSUPP everywhere and save
> "ret" and a return (maybe not needed).
> 
> Just a personal preference, feel free to ignro and do as you like
> it.
Sure I will fix this in next rev.
> 
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> [...]
> 
>> +static int hwmon_power_max_read(struct xe_hwmon_data *ddat, long *value)
>> +{
>> +	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
>> +	u32 reg_val;
>> +	u64 r, min, max;
>> +
>> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val, 0, 0);
>> +	/* Check if PL1 limit is disabled */
>> +	if (!(reg_val & PKG_PWR_LIM_1_EN)) {
>> +		*value = PL1_DISABLE;
>> +		return 0;
>> +	}
>> +
>> +	reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
>> +	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
>> +
>> +	process_hwmon_reg_read64(ddat, REG_PKG_POWER_SKU, &r);
>> +	min = REG_FIELD_GET(PKG_MIN_PWR, r);
>> +	min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
>> +	max = REG_FIELD_GET(PKG_MAX_PWR, r);
>> +	max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
>> +
>> +	if (min && max)
>> +		*value = clamp_t(u64, *value, min, max);
>> +
>> +	return 0;
> 
> you are returning '0' in any case, can we make this void?
Top layer function expects return so added return here.
> 
>> +}
>> +
>> +static inline bool check_reset_in_progress(struct xe_hwmon *hwmon)
>> +{
>> +	mutex_lock(&hwmon->hwmon_lock);
>> +	if (!hwmon->reset_in_progress)
>> +		return true;
>> +	mutex_unlock(&hwmon->hwmon_lock);
>> +		return false;
> 
> This is a bit scary (apart from the indentation) and without a
> strong explanation I can't let this go.
> 
> I'm pretty sure that we don't need this... can you explain?
In case of guc load not in progress (!reset_in_progress) mutex shouldn't 
be unlock, which will get unlocked once rmw operations are over.

Other way could be get mutex_lock after !reset_in_progress but that will 
add race.
	wait_event(hwmon->waitq, reset_in_progress);
	At this place there is posibility that reset_in_progress get 	set. So 
this becomes racy.
	mutex_lock(&hwmon->hwmon_lock);

Any better idea to implement this?
> 
>> +}
>> +
>> +static int hwmon_power_max_write(struct xe_hwmon_data *ddat, long value)
>> +{
>> +	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
>> +	DEFINE_WAIT(wait);
>> +	int ret = 0;
>> +	u32 nval;
>> +
>> +	/* hwmon->hwmon_lock remain held till rmw operation is over */
>> +	wait_event(hwmon->waitq, check_reset_in_progress(hwmon));
>> +
>> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
>> +	if (value == PL1_DISABLE) {
>> +		process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &nval,
>> +				  PKG_PWR_LIM_1_EN, 0);
>> +		process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_READ, &nval,
>> +				  PKG_PWR_LIM_1_EN, 0);
>> +
>> +		if (nval & PKG_PWR_LIM_1_EN)
>> +			ret = -ENODEV;
>> +		goto unlock;
>> +	}
>> +
>> +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
>> +	nval = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
>> +	nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
>> +
>> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &nval,
>> +			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
>> +unlock:
>> +	mutex_unlock(&hwmon->hwmon_lock);
> 
> Where is this lock taken? Are you relying on the fact that this
> lock might not be taken? In any case it is not allowed to unlock
> a without previously locking.
Lock is taken in check_reset_in_progress();
> 
> It's very error prone when you lock in a function and unlock in
> another function and in the rare cases when this is done it has
> to be written in the function name.
Sure I will add comment here.
> 
>> +	return 0;
>> +}
>> +
>> +static int hwmon_power_rated_max_read(struct xe_hwmon_data *ddat, long *value)
>> +{
>> +	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
>> +	u32 reg_val;
>> +
>> +	process_hwmon_reg(ddat, REG_PKG_POWER_SKU, REG_READ, &reg_val, 0, 0);
>> +	reg_val = REG_FIELD_GET(PKG_PKG_TDP, reg_val);
>> +	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
>> +
>> +	return 0;
> 
> Can this function be void?
Top level function expect return.
> 
>> +}
> 
> [...]
> 
>> +void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old)
>> +{
>> +	struct xe_hwmon *hwmon = xe->hwmon;
>> +	struct xe_hwmon_data *ddat = &hwmon->ddat;
>> +	u32 r;
>> +
>> +	if (!(hwmon && hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT)))
>> +		return;
>> +
>> +	xe_device_assert_mem_access(gt_to_xe(ddat->gt));
>> +
>> +	mutex_lock(&hwmon->hwmon_lock);
>> +
>> +	hwmon->reset_in_progress = true;
>> +
>> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &r,
>> +			  PKG_PWR_LIM_1_EN, 0);
>> +	*old = !!(r & PKG_PWR_LIM_1_EN);
> 
> do we need to place under lock these last to lines?
Yes, want to guard this rmw operation.
> 
>> +	mutex_unlock(&hwmon->hwmon_lock);
>> +}
>> +
>> +void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
>> +{
>> +	struct xe_hwmon *hwmon = xe->hwmon;
>> +	struct xe_hwmon_data *ddat = &hwmon->ddat;
>> +	u32 r;
>> +
>> +	if (!(hwmon && hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT)))
>> +		return;
>> +
>> +	xe_device_assert_mem_access(gt_to_xe(ddat->gt));
>> +
>> +	mutex_lock(&hwmon->hwmon_lock);
>> +
>> +	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, &r,
>> +			  PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0);
>> +
>> +	hwmon->reset_in_progress = false;
>> +	wake_up_all(&hwmon->waitq);
> 
> does the wake up need to be under lock?
wake up can be added after unlock.
> 
> Now... does it eve happen that "check_reset_in_progress()"
> returns false and therefore unlocks the mutex?
Didn't get this? check_reset_in_progress() will keep waiting for mutex 
till it is released by this function.
> 
>> +
>> +	mutex_unlock(&hwmon->hwmon_lock);
>> +}
> 
> [...]
> 
>>   void xe_hwmon_register(struct xe_device *xe)
>> @@ -128,13 +425,16 @@ void xe_hwmon_register(struct xe_device *xe)
>>   
>>   	hwmon_get_preregistration_info(xe);
>>   
>> +	init_waitqueue_head(&hwmon->waitq);
>> +
>>   	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>>   
>> -	/*  hwmon_dev points to device hwmon<i> */
>> +	/* hwmon_dev points to device hwmon<i> */
> 
> Please this change needs to go in the previous patch.
> What is <i>?
> 
>>   	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>>   							 ddat,
>>   							 &hwmon_chip_info,
>>   							 NULL);
>> +
> 
> This change in the previous patch.
> 
>>   	if (IS_ERR(hwmon_dev)) {
>>   		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
>>   		xe->hwmon = NULL;
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
>> index a078eeb0a68b..a5dc693569c5 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.h
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
>> @@ -14,9 +14,13 @@ struct xe_device;
>>   #if IS_REACHABLE(CONFIG_HWMON)
>>   void xe_hwmon_register(struct xe_device *xe);
>>   void xe_hwmon_unregister(struct xe_device *xe);
>> +void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old);
>> +void xe_hwmon_power_max_restore(struct xe_device *xe, bool old);
>>   #else
>>   static inline void xe_hwmon_register(struct xe_device *xe) { };
>>   static inline void xe_hwmon_unregister(struct xe_device *xe) { };
>> +static inline void xe_hwmon_power_max_disable(struct xe_device *xe, bool *old) { };
>> +static inline void xe_hwmon_power_max_restore(struct xe_device *xe, bool old) { };
>>   #endif
>>   
>>   #endif /* __XE_HWMON_H__ */
>> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
>> index daf56c846d03..030296f8f863 100644
>> --- a/drivers/gpu/drm/xe/xe_macros.h
>> +++ b/drivers/gpu/drm/xe/xe_macros.h
>> @@ -15,4 +15,7 @@
>>   			    "Ioctl argument check failed at %s:%d: %s", \
>>   			    __FILE__, __LINE__, #cond), 1))
>>   
>> +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
>> +			     __stringify(x), (long)(x))
>> +
> 
> Should this have its own patch?
Sure, I will create separate patch for this.
> 
> Andi
> 
>>   #endif

[PATCH v3 3/6] drm/xe/hwmon: Expose card reactive critical power

[Badal Nilawar]

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

v2:
  - Move PCODE_MBOX macro to pcode file (Riana)
v3:
  - %s/hwm_/hwmon_/ (Matt Brost)
  - s/IS_DG2/(gt_to_xe(gt)->info.platform == XE_DG2)

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

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index d48d98f903ed..ea60add73743 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:		August 2023
+KernelVersion:	6.4
+Contact:	intel-xe@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:		August 2023
+KernelVersion:	6.4
+Contact:	intel-xe@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 ce8dac2168f6..ceab142f6d42 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -12,6 +12,8 @@
 #include "xe_hwmon.h"
 #include "xe_mmio.h"
 #include "xe_gt.h"
+#include "xe_pcode.h"
+#include "xe_pcode_api.h"
 
 enum hwmon_reg_name {
 	REG_PKG_RAPL_LIMIT,
@@ -28,8 +30,10 @@ enum hwmon_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 xe_hwmon_data {
 	struct device *hwmon_dev;
@@ -216,18 +220,43 @@ static int hwmon_power_rated_max_read(struct xe_hwmon_data *ddat, long *value)
 }
 
 static const struct hwmon_channel_info *hwmon_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 hwmon_pcode_read_i1(struct xe_gt *gt, u32 *uval)
+{
+	/* Avoid Illegal Subcommand error */
+	if (gt_to_xe(gt)->info.platform == XE_DG2)
+		return -ENXIO;
+
+	return xe_pcode_read(gt, PCODE_MBOX(PCODE_POWER_SETUP,
+			     POWER_SETUP_SUBCOMMAND_READ_I1, 0),
+			     uval, 0);
+}
+
+static int hwmon_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
 hwmon_power_is_visible(struct xe_hwmon_data *ddat, u32 attr, int chan)
 {
+	u32 uval;
+
 	switch (attr) {
 	case hwmon_power_max:
 		return hwmon_get_reg(ddat, REG_PKG_RAPL_LIMIT) ? 0664 : 0;
 	case hwmon_power_rated_max:
 		return hwmon_get_reg(ddat, REG_PKG_POWER_SKU) ? 0444 : 0;
+	case hwmon_power_crit:
+		return (hwmon_pcode_read_i1(ddat->gt, &uval) ||
+			!(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
 	default:
 		return 0;
 	}
@@ -236,11 +265,23 @@ hwmon_power_is_visible(struct xe_hwmon_data *ddat, u32 attr, int chan)
 static int
 hwmon_power_read(struct xe_hwmon_data *ddat, u32 attr, int chan, long *val)
 {
+	int ret;
+	u32 uval;
+
 	switch (attr) {
 	case hwmon_power_max:
 		return hwmon_power_max_read(ddat, val);
 	case hwmon_power_rated_max:
 		return hwmon_power_rated_max_read(ddat, val);
+	case hwmon_power_crit:
+		ret = hwmon_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;
 	}
@@ -249,9 +290,14 @@ hwmon_power_read(struct xe_hwmon_data *ddat, u32 attr, int chan, long *val)
 static int
 hwmon_power_write(struct xe_hwmon_data *ddat, u32 attr, int chan, long val)
 {
+	u32 uval;
+
 	switch (attr) {
 	case hwmon_power_max:
 		return hwmon_power_max_write(ddat, val);
+	case hwmon_power_crit:
+		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
+		return hwmon_pcode_write_i1(ddat->gt, uval);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -301,6 +347,55 @@ void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
 	mutex_unlock(&hwmon->hwmon_lock);
 }
 
+static umode_t
+hwmon_curr_is_visible(const struct xe_hwmon_data *ddat, u32 attr)
+{
+	u32 uval;
+
+	switch (attr) {
+	case hwmon_curr_crit:
+		return (hwmon_pcode_read_i1(ddat->gt, &uval) ||
+			(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwmon_curr_read(struct xe_hwmon_data *ddat, u32 attr, long *val)
+{
+	int ret;
+	u32 uval;
+
+	switch (attr) {
+	case hwmon_curr_crit:
+		ret = hwmon_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
+hwmon_curr_write(struct xe_hwmon_data *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 hwmon_pcode_write_i1(ddat->gt, uval);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t
 hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 		 u32 attr, int channel)
@@ -314,6 +409,9 @@ hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	case hwmon_power:
 		ret = hwmon_power_is_visible(ddat, attr, channel);
 		break;
+	case hwmon_curr:
+		ret = hwmon_curr_is_visible(ddat, attr);
+		break;
 	default:
 		ret = 0;
 		break;
@@ -337,6 +435,9 @@ hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_power:
 		ret = hwmon_power_read(ddat, attr, channel, val);
 		break;
+	case hwmon_curr:
+		ret = hwmon_curr_read(ddat, attr, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -360,6 +461,9 @@ hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_power:
 		ret = hwmon_power_write(ddat, attr, channel, val);
 		break;
+	case hwmon_curr:
+		ret = hwmon_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;
-- 
2.25.1

Re: [PATCH v3 3/6] drm/xe/hwmon: Expose card reactive critical power

[Andi Shyti]

Hi Badal,

On Wed, Aug 02, 2023 at 07:22:38PM +0530, Badal Nilawar wrote:
> 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).
> 
> v2:
>   - Move PCODE_MBOX macro to pcode file (Riana)
> v3:
>   - %s/hwm_/hwmon_/ (Matt Brost)
>   - s/IS_DG2/(gt_to_xe(gt)->info.platform == XE_DG2)
> 
> 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(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> index d48d98f903ed..ea60add73743 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:		August 2023
> +KernelVersion:	6.4
> +Contact:	intel-xe@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:		August 2023
> +KernelVersion:	6.4
> +Contact:	intel-xe@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 ce8dac2168f6..ceab142f6d42 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -12,6 +12,8 @@
>  #include "xe_hwmon.h"
>  #include "xe_mmio.h"
>  #include "xe_gt.h"
> +#include "xe_pcode.h"
> +#include "xe_pcode_api.h"
>  
>  enum hwmon_reg_name {
>  	REG_PKG_RAPL_LIMIT,
> @@ -28,8 +30,10 @@ enum hwmon_reg_operation {
>  /*
>   * SF_* - scale factors for particular quantities according to hwmon spec.
>   * - power  - microwatts
> + * - curr   - milliamperes

Ah... it's a table...

>   */
>  #define SF_POWER	1000000
> +#define SF_CURR		1000

... you could add it on the side

#define SF_POWER	1000000 /* microwatt */
#define SF_CURR		   1000 /* milliamperes */

The rest looks good

Andi

Re: [PATCH v3 3/6] drm/xe/hwmon: Expose card reactive critical power

[Nilawar, Badal]

On 03-08-2023 04:58, Andi Shyti wrote:
> Hi Badal,
> 
> On Wed, Aug 02, 2023 at 07:22:38PM +0530, Badal Nilawar wrote:
>> 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).
>>
>> v2:
>>    - Move PCODE_MBOX macro to pcode file (Riana)
>> v3:
>>    - %s/hwm_/hwmon_/ (Matt Brost)
>>    - s/IS_DG2/(gt_to_xe(gt)->info.platform == XE_DG2)
>>
>> 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(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> index d48d98f903ed..ea60add73743 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:		August 2023
>> +KernelVersion:	6.4
>> +Contact:	intel-xe@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:		August 2023
>> +KernelVersion:	6.4
>> +Contact:	intel-xe@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 ce8dac2168f6..ceab142f6d42 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -12,6 +12,8 @@
>>   #include "xe_hwmon.h"
>>   #include "xe_mmio.h"
>>   #include "xe_gt.h"
>> +#include "xe_pcode.h"
>> +#include "xe_pcode_api.h"
>>   
>>   enum hwmon_reg_name {
>>   	REG_PKG_RAPL_LIMIT,
>> @@ -28,8 +30,10 @@ enum hwmon_reg_operation {
>>   /*
>>    * SF_* - scale factors for particular quantities according to hwmon spec.
>>    * - power  - microwatts
>> + * - curr   - milliamperes
> 
> Ah... it's a table...
> 
>>    */
>>   #define SF_POWER	1000000
>> +#define SF_CURR		1000
> 
> ... you could add it on the side
> 
> #define SF_POWER	1000000 /* microwatt */
> #define SF_CURR		   1000 /* milliamperes */
> 
> The rest looks good
Sure, will take this in next revision.

Regards,
Badal
> 
> Andi

[PATCH v3 4/6] drm/xe/hwmon: Expose input voltage attribute

[Badal Nilawar]

Use Xe HWMON subsystem to display the input voltage.

v2:
  - Rename hwm_get_vltg to hwm_get_voltage (Riana)
  - Use scale factor SF_VOLTAGE (Riana)
v3:
  - %s/hwm_/hwmon/
  - %s/gt_perf_status/REG_GT_PERF_STATUS/
  - Remove platform check from hwmon_get_voltage()

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  6 ++
 drivers/gpu/drm/xe/regs/xe_gt_regs.h          |  3 +
 drivers/gpu/drm/xe/xe_hwmon.c                 | 59 +++++++++++++++++++
 3 files changed, 68 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index ea60add73743..167bd9480602 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -44,5 +44,11 @@ Description:	RW. Card reactive critical (I1) power limit in milliamperes.
 		the operating frequency if the power averaged over a window
 		exceeds this limit.
 
+What:		/sys/devices/.../hwmon/hwmon<i>/in0_input
+Date:		August 2023
+KernelVersion:	6.4
+Contact:	intel-xe@lists.freedesktop.org
+Description:	RO. Current Voltage in millivolt.
+
 		Only supported for particular Intel xe graphics platforms.
 
diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
index eb7210afbd2c..cc452ec999fc 100644
--- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
@@ -364,6 +364,9 @@
 #define GT_GFX_RC6_LOCKED			XE_REG(0x138104)
 #define GT_GFX_RC6				XE_REG(0x138108)
 
+#define GT_PERF_STATUS				XE_REG(0x1381b4)
+#define   VOLTAGE_MASK				REG_GENMASK(10, 0)
+
 #define GT_INTR_DW(x)				XE_REG(0x190018 + ((x) * 4))
 
 #define GUC_SG_INTR_ENABLE			XE_REG(0x190038)
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index ceab142f6d42..3e69cd79c1e2 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -3,7 +3,9 @@
  * Copyright © 2023 Intel Corporation
  */
 
+#include <linux/hwmon-sysfs.h>
 #include <linux/hwmon.h>
+#include <linux/types.h>
 
 #include <drm/drm_managed.h>
 #include "regs/xe_mchbar_regs.h"
@@ -19,6 +21,7 @@ enum hwmon_reg_name {
 	REG_PKG_RAPL_LIMIT,
 	REG_PKG_POWER_SKU,
 	REG_PKG_POWER_SKU_UNIT,
+	REG_GT_PERF_STATUS,
 };
 
 enum hwmon_reg_operation {
@@ -31,9 +34,11 @@ enum hwmon_reg_operation {
  * SF_* - scale factors for particular quantities according to hwmon spec.
  * - power  - microwatts
  * - curr   - milliamperes
+ * - voltage  - millivolts
  */
 #define SF_POWER	1000000
 #define SF_CURR		1000
+#define SF_VOLTAGE	1000
 
 struct xe_hwmon_data {
 	struct device *hwmon_dev;
@@ -75,6 +80,10 @@ static u32 hwmon_get_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_nam
 		else if (xe->info.platform == XE_PVC)
 			reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
 		break;
+	case REG_GT_PERF_STATUS:
+		if (xe->info.platform == XE_DG2)
+			reg = GT_PERF_STATUS;
+		break;
 	default:
 		XE_MISSING_CASE(reg_name);
 		break;
@@ -222,6 +231,7 @@ static int hwmon_power_rated_max_read(struct xe_hwmon_data *ddat, long *value)
 static const struct hwmon_channel_info *hwmon_info[] = {
 	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
 	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
+	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
 	NULL
 };
 
@@ -244,6 +254,18 @@ static int hwmon_pcode_write_i1(struct xe_gt *gt, u32 uval)
 			      uval);
 }
 
+static int hwmon_get_voltage(struct xe_hwmon_data *ddat, long *value)
+{
+	u32 reg_val;
+
+	process_hwmon_reg(ddat, REG_GT_PERF_STATUS,
+			  REG_READ, &reg_val, 0, 0);
+	/* HW register value in units of 2.5 millivolt */
+	*value = DIV_ROUND_CLOSEST(REG_FIELD_GET(VOLTAGE_MASK, reg_val) * 2500, SF_VOLTAGE);
+
+	return 0;
+}
+
 static umode_t
 hwmon_power_is_visible(struct xe_hwmon_data *ddat, u32 attr, int chan)
 {
@@ -396,6 +418,37 @@ hwmon_curr_write(struct xe_hwmon_data *ddat, u32 attr, long val)
 	}
 }
 
+static umode_t
+hwmon_in_is_visible(struct xe_hwmon_data *ddat, u32 attr)
+{
+	switch (attr) {
+	case hwmon_in_input:
+		return hwmon_get_reg(ddat, REG_GT_PERF_STATUS) ? 0444 : 0;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwmon_in_read(struct xe_hwmon_data *ddat, u32 attr, long *val)
+{
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	switch (attr) {
+	case hwmon_in_input:
+		ret = hwmon_get_voltage(ddat, val);
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
+}
+
 static umode_t
 hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 		 u32 attr, int channel)
@@ -412,6 +465,9 @@ hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	case hwmon_curr:
 		ret = hwmon_curr_is_visible(ddat, attr);
 		break;
+	case hwmon_in:
+		ret = hwmon_in_is_visible(ddat, attr);
+		break;
 	default:
 		ret = 0;
 		break;
@@ -438,6 +494,9 @@ hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_curr:
 		ret = hwmon_curr_read(ddat, attr, val);
 		break;
+	case hwmon_in:
+		ret = hwmon_in_read(ddat, attr, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
-- 
2.25.1

Re: [PATCH v3 4/6] drm/xe/hwmon: Expose input voltage attribute

[Andi Shyti]

Hi Badal,

[...]

>  	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
>  	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
> +	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
>  	NULL
>  };
>  
> @@ -244,6 +254,18 @@ static int hwmon_pcode_write_i1(struct xe_gt *gt, u32 uval)
>  			      uval);
>  }
>  
> +static int hwmon_get_voltage(struct xe_hwmon_data *ddat, long *value)

one thing I forgot to mention also in previous patches is that
hwmon_* and HWMON_* as prefixes do not belong to the xe driver.

You should use an xe related prefix, like xe_hwmon.

Rest looks good,
Andi

Re: [PATCH v3 4/6] drm/xe/hwmon: Expose input voltage attribute

[Nilawar, Badal]

On 03-08-2023 05:02, Andi Shyti wrote:
> Hi Badal,
> 
> [...]
> 
>>   	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
>>   	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
>> +	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
>>   	NULL
>>   };
>>   
>> @@ -244,6 +254,18 @@ static int hwmon_pcode_write_i1(struct xe_gt *gt, u32 uval)
>>   			      uval);
>>   }
>>   
>> +static int hwmon_get_voltage(struct xe_hwmon_data *ddat, long *value)
> 
> one thing I forgot to mention also in previous patches is that
> hwmon_* and HWMON_* as prefixes do not belong to the xe driver.
Yes, that's why in previous series we were using hwm_* prefix for static 
functions. I think I will use xe_hwmon* for static as well as global.

Regards,
Badal
> 
> You should use an xe related prefix, like xe_hwmon.
> 
> Rest looks good,
> Andi

[PATCH v3 5/6] drm/xe/hwmon: Expose hwmon energy attribute

[Badal Nilawar]

Expose hwmon energy attribute to show device level and gt
level energy usage

v2:
  - %s/hwm_/hwmon_/
  - %s/tile_/gt_
  - Convert enums to upper case
  - Print error info for hwmon_gt devices

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  12 +
 drivers/gpu/drm/xe/regs/xe_gt_regs.h          |   2 +
 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h      |   3 +
 drivers/gpu/drm/xe/xe_hwmon.c                 | 216 +++++++++++++++++-
 4 files changed, 229 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index 167bd9480602..4b2d6e1d0c7f 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -52,3 +52,15 @@ Description:	RO. Current Voltage in millivolt.
 
 		Only supported for particular Intel xe graphics platforms.
 
+What:		/sys/devices/.../hwmon/hwmon<i>/energy1_input
+Date:		August 2023
+KernelVersion:	6.4
+Contact:	intel-xe@lists.freedesktop.org
+Description:	RO. Energy input of device or gt in microjoules.
+
+		For xe device level hwmon devices (name "xe") this
+		reflects energy input for the entire device. For gt level
+		hwmon devices (name "xe_gtN") this reflects energy input
+		for the gt.
+
+		Only supported for particular Intel xe graphics platforms.
diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
index cc452ec999fc..8819b934a592 100644
--- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
@@ -400,8 +400,10 @@
 #define XEHPC_BCS5_BCS6_INTR_MASK		XE_REG(0x190118)
 #define XEHPC_BCS7_BCS8_INTR_MASK		XE_REG(0x19011c)
 
+#define PVC_GT0_PACKAGE_ENERGY_STATUS		XE_REG(0x281004)
 #define PVC_GT0_PACKAGE_RAPL_LIMIT		XE_REG(0x281008)
 #define PVC_GT0_PACKAGE_POWER_SKU_UNIT		XE_REG(0x281068)
+#define PVC_GT0_PLATFORM_ENERGY_STATUS		XE_REG(0x28106c)
 #define PVC_GT0_PACKAGE_POWER_SKU		XE_REG(0x281080)
 
 #endif
diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
index cb2d49b5c8a9..473a44bd7c56 100644
--- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
@@ -25,6 +25,9 @@
 
 #define PCU_CR_PACKAGE_POWER_SKU_UNIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
 #define   PKG_PWR_UNIT				REG_GENMASK(3, 0)
+#define   PKG_ENERGY_UNIT			REG_GENMASK(12, 8)
+
+#define PCU_CR_PACKAGE_ENERGY_STATUS		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
 
 #define PCU_CR_PACKAGE_RAPL_LIMIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
 #define   PKG_PWR_LIM_1				REG_GENMASK(14, 0)
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index 3e69cd79c1e2..a337edcebae5 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -22,6 +22,8 @@ enum hwmon_reg_name {
 	REG_PKG_POWER_SKU,
 	REG_PKG_POWER_SKU_UNIT,
 	REG_GT_PERF_STATUS,
+	REG_ENERGY_STATUS_ALL,
+	REG_ENERGY_STATUS_GT,
 };
 
 enum hwmon_reg_operation {
@@ -30,31 +32,50 @@ enum hwmon_reg_operation {
 	REG_RMW,
 };
 
+enum xe_hwmon_device_type {
+	HWMON_GT,
+	HWMON_DEVICE,
+};
+
 /*
  * SF_* - scale factors for particular quantities according to hwmon spec.
  * - power  - microwatts
  * - curr   - milliamperes
  * - voltage  - millivolts
+ * - energy - microjoules
  */
 #define SF_POWER	1000000
 #define SF_CURR		1000
 #define SF_VOLTAGE	1000
+#define SF_ENERGY	1000000
+
+struct hwmon_energy_info {
+	u32 reg_val_prev;
+	long accum_energy;		/* Accumulated energy for energy1_input */
+};
 
 struct xe_hwmon_data {
 	struct device *hwmon_dev;
 	struct xe_gt *gt;
 	char name[12];
+	struct hwmon_energy_info ei;	/*  Energy info for energy1_input */
+	enum xe_hwmon_device_type type;
 };
 
 struct xe_hwmon {
 	struct xe_hwmon_data ddat;
+	struct xe_hwmon_data ddat_gt[XE_MAX_TILES_PER_DEVICE];
 	struct mutex hwmon_lock; /* rmw operations*/
 	bool reset_in_progress;
 	wait_queue_head_t waitq;
 	int scl_shift_power;
+	int scl_shift_energy;
 };
 
-#define ddat_to_xe_hwmon(ddat)	({ container_of(ddat, struct xe_hwmon, ddat); })
+#define ddat_to_xe_hwmon(ddat)	\
+	({ ddat->type == HWMON_GT ?	\
+		container_of(ddat, struct xe_hwmon, ddat_gt[ddat->gt->info.id]) :	\
+		container_of(ddat, struct xe_hwmon, ddat); })
 
 static u32 hwmon_get_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_name)
 {
@@ -84,6 +105,16 @@ static u32 hwmon_get_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_nam
 		if (xe->info.platform == XE_DG2)
 			reg = GT_PERF_STATUS;
 		break;
+	case REG_ENERGY_STATUS_ALL:
+		if (xe->info.platform == XE_DG2)
+			reg = PCU_CR_PACKAGE_ENERGY_STATUS;
+		else if (xe->info.platform == XE_PVC)
+			reg = PVC_GT0_PLATFORM_ENERGY_STATUS;
+		break;
+	case REG_ENERGY_STATUS_GT:
+		if (xe->info.platform == XE_PVC)
+			reg = PVC_GT0_PACKAGE_ENERGY_STATUS;
+		break;
 	default:
 		XE_MISSING_CASE(reg_name);
 		break;
@@ -228,10 +259,69 @@ static int hwmon_power_rated_max_read(struct xe_hwmon_data *ddat, long *value)
 	return 0;
 }
 
+/*
+ * hwmon_energy_get - Obtain energy value
+ *
+ * The underlying energy hardware register is 32-bits and is subject to
+ * overflow. How long before overflow? For example, with an example
+ * scaling bit shift of 14 bits (see register *PACKAGE_POWER_SKU_UNIT) and
+ * a power draw of 1000 watts, the 32-bit counter will overflow in
+ * approximately 4.36 minutes.
+ *
+ * Examples:
+ *    1 watt:  (2^32 >> 14) /    1 W / (60 * 60 * 24) secs/day -> 3 days
+ * 1000 watts: (2^32 >> 14) / 1000 W / 60             secs/min -> 4.36 minutes
+ *
+ * The function significantly increases overflow duration (from 4.36
+ * minutes) by accumulating the energy register into a 'long' as allowed by
+ * the hwmon API. Using x86_64 128 bit arithmetic (see mul_u64_u32_shr()),
+ * a 'long' of 63 bits, SF_ENERGY of 1e6 (~20 bits) and
+ * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits before
+ * energy1_input overflows. This at 1000 W is an overflow duration of 278 years.
+ */
+static void
+hwmon_energy_get(struct xe_hwmon_data *ddat, long *energy)
+{
+	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
+	struct hwmon_energy_info *ei = &ddat->ei;
+	u32 reg_val;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	mutex_lock(&hwmon->hwmon_lock);
+
+	if (ddat->type == HWMON_GT)
+		process_hwmon_reg(ddat, REG_ENERGY_STATUS_GT, REG_READ,
+				  &reg_val, 0, 0);
+	else
+		process_hwmon_reg(ddat, REG_ENERGY_STATUS_ALL, REG_READ,
+				  &reg_val, 0, 0);
+
+	if (reg_val >= ei->reg_val_prev)
+		ei->accum_energy += reg_val - ei->reg_val_prev;
+	else
+		ei->accum_energy += UINT_MAX - ei->reg_val_prev + reg_val;
+
+	ei->reg_val_prev = reg_val;
+
+	*energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
+				  hwmon->scl_shift_energy);
+
+	mutex_unlock(&hwmon->hwmon_lock);
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+}
+
 static const struct hwmon_channel_info *hwmon_info[] = {
 	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
 	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
 	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
+	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
+	NULL
+};
+
+static const struct hwmon_channel_info *hwmon_gt_info[] = {
+	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
 	NULL
 };
 
@@ -449,6 +539,32 @@ hwmon_in_read(struct xe_hwmon_data *ddat, u32 attr, long *val)
 	return ret;
 }
 
+static umode_t
+hwmon_energy_is_visible(struct xe_hwmon_data *ddat, u32 attr)
+{
+	switch (attr) {
+	case hwmon_energy_input:
+		if (ddat->type == HWMON_GT)
+			return hwmon_get_reg(ddat, REG_ENERGY_STATUS_GT) ? 0444 : 0;
+		else
+			return hwmon_get_reg(ddat, REG_ENERGY_STATUS_ALL) ? 0444 : 0;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwmon_energy_read(struct xe_hwmon_data *ddat, u32 attr, long *val)
+{
+	switch (attr) {
+	case hwmon_energy_input:
+		hwmon_energy_get(ddat, val);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t
 hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 		 u32 attr, int channel)
@@ -468,6 +584,9 @@ hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	case hwmon_in:
 		ret = hwmon_in_is_visible(ddat, attr);
 		break;
+	case hwmon_energy:
+		ret = hwmon_energy_is_visible(ddat, attr);
+		break;
 	default:
 		ret = 0;
 		break;
@@ -497,6 +616,9 @@ hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_in:
 		ret = hwmon_in_read(ddat, attr, val);
 		break;
+	case hwmon_energy:
+		ret = hwmon_energy_read(ddat, attr, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -544,12 +666,53 @@ static const struct hwmon_chip_info hwmon_chip_info = {
 	.info = hwmon_info,
 };
 
+static umode_t
+hwmon_gt_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+		    u32 attr, int channel)
+{
+	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
+
+	switch (type) {
+	case hwmon_energy:
+		return hwmon_energy_is_visible(ddat, attr);
+	default:
+		return 0;
+	}
+}
+
+static int
+hwmon_gt_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	      int channel, long *val)
+{
+	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_energy:
+		return hwmon_energy_read(ddat, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops hwmon_gt_ops = {
+	.is_visible = hwmon_gt_is_visible,
+	.read = hwmon_gt_read,
+};
+
+static const struct hwmon_chip_info hwmon_gt_chip_info = {
+	.ops = &hwmon_gt_ops,
+	.info = hwmon_gt_info,
+};
+
 static void
 hwmon_get_preregistration_info(struct xe_device *xe)
 {
 	struct xe_hwmon *hwmon = xe->hwmon;
 	struct xe_hwmon_data *ddat = &hwmon->ddat;
+	struct xe_gt *gt;
+	long energy;
 	u32 val_sku_unit = 0;
+	u8 id;
 	int ret;
 
 	ret = process_hwmon_reg(ddat, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
@@ -557,8 +720,22 @@ hwmon_get_preregistration_info(struct xe_device *xe)
 	 * The contents of register PKG_POWER_SKU_UNIT do not change,
 	 * so read it once and store the shift values.
 	 */
-	if (!ret)
+	if (!ret) {
 		hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
+		hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
+	}
+
+	/*
+	 * Initialize 'struct hwmon_energy_info', i.e. set fields to the
+	 * first value of the energy register read
+	 */
+	if (hwmon_is_visible(ddat, hwmon_energy, hwmon_energy_input, 0))
+		hwmon_energy_get(ddat, &energy);
+
+	for_each_gt(gt, xe, id)
+		if (hwmon_gt_is_visible(&hwmon->ddat_gt[id], hwmon_energy,
+					hwmon_energy_input, 0))
+			hwmon_energy_get(&hwmon->ddat_gt[id], &energy);
 }
 
 void xe_hwmon_register(struct xe_device *xe)
@@ -567,6 +744,9 @@ void xe_hwmon_register(struct xe_device *xe)
 	struct xe_hwmon *hwmon;
 	struct device *hwmon_dev;
 	struct xe_hwmon_data *ddat;
+	struct xe_hwmon_data *ddat_gt;
+	struct xe_gt *gt;
+	u8 id;
 
 	/* hwmon is available only for dGfx */
 	if (!IS_DGFX(xe))
@@ -583,13 +763,21 @@ void xe_hwmon_register(struct xe_device *xe)
 
 	/* primary GT to access device level properties */
 	ddat->gt = xe->tiles[0].primary_gt;
+	ddat->type = HWMON_DEVICE;
 
 	snprintf(ddat->name, sizeof(ddat->name), "xe");
 
-	hwmon_get_preregistration_info(xe);
-
 	init_waitqueue_head(&hwmon->waitq);
 
+	for_each_gt(gt, xe, id) {
+		ddat_gt = hwmon->ddat_gt + id;
+		ddat_gt->gt = gt;
+		snprintf(ddat_gt->name, sizeof(ddat_gt->name), "xe_gt%u", id);
+		ddat_gt->type = HWMON_GT;
+	}
+
+	hwmon_get_preregistration_info(xe);
+
 	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
 
 	/* hwmon_dev points to device hwmon<i> */
@@ -605,6 +793,26 @@ void xe_hwmon_register(struct xe_device *xe)
 	}
 
 	ddat->hwmon_dev = hwmon_dev;
+
+	for_each_gt(gt, xe, id) {
+		ddat_gt = hwmon->ddat_gt + id;
+		/*
+		 * Create per-gt directories only if a per-gt attribute is
+		 * visible. Currently this is only energy
+		 */
+		if (!hwmon_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
+			continue;
+
+		hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
+								 ddat_gt,
+								 &hwmon_gt_chip_info,
+								 NULL);
+		if (IS_ERR(hwmon_dev))
+			drm_warn(&xe->drm, "Fail to register xe_gt %d hwmon, Err:%ld\n",
+				 id, PTR_ERR(hwmon_dev));
+		else
+			ddat_gt->hwmon_dev = hwmon_dev;
+	}
 }
 
 void xe_hwmon_unregister(struct xe_device *xe)
-- 
2.25.1

Re: [PATCH v3 5/6] drm/xe/hwmon: Expose hwmon energy attribute

[Guenter Roeck]

On 8/2/23 06:52, Badal Nilawar wrote:
> Expose hwmon energy attribute to show device level and gt
> level energy usage
> 
> v2:
>    - %s/hwm_/hwmon_/
>    - %s/tile_/gt_
>    - Convert enums to upper case
>    - Print error info for hwmon_gt devices
> 
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  12 +
>   drivers/gpu/drm/xe/regs/xe_gt_regs.h          |   2 +
>   drivers/gpu/drm/xe/regs/xe_mchbar_regs.h      |   3 +
>   drivers/gpu/drm/xe/xe_hwmon.c                 | 216 +++++++++++++++++-
>   4 files changed, 229 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> index 167bd9480602..4b2d6e1d0c7f 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> @@ -52,3 +52,15 @@ Description:	RO. Current Voltage in millivolt.
>   
>   		Only supported for particular Intel xe graphics platforms.
>   
> +What:		/sys/devices/.../hwmon/hwmon<i>/energy1_input
> +Date:		August 2023
> +KernelVersion:	6.4
> +Contact:	intel-xe@lists.freedesktop.org
> +Description:	RO. Energy input of device or gt in microjoules.
> +
> +		For xe device level hwmon devices (name "xe") this
> +		reflects energy input for the entire device. For gt level
> +		hwmon devices (name "xe_gtN") this reflects energy input
> +		for the gt.
> +
> +		Only supported for particular Intel xe graphics platforms.
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index cc452ec999fc..8819b934a592 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -400,8 +400,10 @@
>   #define XEHPC_BCS5_BCS6_INTR_MASK		XE_REG(0x190118)
>   #define XEHPC_BCS7_BCS8_INTR_MASK		XE_REG(0x19011c)
>   
> +#define PVC_GT0_PACKAGE_ENERGY_STATUS		XE_REG(0x281004)
>   #define PVC_GT0_PACKAGE_RAPL_LIMIT		XE_REG(0x281008)
>   #define PVC_GT0_PACKAGE_POWER_SKU_UNIT		XE_REG(0x281068)
> +#define PVC_GT0_PLATFORM_ENERGY_STATUS		XE_REG(0x28106c)
>   #define PVC_GT0_PACKAGE_POWER_SKU		XE_REG(0x281080)
>   
>   #endif
> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> index cb2d49b5c8a9..473a44bd7c56 100644
> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> @@ -25,6 +25,9 @@
>   
>   #define PCU_CR_PACKAGE_POWER_SKU_UNIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
>   #define   PKG_PWR_UNIT				REG_GENMASK(3, 0)
> +#define   PKG_ENERGY_UNIT			REG_GENMASK(12, 8)
> +
> +#define PCU_CR_PACKAGE_ENERGY_STATUS		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
>   
>   #define PCU_CR_PACKAGE_RAPL_LIMIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
>   #define   PKG_PWR_LIM_1				REG_GENMASK(14, 0)
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 3e69cd79c1e2..a337edcebae5 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -22,6 +22,8 @@ enum hwmon_reg_name {
>   	REG_PKG_POWER_SKU,
>   	REG_PKG_POWER_SKU_UNIT,
>   	REG_GT_PERF_STATUS,
> +	REG_ENERGY_STATUS_ALL,
> +	REG_ENERGY_STATUS_GT,
>   };
>   
>   enum hwmon_reg_operation {
> @@ -30,31 +32,50 @@ enum hwmon_reg_operation {
>   	REG_RMW,
>   };
>   
> +enum xe_hwmon_device_type {
> +	HWMON_GT,
> +	HWMON_DEVICE,
> +};
> +
>   /*
>    * SF_* - scale factors for particular quantities according to hwmon spec.
>    * - power  - microwatts
>    * - curr   - milliamperes
>    * - voltage  - millivolts
> + * - energy - microjoules
>    */
>   #define SF_POWER	1000000
>   #define SF_CURR		1000
>   #define SF_VOLTAGE	1000
> +#define SF_ENERGY	1000000
> +
> +struct hwmon_energy_info {
> +	u32 reg_val_prev;
> +	long accum_energy;		/* Accumulated energy for energy1_input */
> +};
>   
>   struct xe_hwmon_data {
>   	struct device *hwmon_dev;
>   	struct xe_gt *gt;
>   	char name[12];
> +	struct hwmon_energy_info ei;	/*  Energy info for energy1_input */
> +	enum xe_hwmon_device_type type;
>   };
>   
>   struct xe_hwmon {
>   	struct xe_hwmon_data ddat;
> +	struct xe_hwmon_data ddat_gt[XE_MAX_TILES_PER_DEVICE];
>   	struct mutex hwmon_lock; /* rmw operations*/
>   	bool reset_in_progress;
>   	wait_queue_head_t waitq;
>   	int scl_shift_power;
> +	int scl_shift_energy;
>   };
>   
> -#define ddat_to_xe_hwmon(ddat)	({ container_of(ddat, struct xe_hwmon, ddat); })
> +#define ddat_to_xe_hwmon(ddat)	\
> +	({ ddat->type == HWMON_GT ?	\
> +		container_of(ddat, struct xe_hwmon, ddat_gt[ddat->gt->info.id]) :	\
> +		container_of(ddat, struct xe_hwmon, ddat); })
>   
>   static u32 hwmon_get_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_name)
>   {
> @@ -84,6 +105,16 @@ static u32 hwmon_get_reg(struct xe_hwmon_data *ddat, enum hwmon_reg_name reg_nam
>   		if (xe->info.platform == XE_DG2)
>   			reg = GT_PERF_STATUS;
>   		break;
> +	case REG_ENERGY_STATUS_ALL:
> +		if (xe->info.platform == XE_DG2)
> +			reg = PCU_CR_PACKAGE_ENERGY_STATUS;
> +		else if (xe->info.platform == XE_PVC)
> +			reg = PVC_GT0_PLATFORM_ENERGY_STATUS;
> +		break;
> +	case REG_ENERGY_STATUS_GT:
> +		if (xe->info.platform == XE_PVC)
> +			reg = PVC_GT0_PACKAGE_ENERGY_STATUS;
> +		break;
>   	default:
>   		XE_MISSING_CASE(reg_name);
>   		break;
> @@ -228,10 +259,69 @@ static int hwmon_power_rated_max_read(struct xe_hwmon_data *ddat, long *value)
>   	return 0;
>   }
>   
> +/*
> + * hwmon_energy_get - Obtain energy value
> + *
> + * The underlying energy hardware register is 32-bits and is subject to
> + * overflow. How long before overflow? For example, with an example
> + * scaling bit shift of 14 bits (see register *PACKAGE_POWER_SKU_UNIT) and
> + * a power draw of 1000 watts, the 32-bit counter will overflow in
> + * approximately 4.36 minutes.
> + *
> + * Examples:
> + *    1 watt:  (2^32 >> 14) /    1 W / (60 * 60 * 24) secs/day -> 3 days
> + * 1000 watts: (2^32 >> 14) / 1000 W / 60             secs/min -> 4.36 minutes
> + *
> + * The function significantly increases overflow duration (from 4.36
> + * minutes) by accumulating the energy register into a 'long' as allowed by
> + * the hwmon API. Using x86_64 128 bit arithmetic (see mul_u64_u32_shr()),
> + * a 'long' of 63 bits, SF_ENERGY of 1e6 (~20 bits) and
> + * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits before
> + * energy1_input overflows. This at 1000 W is an overflow duration of 278 years.
> + */
> +static void
> +hwmon_energy_get(struct xe_hwmon_data *ddat, long *energy)
> +{
> +	struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
> +	struct hwmon_energy_info *ei = &ddat->ei;
> +	u32 reg_val;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	if (ddat->type == HWMON_GT)
> +		process_hwmon_reg(ddat, REG_ENERGY_STATUS_GT, REG_READ,
> +				  &reg_val, 0, 0);
> +	else
> +		process_hwmon_reg(ddat, REG_ENERGY_STATUS_ALL, REG_READ,
> +				  &reg_val, 0, 0);
> +
> +	if (reg_val >= ei->reg_val_prev)
> +		ei->accum_energy += reg_val - ei->reg_val_prev;
> +	else
> +		ei->accum_energy += UINT_MAX - ei->reg_val_prev + reg_val;
> +
> +	ei->reg_val_prev = reg_val;
> +
> +	*energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
> +				  hwmon->scl_shift_energy);
> +
> +	mutex_unlock(&hwmon->hwmon_lock);
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +}
> +
>   static const struct hwmon_channel_info *hwmon_info[] = {
>   	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
>   	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
>   	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> +	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_channel_info *hwmon_gt_info[] = {
> +	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
>   	NULL
>   };
>   
> @@ -449,6 +539,32 @@ hwmon_in_read(struct xe_hwmon_data *ddat, u32 attr, long *val)
>   	return ret;
>   }
>   
> +static umode_t
> +hwmon_energy_is_visible(struct xe_hwmon_data *ddat, u32 attr)
> +{
> +	switch (attr) {
> +	case hwmon_energy_input:
> +		if (ddat->type == HWMON_GT)
> +			return hwmon_get_reg(ddat, REG_ENERGY_STATUS_GT) ? 0444 : 0;
> +		else
> +			return hwmon_get_reg(ddat, REG_ENERGY_STATUS_ALL) ? 0444 : 0;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int
> +hwmon_energy_read(struct xe_hwmon_data *ddat, u32 attr, long *val)
> +{
> +	switch (attr) {
> +	case hwmon_energy_input:
> +		hwmon_energy_get(ddat, val);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>   static umode_t
>   hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>   		 u32 attr, int channel)
> @@ -468,6 +584,9 @@ hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>   	case hwmon_in:
>   		ret = hwmon_in_is_visible(ddat, attr);
>   		break;
> +	case hwmon_energy:
> +		ret = hwmon_energy_is_visible(ddat, attr);
> +		break;
>   	default:
>   		ret = 0;
>   		break;
> @@ -497,6 +616,9 @@ hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>   	case hwmon_in:
>   		ret = hwmon_in_read(ddat, attr, val);
>   		break;
> +	case hwmon_energy:
> +		ret = hwmon_energy_read(ddat, attr, val);
> +		break;
>   	default:
>   		ret = -EOPNOTSUPP;
>   		break;
> @@ -544,12 +666,53 @@ static const struct hwmon_chip_info hwmon_chip_info = {
>   	.info = hwmon_info,
>   };
>   
> +static umode_t
> +hwmon_gt_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> +		    u32 attr, int channel)
> +{
> +	struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
> +
> +	switch (type) {
> +	case hwmon_energy:
> +		return hwmon_energy_is_visible(ddat, attr);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int
> +hwmon_gt_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +	      int channel, long *val)
> +{
> +	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_energy:
> +		return hwmon_energy_read(ddat, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_ops hwmon_gt_ops = {
> +	.is_visible = hwmon_gt_is_visible,
> +	.read = hwmon_gt_read,
> +};
> +
> +static const struct hwmon_chip_info hwmon_gt_chip_info = {
> +	.ops = &hwmon_gt_ops,
> +	.info = hwmon_gt_info,
> +};
> +
>   static void
>   hwmon_get_preregistration_info(struct xe_device *xe)
>   {
>   	struct xe_hwmon *hwmon = xe->hwmon;
>   	struct xe_hwmon_data *ddat = &hwmon->ddat;
> +	struct xe_gt *gt;
> +	long energy;
>   	u32 val_sku_unit = 0;
> +	u8 id;
>   	int ret;
>   
>   	ret = process_hwmon_reg(ddat, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
> @@ -557,8 +720,22 @@ hwmon_get_preregistration_info(struct xe_device *xe)
>   	 * The contents of register PKG_POWER_SKU_UNIT do not change,
>   	 * so read it once and store the shift values.
>   	 */
> -	if (!ret)
> +	if (!ret) {
>   		hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
> +		hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
> +	}
> +
> +	/*
> +	 * Initialize 'struct hwmon_energy_info', i.e. set fields to the
> +	 * first value of the energy register read
> +	 */
> +	if (hwmon_is_visible(ddat, hwmon_energy, hwmon_energy_input, 0))
> +		hwmon_energy_get(ddat, &energy);
> +
> +	for_each_gt(gt, xe, id)
> +		if (hwmon_gt_is_visible(&hwmon->ddat_gt[id], hwmon_energy,
> +					hwmon_energy_input, 0))
> +			hwmon_energy_get(&hwmon->ddat_gt[id], &energy);
>   }
>   
>   void xe_hwmon_register(struct xe_device *xe)
> @@ -567,6 +744,9 @@ void xe_hwmon_register(struct xe_device *xe)
>   	struct xe_hwmon *hwmon;
>   	struct device *hwmon_dev;
>   	struct xe_hwmon_data *ddat;
> +	struct xe_hwmon_data *ddat_gt;
> +	struct xe_gt *gt;
> +	u8 id;
>   
>   	/* hwmon is available only for dGfx */
>   	if (!IS_DGFX(xe))
> @@ -583,13 +763,21 @@ void xe_hwmon_register(struct xe_device *xe)
>   
>   	/* primary GT to access device level properties */
>   	ddat->gt = xe->tiles[0].primary_gt;
> +	ddat->type = HWMON_DEVICE;
>   
>   	snprintf(ddat->name, sizeof(ddat->name), "xe");
>   
> -	hwmon_get_preregistration_info(xe);
> -
>   	init_waitqueue_head(&hwmon->waitq);
>   
> +	for_each_gt(gt, xe, id) {
> +		ddat_gt = hwmon->ddat_gt + id;
> +		ddat_gt->gt = gt;
> +		snprintf(ddat_gt->name, sizeof(ddat_gt->name), "xe_gt%u", id);
> +		ddat_gt->type = HWMON_GT;
> +	}
> +
> +	hwmon_get_preregistration_info(xe);
> +
>   	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>   
>   	/* hwmon_dev points to device hwmon<i> */
> @@ -605,6 +793,26 @@ void xe_hwmon_register(struct xe_device *xe)
>   	}
>   
>   	ddat->hwmon_dev = hwmon_dev;
> +
> +	for_each_gt(gt, xe, id) {
> +		ddat_gt = hwmon->ddat_gt + id;
> +		/*
> +		 * Create per-gt directories only if a per-gt attribute is
> +		 * visible. Currently this is only energy
> +		 */
> +		if (!hwmon_gt_is_visible(ddat_gt, hwmon_energy, hwmon_energy_input, 0))
> +			continue;
> +
> +		hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_gt->name,
> +								 ddat_gt,
> +								 &hwmon_gt_chip_info,
> +								 NULL);
> +		if (IS_ERR(hwmon_dev))
> +			drm_warn(&xe->drm, "Fail to register xe_gt %d hwmon, Err:%ld\n",
> +				 id, PTR_ERR(hwmon_dev));
> +		else
> +			ddat_gt->hwmon_dev = hwmon_dev;
> +	}

There should be just one hardware monitoring device. Just use energyN
and reference the input with an appropriate sensor label.

Guenter

>   }
>   
>   void xe_hwmon_unregister(struct xe_device *xe)

Re: [PATCH v3 5/6] drm/xe/hwmon: Expose hwmon energy attribute

[Nilawar, Badal]

Hi Guenter,

On 02-08-2023 19:44, Guenter Roeck wrote:
> On 8/2/23 06:52, Badal Nilawar wrote:
>> Expose hwmon energy attribute to show device level and gt
>> level energy usage
>>
>> v2:
>>    - %s/hwm_/hwmon_/
>>    - %s/tile_/gt_
>>    - Convert enums to upper case
>>    - Print error info for hwmon_gt devices
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  12 +
>>   drivers/gpu/drm/xe/regs/xe_gt_regs.h          |   2 +
>>   drivers/gpu/drm/xe/regs/xe_mchbar_regs.h      |   3 +
>>   drivers/gpu/drm/xe/xe_hwmon.c                 | 216 +++++++++++++++++-
>>   4 files changed, 229 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon 
>> b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> index 167bd9480602..4b2d6e1d0c7f 100644
>> --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> @@ -52,3 +52,15 @@ Description:    RO. Current Voltage in millivolt.
>>           Only supported for particular Intel xe graphics platforms.
>> +What:        /sys/devices/.../hwmon/hwmon<i>/energy1_input
>> +Date:        August 2023
>> +KernelVersion:    6.4
>> +Contact:    intel-xe@lists.freedesktop.org
>> +Description:    RO. Energy input of device or gt in microjoules.
>> +
>> +        For xe device level hwmon devices (name "xe") this
>> +        reflects energy input for the entire device. For gt level
>> +        hwmon devices (name "xe_gtN") this reflects energy input
>> +        for the gt.
>> +
>> +        Only supported for particular Intel xe graphics platforms.
>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h 
>> b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> index cc452ec999fc..8819b934a592 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> @@ -400,8 +400,10 @@
>>   #define XEHPC_BCS5_BCS6_INTR_MASK        XE_REG(0x190118)
>>   #define XEHPC_BCS7_BCS8_INTR_MASK        XE_REG(0x19011c)
>> +#define PVC_GT0_PACKAGE_ENERGY_STATUS        XE_REG(0x281004)
>>   #define PVC_GT0_PACKAGE_RAPL_LIMIT        XE_REG(0x281008)
>>   #define PVC_GT0_PACKAGE_POWER_SKU_UNIT        XE_REG(0x281068)
>> +#define PVC_GT0_PLATFORM_ENERGY_STATUS        XE_REG(0x28106c)
>>   #define PVC_GT0_PACKAGE_POWER_SKU        XE_REG(0x281080)
>>   #endif
>> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h 
>> b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> index cb2d49b5c8a9..473a44bd7c56 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> @@ -25,6 +25,9 @@
>>   #define PCU_CR_PACKAGE_POWER_SKU_UNIT        
>> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
>>   #define   PKG_PWR_UNIT                REG_GENMASK(3, 0)
>> +#define   PKG_ENERGY_UNIT            REG_GENMASK(12, 8)
>> +
>> +#define PCU_CR_PACKAGE_ENERGY_STATUS        
>> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
>>   #define PCU_CR_PACKAGE_RAPL_LIMIT        
>> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
>>   #define   PKG_PWR_LIM_1                REG_GENMASK(14, 0)
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c 
>> b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 3e69cd79c1e2..a337edcebae5 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -22,6 +22,8 @@ enum hwmon_reg_name {
>>       REG_PKG_POWER_SKU,
>>       REG_PKG_POWER_SKU_UNIT,
>>       REG_GT_PERF_STATUS,
>> +    REG_ENERGY_STATUS_ALL,
>> +    REG_ENERGY_STATUS_GT,
>>   };
>>   enum hwmon_reg_operation {
>> @@ -30,31 +32,50 @@ enum hwmon_reg_operation {
>>       REG_RMW,
>>   };
>> +enum xe_hwmon_device_type {
>> +    HWMON_GT,
>> +    HWMON_DEVICE,
>> +};
>> +
>>   /*
>>    * SF_* - scale factors for particular quantities according to hwmon 
>> spec.
>>    * - power  - microwatts
>>    * - curr   - milliamperes
>>    * - voltage  - millivolts
>> + * - energy - microjoules
>>    */
>>   #define SF_POWER    1000000
>>   #define SF_CURR        1000
>>   #define SF_VOLTAGE    1000
>> +#define SF_ENERGY    1000000
>> +
>> +struct hwmon_energy_info {
>> +    u32 reg_val_prev;
>> +    long accum_energy;        /* Accumulated energy for energy1_input */
>> +};
>>   struct xe_hwmon_data {
>>       struct device *hwmon_dev;
>>       struct xe_gt *gt;
>>       char name[12];
>> +    struct hwmon_energy_info ei;    /*  Energy info for energy1_input */
>> +    enum xe_hwmon_device_type type;
>>   };
>>   struct xe_hwmon {
>>       struct xe_hwmon_data ddat;
>> +    struct xe_hwmon_data ddat_gt[XE_MAX_TILES_PER_DEVICE];
>>       struct mutex hwmon_lock; /* rmw operations*/
>>       bool reset_in_progress;
>>       wait_queue_head_t waitq;
>>       int scl_shift_power;
>> +    int scl_shift_energy;
>>   };
>> -#define ddat_to_xe_hwmon(ddat)    ({ container_of(ddat, struct 
>> xe_hwmon, ddat); })
>> +#define ddat_to_xe_hwmon(ddat)    \
>> +    ({ ddat->type == HWMON_GT ?    \
>> +        container_of(ddat, struct xe_hwmon, 
>> ddat_gt[ddat->gt->info.id]) :    \
>> +        container_of(ddat, struct xe_hwmon, ddat); })
>>   static u32 hwmon_get_reg(struct xe_hwmon_data *ddat, enum 
>> hwmon_reg_name reg_name)
>>   {
>> @@ -84,6 +105,16 @@ static u32 hwmon_get_reg(struct xe_hwmon_data 
>> *ddat, enum hwmon_reg_name reg_nam
>>           if (xe->info.platform == XE_DG2)
>>               reg = GT_PERF_STATUS;
>>           break;
>> +    case REG_ENERGY_STATUS_ALL:
>> +        if (xe->info.platform == XE_DG2)
>> +            reg = PCU_CR_PACKAGE_ENERGY_STATUS;
>> +        else if (xe->info.platform == XE_PVC)
>> +            reg = PVC_GT0_PLATFORM_ENERGY_STATUS;
>> +        break;
>> +    case REG_ENERGY_STATUS_GT:
>> +        if (xe->info.platform == XE_PVC)
>> +            reg = PVC_GT0_PACKAGE_ENERGY_STATUS;
>> +        break;
>>       default:
>>           XE_MISSING_CASE(reg_name);
>>           break;
>> @@ -228,10 +259,69 @@ static int hwmon_power_rated_max_read(struct 
>> xe_hwmon_data *ddat, long *value)
>>       return 0;
>>   }
>> +/*
>> + * hwmon_energy_get - Obtain energy value
>> + *
>> + * The underlying energy hardware register is 32-bits and is subject to
>> + * overflow. How long before overflow? For example, with an example
>> + * scaling bit shift of 14 bits (see register 
>> *PACKAGE_POWER_SKU_UNIT) and
>> + * a power draw of 1000 watts, the 32-bit counter will overflow in
>> + * approximately 4.36 minutes.
>> + *
>> + * Examples:
>> + *    1 watt:  (2^32 >> 14) /    1 W / (60 * 60 * 24) secs/day -> 3 days
>> + * 1000 watts: (2^32 >> 14) / 1000 W / 60             secs/min -> 
>> 4.36 minutes
>> + *
>> + * The function significantly increases overflow duration (from 4.36
>> + * minutes) by accumulating the energy register into a 'long' as 
>> allowed by
>> + * the hwmon API. Using x86_64 128 bit arithmetic (see 
>> mul_u64_u32_shr()),
>> + * a 'long' of 63 bits, SF_ENERGY of 1e6 (~20 bits) and
>> + * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits 
>> before
>> + * energy1_input overflows. This at 1000 W is an overflow duration of 
>> 278 years.
>> + */
>> +static void
>> +hwmon_energy_get(struct xe_hwmon_data *ddat, long *energy)
>> +{
>> +    struct xe_hwmon *hwmon = ddat_to_xe_hwmon(ddat);
>> +    struct hwmon_energy_info *ei = &ddat->ei;
>> +    u32 reg_val;
>> +
>> +    xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
>> +    mutex_lock(&hwmon->hwmon_lock);
>> +
>> +    if (ddat->type == HWMON_GT)
>> +        process_hwmon_reg(ddat, REG_ENERGY_STATUS_GT, REG_READ,
>> +                  &reg_val, 0, 0);
>> +    else
>> +        process_hwmon_reg(ddat, REG_ENERGY_STATUS_ALL, REG_READ,
>> +                  &reg_val, 0, 0);
>> +
>> +    if (reg_val >= ei->reg_val_prev)
>> +        ei->accum_energy += reg_val - ei->reg_val_prev;
>> +    else
>> +        ei->accum_energy += UINT_MAX - ei->reg_val_prev + reg_val;
>> +
>> +    ei->reg_val_prev = reg_val;
>> +
>> +    *energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
>> +                  hwmon->scl_shift_energy);
>> +
>> +    mutex_unlock(&hwmon->hwmon_lock);
>> +
>> +    xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +}
>> +
>>   static const struct hwmon_channel_info *hwmon_info[] = {
>>       HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | 
>> HWMON_P_CRIT),
>>       HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
>>       HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
>> +    HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
>> +    NULL
>> +};
>> +
>> +static const struct hwmon_channel_info *hwmon_gt_info[] = {
>> +    HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
>>       NULL
>>   };
>> @@ -449,6 +539,32 @@ hwmon_in_read(struct xe_hwmon_data *ddat, u32 
>> attr, long *val)
>>       return ret;
>>   }
>> +static umode_t
>> +hwmon_energy_is_visible(struct xe_hwmon_data *ddat, u32 attr)
>> +{
>> +    switch (attr) {
>> +    case hwmon_energy_input:
>> +        if (ddat->type == HWMON_GT)
>> +            return hwmon_get_reg(ddat, REG_ENERGY_STATUS_GT) ? 0444 : 0;
>> +        else
>> +            return hwmon_get_reg(ddat, REG_ENERGY_STATUS_ALL) ? 0444 
>> : 0;
>> +    default:
>> +        return 0;
>> +    }
>> +}
>> +
>> +static int
>> +hwmon_energy_read(struct xe_hwmon_data *ddat, u32 attr, long *val)
>> +{
>> +    switch (attr) {
>> +    case hwmon_energy_input:
>> +        hwmon_energy_get(ddat, val);
>> +        return 0;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +}
>> +
>>   static umode_t
>>   hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>>            u32 attr, int channel)
>> @@ -468,6 +584,9 @@ hwmon_is_visible(const void *drvdata, enum 
>> hwmon_sensor_types type,
>>       case hwmon_in:
>>           ret = hwmon_in_is_visible(ddat, attr);
>>           break;
>> +    case hwmon_energy:
>> +        ret = hwmon_energy_is_visible(ddat, attr);
>> +        break;
>>       default:
>>           ret = 0;
>>           break;
>> @@ -497,6 +616,9 @@ hwmon_read(struct device *dev, enum 
>> hwmon_sensor_types type, u32 attr,
>>       case hwmon_in:
>>           ret = hwmon_in_read(ddat, attr, val);
>>           break;
>> +    case hwmon_energy:
>> +        ret = hwmon_energy_read(ddat, attr, val);
>> +        break;
>>       default:
>>           ret = -EOPNOTSUPP;
>>           break;
>> @@ -544,12 +666,53 @@ static const struct hwmon_chip_info 
>> hwmon_chip_info = {
>>       .info = hwmon_info,
>>   };
>> +static umode_t
>> +hwmon_gt_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>> +            u32 attr, int channel)
>> +{
>> +    struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
>> +
>> +    switch (type) {
>> +    case hwmon_energy:
>> +        return hwmon_energy_is_visible(ddat, attr);
>> +    default:
>> +        return 0;
>> +    }
>> +}
>> +
>> +static int
>> +hwmon_gt_read(struct device *dev, enum hwmon_sensor_types type, u32 
>> attr,
>> +          int channel, long *val)
>> +{
>> +    struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
>> +
>> +    switch (type) {
>> +    case hwmon_energy:
>> +        return hwmon_energy_read(ddat, attr, val);
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +}
>> +
>> +static const struct hwmon_ops hwmon_gt_ops = {
>> +    .is_visible = hwmon_gt_is_visible,
>> +    .read = hwmon_gt_read,
>> +};
>> +
>> +static const struct hwmon_chip_info hwmon_gt_chip_info = {
>> +    .ops = &hwmon_gt_ops,
>> +    .info = hwmon_gt_info,
>> +};
>> +
>>   static void
>>   hwmon_get_preregistration_info(struct xe_device *xe)
>>   {
>>       struct xe_hwmon *hwmon = xe->hwmon;
>>       struct xe_hwmon_data *ddat = &hwmon->ddat;
>> +    struct xe_gt *gt;
>> +    long energy;
>>       u32 val_sku_unit = 0;
>> +    u8 id;
>>       int ret;
>>       ret = process_hwmon_reg(ddat, REG_PKG_POWER_SKU_UNIT, REG_READ, 
>> &val_sku_unit, 0, 0);
>> @@ -557,8 +720,22 @@ hwmon_get_preregistration_info(struct xe_device *xe)
>>        * The contents of register PKG_POWER_SKU_UNIT do not change,
>>        * so read it once and store the shift values.
>>        */
>> -    if (!ret)
>> +    if (!ret) {
>>           hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, 
>> val_sku_unit);
>> +        hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, 
>> val_sku_unit);
>> +    }
>> +
>> +    /*
>> +     * Initialize 'struct hwmon_energy_info', i.e. set fields to the
>> +     * first value of the energy register read
>> +     */
>> +    if (hwmon_is_visible(ddat, hwmon_energy, hwmon_energy_input, 0))
>> +        hwmon_energy_get(ddat, &energy);
>> +
>> +    for_each_gt(gt, xe, id)
>> +        if (hwmon_gt_is_visible(&hwmon->ddat_gt[id], hwmon_energy,
>> +                    hwmon_energy_input, 0))
>> +            hwmon_energy_get(&hwmon->ddat_gt[id], &energy);
>>   }
>>   void xe_hwmon_register(struct xe_device *xe)
>> @@ -567,6 +744,9 @@ void xe_hwmon_register(struct xe_device *xe)
>>       struct xe_hwmon *hwmon;
>>       struct device *hwmon_dev;
>>       struct xe_hwmon_data *ddat;
>> +    struct xe_hwmon_data *ddat_gt;
>> +    struct xe_gt *gt;
>> +    u8 id;
>>       /* hwmon is available only for dGfx */
>>       if (!IS_DGFX(xe))
>> @@ -583,13 +763,21 @@ void xe_hwmon_register(struct xe_device *xe)
>>       /* primary GT to access device level properties */
>>       ddat->gt = xe->tiles[0].primary_gt;
>> +    ddat->type = HWMON_DEVICE;
>>       snprintf(ddat->name, sizeof(ddat->name), "xe");
>> -    hwmon_get_preregistration_info(xe);
>> -
>>       init_waitqueue_head(&hwmon->waitq);
>> +    for_each_gt(gt, xe, id) {
>> +        ddat_gt = hwmon->ddat_gt + id;
>> +        ddat_gt->gt = gt;
>> +        snprintf(ddat_gt->name, sizeof(ddat_gt->name), "xe_gt%u", id);
>> +        ddat_gt->type = HWMON_GT;
>> +    }
>> +
>> +    hwmon_get_preregistration_info(xe);
>> +
>>       drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>>       /* hwmon_dev points to device hwmon<i> */
>> @@ -605,6 +793,26 @@ void xe_hwmon_register(struct xe_device *xe)
>>       }
>>       ddat->hwmon_dev = hwmon_dev;
>> +
>> +    for_each_gt(gt, xe, id) {
>> +        ddat_gt = hwmon->ddat_gt + id;
>> +        /*
>> +         * Create per-gt directories only if a per-gt attribute is
>> +         * visible. Currently this is only energy
>> +         */
>> +        if (!hwmon_gt_is_visible(ddat_gt, hwmon_energy, 
>> hwmon_energy_input, 0))
>> +            continue;
>> +
>> +        hwmon_dev = devm_hwmon_device_register_with_info(dev, 
>> ddat_gt->name,
>> +                                 ddat_gt,
>> +                                 &hwmon_gt_chip_info,
>> +                                 NULL);
>> +        if (IS_ERR(hwmon_dev))
>> +            drm_warn(&xe->drm, "Fail to register xe_gt %d hwmon, 
>> Err:%ld\n",
>> +                 id, PTR_ERR(hwmon_dev));
>> +        else
>> +            ddat_gt->hwmon_dev = hwmon_dev;
>> +    }
> 
> There should be just one hardware monitoring device. Just use energyN
> and reference the input with an appropriate sensor label.
Idea was to expose energy attributes under saperate hwmon folder with 
device names xe_gtN. But with channel/label approach it will look like 
energyN_input (energy1_input - device, energy_2,3input - gt0/gt1) with 
appropriate energyN_label (energy1_label = "energy device", energy_2,3 = 
"energy gt0/gt1". With this approach we can avoid using 2 structures 
xe_hwmon and xe_hwmon_data.

Regards,
Badal
> 
> Guenter
> 
>>   }
>>   void xe_hwmon_unregister(struct xe_device *xe)
> 

Re: [PATCH v3 5/6] drm/xe/hwmon: Expose hwmon energy attribute

[Guenter Roeck]

On 8/2/23 23:34, Nilawar, Badal wrote:

>>
>> There should be just one hardware monitoring device. Just use energyN
>> and reference the input with an appropriate sensor label.
> Idea was to expose energy attributes under saperate hwmon folder with device names xe_gtN. But with channel/label approach it will look like energyN_input (energy1_input - device, energy_2,3input - gt0/gt1) with appropriate energyN_label (energy1_label = "energy device", energy_2,3 = "energy gt0/gt1". With this approach we can avoid using 2 structures xe_hwmon and xe_hwmon_data.
> 

There is really no such thing as "energy device". I'd suggest to find a better name,
such as "Package energy" or just "Package".

Anyway, your code is flawed: It claims to handle energy overflows, but it doesn't
really do that reliably. The returned numbers are only reliable if read at least
once within an overflow interval. Making it reliable would require introducing
a worker or kernel thread which reads the data from the chip in regular intervals
and accumulates independently of attribute reads.

Guenter

[PATCH v3 6/6] drm/xe/hwmon: Expose power1_max_interval

[Badal Nilawar]

Expose power1_max_interval, that is the tau corresponding to PL1, as a
custom hwmon attribute. Some bit manipulation is needed because of the
format of PKG_PWR_LIM_1_TIME in
PACKAGE_RAPL_LIMIT register (1.x * power(2,y))

v2: Get rpm wake ref while accessing power1_max_interval

Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-xe-hwmon   |  11 ++
 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h      |   8 +
 drivers/gpu/drm/xe/xe_hwmon.c                 | 139 +++++++++++++++++-
 3 files changed, 157 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index 4b2d6e1d0c7f..3be1e55bfac0 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -64,3 +64,14 @@ Description:	RO. Energy input of device or gt in microjoules.
 		for the gt.
 
 		Only supported for particular Intel xe graphics platforms.
+
+What:		/sys/devices/.../hwmon/hwmon<i>/power1_max_interval
+Date:		August 2023
+KernelVersion:	6.4
+Contact:	intel-xe@lists.freedesktop.org
+Description:	RW. Sustained power limit interval (Tau in PL1/Tau) in
+		milliseconds over which sustained power is averaged.
+
+		Only supported for particular Intel xe graphics platforms.
+
+
diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
index 473a44bd7c56..6897fe70b243 100644
--- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
@@ -22,16 +22,24 @@
 #define   PKG_PKG_TDP				GENMASK_ULL(14, 0)
 #define   PKG_MIN_PWR				GENMASK_ULL(30, 16)
 #define   PKG_MAX_PWR				GENMASK_ULL(46, 32)
+#define   PKG_MAX_WIN				GENMASK_ULL(54, 48)
+#define     PKG_MAX_WIN_X			GENMASK_ULL(54, 53)
+#define     PKG_MAX_WIN_Y			GENMASK_ULL(52, 48)
+
 
 #define PCU_CR_PACKAGE_POWER_SKU_UNIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
 #define   PKG_PWR_UNIT				REG_GENMASK(3, 0)
 #define   PKG_ENERGY_UNIT			REG_GENMASK(12, 8)
+#define   PKG_TIME_UNIT				REG_GENMASK(19, 16)
 
 #define PCU_CR_PACKAGE_ENERGY_STATUS		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
 
 #define PCU_CR_PACKAGE_RAPL_LIMIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
 #define   PKG_PWR_LIM_1				REG_GENMASK(14, 0)
 #define   PKG_PWR_LIM_1_EN			REG_BIT(15)
+#define   PKG_PWR_LIM_1_TIME			REG_GENMASK(23, 17)
+#define   PKG_PWR_LIM_1_TIME_X			REG_GENMASK(23, 22)
+#define   PKG_PWR_LIM_1_TIME_Y			REG_GENMASK(21, 17)
 
 #endif
 
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index a337edcebae5..f10b43e24db0 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -48,6 +48,7 @@ enum xe_hwmon_device_type {
 #define SF_CURR		1000
 #define SF_VOLTAGE	1000
 #define SF_ENERGY	1000000
+#define SF_TIME		1000
 
 struct hwmon_energy_info {
 	u32 reg_val_prev;
@@ -70,6 +71,7 @@ struct xe_hwmon {
 	wait_queue_head_t waitq;
 	int scl_shift_power;
 	int scl_shift_energy;
+	int scl_shift_time;
 };
 
 #define ddat_to_xe_hwmon(ddat)	\
@@ -312,6 +314,140 @@ hwmon_energy_get(struct xe_hwmon_data *ddat, long *energy)
 	xe_device_mem_access_put(gt_to_xe(ddat->gt));
 }
 
+static ssize_t
+hwmon_power1_max_interval_show(struct device *dev, struct device_attribute *attr,
+			       char *buf)
+{
+	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
+	struct xe_hwmon *hwmon = container_of(ddat, struct xe_hwmon, ddat);
+	u32 r, x, y, x_w = 2; /* 2 bits */
+	u64 tau4, out;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT,
+			  REG_READ, &r, 0, 0);
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
+	y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
+	/*
+	 * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
+	 *     = (4 | x) << (y - 2)
+	 * where (y - 2) ensures a 1.x fixed point representation of 1.x
+	 * However because y can be < 2, we compute
+	 *     tau4 = (4 | x) << y
+	 * but add 2 when doing the final right shift to account for units
+	 */
+	tau4 = ((1 << x_w) | x) << y;
+	/* val in hwmon interface units (millisec) */
+	out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
+
+	return sysfs_emit(buf, "%llu\n", out);
+}
+
+static ssize_t
+hwmon_power1_max_interval_store(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
+	struct xe_hwmon *hwmon = container_of(ddat, struct xe_hwmon, ddat);
+	u32 x, y, rxy, x_w = 2; /* 2 bits */
+	u64 tau4, r, max_win;
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y = 0x12
+	 * The hwmon->scl_shift_time default of 0xa results in a max tau of 256 seconds
+	 */
+#define PKG_MAX_WIN_DEFAULT 0x12ull
+
+	/*
+	 * val must be < max in hwmon interface units. The steps below are
+	 * explained in hwmon_power1_max_interval_show()
+	 */
+	r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT);
+	x = REG_FIELD_GET(PKG_MAX_WIN_X, r);
+	y = REG_FIELD_GET(PKG_MAX_WIN_Y, r);
+	tau4 = ((1 << x_w) | x) << y;
+	max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
+
+	if (val > max_win)
+		return -EINVAL;
+
+	/* val in hw units */
+	val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME);
+	/* Convert to 1.x * power(2,y) */
+	if (!val) {
+		/* Avoid ilog2(0) */
+		y = 0;
+		x = 0;
+	} else {
+		y = ilog2(val);
+		/* x = (val - (1 << y)) >> (y - 2); */
+		x = (val - (1ul << y)) << x_w >> y;
+	}
+
+	rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	mutex_lock(&hwmon->hwmon_lock);
+
+	process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r,
+			  PKG_PWR_LIM_1_TIME, rxy);
+
+	mutex_unlock(&hwmon->hwmon_lock);
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
+			  hwmon_power1_max_interval_show,
+			  hwmon_power1_max_interval_store, 0);
+
+static struct attribute *hwmon_attributes[] = {
+	&sensor_dev_attr_power1_max_interval.dev_attr.attr,
+	NULL
+};
+
+static umode_t hwmon_attributes_visible(struct kobject *kobj,
+					struct attribute *attr, int index)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
+	u32 reg_val;
+	int ret = 0;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr)
+		ret =  process_hwmon_reg(ddat, REG_PKG_RAPL_LIMIT,
+					 REG_READ, &reg_val, 0, 0) ? 0 : attr->mode;
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
+}
+
+static const struct attribute_group hwmon_attrgroup = {
+	.attrs = hwmon_attributes,
+	.is_visible = hwmon_attributes_visible,
+};
+
+static const struct attribute_group *hwmon_groups[] = {
+	&hwmon_attrgroup,
+	NULL
+};
+
 static const struct hwmon_channel_info *hwmon_info[] = {
 	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
 	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
@@ -723,6 +859,7 @@ hwmon_get_preregistration_info(struct xe_device *xe)
 	if (!ret) {
 		hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
 		hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
+		hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT, val_sku_unit);
 	}
 
 	/*
@@ -784,7 +921,7 @@ void xe_hwmon_register(struct xe_device *xe)
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
 							 ddat,
 							 &hwmon_chip_info,
-							 NULL);
+							 hwmon_groups);
 
 	if (IS_ERR(hwmon_dev)) {
 		drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
-- 
2.25.1