[1/7] drm/i915/hwmon: Add HWMON infrastructure

Commit Message

Nilawar, Badal Aug. 18, 2022, 7:38 p.m. UTC

From: Dale B Stimson <[e-mail]>

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)

Cc: Guenter Roeck <[e-mail]>
Signed-off-by: Dale B Stimson <[e-mail]>
Signed-off-by: Ashutosh Dixit <[e-mail]>
Signed-off-by: Riana Tauro <[e-mail]>
Signed-off-by: Badal Nilawar <[e-mail]>
---
 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

Comments

Jani Nikula Aug. 19, 2022, 10:35 a.m. UTC | #1

On Fri, 19 Aug 2022, Badal Nilawar <[e-mail]> wrote:
> From: Dale B Stimson <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Dale B Stimson <[e-mail]>
> Signed-off-by: Ashutosh Dixit <[e-mail]>
> Signed-off-by: Riana Tauro <[e-mail]>
> Signed-off-by: Badal Nilawar <[e-mail]>
> ---
>  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 Aug. 19, 2022, 10:37 a.m. UTC | #2

On Fri, 19 Aug 2022, Badal Nilawar <[e-mail]> wrote:
> 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"

All of the above #includes are unnecessary. Please remove.

BR,
Jani.

> +
> +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__ */

Guenter Roeck Aug. 19, 2022, 11:41 a.m. UTC | #3

On Fri, Aug 19, 2022 at 01:35:52PM +0300, Jani Nikula wrote:
> On Fri, 19 Aug 2022, Badal Nilawar <[e-mail]> wrote:
> > From: Dale B Stimson <[e-mail]>
> >
> > 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
> 

Ok with me, but not my call to make. The ifdef should then use
IS_ENABLED(), though.

Guenter

> Which rejects the CONFIG_HWMON=m && CONFIG_DRM_I915=y combo.
> 
> >
> > Cc: Guenter Roeck <[e-mail]>
> > Signed-off-by: Dale B Stimson <[e-mail]>
> > Signed-off-by: Ashutosh Dixit <[e-mail]>
> > Signed-off-by: Riana Tauro <[e-mail]>
> > Signed-off-by: Badal Nilawar <[e-mail]>
> > ---
> >  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

Nilawar, Badal Aug. 23, 2022, 8:42 a.m. UTC | #4

On 19-08-2022 16:05, Jani Nikula wrote:
> On Fri, 19 Aug 2022, Badal Nilawar <[e-mail]> wrote:
>> From: Dale B Stimson <[e-mail]>
>>
>> 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
With this change I am getting recursive dependancy error when I run make 
oldconfig

badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
   HOSTCC  scripts/basic/fixdep
   HOSTCC  scripts/kconfig/conf.o
   HOSTCC  scripts/kconfig/confdata.o
   HOSTCC  scripts/kconfig/expr.o
   LEX     scripts/kconfig/lexer.lex.c
   YACC    scripts/kconfig/parser.tab.[ch]
   HOSTCC  scripts/kconfig/lexer.lex.o
   HOSTCC  scripts/kconfig/menu.o
   HOSTCC  scripts/kconfig/parser.tab.o
   HOSTCC  scripts/kconfig/preprocess.o
   HOSTCC  scripts/kconfig/symbol.o
   HOSTCC  scripts/kconfig/util.o
   HOSTLD  scripts/kconfig/conf
drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
drivers/platform/x86/Kconfig:332:       symbol EEEPC_LAPTOP depends on INPUT
drivers/input/Kconfig:8:        symbol INPUT is selected by DRM_I915
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"

make[1]: *** [scripts/kconfig/Makefile:77: oldconfig] Error 1
make: *** [Makefile:632: oldconfig] Error 2


