On 27/03/2023 08:07, Lionel Landwerlin wrote:
On 26/03/2023 14:18, Rodrigo Vivi wrote:
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.
These two:
e6177ec586d1 ("drm/i915/huc: stall media submission until HuC is loaded")
b76c14c8fb2a ("drm/i915/huc: better define HuC status getparam possible return values.")
They do not help here? It is not possible to use or extend the refined I915_PARAM_HUC_STATUS return values combined with huc load fence for this all to keep working?
Regards,
Tvrtko
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.
I like option 2 better because we have to report support as fast as we
can when enumerating devices on the system for example.
If I understand correctly, with the get param, most apps won't ever be
blocking on any PXP stuff if they don't use it.
Only the ones that require protected support might block.
-Lionel
