On Fri, May 29, 2026 at 05:16:43PM +0300, Ville Syrjälä wrote: > On Fri, May 29, 2026 at 02:04:03PM +0300, Jani Nikula wrote: > > Move the intel_dmc_suspend() calls from i915 and xe suspend and shutdown > > hooks all the way down to a shared location in > > __intel_display_driver_pm_suspend(). > > > > This is a change in the suspend/shutdown sequences, but hopefully one > > without problems. > > > > Signed-off-by: Jani Nikula <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_display_driver.c | 2 ++ > > drivers/gpu/drm/i915/i915_driver.c | 4 ---- > > drivers/gpu/drm/xe/display/xe_display.c | 4 ---- > > 3 files changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > > b/drivers/gpu/drm/i915/display/intel_display_driver.c > > index bc632ac8c9b4..6cac36157bea 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > > @@ -755,6 +755,8 @@ static int __intel_display_driver_pm_suspend(struct > > intel_display *display, bool > > > > intel_encoder_suspend_all(display); > > > > + intel_dmc_suspend(display); > > + > > return ret; > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > b/drivers/gpu/drm/i915/i915_driver.c > > index 209ac512455d..40fc15017486 100644 > > --- a/drivers/gpu/drm/i915/i915_driver.c > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > @@ -1045,8 +1045,6 @@ void i915_driver_shutdown(struct drm_i915_private > > *i915) > > > > intel_irq_suspend(i915); > > > > - intel_dmc_suspend(display); > > - > > I wonder what we're even trying to achieve here. Just to make sure > the DMC firmware has been loaded before we system suspend? That might > be reasonable, but we now wait for the firmware load somewhere during > driver init due to flip queue needing it. So this all seems rather > redundant now.
Enabling/disabling the display power wells on demand during runtime is prevented if DMC is not loaded, by holding the INIT domain reference. This reference must be still dropped for suspend (and then reacquired during resume). This - besides waiting for the firmware to load - is done in intel_dmc_suspend()/intel_dmc_resume(). Yes, waiting for the firmware to load could be removed from intel_dmc_suspend() after it's waited already during driver loading/resume. > > > i915_gem_suspend(i915); > > > > /* > > @@ -1117,8 +1115,6 @@ static int i915_drm_suspend(struct drm_device *dev) > > > > dev_priv->suspend_count++; > > > > - intel_dmc_suspend(display); > > - > > enable_rpm_wakeref_asserts(&dev_priv->runtime_pm); > > > > i915_gem_drain_freed_objects(dev_priv); > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c > > b/drivers/gpu/drm/xe/display/xe_display.c > > index 561ec1ed6845..9416405667d8 100644 > > --- a/drivers/gpu/drm/xe/display/xe_display.c > > +++ b/drivers/gpu/drm/xe/display/xe_display.c > > @@ -198,8 +198,6 @@ void xe_display_shutdown(struct xe_device *xe) > > intel_display_driver_shutdown(display); > > > > intel_opregion_suspend(display, PCI_D3cold); > > - > > - intel_dmc_suspend(display); > > } > > > > void xe_display_shutdown_late(struct xe_device *xe) > > @@ -276,8 +274,6 @@ void xe_display_pm_suspend(struct xe_device *xe) > > intel_display_driver_pm_suspend(display); > > > > intel_opregion_suspend(display, s2idle ? PCI_D1 : PCI_D3cold); > > - > > - intel_dmc_suspend(display); > > } > > > > void xe_display_pm_suspend_late(struct xe_device *xe) > > -- > > 2.47.3 > > -- > Ville Syrjälä > Intel
