-----Original Message-----
From: Intel-gfx <intel-gfx-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 31/31] drm/{i915, xe}/display: Consolidade entire runtime pm sequence
>
> No functional change.
>
> Consolidate the entire runtime pm sequences under
> intel_display_driver.
>
> Simplifications and optimizations around the D3cold sequences are
> likely still possible. But before that can be done, consolidate
> everything at the intel_display_driver side.
>
> Xe display power management functions are now only a wrapper
> checking for xe's display probe parameter.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
LGTM.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
-Jonathan Cavitt
> ---
> .../drm/i915/display/intel_display_driver.c | 66 ++++++++++++++++---
> .../drm/i915/display/intel_display_driver.h | 12 ++--
> drivers/gpu/drm/i915/i915_driver.c | 8 +--
> drivers/gpu/drm/xe/display/xe_display.c | 56 ++--------------
> 4 files changed, 74 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 62a7aa56f0da..3861fdbefaff 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -856,12 +856,45 @@ void intel_display_driver_resume(struct drm_i915_private *i915)
> intel_power_domains_enable(i915);
> }
>
> -void intel_display_driver_runtime_suspend(struct drm_i915_private *i915)
> +static void intel_display_to_d3cold(struct drm_i915_private *i915)
> {
> - intel_display_power_suspend(i915);
> + struct intel_display *display = &i915->display;
> +
> + /* We do a lot of poking in a lot of registers, make sure they work properly. */
> + intel_power_domains_disable(i915);
> +
> + intel_hpd_cancel_work(i915);
> +
> + intel_opregion_suspend(display, PCI_D3cold);
> +
> + intel_dmc_suspend(display);
> +}
> +
> +static void intel_display_from_d3cold(struct drm_i915_private *i915)
> +{
> + struct intel_display *display = &i915->display;
> +
> + intel_dmc_resume(display);
> +
> + if (HAS_DISPLAY(i915))
> + drm_mode_config_reset(&i915->drm);
> +
> + intel_display_driver_init_hw(i915);
> +
> + intel_opregion_resume(display);
> +
> + intel_power_domains_enable(i915);
> +}
> +
> +void intel_display_driver_runtime_suspend(struct drm_i915_private *i915, bool d3cold_allowed)
> +{
> + if (d3cold_allowed)
> + intel_display_to_d3cold(i915);
> + else
> + intel_display_power_suspend(i915);
> }
>
> -void intel_display_driver_runtime_suspend_late(struct drm_i915_private *i915)
> +static void display_runtime_suspend_notify_opregion(struct drm_i915_private *i915)
> {
> struct intel_display *display = &i915->display;
>
> @@ -887,20 +920,37 @@ void intel_display_driver_runtime_suspend_late(struct drm_i915_private *i915)
> */
> intel_opregion_notify_adapter(display, PCI_D1);
> }
> +}
> +
> +void intel_display_driver_runtime_suspend_late(struct drm_i915_private *i915,
> + bool d3cold_allowed)
> +{
> + if (d3cold_allowed)
> + intel_display_power_suspend_late(i915, false);
> + else
> + display_runtime_suspend_notify_opregion(i915);
>
> if (!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915))
> intel_hpd_poll_enable(i915);
> }
>
> -void intel_display_driver_runtime_resume_early(struct drm_i915_private *i915)
> +void intel_display_driver_runtime_resume_early(struct drm_i915_private *i915,
> + bool d3cold_allowed)
> {
> - intel_opregion_notify_adapter(&i915->display, PCI_D0);
> -
> - intel_display_power_resume(i915);
> + if (d3cold_allowed) {
> + intel_display_power_resume_early(i915);
> + } else {
> + intel_opregion_notify_adapter(&i915->display, PCI_D0);
> + intel_display_power_resume(i915);
> + }
> }
>
> -void intel_display_driver_runtime_resume(struct drm_i915_private *i915)
> +void intel_display_driver_runtime_resume(struct drm_i915_private *i915,
> + bool d3cold_allowed)
> {
> + if (d3cold_allowed)
> + intel_display_from_d3cold(i915);
> +
> /*
> * On VLV/CHV display interrupts are part of the display
> * power well, so hpd is reinitialized from there. For
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.h b/drivers/gpu/drm/i915/display/intel_display_driver.h
> index b1441a55d72d..21aa0e551898 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.h
> @@ -33,10 +33,14 @@ void intel_display_driver_resume(struct drm_i915_private *i915);
> void intel_display_driver_resume_noirq(struct drm_i915_private *i915);
> void intel_display_driver_resume_noirq_legacy(struct drm_i915_private *i915);
> void intel_display_driver_resume_nogem(struct intel_display *display);
> -void intel_display_driver_runtime_suspend(struct drm_i915_private *i915);
> -void intel_display_driver_runtime_suspend_late(struct drm_i915_private *i915);
> -void intel_display_driver_runtime_resume_early(struct drm_i915_private *i915);
> -void intel_display_driver_runtime_resume(struct drm_i915_private *i915);
> +void intel_display_driver_runtime_suspend(struct drm_i915_private *i915,
> + bool d3cold_allowed);
> +void intel_display_driver_runtime_suspend_late(struct drm_i915_private *i915,
> + bool d3cold_allowed);
> +void intel_display_driver_runtime_resume_early(struct drm_i915_private *i915,
> + bool d3cold_allowed);
> +void intel_display_driver_runtime_resume(struct drm_i915_private *i915,
> + bool d3cold_allowed);
> void intel_display_driver_shutdown(struct drm_i915_private *i915);
> void intel_display_driver_shutdown_noirq(struct drm_i915_private *i915);
> void intel_display_driver_shutdown_nogem(struct drm_i915_private *i915);
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index b3eaa55ebacb..719b1c21b695 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1402,7 +1402,7 @@ static int intel_runtime_suspend(struct device *kdev)
> for_each_gt(gt, dev_priv, i)
> intel_uncore_suspend(gt->uncore);
>
> - intel_display_driver_runtime_suspend(dev_priv);
> + intel_display_driver_runtime_suspend(dev_priv, false);
>
> ret = vlv_suspend_complete(dev_priv);
> if (ret) {
> @@ -1436,7 +1436,7 @@ static int intel_runtime_suspend(struct device *kdev)
> if (root_pdev)
> pci_d3cold_disable(root_pdev);
>
> - intel_display_driver_runtime_suspend_late(dev_priv);
> + intel_display_driver_runtime_suspend_late(dev_priv, false);
>
> assert_forcewakes_inactive(&dev_priv->uncore);
>
> @@ -1469,7 +1469,7 @@ static int intel_runtime_resume(struct device *kdev)
> drm_dbg(&dev_priv->drm,
> "Unclaimed access during suspend, bios?\n");
>
> - intel_display_driver_runtime_resume_early(dev_priv);
> + intel_display_driver_runtime_resume_early(dev_priv, false);
>
> ret = vlv_resume_prepare(dev_priv, true);
>
> @@ -1487,7 +1487,7 @@ static int intel_runtime_resume(struct device *kdev)
>
> intel_pxp_runtime_resume(dev_priv->pxp);
>
> - intel_display_driver_runtime_resume_early(dev_priv);
> + intel_display_driver_runtime_resume_early(dev_priv, false);
>
> enable_rpm_wakeref_asserts(rpm);
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index ab85c7fb217a..9a652292d988 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -283,36 +283,6 @@ static bool suspend_to_idle(void)
> return false;
> }
>
> -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);
> -
> - 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_opregion_resume(display);
> -
> - intel_power_domains_enable(xe);
> -}
> -
> void xe_display_pm_suspend(struct xe_device *xe)
> {
> if (!xe->info.probe_display)
> @@ -413,10 +383,7 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe)
> if (!xe->info.probe_display)
> return;
>
> - if (xe->d3cold.allowed)
> - xe_display_to_d3cold(xe);
> - else
> - intel_display_power_suspend(xe);
> + intel_display_driver_runtime_suspend(xe, xe->d3cold.allowed);
> }
>
> void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
> @@ -424,12 +391,7 @@ void xe_display_pm_runtime_suspend_late(struct xe_device *xe)
> if (!xe->info.probe_display)
> return;
>
> - if (xe->d3cold.allowed)
> - intel_display_power_suspend_late(xe, false);
> - else
> - intel_opregion_notify_adapter(&xe->display, PCI_D1);
> -
> - intel_hpd_poll_enable(xe);
> + intel_display_driver_runtime_suspend_late(xe, xe->d3cold.allowed);
> }
>
> void xe_display_pm_runtime_resume_early(struct xe_device *xe)
> @@ -437,12 +399,7 @@ void xe_display_pm_runtime_resume_early(struct xe_device *xe)
> if (!xe->info.probe_display)
> return;
>
> - if (xe->d3cold.allowed) {
> - intel_display_power_resume_early(xe);
> - } else {
> - intel_opregion_notify_adapter(&xe->display, PCI_D0);
> - intel_display_power_resume(xe);
> - }
> + intel_display_driver_runtime_resume_early(xe, xe->d3cold.allowed);
> }
>
> void xe_display_pm_runtime_resume(struct xe_device *xe)
> @@ -450,12 +407,7 @@ void xe_display_pm_runtime_resume(struct xe_device *xe)
> if (!xe->info.probe_display)
> return;
>
> - if (xe->d3cold.allowed)
> - xe_display_from_d3cold(xe);
> -
> - intel_hpd_init(xe);
> - intel_hpd_poll_disable(xe);
> - skl_watermark_ipc_update(xe);
> + intel_display_driver_runtime_resume(xe, xe->d3cold.allowed);
> }
>
> static void display_device_remove(struct drm_device *dev, void *arg)
> --
> 2.46.0
>
>
