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

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

[Badal Nilawar]

This series adds the hwmon support on xe driver for 
DGFX

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                 | 989 ++++++++++++++++++
 drivers/gpu/drm/xe/xe_hwmon.h                 |  26 +
 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 +
 11 files changed, 1174 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 v2 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.
This is port from i915 hwmon.

v2: Fix review comments (Riana)

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        | 116 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_hwmon.h        |  22 +++++
 5 files changed, 148 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 4b82cb2773ad..e39d77037622 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -113,6 +113,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 c7985af85a53..0fcd60037d66 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"
 
 static int xe_file_open(struct drm_device *dev, struct drm_file *file)
 {
@@ -328,6 +329,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;
@@ -354,6 +357,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 0226d44a6af2..21bff0e610a1 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -332,6 +332,8 @@ struct xe_device {
 	/** @d3cold_allowed: Indicates if d3cold is a valid device state */
 	bool d3cold_allowed;
 
+	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..8f653fdf4ad5
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include <linux/hwmon.h>
+
+#include "regs/xe_gt_regs.h"
+#include "xe_device.h"
+#include "xe_hwmon.h"
+
+struct hwm_drvdata {
+	struct xe_hwmon *hwmon;
+	struct device *hwmon_dev;
+	char name[12];
+};
+
+struct xe_hwmon {
+	struct hwm_drvdata ddat;
+	struct mutex hwmon_lock;
+};
+
+static const struct hwmon_channel_info *hwm_info[] = {
+	NULL
+};
+
+static umode_t
+hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+	       u32 attr, int channel)
+{
+	switch (type) {
+	default:
+		return 0;
+	}
+}
+
+static int
+hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	 int channel, long *val)
+{
+	switch (type) {
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int
+hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	  int channel, long val)
+{
+	switch (type) {
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops hwm_ops = {
+	.is_visible = hwm_is_visible,
+	.read = hwm_read,
+	.write = hwm_write,
+};
+
+static const struct hwmon_chip_info hwm_chip_info = {
+	.ops = &hwm_ops,
+	.info = hwm_info,
+};
+
+static void
+hwm_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 hwm_drvdata *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;
+	mutex_init(&hwmon->hwmon_lock);
+	ddat = &hwmon->ddat;
+
+	ddat->hwmon = hwmon;
+	snprintf(ddat->name, sizeof(ddat->name), "xe");
+
+	hwm_get_preregistration_info(xe);
+
+	drm_dbg(&xe->drm, "Register HWMON interface\n");
+
+	/*  hwmon_dev points to device hwmon<i> */
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
+							 ddat,
+							 &hwm_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev)) {
+		drm_warn(&xe->drm, "Fail to register xe hwmon\n");
+		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 v2 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Matthew Brost]

On Wed, Jun 28, 2023 at 12:00:38AM +0530, 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.
> This is port from i915 hwmon.
> 
> v2: Fix review comments (Riana)
> 
> 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        | 116 +++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hwmon.h        |  22 +++++
>  5 files changed, 148 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 4b82cb2773ad..e39d77037622 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -113,6 +113,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 c7985af85a53..0fcd60037d66 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"
>  
>  static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>  {
> @@ -328,6 +329,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;
> @@ -354,6 +357,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 0226d44a6af2..21bff0e610a1 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -332,6 +332,8 @@ struct xe_device {
>  	/** @d3cold_allowed: Indicates if d3cold is a valid device state */
>  	bool d3cold_allowed;
>  
> +	struct xe_hwmon *hwmon;
> +

You likely need a 'struct xe_hwmon' forward declaration at the top of
the file.

>  	/* 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..8f653fdf4ad5
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/hwmon.h>
> +
> +#include "regs/xe_gt_regs.h"
> +#include "xe_device.h"
> +#include "xe_hwmon.h"
> +
> +struct hwm_drvdata {
> +	struct xe_hwmon *hwmon;
> +	struct device *hwmon_dev;
> +	char name[12];
> +};
> +
> +struct xe_hwmon {
> +	struct hwm_drvdata ddat;
> +	struct mutex hwmon_lock;
> +};
> +

These two structures look very goofy, e.g. why two 2 structs instead of
1, why a back pointer in 'struct hwm_drvdata' rather than using
container_of? Just make this one structure?

> +static const struct hwmon_channel_info *hwm_info[] = {
> +	NULL
> +};
> +
> +static umode_t
> +hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,

Nit and matter of opinion but...

s/hwm_is_visible/xe_hwmon_is_visible

> +	       u32 attr, int channel)
> +{
> +	switch (type) {
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int
> +hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +	 int channel, long *val)

s/hwm_read/xe_hwmon_read

> +{
> +	switch (type) {
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int
> +hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +	  int channel, long val)
> +{

s/hwm_write/xe_hwmon_write

> +	switch (type) {
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_ops hwm_ops = {

s/hwm_ops/xe_hwmon_ops

> +	.is_visible = hwm_is_visible,
> +	.read = hwm_read,
> +	.write = hwm_write,
> +};
> +
> +static const struct hwmon_chip_info hwm_chip_info = {

s/hwm_chip_info/xe_hwmon_chip_info

> +	.ops = &hwm_ops,
> +	.info = hwm_info,
> +};
> +
> +static void
> +hwm_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 hwm_drvdata *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;
> +	mutex_init(&hwmon->hwmon_lock);

drmm_mutex_init

Matt

> +	ddat = &hwmon->ddat;
> +
> +	ddat->hwmon = hwmon;
> +	snprintf(ddat->name, sizeof(ddat->name), "xe");
> +
> +	hwm_get_preregistration_info(xe);
> +
> +	drm_dbg(&xe->drm, "Register HWMON interface\n");
> +
> +	/*  hwmon_dev points to device hwmon<i> */
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> +							 ddat,
> +							 &hwm_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev)) {
> +		drm_warn(&xe->drm, "Fail to register xe hwmon\n");
> +		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 v2 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Nilawar, Badal]

Hi Matthew,

On 29-06-2023 04:20, Matthew Brost wrote:
> On Wed, Jun 28, 2023 at 12:00:38AM +0530, 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.
>> This is port from i915 hwmon.
>>
>> v2: Fix review comments (Riana)
>>
>> 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        | 116 +++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_hwmon.h        |  22 +++++
>>   5 files changed, 148 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 4b82cb2773ad..e39d77037622 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -113,6 +113,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 c7985af85a53..0fcd60037d66 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"
>>   
>>   static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>>   {
>> @@ -328,6 +329,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;
>> @@ -354,6 +357,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 0226d44a6af2..21bff0e610a1 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -332,6 +332,8 @@ struct xe_device {
>>   	/** @d3cold_allowed: Indicates if d3cold is a valid device state */
>>   	bool d3cold_allowed;
>>   
>> +	struct xe_hwmon *hwmon;
>> +
> 
> You likely need a 'struct xe_hwmon' forward declaration at the top of
> the file.
Sure it will move it to top of the file.
> 
>>   	/* 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..8f653fdf4ad5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -0,0 +1,116 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +
>> +#include "regs/xe_gt_regs.h"
>> +#include "xe_device.h"
>> +#include "xe_hwmon.h"
>> +
>> +struct hwm_drvdata {
>> +	struct xe_hwmon *hwmon;
>> +	struct device *hwmon_dev;
>> +	char name[12];
>> +};
>> +
>> +struct xe_hwmon {
>> +	struct hwm_drvdata ddat;
>> +	struct mutex hwmon_lock;
>> +};
>> +
> 
> These two structures look very goofy, e.g. why two 2 structs instead of
> 1, why a back pointer in 'struct hwm_drvdata' rather than using
> container_of? Just make this one structure?
In energy patch to cover gt specific energy attr there are multiple 
instances of hwm_drvdata defined. which creates saperate hwmon entry for 
each gt. But I will think about adding one structure.
> 
>> +static const struct hwmon_channel_info *hwm_info[] = {
>> +	NULL
>> +};
>> +
>> +static umode_t
>> +hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> 
> Nit and matter of opinion but...
> 
> s/hwm_is_visible/xe_hwmon_is_visible
Throught out driver prefix hwm_ is used for static function and for 
global functions prefix xe_hwmon_ is used. Instead of hwm_* prefix 
hwmon_ prefix can be used.
> 
>> +	       u32 attr, int channel)
>> +{
>> +	switch (type) {
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int
>> +hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +	 int channel, long *val)
> 
> s/hwm_read/xe_hwmon_read
> 
>> +{
>> +	switch (type) {
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int
>> +hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +	  int channel, long val)
>> +{
> 
> s/hwm_write/xe_hwmon_write
> 
>> +	switch (type) {
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static const struct hwmon_ops hwm_ops = {
> 
> s/hwm_ops/xe_hwmon_ops
> 
>> +	.is_visible = hwm_is_visible,
>> +	.read = hwm_read,
>> +	.write = hwm_write,
>> +};
>> +
>> +static const struct hwmon_chip_info hwm_chip_info = {
> 
> s/hwm_chip_info/xe_hwmon_chip_info
> 
>> +	.ops = &hwm_ops,
>> +	.info = hwm_info,
>> +};
>> +
>> +static void
>> +hwm_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 hwm_drvdata *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;
>> +	mutex_init(&hwmon->hwmon_lock);
> 
> drmm_mutex_init
Sure I will change this.
> 
> Matt
> 
>> +	ddat = &hwmon->ddat;
>> +
>> +	ddat->hwmon = hwmon;
>> +	snprintf(ddat->name, sizeof(ddat->name), "xe");
>> +
>> +	hwm_get_preregistration_info(xe);
>> +
>> +	drm_dbg(&xe->drm, "Register HWMON interface\n");
>> +
>> +	/*  hwmon_dev points to device hwmon<i> */
>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>> +							 ddat,
>> +							 &hwm_chip_info,
>> +							 NULL);
>> +	if (IS_ERR(hwmon_dev)) {
>> +		drm_warn(&xe->drm, "Fail to register xe hwmon\n");
>> +		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 v2 1/6] drm/xe/hwmon: Add HWMON infrastructure

[Andi Shyti]

Hi Badal,

[...]

>  static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>  {
> @@ -328,6 +329,8 @@ int xe_device_probe(struct xe_device *xe)
>  
>  	xe_debugfs_register(xe);
>  
> +	xe_hwmon_register(xe);

we don't have any information whe hwmon has been created or not.

Please return an error and print a warning if something wrong
happens and we fail to register the hwmon interface.

[...]

> +struct hwm_drvdata {
> +	struct xe_hwmon *hwmon;
> +	struct device *hwmon_dev;
> +	char name[12];
> +};
> +
> +struct xe_hwmon {
> +	struct hwm_drvdata ddat;
> +	struct mutex hwmon_lock;
> +};

What's the reason for having two different structures here?

[...]

> +void xe_hwmon_register(struct xe_device *xe)
> +{
> +	struct device *dev = xe->drm.dev;
> +	struct xe_hwmon *hwmon;
> +	struct device *hwmon_dev;
> +	struct hwm_drvdata *ddat;
> +
> +	/* hwmon is available only for dGfx */
> +	if (!IS_DGFX(xe))
> +		return;

this check is useless, we wouldn't be here otherwise.

[...]

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

We could also destroy the mutex and unregister the hwmon?

[...]

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

for readability we can leave some space between the
#if...#else...#endif and the content.

Andi

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

[Nilawar, Badal]

Hi Andy,

On 29-06-2023 19:19, Andi Shyti wrote:
> Hi Badal,
> 
> [...]
> 
>>   static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>>   {
>> @@ -328,6 +329,8 @@ int xe_device_probe(struct xe_device *xe)
>>   
>>   	xe_debugfs_register(xe);
>>   
>> +	xe_hwmon_register(xe);
> 
> we don't have any information whe hwmon has been created or not.
> 
> Please return an error and print a warning if something wrong
> happens and we fail to register the hwmon interface.
Inside xe_hwmon_register warning is being printed, is it not enough?
> 
> [...]
> 
>> +struct hwm_drvdata {
>> +	struct xe_hwmon *hwmon;
>> +	struct device *hwmon_dev;
>> +	char name[12];
>> +};
>> +
>> +struct xe_hwmon {
>> +	struct hwm_drvdata ddat;
>> +	struct mutex hwmon_lock;
>> +};
> 
> What's the reason for having two different structures here?
Going further to handle gt specific hwmon (energy) attributes multiple 
instances of hwm_drvdata added in xe_hwmon. With that it will create 
separate hwmon sysfs entry under separate name. Also xe_hwmon holds 
common elements. With single structure approach we might need to add 
xe_hwmon instances in xe gt structure as well. As of now we are handling 
device specific and gt specific attributes. Going further there could be 
card specific attributes as well. With existing approach we can add 
another instance of hwm_drvdata to xe_hwmon with single structure 
approach it will be tricky to handle card specific attrs.

root@DUT729PVC:/home/gta/bellekal/kernel# cat /sys/class/hwmon/hwmon*/name
xe
xe_tile0
xe_tile1
> 
> [...]
> 
>> +void xe_hwmon_register(struct xe_device *xe)
>> +{
>> +	struct device *dev = xe->drm.dev;
>> +	struct xe_hwmon *hwmon;
>> +	struct device *hwmon_dev;
>> +	struct hwm_drvdata *ddat;
>> +
>> +	/* hwmon is available only for dGfx */
>> +	if (!IS_DGFX(xe))
>> +		return;
> 
> this check is useless, we wouldn't be here otherwise.
As of now hwmon is only applicable for dgfx so added this check.
> 
> [...]
> 
>> +void xe_hwmon_unregister(struct xe_device *xe)
>> +{
>> +	xe->hwmon = NULL;
> 
> We could also destroy the mutex and unregister the hwmon?
During i915_hwmon discussion there was suggestion from you to remove 
mutex_destroy as this is not the case of create/destroy or debug.
https://patchwork.freedesktop.org/patch/503368/?series=104278&rev=6
As suggested by Matt planning to use drmm_mutex_init which will take 
care of destroy.
> 
> [...]
> 
>> +#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
> 
> for readability we can leave some space between the
Sure I will fix this.
> #if...#else...#endif and the content.
> 
> Andi

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

[Badal Nilawar]

Expose power_max (pl1) and power_rated_max (tdp) attributes.
This is port from i915 hwmon.

v2:
  - Move rpm calls (xe_device_mem_access_get/put) to hwmon functions from
    process_hwmon_reg to avoid multiple rpm entry exits during consecutive
    reg accesses
  - Fix review comments (Riana)

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                 | 372 +++++++++++++++++-
 drivers/gpu/drm/xe/xe_hwmon.h                 |   4 +
 drivers/gpu/drm/xe/xe_uc.c                    |   6 +
 6 files changed, 435 insertions(+), 7 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..ff3465195870
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -0,0 +1,22 @@
+What:		/sys/devices/.../hwmon/hwmon<i>/power1_max
+Date:		July 2023
+KernelVersion:	6.3
+Contact:	intel-gfx@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:		July 2023
+KernelVersion:	6.3
+Contact:	intel-gfx@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 8f653fdf4ad5..a4fba29d5d5a 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -5,53 +5,394 @@
 
 #include <linux/hwmon.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 hwm_reg_name {
+	pkg_rapl_limit,
+	pkg_power_sku,
+	pkg_power_sku_unit,
+};
+
+enum hwm_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 hwm_drvdata {
 	struct xe_hwmon *hwmon;
 	struct device *hwmon_dev;
+	struct xe_gt *gt;
 	char name[12];
+	bool reset_in_progress;
+	wait_queue_head_t waitq;
 };
 
 struct xe_hwmon {
 	struct hwm_drvdata ddat;
 	struct mutex hwmon_lock;
+	int scl_shift_power;
 };
 
+struct xe_reg hwm_get_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name)
+{
+	struct xe_device *xe = gt_to_xe(ddat->gt);
+
+	switch (reg_name) {
+	case pkg_rapl_limit:
+		if (xe->info.platform == XE_DG2)
+			return PCU_CR_PACKAGE_RAPL_LIMIT;
+		else if (xe->info.platform == XE_PVC)
+			return PVC_GT0_PACKAGE_RAPL_LIMIT;
+		break;
+	case pkg_power_sku:
+		if (xe->info.platform == XE_DG2)
+			return PCU_CR_PACKAGE_POWER_SKU;
+		else if (xe->info.platform == XE_PVC)
+			return PVC_GT0_PACKAGE_POWER_SKU;
+		break;
+	case pkg_power_sku_unit:
+		if (xe->info.platform == XE_DG2)
+			return PCU_CR_PACKAGE_POWER_SKU_UNIT;
+		else if (xe->info.platform == XE_PVC)
+			return PVC_GT0_PACKAGE_POWER_SKU_UNIT;
+		break;
+	default:
+		break;
+	}
+
+	return XE_REG(0);
+}
+
+int process_hwmon_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name,
+		      enum hwm_reg_operation operation, u32 *value,
+		      u32 clr, u32 set)
+{
+	struct xe_reg reg;
+	int ret = 0;
+
+	reg = hwm_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:
+		ret = -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+int process_hwmon_reg_read64(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name, u64 *value)
+{
+	struct xe_reg reg;
+
+	reg = hwm_get_reg(ddat, reg_name);
+
+	if (!reg.raw)
+		return -EOPNOTSUPP;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	*value = xe_mmio_read64(ddat->gt, reg);
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	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 hwm_power_max_read(struct hwm_drvdata *ddat, long *value)
+{
+	struct xe_hwmon *hwmon = ddat->hwmon;
+	u32 reg_val;
+	u64 r, min, max;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	process_hwmon_reg(ddat, 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;
+		xe_device_mem_access_put(gt_to_xe(ddat->gt));
+		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, 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);
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+	return 0;
+}
+
+static int hwm_power_max_write(struct hwm_drvdata *ddat, long value)
+{
+	struct xe_hwmon *hwmon = ddat->hwmon;
+	DEFINE_WAIT(wait);
+	int ret = 0;
+	u32 nval;
+
+	/* Block waiting for GuC reset to complete when needed */
+	for (;;) {
+		mutex_lock(&hwmon->hwmon_lock);
+
+		prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE);
+
+		if (!hwmon->ddat.reset_in_progress)
+			break;
+
+		if (signal_pending(current)) {
+			ret = -EINTR;
+			break;
+		}
+
+		mutex_unlock(&hwmon->hwmon_lock);
+
+		schedule();
+	}
+	finish_wait(&ddat->waitq, &wait);
+	if (ret)
+		goto unlock;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
+	if (value == PL1_DISABLE) {
+		process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &nval,
+				  PKG_PWR_LIM_1_EN, 0);
+		process_hwmon_reg(ddat, pkg_rapl_limit, reg_read, &nval,
+				  PKG_PWR_LIM_1_EN, 0);
+
+		if (nval & PKG_PWR_LIM_1_EN)
+			ret = -ENODEV;
+		goto exit;
+	}
+
+	/* 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, pkg_rapl_limit, reg_rmw, &nval,
+			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
+exit:
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+unlock:
+	mutex_unlock(&hwmon->hwmon_lock);
+
+	return 0;
+}
+
+static int hwm_power_rated_max_read(struct hwm_drvdata *ddat, long *value)
+{
+	struct xe_hwmon *hwmon = ddat->hwmon;
+	u32 reg_val;
+
+	process_hwmon_reg(ddat, 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 *hwm_info[] = {
+	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
 	NULL
 };
 
+static umode_t
+hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
+{
+	u32 reg_val;
+
+	switch (attr) {
+	case hwmon_power_max:
+		return process_hwmon_reg(ddat, pkg_rapl_limit,
+					 reg_read, &reg_val, 0, 0) ? 0 : 0664;
+	case hwmon_power_rated_max:
+		return process_hwmon_reg(ddat, pkg_power_sku,
+					 reg_read, &reg_val, 0, 0) ? 0 : 0444;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
+{
+	switch (attr) {
+	case hwmon_power_max:
+		return hwm_power_max_read(ddat, val);
+	case hwmon_power_rated_max:
+		return hwm_power_rated_max_read(ddat, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int
+hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
+{
+	switch (attr) {
+	case hwmon_power_max:
+		return hwm_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 hwm_drvdata *ddat = &hwmon->ddat;
+	u32 r;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	if (!hwmon || process_hwmon_reg(ddat, pkg_rapl_limit,
+					reg_read, &r, 0, 0))
+		return;
+
+	mutex_lock(&hwmon->hwmon_lock);
+
+	hwmon->ddat.reset_in_progress = true;
+
+	process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &r,
+			  PKG_PWR_LIM_1_EN, 0);
+	*old = !!(r & PKG_PWR_LIM_1_EN);
+
+	mutex_unlock(&hwmon->hwmon_lock);
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+}
+
+void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
+{
+	struct xe_hwmon *hwmon = xe->hwmon;
+	struct hwm_drvdata *ddat = &hwmon->ddat;
+	u32 r;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	if (!hwmon || process_hwmon_reg(ddat, pkg_rapl_limit,
+					reg_read, &r, 0, 0))
+		return;
+
+	mutex_lock(&hwmon->hwmon_lock);
+
+	process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &r,
+			  PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0);
+
+	hwmon->ddat.reset_in_progress = false;
+	wake_up_all(&hwmon->ddat.waitq);
+
+	mutex_unlock(&hwmon->hwmon_lock);
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+}
+
 static umode_t
 hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	       u32 attr, int channel)
 {
+	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
 	switch (type) {
+	case hwmon_power:
+		ret = hwm_power_is_visible(ddat, attr, channel);
+		break;
 	default:
-		return 0;
+		ret = 0;
 	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
 }
 
 static int
 hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	 int channel, long *val)
 {
+	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
 	switch (type) {
+	case hwmon_power:
+		ret = hwm_power_read(ddat, attr, channel, val);
+		break;
 	default:
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		break;
 	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
 }
 
 static int
 hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	  int channel, long val)
 {
+	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
 	switch (type) {
+	case hwmon_power:
+		ret = hwm_power_write(ddat, attr, channel, val);
+		break;
 	default:
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		break;
 	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
 }
 
 static const struct hwmon_ops hwm_ops = {
@@ -66,8 +407,19 @@ static const struct hwmon_chip_info hwm_chip_info = {
 };
 
 static void
-hwm_get_preregistration_info(struct xe_device *xe)
+hwm_get_preregistration_info(struct hwm_drvdata *ddat)
 {
+	struct xe_hwmon *hwmon = ddat->hwmon;
+	u32 val_sku_unit = 0;
+	int ret;
+
+	ret = process_hwmon_reg(ddat, 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)
@@ -89,18 +441,24 @@ void xe_hwmon_register(struct xe_device *xe)
 	mutex_init(&hwmon->hwmon_lock);
 	ddat = &hwmon->ddat;
 
+	/* primary GT to access device level properties */
+	ddat->gt = xe->tiles[0].primary_gt;
+
 	ddat->hwmon = hwmon;
 	snprintf(ddat->name, sizeof(ddat->name), "xe");
 
-	hwm_get_preregistration_info(xe);
+	init_waitqueue_head(&ddat->waitq);
 
-	drm_dbg(&xe->drm, "Register HWMON interface\n");
+	hwm_get_preregistration_info(ddat);
 
-	/*  hwmon_dev points to device hwmon<i> */
+	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,
 							 &hwm_chip_info,
 							 NULL);
+
 	if (IS_ERR(hwmon_dev)) {
 		drm_warn(&xe->drm, "Fail to register xe hwmon\n");
 		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_uc.c b/drivers/gpu/drm/xe/xe_uc.c
index 70eabf567156..9df5a3a85dc3 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 v2 2/6] drm/xe/hwmon: Expose power attributes

[Matthew Brost]

On Wed, Jun 28, 2023 at 12:00:39AM +0530, Badal Nilawar wrote:
> Expose power_max (pl1) and power_rated_max (tdp) attributes.
> This is port from i915 hwmon.
> 

Not expert in this domain but this statement makes be nervous as
generally shouldn't just blindly port things from the i915. Doing an
initial review but will need a domain expert too.

> v2:
>   - Move rpm calls (xe_device_mem_access_get/put) to hwmon functions from
>     process_hwmon_reg to avoid multiple rpm entry exits during consecutive
>     reg accesses
>   - Fix review comments (Riana)
> 
> 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                 | 372 +++++++++++++++++-
>  drivers/gpu/drm/xe/xe_hwmon.h                 |   4 +
>  drivers/gpu/drm/xe/xe_uc.c                    |   6 +
>  6 files changed, 435 insertions(+), 7 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..ff3465195870
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> @@ -0,0 +1,22 @@
> +What:		/sys/devices/.../hwmon/hwmon<i>/power1_max
> +Date:		July 2023

Future?

> +KernelVersion:	6.3
> +Contact:	intel-gfx@lists.freedesktop.org

s/intel-gfx/intel-xe

> +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:		July 2023
> +KernelVersion:	6.3
> +Contact:	intel-gfx@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 8f653fdf4ad5..a4fba29d5d5a 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -5,53 +5,394 @@
>  
>  #include <linux/hwmon.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"

Nit - includes should be in alphabetical order.

> +
> +enum hwm_reg_name {
> +	pkg_rapl_limit,
> +	pkg_power_sku,
> +	pkg_power_sku_unit,

Upper case? I guess hwmon_power_max is lower case so is that the
convention?

> +};
> +
> +enum hwm_reg_operation {
> +	reg_read,
> +	reg_write,
> +	reg_rmw,

Upper case?

> +};
> +

s/hwm_reg/xe_hwm_reg

> +/*
> + * SF_* - scale factors for particular quantities according to hwmon spec.
> + * - power  - microwatts
> + */
> +#define SF_POWER	1000000
>  
>  struct hwm_drvdata {
>  	struct xe_hwmon *hwmon;
>  	struct device *hwmon_dev;
> +	struct xe_gt *gt;
>  	char name[12];
> +	bool reset_in_progress;
> +	wait_queue_head_t waitq;
>  };
>  
>  struct xe_hwmon {
>  	struct hwm_drvdata ddat;
>  	struct mutex hwmon_lock;
> +	int scl_shift_power;
>  };
>

Same as previous patch, 1 struct seems like a better idea to me.

> +struct xe_reg hwm_get_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name)

s/hwm_get_reg/xe_hwmon_get_reg

> +{
> +	struct xe_device *xe = gt_to_xe(ddat->gt);
> +
> +	switch (reg_name) {
> +	case pkg_rapl_limit:
> +		if (xe->info.platform == XE_DG2)
> +			return PCU_CR_PACKAGE_RAPL_LIMIT;
> +		else if (xe->info.platform == XE_PVC)
> +			return PVC_GT0_PACKAGE_RAPL_LIMIT;
> +		break;
> +	case pkg_power_sku:
> +		if (xe->info.platform == XE_DG2)
> +			return PCU_CR_PACKAGE_POWER_SKU;
> +		else if (xe->info.platform == XE_PVC)
> +			return PVC_GT0_PACKAGE_POWER_SKU;
> +		break;
> +	case pkg_power_sku_unit:
> +		if (xe->info.platform == XE_DG2)
> +			return PCU_CR_PACKAGE_POWER_SKU_UNIT;
> +		else if (xe->info.platform == XE_PVC)
> +			return PVC_GT0_PACKAGE_POWER_SKU_UNIT;
> +		break;
> +	default:

BUG_ON or WARN_ON saying not possible?

> +		break;
> +	}
> +
> +	return XE_REG(0);
> +}
> +
> +int process_hwmon_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name,
> +		      enum hwm_reg_operation operation, u32 *value,
> +		      u32 clr, u32 set)
> +{
> +	struct xe_reg reg;
> +	int ret = 0;
> +
> +	reg = hwm_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:

BUG_ON or WARN_ON saying not possible?

> +		ret = -EOPNOTSUPP;
> +	}
> +
> +	return ret;
> +}
> +
> +int process_hwmon_reg_read64(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name, u64 *value)
> +{
> +	struct xe_reg reg;
> +
> +	reg = hwm_get_reg(ddat, reg_name);
> +
> +	if (!reg.raw)
> +		return -EOPNOTSUPP;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));

