[Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP

[Teres Alexis, Alan Previn]

On Fri, 2023-05-05 at 00:39 -0700, Justen, Jordan L wrote:
> On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote:
> > On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote:
> > > Because of the additional firmware, component-driver and
> > > initialization depedencies required on MTL platform before a
> > > PXP context can be created, UMD calling for PXP creation as a
> > > way to get-caps can take a long time. An actual real world
> > > customer stack has seen this happen in the 4-to-8 second range
> > > after the kernel starts (which sees MESA's init appear in the
> > > middle of this range as the compositor comes up). To avoid
> > > unncessary delays experienced by the UMD for get-caps purposes,
> > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT.
> > > 
> > alan:snip.
> > Progress update on the UMD side - I'm working on patch for PR here: 
> > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57
> > but taking time to verify certain code paths
> 
> Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready
> soon"), then it is basically certain that in a production environment,
> then it will eventually return 1 meaning it's ready, right?
alan: yes - but not 100%. non-platform-state-machine could still
cause unexpected failures for example, [1] if hw was fused in a way
that doesnt permit PXP or [2] enabling certain BIOS debug knobs
doesnt allow PXP. However at the moment, there is no way for us
to know for sure without actually creating a protected context.
Of course having hw-fusing + bios-config that supports PXP have
always 100% succeeded for me.

> 
> If this is correct, then I think that the change in
> i915_gem_supports_protected_context() is good, and probably we can
> skip the change in iris_create_hw_context().
> 
> Basically, we're timing out for multiple seconds either way. And, I'm
> hoping that the kernel will eventually get the PXP init done and
> create the context.
> 
> I think there's 2 cases of interest here.
> 
> First, and most likely, the application running doesn't care about
> protected content. In this case we can quickly advertise the support,
> but there will be no consequence because the application won't use the
> feature.
alan: yes - that was one of the goals of this UAPI.
> 
> Second, the application does care about protected content. In this
> case we can quickly advertise the support, but if the feature is used
> too quickly, then the context create might take a long time.
alan: no, that isnt the case now, we started at 8-secs, then 2-secs,
and finally settled on the same timeout as ADL since this new UAPI
will be something that can be polled on to be sure we have readiness
to make the create-context-call. That's why i wanted to add the
polling wait in the actual create call - but not the get caps call
which can be quick (as you said above).

> 
> If I915_PARAM_PXP_STATUS returning 2 has a reasonable chance in a
> production environment of eventually finding out that pxp can't work,
> then perhaps more disussion is needed. I guess the worst case scenario
> is no worse than today though. (Except it is still somewhat better,
> because the common case would not involve protected content being
> used by the application.)
alan: hmmm... i guess it depends on the meaning of resonable: if 50%
of the CI platforms being run have incorrect bios config / hw fusing
does it mean this UAPI is only 50% useful? My opinion is the alternative:
in the case of platform that has correctly configured BIOS + fusing,
is the chance of 2 eventually leading to a failure high? The answer is no.
Hypothetically i have actually never seen this happen (note: this UAPI
is new but i know from past debug of customer issues). There are some very
corner-cases but those get into the weeds of pxp runtime state machine..
I am sure we don't wanna discuss that rabbit hole right now.
> 
> Another option besides from the timeout loop in
> iris_create_hw_context() might be to check I915_PARAM_PXP_STATUS after
> the context create fails to tweak the debug message.
alan: Yeah, that is an option - I'm thinking we can add a DBG that reads
either"PXP context failure expected due not ready" vs
"Unexpected PXP context failure" 

> 
> -Jordan