[Intel-xe] [PATCH v3 1/6] drm/xe/hwmon: Add HWMON infrastructure
Guenter Roeck
linux at roeck-us.net
Wed Aug 2 14:15:03 UTC 2023
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 at intel.com>
> ---
> drivers/gpu/drm/xe/Makefile | 3 +
> drivers/gpu/drm/xe/xe_device.c | 5 +
> drivers/gpu/drm/xe/xe_device_types.h | 2 +
> drivers/gpu/drm/xe/xe_hwmon.c | 150 +++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_hwmon.h | 22 ++++
> 5 files changed, 182 insertions(+)
> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 4ea9e3150c20..831be23e000b 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -116,6 +116,9 @@ xe-y += xe_bb.o \
> xe_wa.o \
> xe_wopcm.o
>
> +# graphics hardware monitoring (HWMON) support
> +xe-$(CONFIG_HWMON) += xe_hwmon.o
> +
> # i915 Display compat #defines and #includes
> subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
> -I$(srctree)/$(src)/display/ext \
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 5409cf7895d3..01bd08812514 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -34,6 +34,7 @@
> #include "xe_vm.h"
> #include "xe_vm_madvise.h"
> #include "xe_wait_user_fence.h"
> +#include "xe_hwmon.h"
>
> #ifdef CONFIG_LOCKDEP
> struct lockdep_map xe_device_mem_access_lockdep_map = {
> @@ -335,6 +336,8 @@ int xe_device_probe(struct xe_device *xe)
>
> xe_debugfs_register(xe);
>
> + xe_hwmon_register(xe);
> +
> err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
> if (err)
> return err;
> @@ -361,6 +364,8 @@ static void xe_device_remove_display(struct xe_device *xe)
>
> void xe_device_remove(struct xe_device *xe)
> {
> + xe_hwmon_unregister(xe);
> +
> xe_device_remove_display(xe);
>
> xe_display_unlink(xe);
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index b156f69d7320..dd06eba815ec 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -376,6 +376,8 @@ struct xe_device {
> */
> struct task_struct *pm_callback_task;
>
> + struct xe_hwmon *hwmon;
> +
> /* private: */
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> new file mode 100644
> index 000000000000..5e35128a61a8
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/hwmon.h>
> +
> +#include <drm/drm_managed.h>
> +#include "regs/xe_gt_regs.h"
> +#include "xe_device.h"
> +#include "xe_hwmon.h"
> +
> +struct xe_hwmon_data {
> + struct device *hwmon_dev;
> + struct xe_gt *gt;
> + char name[12];
> +};
> +
> +struct xe_hwmon {
> + struct xe_hwmon_data ddat;
> + struct mutex hwmon_lock;
> +};
> +
> +static const struct hwmon_channel_info *hwmon_info[] = {
> + NULL
> +};
> +
> +static umode_t
> +hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + struct xe_hwmon_data *ddat = (struct xe_hwmon_data *)drvdata;
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> + switch (type) {
> + default:
> + ret = 0;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> + return ret;
> +}
> +
> +static int
> +hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> + switch (type) {
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> + return ret;
> +}
> +
> +static int
> +hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long val)
> +{
> + struct xe_hwmon_data *ddat = dev_get_drvdata(dev);
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(ddat->gt));
> +
> + switch (type) {
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(ddat->gt));
> +
> + return ret;
> +}
> +
> +static const struct hwmon_ops hwmon_ops = {
> + .is_visible = hwmon_is_visible,
> + .read = hwmon_read,
> + .write = hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info hwmon_chip_info = {
> + .ops = &hwmon_ops,
> + .info = hwmon_info,
> +};
> +
> +static void
> +hwmon_get_preregistration_info(struct xe_device *xe)
> +{
> +}
> +
> +void xe_hwmon_register(struct xe_device *xe)
> +{
> + struct device *dev = xe->drm.dev;
> + struct xe_hwmon *hwmon;
> + struct device *hwmon_dev;
> + struct xe_hwmon_data *ddat;
> +
> + /* hwmon is available only for dGfx */
> + if (!IS_DGFX(xe))
> + return;
> +
> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return;
> +
> + xe->hwmon = hwmon;
> + drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
> +
> + ddat = &hwmon->ddat;
> +
> + /* primary GT to access device level properties */
> + ddat->gt = xe->tiles[0].primary_gt;
> +
> + snprintf(ddat->name, sizeof(ddat->name), "xe");
Why not just pass "xe" as string to devm_hwmon_device_register_with_info() ?
> +
> + hwmon_get_preregistration_info(xe);
> +
> + drm_dbg(&xe->drm, "Register xe hwmon interface\n");
> +
> + /* hwmon_dev points to device hwmon<i> */
> + hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
> + ddat,
> + &hwmon_chip_info,
> + NULL);
> + if (IS_ERR(hwmon_dev)) {
> + drm_warn(&xe->drm, "Fail to register xe hwmon, Err:%ld\n", PTR_ERR(hwmon_dev));
> + xe->hwmon = NULL;
> + return;
> + }
> +
What is xe->hwmon used for other than for setting it to NULL
in xe_hwmon_unregister() ?
> + ddat->hwmon_dev = hwmon_dev;
> +}
> +
> +void xe_hwmon_unregister(struct xe_device *xe)
> +{
> + xe->hwmon = NULL;
> +}
Not sure I understand why this function is needed. Please explain.
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
> new file mode 100644
> index 000000000000..a078eeb0a68b
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __XE_HWMON_H__
> +#define __XE_HWMON_H__
> +
> +#include <linux/types.h>
> +
> +struct xe_device;
> +
> +#if IS_REACHABLE(CONFIG_HWMON)
> +void xe_hwmon_register(struct xe_device *xe);
> +void xe_hwmon_unregister(struct xe_device *xe);
> +#else
> +static inline void xe_hwmon_register(struct xe_device *xe) { };
> +static inline void xe_hwmon_unregister(struct xe_device *xe) { };
> +#endif
> +
> +#endif /* __XE_HWMON_H__ */
More information about the Intel-xe
mailing list