Re: [Intel-gfx] [PATCH 3/4] drm/i915/hwmon: Add HWMON power sensor support

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Jun 21, 2022 at 12:29:21PM -0700, Dixit, Ashutosh wrote:
> On Tue, 21 Jun 2022 10:44:21 -0700, Guenter Roeck wrote:
> >
> > On Mon, Jun 20, 2022 at 11:41:41PM -0700, Dixit, Ashutosh wrote:
> > > On Mon, 20 Jun 2022 13:58:49 -0700, Guenter Roeck wrote:
> > > Hi Guenter, Thanks for taking a look.
> > >
> > > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > index 24c4b7477d51..945f472dd4a2 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > @@ -5,3 +5,23 @@ Contact:	dri-devel@lists.freedesktop.org
> > > > >   Description:	RO. Current Voltage in millivolt.
> > > > >			Only supported for particular Intel i915 graphics
> > > > > platforms.
> > > > > +
> > > > > +What:		/sys/devices/.../hwmon/hwmon<i>/power1_max
> > > > > +Date:		June 2022
> > > > > +KernelVersion:	5.19
> > > > > +Contact:	dri-devel@lists.freedesktop.org
> > > > > +Description:	RW. Card reactive sustained  (PL1/Tau) power limit in microwatts.
> > > > > +
> > > > > +		The power controller will throttle the operating frequency
> > > > > +		if the power averaged over a window (typically seconds)
> > > > > +		exceeds this limit.
> > > > > +
> > > > > +		Only supported for particular Intel i915 graphics platforms.
> > > > > +
> > > > > +What:		/sys/devices/.../hwmon/hwmon<i>/power1_max_default
> > > >
> > > > I don't immediately see the reason for not using the standard power1_cap
> > > > attribute, which is described as
> > > >
> > > >		If power use rises above this limit, the
> > > >                 system should take action to reduce power use.
> > > >
> > > > and pretty much matches the description above.
> > >
> > > Sorry I believe you are referring to the description above which is for the
> > > standard power1_max attribute (as we have used it). The non-standard
> > > attribute is power1_max_default the description for which is below ("Card
> > > default power limit (default TDP setting)").
> > >
> >
> > If you use power1_max to limit power consumption if exceeded, power1_cap
> > might have been more appropriate.
> 
> Looks like in this case the file name is ok but the problem is with the
> description (which does not correspond to what PL1 is). Will fix.
> 
> > > > > +Date:		June 2022
> > > > > +KernelVersion:	5.19
> > > > > +Contact:	dri-devel@lists.freedesktop.org
> > > > > +Description:	RO. Card default power limit (default TDP setting).
> > >
> > > Actually we do not want to use custom hwmon attributes as far as
> > > possible and are looking for some guidance on which standard attributes
> > > which we should use instead.
> > >
> > You could possibly have used power1_rated_max instead of power1_max_default.
> 
> Yes looks like this might work for TDP. We will consider this.
> 
> > > These are the power attributes we are interested in: the two above and
> > > another one which will come in a future patch:
> > >
> > > 1. PL1 (RW)
> > >
> > >    https://www.hardwaretimes.com/intel-10th-gen-cpu-power-consumption-explained-pl1-pl2-and-tau/
> > >
> > > 2. TDP (RO)
> > >
> > >    https://en.wikipedia.org/wiki/Thermal_design_power
> > >
> > Not sure I understand the difference between 'default TDP' (RO),
> > 'TDP' (RO), and PL1.
> 
> 'default TDP' (RO) and 'TDP' (RO) are the same but PL1 is somewhat
> different from TDP (see the first link above) and also I believe chip
> makers specify PL1 and TDP separately (as in this case).
> 
> > > 3. Tau (RW)
> > >
> > >    https://www.hardwaretimes.com/intel-10th-gen-cpu-power-consumption-explained-pl1-pl2-and-tau/
> > >
> > > Would you be able to suggest if there are standard hwmon attributes which
> > > we would be able to use for these three? We also want to use the read/write
> > > permissions as shown above.
> > >
> >
> > There are a number of standard power attributes available to set or report
> > limits (cap, cap_max, cap_min, max, crit, rated_min, rated_max). I would
> > suggest to pick from that list whatever you think fits best. I don't have
> > a recommendation for Tau.
> 
> OK, in that case would a custom setting for Tau also be ok?
> 
Yes.

> > Either case, when reporting power, make sure you don't hit any of the
> > security issues which caused power reporting to be deleted for other CPUs.
> > Restricting read access to hwmon attributes for non-privileged users is not
> > acceptable.
> 
> OK, I believe you are referring to 9049572fb145. Will keep this in mind too.
> 

Correct. Something similar is in the works for another architecture,
for the same reason. Also see 'Attribute access' in
Documentation/hwmon/sysfs-interface.rst.

Thanks,
Guenter