On Fri, 19 Aug 2022, Badal Nilawar <badal.nilawar@xxxxxxxxx> wrote:
> From: Dale B Stimson <dale.b.stimson@xxxxxxxxx>
>
> The i915 HWMON module will be used to expose voltage, power and energy
> values for dGfx. Here we set up i915 hwmon infrastructure including i915
> hwmon registration, basic data structures and functions.
>
> v2:
> - Create HWMON infra patch (Ashutosh)
> - Fixed review comments (Jani)
> - Remove "select HWMON" from i915/Kconfig (Jani)
> v3: Use hwm_ prefix for static functions (Ashutosh)
> v4: s/#ifdef CONFIG_HWMON/#if IS_REACHABLE(CONFIG_HWMON)/ since the former
> doesn't work if hwmon is compiled as a module (Guenter)
Is this really what we want to do?
In my books, it's a misconfiguration to have CONFIG_HWMON=m with
CONFIG_DRM_I915=y. That's really the problematic combo, not just
CONFIG_HWMON=m, right? Why do we allow it at the kconfig level, and then
have ugly hacks around it at the code level? Especially as
CONFIG_DRM_I915=y should really be thought of as a corner case.
So why not do this in i915 Kconfig:
config DRM_I915
...
depends on HWMON || HWMON=n
Which rejects the CONFIG_HWMON=m && CONFIG_DRM_I915=y combo.
>
> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> Signed-off-by: Dale B Stimson <dale.b.stimson@xxxxxxxxx>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> Signed-off-by: Riana Tauro <riana.tauro@xxxxxxxxx>
> Signed-off-by: Badal Nilawar <badal.nilawar@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 3 +
> drivers/gpu/drm/i915/i915_driver.c | 7 ++
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> drivers/gpu/drm/i915/i915_hwmon.c | 135 +++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_hwmon.h | 20 +++++
> 5 files changed, 167 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/i915_hwmon.c
> create mode 100644 drivers/gpu/drm/i915/i915_hwmon.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 522ef9b4aff3..2b235f747490 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -208,6 +208,9 @@ i915-y += gt/uc/intel_uc.o \
> # graphics system controller (GSC) support
> i915-y += gt/intel_gsc.o
>
> +# graphics hardware monitoring (HWMON) support
> +i915-$(CONFIG_HWMON) += i915_hwmon.o
Moreover, this builds i915_hwmon.o as part of i915.ko (or kernel as it's
builtin) even if we can't use it!
BR,
Jani.
> +
> # modesetting core code
> i915-y += \
> display/hsw_ips.o \
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index deb8a8b76965..62340cd01dde 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -80,6 +80,7 @@
> #include "i915_drm_client.h"
> #include "i915_drv.h"
> #include "i915_getparam.h"
> +#include "i915_hwmon.h"
> #include "i915_ioc32.h"
> #include "i915_ioctl.h"
> #include "i915_irq.h"
> @@ -736,6 +737,9 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>
> intel_gt_driver_register(to_gt(dev_priv));
>
> +#if IS_REACHABLE(CONFIG_HWMON)
> + i915_hwmon_register(dev_priv);
> +#endif
> intel_display_driver_register(dev_priv);
>
> intel_power_domains_enable(dev_priv);
> @@ -762,6 +766,9 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>
> intel_display_driver_unregister(dev_priv);
>
> +#if IS_REACHABLE(CONFIG_HWMON)
> + i915_hwmon_unregister(dev_priv);
> +#endif
> intel_gt_driver_unregister(to_gt(dev_priv));
>
> i915_perf_unregister(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 086bbe8945d6..d437d588dec9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -705,6 +705,8 @@ struct drm_i915_private {
>
> struct i915_perf perf;
>
> + struct i915_hwmon *hwmon;
> +
> /* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
> struct intel_gt gt0;
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> new file mode 100644
> index 000000000000..5b80a0f024f0
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/types.h>
> +
> +#include "i915_drv.h"
> +#include "i915_hwmon.h"
> +#include "intel_mchbar_regs.h"
> +
> +struct hwm_reg {
> +};
> +
> +struct hwm_drvdata {
> + struct i915_hwmon *hwmon;
> + struct intel_uncore *uncore;
> + struct device *hwmon_dev;
> + char name[12];
> +};
> +
> +struct i915_hwmon {
> + struct hwm_drvdata ddat;
> + struct mutex hwmon_lock; /* counter overflow logic and rmw */
> + struct hwm_reg rg;
> +};
> +
> +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 drm_i915_private *i915)
> +{
> +}
> +
> +void i915_hwmon_register(struct drm_i915_private *i915)
> +{
> + struct device *dev = i915->drm.dev;
> + struct i915_hwmon *hwmon;
> + struct device *hwmon_dev;
> + struct hwm_drvdata *ddat;
> +
> + /* hwmon is available only for dGfx */
> + if (!IS_DGFX(i915))
> + return;
> +
> + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return;
> +
> + i915->hwmon = hwmon;
> + mutex_init(&hwmon->hwmon_lock);
> + ddat = &hwmon->ddat;
> +
> + ddat->hwmon = hwmon;
> + ddat->uncore = &i915->uncore;
> + snprintf(ddat->name, sizeof(ddat->name), "i915");
> +
> + hwm_get_preregistration_info(i915);
> +
> + /* hwmon_dev points to device hwmon<i> */
> + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name,
> + ddat,
> + &hwm_chip_info,
> + NULL);
> + if (IS_ERR(hwmon_dev)) {
> + mutex_destroy(&hwmon->hwmon_lock);
> + i915->hwmon = NULL;
> + kfree(hwmon);
> + return;
> + }
> +
> + ddat->hwmon_dev = hwmon_dev;
> +}
> +
> +void i915_hwmon_unregister(struct drm_i915_private *i915)
> +{
> + struct i915_hwmon *hwmon;
> + struct hwm_drvdata *ddat;
> +
> + hwmon = fetch_and_zero(&i915->hwmon);
> + if (!hwmon)
> + return;
> +
> + ddat = &hwmon->ddat;
> + if (ddat->hwmon_dev)
> + hwmon_device_unregister(ddat->hwmon_dev);
> +
> + mutex_destroy(&hwmon->hwmon_lock);
> + kfree(hwmon);
> +}
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h
> new file mode 100644
> index 000000000000..921ae76099d3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_hwmon.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#ifndef __I915_HWMON_H__
> +#define __I915_HWMON_H__
> +
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +#include "i915_reg.h"
> +
> +struct drm_i915_private;
> +
> +void i915_hwmon_register(struct drm_i915_private *i915);
> +void i915_hwmon_unregister(struct drm_i915_private *i915);
> +
> +#endif /* __I915_HWMON_H__ */
--
Jani Nikula, Intel Open Source Graphics Center
