Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Use -1 to designate disabled PL1 power limit

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

On Mon, 13 Feb 2023 13:00:49 -0800, Ashutosh Dixit wrote:
>
> On ATSM the PL1 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 the
> PL1 limit is disabled. For example at power up, the disabled ATSM PL1 limit
> was previously 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, the PL1 limit uapi is expanded to include a
> special value, -1, to designate a disabled PL1 limit.

Please don't review this patch. I have replaced this patch with a different
one in v2. Will send v2 out in a bit. Thanks!

>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  .../ABI/testing/sysfs-driver-intel-i915-hwmon |  3 ++-
>  drivers/gpu/drm/i915/i915_hwmon.c             | 24 +++++++++++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> index 2d6a472eef885..7386c39c65cd9 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> @@ -14,7 +14,8 @@ 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.
> +		exceeds this limit. A value of -1 indicates that the PL1
> +		power limit is disabled.
>
>		Only supported for particular Intel i915 graphics platforms.
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index 7c20a6f47b92e..e926f2feaef4b 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -345,6 +345,8 @@ hwm_power_is_visible(const struct hwm_drvdata *ddat, u32 attr, int chan)
>	}
>  }
>
> +#define PL1_DISABLE	-1
> +
>  /*
>   * HW allows arbitrary PL1 limits to be set but silently clamps these values to
>   * "typical but not guaranteed" min/max values in rg.pkg_power_sku. Follow the
> @@ -358,6 +360,14 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
>	intel_wakeref_t wakeref;
>	u64 r, min, max;
>
> +	/* Check if PL1 limit is disabled */
> +	with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> +		r = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit);
> +	if (!(r & PKG_PWR_LIM_1_EN)) {
> +		*val = PL1_DISABLE;
> +		return 0;
> +	}
> +
>	*val = hwm_field_read_and_scale(ddat,
>					hwmon->rg.pkg_rapl_limit,
>					PKG_PWR_LIM_1,
> @@ -381,8 +391,22 @@ static int
>  hwm_power_max_write(struct hwm_drvdata *ddat, long val)
>  {
>	struct i915_hwmon *hwmon = ddat->hwmon;
> +	intel_wakeref_t wakeref;
>	u32 nval;
>
> +	if (val == PL1_DISABLE) {
> +		/* Disable PL1 limit */
> +		hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit,
> +						    PKG_PWR_LIM_1_EN, 0);
> +
> +		/* Verify, because PL1 limit cannot be disabled on all platforms */
> +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> +			nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit);
> +		if (nval & PKG_PWR_LIM_1_EN)
> +			return -EPERM;
> +		return 0;
> +	}
> +
>	/* 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);
> --
> 2.38.0
>