Re: [PATCH v3] drm/xe/pxp: add termination on resume

On 7/28/2026 5:01 PM, Teres Alexis, Alan Previn wrote:
> On Mon, 2026-07-20 at 15:27 -0700, Daniele Ceraolo Spurio wrote:
>> Suspend/resume causes the PXP keys to become invalid, but doesn't
>> actually kill the session. The driver also doesn't explicitly kill and
>> re-start the session until a new PXP request comes in, which means that
>> the "zombie" session can potentially stick around if there are no new
>> requests from userspace. While this is not an issue for PXP, HDCP has a
>> new behavior starting on PTL where a communication is sent to GSC if a
>> session is active at suspend time (even if it doesn't have a valid key),
>> which can lead to delays in the suspend flow if we suspend while the
>> zombie session is still active.
>> To avoid this, we can trigger a termination on resume and kill the
>> zombie session immediately, instead of delaying the termination to the
>>
> [snip]
>> diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
>> index fea3d8ceeddb..2fef274a585e 100644
>> --- a/drivers/gpu/drm/xe/xe_pxp.c
>> +++ b/drivers/gpu/drm/xe/xe_pxp.c
>> @@ -8,6 +8,8 @@
>>   #include <drm/drm_managed.h>
>>   #include <uapi/drm/xe_drm.h>
>>   
>> +#include <linux/device.h>
>> +
>>   #include "xe_bo.h"
>>   #include "xe_bo_types.h"
>>   #include "xe_device_types.h"
>> @@ -174,16 +176,9 @@ static void mark_termination_in_progress(struct xe_pxp *pxp)
>>    pxp->status = XE_PXP_TERMINATION_IN_PROGRESS;
>>   }
>>   
>> -static void pxp_terminate(struct xe_pxp *pxp)
>> +static bool pxp_prep_for_termination(struct xe_pxp *pxp)
>>   {
>> - int ret = 0;
>> - struct xe_device *xe = pxp->xe;
>> -
>> - if (!wait_for_completion_timeout(&pxp->activation,
>> - msecs_to_jiffies(PXP_ACTIVATION_TIMEOUT_MS)))
>> - drm_err(&xe->drm, "failed to wait for PXP start before termination\n");
>> -
>> - mutex_lock(&pxp->mutex);
>> + lockdep_assert_held(&pxp->mutex);
>>   
>>    if (pxp->status == XE_PXP_ACTIVE)
>>    pxp->key_instance++;
>> @@ -192,10 +187,8 @@ static void pxp_terminate(struct xe_pxp *pxp)
>>    * we'll mark the status as needing termination on resume, so no need to
>>    * emit a termination now.
>>    */
>> - if (pxp->status == XE_PXP_SUSPENDED) {
>> - mutex_unlock(&pxp->mutex);
>> - return;
>> - }
>> + if (pxp->status == XE_PXP_SUSPENDED)
>> + return false;
> alan: in xe_pxp_pm_resume, we assert "pxp->status == XE_PXP_SUSPENDED" before calling
> prep_for_termination... this assertion will mostly be true (i.e. we did successfully

I then also change the state to XE_PXP_NEEDS_TERMINATION before calling 
prep_for_termination, so for the resume case that if condition is always 
false.

> go into suspend before executing the resume code). In that case, above check will always
> return false (i.e. the worker will not get fired). i am only guessing, but perhaps
> above check (from original pxp_terminate) was meant to deal with the case of an incoming
> termination IRQ while we are in the midst of suspending? (i.e. we just defer the termination
> code). But for terminate-prep, i guess we should not check this? would just removing this work
> fine since this patch adds "PXP_TERMINATION_REQUEST_ON_RESUME" into pxp->events.pending
> and you handle this in the event-worker now (i.e. if it is a HW IRQ, u drop it if we
> just started suspending and proceed with the new suspend-prep->resume-actual termination.

I could split the check out from the prep_for_termination and only have 
in the !hw_only path in the termination worker, but I liked the idea of 
having all the checks in the same place, even if one of them is 
meaningless for the resume case.

Daniele

> [snip]
>
>