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@xxxxxxxxx>
> ---
> 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
>
