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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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

As mentioned in the commit message, power1_max_enable is exposed to avoid
possible misinterpretations in measured energy in response to the set power
limit (something specific to our HW). We may have multiple such limits in
the future with similar issues and multiplexing enabling/disabling these
power limits via a single power1_enable file will not provide sufficient
granularity for our purposes.

Also, I had previously posted this patch:

https://patchwork.freedesktop.org/patch/522612/?series=113972&rev=1

which avoids the power1_max_enable file and instead uses a power1_max value
of -1 to indicate that the power1_max limit is disabled.

So in summary we have the following options:

1. Go with power1_max_enable (preferred, works well for us)
2. Go with -1 to indicate that the power1_max limit is disabled
   (non-intuitive and also a little ugly)
3. Go with power1_enable (possible but confusing because multiple power
   limits/entities are multiplexed via a single file)

If you still think we should not use power1_max_enable I think I might drop
this patch for now (I am trying to preempt future issues but in this case
it's better to wait till people actually complain rather than expose a
non-ideal uapi).

Even if drop we this patch now, it would be good to know your preference in
case we need to revisit this issue later.

Thanks and also sorry for the rather long winded email.

Ashutosh

> Guenter
>
> > +Date:		May 2023
> > +KernelVersion:	6.3
> > +Contact:	intel-gfx@lists.freedesktop.org
> > +Description:	RW. Enable/disable the PL1 power limit (power1_max).
> > +
> > +		Only supported for particular Intel i915 graphics platforms.
> >   What:		/sys/devices/.../hwmon/hwmon<i>/power1_rated_max
> >   Date:		February 2023
> >   KernelVersion:	6.2
> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > index 7c20a6f47b92e..5665869d8602b 100644
> > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > @@ -230,13 +230,52 @@ hwm_power1_max_interval_store(struct device *dev,
> >					    PKG_PWR_LIM_1_TIME, rxy);
> >	return count;
> >   }
> > +static SENSOR_DEVICE_ATTR_RW(power1_max_interval, hwm_power1_max_interval, 0);
> >   -static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
> > -			  hwm_power1_max_interval_show,
> > -			  hwm_power1_max_interval_store, 0);
> > +static ssize_t
> > +hwm_power1_max_enable_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
> > +	intel_wakeref_t wakeref;
> > +	u32 r;
> > +
> > +	with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > +		r = intel_uncore_read(ddat->uncore, ddat->hwmon->rg.pkg_rapl_limit);
> > +
> > +	return sysfs_emit(buf, "%u\n", !!(r & PKG_PWR_LIM_1_EN));
> > +}
> > +
> > +static ssize_t
> > +hwm_power1_max_enable_store(struct device *dev, struct device_attribute *attr,
> > +			    const char *buf, size_t count)
> > +{
> > +	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
> > +	intel_wakeref_t wakeref;
> > +	u32 en, r;
> > +	bool _en;
> > +	int ret;
> > +
> > +	ret = kstrtobool(buf, &_en);
> > +	if (ret)
> > +		return ret;
> > +
> > +	en = REG_FIELD_PREP(PKG_PWR_LIM_1_EN, _en);
> > +	hwm_locked_with_pm_intel_uncore_rmw(ddat, ddat->hwmon->rg.pkg_rapl_limit,
> > +					    PKG_PWR_LIM_1_EN, en);
> > +
> > +	/* Verify, because PL1 limit cannot be disabled on all platforms */
> > +	with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > +		r = intel_uncore_read(ddat->uncore, ddat->hwmon->rg.pkg_rapl_limit);
> > +	if ((r & PKG_PWR_LIM_1_EN) != en)
> > +		return -EPERM;
> > +
> > +	return count;
> > +}
> > +static SENSOR_DEVICE_ATTR_RW(power1_max_enable, hwm_power1_max_enable, 0);
> >     static struct attribute *hwm_attributes[] = {
> >	&sensor_dev_attr_power1_max_interval.dev_attr.attr,
> > +	&sensor_dev_attr_power1_max_enable.dev_attr.attr,
> >	NULL
> >   };
> >   @@ -247,7 +286,8 @@ static umode_t hwm_attributes_visible(struct
> > kobject *kobj,
> >	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
> >	struct i915_hwmon *hwmon = ddat->hwmon;
> >   -	if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr)
> > +	if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr ||
> > +	    attr == &sensor_dev_attr_power1_max_enable.dev_attr.attr)
> >		return i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit) ? attr->mode : 0;
> >		return 0;
>