Re: [Intel-gfx] [PATCH v5 6/6] drm/i915/pxp: Pxp hw init should be in resume_complete

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jan 19, 2023 at 11:10:21AM -0800, Ceraolo Spurio, Daniele wrote:
> 
> 
> On 1/12/2023 5:18 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.
> > 
> > Because this callback is the last thing at the end of resuming
> > we expect little to no impact to the rest of the i915 resume sequence
> > with this change.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_gt_pm.h   |  1 +
> >   drivers/gpu/drm/i915/i915_driver.c      | 20 ++++++++++++++++++--
> >   drivers/gpu/drm/i915/pxp/intel_pxp_pm.c |  2 +-
> >   drivers/gpu/drm/i915/pxp/intel_pxp_pm.h |  6 +++---
> >   4 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.h b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> > index 6c9a46452364..fd1a23621222 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.h
> > @@ -77,6 +77,7 @@ void intel_gt_pm_fini(struct intel_gt *gt);
> >   void intel_gt_suspend_prepare(struct intel_gt *gt);
> >   void intel_gt_suspend_late(struct intel_gt *gt);
> > +
> 
> Stray newline. With this removed:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Daniele
> 
> >   int intel_gt_resume(struct intel_gt *gt);
> >   void intel_gt_runtime_suspend(struct intel_gt *gt);
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index c1e427ba57ae..4c68a3f26e96 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1170,6 +1170,13 @@ static bool suspend_to_idle(struct drm_i915_private *dev_priv)
> >   	return false;
> >   }
> > +static void i915_drm_complete(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(dev);
> > +
> > +	intel_pxp_resume_complete(i915->pxp);
> > +}
> > +
> >   static int i915_drm_prepare(struct drm_device *dev)
> >   {
> >   	struct drm_i915_private *i915 = to_i915(dev);
> > @@ -1370,8 +1377,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >   	i915_gem_resume(dev_priv);
> > -	intel_pxp_resume(dev_priv->pxp);
> > -
> >   	intel_modeset_init_hw(dev_priv);
> >   	intel_init_clock_gating(dev_priv);
> >   	intel_hpd_init(dev_priv);
> > @@ -1563,6 +1568,16 @@ static int i915_pm_resume(struct device *kdev)
> >   	return i915_drm_resume(&i915->drm);
> >   }
> > +static void i915_pm_complete(struct device *kdev)
> > +{
> > +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> > +
> > +	if (i915->drm.switch_power_state == DRM_SWITCH_POWER_OFF)
> > +		return;
> > +
> > +	i915_drm_complete(&i915->drm);
> > +}
> > +
> >   /* freeze: before creating the hibernation_image */
> >   static int i915_pm_freeze(struct device *kdev)
> >   {
> > @@ -1783,6 +1798,7 @@ const struct dev_pm_ops i915_pm_ops = {
> >   	.suspend_late = i915_pm_suspend_late,
> >   	.resume_early = i915_pm_resume_early,
> >   	.resume = i915_pm_resume,
> > +	.complete = i915_pm_complete,
> >   	/*
> >   	 * S4 event handlers
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > index e427464aa131..4f836b317424 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > @@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
> >   	}
> >   }
> > -void intel_pxp_resume(struct intel_pxp *pxp)
> > +void intel_pxp_resume_complete(struct intel_pxp *pxp)
> >   {
> >   	if (!intel_pxp_is_enabled(pxp))
> >   		return;
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > index 586be769104f..06b46f535b42 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > @@ -11,7 +11,7 @@ struct intel_pxp;
> >   #ifdef CONFIG_DRM_I915_PXP
> >   void intel_pxp_suspend_prepare(struct intel_pxp *pxp);
> >   void intel_pxp_suspend(struct intel_pxp *pxp);
> > -void intel_pxp_resume(struct intel_pxp *pxp);
> > +void intel_pxp_resume_complete(struct intel_pxp *pxp);
> >   void intel_pxp_runtime_suspend(struct intel_pxp *pxp);
> >   #else
> >   static inline void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
> > @@ -22,7 +22,7 @@ static inline void intel_pxp_suspend(struct intel_pxp *pxp)
> >   {
> >   }
> > -static inline void intel_pxp_resume(struct intel_pxp *pxp)
> > +static inline void intel_pxp_resume_complete(struct intel_pxp *pxp)
> >   {
> >   }
> > @@ -32,6 +32,6 @@ static inline void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
> >   #endif
> >   static inline void intel_pxp_runtime_resume(struct intel_pxp *pxp)
> >   {
> > -	intel_pxp_resume(pxp);
> > +	intel_pxp_resume_complete(pxp);
> >   }
> >   #endif /* __INTEL_PXP_PM_H__ */
>