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@xxxxxxxxx>
---
.../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
