Re: [PATCH 2/2] drm/xe/hwmon: Increase timeout for mailbox power limits

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

On Wed, Jul 29, 2026 at 10:45:46PM +0530, Karthik Poosa wrote:
> Mailbox power limit operations can occasionally take longer than the
> current 1 ms timeout, causing sporadic failures.
> Increase the timeout to 10 ms and rename PL_WRITE_MBX_TIMEOUT_MS to
> PL_MBX_TIMEOUT_MS to better reflect its usage.
> 
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hwmon.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index de3f2aeffc3f..459ad953dfc8 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -102,9 +102,9 @@ enum sensor_attr_power {
>  #define PWR_ATTR_TO_STR(attr)	(((attr) == hwmon_power_max) ? "PL1" : "PL2")
>  
>  /*
> - * Timeout for power limit write mailbox command.
> + * Timeout for power limit mailbox commands.

while at it, please remove this useless comments all over.
I know it pre-dates AI, but it looks like AI code, commenting the obvious,
which is already clear in the define name...

>   */
> -#define PL_WRITE_MBX_TIMEOUT_MS	(1)
> +#define PL_MBX_TIMEOUT_MS	(10)
>  
>  /* Index of memory controller in READ_THERMAL_DATA output */
>  #define TEMP_INDEX_MCTRL	2
> @@ -199,11 +199,12 @@ static int xe_hwmon_pcode_read_power_limit(const struct xe_hwmon *hwmon, u32 att
>  	u32 val0 = 0, val1 = 0;
>  	int ret = 0;
>  
> -	ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_POWER_SETUP,
> -						  (channel == CHANNEL_CARD) ?
> -						  READ_PSYSGPU_POWER_LIMIT :
> -						  READ_PACKAGE_POWER_LIMIT,
> -						  prepare_power_limit_param2(hwmon)), &val0, &val1);
> +	ret = xe_pcode_read_timeout(root_tile, PCODE_MBOX(PCODE_POWER_SETUP,
> +							  (channel == CHANNEL_CARD) ?
> +							  READ_PSYSGPU_POWER_LIMIT :
> +							  READ_PACKAGE_POWER_LIMIT,
> +							  prepare_power_limit_param2(hwmon)),
> +							  &val0, &val1, PL_MBX_TIMEOUT_MS);
>  
>  	if (ret) {
>  		drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
> @@ -232,11 +233,12 @@ static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr
>  	u32 val0 = 0, val1 = 0;
>  	int ret = 0;
>  
> -	ret = xe_pcode_read(root_tile, PCODE_MBOX(PCODE_POWER_SETUP,
> -						  (channel == CHANNEL_CARD) ?
> -						  READ_PSYSGPU_POWER_LIMIT :
> -						  READ_PACKAGE_POWER_LIMIT,
> -						  prepare_power_limit_param2(hwmon)), &val0, &val1);
> +	ret = xe_pcode_read_timeout(root_tile, PCODE_MBOX(PCODE_POWER_SETUP,
> +							  (channel == CHANNEL_CARD) ?
> +							  READ_PSYSGPU_POWER_LIMIT :
> +							  READ_PACKAGE_POWER_LIMIT,
> +							  prepare_power_limit_param2(hwmon)),
> +							  &val0, &val1, PL_MBX_TIMEOUT_MS);
>  	if (ret)
>  		drm_dbg(&hwmon->xe->drm, "read failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>  			channel, val0, val1, ret);
> @@ -252,7 +254,7 @@ static int xe_hwmon_pcode_rmw_power_limit(const struct xe_hwmon *hwmon, u32 attr
>  							     (channel == CHANNEL_CARD) ?
>  							     WRITE_PSYSGPU_POWER_LIMIT :
>  							     WRITE_PACKAGE_POWER_LIMIT, 0),
> -							     val0, val1, PL_WRITE_MBX_TIMEOUT_MS);
> +							     val0, val1, PL_MBX_TIMEOUT_MS);
>  	if (ret)
>  		drm_dbg(&hwmon->xe->drm, "write failed ch %d val0 0x%08x, val1 0x%08x, ret %d\n",
>  			channel, val0, val1, ret);
> -- 
> 2.25.1
>