Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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

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
>