Re: [PATCH v2 1/4] drm/xe/pcode: Introduce PCODE_FALLBACK_MAGIC

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

On Mon, Dec 01, 2025 at 03:57:55PM +0530, Raag Jadav wrote:
> If the device is running older PCODE firmware, it is possible that newer
> mailbox commands are not supported by it. The respective functionality
> isn't useful in that case but nor is error logging, as it doesn't
> particularly signify anything wrong with PCODE firmware or device as a
> whole. Introduce PCODE_FALLBACK_MAGIC bit which allows checking for
> mailbox command support without it being reported as a fatal error. This
> is useful in cases where we want to make functionality decisions in the
> driver and allow the caller to determine the fallback path based on PCODE
> return codes. This is a relatively simpler design choice compared to other
> solutions like checking PCODE firmware version, which comes with additional

If we have the ability to read the PCODE firmware version and handle it properly
we should. This explanation is not valid.

> complexity of binding every single command to a specific version and deems
> PCODE commands useless in case the driver fails to obtain it for reasons
> unrelated to PCODE.
> 
> The fallback is only allowed for command and subcommand failures and other
> failures are still reported as fatal errors, so we don't end up hiding the
> genuine ones.
> 
> v2: Convert fallback cases to -EOPNOTSUPP (Lucas)
>     Use xe_tile_err() (Michal)
> 
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pcode.c     | 47 ++++++++++++++++++++++---------
>  drivers/gpu/drm/xe/xe_pcode_api.h |  1 +
>  2 files changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 0d33c14ea0cf..4078c4aa2b35 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -15,6 +15,7 @@
>  #include "xe_device.h"
>  #include "xe_mmio.h"
>  #include "xe_pcode_api.h"
> +#include "xe_tile_printk.h"
>  
>  /**
>   * DOC: PCODE
> @@ -25,12 +26,28 @@
>   * single and consolidated place that will communicate with PCODE. All read
>   * and write operations to PCODE will be internal and private to this component.
>   *
> + * The usual PCODE return codes translate to:
> + * - 0: "Command Success"
> + * - -ENXIO: "Illegal Command"
> + * - -ETIMEDOUT: "Timed out"
> + * - -EINVAL: "Illegal Data"
> + * - -ENXIO, "Illegal Subcommand"
> + * - -EBUSY: "PCODE Locked"
> + * - -EOVERFLOW, "GT ratio out of range"
> + * - -EACCES, "PCODE Rejected"
> + * - -EPROTO, "Unknown"
> + *
> + * All failure are reported as fatal errors except -EOPNOTSUPP, which is returned only
> + * when PCODE_FALLBACK_MAGIC is embedded with mailbox command. This is facilitated to
> + * allow the caller to check for mailbox command support without it being reported as
> + * a fatal error.
> + *
>   * What's next:
>   * - PCODE hw metrics
>   * - PCODE for display operations
>   */
>  
> -static int pcode_mailbox_status(struct xe_tile *tile)
> +static int pcode_mailbox_status(struct xe_tile *tile, bool fallback)
>  {
>  	const char *err_str;
>  	int err_decode;
> @@ -57,8 +74,14 @@ static int pcode_mailbox_status(struct xe_tile *tile)
>  	}
>  
>  	if (err) {
> -		drm_err(&tile_to_xe(tile)->drm, "PCODE Mailbox failed: %d %s",
> -			err_decode, err_str);
> +		if (fallback && err_decode == -ENXIO)
> +			/*
> +			 * Command/Subcommand is not supported, allow the caller to fallback
> +			 * instead of reporting a fatal error.
> +			 */
> +			err_decode = -EOPNOTSUPP;
> +		else
> +			xe_tile_err(tile, "PCODE Mailbox failed: %d %s", err_decode, err_str);
>  
>  		return err_decode;
>  	}
> @@ -72,6 +95,7 @@ static int __pcode_mailbox_rw(struct xe_tile *tile, u32 mbox, u32 *data0, u32 *d
>  			      bool atomic)
>  {
>  	struct xe_mmio *mmio = &tile->mmio;
> +	bool fallback;
>  	int err;
>  
>  	if (tile_to_xe(tile)->info.skip_pcode)
> @@ -80,6 +104,10 @@ static int __pcode_mailbox_rw(struct xe_tile *tile, u32 mbox, u32 *data0, u32 *d
>  	if ((xe_mmio_read32(mmio, PCODE_MAILBOX) & PCODE_READY) != 0)
>  		return -EAGAIN;
>  
> +	fallback = REG_FIELD_GET(PCODE_FALLBACK_MAGIC, mbox);
> +	if (fallback)
> +		mbox &= ~PCODE_FALLBACK_MAGIC;
> +
>  	xe_mmio_write32(mmio, PCODE_DATA0, *data0);
>  	xe_mmio_write32(mmio, PCODE_DATA1, data1 ? *data1 : 0);
>  	xe_mmio_write32(mmio, PCODE_MAILBOX, PCODE_READY | mbox);
> @@ -95,7 +123,7 @@ static int __pcode_mailbox_rw(struct xe_tile *tile, u32 mbox, u32 *data0, u32 *d
>  			*data1 = xe_mmio_read32(mmio, PCODE_DATA1);
>  	}
>  
> -	return pcode_mailbox_status(tile);
> +	return pcode_mailbox_status(tile, fallback);
>  }
>  
>  static int pcode_mailbox_rw(struct xe_tile *tile, u32 mbox, u32 *data0, u32 *data1,
> @@ -240,16 +268,7 @@ int xe_pcode_request(struct xe_tile *tile, u32 mbox, u32 request,
>   * not take the right decisions for some memory frequencies and affect latency.
>   *
>   * It returns 0 on success, and -ERROR number on failure, -EINVAL if max
> - * frequency is higher then the minimal, and other errors directly translated
> - * from the PCODE Error returns:
> - * - -ENXIO: "Illegal Command"
> - * - -ETIMEDOUT: "Timed out"
> - * - -EINVAL: "Illegal Data"
> - * - -ENXIO, "Illegal Subcommand"
> - * - -EBUSY: "PCODE Locked"
> - * - -EOVERFLOW, "GT ratio out of range"
> - * - -EACCES, "PCODE Rejected"
> - * - -EPROTO, "Unknown"
> + * frequency is higher then the minimal.
>   */
>  int xe_pcode_init_min_freq_table(struct xe_tile *tile, u32 min_gt_freq,
>  				 u32 max_gt_freq)
> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
> index 70dcd6625680..a5157966478a 100644
> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
> @@ -9,6 +9,7 @@
>  
>  #define PCODE_MAILBOX			XE_REG(0x138124)
>  #define   PCODE_READY			REG_BIT(31)
> +#define   PCODE_FALLBACK_MAGIC		REG_BIT(30)	/* Driver use only, not used by PCODE */

I don't like 'magic' code. I don't like the fact that we are using
a reserved bit.
Let's handle it properly, without magic please.

>  #define   PCODE_MB_PARAM2		REG_GENMASK(23, 16)
>  #define   PCODE_MB_PARAM1		REG_GENMASK(15, 8)
>  #define   PCODE_MB_COMMAND		REG_GENMASK(7, 0)
> -- 
> 2.43.0
>