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