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

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

On 12/16/2022 12:47 AM, Ashutosh Dixit 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, so that users see PL1 limits
> HW is likely using. Otherwise users think HW is using arbitrarily high/low
> PL1 limits they might have set. The previous write/read I1 power1_crit
> limit also follows the same clamping pattern.
> 
> v2: Explain "why" in commit message and include bug link (Jani Nikula)
> 
> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7704
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Looks good to me.
Reviewed-by: Anshuman Gupta <anshuman.gupta@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)