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

[Dixit, Ashutosh]

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 at 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 at 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 at 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?

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

Thanks.
--
Ashutosh