> 
> Which rejects the CONFIG_HWMON=m && CONFIG_DRM_I915=y combo.
> 
>>
>> Cc: Guenter Roeck <[e-mail]>
>> Signed-off-by: Dale B Stimson <[e-mail]>
>> Signed-off-by: Ashutosh Dixit <[e-mail]>
>> Signed-off-by: Riana Tauro <[e-mail]>
>> Signed-off-by: Badal Nilawar <[e-mail]>
>> ---
>>   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!
For CONFIG_HWMON=m && CONFIG_DRM_I915=y combo i915_hwmon.o didn't get 
build. It is only getting build for below combos
CONFIG_HWMON=m && CONFIG_DRM_I915=y
CONFIG_HWMON=m && CONFIG_DRM_I915=m
CONFIG_HWMON=y && CONFIG_DRM_I915=m

Regards,
Badal
> 
> 
> 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 Aug. 23, 2022, 9:46 a.m. UTC | #5

On Tue, 23 Aug 2022, "Nilawar, Badal" <[e-mail]> wrote:
> On 19-08-2022 16:05, Jani Nikula wrote:
>> On Fri, 19 Aug 2022, Badal Nilawar <[e-mail]> wrote:
>>> From: Dale B Stimson <[e-mail]>
>>>
>>> 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
> With this change I am getting recursive dependancy error when I run make 
> oldconfig
>
> badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
>    HOSTCC  scripts/basic/fixdep
>    HOSTCC  scripts/kconfig/conf.o
>    HOSTCC  scripts/kconfig/confdata.o
>    HOSTCC  scripts/kconfig/expr.o
>    LEX     scripts/kconfig/lexer.lex.c
>    YACC    scripts/kconfig/parser.tab.[ch]
>    HOSTCC  scripts/kconfig/lexer.lex.o
>    HOSTCC  scripts/kconfig/menu.o
>    HOSTCC  scripts/kconfig/parser.tab.o
>    HOSTCC  scripts/kconfig/preprocess.o
>    HOSTCC  scripts/kconfig/symbol.o
>    HOSTCC  scripts/kconfig/util.o
>    HOSTLD  scripts/kconfig/conf
> drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
> drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
> drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
> drivers/platform/x86/Kconfig:332:       symbol EEEPC_LAPTOP depends on INPUT
> drivers/input/Kconfig:8:        symbol INPUT is selected by DRM_I915
> For a resolution refer to Documentation/kbuild/kconfig-language.rst
> subsection "Kconfig recursive dependency limitations"

*sigh*

  Note:
	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
	In general use select only for non-visible symbols
	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.

One day someone's going to need to fix menuconfig to first start
complaining about selecting stuff that shouldn't be selected, and then
eventually refusing to select stuff that shouldn't be selected. This is
an endless whack-a-mole, preventing people from adding reasonable
dependencies.

BR,
Jani.


>
> make[1]: *** [scripts/kconfig/Makefile:77: oldconfig] Error 1
> make: *** [Makefile:632: oldconfig] Error 2
>
>
>> 
>> Which rejects the CONFIG_HWMON=m && CONFIG_DRM_I915=y combo.
>> 
>>>
>>> Cc: Guenter Roeck <[e-mail]>
>>> Signed-off-by: Dale B Stimson <[e-mail]>
>>> Signed-off-by: Ashutosh Dixit <[e-mail]>
>>> Signed-off-by: Riana Tauro <[e-mail]>
>>> Signed-off-by: Badal Nilawar <[e-mail]>
>>> ---
>>>   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!
> For CONFIG_HWMON=m && CONFIG_DRM_I915=y combo i915_hwmon.o didn't get 
> build. It is only getting build for below combos
> CONFIG_HWMON=m && CONFIG_DRM_I915=y
> CONFIG_HWMON=m && CONFIG_DRM_I915=m
> CONFIG_HWMON=y && CONFIG_DRM_I915=m
>
> Regards,
> Badal
>> 
>> 
>> 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__ */
>>

Guenter Roeck Aug. 23, 2022, 12:19 p.m. UTC | #6

