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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Karthik

On Tue, 2025-06-17 at 17:30 +0530, 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

This patch has non-trivial conflicts in drm-xe-fixes. Please help
backporting by helping resolving those:

dim update-branches
dim checkout drm-xe-fixes
dim cherry-pick 8aa7306631f0

Manually fix conflicts

git add <fixed files>
git cherry-pick --continue

Then please send me the resulting patch (do not push drm-xe-fixes).

Please let me know if you need assistance.

Thanks,
Thomas


> 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            | 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, &reg_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]);
>  			hwmon->scl_shift_power = PWR_UNIT;
>  			hwmon->scl_shift_energy = ENERGY_UNIT;
>  			hwmon->scl_shift_time = TIME_UNIT;