[PATCH v3] drm/xe/hwmon: Fix xe_hwmon_power_max_write
Riana Tauro
riana.tauro at intel.com
Mon Jun 16 07:40:03 UTC 2025
Hi Karthik
On 6/16/2025 12:41 PM, 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.
>
Since you are making changes to power interval also .Split the patches
into two. One to add rmw and make changes to existing code. The other as
the new fix.
Thanks
Riana
> 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.
>
> 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 | 48 ++++++++++++------------
> 2 files changed, 24 insertions(+), 25 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..fa841311bdcf 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,7 +378,8 @@ 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_EN | PWR_LIM_VAL,
> + reg_val);
> else
> reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN | PWR_LIM_VAL,
> reg_val);
> @@ -591,14 +592,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 +1215,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]);
> 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