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