[PATCH v3] drm/xe/hwmon: Fix xe_hwmon_power_max_write
Riana Tauro
riana.tauro at intel.com
Tue Jun 17 11:33:29 UTC 2025
On 6/17/2025 3:43 PM, Poosa, Karthik wrote:
>
> On 16-06-2025 13:10, Riana Tauro wrote:
>> 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.
>
> We can't split xe_hwmon_power_max_interval_store changes in separate
> patch, as it would cause compilation issue.
Okay.
Use either one across file PWR_LIM or PWR_LIM_EN | PWR_LIM_VAL as its
same bits
You can retain my RB.
Thanks
Riana
>
>>
>> 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