Re: [Intel-xe] [PATCH v2 1/6] drm/xe/hwmon: Add HWMON infrastructure

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

Hi Matthew,

On 29-06-2023 04:20, Matthew Brost wrote:
> On Wed, Jun 28, 2023 at 12:00:38AM +0530, 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.
>> This is port from i915 hwmon.
>>
>> v2: Fix review comments (Riana)
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@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        | 116 +++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_hwmon.h        |  22 +++++
>>   5 files changed, 148 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 4b82cb2773ad..e39d77037622 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -113,6 +113,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 c7985af85a53..0fcd60037d66 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"
>>   
>>   static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>>   {
>> @@ -328,6 +329,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;
>> @@ -354,6 +357,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 0226d44a6af2..21bff0e610a1 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -332,6 +332,8 @@ struct xe_device {
>>   	/** @d3cold_allowed: Indicates if d3cold is a valid device state */
>>   	bool d3cold_allowed;
>>   
>> +	struct xe_hwmon *hwmon;
>> +
> 
> You likely need a 'struct xe_hwmon' forward declaration at the top of
> the file.
Sure it will move it to top of the file.
> 
>>   	/* 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..8f653fdf4ad5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -0,0 +1,116 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +
>> +#include "regs/xe_gt_regs.h"
>> +#include "xe_device.h"
>> +#include "xe_hwmon.h"
>> +
>> +struct hwm_drvdata {
>> +	struct xe_hwmon *hwmon;
>> +	struct device *hwmon_dev;
>> +	char name[12];
>> +};
>> +
>> +struct xe_hwmon {
>> +	struct hwm_drvdata ddat;
>> +	struct mutex hwmon_lock;
>> +};
>> +
> 
> These two structures look very goofy, e.g. why two 2 structs instead of
> 1, why a back pointer in 'struct hwm_drvdata' rather than using
> container_of? Just make this one structure?
In energy patch to cover gt specific energy attr there are multiple 
instances of hwm_drvdata defined. which creates saperate hwmon entry for 
each gt. But I will think about adding one structure.
> 
>> +static const struct hwmon_channel_info *hwm_info[] = {
>> +	NULL
>> +};
>> +
>> +static umode_t
>> +hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> 
> Nit and matter of opinion but...
> 
> s/hwm_is_visible/xe_hwmon_is_visible
Throught out driver prefix hwm_ is used for static function and for 
global functions prefix xe_hwmon_ is used. Instead of hwm_* prefix 
hwmon_ prefix can be used.
> 
>> +	       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)
> 
> s/hwm_read/xe_hwmon_read
> 
>> +{
>> +	switch (type) {
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static int
>> +hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +	  int channel, long val)
>> +{
> 
> s/hwm_write/xe_hwmon_write
> 
>> +	switch (type) {
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +}
>> +
>> +static const struct hwmon_ops hwm_ops = {
> 
> s/hwm_ops/xe_hwmon_ops
> 
>> +	.is_visible = hwm_is_visible,
>> +	.read = hwm_read,
>> +	.write = hwm_write,
>> +};
>> +
>> +static const struct hwmon_chip_info hwm_chip_info = {
> 
> s/hwm_chip_info/xe_hwmon_chip_info
> 
>> +	.ops = &hwm_ops,
>> +	.info = hwm_info,
>> +};
>> +
>> +static void
>> +hwm_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 hwm_drvdata *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;
>> +	mutex_init(&hwmon->hwmon_lock);
> 
> drmm_mutex_init
Sure I will change this.
> 
> Matt
> 
>> +	ddat = &hwmon->ddat;
>> +
>> +	ddat->hwmon = hwmon;
>> +	snprintf(ddat->name, sizeof(ddat->name), "xe");
>> +
>> +	hwm_get_preregistration_info(xe);
>> +
>> +	drm_dbg(&xe->drm, "Register HWMON interface\n");
>> +
>> +	/*  hwmon_dev points to device hwmon<i> */
>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, ddat->name,
>> +							 ddat,
>> +							 &hwm_chip_info,
>> +							 NULL);
>> +	if (IS_ERR(hwmon_dev)) {
>> +		drm_warn(&xe->drm, "Fail to register xe hwmon\n");
>> +		xe->hwmon = NULL;
>> +		return;
>> +	}
>> +
>> +	ddat->hwmon_dev = hwmon_dev;
>> +}
>> +
>> +void xe_hwmon_unregister(struct xe_device *xe)
>> +{
>> +	xe->hwmon = NULL;
>> +}
>> 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__ */
>> -- 
>> 2.25.1
>>