* [Intel-gfx] [PATCH] drm/i915/hwmon: Display clamped PL1 limit
@ 2022-12-15 18:44 Ashutosh Dixit
2022-12-15 18:48 ` Jani Nikula
0 siblings, 1 reply; 4+ messages in thread
From: Ashutosh Dixit @ 2022-12-15 18:44 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
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.
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@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)
--
2.38.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Display clamped PL1 limit
2022-12-15 18:44 [Intel-gfx] [PATCH] drm/i915/hwmon: Display clamped PL1 limit Ashutosh Dixit
@ 2022-12-15 18:48 ` Jani Nikula
0 siblings, 0 replies; 4+ messages in thread
From: Jani Nikula @ 2022-12-15 18:48 UTC (permalink / raw)
To: Ashutosh Dixit, intel-gfx; +Cc: dri-devel
On Thu, 15 Dec 2022, Ashutosh Dixit <ashutosh.dixit@intel.com> 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.
The commit message lacks the most important thing: why?
BR,
Jani.
>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@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)
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Intel-gfx] [PATCH] drm/i915/hwmon: Display clamped PL1 limit
@ 2022-12-15 19:17 Ashutosh Dixit
2023-01-06 10:16 ` Gupta, Anshuman
0 siblings, 1 reply; 4+ messages in thread
From: Ashutosh Dixit @ 2022-12-15 19:17 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel
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>
---
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)
--
2.38.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Display clamped PL1 limit
2022-12-15 19:17 Ashutosh Dixit
@ 2023-01-06 10:16 ` Gupta, Anshuman
0 siblings, 0 replies; 4+ messages in thread
From: Gupta, Anshuman @ 2023-01-06 10:16 UTC (permalink / raw)
To: Ashutosh Dixit, intel-gfx; +Cc: dri-devel
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-06 10:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-15 18:44 [Intel-gfx] [PATCH] drm/i915/hwmon: Display clamped PL1 limit Ashutosh Dixit
2022-12-15 18:48 ` Jani Nikula
-- strict thread matches above, loose matches on Subject: below --
2022-12-15 19:17 Ashutosh Dixit
2023-01-06 10:16 ` Gupta, Anshuman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox