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

["Souza, Jose" <jose.souza@intel.com>]

On Mon, 2023-05-08 at 17:49 +0000, Teres Alexis, Alan Previn wrote:
> 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.

In my opinion it is problematic mark that we support protected context but then it fails to create protected context.

It should check the I915_PARAM_PXP_STATUS in i915_gem_supports_protected_context() return earlier if know that protected context is not supported.
Then create a protected context so we know that all other calls will succeed.

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