Don't need this, the upper levels of the stack should have this. You
could just do a xe_device_assert_mem_access here. 

> +
> +	*value = xe_mmio_read64(ddat->gt, reg);
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	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 hwm_power_max_read(struct hwm_drvdata *ddat, long *value)

The return value is always 0, why not return value?

s/hwm_power_max_read/xe_hwmon_power_max_read

> +{
> +	struct xe_hwmon *hwmon = ddat->hwmon;
> +	u32 reg_val;
> +	u64 r, min, max;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +

Same as above, use xe_device_assert_mem_access.

> +	process_hwmon_reg(ddat, 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;
> +		xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +		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, 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);
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +	return 0;
> +}
> +
> +static int hwm_power_max_write(struct hwm_drvdata *ddat, long value)
> +{
> +	struct xe_hwmon *hwmon = ddat->hwmon;
> +	DEFINE_WAIT(wait);
> +	int ret = 0;
> +	u32 nval;
> +
> +	/* Block waiting for GuC reset to complete when needed */
> +	for (;;) {
> +		mutex_lock(&hwmon->hwmon_lock);
> +
> +		prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE);
> +
> +		if (!hwmon->ddat.reset_in_progress)
> +			break;
> +
> +		if (signal_pending(current)) {
> +			ret = -EINTR;
> +			break;
> +		}
> +
> +		mutex_unlock(&hwmon->hwmon_lock);
> +
> +		schedule();
> +	}
> +	finish_wait(&ddat->waitq, &wait);
> +	if (ret)
> +		goto unlock;

