From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com> To: Badal Nilawar <badal.nilawar@intel.com> Cc: <intel-gfx@lists.freedesktop.org>, <riana.tauro@intel.com>, <anshuman.gupta@intel.com>, <jon.ewins@intel.com>, <linux-hwmon@vger.kernel.org> Subject: Re: [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Date: Mon, 29 Aug 2022 19:33:43 -0700 [thread overview] Message-ID: <87k06qzchk.wl-ashutosh.dixit@intel.com> (raw) In-Reply-To: <20220825132118.784407-4-badal.nilawar@intel.com> On Thu, 25 Aug 2022 06:21:14 -0700, Badal Nilawar wrote: > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 2192d0fd4c66..922463da65bf 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -13,8 +13,22 @@ > #include "intel_mchbar_regs.h" > #include "gt/intel_gt_regs.h" > > +/* > + * SF_* - scale factors for particular quantities according to hwmon spec. > + * - power - microwatts > + */ > +#define SF_POWER 1000000 > + > +#define FIELD_SHIFT(__mask) \ > + (BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) + \ > + BUILD_BUG_ON_ZERO((__mask) == 0) + \ > + __bf_shf(__mask)) Let's remove this macro, it's not needed, see below. > +/* > + * This function's return type of u64 allows for the case where the scaling > + * of the field taken from the 32-bit register value might cause a result to > + * exceed 32 bits. > + */ > +static u64 > +hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr, > + u32 field_msk, int field_shift, Let's remove field_shift arg. > + int nshift, u32 scale_factor) > +{ > + struct intel_uncore *uncore = ddat->uncore; > + intel_wakeref_t wakeref; > + u32 reg_value; > + > + with_intel_runtime_pm(uncore->rpm, wakeref) > + reg_value = intel_uncore_read(uncore, rgadr); > + > + reg_value = (reg_value & field_msk) >> field_shift; This is just 'REG_FIELD_GET(field_msk, reg_value)'. > + > + return mul_u64_u32_shr(reg_value, scale_factor, nshift); > +} > + > +static void > +hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, > + u32 field_msk, int field_shift, Let's remove field_shift arg. > + int nshift, unsigned int scale_factor, long lval) > +{ > + u32 nval; > + u32 bits_to_clear; > + u32 bits_to_set; > + > + /* Computation in 64-bits to avoid overflow. Round to nearest. */ > + nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); > + > + bits_to_clear = field_msk; > + bits_to_set = (nval << field_shift) & field_msk; This is just 'REG_FIELD_PREP(field_msk, nval)'. > @@ -118,11 +260,40 @@ static void > hwm_get_preregistration_info(struct drm_i915_private *i915) > { > struct i915_hwmon *hwmon = i915->hwmon; > + struct intel_uncore *uncore = &i915->uncore; > + intel_wakeref_t wakeref; > + u32 val_sku_unit; > > - if (IS_DG1(i915) || IS_DG2(i915)) > + if (IS_DG1(i915) || IS_DG2(i915)) { > hwmon->rg.gt_perf_status = GEN12_RPSTAT1; > - else > + 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.gt_perf_status = INVALID_MMIO_REG; > + 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; > + } > + > + with_intel_runtime_pm(uncore->rpm, wakeref) { > + /* > + * 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. > + */ Let's delete this 2nd paragraph above, if values are available per tile we should just be using per tile counters consistently (this is only true for energy in our case, that patch will need to be fixed). > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2e3aa684cf1b..fd13411a28d9 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1866,6 +1866,22 @@ > #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) Let's change this entire block above to just the following for this patch: /* *_PACKAGE_POWER_SKU - SKU power and timing parameters */ #define PKG_PKG_TDP GENMASK_ULL(14, 0) That is because none of the following #define's are used in this patch, they will be added in the patches they are used (PKG_MIN_PWR and PKG_MAX_PWR are not used at all). Also these fields are explained in i915_hwmon.c, no need to explain so much in a common header file. Thanks. -- Ashutosh
WARNING: multiple messages have this Message-ID (diff)
From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com> To: Badal Nilawar <badal.nilawar@intel.com> Cc: linux-hwmon@vger.kernel.org, intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Date: Mon, 29 Aug 2022 19:33:43 -0700 [thread overview] Message-ID: <87k06qzchk.wl-ashutosh.dixit@intel.com> (raw) In-Reply-To: <20220825132118.784407-4-badal.nilawar@intel.com> On Thu, 25 Aug 2022 06:21:14 -0700, Badal Nilawar wrote: > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 2192d0fd4c66..922463da65bf 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -13,8 +13,22 @@ > #include "intel_mchbar_regs.h" > #include "gt/intel_gt_regs.h" > > +/* > + * SF_* - scale factors for particular quantities according to hwmon spec. > + * - power - microwatts > + */ > +#define SF_POWER 1000000 > + > +#define FIELD_SHIFT(__mask) \ > + (BUILD_BUG_ON_ZERO(!__builtin_constant_p(__mask)) + \ > + BUILD_BUG_ON_ZERO((__mask) == 0) + \ > + __bf_shf(__mask)) Let's remove this macro, it's not needed, see below. > +/* > + * This function's return type of u64 allows for the case where the scaling > + * of the field taken from the 32-bit register value might cause a result to > + * exceed 32 bits. > + */ > +static u64 > +hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr, > + u32 field_msk, int field_shift, Let's remove field_shift arg. > + int nshift, u32 scale_factor) > +{ > + struct intel_uncore *uncore = ddat->uncore; > + intel_wakeref_t wakeref; > + u32 reg_value; > + > + with_intel_runtime_pm(uncore->rpm, wakeref) > + reg_value = intel_uncore_read(uncore, rgadr); > + > + reg_value = (reg_value & field_msk) >> field_shift; This is just 'REG_FIELD_GET(field_msk, reg_value)'. > + > + return mul_u64_u32_shr(reg_value, scale_factor, nshift); > +} > + > +static void > +hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, > + u32 field_msk, int field_shift, Let's remove field_shift arg. > + int nshift, unsigned int scale_factor, long lval) > +{ > + u32 nval; > + u32 bits_to_clear; > + u32 bits_to_set; > + > + /* Computation in 64-bits to avoid overflow. Round to nearest. */ > + nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); > + > + bits_to_clear = field_msk; > + bits_to_set = (nval << field_shift) & field_msk; This is just 'REG_FIELD_PREP(field_msk, nval)'. > @@ -118,11 +260,40 @@ static void > hwm_get_preregistration_info(struct drm_i915_private *i915) > { > struct i915_hwmon *hwmon = i915->hwmon; > + struct intel_uncore *uncore = &i915->uncore; > + intel_wakeref_t wakeref; > + u32 val_sku_unit; > > - if (IS_DG1(i915) || IS_DG2(i915)) > + if (IS_DG1(i915) || IS_DG2(i915)) { > hwmon->rg.gt_perf_status = GEN12_RPSTAT1; > - else > + 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.gt_perf_status = INVALID_MMIO_REG; > + 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; > + } > + > + with_intel_runtime_pm(uncore->rpm, wakeref) { > + /* > + * 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. > + */ Let's delete this 2nd paragraph above, if values are available per tile we should just be using per tile counters consistently (this is only true for energy in our case, that patch will need to be fixed). > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2e3aa684cf1b..fd13411a28d9 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1866,6 +1866,22 @@ > #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) Let's change this entire block above to just the following for this patch: /* *_PACKAGE_POWER_SKU - SKU power and timing parameters */ #define PKG_PKG_TDP GENMASK_ULL(14, 0) That is because none of the following #define's are used in this patch, they will be added in the patches they are used (PKG_MIN_PWR and PKG_MAX_PWR are not used at all). Also these fields are explained in i915_hwmon.c, no need to explain so much in a common header file. Thanks. -- Ashutosh
next prev parent reply other threads:[~2022-08-30 2:34 UTC|newest] Thread overview: 85+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-25 13:21 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-25 13:21 ` [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-26 13:30 ` Guenter Roeck 2022-08-26 13:30 ` [Intel-gfx] " Guenter Roeck 2022-08-29 17:26 ` Dixit, Ashutosh 2022-08-29 17:26 ` [Intel-gfx] " Dixit, Ashutosh 2022-08-25 13:21 ` [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-29 17:30 ` Dixit, Ashutosh 2022-08-29 17:30 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-15 14:40 ` Nilawar, Badal 2022-09-15 14:40 ` Nilawar, Badal 2022-09-21 0:02 ` Dixit, Ashutosh 2022-09-21 0:02 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-12 14:09 ` Gupta, Anshuman 2022-09-12 14:09 ` [Intel-gfx] " Gupta, Anshuman 2022-09-12 16:37 ` Dixit, Ashutosh 2022-09-12 16:37 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-13 8:11 ` Gupta, Anshuman 2022-09-13 8:11 ` [Intel-gfx] " Gupta, Anshuman 2022-09-13 15:19 ` Dixit, Ashutosh 2022-09-15 6:29 ` Gupta, Anshuman 2022-09-15 6:29 ` Gupta, Anshuman 2022-08-25 13:21 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-30 2:33 ` Dixit, Ashutosh [this message] 2022-08-30 2:33 ` Dixit, Ashutosh 2022-08-25 13:21 ` [PATCH 4/7] drm/i915/hwmon: Show device level energy usage Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-30 3:14 ` Dixit, Ashutosh 2022-08-30 3:14 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-13 8:50 ` Tvrtko Ursulin 2022-09-21 0:24 ` Dixit, Ashutosh 2022-09-21 0:24 ` Dixit, Ashutosh 2022-09-21 7:43 ` Tvrtko Ursulin 2022-09-21 7:43 ` Tvrtko Ursulin 2022-08-25 13:21 ` [PATCH 5/7] drm/i915/hwmon: Expose card reactive critical power Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-25 13:21 ` [PATCH 6/7] drm/i915/hwmon: Expose power1_max_interval Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-25 13:21 ` [PATCH 7/7] drm/i915/hwmon: Extend power/energy for XEHPSDV Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] " Badal Nilawar 2022-08-30 5:34 ` Dixit, Ashutosh 2022-08-30 5:34 ` [Intel-gfx] " Dixit, Ashutosh 2022-08-25 14:01 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add HWMON support (rev5) Patchwork 2022-08-25 14:01 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2022-08-25 14:21 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2022-08-29 20:08 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork -- strict thread matches above, loose matches on Subject: below -- 2022-10-20 1:15 [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting kernel test robot 2022-10-13 15:45 [PATCH 0/7] drm/i915: Add HWMON support Ashutosh Dixit 2022-10-13 15:45 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Ashutosh Dixit 2022-10-13 15:45 ` Ashutosh Dixit 2022-10-13 19:32 ` kernel test robot 2022-10-22 13:31 ` kernel test robot 2022-09-27 5:50 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-09-27 5:50 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-09-27 5:50 ` Badal Nilawar 2022-09-28 7:08 ` Gupta, Anshuman 2022-09-28 7:08 ` Gupta, Anshuman 2022-09-26 17:52 [PATCH 0/7] Add HWMON support Badal Nilawar 2022-09-26 17:52 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-09-26 17:52 ` Badal Nilawar 2022-09-27 8:04 ` kernel test robot 2022-09-27 13:51 ` Gupta, Anshuman 2022-09-27 13:51 ` Gupta, Anshuman 2022-09-23 19:56 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-09-23 19:56 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-09-23 19:56 ` Badal Nilawar 2022-09-16 15:00 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-09-16 15:00 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-21 0:02 ` Dixit, Ashutosh 2022-09-21 0:02 ` Dixit, Ashutosh 2022-09-21 11:45 ` Gupta, Anshuman 2022-09-21 11:45 ` Gupta, Anshuman 2022-09-21 14:53 ` Nilawar, Badal 2022-09-21 14:53 ` Nilawar, Badal 2022-09-22 7:08 ` Gupta, Anshuman 2022-09-22 7:08 ` Gupta, Anshuman 2022-09-23 2:26 ` Dixit, Ashutosh 2022-09-23 2:26 ` Dixit, Ashutosh 2022-08-18 19:38 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-08-18 19:38 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-08-12 17:37 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-08-12 17:37 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-08-12 18:06 ` Guenter Roeck 2023-02-28 21:18 ` Dixit, Ashutosh 2023-02-28 21:18 ` Dixit, Ashutosh 2023-03-09 16:33 ` Guenter Roeck 2023-03-09 16:33 ` Guenter Roeck
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87k06qzchk.wl-ashutosh.dixit@intel.com \ --to=ashutosh.dixit@intel.com \ --cc=anshuman.gupta@intel.com \ --cc=badal.nilawar@intel.com \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jon.ewins@intel.com \ --cc=linux-hwmon@vger.kernel.org \ --cc=riana.tauro@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.