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

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

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.
>
> 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);
> +
>   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..c3e7c40daaeb 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);
> @@ -1484,6 +1489,16 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
>   	return i915_drm_resume(&i915->drm);
>   }
>   
> +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)

> +{
> +	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 ?

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.

Daniele

>   	.suspend = i915_pm_suspend,
>   	.suspend_late = i915_pm_suspend_late,
>   	.resume_early = i915_pm_resume_early,
> 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__ */