Re: [PATCH 24/24] drm/xe/display: unify runtime suspend/resume with i915 for non-d3cold

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

On Fri, Jun 05, 2026 at 07:04:59PM +0300, Jani Nikula wrote:
> On Fri, 29 May 2026, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, May 29, 2026 at 02:04:08PM +0300, Jani Nikula wrote:
> >> With the special d3cold handling in xe runtime suspend/resume separated,
> >> unify the non-d3cold paths with i915. There are multiple changes here at
> >> once that probably wouldn't make much sense to split out further.
> >> 
> >> Add the call to intel_display_driver_pm_runtime_suspend(), which in turn
> >> starts calling intel_display_power_runtime_suspend(). Deep down, this
> >> should take care of intel_dmc_wl_flush_release_work(), allowing us to
> >> drop the extra call in xe_display_pm_runtime_suspend_late().
> >> 
> >> Add the call to intel_display_driver_pm_runtime_suspend_late(), which
> >> starts calling intel_opregion_notify_adapter(). The
> >> intel_hpd_poll_enable() call is also done here, postponed from the
> >> previous location in xe_display_pm_runtime_suspend().
> >> 
> >> Add the call to intel_display_driver_pm_runtime_resume_early(), which
> >> contains the intel_display_power_runtime_resume() and
> >> intel_opregion_notify_adapter() counterparts found in
> >> intel_display_power_runtime_suspend() and
> >> intel_display_power_runtime_suspend_late(). (They are not symmetric.)
> >> 
> >> Finally, intel_display_driver_pm_runtime_resume() replaces the direct
> >> calls to intel_hpd_init(), intel_hpd_poll_disable(), and
> >> skl_watermark_ipc_update().
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/display/xe_display.c | 14 ++++----------
> >>  1 file changed, 4 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> >> index 0fadc62e1cdd..cd7264d7dfe2 100644
> >> --- a/drivers/gpu/drm/xe/display/xe_display.c
> >> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> >> @@ -363,7 +363,7 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe)
> >>  		return;
> >>  	}
> >>  
> >> -	intel_hpd_poll_enable(display);
> >> +	intel_display_driver_pm_runtime_suspend(display);
> >>  }
> >>  
> >>  void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
> >> @@ -379,12 +379,7 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
> >>  		return;
> >>  	}
> >>  
> >> -	/*
> >> -	 * If xe_display_pm_suspend_late() is not called, it is likely
> >> -	 * that we will be on dynamic DC states with DMC wakelock enabled. We
> >> -	 * need to flush the release work in that case.
> >> -	 */
> >
> > The entire comment seems wonky. I don't think we have any platforms
> > with wakelock+!DC9. Ah, it was added exactly because the D3cold path
> > is broken and doesn't go into DC9. Would have been better to fix the
> > actual problem instead. I guess you should move the comment to the
> > D3cold path in the previous patch to keep it where it actually makes
> > some sense.
> 
> Huh, but the comment says, "If xe_display_pm_suspend_late() is not
> called", and the d3cold path above calls that. It's the non-d3cold path
> that does not call the function.
> 
> Yeah, I don't know what to do with this.
> 
> Commit 731c74e988ff ("drm/xe/display: Flush DMC wakelock release work on
> runtime suspend") says, "We currently are not calling display runtime
> suspend functions when D3cold is not allowed." But that's not the whole
> truth, we're not calling the runtime suspend/resume functions *at all*
> on any xe path. (Which is what the patch at hand is trying to fix for
> non-d3cold.)
> 
> Why is the fix in the above commit just adding the single DMC wakelock
> call, instead of doing the proper runtime suspend/resume for non-d3cold?
> 
> I don't get it.

Yeap, it was never called and we were indeed dropping the DC9 savings out.
DC5/DC6 was the only thing that would be working anyway I believe.

And Gustavo's patch was more like a temporary workaround at the time that
we knew we would be working to unify the sequences. I'm sorry for having
dropped that effort :(

> 
> Cc: Gustavo, Rodrigo
> 
> 
> BR,
> Jani.
> 
> 
> >
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >> -	intel_dmc_wl_flush_release_work(display);
> >> +	intel_display_driver_pm_runtime_suspend_late(display);
> >>  }
> >>  
> >>  void xe_display_pm_runtime_resume(struct xe_device *xe)
> >> @@ -399,9 +394,8 @@ void xe_display_pm_runtime_resume(struct xe_device *xe)
> >>  		return;
> >>  	}
> >>  
> >> -	intel_hpd_init(display);
> >> -	intel_hpd_poll_disable(display);
> >> -	skl_watermark_ipc_update(display);
> >> +	intel_display_driver_pm_runtime_resume_early(display);
> >> +	intel_display_driver_pm_runtime_resume(display);
> >>  }
> >>  
> >>  
> >> -- 
> >> 2.47.3
> 
> -- 
> Jani Nikula, Intel