-----Original Message-----
From: Intel-xe <intel-xe-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Rodrigo Vivi
Sent: Tuesday, September 24, 2024 1:36 PM
To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx
Cc: Deak, Imre <imre.deak@xxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
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@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);
> +
> + 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@xxxxxxxxx>
-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
>
>