Anyway to not open code this? We similar in with
xe_guc_submit_reset_wait, could we expose a global reset in progress in
function which we can expose at the gt level?

> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +

This certainly is an outer most thing, e.g. doing this under
hwmon->hwmon_lock seems dangerous. Again the upper levels of the stack
should do the xe_device_mem_access_get, which it does. Just pointing out
doing xe_device_mem_access_get/put under a lock isn't a good idea.

Also the the loop which acquires hwmon->hwmon_lock is confusing too.

> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> +	if (value == PL1_DISABLE) {
> +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &nval,
> +				  PKG_PWR_LIM_1_EN, 0);
> +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_read, &nval,
> +				  PKG_PWR_LIM_1_EN, 0);
> +
> +		if (nval & PKG_PWR_LIM_1_EN)
> +			ret = -ENODEV;
> +		goto exit;
> +	}
> +
> +	/* 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, pkg_rapl_limit, reg_rmw, &nval,
> +			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
> +exit:
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +unlock:
> +	mutex_unlock(&hwmon->hwmon_lock);
> +

Again a clear lock / unlock is desirable.

> +	return 0;
> +}
> +
> +static int hwm_power_rated_max_read(struct hwm_drvdata *ddat, long *value)
> +{

s/hwm_power_rated_max_read/xe_hwmon_power_rated_max_read

> +	struct xe_hwmon *hwmon = ddat->hwmon;
> +	u32 reg_val;
> +
> +	process_hwmon_reg(ddat, 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 *hwm_info[] = {

s/hwm_info/xe_hwmon_info

> +	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
>  	NULL
>  };
>  
> +static umode_t
> +hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
> +{

s/hwm_power_is_visible/xe_hwmon_power_is_visible

> +	u32 reg_val;
> +
> +	switch (attr) {
> +	case hwmon_power_max:
> +		return process_hwmon_reg(ddat, pkg_rapl_limit,
> +					 reg_read, &reg_val, 0, 0) ? 0 : 0664;
> +	case hwmon_power_rated_max:
> +		return process_hwmon_reg(ddat, pkg_power_sku,
> +					 reg_read, &reg_val, 0, 0) ? 0 : 0444;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int
> +hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
> +{

s/hwm_power_read/xe_hwmon_power_read

> +	switch (attr) {
> +	case hwmon_power_max:
> +		return hwm_power_max_read(ddat, val);
> +	case hwmon_power_rated_max:
> +		return hwm_power_rated_max_read(ddat, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int
> +hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
> +{

s/hwm_power_write/xe_hwmon_power_write

> +	switch (attr) {
> +	case hwmon_power_max:
> +		return hwm_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 hwm_drvdata *ddat = &hwmon->ddat;
> +	u32 r;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +

Upper level should have, use an assert if anything.

> +	if (!hwmon || process_hwmon_reg(ddat, pkg_rapl_limit,
> +					reg_read, &r, 0, 0))
> +		return;
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	hwmon->ddat.reset_in_progress = true;
> +
> +	process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &r,
> +			  PKG_PWR_LIM_1_EN, 0);
> +	*old = !!(r & PKG_PWR_LIM_1_EN);
> +
> +	mutex_unlock(&hwmon->hwmon_lock);
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +}
> +
> +void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
> +{
> +	struct xe_hwmon *hwmon = xe->hwmon;
> +	struct hwm_drvdata *ddat = &hwmon->ddat;
> +	u32 r;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));

Upper level should have, use an assert if anything.

> +
> +	if (!hwmon || process_hwmon_reg(ddat, pkg_rapl_limit,
> +					reg_read, &r, 0, 0))
> +		return;
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &r,
> +			  PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0);
> +
> +	hwmon->ddat.reset_in_progress = false;
> +	wake_up_all(&hwmon->ddat.waitq);
> +
> +	mutex_unlock(&hwmon->hwmon_lock);
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +}
> +
>  static umode_t
>  hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>  	       u32 attr, int channel)
>  {
> +	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
> +	int ret;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +

Nit, include the get / put in previous patch.

>  	switch (type) {
> +	case hwmon_power:
> +		ret = hwm_power_is_visible(ddat, attr, channel);
> +		break;
>  	default:
> -		return 0;
> +		ret = 0;
>  	}
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	return ret;
>  }
>  
>  static int
>  hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>  	 int channel, long *val)
>  {
> +	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
> +	int ret;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));

Nit, include the get / put in previous patch.

> +
>  	switch (type) {
> +	case hwmon_power:
> +		ret = hwm_power_read(ddat, attr, channel, val);
> +		break;
>  	default:
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		break;
>  	}
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	return ret;
>  }
>  
>  static int
>  hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>  	  int channel, long val)
>  {
> +	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
> +	int ret;
> +
> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));

Nit, include the get / put in previous patch.

> +
>  	switch (type) {
> +	case hwmon_power:
> +		ret = hwm_power_write(ddat, attr, channel, val);
> +		break;
>  	default:
> -		return -EOPNOTSUPP;
> +		ret = -EOPNOTSUPP;
> +		break;
>  	}
> +
> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> +	return ret;
>  }
>  
>  static const struct hwmon_ops hwm_ops = {
> @@ -66,8 +407,19 @@ static const struct hwmon_chip_info hwm_chip_info = {
>  };
>  
>  static void
> -hwm_get_preregistration_info(struct xe_device *xe)
> +hwm_get_preregistration_info(struct hwm_drvdata *ddat)

Why change the function argument?

>  {
> +	struct xe_hwmon *hwmon = ddat->hwmon;
> +	u32 val_sku_unit = 0;
> +	int ret;
> +
> +	ret = process_hwmon_reg(ddat, 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)
> @@ -89,18 +441,24 @@ void xe_hwmon_register(struct xe_device *xe)
>  	mutex_init(&hwmon->hwmon_lock);
>  	ddat = &hwmon->ddat;
>  
> +	/* primary GT to access device level properties */
> +	ddat->gt = xe->tiles[0].primary_gt;
> +
>  	ddat->hwmon = hwmon;
>  	snprintf(ddat->name, sizeof(ddat->name), "xe");
>  
> -	hwm_get_preregistration_info(xe);
> +	init_waitqueue_head(&ddat->waitq);
>  
> -	drm_dbg(&xe->drm, "Register HWMON interface\n");
> +	hwm_get_preregistration_info(ddat);
>  
> -	/*  hwmon_dev points to device hwmon<i> */
> +	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,
>  							 &hwm_chip_info,
>  							 NULL);
> +

Unrelated, probably delete,

>  	if (IS_ERR(hwmon_dev)) {
>  		drm_warn(&xe->drm, "Fail to register xe hwmon\n");

Missed this the prior patch but include the error value in the print.

>  		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_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index 70eabf567156..9df5a3a85dc3 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);

Don't love the pass by reference, how about 'pl1en =
xe_hwmon_power_max_disable(...'

Matt

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

Add line of white space.

Matt

>  	/* 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 v2 2/6] drm/xe/hwmon: Expose power attributes

[Andi Shyti]

Hi Matt and Badal,

[...]

> > +};
> > +
> > +enum hwm_reg_operation {
> > +	reg_read,
> > +	reg_write,
> > +	reg_rmw,
> 
> Upper case?
> 
> > +};
> > +
> 
> s/hwm_reg/xe_hwm_reg

I agree with Matt here... throughout this series of patches the
naming is very confusing, sometimes starting with xe_*, sometimes
with hwm_*, sometimes with hwmon_*... there is no consistency.

Please, Badal, choos a consistent prefix and stick with it for
every function and global definition.

Matt suggested xe_hwmon_*, I'm fine with it.

[...]

> >  struct hwm_drvdata {
> >  	struct xe_hwmon *hwmon;
> >  	struct device *hwmon_dev;
> > +	struct xe_gt *gt;
> >  	char name[12];
> > +	bool reset_in_progress;
> > +	wait_queue_head_t waitq;
> >  };
> >  
> >  struct xe_hwmon {
> >  	struct hwm_drvdata ddat;
> >  	struct mutex hwmon_lock;
> > +	int scl_shift_power;
> >  };
> >
> 
> Same as previous patch, 1 struct seems like a better idea to me.

I made the same comment in the previous patch.

[...]

> > +	switch (reg_name) {
> > +	case pkg_rapl_limit:
> > +		if (xe->info.platform == XE_DG2)
> > +			return PCU_CR_PACKAGE_RAPL_LIMIT;
> > +		else if (xe->info.platform == XE_PVC)
> > +			return PVC_GT0_PACKAGE_RAPL_LIMIT;
> > +		break;
> > +	case pkg_power_sku:
> > +		if (xe->info.platform == XE_DG2)
> > +			return PCU_CR_PACKAGE_POWER_SKU;
> > +		else if (xe->info.platform == XE_PVC)
> > +			return PVC_GT0_PACKAGE_POWER_SKU;
> > +		break;
> > +	case pkg_power_sku_unit:
> > +		if (xe->info.platform == XE_DG2)
> > +			return PCU_CR_PACKAGE_POWER_SKU_UNIT;
> > +		else if (xe->info.platform == XE_PVC)
> > +			return PVC_GT0_PACKAGE_POWER_SKU_UNIT;
> > +		break;
> > +	default:
> 
> BUG_ON or WARN_ON saying not possible?

MISSING_CASE() is in i915_utils.h, perhaps we can move it to a
more generic place... it would be at handy here.

> > +static int hwm_power_max_read(struct hwm_drvdata *ddat, long *value)
> 
> The return value is always 0, why not return value?
> 
> s/hwm_power_max_read/xe_hwmon_power_max_read
> 
> > +{
> > +	struct xe_hwmon *hwmon = ddat->hwmon;
> > +	u32 reg_val;
> > +	u64 r, min, max;
> > +
> > +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> > +
> 
> Same as above, use xe_device_assert_mem_access.
> 
> > +	process_hwmon_reg(ddat, 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;
> > +		xe_device_mem_access_put(gt_to_xe(ddat->gt));
> > +		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, 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);
> > +
> > +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> > +	return 0;
> > +}
> > +
> > +static int hwm_power_max_write(struct hwm_drvdata *ddat, long value)
> > +{
> > +	struct xe_hwmon *hwmon = ddat->hwmon;
> > +	DEFINE_WAIT(wait);
> > +	int ret = 0;
> > +	u32 nval;
> > +
> > +	/* Block waiting for GuC reset to complete when needed */
> > +	for (;;) {

with a do...while() you shouldn't need a for(;;)... your choice,
not going to beat on that.

> > +		mutex_lock(&hwmon->hwmon_lock);
> > +
> > +		prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE);
> > +
> > +		if (!hwmon->ddat.reset_in_progress)
> > +			break;
> > +
> > +		if (signal_pending(current)) {
> > +			ret = -EINTR;
> > +			break;

cough! cough! unlock! cough! cough!

> > +		}
> > +
> > +		mutex_unlock(&hwmon->hwmon_lock);
> > +
> > +		schedule();
> > +	}
> > +	finish_wait(&ddat->waitq, &wait);
> > +	if (ret)
> > +		goto unlock;
> 
> Anyway to not open code this? We similar in with
> xe_guc_submit_reset_wait, could we expose a global reset in progress in
> function which we can expose at the gt level?
> 
> > +
> > +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> > +
> 
> This certainly is an outer most thing, e.g. doing this under
> hwmon->hwmon_lock seems dangerous. Again the upper levels of the stack
> should do the xe_device_mem_access_get, which it does. Just pointing out
> doing xe_device_mem_access_get/put under a lock isn't a good idea.
> 
> Also the the loop which acquires hwmon->hwmon_lock is confusing too.
> 
> > +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> > +	if (value == PL1_DISABLE) {
> > +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &nval,
> > +				  PKG_PWR_LIM_1_EN, 0);
> > +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_read, &nval,
> > +				  PKG_PWR_LIM_1_EN, 0);
> > +
> > +		if (nval & PKG_PWR_LIM_1_EN)
> > +			ret = -ENODEV;
> > +		goto exit;

