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

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

On Thu, Nov 27, 2025 at 05:51:29AM +0100, Raag Jadav wrote:
> 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?

I was thinking through this over the weekend and came up with another
hack. Hold on for v2.

Raag