[PATCH v4 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete

[Teres Alexis, Alan Previn]

Thanks for reviewing. Will fix all and re-rev.

On Thu, 2023-01-12 at 09:28 -0800, Ceraolo Spurio, Daniele wrote:
> 
> On 1/11/2023 4:37 PM, Alan Previn wrote:
> > During suspend flow, i915 currently achors' on the
> > pm_suspend_prepare
> > callback as the location where we quiesce the entire GPU and
> > perform
> > all necessary cleanup in order to go into suspend. PXP is also
> > called
> > during this time to perform the arbitration session teardown (with
> > the assurance no additional GEM IOCTLs will come after that could
> > restart the session).
> > 
> > However, if other devices or drivers fail their suspend_prepare,
> > the
> > system will not go into suspend and i915 will be expected to resume
> > operation. In this case, we need to re-initialize the PXP hardware
> > and this really should be done within the pm_resume_complete
> > callback
> > which is the correct opposing function in the resume sequence to
> > match pm_suspend_prepare of the suspend sequence.
> > 
Alan:[snip]
> >   
> > +static void i915_pm_complete(struct device *kdev)
> 
> nit: this function should probably be moved to after pm_resume to
> keep 
> the order of the PM functions (currently they're in the order they're
> called in a full suspend flow)
> 
Alan: Will do.

> > +{
> > +       struct drm_i915_private *i915 = kdev_to_i915(kdev);
> > +
> > +       if (!i915)
> > +               dev_err(kdev, "DRM not initialized, aborting
> > suspend.\n");
> 
> This is a resume call, so you're not aborting suspend. The other 2 
> resume calls don't check for i915, any reason you have to do so here?
> Also, any reason not to check for DRM_SWITCH_POWER_OFF ?
> 
Alan: Will correct both above: remove the check and add the POWER_OFF)
as check.
> Daniele
> 
> > +
> > +       i915_drm_complete(&i915->drm);
> > +}
> > +
> >   static int i915_pm_prepare(struct device *kdev)
> >   {
> >         struct drm_i915_private *i915 = kdev_to_i915(kdev);
> > @@ -1779,6 +1794,7 @@ const struct dev_pm_ops i915_pm_ops = {
> >          * PMSG_RESUME]
> >          */
> >         .prepare = i915_pm_prepare,
> > +       .complete = i915_pm_complete,
> 
> Same as above, I'd move this to after .resume to keep the call order.
> 
Alan: Will do.
> Daniele
>