cough! cough! lock! cough! cough!

> > +	}
> > +
> > +	/* 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, pkg_rapl_limit, reg_rmw, &nval,
> > +			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
> > +exit:
> > +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> > +unlock:
> > +	mutex_unlock(&hwmon->hwmon_lock);
> > +

mmhhh???... jokes apart this is so error prone that it will
deadlock as soon as someone will think of editing this file :)

It worried me already at the first part.

Please, as Matt said, have a more linear locking here.

[...]

Andi

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

[Dixit, Ashutosh]

On Thu, 29 Jun 2023 07:09:31 -0700, Andi Shyti wrote:
>

Hi Badal/Andi/Matt,

> > > +static int hwm_power_max_write(struct hwm_drvdata *ddat, long value)
> > > +{
> > > +	struct xe_hwmon *hwmon = ddat->hwmon;
> > > +	DEFINE_WAIT(wait);
> > > +	int ret = 0;
> > > +	u32 nval;
> > > +
> > > +	/* Block waiting for GuC reset to complete when needed */
> > > +	for (;;) {
>
> with a do...while() you shouldn't need a for(;;)... your choice,
> not going to beat on that.
>
> > > +		mutex_lock(&hwmon->hwmon_lock);
> > > +
> > > +		prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE);
> > > +
> > > +		if (!hwmon->ddat.reset_in_progress)
> > > +			break;
> > > +
> > > +		if (signal_pending(current)) {
> > > +			ret = -EINTR;
> > > +			break;
>
> cough! cough! unlock! cough! cough!

Why? It's fine as is.

>
> > > +		}
> > > +
> > > +		mutex_unlock(&hwmon->hwmon_lock);
> > > +
> > > +		schedule();
> > > +	}
> > > +	finish_wait(&ddat->waitq, &wait);
> > > +	if (ret)
> > > +		goto unlock;
> >
> > Anyway to not open code this? We similar in with
> > xe_guc_submit_reset_wait, could we expose a global reset in progress in
> > function which we can expose at the gt level?

I don't know of any way to not open code this which is guaranteed to not
deadlock (not to say there are no other ways).

> >
> > > +
> > > +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> > > +
> >
> > This certainly is an outer most thing, e.g. doing this under
> > hwmon->hwmon_lock seems dangerous. Again the upper levels of the stack
> > should do the xe_device_mem_access_get, which it does. Just pointing out
> > doing xe_device_mem_access_get/put under a lock isn't a good idea.

Agree, this is the only change we should make to this code.

> >
> > Also the the loop which acquires hwmon->hwmon_lock is confusing too.

Confusing but correct.

> >
> > > +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> > > +	if (value == PL1_DISABLE) {
> > > +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &nval,
> > > +				  PKG_PWR_LIM_1_EN, 0);
> > > +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_read, &nval,
> > > +				  PKG_PWR_LIM_1_EN, 0);
> > > +
> > > +		if (nval & PKG_PWR_LIM_1_EN)
> > > +			ret = -ENODEV;
> > > +		goto exit;
>
> cough! cough! lock! cough! cough!

Why? It's fine as is.

