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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

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