On Mon, 2022-11-21 at 14:14 -0800, Juston Li wrote:
> > Alan:[snip]
> >
> > > > +void intel_pxp_end(struct intel_pxp *pxp)
> > > > +{
> > > > + struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
> > > > + intel_wakeref_t wakeref;
> > > > +
> > > > + if (!intel_pxp_is_enabled(pxp))
> > > > + return;
> > > > +
> > > > + wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > > > +
> > > > + mutex_lock(&pxp->arb_mutex);
> > > > +
> > > > + if (__pxp_global_teardown_locked(pxp, true))
> > > > + drm_dbg(&(pxp_to_gt(pxp))->i915->drm, "PXP end
> > > > timed
> > > > out\n");
> > > > +
> > > > + mutex_unlock(&pxp->arb_mutex);
> > > > +
> > > > + intel_pxp_fini_hw(pxp);
> > >
> > > Is intel_pxp_suspend() still needed then if we already fini_hw()
> > > here
> > > and mark invalidation in intel_pxp_terminate()?
> > >
> >
> > Good catch - looks like we might not need intel_pxp_suspend. But I'll
> > verify that for you.
>
> Actually, might need to be careful here. If system aborts suspend or
> fails to suspend for any reason, suspend_prepare()->intel_pxp_fini_hw()
> might have been called but not suspend().
>
> Correct me if I'm wrong, but in that case I don't think resume() will
> be called and thus intel_pxp_init_hw().
>
> For some background, there were some issues with PXP ending up in a bad
> state when some other driver caused suspend to fail or user
> closed/opened lid quickly and aborted suspend.
>
>
Yeah, i only now notice that we although we define an i915 callback for 'struct dev_pm_ops.prepare' we don't provide one
for 'struct dev_pm_ops.complete' which are the two opposing sides of suspend-vs-resume phases - meaning we really should
be doing the intel_pxp_init_hw() in the "complete" callback. I will go ahead and propose this change (considering this
is the last part of resume, I'm hoping it will not cause any regression) and should address this concern and fix a "lack
of symmetry"
> > Also, looks like i forgot to include a non-CONFIG_DRM_I915_PXP
> > version of intel_pxp_end which was causing the build
> > failure. Will resend.
> >
> > Btw, thanks for reviewing this Juston, i had cc'd you because of the
> > impact to suspend-resume flows and I believe you
> > have had prior experience debugging issues with that and runtime-
> > suspend/resume. Do you any other issues with this
> > change?
>
> Np, thanks for the patches!
>
> The only other concern I had that's more of a downstream issue is we
> ended up using hw_state_invalidated to block PXP ioctl ops during
> teardown to prevent further PXP ioctls triggering pxp_start and another
> termination queued.
> I don't recall if I sent you this patch on our tree:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3207105
> I think this could happen in suspend now too, if app sends PXP ops
> while suspend termination is in progress.
>
> Juston
Yes we did receive this patch.