>
> > > +	}
> > > +
> > > +	/* 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, pkg_rapl_limit, reg_rmw, &nval,
> > > +			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
> > > +exit:
> > > +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
> > > +unlock:
> > > +	mutex_unlock(&hwmon->hwmon_lock);
> > > +
>
> mmhhh???... jokes apart this is so error prone that it will
> deadlock as soon as someone will think of editing this file :)

Why is it error prone and how will it deadlock? In fact this
prepare_to_wait/finish_wait pattern is widely used in the kernel (see
e.g. rpm_suspend) and is one of the few patterns guaranteed to not deadlock
(see also 6.2.5 "Advanced Sleeping" in LDD3 if needed). This is the same
code pattern we also implemented in i915 hwm_power_max_write.

In i915 first a scheme which held a mutex across GuC reset was
proposed. But that was then deemed to be risky and this complex scheme was
then implemented. Just to give some history.

Regarding editing the code, it's kernel code involving locking which needs
to be edited carefully, it's all confined to a single (or maybe a couple of
functions), but otherwise yes definitely not to mess around with :)

>
> It worried me already at the first part.
>
> Please, as Matt said, have a more linear locking here.

Afaiac I don't know of any other race-free way to do this other than what
was done in v2 (and in i915). So I want to discard the changes made in v3
and go back to the changes made in v2. If others can suggest other ways
that which they can guarantee are race-free and correct and can R-b that
code, that's fine.

But I can R-b the v2 code (with the only change of moving
xe_device_mem_access_get out of the lock). (Of course I am only talking
about R-b'ing the above scheme, other review comments may be valid).

Badal, also, if there are questions about this scheme, maybe we should move
this to a separate patch as was done in i915? We can just return -EAGAIN as
in 1b44019a93e2.

Thanks.
--
Ashutosh

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

[Nilawar, Badal]

On 16-08-2023 04:50, Dixit, Ashutosh wrote:
> On Thu, 29 Jun 2023 07:09:31 -0700, Andi Shyti wrote:
>>
> 
> Hi Badal/Andi/Matt,
> 
>>>> +static int hwm_power_max_write(struct hwm_drvdata *ddat, long value)
>>>> +{
>>>> +	struct xe_hwmon *hwmon = ddat->hwmon;
>>>> +	DEFINE_WAIT(wait);
>>>> +	int ret = 0;
>>>> +	u32 nval;
>>>> +
>>>> +	/* Block waiting for GuC reset to complete when needed */
>>>> +	for (;;) {
>>
>> with a do...while() you shouldn't need a for(;;)... your choice,
>> not going to beat on that.
>>
>>>> +		mutex_lock(&hwmon->hwmon_lock);
>>>> +
>>>> +		prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE);
>>>> +
>>>> +		if (!hwmon->ddat.reset_in_progress)
>>>> +			break;
>>>> +
>>>> +		if (signal_pending(current)) {
>>>> +			ret = -EINTR;
>>>> +			break;
>>
>> cough! cough! unlock! cough! cough!
> 
> Why? It's fine as is.
> 
>>
>>>> +		}
>>>> +
>>>> +		mutex_unlock(&hwmon->hwmon_lock);
>>>> +
>>>> +		schedule();
>>>> +	}
>>>> +	finish_wait(&ddat->waitq, &wait);
>>>> +	if (ret)
>>>> +		goto unlock;
>>>
>>> Anyway to not open code this? We similar in with
>>> xe_guc_submit_reset_wait, could we expose a global reset in progress in
>>> function which we can expose at the gt level?
> 
> I don't know of any way to not open code this which is guaranteed to not
> deadlock (not to say there are no other ways).
> 
>>>
>>>> +
>>>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>>>> +
>>>
>>> This certainly is an outer most thing, e.g. doing this under
>>> hwmon->hwmon_lock seems dangerous. Again the upper levels of the stack
>>> should do the xe_device_mem_access_get, which it does. Just pointing out
>>> doing xe_device_mem_access_get/put under a lock isn't a good idea.
> 
> Agree, this is the only change we should make to this code.
> 
>>>
>>> Also the the loop which acquires hwmon->hwmon_lock is confusing too.
> 
> Confusing but correct.
> 
>>>
>>>> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
>>>> +	if (value == PL1_DISABLE) {
>>>> +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &nval,
>>>> +				  PKG_PWR_LIM_1_EN, 0);
>>>> +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_read, &nval,
>>>> +				  PKG_PWR_LIM_1_EN, 0);
>>>> +
>>>> +		if (nval & PKG_PWR_LIM_1_EN)
>>>> +			ret = -ENODEV;
>>>> +		goto exit;
>>
>> cough! cough! lock! cough! cough!
> 
> Why? It's fine as is.
> 
>>
>>>> +	}
>>>> +
>>>> +	/* 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, pkg_rapl_limit, reg_rmw, &nval,
>>>> +			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
>>>> +exit:
>>>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>>>> +unlock:
>>>> +	mutex_unlock(&hwmon->hwmon_lock);
>>>> +
>>
>> mmhhh???... jokes apart this is so error prone that it will
>> deadlock as soon as someone will think of editing this file :)
> 
> Why is it error prone and how will it deadlock? In fact this
> prepare_to_wait/finish_wait pattern is widely used in the kernel (see
> e.g. rpm_suspend) and is one of the few patterns guaranteed to not deadlock
> (see also 6.2.5 "Advanced Sleeping" in LDD3 if needed). This is the same
> code pattern we also implemented in i915 hwm_power_max_write.
> 
> In i915 first a scheme which held a mutex across GuC reset was
> proposed. But that was then deemed to be risky and this complex scheme was
> then implemented. Just to give some history.
> 
> Regarding editing the code, it's kernel code involving locking which needs
> to be edited carefully, it's all confined to a single (or maybe a couple of
> functions), but otherwise yes definitely not to mess around with :)
> 
>>
>> It worried me already at the first part.
>>
>> Please, as Matt said, have a more linear locking here.
> 
> Afaiac I don't know of any other race-free way to do this other than what
> was done in v2 (and in i915). So I want to discard the changes made in v3
> and go back to the changes made in v2. If others can suggest other ways
> that which they can guarantee are race-free and correct and can R-b that
> code, that's fine.
> 
> But I can R-b the v2 code (with the only change of moving
> xe_device_mem_access_get out of the lock). (Of course I am only talking
> about R-b'ing the above scheme, other review comments may be valid).
> 
> Badal, also, if there are questions about this scheme, maybe we should move
> this to a separate patch as was done in i915? We can just return -EAGAIN as
> in 1b44019a93e2.

Thanks Ashutosh. I think for now I will drop changes for "PL1 disable 
during GuC load" from this series and will handle it in separate patch.

Regards,
Badal
> Thanks.
> --
> Ashutosh

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

[Andi Shyti]

Hi Ashutosh,

> > >
> > > Also the the loop which acquires hwmon->hwmon_lock is confusing too.
> 
> Confusing but correct.

Confusing is implicitely not correct.

Might be correct now, but in some moenths someone else might miss
the point because it's confusing, mis-interpret and send the
wrong code.

Reviewers, going through the +/- commit will have a tough time
figuring out.

Let's keep things easy and simple... there are easier paths for
locking here and we've been discussing them with Badal.

Andi

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

[Nilawar, Badal]

Hi Matt,

On 29-06-2023 05:48, Matthew Brost wrote:
> On Wed, Jun 28, 2023 at 12:00:39AM +0530, Badal Nilawar wrote:
>> Expose power_max (pl1) and power_rated_max (tdp) attributes.
>> This is port from i915 hwmon.
>>
> 
> Not expert in this domain but this statement makes be nervous as
> generally shouldn't just blindly port things from the i915. Doing an
> initial review but will need a domain expert too.
This is not blindly ported. Just for reference I added that comment in 
case if someone want to see i915 implementation. I will move that 
comment to cover letter.
> 
>> v2:
>>    - Move rpm calls (xe_device_mem_access_get/put) to hwmon functions from
>>      process_hwmon_reg to avoid multiple rpm entry exits during consecutive
>>      reg accesses
>>    - Fix review comments (Riana)
>>
>> 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                 | 372 +++++++++++++++++-
>>   drivers/gpu/drm/xe/xe_hwmon.h                 |   4 +
>>   drivers/gpu/drm/xe/xe_uc.c                    |   6 +
>>   6 files changed, 435 insertions(+), 7 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..ff3465195870
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> @@ -0,0 +1,22 @@
>> +What:		/sys/devices/.../hwmon/hwmon<i>/power1_max
>> +Date:		July 2023
> 
> Future?
Considering review time frame added future date.
> 
>> +KernelVersion:	6.3
>> +Contact:	intel-gfx@lists.freedesktop.org
> 
> s/intel-gfx/intel-xe
Sure.
> 
>> +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:		July 2023
>> +KernelVersion:	6.3
>> +Contact:	intel-gfx@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 8f653fdf4ad5..a4fba29d5d5a 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -5,53 +5,394 @@
>>   
>>   #include <linux/hwmon.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"
> 
> Nit - includes should be in alphabetical order.Ok
> 
>> +
>> +enum hwm_reg_name {
>> +	pkg_rapl_limit,
>> +	pkg_power_sku,
>> +	pkg_power_sku_unit,
> 
> Upper case? I guess hwmon_power_max is lower case so is that the
> convention?
Yes, but can be made Upper case.
> 
>> +};
>> +
>> +enum hwm_reg_operation {
>> +	reg_read,
>> +	reg_write,
>> +	reg_rmw,
> 
> Upper case?
Ok, I will change this to upper case.
> 
>> +};
>> +
> 
> s/hwm_reg/xe_hwm_reg
> 
>> +/*
>> + * SF_* - scale factors for particular quantities according to hwmon spec.
>> + * - power  - microwatts
>> + */
>> +#define SF_POWER	1000000
>>   
>>   struct hwm_drvdata {
>>   	struct xe_hwmon *hwmon;
>>   	struct device *hwmon_dev;
>> +	struct xe_gt *gt;
>>   	char name[12];
>> +	bool reset_in_progress;
>> +	wait_queue_head_t waitq;
>>   };
>>   
>>   struct xe_hwmon {
>>   	struct hwm_drvdata ddat;
>>   	struct mutex hwmon_lock;
>> +	int scl_shift_power;
>>   };
>>
> 
> Same as previous patch, 1 struct seems like a better idea to me.
> 
>> +struct xe_reg hwm_get_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name)
> 
> s/hwm_get_reg/xe_hwmon_get_reg
As mentioned in previous patch will follow xe_hwmon_ for global functions.
> 
>> +{
>> +	struct xe_device *xe = gt_to_xe(ddat->gt);
>> +
>> +	switch (reg_name) {
>> +	case pkg_rapl_limit:
>> +		if (xe->info.platform == XE_DG2)
>> +			return PCU_CR_PACKAGE_RAPL_LIMIT;
>> +		else if (xe->info.platform == XE_PVC)
>> +			return PVC_GT0_PACKAGE_RAPL_LIMIT;
>> +		break;
>> +	case pkg_power_sku:
>> +		if (xe->info.platform == XE_DG2)
>> +			return PCU_CR_PACKAGE_POWER_SKU;
>> +		else if (xe->info.platform == XE_PVC)
>> +			return PVC_GT0_PACKAGE_POWER_SKU;
>> +		break;
>> +	case pkg_power_sku_unit:
>> +		if (xe->info.platform == XE_DG2)
>> +			return PCU_CR_PACKAGE_POWER_SKU_UNIT;
>> +		else if (xe->info.platform == XE_PVC)
>> +			return PVC_GT0_PACKAGE_POWER_SKU_UNIT;
>> +		break;
>> +	default:
> 
> BUG_ON or WARN_ON saying not possible?
Not needed. For attribute doesn't present it shoud return XE_REG(0).
> 
>> +		break;
>> +	}
>> +
>> +	return XE_REG(0);
>> +}
>> +
>> +int process_hwmon_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name,
>> +		      enum hwm_reg_operation operation, u32 *value,
>> +		      u32 clr, u32 set)
s/process_hwmon_reg/hwmon_process_reg
>> +{
>> +	struct xe_reg reg;
>> +	int ret = 0;
>> +
>> +	reg = hwm_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:
> 
> BUG_ON or WARN_ON saying not possible?
Will add WARN_ON here.
> 
>> +		ret = -EOPNOTSUPP;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +int process_hwmon_reg_read64(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name, u64 *value)
>> +{
>> +	struct xe_reg reg;
>> +
>> +	reg = hwm_get_reg(ddat, reg_name);
>> +
>> +	if (!reg.raw)
>> +		return -EOPNOTSUPP;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> 
> Don't need this, the upper levels of the stack should have this. You
> could just do a xe_device_assert_mem_access here.
Agreed I will remove this and whereever needed will use 
xe_device_assert_mem_access .
> 
>> +
>> +	*value = xe_mmio_read64(ddat->gt, reg);
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +
>> +	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 hwm_power_max_read(struct hwm_drvdata *ddat, long *value)
> 
> The return value is always 0, why not return value?
0 is success.
static ssize_t hwmon_attr_show(struct device *dev,
                                struct device_attribute *devattr, char *buf)
{
         struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
         long val;
         int ret;

         ret = hattr->ops->read(dev, hattr->type, hattr->attr, hattr->index,
                                &val);
         if (ret < 0)
                 return ret;

         trace_hwmon_attr_show(hattr->index + hwmon_attr_base(hattr->type),
                               hattr->name, val);

         return sprintf(buf, "%ld\n", val);
}
> 
> s/hwm_power_max_read/xe_hwmon_power_max_read
> 
>> +{
>> +	struct xe_hwmon *hwmon = ddat->hwmon;
>> +	u32 reg_val;
>> +	u64 r, min, max;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
> 
> Same as above, use xe_device_assert_mem_access.
Not required here as its added at top level function hwm_read. As 
mentioned in v2: I moved rpm calls to top level funcitons but I forgot 
to remove from here and other places. Will fix this in next rev.
> 
>> +	process_hwmon_reg(ddat, 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;
>> +		xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +		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, 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);
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
This is also not needed.
>> +	return 0;
>> +}
>> +
>> +static int hwm_power_max_write(struct hwm_drvdata *ddat, long value)
>> +{
>> +	struct xe_hwmon *hwmon = ddat->hwmon;
>> +	DEFINE_WAIT(wait);
>> +	int ret = 0;
>> +	u32 nval;
>> +
>> +	/* Block waiting for GuC reset to complete when needed */
>> +	for (;;) {
>> +		mutex_lock(&hwmon->hwmon_lock);
>> +
>> +		prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE);
>> +
>> +		if (!hwmon->ddat.reset_in_progress)
>> +			break;
>> +
>> +		if (signal_pending(current)) {
>> +			ret = -EINTR;
>> +			break;
>> +		}
>> +
>> +		mutex_unlock(&hwmon->hwmon_lock);
>> +
>> +		schedule();
>> +	}
>> +	finish_wait(&ddat->waitq, &wait);
>> +	if (ret)
>> +		goto unlock;
> 
> Anyway to not open code this? We similar in with
> xe_guc_submit_reset_wait, could we expose a global reset in progress in
> function which we can expose at the gt level?
Sure, I will fix this.
> 
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
> 
> This certainly is an outer most thing, e.g. doing this under
> hwmon->hwmon_lock seems dangerous. Again the upper levels of the stack
> should do the xe_device_mem_access_get, which it does. Just pointing out
> doing xe_device_mem_access_get/put under a lock isn't a good idea.
Agreed, this is not needed as its called in top level function.
> 
> Also the the loop which acquires hwmon->hwmon_lock is confusing too.
> 
>> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
>> +	if (value == PL1_DISABLE) {
>> +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &nval,
>> +				  PKG_PWR_LIM_1_EN, 0);
>> +		process_hwmon_reg(ddat, pkg_rapl_limit, reg_read, &nval,
>> +				  PKG_PWR_LIM_1_EN, 0);
>> +
>> +		if (nval & PKG_PWR_LIM_1_EN)
>> +			ret = -ENODEV;
>> +		goto exit;
>> +	}
>> +
>> +	/* 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, pkg_rapl_limit, reg_rmw, &nval,
>> +			  PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval);
>> +exit:
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
Not needed here.
>> +unlock:
>> +	mutex_unlock(&hwmon->hwmon_lock);
>> +
> 
> Again a clear lock / unlock is desirable.
> 
>> +	return 0;
>> +}
>> +
>> +static int hwm_power_rated_max_read(struct hwm_drvdata *ddat, long *value)
>> +{
> 
> s/hwm_power_rated_max_read/xe_hwmon_power_rated_max_read
> 
>> +	struct xe_hwmon *hwmon = ddat->hwmon;
>> +	u32 reg_val;
>> +
>> +	process_hwmon_reg(ddat, 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 *hwm_info[] = {
> 
> s/hwm_info/xe_hwmon_info
Sure.
> 
>> +	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
>>   	NULL
>>   };
>>   
>> +static umode_t
>> +hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
>> +{
> 
> s/hwm_power_is_visible/xe_hwmon_power_is_visible
Will prefer xe_hwmon prefix for global functions.
> 
>> +	u32 reg_val;
>> +
>> +	switch (attr) {
>> +	case hwmon_power_max:
>> +		return process_hwmon_reg(ddat, pkg_rapl_limit,
>> +					 reg_read, &reg_val, 0, 0) ? 0 : 0664;
>> +	case hwmon_power_rated_max:
>> +		return process_hwmon_reg(ddat, pkg_power_sku,
>> +					 reg_read, &reg_val, 0, 0) ? 0 : 0444;
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int
>> +hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
>> +{
> 
> s/hwm_power_read/xe_hwmon_power_read
> 
>> +	switch (attr) {
>> +	case hwmon_power_max:
>> +		return hwm_power_max_read(ddat, val);
>> +	case hwmon_power_rated_max:
>> +		return hwm_power_rated_max_read(ddat, val);
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int
>> +hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
>> +{
> 
> s/hwm_power_write/xe_hwmon_power_write
> 
>> +	switch (attr) {
>> +	case hwmon_power_max:
>> +		return hwm_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 hwm_drvdata *ddat = &hwmon->ddat;
>> +	u32 r;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
> 
> Upper level should have, use an assert if anything.
Aggree, Infact I am thinking to remove this as this is already called at 
top level functions.
> 
>> +	if (!hwmon || process_hwmon_reg(ddat, pkg_rapl_limit,
>> +					reg_read, &r, 0, 0))
>> +		return;
>> +
>> +	mutex_lock(&hwmon->hwmon_lock);
>> +
>> +	hwmon->ddat.reset_in_progress = true;
>> +
>> +	process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &r,
>> +			  PKG_PWR_LIM_1_EN, 0);
>> +	*old = !!(r & PKG_PWR_LIM_1_EN);
>> +
>> +	mutex_unlock(&hwmon->hwmon_lock);
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +}
>> +
>> +void xe_hwmon_power_max_restore(struct xe_device *xe, bool old)
>> +{
>> +	struct xe_hwmon *hwmon = xe->hwmon;
>> +	struct hwm_drvdata *ddat = &hwmon->ddat;
>> +	u32 r;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> 
> Upper level should have, use an assert if anything.
Agree. Will remove this as top level function already calling this.
> 
>> +
>> +	if (!hwmon || process_hwmon_reg(ddat, pkg_rapl_limit,
>> +					reg_read, &r, 0, 0))
>> +		return;
>> +
>> +	mutex_lock(&hwmon->hwmon_lock);
>> +
>> +	process_hwmon_reg(ddat, pkg_rapl_limit, reg_rmw, &r,
>> +			  PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0);
>> +
>> +	hwmon->ddat.reset_in_progress = false;
>> +	wake_up_all(&hwmon->ddat.waitq);
>> +
>> +	mutex_unlock(&hwmon->hwmon_lock);
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +}
>> +
>>   static umode_t
>>   hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>>   	       u32 attr, int channel)
>>   {
>> +	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
>> +	int ret;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
>> +
> 
> Nit, include the get / put in previous patch.
Ok.
> 
>>   	switch (type) {
>> +	case hwmon_power:
>> +		ret = hwm_power_is_visible(ddat, attr, channel);
>> +		break;
>>   	default:
>> -		return 0;
>> +		ret = 0;
>>   	}
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +
>> +	return ret;
>>   }
>>   
>>   static int
>>   hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>   	 int channel, long *val)
>>   {
>> +	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> 
> Nit, include the get / put in previous patch.
Ok
> 
>> +
>>   	switch (type) {
>> +	case hwmon_power:
>> +		ret = hwm_power_read(ddat, attr, channel, val);
>> +		break;
>>   	default:
>> -		return -EOPNOTSUPP;
>> +		ret = -EOPNOTSUPP;
>> +		break;
>>   	}
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +
>> +	return ret;
>>   }
>>   
>>   static int
>>   hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>>   	  int channel, long val)
>>   {
>> +	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	xe_device_mem_access_get(gt_to_xe(ddat->gt));
> 
> Nit, include the get / put in previous patch.
Ok
> 
>> +
>>   	switch (type) {
>> +	case hwmon_power:
>> +		ret = hwm_power_write(ddat, attr, channel, val);
>> +		break;
>>   	default:
>> -		return -EOPNOTSUPP;
>> +		ret = -EOPNOTSUPP;
>> +		break;
>>   	}
>> +
>> +	xe_device_mem_access_put(gt_to_xe(ddat->gt));
>> +
>> +	return ret;
>>   }
>>   
>>   static const struct hwmon_ops hwm_ops = {
>> @@ -66,8 +407,19 @@ static const struct hwmon_chip_info hwm_chip_info = {
>>   };
>>   
>>   static void
>> -hwm_get_preregistration_info(struct xe_device *xe)
>> +hwm_get_preregistration_info(struct hwm_drvdata *ddat)
> 
> Why change the function argument?
> 
>>   {
>> +	struct xe_hwmon *hwmon = ddat->hwmon;
>> +	u32 val_sku_unit = 0;
>> +	int ret;
>> +
>> +	ret = process_hwmon_reg(ddat, 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)
>> @@ -89,18 +441,24 @@ void xe_hwmon_register(struct xe_device *xe)
>>   	mutex_init(&hwmon->hwmon_lock);
>>   	ddat = &hwmon->ddat;
>>   
>> +	/* primary GT to access device level properties */
>> +	ddat->gt = xe->tiles[0].primary_gt;
>> +
>>   	ddat->hwmon = hwmon;
>>   	snprintf(ddat->name, sizeof(ddat->name), "xe");
>>   
>> -	hwm_get_preregistration_info(xe);
>> +	init_waitqueue_head(&ddat->waitq);
>>   
>> -	drm_dbg(&xe->drm, "Register HWMON interface\n");
>> +	hwm_get_preregistration_info(ddat);
>>   
>> -	/*  hwmon_dev points to device hwmon<i> */
>> +	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,
>>   							 &hwm_chip_info,
>>   							 NULL);
>> +
> 
> Unrelated, probably delete,
> 
>>   	if (IS_ERR(hwmon_dev)) {
>>   		drm_warn(&xe->drm, "Fail to register xe hwmon\n");
> 
> Missed this the prior patch but include the error value in the print.
Sure.
> 
>>   		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_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>> index 70eabf567156..9df5a3a85dc3 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);
> 
> Don't love the pass by reference, how about 'pl1en =
> xe_hwmon_power_max_disable(...'
Sure, I will remove pass by reference.
> 
> Matt
> 
>> +
>>   	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);
> 
> Add line of white space.
Ok.

