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

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

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

> 
> Raag