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

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Nov 18, 2025 at 08:42:19AM -0500, Rodrigo Vivi wrote:
> 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' ?!

This is following Ville's original comment[1] as in 'probing' for something
that may or may not exist.

[1] https://lore.kernel.org/intel-xe/aQ3xItyGMVnKdzoi@intel.com/

> 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.)

I don't even begin to qualify here so it's upto you all.

Raag