[PATCH 0/3] PL1 power limit fixes for ATSM

[PATCH 0/3] PL1 power limit fixes for ATSM

[Ashutosh Dixit]

Previous PL1 power limit implementation assumed that the PL1 limit is
always enabled in HW. However we now find this not to be the case on ATSM
where the PL1 limit is disabled at power up. This requires changes in the
previous PL1 limit implementation.

Submitting 3 patches for easier review but patches can be squashed if
needed.

Ashutosh Dixit (3):
  drm/i915/hwmon: Replace hwm_field_scale_and_write with
    hwm_power_max_write
  drm/i915/hwmon: Enable PL1 limit when writing limit value to HW
  drm/i915/hwmon: Expose power1_max_enable

 .../ABI/testing/sysfs-driver-intel-i915-hwmon |  7 ++
 drivers/gpu/drm/i915/i915_hwmon.c             | 85 +++++++++++++------
 2 files changed, 68 insertions(+), 24 deletions(-)

-- 
2.38.0

[PATCH 1/3] drm/i915/hwmon: Replace hwm_field_scale_and_write with hwm_power_max_write

[Ashutosh Dixit]

hwm_field_scale_and_write has a single caller hwm_power_write and is
specific to hwm_power_write but makes it appear that it is a general
function which can have multiple callers. Replace the function with
hwm_power_max_write which is specific to hwm_power_write and use that in
future patches where the function needs to be extended.

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_hwmon.c | 36 ++++++++++++++-----------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index 1225bc432f0d5..85195d61f89c7 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -99,20 +99,6 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
 	return mul_u64_u32_shr(reg_value, scale_factor, nshift);
 }
 
-static void
-hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
-			  int nshift, unsigned int scale_factor, long lval)
-{
-	u32 nval;
-
-	/* Computation in 64-bits to avoid overflow. Round to nearest. */
-	nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
-
-	hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
-					    PKG_PWR_LIM_1,
-					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
-}
-
 /*
  * hwm_energy - Obtain energy value
  *
@@ -391,6 +377,21 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
 	return 0;
 }
 
+static int
+hwm_power_max_write(struct hwm_drvdata *ddat, long val)
+{
+	struct i915_hwmon *hwmon = ddat->hwmon;
+	u32 nval;
+
+	/* Computation in 64-bits to avoid overflow. Round to nearest. */
+	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
+
+	hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
+					    PKG_PWR_LIM_1,
+					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
+	return 0;
+}
+
 static int
 hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
 {
@@ -425,16 +426,11 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
 static int
 hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
 {
-	struct i915_hwmon *hwmon = ddat->hwmon;
 	u32 uval;
 
 	switch (attr) {
 	case hwmon_power_max:
-		hwm_field_scale_and_write(ddat,
-					  hwmon->rg.pkg_rapl_limit,
-					  hwmon->scl_shift_power,
-					  SF_POWER, val);
-		return 0;
+		return hwm_power_max_write(ddat, val);
 	case hwmon_power_crit:
 		uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
 		return hwm_pcode_write_i1(ddat->uncore->i915, uval);
-- 
2.38.0

[PATCH 2/3] drm/i915/hwmon: Enable PL1 limit when writing limit value to HW

[Ashutosh Dixit]

Previous documentation suggested that the PL1 power limit is always enabled
in HW. However we now find this not to be the case on some platforms (such
as ATSM). Therefore enable the PL1 power limit (by setting the enable bit)
when writing the PL1 limit value to HW.

Bspec: 51864

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_hwmon.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index 85195d61f89c7..7c20a6f47b92e 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -385,10 +385,11 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val)
 
 	/* Computation in 64-bits to avoid overflow. Round to nearest. */
 	nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER);
+	nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
 
 	hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
-					    PKG_PWR_LIM_1,
-					    REG_FIELD_PREP(PKG_PWR_LIM_1, nval));
+					    PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1,
+					    nval);
 	return 0;
 }
 
-- 
2.38.0

[PATCH 3/3] drm/i915/hwmon: Expose power1_max_enable

[Ashutosh Dixit]

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
+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;
-- 
2.38.0

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

[Guenter Roeck]

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.

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;

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

[Dixit, Ashutosh]

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

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

[Rodrigo Vivi]

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

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

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

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

[Guenter Roeck]

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.

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

Guenter

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

[Rodrigo Vivi]

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
>