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

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

On Tue, Nov 25, 2025 at 12:41:55PM -0500, Rodrigo Vivi wrote:
> On Tue, Nov 25, 2025 at 06:04:59AM +0100, Raag Jadav wrote:
> > On Fri, Nov 21, 2025 at 09:33:42AM +0100, Raag Jadav wrote:
> > > On Thu, Nov 20, 2025 at 09:02:29AM -0600, Lucas De Marchi wrote:
> > > > On Tue, Nov 18, 2025 at 04:38:46PM +0100, Raag Jadav wrote:
> > 
> > ...
> > 
> > > > > > xe_pcode_read_if_supported
> > > > > > (Explicit about conditional support.)
> > > > > 
> > > > > I don't even begin to qualify here so it's upto you all.
> > > > 
> > > > xe_pcode_read_if_supported() would be ok IMO, documenting it to mask
> > > > not-supported errors.
> > > > 
> > > > But the the way this is implemented with the extra flag seems weird.
> > > > By "having the caller check" I think it's about handling
> > > > the return code from this function and treating it as a fatal or normal
> > > > case depending on the command being sent, if there's a fallback etc.
> > > > This patch seems to add a function and not used it, but I may be missing
> > > > something.
> > > 
> > > Forgot to doc. I had an impression that -ENXIO could be used for the
> > > fallback since we already have it here but ...
> > > 
> > > > I'd rather have this:
> > > > 
> > > > 1) Caller should handle errors and treat it as fatal or normal,
> > > >    depending on having a fallback or not. Emit an err there if
> > > >    appropriate rather than here. It seems we are already emitting
> > > >    additional dbgs in the caller for when pcode_read fails
> > > > 
> > > > 2) What is the command/subcommand triggering this error? We could have a
> > > >    helper like xe_pcode_strerr() that users could call if needed (but
> > > >    then we'd need to return the undecoded error), or we could change
> > > >    this specific return code to -ENOTSUPP.
> > > 
> > > ... converting to -ENOTSUPP makes much more sense, considering the undecoded
> > > return will be inconsistent with other pcode helpers.
> > 
> > On second thought, looking at it from caller standpoint, I'm wondering why
> > this shouldn't be the expectation with xe_pcode_read() itself?
> > 
> > I'm okay with the new helper but if we're claiming that something happens
> > *only* if supported, I'd expect it to be the default behaviour instead of
> > having extra bells and whistles. The only difference here is how the caller
> > chooses to treat it (-ENOTSUPP) anyway.
> > 
> > Thoughts?
> 
> It makes sense to me. I'd just peak into the history to see if there was any
> documented reason for the current code or just some bad decision/judgement
> from my side when adding that... if so, I'm sorry in advance...

Don't get me wrong, I still fully believe that we should not let this be
an excuse for callers to attempt random commands and abuse the mailbox.
So for now we facilitate the fallback for xe_pcode_read() only and let
other helpers operate as they are.

With that we'll also have the flexibility to change the behaviour per
helper in the future.

Sound okay?

Raag