On Tue, Aug 23, 2022 at 12:46:14PM +0300, Jani Nikula wrote:
[ ... ]
> >> 
> >> So why not do this in i915 Kconfig:
> >> 
> >> config DRM_I915
> >> 	...
> >> 	depends on HWMON || HWMON=n
> > With this change I am getting recursive dependancy error when I run make 
> > oldconfig
> >
> > badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
> >    HOSTCC  scripts/basic/fixdep
> >    HOSTCC  scripts/kconfig/conf.o
> >    HOSTCC  scripts/kconfig/confdata.o
> >    HOSTCC  scripts/kconfig/expr.o
> >    LEX     scripts/kconfig/lexer.lex.c
> >    YACC    scripts/kconfig/parser.tab.[ch]
> >    HOSTCC  scripts/kconfig/lexer.lex.o
> >    HOSTCC  scripts/kconfig/menu.o
> >    HOSTCC  scripts/kconfig/parser.tab.o
> >    HOSTCC  scripts/kconfig/preprocess.o
> >    HOSTCC  scripts/kconfig/symbol.o
> >    HOSTCC  scripts/kconfig/util.o
> >    HOSTLD  scripts/kconfig/conf
> > drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
> > drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
> > drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
> > drivers/platform/x86/Kconfig:332:       symbol EEEPC_LAPTOP depends on INPUT
> > drivers/input/Kconfig:8:        symbol INPUT is selected by DRM_I915
> > For a resolution refer to Documentation/kbuild/kconfig-language.rst
> > subsection "Kconfig recursive dependency limitations"
> 
> *sigh*
> 
>   Note:
> 	select should be used with care. select will force
> 	a symbol to a value without visiting the dependencies.
> 	By abusing select you are able to select a symbol FOO even
> 	if FOO depends on BAR that is not set.
> 	In general use select only for non-visible symbols
> 	(no prompts anywhere) and for symbols with no dependencies.
> 	That will limit the usefulness but on the other hand avoid
> 	the illegal configurations all over.
> 
Agreed. HWMON should not be selected anywhere. Unfortunately it is, and
drm is no exception. It is selected by DRM_RADEON and DRM_AMDGPU.
Maybe just select it in DRM_I915 as well after all; in practice it won't
make a difference.

Guenter

Jani Nikula Aug. 23, 2022, 1:35 p.m. UTC | #7

On Tue, 23 Aug 2022, Guenter Roeck <[e-mail]> wrote:
> On Tue, Aug 23, 2022 at 12:46:14PM +0300, Jani Nikula wrote:
> [ ... ]
>> >> 
>> >> So why not do this in i915 Kconfig:
>> >> 
>> >> config DRM_I915
>> >> 	...
>> >> 	depends on HWMON || HWMON=n
>> > With this change I am getting recursive dependancy error when I run make 
>> > oldconfig
>> >
>> > badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
>> >    HOSTCC  scripts/basic/fixdep
>> >    HOSTCC  scripts/kconfig/conf.o
>> >    HOSTCC  scripts/kconfig/confdata.o
>> >    HOSTCC  scripts/kconfig/expr.o
>> >    LEX     scripts/kconfig/lexer.lex.c
>> >    YACC    scripts/kconfig/parser.tab.[ch]
>> >    HOSTCC  scripts/kconfig/lexer.lex.o
>> >    HOSTCC  scripts/kconfig/menu.o
>> >    HOSTCC  scripts/kconfig/parser.tab.o
>> >    HOSTCC  scripts/kconfig/preprocess.o
>> >    HOSTCC  scripts/kconfig/symbol.o
>> >    HOSTCC  scripts/kconfig/util.o
>> >    HOSTLD  scripts/kconfig/conf
>> > drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
>> > drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
>> > drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
>> > drivers/platform/x86/Kconfig:332:       symbol EEEPC_LAPTOP depends on INPUT
>> > drivers/input/Kconfig:8:        symbol INPUT is selected by DRM_I915
>> > For a resolution refer to Documentation/kbuild/kconfig-language.rst
>> > subsection "Kconfig recursive dependency limitations"
>> 
>> *sigh*
>> 
>>   Note:
>> 	select should be used with care. select will force
>> 	a symbol to a value without visiting the dependencies.
>> 	By abusing select you are able to select a symbol FOO even
>> 	if FOO depends on BAR that is not set.
>> 	In general use select only for non-visible symbols
>> 	(no prompts anywhere) and for symbols with no dependencies.
>> 	That will limit the usefulness but on the other hand avoid
>> 	the illegal configurations all over.
>> 
> Agreed. HWMON should not be selected anywhere. Unfortunately it is, and
> drm is no exception. It is selected by DRM_RADEON and DRM_AMDGPU.
> Maybe just select it in DRM_I915 as well after all; in practice it won't
> make a difference.

