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 04:38:46PM +0100, Raag Jadav wrote:
> 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.

Ville, thoughts?

> 
> Raag