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

[Nilawar, Badal]

Hi Andy,

On 29-06-2023 19:19, Andi Shyti wrote:
> Hi Badal,
> 
> [...]
> 
>>   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);
> 
> we don't have any information whe hwmon has been created or not.
> 
> Please return an error and print a warning if something wrong
> happens and we fail to register the hwmon interface.
Inside xe_hwmon_register warning is being printed, is it not enough?
> 
> [...]
> 
>> +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;
>> +};
> 
> What's the reason for having two different structures here?
Going further to handle gt specific hwmon (energy) attributes multiple 
instances of hwm_drvdata added in xe_hwmon. With that it will create 
separate hwmon sysfs entry under separate name. Also xe_hwmon holds 
common elements. With single structure approach we might need to add 
xe_hwmon instances in xe gt structure as well. As of now we are handling 
device specific and gt specific attributes. Going further there could be 
card specific attributes as well. With existing approach we can add 
another instance of hwm_drvdata to xe_hwmon with single structure 
approach it will be tricky to handle card specific attrs.

root at DUT729PVC:/home/gta/bellekal/kernel# cat /sys/class/hwmon/hwmon*/name
xe
xe_tile0
xe_tile1
> 
> [...]
> 
>> +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;
> 
> this check is useless, we wouldn't be here otherwise.
As of now hwmon is only applicable for dgfx so added this check.
> 
> [...]
> 
>> +void xe_hwmon_unregister(struct xe_device *xe)
>> +{
>> +	xe->hwmon = NULL;
> 
> We could also destroy the mutex and unregister the hwmon?
During i915_hwmon discussion there was suggestion from you to remove 
mutex_destroy as this is not the case of create/destroy or debug.
https://patchwork.freedesktop.org/patch/503368/?series=104278&rev=6
As suggested by Matt planning to use drmm_mutex_init which will take 
care of destroy.
> 
> [...]
> 
>> +#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
> 
> for readability we can leave some space between the
Sure I will fix this.
> #if...#else...#endif and the content.
> 
> Andi