And I guess everyone just does what I'm about to do now, throw my hands
up in the air in disgust and resignation. :p

BR,
Jani.

Nilawar, Badal Aug. 23, 2022, 2:28 p.m. UTC | #8

On 23-08-2022 19:05, Jani Nikula wrote:
> On Tue, 23 Aug 2022, Guenter Roeck <[e-mail]> wrote:
>> On Tue, Aug 23, 2022 at 12:46:14PM +0300, Jani Nikula wrote:
>> [ ... ]
>>>>>
>>>>> So why not do this in i915 Kconfig:
>>>>>
>>>>> config DRM_I915
>>>>> 	...
>>>>> 	depends on HWMON || HWMON=n
>>>> With this change I am getting recursive dependancy error when I run make
>>>> oldconfig
>>>>
>>>> badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
>>>>     HOSTCC  scripts/basic/fixdep
>>>>     HOSTCC  scripts/kconfig/conf.o
>>>>     HOSTCC  scripts/kconfig/confdata.o
>>>>     HOSTCC  scripts/kconfig/expr.o
>>>>     LEX     scripts/kconfig/lexer.lex.c
>>>>     YACC    scripts/kconfig/parser.tab.[ch]
>>>>     HOSTCC  scripts/kconfig/lexer.lex.o
>>>>     HOSTCC  scripts/kconfig/menu.o
>>>>     HOSTCC  scripts/kconfig/parser.tab.o
>>>>     HOSTCC  scripts/kconfig/preprocess.o
>>>>     HOSTCC  scripts/kconfig/symbol.o
>>>>     HOSTCC  scripts/kconfig/util.o
>>>>     HOSTLD  scripts/kconfig/conf
>>>> drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
>>>> drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
>>>> drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
>>>> drivers/platform/x86/Kconfig:332:       symbol EEEPC_LAPTOP depends on INPUT
>>>> drivers/input/Kconfig:8:        symbol INPUT is selected by DRM_I915
>>>> For a resolution refer to Documentation/kbuild/kconfig-language.rst
>>>> subsection "Kconfig recursive dependency limitations"
>>>
>>> *sigh*
>>>
>>>    Note:
>>> 	select should be used with care. select will force
>>> 	a symbol to a value without visiting the dependencies.
>>> 	By abusing select you are able to select a symbol FOO even
>>> 	if FOO depends on BAR that is not set.
>>> 	In general use select only for non-visible symbols
>>> 	(no prompts anywhere) and for symbols with no dependencies.
>>> 	That will limit the usefulness but on the other hand avoid
>>> 	the illegal configurations all over.
>>>
>> Agreed. HWMON should not be selected anywhere. Unfortunately it is, and
>> drm is no exception. It is selected by DRM_RADEON and DRM_AMDGPU.
>> Maybe just select it in DRM_I915 as well after all; in practice it won't
>> make a difference.
> 
> And I guess everyone just does what I'm about to do now, throw my hands
> up in the air in disgust and resignation. :p
How about sticking to existing approach only. In my previous response I 
mentioned that for combo which we want to reject CONFIG_HWMON=m && 
CONFIG_DRM_I915=y combo i915_hwmon.o is not getting build.
It is only getting build for below combos
CONFIG_HWMON=m && CONFIG_DRM_I915=y
CONFIG_HWMON=m && CONFIG_DRM_I915=m
CONFIG_HWMON=y && CONFIG_DRM_I915=m
Regards,
Badal
> 
> BR,
> Jani.
> 
> 
>

Jani Nikula Aug. 23, 2022, 2:41 p.m. UTC | #9

