[Intel-gfx] [PATCH] drm/i915/pxp: don't start pxp without mei_pxp bind

[Juston Li]

On Tue, Aug 9, 2022 at 4:23 PM Ceraolo Spurio, Daniele
<daniele.ceraolospurio at intel.com> wrote:
>
>
>
> On 8/9/2022 4:20 PM, Juston Li wrote:
> > On Tue, Aug 9, 2022 at 4:10 PM Ceraolo Spurio, Daniele
> > <daniele.ceraolospurio at intel.com> wrote:
> >>
> >>
> >> On 8/9/2022 3:57 PM, Juston Li wrote:
> >>> pxp will not start correctly until after mei_pxp bind completes and
> >>> intel_pxp_init_hw() is called.
> >>>
> >>> Signed-off-by: Juston Li <justonli at google.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/pxp/intel_pxp.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> >>> index 15311eaed848..3ef9e4e1870b 100644
> >>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> >>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> >>> @@ -184,7 +184,7 @@ int intel_pxp_start(struct intel_pxp *pxp)
> >>>    {
> >>>        int ret = 0;
> >>>
> >>> -     if (!intel_pxp_is_enabled(pxp))
> >>> +     if (!intel_pxp_is_enabled(pxp) || !pxp->pxp_component_added)
> >> pxp_component_added being true only indicates that we've called
> >> component_add and not if the component is currently bound. For checking
> >> the current state of the component you can look at the
> >> pxp->pxp_component pointer, which is set/cleared on component
> >> bind/unbind. Note that pxp_component has to be accessed under
> >> pxp->tee_mutex.
> >>
> >> Are you actually seeing a scenario where the user manages to submit
> >> before the bind is complete? the bind is async to i915, but I've always
> >> seen it complete before control was given to the user to start submitting.
> >>
> >> Daniele
> > Opps! I made a typo sending it out. Indeed when I tested it was with
> > pxp->component_add.
> > I did not have pxp->tee_mutex though so I'll add that too.
> >
> > We moved initialization earlier so it does try to call pxp_start() before mei
> > finishes binding.
>
> I'd also suggest to use a different error code if that is the case, so
> userspace can differentiate between !enabled and !ready and only retry
> in the latter case.

Sounds good, I think I'll use -EAGAIN

Thanks!
Juston

> Daniele
>
> >
> > Thanks
> > Juston
> >
> >>>                return -ENODEV;
> >>>
> >>>        mutex_lock(&pxp->arb_mutex);
>