Re: [PATCH v13 01/16] drm/xe/hwmon: Fix pcode power limit handling

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Oct 14, 2026 at 05:27:51PM +0530, Karthik Poosa wrote:
> Return early on pcode read failure in xe_hwmon_pcode_rmw_power_limit()
> and propagate errors to callers.
> 
> Replace boot_power_limit_read with per-channel
> read_power_limit_from_pcode[], set on the first successful pcode write,
> so reads and writes stay consistent even if the default limit write at
> boot fails.
> 
> Also return PWR_LIM_EN for power label so card label visibility works.
> 
> Fixes: 8aa7306631f0 ("drm/xe/hwmon: Fix xe_hwmon_power_max_write")
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> ---
> 
> v2: Check return in xe_hwmon_get_preregistration_info() also. (Sashiko)
> v3: Track pcode limit updates per channel. (Sashiko)
> 
>  drivers/gpu/drm/xe/xe_hwmon.c | 77 +++++++++++++++++++++--------------
>  1 file changed, 47 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 5edeac961ec3..c7f936892512 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -165,8 +165,8 @@ struct xe_hwmon {
>  	struct xe_hwmon_energy_info ei[CHANNEL_MAX];
>  	/** @fi: Fan info for fanN_input */
>  	struct xe_hwmon_fan_info fi[FAN_MAX];
> -	/** @boot_power_limit_read: is boot power limits read */
> -	bool boot_power_limit_read;
> +	/** @read_power_limit_from_pcode: pcode accepted power limits write for the channel */
> +	bool read_power_limit_from_pcode[CHANNEL_MAX];
>  	/** @pl1_on_boot: power limit PL1 on boot */
>  	u32 pl1_on_boot[CHANNEL_MAX];
>  	/** @pl2_on_boot: power limit PL2 on boot */
> @@ -175,9 +175,9 @@ struct xe_hwmon {
>  	struct xe_hwmon_thermal_info temp;
>  };
>  
> -static inline int prepare_power_limit_param2(const struct xe_hwmon *hwmon)
> +static inline int prepare_power_limit_param2(const struct xe_hwmon *hwmon, int channel)
>  {
> -	if (hwmon->boot_power_limit_read) {
> +	if (hwmon->read_power_limit_from_pcode[channel]) {
>  		if (hwmon->xe->info.platform >= XE_CRESCENTISLAND)
>  			return READ_PL_ACCEPTED;
>  		else
> @@ -198,7 +198,8 @@ static int xe_hwmon_pcode_read_power_limit(const struct xe_hwmon *hwmon, u32 att
>  						  (channel == CHANNEL_CARD) ?
>  						  READ_PSYSGPU_POWER_LIMIT :
>  						  READ_PACKAGE_POWER_LIMIT,
> -						  prepare_power_limit_param2(hwmon)), &val0, &val1);
> +						  prepare_power_limit_param2(hwmon, channel)),
> +			    &val0, &val1);
>  
>  	if (ret) {
>  		drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
> @@ -213,14 +214,14 @@ static int xe_hwmon_pcode_read_power_limit(const struct xe_hwmon *hwmon, u32 att
>  	else if (attr == PL2_HWMON_ATTR)
>  		*uval = (val1 & PWR_LIM_EN) ? val1 : 0;
>  	else if (attr == hwmon_power_label)
> -		*uval = (val0 & PWR_LIM_EN) ? 1 : (val1 & PWR_LIM_EN) ? 1 : 0;
> +		*uval = (val0 | val1) & PWR_LIM_EN;
>  	else
>  		*uval = 0;
>  
>  	return ret;
>  }
>  
> -static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr, u8 channel,
> +static int xe_hwmon_pcode_rmw_power_limit(struct xe_hwmon *hwmon, u32 attr, u8 channel,
>  					  u32 clr, u32 set)
>  {
>  	struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
> @@ -231,10 +232,13 @@ static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr
>  						  (channel == CHANNEL_CARD) ?
>  						  READ_PSYSGPU_POWER_LIMIT :
>  						  READ_PACKAGE_POWER_LIMIT,
> -						  prepare_power_limit_param2(hwmon)), &val0, &val1);
> -	if (ret)
> -		drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
> +						  prepare_power_limit_param2(hwmon, channel)),
> +			    &val0, &val1);
> +	if (ret) {
> +		drm_err(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>  			channel, val0, val1, ret);
> +		return ret;
> +	}
>  
>  	if (attr == PL1_HWMON_ATTR)
>  		val0 = (val0 & ~clr) | set;
> @@ -248,10 +252,17 @@ static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr
>  							     WRITE_PSYSGPU_POWER_LIMIT :
>  							     WRITE_PACKAGE_POWER_LIMIT, 0),
>  							     val0, val1, PCODE_DEFAULT_TIMEOUT_MS);
> -	if (ret)
> +	if (ret) {
>  		drm_dbg(&hwmon->xe->drm, "write failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>  			channel, val0, val1, ret);
> -	return ret;
> +		return ret;
> +	}
> +
> +	/* Read from pcode once it holds a driver-written limit */
> +	if (!hwmon->read_power_limit_from_pcode[channel])
> +		hwmon->read_power_limit_from_pcode[channel] = true;
> +
> +	return 0;
>  }
>  
>  static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> @@ -394,7 +405,9 @@ 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_rmw_power_limit(hwmon, attr, channel, PWR_LIM_EN, 0);
> +			ret = xe_hwmon_pcode_rmw_power_limit(hwmon, attr, channel, PWR_LIM_EN, 0);
> +			if (ret)
> +				goto unlock;
>  			xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, &reg_val);
>  		} else {
>  			reg_val = xe_mmio_rmw32(mmio, rapl_limit, PWR_LIM_EN, 0);
> @@ -679,15 +692,19 @@ 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)
> -		xe_hwmon_pcode_rmw_power_limit(hwmon, power_attr, channel, PWR_LIM_TIME, rxy);
> -	else
> +	ret = count;
> +	if (hwmon->xe->info.has_mbx_power_limits) {
> +		ret = xe_hwmon_pcode_rmw_power_limit(hwmon, power_attr, channel, PWR_LIM_TIME, rxy);
> +		if (!ret)
> +			ret = count;
> +	} 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);
>  
> -	return count;
> +	return ret;
>  }
>  
>  /* PSYS PL1 */
> @@ -1511,22 +1528,22 @@ xe_hwmon_get_preregistration_info(struct xe_hwmon *hwmon)
>  		} 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_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]);
> +			if (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]))
> +				drm_warn(&hwmon->xe->drm, "Failed to set default power limits\n");

this is wrong and ugly, please move this to a function where you can combine
the returns and handle it only once...j

>  			hwmon->scl_shift_power = PWR_UNIT;
>  			hwmon->scl_shift_energy = ENERGY_UNIT;
>  			hwmon->scl_shift_time = TIME_UNIT;
> -			hwmon->boot_power_limit_read = true;
>  		}
>  	} else {
>  		drm_info(&hwmon->xe->drm, "Using register for power limits\n");
> -- 
> 2.25.1
>