[PATCH v4] drm/xe/hwmon: Fix xe_hwmon_power_max_write
Nilawar, Badal
badal.nilawar at intel.com
Tue Jun 17 12:03:06 UTC 2025
On 17-06-2025 17:30, Karthik Poosa wrote:
> Prevent other bits of mailbox power limit from being overwritten with 0.
> This issue was due to a missing read and modify of current power limit,
> before setting a requested mailbox power limit, which is added in this
> patch.
>
> v2:
> - Improve commit message. (Anshuman)
>
> v3:
> - Rebase.
> - Rephrase commit message. (Riana)
> - Add read-modify-write variant of xe_hwmon_pcode_write_power_limit()
> i.e. xe_hwmon_pcode_rmw_power_limit(). (Badal)
> - Use xe_hwmon_pcode_rmw_power_limit() to set mailbox power limits.
> - Remove xe_hwmon_pcode_write_power_limit() as all mailbox power limits
> writes use xe_hwmon_pcode_rmw_power_limit() only.
>
> v4:
> - Use PWR_LIM in place of (PWR_LIM_EN | PWR_LIM_VAL) wherever
> applicable. (Riana)
>
> Fixes: 7596d839f6228 ("drm/xe/hwmon: Add support to manage power limits though mailbox")
> Reviewed-by: Riana Tauro <riana.tauro at intel.com>
> Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
> ---
> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 1 +
> drivers/gpu/drm/xe/xe_hwmon.c | 50 +++++++++++-------------
> 2 files changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> index 5394a1373a6b..ef2bf984723f 100644
> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> @@ -40,6 +40,7 @@
> #define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> #define PWR_LIM_VAL REG_GENMASK(14, 0)
> #define PWR_LIM_EN REG_BIT(15)
> +#define PWR_LIM REG_GENMASK(15, 0)
> #define PWR_LIM_TIME REG_GENMASK(23, 17)
> #define PWR_LIM_TIME_X REG_GENMASK(23, 22)
> #define PWR_LIM_TIME_Y REG_GENMASK(21, 17)
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 0d32e977537c..f08fc4377d25 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -175,8 +175,8 @@ static int xe_hwmon_pcode_read_power_limit(const struct xe_hwmon *hwmon, u32 att
> return ret;
> }
>
> -static int xe_hwmon_pcode_write_power_limit(const struct xe_hwmon *hwmon, u32 attr, u8 channel,
> - u32 uval)
> +static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr, u8 channel,
> + u32 clr, u32 set)
> {
> struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
> u32 val0, val1;
> @@ -195,9 +195,9 @@ static int xe_hwmon_pcode_write_power_limit(const struct xe_hwmon *hwmon, u32 at
> channel, val0, val1, ret);
>
> if (attr == PL1_HWMON_ATTR)
> - val0 = uval;
> + val0 = (val0 & ~clr) | set;
> else if (attr == PL2_HWMON_ATTR)
> - val1 = uval;
> + val1 = (val1 & ~clr) | set;
> else
> return -EIO;
>
> @@ -342,7 +342,7 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, u32 attr, int channe
> if (hwmon->xe->info.has_mbx_power_limits) {
> drm_dbg(&hwmon->xe->drm, "disabling %s on channel %d\n",
> PWR_ATTR_TO_STR(attr), channel);
> - xe_hwmon_pcode_write_power_limit(hwmon, attr, channel, 0);
> + xe_hwmon_pcode_rmw_power_limit(hwmon, attr, channel, PWR_LIM_EN, 0);
> xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, ®_val);
> } else {
> reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN, 0);
> @@ -378,10 +378,9 @@ static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, u32 attr, int channe
> reg_val = PWR_LIM_EN | REG_FIELD_PREP(PWR_LIM_VAL, reg_val);
>
> if (hwmon->xe->info.has_mbx_power_limits)
> - ret = xe_hwmon_pcode_write_power_limit(hwmon, attr, channel, reg_val);
> + ret = xe_hwmon_pcode_rmw_power_limit(hwmon, attr, channel, PWR_LIM, reg_val);
> else
> - reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN | PWR_LIM_VAL,
> - reg_val);
> + reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM, reg_val);
> unlock:
> mutex_unlock(&hwmon->hwmon_lock);
> return ret;
> @@ -591,14 +590,11 @@ xe_hwmon_power_max_interval_store(struct device *dev, struct device_attribute *a
>
> mutex_lock(&hwmon->hwmon_lock);
>
> - if (hwmon->xe->info.has_mbx_power_limits) {
> - ret = xe_hwmon_pcode_read_power_limit(hwmon, power_attr, channel, (u32 *)&r);
> - r = (r & ~PWR_LIM_TIME) | rxy;
> - xe_hwmon_pcode_write_power_limit(hwmon, power_attr, channel, r);
> - } else {
> + if (hwmon->xe->info.has_mbx_power_limits)
> + xe_hwmon_pcode_rmw_power_limit(hwmon, power_attr, channel, PWR_LIM_TIME, rxy);
> + else
> r = xe_mmio_rmw32(mmio, xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel),
> PWR_LIM_TIME, rxy);
> - }
>
> mutex_unlock(&hwmon->hwmon_lock);
>
> @@ -1217,25 +1213,25 @@ xe_hwmon_get_preregistration_info(struct xe_hwmon *hwmon)
> &hwmon->pl1_on_boot[CHANNEL_PKG]) |
> xe_hwmon_pcode_read_power_limit(hwmon, PL2_HWMON_ATTR, CHANNEL_CARD,
> &hwmon->pl2_on_boot[CHANNEL_CARD]) |
> - xe_hwmon_pcode_read_power_limit(hwmon, PL1_HWMON_ATTR, CHANNEL_PKG,
> + xe_hwmon_pcode_read_power_limit(hwmon, PL2_HWMON_ATTR, CHANNEL_PKG,
> &hwmon->pl2_on_boot[CHANNEL_PKG])) {
> drm_warn(&hwmon->xe->drm,
> "Failed to read power limits, check GPU firmware !\n");
> } else {
> drm_info(&hwmon->xe->drm, "Using mailbox commands for power limits\n");
> /* Write default limits to read from pcode from now on. */
> - xe_hwmon_pcode_write_power_limit(hwmon, PL1_HWMON_ATTR,
> - CHANNEL_CARD,
> - hwmon->pl1_on_boot[CHANNEL_CARD]);
> - xe_hwmon_pcode_write_power_limit(hwmon, PL1_HWMON_ATTR,
> - CHANNEL_PKG,
> - hwmon->pl1_on_boot[CHANNEL_PKG]);
> - xe_hwmon_pcode_write_power_limit(hwmon, PL2_HWMON_ATTR,
> - CHANNEL_CARD,
> - hwmon->pl2_on_boot[CHANNEL_CARD]);
> - xe_hwmon_pcode_write_power_limit(hwmon, PL2_HWMON_ATTR,
> - CHANNEL_PKG,
> - hwmon->pl2_on_boot[CHANNEL_PKG]);
> + xe_hwmon_pcode_rmw_power_limit(hwmon, PL1_HWMON_ATTR,
> + CHANNEL_CARD, PWR_LIM | PWR_LIM_TIME,
> + hwmon->pl1_on_boot[CHANNEL_CARD]);
> + xe_hwmon_pcode_rmw_power_limit(hwmon, PL1_HWMON_ATTR,
> + CHANNEL_PKG, PWR_LIM | PWR_LIM_TIME,
> + hwmon->pl1_on_boot[CHANNEL_PKG]);
> + xe_hwmon_pcode_rmw_power_limit(hwmon, PL2_HWMON_ATTR,
> + CHANNEL_CARD, PWR_LIM | PWR_LIM_TIME,
> + hwmon->pl2_on_boot[CHANNEL_CARD]);
> + xe_hwmon_pcode_rmw_power_limit(hwmon, PL2_HWMON_ATTR,
> + CHANNEL_PKG, PWR_LIM | PWR_LIM_TIME,
> + hwmon->pl2_on_boot[CHANNEL_PKG]);
Looks good to me.
Reviewed-by: Badal Nilawar <badal.nilawar at intel.com>
> hwmon->scl_shift_power = PWR_UNIT;
> hwmon->scl_shift_energy = ENERGY_UNIT;
> hwmon->scl_shift_time = TIME_UNIT;
More information about the Intel-xe
mailing list