Re: [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Add HWMON power sensor support

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

On Mon, 23 May 2022 04:08:39 -0700, Badal Nilawar wrote:
>

A few initial comments.

> +static void
> +i915_hwmon_get_preregistration_info(struct drm_i915_private *i915)
> +{
> +	struct i915_hwmon *hwmon = i915->hwmon;
> +	struct intel_uncore *uncore = &i915->uncore;
> +	struct i915_hwmon_drvdata *ddat = &hwmon->ddat;
> +	intel_wakeref_t wakeref;
> +	u32 val_sku_unit;
> +	__le32 le_sku_unit;
> +
> +	if (IS_DG1(i915) || IS_DG2(i915)) {
> +		hwmon->rg.pkg_power_sku_unit = PCU_PACKAGE_POWER_SKU_UNIT;
> +		hwmon->rg.pkg_power_sku = INVALID_MMIO_REG;
> +		hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT;
> +	} else {
> +		hwmon->rg.pkg_power_sku_unit = INVALID_MMIO_REG;
> +		hwmon->rg.pkg_power_sku = INVALID_MMIO_REG;
> +		hwmon->rg.pkg_rapl_limit = INVALID_MMIO_REG;
> +	}
> +
> +	wakeref = intel_runtime_pm_get(uncore->rpm);

Let's just use with_intel_runtime_pm().

> +
> +	/*
> +	 * The contents of register hwmon->rg.pkg_power_sku_unit do not change,
> +	 * so read it once and store the shift values.
> +	 *
> +	 * For some platforms, this value is defined as available "for all
> +	 * tiles", with the values consistent across all tiles.
> +	 * In this case, use the tile 0 value for all.

If we are going to introduce multiple tiles "later", let's introduce this
comment later too.

> +	 */
> +	if (i915_mmio_reg_valid(hwmon->rg.pkg_power_sku_unit))
> +		val_sku_unit = intel_uncore_read(uncore,
> +						 hwmon->rg.pkg_power_sku_unit);
> +	else
> +		val_sku_unit = 0;
> +
> +	intel_runtime_pm_put(uncore->rpm, wakeref);
> +
> +	le_sku_unit = cpu_to_le32(val_sku_unit);
> +	hwmon->scl_shift_power = le32_get_bits(le_sku_unit, PKG_PWR_UNIT);

Do we need such endianness conversions, typically we don't do them?

> +
> +	/*
> +	 * The value of power1_max is reset to the default on reboot, but is
> +	 * not reset by a module unload/load sequence.  To allow proper
> +	 * functioning after a module reload, the value for power1_max is
> +	 * restored to its original value at module unload time in
> +	 * i915_hwmon_unregister().
> +	 */

Because on production systems typically modules are not reloaded, I am not
sure if we need to take care of this save/restore. Also there may be other
ways of doing this e.g.:

https://patchwork.freedesktop.org/patch/483988/?series=102665&rev=3

That is above we just expose the default or init values but don't expose
them.

In order for such issues to be reviewed/debated, I would submit this
save/restore part as a separate patch, so decouple it from the
hwmon_power_max patch.

> +void i915_hwmon_register(struct drm_i915_private *i915);
> +void i915_hwmon_unregister(struct drm_i915_private *i915);
> +#endif
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d8579ab9384c..1c570c706ff8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1866,6 +1866,21 @@
>  #define   POWER_LIMIT_4_MASK		REG_BIT(9)
>  #define   POWER_LIMIT_1_MASK		REG_BIT(11)
>  #define   POWER_LIMIT_2_MASK		REG_BIT(12)
> +/*
> + * *_PACKAGE_POWER_SKU - SKU power and timing parameters.
> + * Used herein as a 64-bit register.
> + * These masks are defined using GENMASK_ULL as REG_GENMASK is limited to u32
> + * and as GENMASK is "long" and therefore 32-bits on a 32-bit system.
> + * PKG_PKG_TDP, PKG_MIN_PWR, and PKG_MAX_PWR are scaled in the same way as
> + * PKG_PWR_LIM_*, above.
> + * PKG_MAX_WIN has sub-fields for x and y, and has the value: is 1.x * 2^y.
> + */
> +#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_Y		GENMASK_ULL(54, 53)
> +#define     PKG_MAX_WIN_X		GENMASK_ULL(52, 48)
>
>  #define CHV_CLK_CTL1			_MMIO(0x101100)
>  #define VLV_CLK_CTL2			_MMIO(0x101104)
> diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h b/drivers/gpu/drm/i915/intel_mchbar_regs.h
> index 2aad2f0cc8db..247561d7974c 100644
> --- a/drivers/gpu/drm/i915/intel_mchbar_regs.h
> +++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h
> @@ -191,11 +191,20 @@
>
>  #define GEN6_GT_PERF_STATUS			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5948)
>  #define GEN6_RP_STATE_LIMITS			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5994)
> +#define PCU_PACKAGE_POWER_SKU_UNIT		_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5938)
> +#define   PKG_PWR_UNIT				REG_GENMASK(3, 0)
> +#define   PKG_TIME_UNIT				REG_GENMASK(19, 16)
> +
>  #define GEN6_RP_STATE_CAP			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5998)
>  #define   RP0_CAP_MASK				REG_GENMASK(7, 0)
>  #define   RP1_CAP_MASK				REG_GENMASK(15, 8)
>  #define   RPN_CAP_MASK				REG_GENMASK(23, 16)
>
> +#define PCU_PACKAGE_RAPL_LIMIT			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> +#define   PKG_PWR_LIM_1				REG_GENMASK(14, 0)
> +#define   PKG_PWR_LIM_1_EN			REG_BIT(15)
> +#define   PKG_PWR_LIM_1_TIME			REG_GENMASK(23, 17)
> +

I think we should remove #define's which are not actually used in the patch
and introduce them in the patches in which they are used.

>  /* snb MCH registers for priority tuning */
>  #define MCH_SSKPD				_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5d10)
>  #define   SSKPD_NEW_WM0_MASK_HSW		REG_GENMASK64(63, 56)
> --
> 2.25.1
>

Thanks.
--
Ashutosh