Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Display clamped PL1 limit

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 15 Dec 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
> HW allows arbitrary PL1 limits to be set but silently clamps these values
> to "typical but not guaranteed" min/max values in pkg_power_sku
> register. Follow the same pattern for sysfs, allow arbitrary PL1 limits to
> be set but display clamped values when read.

The commit message lacks the most important thing: why?


BR,
Jani.

>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_hwmon.c        | 39 ++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_mchbar_regs.h |  2 ++
>  2 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index cca7a4350ec8f..1225bc432f0d5 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -359,6 +359,38 @@ hwm_power_is_visible(const struct hwm_drvdata *ddat, u32 attr, int chan)
>  	}
>  }
>  
> +/*
> + * 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
> + * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
> + * clamped values when read. Write/read I1 also follows the same pattern.
> + */
> +static int
> +hwm_power_max_read(struct hwm_drvdata *ddat, long *val)
> +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +	intel_wakeref_t wakeref;
> +	u64 r, min, max;
> +
> +	*val = hwm_field_read_and_scale(ddat,
> +					hwmon->rg.pkg_rapl_limit,
> +					PKG_PWR_LIM_1,
> +					hwmon->scl_shift_power,
> +					SF_POWER);
> +
> +	with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> +		r = intel_uncore_read64(ddat->uncore, hwmon->rg.pkg_power_sku);
> +	min = REG_FIELD_GET(PKG_MIN_PWR, r);
> +	min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
> +	max = REG_FIELD_GET(PKG_MAX_PWR, r);
> +	max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
> +
> +	if (min && max)
> +		*val = clamp_t(u64, *val, min, max);
> +
> +	return 0;
> +}
> +
>  static int
>  hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
>  {
> @@ -368,12 +400,7 @@ hwm_power_read(struct hwm_drvdata *ddat, u32 attr, int chan, long *val)
>  
>  	switch (attr) {
>  	case hwmon_power_max:
> -		*val = hwm_field_read_and_scale(ddat,
> -						hwmon->rg.pkg_rapl_limit,
> -						PKG_PWR_LIM_1,
> -						hwmon->scl_shift_power,
> -						SF_POWER);
> -		return 0;
> +		return hwm_power_max_read(ddat, val);
>  	case hwmon_power_rated_max:
>  		*val = hwm_field_read_and_scale(ddat,
>  						hwmon->rg.pkg_power_sku,
> diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h b/drivers/gpu/drm/i915/intel_mchbar_regs.h
> index f93e9af43ac35..73900c098d591 100644
> --- a/drivers/gpu/drm/i915/intel_mchbar_regs.h
> +++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h
> @@ -194,6 +194,8 @@
>   */
>  #define PCU_PACKAGE_POWER_SKU			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5930)
>  #define   PKG_PKG_TDP				GENMASK_ULL(14, 0)
> +#define   PKG_MIN_PWR				GENMASK_ULL(30, 16)
> +#define   PKG_MAX_PWR				GENMASK_ULL(46, 32)
>  #define   PKG_MAX_WIN				GENMASK_ULL(54, 48)
>  #define     PKG_MAX_WIN_X			GENMASK_ULL(54, 53)
>  #define     PKG_MAX_WIN_Y			GENMASK_ULL(52, 48)

-- 
Jani Nikula, Intel Open Source Graphics Center