| Message ID | 20230112003706.950931-7-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Add missing cleanup steps for PXP global-teardown | expand |
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__ */
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 >
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) +{ + struct drm_i915_private *i915 = kdev_to_i915(kdev); + + if (!i915) + dev_err(kdev, "DRM not initialized, aborting suspend.\n"); + + 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, .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__ */
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(-)