On Tue, 23 Aug 2022, "Nilawar, Badal" <[e-mail]> wrote:
> On 23-08-2022 19:05, Jani Nikula wrote:
>> On Tue, 23 Aug 2022, Guenter Roeck <[e-mail]> wrote:
>>> On Tue, Aug 23, 2022 at 12:46:14PM +0300, Jani Nikula wrote:
>>> [ ... ]
>>>>>>
>>>>>> So why not do this in i915 Kconfig:
>>>>>>
>>>>>> config DRM_I915
>>>>>> 	...
>>>>>> 	depends on HWMON || HWMON=n
>>>>> With this change I am getting recursive dependancy error when I run make
>>>>> oldconfig
>>>>>
>>>>> badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
>>>>>     HOSTCC  scripts/basic/fixdep
>>>>>     HOSTCC  scripts/kconfig/conf.o
>>>>>     HOSTCC  scripts/kconfig/confdata.o
>>>>>     HOSTCC  scripts/kconfig/expr.o
>>>>>     LEX     scripts/kconfig/lexer.lex.c
>>>>>     YACC    scripts/kconfig/parser.tab.[ch]
>>>>>     HOSTCC  scripts/kconfig/lexer.lex.o
>>>>>     HOSTCC  scripts/kconfig/menu.o
>>>>>     HOSTCC  scripts/kconfig/parser.tab.o
>>>>>     HOSTCC  scripts/kconfig/preprocess.o
>>>>>     HOSTCC  scripts/kconfig/symbol.o
>>>>>     HOSTCC  scripts/kconfig/util.o
>>>>>     HOSTLD  scripts/kconfig/conf
>>>>> drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
>>>>> drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
>>>>> drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
>>>>> drivers/platform/x86/Kconfig:332:       symbol EEEPC_LAPTOP depends on INPUT
>>>>> drivers/input/Kconfig:8:        symbol INPUT is selected by DRM_I915
>>>>> For a resolution refer to Documentation/kbuild/kconfig-language.rst
>>>>> subsection "Kconfig recursive dependency limitations"
>>>>
>>>> *sigh*
>>>>
>>>>    Note:
>>>> 	select should be used with care. select will force
>>>> 	a symbol to a value without visiting the dependencies.
>>>> 	By abusing select you are able to select a symbol FOO even
>>>> 	if FOO depends on BAR that is not set.
>>>> 	In general use select only for non-visible symbols
>>>> 	(no prompts anywhere) and for symbols with no dependencies.
>>>> 	That will limit the usefulness but on the other hand avoid
>>>> 	the illegal configurations all over.
>>>>
>>> Agreed. HWMON should not be selected anywhere. Unfortunately it is, and
>>> drm is no exception. It is selected by DRM_RADEON and DRM_AMDGPU.
>>> Maybe just select it in DRM_I915 as well after all; in practice it won't
>>> make a difference.
>> 
>> And I guess everyone just does what I'm about to do now, throw my hands
>> up in the air in disgust and resignation. :p
> How about sticking to existing approach only. In my previous response I 
> mentioned that for combo which we want to reject CONFIG_HWMON=m && 
> CONFIG_DRM_I915=y combo i915_hwmon.o is not getting build.
> It is only getting build for below combos
> CONFIG_HWMON=m && CONFIG_DRM_I915=y
> CONFIG_HWMON=m && CONFIG_DRM_I915=m
> CONFIG_HWMON=y && CONFIG_DRM_I915=m

Then please hide the IS_REACHABLE() within i915_hwmon.h and add stubs as
is customary. Let's not clutter high level driver code with some random
#if directives.

BR,
Jani.


> Regards,
> Badal
>> 
>> BR,
>> Jani.
>> 
>> 
>>

Nilawar, Badal Aug. 25, 2022, 7:27 a.m. UTC | #10

