Re: [PATCH 1/2] drm/xe/display: Match i915 driver suspend/resume sequences better

[Imre Deak <imre.deak@intel.com>]

On Tue, Aug 06, 2024 at 10:44:45AM +0300, Jani Nikula wrote:
> On Mon, 05 Aug 2024, Imre Deak <imre.deak@intel.com> wrote:
> > On Mon, Aug 05, 2024 at 05:13:08PM +0200, Maarten Lankhorst wrote:
> >> Suspend fbdev sooner, and disable user access before suspending to
> >> prevent some races. I've noticed this when comparing xe suspend to
> >> i915's.
> >
> > This is only part of
> >
> > 24b412b1bfeb ("drm/i915: Disable intel HPD poll after DRM poll init/enable")
> > f4ed123ae295 ("drm/i915: Suspend the framebuffer console during driver shutdown")
> > 1ef28d86bea9 ("drm/i915: Suspend the framebuffer console earlier during system suspend")
> > bd738d859e71 ("drm/i915: Prevent modesets during driver init/shutdown")
> >
> > done for i915, imo all the above changes should be applied to xe.
> 
> Really, all of should happen inside drivers/gpu/drm/i915/display/, not
> duplicated in i915 and xe drivers.

I agree and I was planning to do that, but AFAIR sharing the code
(moving the display suspend/resume steps to intel_display_driver.c) also
involves changing the order of some of the steps at least in the xe
driver. So imo adding now these separately to xe and then making i915
and xe share the s/r handlers in a separate change would be better.

> BR,
> Jani.
> 
> 
> >
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/display/xe_display.c | 12 ++++++++----
> >>  1 file changed, 8 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> >> index ca4468c820788..44405f17c1d28 100644
> >> --- a/drivers/gpu/drm/xe/display/xe_display.c
> >> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> >> @@ -294,8 +294,12 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> >>  	 * properly.
> >>  	 */
> >>  	intel_power_domains_disable(xe);
> >> -	if (has_display(xe))
> >> +	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
> >> +
> >> +	if (has_display(xe)) {
> >>  		drm_kms_helper_poll_disable(&xe->drm);
> >> +		intel_display_driver_disable_user_access(xe);
> >> +	}
> >>  
> >>  	if (!runtime)
> >>  		intel_display_driver_suspend(xe);
> >> @@ -308,8 +312,6 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> >>  
> >>  	intel_opregion_suspend(xe, s2idle ? PCI_D1 : PCI_D3cold);
> >>  
> >> -	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
> >> -
> >>  	intel_dmc_suspend(xe);
> >>  }
> >>  
> >> @@ -353,8 +355,10 @@ void xe_display_pm_resume(struct xe_device *xe, bool runtime)
> >>  		intel_display_driver_resume(xe);
> >>  
> >>  	intel_hpd_poll_disable(xe);
> >> -	if (has_display(xe))
> >> +	if (has_display(xe)) {
> >>  		drm_kms_helper_poll_enable(&xe->drm);
> >> +		intel_display_driver_enable_user_access(xe);
> >> +	}
> >>  
> >>  	intel_opregion_resume(xe);
> >>  
> >> -- 
> >> 2.45.2
> >> 
> 
> -- 
> Jani Nikula, Intel