Re: [Intel-gfx] [PATCH v6 5/8] drm/i915/pxp: Add ARB session creation and cleanup

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

On Sat, Mar 25, 2023 at 02:19:21AM -0400, Teres Alexis, Alan Previn wrote:
> alan:snip
> 
> > 
> @@ -353,8 +367,20 @@ int intel_pxp_start(struct intel_pxp *pxp)
> alan:snip
> > > +	if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
> > > +		/*
> > > +		 * GSC-fw loading, GSC-proxy init (requiring an mei component driver) and
> > > +		 * HuC-fw loading must all occur first before we start requesting for PXP
> > > +		 * sessions. Checking HuC authentication (the last dependency)  will suffice.
> > > +		 * Let's use a much larger 8 second timeout considering all the types of
> > > +		 * dependencies prior to that.
> > > +		 */
> > > +		if (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 8000))
> > 
> > This big timeout needs an ack from userspace drivers, as intel_pxp_start 
> > is called during context creation and the current way to query if the 
> > feature is supported is to create a protected context. Unfortunately, we 
> > do need to wait to confirm that PXP is available (although in most cases 
> > it shouldn't take even close to 8 secs), because until everything is 
> > setup we're not sure if things will work as expected. I see 2 potential 
> > mitigations in case the timeout doesn't work as-is:
> > 
> > 1) we return -EAGAIN (or another dedicated error code) to userspace if 
> > the prerequisite steps aren't done yet. This would indicate that the 
> > feature is there, but that we haven't completed the setup yet. The 
> > caller can then decide if they want to retry immediately or later. Pro: 
> > more flexibility for userspace; Cons: new interface return code.
> > 
> > 2) we add a getparam to say if PXP is supported in HW and the support is 
> > compiled in i915. Userspace can query this as a way to check the feature 
> > support and only create the context if they actually need it for PXP 
> > operations. Pro: simpler kernel implementation; Cons: new getparam, plus 
> > even if the getparam returns true the pxp_start could later fail, so 
> > userspace needs to handle that case.
> > 
> 
> alan: I've cc'd Rodrigo, Joonas and Lionel. Folks - what are your thoughts on above issue?
> Recap: On MTL, only when creating a GEM Protected (PXP) context for the very first time after
> a driver load, it will be dependent on (1) loading the GSC firmware, (2) GuC loading the HuC
> firmware and (3) GSC authenticating the HuC fw. But step 3 also depends on additional
> GSC-proxy-init steps that depend on a new mei-gsc-proxy component driver. I'd used the
> 8 second number based on offline conversations with Daniele but that is a worse-case.
> Alternatively, should we change UAPI instead to return -EAGAIN as per Daniele's proposal?
> I believe we've had the get-param conversation offline recently and the direction was to
> stick with attempting to create the context as it is normal in 3D UMD when it comes to
> testing capabilities for other features too.
> 
> Thoughts?

I like the option 1 more. This extra return handling won't break compatibility.