Regards,
Badal
> 
> Matt
> 
>>   	/* 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
>>

[PATCH v2 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).
This is port from i915 hwmon.

v2: Move PCODE_MBOX macro to pcode file (Riana)

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

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

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

[Andi Shyti]

Hi Badal,

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

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

[...]

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

I like better the form below, with

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

>  	default:
>  		return 0;

[...]

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

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

Andi

[...]

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

[Dixit, Ashutosh]

On Thu, 29 Jun 2023 07:40:00 -0700, Andi Shyti wrote:
>

Hi Andi,

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

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

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

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

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

Thanks.
--
Ashutosh

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

[Badal Nilawar]

Use Xe HWMON subsystem to display the input voltage.
This is port from i915 hwmon.

v2:
  - Rename hwm_get_vltg to hwm_get_voltage (Riana)
  - Use scale factor SF_VOLTAGE (Riana)

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                 | 67 +++++++++++++++++++
 3 files changed, 76 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index bee1d62bfddb..33a793b58157 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:		July 2023
+KernelVersion:	6.3
+Contact:	intel-gfx@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 7068120d9200..06b4251f26fd 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 "regs/xe_mchbar_regs.h"
 #include "regs/xe_gt_regs.h"
@@ -19,6 +21,7 @@ enum hwm_reg_name {
 	pkg_rapl_limit,
 	pkg_power_sku,
 	pkg_power_sku_unit,
+	gt_perf_status,
 };
 
 enum hwm_reg_operation {
@@ -31,9 +34,11 @@ enum hwm_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 hwm_drvdata {
 	struct xe_hwmon *hwmon;
@@ -73,6 +78,11 @@ struct xe_reg hwm_get_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name)
 		else if (xe->info.platform == XE_PVC)
 			return PVC_GT0_PACKAGE_POWER_SKU_UNIT;
 		break;
+	case gt_perf_status:
+		if (IS_DG2(gt_to_xe(ddat->gt)))
+			return GT_PERF_STATUS;
+		else
+			return XE_REG(0);
 	default:
 		break;
 	}
@@ -239,6 +249,7 @@ static int hwm_power_rated_max_read(struct hwm_drvdata *ddat, long *value)
 static const struct hwmon_channel_info *hwm_info[] = {
 	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
 	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
+	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
 	NULL
 };
 
@@ -261,6 +272,22 @@ static int hwm_pcode_write_i1(struct xe_gt *gt, u32 uval)
 			      uval);
 }
 
+static int hwm_get_voltage(struct hwm_drvdata *ddat, long *value)
+{
+	u32 reg_val;
+
+	if (IS_DG2(gt_to_xe(ddat->gt))) {
+		process_hwmon_reg(ddat, 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;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static umode_t
 hwm_power_is_visible(struct hwm_drvdata *ddat, u32 attr, int chan)
 {
@@ -422,6 +449,40 @@ hwm_curr_write(struct hwm_drvdata *ddat, u32 attr, long val)
 	}
 }
 
+static umode_t
+hwm_in_is_visible(struct hwm_drvdata *ddat, u32 attr)
+{
+	u32 reg_val;
+
+	switch (attr) {
+	case hwmon_in_input:
+		return process_hwmon_reg(ddat, gt_perf_status,
+					 reg_read, &reg_val, 0, 0) ? 0 : 0444;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val)
+{
+	int ret;
+
+	xe_device_mem_access_get(gt_to_xe(ddat->gt));
+
+	switch (attr) {
+	case hwmon_in_input:
+		ret = hwm_get_voltage(ddat, val);
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+	}
+
+	xe_device_mem_access_put(gt_to_xe(ddat->gt));
+
+	return ret;
+}
+
 static umode_t
 hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	       u32 attr, int channel)
@@ -438,6 +499,9 @@ hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	case hwmon_curr:
 		ret = hwm_curr_is_visible(ddat, attr);
 		break;
+	case hwmon_in:
+		ret = hwm_in_is_visible(ddat, attr);
+		break;
 	default:
 		ret = 0;
 	}
@@ -463,6 +527,9 @@ hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_curr:
 		ret = hwm_curr_read(ddat, attr, val);
 		break;
+	case hwmon_in:
+		ret = hwm_in_read(ddat, attr, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
-- 
2.25.1

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

[Andi Shyti]

Hi Badal,

> +static int hwm_get_voltage(struct hwm_drvdata *ddat, long *value)
> +{
> +	u32 reg_val;
> +
> +	if (IS_DG2(gt_to_xe(ddat->gt))) {

if you revert this if() you save a level of indentation:

	if (!IS_DG2(gt_to_xe(ddat->gt)))
		return -EOPNOTSUPP;

The rest looks straight forward.

Andi

> +		process_hwmon_reg(ddat, 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;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +

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

[Badal Nilawar]

Expose hwmon energy attribute to show device level and gt
level energy usage
This is port from i915 hwmon.

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                 | 208 +++++++++++++++++-
 4 files changed, 224 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index 33a793b58157..f76f4c691946 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:		July 2023
+KernelVersion:	6.3
+Contact:	intel-gfx@lists.freedesktop.org
+Description:	RO. Energy input of device or tile 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_tileN") 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 06b4251f26fd..2faf0f43f2d5 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -22,6 +22,8 @@ enum hwm_reg_name {
 	pkg_power_sku,
 	pkg_power_sku_unit,
 	gt_perf_status,
+	energy_status_all,
+	energy_status_tile,
 };
 
 enum hwm_reg_operation {
@@ -35,10 +37,17 @@ enum hwm_reg_operation {
  * - 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 hwm_energy_info {
+	u32 reg_val_prev;
+	long accum_energy;		/* Accumulated energy for energy1_input */
+};
 
 struct hwm_drvdata {
 	struct xe_hwmon *hwmon;
@@ -47,12 +56,16 @@ struct hwm_drvdata {
 	char name[12];
 	bool reset_in_progress;
 	wait_queue_head_t waitq;
+	struct hwm_energy_info ei;	/*  Energy info for energy1_input */
+	int gt_n;
 };
 
 struct xe_hwmon {
 	struct hwm_drvdata ddat;
+	struct hwm_drvdata ddat_tile[XE_MAX_TILES_PER_DEVICE];
 	struct mutex hwmon_lock;
 	int scl_shift_power;
+	int scl_shift_energy;
 };
 
 struct xe_reg hwm_get_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name)
@@ -83,6 +96,18 @@ struct xe_reg hwm_get_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name)
 			return GT_PERF_STATUS;
 		else
 			return XE_REG(0);
+	case energy_status_all:
+		if (IS_DG2(gt_to_xe(ddat->gt)))
+			return PCU_CR_PACKAGE_ENERGY_STATUS;
+		else if (IS_PONTEVECCHIO(gt_to_xe(ddat->gt)))
+			return PVC_GT0_PLATFORM_ENERGY_STATUS;
+		else
+			return XE_REG(0);
+	case energy_status_tile:
+		if (IS_PONTEVECCHIO(gt_to_xe(ddat->gt)))
+			return PVC_GT0_PACKAGE_ENERGY_STATUS;
+		else
+			return XE_REG(0);
 	default:
 		break;
 	}
@@ -246,10 +271,69 @@ static int hwm_power_rated_max_read(struct hwm_drvdata *ddat, long *value)
 	return 0;
 }
 
+/*
+ * hwm_energy - 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
+hwm_energy(struct hwm_drvdata *ddat, long *energy)
+{
+	struct xe_hwmon *hwmon = ddat->hwmon;
+	struct hwm_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->gt_n >= 0)
+		process_hwmon_reg(ddat, energy_status_tile, reg_read,
+				  &reg_val, 0, 0);
+	else
+		process_hwmon_reg(ddat, 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 *hwm_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 *hwm_gt_info[] = {
+	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
 	NULL
 };
 
@@ -483,6 +567,36 @@ hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val)
 	return ret;
 }
 
+static umode_t
+hwm_energy_is_visible(struct hwm_drvdata *ddat, u32 attr)
+{
+	u32 reg_val;
+
+	switch (attr) {
+	case hwmon_energy_input:
+		if (ddat->gt_n >= 0)
+			return process_hwmon_reg(ddat, energy_status_tile, reg_read,
+						 &reg_val, 0, 0) ? 0 : 0444;
+		else
+			return process_hwmon_reg(ddat, energy_status_all, reg_read,
+						 &reg_val, 0, 0) ? 0 : 0444;
+	default:
+		return 0;
+	}
+}
+
+static int
+hwm_energy_read(struct hwm_drvdata *ddat, u32 attr, long *val)
+{
+	switch (attr) {
+	case hwmon_energy_input:
+		hwm_energy(ddat, val);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t
 hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	       u32 attr, int channel)
@@ -502,6 +616,9 @@ hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	case hwmon_in:
 		ret = hwm_in_is_visible(ddat, attr);
 		break;
+	case hwmon_energy:
+		ret = hwm_energy_is_visible(ddat, attr);
+		break;
 	default:
 		ret = 0;
 	}
@@ -530,6 +647,9 @@ hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_in:
 		ret = hwm_in_read(ddat, attr, val);
 		break;
+	case hwmon_energy:
+		ret = hwm_energy_read(ddat, attr, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -577,11 +697,53 @@ static const struct hwmon_chip_info hwm_chip_info = {
 	.info = hwm_info,
 };
 
+static umode_t
+hwm_gt_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+		  u32 attr, int channel)
+{
+	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
+
+	switch (type) {
+	case hwmon_energy:
+		return hwm_energy_is_visible(ddat, attr);
+	default:
+		return 0;
+	}
+}
+
+static int
+hwm_gt_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+	    int channel, long *val)
+{
+	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_energy:
+		return hwm_energy_read(ddat, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops hwm_gt_ops = {
+	.is_visible = hwm_gt_is_visible,
+	.read = hwm_gt_read,
+};
+
+static const struct hwmon_chip_info hwm_gt_chip_info = {
+	.ops = &hwm_gt_ops,
+	.info = hwm_gt_info,
+};
+
 static void
 hwm_get_preregistration_info(struct hwm_drvdata *ddat)
 {
 	struct xe_hwmon *hwmon = ddat->hwmon;
+	struct xe_device *xe = gt_to_xe(ddat->gt);
+	struct xe_gt *gt;
+	long energy;
 	u32 val_sku_unit = 0;
+	u8 id;
 	int ret;
 
 	ret = process_hwmon_reg(ddat, pkg_power_sku_unit, reg_read, &val_sku_unit, 0, 0);
@@ -589,8 +751,22 @@ hwm_get_preregistration_info(struct hwm_drvdata *ddat)
 	 * 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 hwm_energy_info', i.e. set fields to the
+	 * first value of the energy register read
+	 */
+	if (hwm_is_visible(ddat, hwmon_energy, hwmon_energy_input, 0))
+		hwm_energy(ddat, &energy);
+
+	for_each_gt(gt, xe, id)
+		if (hwm_gt_is_visible(&hwmon->ddat_tile[id], hwmon_energy,
+				      hwmon_energy_input, 0))
+			hwm_energy(&hwmon->ddat_tile[id], &energy);
 }
 
 void xe_hwmon_register(struct xe_device *xe)
@@ -599,6 +775,9 @@ void xe_hwmon_register(struct xe_device *xe)
 	struct xe_hwmon *hwmon;
 	struct device *hwmon_dev;
 	struct hwm_drvdata *ddat;
+	struct hwm_drvdata *ddat_tile;
+	struct xe_gt *gt;
+	u8 id;
 
 	/* hwmon is available only for dGfx */
 	if (!IS_DGFX(xe))
@@ -614,12 +793,22 @@ void xe_hwmon_register(struct xe_device *xe)
 
 	/* primary GT to access device level properties */
 	ddat->gt = xe->tiles[0].primary_gt;
+//	ddat->gt = &xe->gt[0];
+	ddat->gt_n = -1;
 
 	ddat->hwmon = hwmon;
 	snprintf(ddat->name, sizeof(ddat->name), "xe");
 
 	init_waitqueue_head(&ddat->waitq);
 
+	for_each_gt(gt, xe, id) {
+		ddat_tile = hwmon->ddat_tile + id;
+		ddat_tile->hwmon = hwmon;
+		ddat_tile->gt = gt;
+		snprintf(ddat_tile->name, sizeof(ddat_tile->name), "xe_tile%u", id);
+		ddat_tile->gt_n = id;
+	}
+
 	hwm_get_preregistration_info(ddat);
 
 	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
@@ -637,6 +826,23 @@ void xe_hwmon_register(struct xe_device *xe)
 	}
 
 	ddat->hwmon_dev = hwmon_dev;
