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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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