[Nouveau] [PATCH 3/3] hwmon: expose power_max and power_crit

[Karol Herbst]

On 25 October 2016 7:33:08 a.m. GMT+02:00, Martin Peres <martin.peres at free.fr> wrote:
>On 25/10/16 00:11, Karol Herbst wrote:
>> Signed-off-by: Karol Herbst <karolherbst at gmail.com>
>> ---
>>  drm/nouveau/nouveau_hwmon.c | 44
>++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/drm/nouveau/nouveau_hwmon.c
>b/drm/nouveau/nouveau_hwmon.c
>> index 71f764b..3d4672a 100644
>> --- a/drm/nouveau/nouveau_hwmon.c
>> +++ b/drm/nouveau/nouveau_hwmon.c
>> @@ -596,6 +596,32 @@ nouveau_hwmon_get_power1_input(struct device *d,
>struct device_attribute *a,
>>  static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO,
>>  			  nouveau_hwmon_get_power1_input, NULL, 0);
>>
>> +static ssize_t
>> +nouveau_hwmon_get_power1_max(struct device *d, struct
>device_attribute *a,
>> +			     char *buf)
>> +{
>> +	struct drm_device *dev = dev_get_drvdata(d);
>> +	struct nouveau_drm *drm = nouveau_drm(dev);
>> +	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
>> +	return sprintf(buf, "%i\n", iccsense->power_w_max);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(power1_max, S_IRUGO,
>> +			  nouveau_hwmon_get_power1_max, NULL, 0);
>> +
>> +static ssize_t
>> +nouveau_hwmon_get_power1_crit(struct device *d, struct
>device_attribute *a,
>> +			      char *buf)
>> +{
>> +	struct drm_device *dev = dev_get_drvdata(d);
>> +	struct nouveau_drm *drm = nouveau_drm(dev);
>> +	struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->device);
>> +	return sprintf(buf, "%i\n", iccsense->power_w_crit);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(power1_crit, S_IRUGO,
>> +			  nouveau_hwmon_get_power1_crit, NULL, 0);
>> +
>>  static struct attribute *hwmon_default_attributes[] = {
>>  	&sensor_dev_attr_name.dev_attr.attr,
>>  	&sensor_dev_attr_update_rate.dev_attr.attr,
>> @@ -639,6 +665,12 @@ static struct attribute
>*hwmon_power_attributes[] = {
>>  	NULL
>>  };
>>
>> +static struct attribute *hwmon_power_caps_attributes[] = {
>> +	&sensor_dev_attr_power1_max.dev_attr.attr,
>> +	&sensor_dev_attr_power1_crit.dev_attr.attr,
>> +	NULL
>> +};
>> +
>>  static const struct attribute_group hwmon_default_attrgroup = {
>>  	.attrs = hwmon_default_attributes,
>>  };
>> @@ -657,6 +689,9 @@ static const struct attribute_group
>hwmon_in0_attrgroup = {
>>  static const struct attribute_group hwmon_power_attrgroup = {
>>  	.attrs = hwmon_power_attributes,
>>  };
>> +static const struct attribute_group hwmon_power_caps_attrgroup = {
>> +	.attrs = hwmon_power_caps_attributes,
>> +};
>>  #endif
>>
>>  int
>> @@ -728,8 +763,16 @@ nouveau_hwmon_init(struct drm_device *dev)
>>  	if (iccsense && iccsense->data_valid &&
>!list_empty(&iccsense->rails)) {
>>  		ret = sysfs_create_group(&hwmon_dev->kobj,
>>  					 &hwmon_power_attrgroup);
>> +
>>  		if (ret)
>>  			goto error;
>> +
>> +		if (iccsense->power_w_max && iccsense->power_w_crit) {
>> +			ret = sysfs_create_group(&hwmon_dev->kobj,
>> +						 &hwmon_power_caps_attrgroup);
>> +			if (ret)
>> +				goto error;
>> +		}
>>  	}
>>
>>  	hwmon->hwmon = hwmon_dev;
>> @@ -759,6 +802,7 @@ nouveau_hwmon_fini(struct drm_device *dev)
>>  		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_fan_rpm_attrgroup);
>>  		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_in0_attrgroup);
>>  		sysfs_remove_group(&hwmon->hwmon->kobj, &hwmon_power_attrgroup);
>> +		sysfs_remove_group(&hwmon->hwmon->kobj,
>&hwmon_power_caps_attrgroup);
>>
>>  		hwmon_device_unregister(hwmon->hwmon);
>>  	}
>>
>
>So, you wanted to make a cache because you felt you may want to let the
>
>user edit the values?
>
>Not sure we should ever allow this :s At least, a RO file is a very
>good 
>idea to help debugging and just letting the user know about such 
>limitations.
>
>So, in my opinion, I think we should just kill patch 2 and use directly
>
>the output of the bios table here. Maybe you can write a helper
>function 
>in patch 1 to get the min, avg and max values and return -1 if it is
>not 
>valid? This way, you can access this info very easily in multiple
>places 
>without duplicating this 0xff check.
>
>

no, my intention was to parse the vbios once and be done with it, otherwise we would spread the logic how to read the values. But maybe the entries are indeed always min/max/crit and there is no avg value. But the min of the cap entry means something like min max consumption.

Aditinally the other ebtries have really weird affects, so I would rather not want to parse the table every time.

There are some flags in the entries which affect what is the max allowed consumption and I alrwady see it will get a little complicated.

>_______________________________________________
>Nouveau mailing list
>Nouveau at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/nouveau