+
+	for_each_gt(gt, xe, id) {
+		ddat_tile = hwmon->ddat_tile + id;
+		/*
+		 * Create per-gt directories only if a per-gt attribute is
+		 * visible. Currently this is only energy
+		 */
+		if (!hwm_gt_is_visible(ddat_tile, hwmon_energy, hwmon_energy_input, 0))
+			continue;
+
+		hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_tile->name,
+								 ddat_tile,
+								 &hwm_gt_chip_info,
+								 NULL);
+		if (!IS_ERR(hwmon_dev))
+			ddat_tile->hwmon_dev = hwmon_dev;
+	}
 }
 
 void xe_hwmon_unregister(struct xe_device *xe)
-- 
2.25.1

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

[Andi Shyti]

Hi Badal,

[...]

> @@ -614,12 +793,22 @@ void xe_hwmon_register(struct xe_device *xe)
>  
>  	/* primary GT to access device level properties */
>  	ddat->gt = xe->tiles[0].primary_gt;
> +//	ddat->gt = &xe->gt[0];

please clean it up before sending

> +	ddat->gt_n = -1;
>  
>  	ddat->hwmon = hwmon;
>  	snprintf(ddat->name, sizeof(ddat->name), "xe");
>  
>  	init_waitqueue_head(&ddat->waitq);
>  
> +	for_each_gt(gt, xe, id) {
> +		ddat_tile = hwmon->ddat_tile + id;
> +		ddat_tile->hwmon = hwmon;
> +		ddat_tile->gt = gt;
> +		snprintf(ddat_tile->name, sizeof(ddat_tile->name), "xe_tile%u", id);
> +		ddat_tile->gt_n = id;
> +	}
> +
>  	hwm_get_preregistration_info(ddat);
>  
>  	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
> @@ -637,6 +826,23 @@ void xe_hwmon_register(struct xe_device *xe)
>  	}
>  
>  	ddat->hwmon_dev = hwmon_dev;
> +
> +	for_each_gt(gt, xe, id) {

you could eventually make a for_each_hwmon_gt()

Andi

> +		ddat_tile = hwmon->ddat_tile + id;
> +		/*
> +		 * Create per-gt directories only if a per-gt attribute is
> +		 * visible. Currently this is only energy
> +		 */
> +		if (!hwm_gt_is_visible(ddat_tile, hwmon_energy, hwmon_energy_input, 0))
> +			continue;
> +
> +		hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat_tile->name,
> +								 ddat_tile,
> +								 &hwm_gt_chip_info,
> +								 NULL);
> +		if (!IS_ERR(hwmon_dev))
> +			ddat_tile->hwmon_dev = hwmon_dev;
> +	}

[PATCH v2 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))
This is port from i915 hwmon.

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                 | 140 +++++++++++++++++-
 3 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index f76f4c691946..4895b59f96c6 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 tile in microjoules.
 		for the gt.
 
 		Only supported for particular Intel xe graphics platforms.
