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
