Re: [PATCH v1 1/4] drm/xe/pcode: Introduce xe_pcode_read_probe()

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

On Tue, Nov 18, 2025 at 02:29:17PM +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 xe_pcode_read_probe() which allows the caller to check
> for mailbox command support and determine if the respective functionality
> exists on the device without it being reported as an error. This is useful
> in cases where we want to make functionality decisions in the driver based
> on pcode return codes and a relatively simpler design choice compared to
> other solutions like checking pcode firmware version, which comes with
> additional 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.
> 
> This only silences command related failures and still reports other
> failures as errors, so we don't end up hiding the genuine ones.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pcode.c | 42 ++++++++++++++++++++++++-----------
>  drivers/gpu/drm/xe/xe_pcode.h |  1 +
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pcode.c b/drivers/gpu/drm/xe/xe_pcode.c
> index 0d33c14ea0cf..797b757f7a68 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.c
> +++ b/drivers/gpu/drm/xe/xe_pcode.c
> @@ -30,7 +30,7 @@
>   * - PCODE for display operations
>   */
>  
> -static int pcode_mailbox_status(struct xe_tile *tile)
> +static int pcode_mailbox_status(struct xe_tile *tile, bool probe)
>  {
>  	const char *err_str;
>  	int err_decode;
> @@ -57,8 +57,12 @@ 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 (probe && err_decode == -ENXIO)
> +			drm_dbg(&tile_to_xe(tile)->drm, "PCODE Mailbox unsupported: %d %s",
> +				err_decode, err_str);
> +		else
> +			drm_err(&tile_to_xe(tile)->drm, "PCODE Mailbox failed: %d %s",
> +				err_decode, err_str);
>  
>  		return err_decode;
>  	}
> @@ -69,7 +73,7 @@ static int pcode_mailbox_status(struct xe_tile *tile)
>  
>  static int __pcode_mailbox_rw(struct xe_tile *tile, u32 mbox, u32 *data0, u32 *data1,
>  			      unsigned int timeout_ms, bool return_data,
> -			      bool atomic)
> +			      bool atomic, bool probe)
>  {
>  	struct xe_mmio *mmio = &tile->mmio;
>  	int err;
> @@ -95,19 +99,20 @@ 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, probe);
>  }
>  
>  static int pcode_mailbox_rw(struct xe_tile *tile, u32 mbox, u32 *data0, u32 *data1,
>  			    unsigned int timeout_ms, bool return_data,
> -			    bool atomic)
> +			    bool atomic, bool probe)
>  {
>  	if (tile_to_xe(tile)->info.skip_pcode)
>  		return 0;
>  
>  	lockdep_assert_held(&tile->pcode.lock);
>  
> -	return __pcode_mailbox_rw(tile, mbox, data0, data1, timeout_ms, return_data, atomic);
> +	return __pcode_mailbox_rw(tile, mbox, data0, data1, timeout_ms, return_data,
> +				  atomic, probe);
>  }
>  
>  int xe_pcode_write_timeout(struct xe_tile *tile, u32 mbox, u32 data, int timeout)
> @@ -115,7 +120,7 @@ int xe_pcode_write_timeout(struct xe_tile *tile, u32 mbox, u32 data, int timeout
>  	int err;
>  
>  	mutex_lock(&tile->pcode.lock);
> -	err = pcode_mailbox_rw(tile, mbox, &data, NULL, timeout, false, false);
> +	err = pcode_mailbox_rw(tile, mbox, &data, NULL, timeout, false, false, false);
>  	mutex_unlock(&tile->pcode.lock);
>  
>  	return err;
> @@ -126,7 +131,7 @@ int xe_pcode_write64_timeout(struct xe_tile *tile, u32 mbox, u32 data0, u32 data
>  	int err;
>  
>  	mutex_lock(&tile->pcode.lock);
> -	err = pcode_mailbox_rw(tile, mbox, &data0, &data1, timeout, false, false);
> +	err = pcode_mailbox_rw(tile, mbox, &data0, &data1, timeout, false, false, false);
>  	mutex_unlock(&tile->pcode.lock);
>  
>  	return err;
> @@ -137,7 +142,18 @@ int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1)
>  	int err;
>  
>  	mutex_lock(&tile->pcode.lock);
> -	err = pcode_mailbox_rw(tile, mbox, val, val1, 1, true, false);
> +	err = pcode_mailbox_rw(tile, mbox, val, val1, 1, true, false, false);
> +	mutex_unlock(&tile->pcode.lock);
> +
> +	return err;
> +}
> +
> +int xe_pcode_read_probe(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1)
> +{
> +	int err;
> +
> +	mutex_lock(&tile->pcode.lock);
> +	err = pcode_mailbox_rw(tile, mbox, val, val1, 1, true, false, true);
>  	mutex_unlock(&tile->pcode.lock);
>  
>  	return err;
> @@ -154,10 +170,10 @@ static int pcode_try_request(struct xe_tile *tile, u32 mbox,
>  	for (slept = 0; slept < timeout_us; slept += wait) {
>  		if (locked)
>  			*status = pcode_mailbox_rw(tile, mbox, &request, NULL, 1, true,
> -						   atomic);
> +						   atomic, false);
>  		else
>  			*status = __pcode_mailbox_rw(tile, mbox, &request, NULL, 1, true,
> -						     atomic);
> +						     atomic, false);
>  		if ((*status == 0) && ((request & reply_mask) == reply))
>  			return 0;
>  
> @@ -268,7 +284,7 @@ int xe_pcode_init_min_freq_table(struct xe_tile *tile, u32 min_gt_freq,
>  		u32 data = freq << PCODE_FREQ_RING_RATIO_SHIFT | freq;
>  
>  		ret = pcode_mailbox_rw(tile, PCODE_WRITE_MIN_FREQ_TABLE,
> -				       &data, NULL, 1, false, false);
> +				       &data, NULL, 1, false, false, false);
>  		if (ret)
>  			goto unlock;
>  	}
> diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
> index a5584c1c75f9..688eca8f24e6 100644
> --- a/drivers/gpu/drm/xe/xe_pcode.h
> +++ b/drivers/gpu/drm/xe/xe_pcode.h
> @@ -18,6 +18,7 @@ int xe_pcode_ready(struct xe_device *xe, bool locked);
>  int xe_pcode_init_min_freq_table(struct xe_tile *tile, u32 min_gt_freq,
>  				 u32 max_gt_freq);
>  int xe_pcode_read(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1);
> +int xe_pcode_read_probe(struct xe_tile *tile, u32 mbox, u32 *val, u32 *val1);

Is "probe" the right condition? I mean, the right name for the exported function?

The caller is deciding to downgrade the Illegal command from error to debug, but
is it because it is in the probe? Or because we know that most of FW out there
might not have this command yet and driver knows that and will handle the
lack of backward compatibility properly... in a way that this is not an error.

But is this 'probe' ?!

I'm bad with naming as well, so asking help from AI:

Alternative naming ideas:
xe_pcode_read_optional
(Indicates the command is optional and failure is acceptable.)
xe_pcode_read_safe
(Suggests a safe read that won't break if unsupported.)
xe_pcode_read_tolerant
(Highlights tolerance for missing command.)
xe_pcode_try_read
(Common pattern for non-fatal attempts.)
xe_pcode_read_if_supported
(Explicit about conditional support.)

>  int xe_pcode_write_timeout(struct xe_tile *tile, u32 mbox, u32 val,
>  			   int timeout_ms);
>  int xe_pcode_write64_timeout(struct xe_tile *tile, u32 mbox, u32 data0,
> -- 
> 2.43.0
>