On Tue, 23 Aug 2022, "Nilawar, Badal" <badal.nilawar@xxxxxxxxx> wrote:
> On 19-08-2022 16:05, Jani Nikula wrote:
>> 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
> 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 <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!
> 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, Intel Open Source Graphics Center
