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

On Wed, 2026-07-29 at 12:05 -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> 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]
> > > 
[snip]
> > > -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
alan: my bad - not sure how the heck i missed that:
Reviewed-by: Alan Previn <[email protected]>
> 
> > [snip]
> > 
> > 
>