| Message ID | 20240924204222.246862-11-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reconcile i915's and xe's display power mgt sequences | expand |
-----Original Message----- From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Rodrigo Vivi Sent: Tuesday, September 24, 2024 1:36 PM To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org Cc: Deak, Imre <imre.deak@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com> Subject: [PATCH 10/31] drm/xe/display: Spin-off xe_display runtime/d3cold sequences > > No functional change. This patch only splits the xe_display_pm > suspend/resume functions in the regular suspend/resume from the > runtime/d3cold ones. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/xe/display/xe_display.c | 68 ++++++++++++++++--------- > 1 file changed, 45 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c > index be431d9907df..a4705a452adb 100644 > --- a/drivers/gpu/drm/xe/display/xe_display.c > +++ b/drivers/gpu/drm/xe/display/xe_display.c > @@ -308,8 +308,41 @@ static void xe_display_flush_cleanup_work(struct xe_device *xe) > } > } > > -/* TODO: System and runtime suspend/resume sequences will be sanitized as a follow-up. */ > -static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > +static void xe_display_to_d3cold(struct xe_device *xe) > +{ > + struct intel_display *display = &xe->display; > + > + /* We do a lot of poking in a lot of registers, make sure they work properly. */ > + intel_power_domains_disable(xe); > + > + xe_display_flush_cleanup_work(xe); > + > + intel_hpd_cancel_work(xe); > + > + intel_opregion_suspend(display, PCI_D3cold); > + > + intel_dmc_suspend(display); > +} > + > +static void xe_display_from_d3cold(struct xe_device *xe) > +{ > + struct intel_display *display = &xe->display; > + > + intel_dmc_resume(display); > + > + if (has_display(xe)) > + drm_mode_config_reset(&xe->drm); > + > + intel_display_driver_init_hw(xe); > + > + intel_hpd_init(xe); > + > + intel_opregion_resume(display); > + > + intel_power_domains_enable(xe); > +} xe_display_to_d3cold and xe_display_from_d3cold sound like conversion functions that take an xe_display and return a d3cold, and vice versa, respectively. Perhaps we should consider a different naming scheme? Something like: xe_display_enable_d3cold xe_display_disable_d3cold Also, we're going to need to move these functions in patch 31 later, so perhaps it would be for the best to put these functions in the correct place from the start? I won't block on this, but do consider renaming these functions. Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt > + > +void xe_display_pm_suspend(struct xe_device *xe) > { > struct intel_display *display = &xe->display; > bool s2idle = suspend_to_idle(); > @@ -321,10 +354,10 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > * properly. > */ > intel_power_domains_disable(xe); > - if (!runtime) > - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true); > > - if (!runtime && 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); > intel_display_driver_suspend(xe); > @@ -334,7 +367,7 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > > intel_hpd_cancel_work(xe); > > - if (!runtime && has_display(xe)) { > + if (has_display(xe)) { > intel_display_driver_suspend_access(xe); > intel_encoder_suspend_all(&xe->display); > } > @@ -344,11 +377,6 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > intel_dmc_suspend(display); > } > > -void xe_display_pm_suspend(struct xe_device *xe) > -{ > - __xe_display_pm_suspend(xe, false); > -} > - > void xe_display_pm_shutdown(struct xe_device *xe) > { > if (!xe->info.probe_display) > @@ -379,7 +407,7 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe) > return; > > if (xe->d3cold.allowed) > - __xe_display_pm_suspend(xe, true); > + xe_display_to_d3cold(xe); > > intel_hpd_poll_enable(xe); > } > @@ -405,7 +433,7 @@ void xe_display_pm_resume_early(struct xe_device *xe) > intel_power_domains_resume(xe); > } > > -static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) > +void xe_display_pm_resume(struct xe_device *xe) > { > struct intel_display *display = &xe->display; > > @@ -419,12 +447,12 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) > > intel_display_driver_init_hw(xe); > > - if (!runtime && has_display(xe)) > + if (has_display(xe)) > intel_display_driver_resume_access(xe); > > intel_hpd_init(xe); > > - if (!runtime && has_display(xe)) { > + if (has_display(xe)) { > intel_display_driver_resume(xe); > drm_kms_helper_poll_enable(&xe->drm); > intel_display_driver_enable_user_access(xe); > @@ -433,17 +461,11 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) > > intel_opregion_resume(display); > > - if (!runtime) > - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false); > + intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false); > > intel_power_domains_enable(xe); > } > > -void xe_display_pm_resume(struct xe_device *xe) > -{ > - __xe_display_pm_resume(xe, false); > -} > - > void xe_display_pm_runtime_resume(struct xe_device *xe) > { > if (!xe->info.probe_display) > @@ -452,7 +474,7 @@ void xe_display_pm_runtime_resume(struct xe_device *xe) > intel_hpd_poll_disable(xe); > > if (xe->d3cold.allowed) > - __xe_display_pm_resume(xe, true); > + xe_display_from_d3cold(xe); > } > > > -- > 2.46.0 > >
On Tue, Sep 24, 2024 at 04:35:31PM -0400, Rodrigo Vivi wrote: > No functional change. This patch only splits the xe_display_pm > suspend/resume functions in the regular suspend/resume from the > runtime/d3cold ones. > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/xe/display/xe_display.c | 68 ++++++++++++++++--------- > 1 file changed, 45 insertions(+), 23 deletions(-) > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c > index be431d9907df..a4705a452adb 100644 > --- a/drivers/gpu/drm/xe/display/xe_display.c > +++ b/drivers/gpu/drm/xe/display/xe_display.c > @@ -308,8 +308,41 @@ static void xe_display_flush_cleanup_work(struct xe_device *xe) > } > } > > -/* TODO: System and runtime suspend/resume sequences will be sanitized as a follow-up. */ > -static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > +static void xe_display_to_d3cold(struct xe_device *xe) > +{ > + struct intel_display *display = &xe->display; > + > + /* We do a lot of poking in a lot of registers, make sure they work properly. */ > + intel_power_domains_disable(xe); > + > + xe_display_flush_cleanup_work(xe); > + > + intel_hpd_cancel_work(xe); This cancels a work synchronously that takes a runtime PM reference, so it could dead-lock. I know that xe adds a way to get an RPM reference in the runtime suspend/resume handlers, not sure if that's a good approach in general. Also, not sure if it's ok to to drop all interrupts - in intel_hpd_cancel_work() - here that got raised before runtime suspend, instead of handling them as expected. So I think intel_hpd_cancel_work() shouldn't be called here. > + > + intel_opregion_suspend(display, PCI_D3cold); > + > + intel_dmc_suspend(display); > +} > + > +static void xe_display_from_d3cold(struct xe_device *xe) > +{ > + struct intel_display *display = &xe->display; > + > + intel_dmc_resume(display); > + > + if (has_display(xe)) > + drm_mode_config_reset(&xe->drm); The above is the counterpart of intel_encoder_suspend_all() which is only called during system suspend. > + > + intel_display_driver_init_hw(xe); > + > + intel_hpd_init(xe); > + > + intel_opregion_resume(display); > + > + intel_power_domains_enable(xe); > +} > + > +void xe_display_pm_suspend(struct xe_device *xe) > { > struct intel_display *display = &xe->display; > bool s2idle = suspend_to_idle(); > @@ -321,10 +354,10 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > * properly. > */ > intel_power_domains_disable(xe); > - if (!runtime) > - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true); > > - if (!runtime && 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); > intel_display_driver_suspend(xe); > @@ -334,7 +367,7 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > > intel_hpd_cancel_work(xe); > > - if (!runtime && has_display(xe)) { > + if (has_display(xe)) { > intel_display_driver_suspend_access(xe); > intel_encoder_suspend_all(&xe->display); > } > @@ -344,11 +377,6 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) > intel_dmc_suspend(display); > } > > -void xe_display_pm_suspend(struct xe_device *xe) > -{ > - __xe_display_pm_suspend(xe, false); > -} > - > void xe_display_pm_shutdown(struct xe_device *xe) > { > if (!xe->info.probe_display) > @@ -379,7 +407,7 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe) > return; > > if (xe->d3cold.allowed) > - __xe_display_pm_suspend(xe, true); > + xe_display_to_d3cold(xe); > > intel_hpd_poll_enable(xe); > } > @@ -405,7 +433,7 @@ void xe_display_pm_resume_early(struct xe_device *xe) > intel_power_domains_resume(xe); > } > > -static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) > +void xe_display_pm_resume(struct xe_device *xe) > { > struct intel_display *display = &xe->display; > > @@ -419,12 +447,12 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) > > intel_display_driver_init_hw(xe); > > - if (!runtime && has_display(xe)) > + if (has_display(xe)) > intel_display_driver_resume_access(xe); > > intel_hpd_init(xe); > > - if (!runtime && has_display(xe)) { > + if (has_display(xe)) { > intel_display_driver_resume(xe); > drm_kms_helper_poll_enable(&xe->drm); > intel_display_driver_enable_user_access(xe); > @@ -433,17 +461,11 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) > > intel_opregion_resume(display); > > - if (!runtime) > - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false); > + intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false); > > intel_power_domains_enable(xe); > } > > -void xe_display_pm_resume(struct xe_device *xe) > -{ > - __xe_display_pm_resume(xe, false); > -} > - > void xe_display_pm_runtime_resume(struct xe_device *xe) > { > if (!xe->info.probe_display) > @@ -452,7 +474,7 @@ void xe_display_pm_runtime_resume(struct xe_device *xe) > intel_hpd_poll_disable(xe); > > if (xe->d3cold.allowed) > - __xe_display_pm_resume(xe, true); > + xe_display_from_d3cold(xe); > } > > > -- > 2.46.0 >
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c index be431d9907df..a4705a452adb 100644 --- a/drivers/gpu/drm/xe/display/xe_display.c +++ b/drivers/gpu/drm/xe/display/xe_display.c @@ -308,8 +308,41 @@ static void xe_display_flush_cleanup_work(struct xe_device *xe) } } -/* TODO: System and runtime suspend/resume sequences will be sanitized as a follow-up. */ -static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) +static void xe_display_to_d3cold(struct xe_device *xe) +{ + struct intel_display *display = &xe->display; + + /* We do a lot of poking in a lot of registers, make sure they work properly. */ + intel_power_domains_disable(xe); + + xe_display_flush_cleanup_work(xe); + + intel_hpd_cancel_work(xe); + + intel_opregion_suspend(display, PCI_D3cold); + + intel_dmc_suspend(display); +} + +static void xe_display_from_d3cold(struct xe_device *xe) +{ + struct intel_display *display = &xe->display; + + intel_dmc_resume(display); + + if (has_display(xe)) + drm_mode_config_reset(&xe->drm); + + intel_display_driver_init_hw(xe); + + intel_hpd_init(xe); + + intel_opregion_resume(display); + + intel_power_domains_enable(xe); +} + +void xe_display_pm_suspend(struct xe_device *xe) { struct intel_display *display = &xe->display; bool s2idle = suspend_to_idle(); @@ -321,10 +354,10 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) * properly. */ intel_power_domains_disable(xe); - if (!runtime) - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true); - if (!runtime && 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); intel_display_driver_suspend(xe); @@ -334,7 +367,7 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) intel_hpd_cancel_work(xe); - if (!runtime && has_display(xe)) { + if (has_display(xe)) { intel_display_driver_suspend_access(xe); intel_encoder_suspend_all(&xe->display); } @@ -344,11 +377,6 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime) intel_dmc_suspend(display); } -void xe_display_pm_suspend(struct xe_device *xe) -{ - __xe_display_pm_suspend(xe, false); -} - void xe_display_pm_shutdown(struct xe_device *xe) { if (!xe->info.probe_display) @@ -379,7 +407,7 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe) return; if (xe->d3cold.allowed) - __xe_display_pm_suspend(xe, true); + xe_display_to_d3cold(xe); intel_hpd_poll_enable(xe); } @@ -405,7 +433,7 @@ void xe_display_pm_resume_early(struct xe_device *xe) intel_power_domains_resume(xe); } -static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) +void xe_display_pm_resume(struct xe_device *xe) { struct intel_display *display = &xe->display; @@ -419,12 +447,12 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) intel_display_driver_init_hw(xe); - if (!runtime && has_display(xe)) + if (has_display(xe)) intel_display_driver_resume_access(xe); intel_hpd_init(xe); - if (!runtime && has_display(xe)) { + if (has_display(xe)) { intel_display_driver_resume(xe); drm_kms_helper_poll_enable(&xe->drm); intel_display_driver_enable_user_access(xe); @@ -433,17 +461,11 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime) intel_opregion_resume(display); - if (!runtime) - intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false); + intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false); intel_power_domains_enable(xe); } -void xe_display_pm_resume(struct xe_device *xe) -{ - __xe_display_pm_resume(xe, false); -} - void xe_display_pm_runtime_resume(struct xe_device *xe) { if (!xe->info.probe_display) @@ -452,7 +474,7 @@ void xe_display_pm_runtime_resume(struct xe_device *xe) intel_hpd_poll_disable(xe); if (xe->d3cold.allowed) - __xe_display_pm_resume(xe, true); + xe_display_from_d3cold(xe); }
No functional change. This patch only splits the xe_display_pm suspend/resume functions in the regular suspend/resume from the runtime/d3cold ones. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/xe/display/xe_display.c | 68 ++++++++++++++++--------- 1 file changed, 45 insertions(+), 23 deletions(-)