[PATCH v3] drm/xe/hwmon: Fix xe_hwmon_power_max_write
Poosa, Karthik
karthik.poosa at intel.com
Wed Jun 18 09:49:15 UTC 2025
On 17-06-2025 17:03, Riana Tauro wrote:
>
>
> 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
Changed this to PMR_LIM in next revision.
>
> 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