On 23-08-2022 20:11, Jani Nikula wrote:
> On Tue, 23 Aug 2022, "Nilawar, Badal" <[e-mail]> wrote:
>> On 23-08-2022 19:05, Jani Nikula wrote:
>>> On Tue, 23 Aug 2022, Guenter Roeck <[e-mail]> wrote:
>>>> On Tue, Aug 23, 2022 at 12:46:14PM +0300, Jani Nikula wrote:
>>>> [ ... ]
>>>>>>>
>>>>>>> So why not do this in i915 Kconfig:
>>>>>>>
>>>>>>> config DRM_I915
>>>>>>> 	...
>>>>>>> 	depends on HWMON || HWMON=n
>>>>>> With this change I am getting recursive dependancy error when I run make
>>>>>> oldconfig
>>>>>>
>>>>>> badal@bnilawar-desk1:~/workspace/wp3/drm-tip$ make oldconfig
>>>>>>      HOSTCC  scripts/basic/fixdep
>>>>>>      HOSTCC  scripts/kconfig/conf.o
>>>>>>      HOSTCC  scripts/kconfig/confdata.o
>>>>>>      HOSTCC  scripts/kconfig/expr.o
>>>>>>      LEX     scripts/kconfig/lexer.lex.c
>>>>>>      YACC    scripts/kconfig/parser.tab.[ch]
>>>>>>      HOSTCC  scripts/kconfig/lexer.lex.o
>>>>>>      HOSTCC  scripts/kconfig/menu.o
>>>>>>      HOSTCC  scripts/kconfig/parser.tab.o
>>>>>>      HOSTCC  scripts/kconfig/preprocess.o
>>>>>>      HOSTCC  scripts/kconfig/symbol.o
>>>>>>      HOSTCC  scripts/kconfig/util.o
>>>>>>      HOSTLD  scripts/kconfig/conf
>>>>>> drivers/gpu/drm/i915/Kconfig:2:error: recursive dependency detected!
>>>>>> drivers/gpu/drm/i915/Kconfig:2: symbol DRM_I915 depends on HWMON
>>>>>> drivers/hwmon/Kconfig:6:        symbol HWMON is selected by EEEPC_LAPTOP
>>>>>> drivers/platform/x86/Kconfig:332:       symbol EEEPC_LAPTOP depends on INPUT
>>>>>> drivers/input/Kconfig:8:        symbol INPUT is selected by DRM_I915
>>>>>> For a resolution refer to Documentation/kbuild/kconfig-language.rst
>>>>>> subsection "Kconfig recursive dependency limitations"
>>>>>
>>>>> *sigh*
>>>>>
>>>>>     Note:
>>>>> 	select should be used with care. select will force
>>>>> 	a symbol to a value without visiting the dependencies.
>>>>> 	By abusing select you are able to select a symbol FOO even
>>>>> 	if FOO depends on BAR that is not set.
>>>>> 	In general use select only for non-visible symbols
>>>>> 	(no prompts anywhere) and for symbols with no dependencies.
>>>>> 	That will limit the usefulness but on the other hand avoid
>>>>> 	the illegal configurations all over.
>>>>>
>>>> Agreed. HWMON should not be selected anywhere. Unfortunately it is, and
>>>> drm is no exception. It is selected by DRM_RADEON and DRM_AMDGPU.
>>>> Maybe just select it in DRM_I915 as well after all; in practice it won't
>>>> make a difference.
>>>
>>> And I guess everyone just does what I'm about to do now, throw my hands
>>> up in the air in disgust and resignation. :p
>> How about sticking to existing approach only. In my previous response I
>> mentioned that for combo which we want to reject CONFIG_HWMON=m &&
>> CONFIG_DRM_I915=y combo i915_hwmon.o is not getting build.
>> It is only getting build for below combos
>> CONFIG_HWMON=m && CONFIG_DRM_I915=y
>> CONFIG_HWMON=m && CONFIG_DRM_I915=m
>> CONFIG_HWMON=y && CONFIG_DRM_I915=m
> 
> Then please hide the IS_REACHABLE() within i915_hwmon.h and add stubs as
> is customary. Let's not clutter high level driver code with some random
> #if directives.

Thanks, I will resolve this and float the new series.

Regards,
Badal
> 
> BR,
> Jani.
> 
> 
>> Regards,
>> Badal
>>>
>>> BR,
>>> Jani.
>>>
>>>
>>>
>

Patch

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
+
 # 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__ */