+
+What:		/sys/devices/.../hwmon/hwmon<i>/power1_max_interval
+Date:		July 2023
+KernelVersion:	6.3
+Contact:	intel-gfx@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 i915 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 2faf0f43f2d5..b8a2e327a22b 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -43,6 +43,7 @@ enum hwm_reg_operation {
 #define SF_CURR		1000
 #define SF_VOLTAGE	1000
 #define SF_ENERGY	1000000
+#define SF_TIME		1000
 
 struct hwm_energy_info {
 	u32 reg_val_prev;
@@ -66,6 +67,7 @@ struct xe_hwmon {
 	struct mutex hwmon_lock;
 	int scl_shift_power;
 	int scl_shift_energy;
+	int scl_shift_time;
 };
 
 struct xe_reg hwm_get_reg(struct hwm_drvdata *ddat, enum hwm_reg_name reg_name)
@@ -324,6 +326,141 @@ hwm_energy(struct hwm_drvdata *ddat, long *energy)
 	xe_device_mem_access_put(gt_to_xe(ddat->gt));
 }
 
+static ssize_t
+hwm_power1_max_interval_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+	struct xe_hwmon *hwmon = ddat->hwmon;
+	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, 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
+hwm_power1_max_interval_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
+	struct xe_hwmon *hwmon = ddat->hwmon;
+	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 hwm_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, 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,
+			  hwm_power1_max_interval_show,
+			  hwm_power1_max_interval_store, 0);
+
+static struct attribute *hwm_attributes[] = {
+	&sensor_dev_attr_power1_max_interval.dev_attr.attr,
+	NULL
+};
+
+static umode_t hwm_attributes_visible(struct kobject *kobj,
+				      struct attribute *attr, int index)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct hwm_drvdata *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, 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 hwm_attrgroup = {
+	.attrs = hwm_attributes,
+	.is_visible = hwm_attributes_visible,
+};
+
+static const struct attribute_group *hwm_groups[] = {
+	&hwm_attrgroup,
+	NULL
+};
+
 static const struct hwmon_channel_info *hwm_info[] = {
 	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
 	HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
@@ -754,6 +891,7 @@ hwm_get_preregistration_info(struct hwm_drvdata *ddat)
 	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);
 	}
 
 	/*
@@ -817,7 +955,7 @@ void xe_hwmon_register(struct xe_device *xe)
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
 							 ddat,
 							 &hwm_chip_info,
-							 NULL);
+							 hwm_groups);
 
 	if (IS_ERR(hwmon_dev)) {
 		drm_warn(&xe->drm, "Fail to register xe hwmon\n");
-- 
2.25.1

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Dixit, Ashutosh]

On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
>

Hi Badal,

> This series adds the hwmon support on xe driver for DGFX

Needs some discussion but I have a general comment on this series
first. The implementation here follow what was done for i915. But how
"hwmon attributes are defined" I think we should look at how this was done
in other drm drivers, namely amdgpu and radeon. Look here (search for
"hwmon_attributes"):

drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
drivers/gpu/drm/radeon/radeon_pm.c

Here the hwmon attribute definition is very similar to how general sysfs
attributes are defined (they will just appear in hwmon directories) and
does not carry baggage of the hwmon infrastructure (what i915 has). So my
preference is to shift to this amd/radeon way for xe.

There is also a separate discussion on whether to use hwmon sysfs for other
custom attributes, as has been done in these other drm drivers, and using
this light-weight method should help if we went this route too.

Thanks.
--
Ashutosh

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Guenter Roeck]

On 7/1/23 18:31, Dixit, Ashutosh wrote:
> On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
>>
> 
> Hi Badal,
> 
>> This series adds the hwmon support on xe driver for DGFX
> 
> Needs some discussion but I have a general comment on this series
> first. The implementation here follow what was done for i915. But how
> "hwmon attributes are defined" I think we should look at how this was done
> in other drm drivers, namely amdgpu and radeon. Look here (search for
> "hwmon_attributes"):
> 
> drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
> drivers/gpu/drm/radeon/radeon_pm.c
> 
> Here the hwmon attribute definition is very similar to how general sysfs
> attributes are defined (they will just appear in hwmon directories) and
> does not carry baggage of the hwmon infrastructure (what i915 has). So my
> preference is to shift to this amd/radeon way for xe.
> 

You mean your preference is to use a deprecated hardware monitoring
registration function and to explicitly violate the following statement
from Documentation/hwmon/hwmon-kernel-api.rst ?

   All other hardware monitoring device registration functions are deprecated
   and must not be used in new drivers.

That is quite interesting. Please elaborate and explain your rationale.

Thanks,
Guenter

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Dixit, Ashutosh]

On Sat, 01 Jul 2023 20:02:51 -0700, Guenter Roeck wrote:
>
> On 7/1/23 18:31, Dixit, Ashutosh wrote:
> > On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
> >>
> >
> > Hi Badal,
> >
> >> This series adds the hwmon support on xe driver for DGFX
> >
> > Needs some discussion but I have a general comment on this series
> > first. The implementation here follow what was done for i915. But how
> > "hwmon attributes are defined" I think we should look at how this was done
> > in other drm drivers, namely amdgpu and radeon. Look here (search for
> > "hwmon_attributes"):
> >
> > drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
> > drivers/gpu/drm/radeon/radeon_pm.c
> >
> > Here the hwmon attribute definition is very similar to how general sysfs
> > attributes are defined (they will just appear in hwmon directories) and
> > does not carry baggage of the hwmon infrastructure (what i915 has). So my
> > preference is to shift to this amd/radeon way for xe.
> >
>
> You mean your preference is to use a deprecated hardware monitoring
> registration function and to explicitly violate the following statement
> from Documentation/hwmon/hwmon-kernel-api.rst ?
>
>   All other hardware monitoring device registration functions are deprecated
>   and must not be used in new drivers.

I missed that, but since we also have this in ddaefa209c4a ("hwmon: Make
chip parameter for with_info API mandatory"), yes that is what it would
boil down to.

> That is quite interesting. Please elaborate and explain your rationale.

Basically, like those other drm drivers, the chip parameter is of no use to
us (or at least we'd be totally fine not using it), hence the desire to
skip it.

But we are still required to use what we don't need? Do you care about
drivers outside drivers/hwmon?

Thanks.
--
Ashutosh

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Guenter Roeck]

On 7/2/23 08:57, Dixit, Ashutosh wrote:
> On Sat, 01 Jul 2023 20:02:51 -0700, Guenter Roeck wrote:
>>
>> On 7/1/23 18:31, Dixit, Ashutosh wrote:
>>> On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
>>>>
>>>
>>> Hi Badal,
>>>
>>>> This series adds the hwmon support on xe driver for DGFX
>>>
>>> Needs some discussion but I have a general comment on this series
>>> first. The implementation here follow what was done for i915. But how
>>> "hwmon attributes are defined" I think we should look at how this was done
>>> in other drm drivers, namely amdgpu and radeon. Look here (search for
>>> "hwmon_attributes"):
>>>
>>> drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
>>> drivers/gpu/drm/radeon/radeon_pm.c
>>>
>>> Here the hwmon attribute definition is very similar to how general sysfs
>>> attributes are defined (they will just appear in hwmon directories) and
>>> does not carry baggage of the hwmon infrastructure (what i915 has). So my
>>> preference is to shift to this amd/radeon way for xe.
>>>
>>
>> You mean your preference is to use a deprecated hardware monitoring
>> registration function and to explicitly violate the following statement
>> from Documentation/hwmon/hwmon-kernel-api.rst ?
>>
>>    All other hardware monitoring device registration functions are deprecated
>>    and must not be used in new drivers.
> 
> I missed that, but since we also have this in ddaefa209c4a ("hwmon: Make
> chip parameter for with_info API mandatory"), yes that is what it would
> boil down to.
> 

The chip parameter covers all standard hwmon sysfs attributes. A hwmon driver
without standard sysfs attributes is not a hwmon driver. It abuses the hwmon
subsystem and its API/ABI. If I catch such a driver, I'll NACK it. If I find
one in the kernel, I will do my best to get it removed.

>> That is quite interesting. Please elaborate and explain your rationale.
> 
> Basically, like those other drm drivers, the chip parameter is of no use to
> us (or at least we'd be totally fine not using it), hence the desire to
> skip it.
> 
> But we are still required to use what we don't need? Do you care about
> drivers outside drivers/hwmon?
> 

I would suggest to read the hwmon API more closely to understand it. Your claim
that "the chip parameter is of no use to us" is simply wrong, as should be obvious
when you read this submission. Actually, if you would convert the other
drm drivers to use it, it would reduce the size of the hwmon specific code
in those drivers, typically by 20-40%. Given that, I must admit that I am quite
baffled by your claim. Maybe you could explain that in more detail.

Of course, I care about use of the hardware monitoring subsystem
outside drivers/hwmon. Unlike other maintainers, I let people register drivers
from outside that directory, but that doesn't mean that I don't care.

FWIW, you are close to convincing me to add a warning message to the kernel
to tell users of deprecated hwmon APIs that the API is deprecated.
Alternatively, I may stop permitting new hwmon drivers outside drivers/hwmon.

Guenter

> Thanks.
> --
> Ashutosh

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Dixit, Ashutosh]

On Sun, 02 Jul 2023 10:01:00 -0700, Guenter Roeck wrote:
>
> On 7/2/23 08:57, Dixit, Ashutosh wrote:
> > On Sat, 01 Jul 2023 20:02:51 -0700, Guenter Roeck wrote:
> >>
> >> On 7/1/23 18:31, Dixit, Ashutosh wrote:
> >>> On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
> >>>>
> >>>
> >>> Hi Badal,
> >>>
> >>>> This series adds the hwmon support on xe driver for DGFX
> >>>
> >>> Needs some discussion but I have a general comment on this series
> >>> first. The implementation here follow what was done for i915. But how
> >>> "hwmon attributes are defined" I think we should look at how this was done
> >>> in other drm drivers, namely amdgpu and radeon. Look here (search for
> >>> "hwmon_attributes"):
> >>>
> >>> drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
> >>> drivers/gpu/drm/radeon/radeon_pm.c
> >>>
> >>> Here the hwmon attribute definition is very similar to how general sysfs
> >>> attributes are defined (they will just appear in hwmon directories) and
> >>> does not carry baggage of the hwmon infrastructure (what i915 has). So my
> >>> preference is to shift to this amd/radeon way for xe.
> >>>
> >>
> >> You mean your preference is to use a deprecated hardware monitoring
> >> registration function and to explicitly violate the following statement
> >> from Documentation/hwmon/hwmon-kernel-api.rst ?
> >>
> >>    All other hardware monitoring device registration functions are deprecated
> >>    and must not be used in new drivers.
> >
> > I missed that, but since we also have this in ddaefa209c4a ("hwmon: Make
> > chip parameter for with_info API mandatory"), yes that is what it would
> > boil down to.
> >
>
> The chip parameter covers all standard hwmon sysfs attributes. A hwmon driver
> without standard sysfs attributes is not a hwmon driver. It abuses the hwmon
> subsystem and its API/ABI.

To me, hwmon is a means to expose some standard attributes to standard
userspace apps so that those apps can find those attributes. What kernel
API's are used internally is an internal matter of the kernel. As subsytem
maintainer you may have reasons for allowing only certain API's.

> If I catch such a driver, I'll NACK it. If I find one in the kernel, I
> will do my best to get it removed.
>
> >> That is quite interesting. Please elaborate and explain your rationale.
> >
> > Basically, like those other drm drivers, the chip parameter is of no use to
> > us (or at least we'd be totally fine not using it), hence the desire to
> > skip it.
> >
> > But we are still required to use what we don't need? Do you care about
> > drivers outside drivers/hwmon?
> >
>
> I would suggest to read the hwmon API more closely to understand it. Your claim
> that "the chip parameter is of no use to us" is simply wrong, as should be obvious
> when you read this submission. Actually, if you would convert the other
> drm drivers to use it, it would reduce the size of the hwmon specific code
> in those drivers, typically by 20-40%. Given that, I must admit that I am quite
> baffled by your claim. Maybe you could explain that in more detail.

Of course when the chip parameter helps it likely reduces code. But when it
is not needed it adds unnecessary code. Those drm drivers
(amdgpu/radeon/i915) I mentioned above are available in the kernel, anyone
can see and judge for themselves.

Of course people might have been abusing the deprecated API's (or NULL chip
parameter) but to me it seems there is also some legitimate use for them.

> Of course, I care about use of the hardware monitoring subsystem
> outside drivers/hwmon. Unlike other maintainers, I let people register drivers
> from outside that directory, but that doesn't mean that I don't care.
>
> FWIW, you are close to convincing me to add a warning message to the kernel
> to tell users of deprecated hwmon APIs that the API is deprecated.
> Alternatively, I may stop permitting new hwmon drivers outside drivers/hwmon.

OK, thanks for clarifying, since you disagree we will not use deprecated
API's, so we will continue with the approach taken in this series.

Ashutosh

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Guenter Roeck]

On 7/2/23 13:29, Dixit, Ashutosh wrote:

> Of course people might have been abusing the deprecated API's (or NULL chip
> parameter) but to me it seems there is also some legitimate use for them.
> 

You still neglect to explain what you think that legitimate use would be.

Guenter

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Dixit, Ashutosh]

On Sun, 02 Jul 2023 13:51:40 -0700, Guenter Roeck wrote:
>
> On 7/2/23 13:29, Dixit, Ashutosh wrote:
>
> > Of course people might have been abusing the deprecated API's (or NULL chip
> > parameter) but to me it seems there is also some legitimate use for them.
> >
>
> You still neglect to explain what you think that legitimate use would be.

To me "drivers/gpu/drm/amd/pm/amdgpu_pm.c" is a legitimate use case which
doesn't use chip_info (both standard and custom hwmon attributes are
defined without using chip_info). "drivers/gpu/drm/i915/i915_hwmon.c" has
all this extra code related to chip_info/channel_info which is not
needed. i915 could well move to the amdgpu model and that would reduce i915
code. That is what I was originally proposing for this new patch series.

Ashutosh

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Guenter Roeck]

On 7/2/23 18:48, Dixit, Ashutosh wrote:
> On Sun, 02 Jul 2023 13:51:40 -0700, Guenter Roeck wrote:
>>
>> On 7/2/23 13:29, Dixit, Ashutosh wrote:
>>
>>> Of course people might have been abusing the deprecated API's (or NULL chip
>>> parameter) but to me it seems there is also some legitimate use for them.
>>>
>>
>> You still neglect to explain what you think that legitimate use would be.
> 
> To me "drivers/gpu/drm/amd/pm/amdgpu_pm.c" is a legitimate use case which
> doesn't use chip_info (both standard and custom hwmon attributes are
> defined without using chip_info). "drivers/gpu/drm/i915/i915_hwmon.c" has

In new code, standard hwmon attributes MUST be defined using chip_info.
Declaring the use of a deprecated API as "legitimate use case" and use it
as example for new code is never appropriate.

> all this extra code related to chip_info/channel_info which is not
> needed. i915 could well move to the amdgpu model and that would reduce i915

Yes, and the proposed i915 code _doesn't_ have all the extra code that would
otherwise be needed to generate and read/write sysfs attributes directly.

> code. That is what I was originally proposing for this new patch series.
> 

This is wrong. Using chip_info _always_ reduces code size for standard
hwmon attributes, because the code can concentrate on reading and
writing values from/to the chip and doesn't have to bother with sysfs
attribute handling. Convert drivers/gpu/drm/amd/pm/amdgpu_pm.c to use
the with_info API and you'll see.

Guenter

Re: [Intel-xe] [PATCH v2 0/6] Add HWMON support for DGFX

[Rodrigo Vivi]

On Sun, Jul 02, 2023 at 10:01:00AM -0700, Guenter Roeck wrote:
> On 7/2/23 08:57, Dixit, Ashutosh wrote:
> > On Sat, 01 Jul 2023 20:02:51 -0700, Guenter Roeck wrote:
> > > 
> > > On 7/1/23 18:31, Dixit, Ashutosh wrote:
> > > > On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
> > > > > 
> > > > 
> > > > Hi Badal,
> > > > 
> > > > > This series adds the hwmon support on xe driver for DGFX
> > > > 
> > > > Needs some discussion but I have a general comment on this series
> > > > first. The implementation here follow what was done for i915. But how
> > > > "hwmon attributes are defined" I think we should look at how this was done
> > > > in other drm drivers, namely amdgpu and radeon. Look here (search for
> > > > "hwmon_attributes"):
> > > > 
> > > > drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
> > > > drivers/gpu/drm/radeon/radeon_pm.c
> > > > 
> > > > Here the hwmon attribute definition is very similar to how general sysfs
> > > > attributes are defined (they will just appear in hwmon directories) and
> > > > does not carry baggage of the hwmon infrastructure (what i915 has). So my
> > > > preference is to shift to this amd/radeon way for xe.
> > > > 
> > > 
> > > You mean your preference is to use a deprecated hardware monitoring
> > > registration function and to explicitly violate the following statement
> > > from Documentation/hwmon/hwmon-kernel-api.rst ?
> > > 
> > >    All other hardware monitoring device registration functions are deprecated
> > >    and must not be used in new drivers.
> > 
> > I missed that, but since we also have this in ddaefa209c4a ("hwmon: Make
> > chip parameter for with_info API mandatory"), yes that is what it would
> > boil down to.
> > 
> 
> The chip parameter covers all standard hwmon sysfs attributes. A hwmon driver
> without standard sysfs attributes is not a hwmon driver. It abuses the hwmon
> subsystem and its API/ABI. If I catch such a driver, I'll NACK it. If I find
> one in the kernel, I will do my best to get it removed.
> 
> > > That is quite interesting. Please elaborate and explain your rationale.
> > 
> > Basically, like those other drm drivers, the chip parameter is of no use to
> > us (or at least we'd be totally fine not using it), hence the desire to
> > skip it.
> > 
> > But we are still required to use what we don't need? Do you care about
> > drivers outside drivers/hwmon?
> > 
> 
> I would suggest to read the hwmon API more closely to understand it. Your claim
> that "the chip parameter is of no use to us" is simply wrong, as should be obvious
> when you read this submission. Actually, if you would convert the other
> drm drivers to use it, it would reduce the size of the hwmon specific code
> in those drivers, typically by 20-40%. Given that, I must admit that I am quite
> baffled by your claim. Maybe you could explain that in more detail.
> 
> Of course, I care about use of the hardware monitoring subsystem
> outside drivers/hwmon. Unlike other maintainers, I let people register drivers
> from outside that directory, but that doesn't mean that I don't care.

Hi Guenter,

First of all sorry for jumping late here. I'm totally with you here and we should
definitely only use the new API. For standard entries that will definitely
reduce the code size.

So, since we are talking about reducing code here, and looking to other DRM
drivers, and thinking about the needs on this new Xe driver, I'm wondering
if you would consider accepting 'frequency' as a standard hwmon attribute.

We would need it to be RW so we could use to put freq requests as well,
and possibly different types/domains and even throttle reasons on top.

So we could then try to unify all the drm drivers in a common drm-hwmon
layer putting an end in all abuses and deprecated users.

But before moving fwd with any proposal I'd like to hear your thoughts on
this 'frequency' block as standard attribute.

Thanks,
Rodrigo.

> 
> FWIW, you are close to convincing me to add a warning message to the kernel
> to tell users of deprecated hwmon APIs that the API is deprecated.
> Alternatively, I may stop permitting new hwmon drivers outside drivers/hwmon.
> 
> Guenter
> 
> > Thanks.
> > --
> > Ashutosh
> 

Re: [Intel-xe] [PATCH v2 0/6] Add HWMON support for DGFX

[Guenter Roeck]

On 7/14/23 13:21, Rodrigo Vivi wrote:
[ ... ]

> Hi Guenter,
> 
> First of all sorry for jumping late here. I'm totally with you here and we should
> definitely only use the new API. For standard entries that will definitely
> reduce the code size.
> 
> So, since we are talking about reducing code here, and looking to other DRM
> drivers, and thinking about the needs on this new Xe driver, I'm wondering
> if you would consider accepting 'frequency' as a standard hwmon attribute.
> 
> We would need it to be RW so we could use to put freq requests as well,
> and possibly different types/domains and even throttle reasons on top.
> 
> So we could then try to unify all the drm drivers in a common drm-hwmon
> layer putting an end in all abuses and deprecated users.
> 
> But before moving fwd with any proposal I'd like to hear your thoughts on
> this 'frequency' block as standard attribute.
> 

I really don't see how this would fit under "hardware monitoring".
Making it writable would be even worse - this is most definitely not a limit but
an actual value. The notion of limit actually shows that it is not a good fit as
a monitoring attribute: I can not conceive the notion of a "maximum" or "minimum"
frequency limit, or an "under" or "over" frequency.

If this is about thermal control/management, you might want to consider registering
with devfreq and the thermal subsystem (see devfreq_cooling_register() and
friends for reference).

Thanks,
Guenter

Re: [Intel-xe] [PATCH v2 0/6] Add HWMON support for DGFX

[Rodrigo Vivi]

On Fri, Jul 14, 2023 at 03:26:49PM -0700, Guenter Roeck wrote:
> On 7/14/23 13:21, Rodrigo Vivi wrote:
> [ ... ]
> 
> > Hi Guenter,
> > 
> > First of all sorry for jumping late here. I'm totally with you here and we should
> > definitely only use the new API. For standard entries that will definitely
> > reduce the code size.
> > 
> > So, since we are talking about reducing code here, and looking to other DRM
> > drivers, and thinking about the needs on this new Xe driver, I'm wondering
> > if you would consider accepting 'frequency' as a standard hwmon attribute.
> > 
> > We would need it to be RW so we could use to put freq requests as well,
> > and possibly different types/domains and even throttle reasons on top.
> > 
> > So we could then try to unify all the drm drivers in a common drm-hwmon
> > layer putting an end in all abuses and deprecated users.
> > 
> > But before moving fwd with any proposal I'd like to hear your thoughts on
> > this 'frequency' block as standard attribute.
> > 
> 
> I really don't see how this would fit under "hardware monitoring".
> Making it writable would be even worse - this is most definitely not a limit but
> an actual value. The notion of limit actually shows that it is not a good fit as
> a monitoring attribute: I can not conceive the notion of a "maximum" or "minimum"
> frequency limit, or an "under" or "over" frequency.

how's that different from the voltage/pwm/current/etc min, max, critical RW limits
already existent?

> 
> If this is about thermal control/management, you might want to consider registering
> with devfreq and the thermal subsystem (see devfreq_cooling_register() and
> friends for reference).

yeap, it looks like devfreq is a good candidate for the unification. It is just
sad that it is not as robust and flexible as hwmon infrastructure.

> 
> Thanks,
> Guenter
> 

Re: [PATCH v2 0/6] Add HWMON support for DGFX

[Andi Shyti]

Hi,

On Sat, Jul 01, 2023 at 08:02:51PM -0700, Guenter Roeck wrote:
> On 7/1/23 18:31, Dixit, Ashutosh wrote:
> > On Tue, 27 Jun 2023 11:30:37 -0700, Badal Nilawar wrote:
> > > 
> > 
> > Hi Badal,
> > 
> > > This series adds the hwmon support on xe driver for DGFX
> > 
> > Needs some discussion but I have a general comment on this series
> > first. The implementation here follow what was done for i915. But how
> > "hwmon attributes are defined" I think we should look at how this was done
> > in other drm drivers, namely amdgpu and radeon. Look here (search for
> > "hwmon_attributes"):
> > 
> > drivers/gpu/drm/amd/pm/amdgpu_pm.c, and
> > drivers/gpu/drm/radeon/radeon_pm.c
> > 
> > Here the hwmon attribute definition is very similar to how general sysfs
> > attributes are defined (they will just appear in hwmon directories) and
> > does not carry baggage of the hwmon infrastructure (what i915 has). So my
> > preference is to shift to this amd/radeon way for xe.
> > 
> 
> You mean your preference is to use a deprecated hardware monitoring
> registration function and to explicitly violate the following statement
> from Documentation/hwmon/hwmon-kernel-api.rst ?
> 
>   All other hardware monitoring device registration functions are deprecated
>   and must not be used in new drivers.
> 
> That is quite interesting. Please elaborate and explain your rationale.

how about using iio instead of hwmon?

Andi