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

[Ceraolo Spurio, Daniele]

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.

Daniele

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