Re: [PATCH v3] drm/xe/hwmon: Fix xe_hwmon_power_max_write

["Poosa, Karthik" <karthik.poosa@intel.com>]

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.

>
> 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@intel.com>
>> Signed-off-by: Karthik Poosa <karthik.poosa@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, 
>> &reg_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;
>