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

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

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>

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__ */