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

Guenter Roeck <linux@xxxxxxxxxxxx> · Wed, 2 Aug 2023 07:15:03 -0700

On 8/2/23 06:52, Badal Nilawar wrote:
The xe HWMON module will be used to expose voltage, power and energy
values for dGfx. Here we set up xe hwmon infrastructure including xe
hwmon registration, basic data structures and functions.

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

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

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

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

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

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

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

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

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







