Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 19-08-2022 16:05, Jani Nikula wrote:
> On Fri, 19 Aug 2022, Badal Nilawar <badal.nilawar@intel.com> wrote:
>> From: Dale B Stimson <dale.b.stimson@intel.com>
>>
>> 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 <linux@roeck-us.net>
>> Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com>
>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   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__ */
>