On Thu, Feb 16, 2023 at 11:25:50AM -0800, Guenter Roeck wrote: > 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. I'm sorry for having missed this part. It is not that I ignored it, I hadn't opened it because the title is on how to get patches "accepted into the hwmon subsystem". I was only reading the docs related to use hwmon in the drivers, not yet at the point were I thought this case was generic enough to get that *into* hwmon subsystem. > > > 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. For the record, I am also not arguing. I'm just trying to understand the rules. I believe hwmon is such a good infra and based on the basic docs I had the impression that the expansion of non-standards was allowed and desireable on non-standard cases and that the contribution to get standard into hwmon would just come on things that it really looks that more devices have in common. > > Guenter >
