[PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable

[Guenter Roeck]

On 2/16/23 10:57, Rodrigo Vivi wrote:
> On Tue, Feb 14, 2023 at 07:11:16PM -0800, Dixit, Ashutosh wrote:
>> On Mon, 13 Feb 2023 22:16:44 -0800, Guenter Roeck wrote:
>>>
>>
>> Hi Guenter,
>>
>>> On 2/13/23 21:33, Ashutosh Dixit wrote:
>>>> On ATSM the PL1 power limit is disabled at power up. The previous uapi
>>>> assumed that the PL1 limit is always enabled and therefore did not have a
>>>> notion of a disabled PL1 limit. This results in erroneous PL1 limit values
>>>> when PL1 limit is disabled. For example at power up, the disabled ATSM PL1
>>>> limit is shown as 0 which means a low PL1 limit whereas the limit being
>>>> disabled actually implies a high effective PL1 limit value.
>>>>
>>>> To get round this problem, expose power1_max_enable as a custom hwmon
>>>> attribute. power1_max_enable can be used in conjunction with power1_max to
>>>> interpret power1_max (PL1 limit) values correctly. It can also be used to
>>>> enable/disable the PL1 power limit.
>>>>
>>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
>>>> ---
>>>>    .../ABI/testing/sysfs-driver-intel-i915-hwmon |  7 +++
>>>>    drivers/gpu/drm/i915/i915_hwmon.c             | 48 +++++++++++++++++--
>>>>    2 files changed, 51 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>> index 2d6a472eef885..edd94a44b4570 100644
>>>> --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>> @@ -18,6 +18,13 @@ Description:	RW. Card reactive sustained  (PL1/Tau) power limit in microwatts.
>>>> 			Only supported for particular Intel i915 graphics
>>>> platforms.
>>>>    +What:		/sys/devices/.../hwmon/hwmon<i>/power1_max_enable
>>>
>>> This is not a standard hwmon attribute. The standard attribute would be
>>> power1_enable.
>>>
>>> So from hwmon perspective this is a NACK.
>>
>> Thanks for the feedback. I did consider power1_enable but decided to go
>> with the power1_max_enable custom attribute. Documentation for
>> power1_enable says it is to "Enable or disable the sensors" but in our case
>> we are not enabling/disabling sensors (which we don't have any ability to,
>> neither do we expose any power measurements, only energy from which power
>> can be derived) but enabling/disabling a "power limit" (a limit beyond
>> which HW takes steps to limit power).
> 
> Hi Guenter,
> 
> are you okay with this explanation to release the previous 'nack'?
> 

Not really. My suggested solution would have been to use a value of '0'
to indicate 'disabled' and document it accordingly.

> For me it looks like this case really doesn't fit in the standard ones.
> 
> But also this made me wonder what are the rules for non-standard cases?
> 
> I couldn't find any clear guidelines in here:
> https://docs.kernel.org/hwmon/hwmon-kernel-api.html#driver-provided-sysfs-attributes
> 
> We are seeing drivers around to freely use non-standard hwmon.

Yes, sure, freely. You conveniently ignore

Do not create non-standard attributes unless really needed. If you have to use
non-standard attributes, or you believe you do, discuss it on the mailing list
first. Either case, provide a detailed explanation why you need the non-standard
attribute(s). Standard attributes are specified in Naming and data format
standards for sysfs files.

from Documentation/hwmon/submitting-patches.rst.

> Are we free to add non standard ones as long if doesn't fit in the defined
> standards, or should we really limit the use and do our own thing on our own?
> 

> I mean, for the new Xe driver I was considering to standardize everything
> related to freq and power on top of the hwmon instead of separated sysfs
> files. But this would mean a lot of non-standard stuff on top of a few
> standard hwmon stuff. But I will hold this plan if you tell me that we
> should avoid and limit the non-standard cases.
> 

Oh, I really don't want to keep arguing, especially after your "freely"
above. Do whatever you want, just keep it out of drivers/hwmon.

Guenter