diff mbox series

[23/31] drm/xe/display: Prepare runtime pm functions

Message ID 20240924204222.246862-24-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show
Series Reconcile i915's and xe's display power mgt sequences | expand

Commit Message

Rodrigo Vivi Sept. 24, 2024, 8:35 p.m. UTC 
No functional change. Just organize the runtime_pm related
functions to allow a later sync with the i915.

Move runtime_suspend down near the runtime_resume.
Create runtime_suspend_late and runtime_suspend_early
stages for a better visualization of the missed i915
sequences.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/xe/display/xe_display.c | 41 +++++++++++++++++--------
 drivers/gpu/drm/xe/display/xe_display.h |  2 ++
 drivers/gpu/drm/xe/xe_pm.c              |  7 +++--
 3 files changed, 35 insertions(+), 15 deletions(-)

Comments

Cavitt, Jonathan Oct. 8, 2024, 2:31 p.m. UTC | #1 
-----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 23/31] drm/xe/display: Prepare runtime pm functions
> 
> No functional change. Just organize the runtime_pm related
> functions to allow a later sync with the i915.
> 
> Move runtime_suspend down near the runtime_resume.
> Create runtime_suspend_late and runtime_suspend_early
> stages for a better visualization of the missed i915
> sequences.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/display/xe_display.c | 41 +++++++++++++++++--------
>  drivers/gpu/drm/xe/display/xe_display.h |  2 ++
>  drivers/gpu/drm/xe/xe_pm.c              |  7 +++--
>  3 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 6bfad26a3c06..1ab4dd51094f 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -388,17 +388,6 @@ void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
>  	intel_display_driver_shutdown_nogem(xe);
>  }
>  
> -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);
> -
> -	intel_hpd_poll_enable(xe);
> -}
> -
>  void xe_display_pm_suspend_late(struct xe_device *xe)
>  {
>  	bool s2idle = suspend_to_idle();
> @@ -443,6 +432,35 @@ void xe_display_pm_resume_noaccel(struct xe_device *xe)
>  	intel_display_driver_resume_nogem(&xe->display);
>  }
>  
> +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);
> +
> +	intel_hpd_poll_enable(xe);
> +}
> +
> +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);
> +}
> +
> +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);
> +}
> +
>  void xe_display_pm_runtime_resume(struct xe_device *xe)
>  {
>  	if (!xe->info.probe_display)
> @@ -454,7 +472,6 @@ void xe_display_pm_runtime_resume(struct xe_device *xe)
>  		xe_display_from_d3cold(xe);
>  }
>  
> -
>  static void display_device_remove(struct drm_device *dev, void *arg)
>  {
>  	struct xe_device *xe = arg;
> diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
> index 256bd2d23964..64ff2d2f5270 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.h
> +++ b/drivers/gpu/drm/xe/display/xe_display.h
> @@ -46,6 +46,8 @@ void xe_display_pm_resume(struct xe_device *xe);
>  void xe_display_pm_resume_noirq(struct xe_device *xe);
>  void xe_display_pm_resume_noaccel(struct xe_device *xe);
>  void xe_display_pm_runtime_suspend(struct xe_device *xe);
> +void xe_display_pm_runtime_suspend_late(struct xe_device *xe);
> +void xe_display_pm_runtime_resume_early(struct xe_device *xe);
>  void xe_display_pm_runtime_resume(struct xe_device *xe);
>  
>  #else
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 77eb45a641e8..4cacf4b33d83 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -418,8 +418,7 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>  
>  	xe_irq_suspend(xe);
>  
> -	if (xe->d3cold.allowed)
> -		xe_display_pm_suspend_late(xe);
> +	xe_display_pm_runtime_suspend_late(xe);
>  out:
>  	if (err)
>  		xe_display_pm_runtime_resume(xe);
> @@ -450,9 +449,11 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>  		err = xe_pcode_ready(xe, true);
>  		if (err)
>  			goto out;
> +	}
>  
> -		xe_display_pm_resume_early(xe);
> +	xe_display_pm_runtime_resume_early(xe);

Putting a split here makes us check the above and below xe->d3cold.allowed status
twice.  I think it's mandatory for us to do so, but I can't help but wonder if we can't
streamline it a bit.  Maybe breaking the above and below checks into their own
functions?  Something like:

"""
static int xe_pcode_ready_on_d3cold(struct xe_device *xe)
{
	if (xe->d3cold.allowed)
		return xe_pcode_ready(xe, true);
	return 0;
}

static int xe_bo_restore_on_d3cold(struct xe_device *xe)
{
	if (xe->d3cold.allowed)
		return xe_bo_restore_kernel(xe);
	return 0;
}
...
int xe_pm_runtime_resume(struct xe_device *xe)
{
	struct xe_gt *gt;
	u8 id;
	int err = 0;

	trace_xe_pm_runtime_resume(xe, __builtin_return_address(0));
	/* Disable access_ongoing asserts and prevent recursive pm calls */
	xe_pm_write_callback_task(xe, current);

	xe_rpm_lockmap_acquire(xe);

	err = xe_pcode_ready_on_d3cold(xe);
	if (err)
		goto out;
	
	xe_display_pm_runtime_resume_early(xe);

	err = xe_bo_restore_on_d3cold(xe);
	if (err)
		goto out;
"""

Or perhaps it'd work better as an inline function?

"""
	err = xe->d3cold.allowed ?
		xe_pcode_ready(xe, true) : 0;
	if (err)
		goto out;

	xe_display_pm_runtime_resume_early(xe);

	err = xe->d3cold.allowed ?
		xe_bo_restore_kernel(xe) : 0;
	if (err)
		goto out;
"""

IMO I don't think either of these new options are particular upgrades to
the current implementation.  If anything, they're probably just side-grades.
So I won't force the issue.

Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt
	         
>  
> +	if (xe->d3cold.allowed) {
>  		/*
>  		 * This only restores pinned memory which is the memory
>  		 * required for the GT(s) to resume.
> -- 
> 2.46.0
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index 6bfad26a3c06..1ab4dd51094f 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -388,17 +388,6 @@  void xe_display_pm_shutdown_noaccel(struct xe_device *xe)
 	intel_display_driver_shutdown_nogem(xe);
 }
 
-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);
-
-	intel_hpd_poll_enable(xe);
-}
-
 void xe_display_pm_suspend_late(struct xe_device *xe)
 {
 	bool s2idle = suspend_to_idle();
@@ -443,6 +432,35 @@  void xe_display_pm_resume_noaccel(struct xe_device *xe)
 	intel_display_driver_resume_nogem(&xe->display);
 }
 
+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);
+
+	intel_hpd_poll_enable(xe);
+}
+
+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);
+}
+
+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);
+}
+
 void xe_display_pm_runtime_resume(struct xe_device *xe)
 {
 	if (!xe->info.probe_display)
@@ -454,7 +472,6 @@  void xe_display_pm_runtime_resume(struct xe_device *xe)
 		xe_display_from_d3cold(xe);
 }
 
-
 static void display_device_remove(struct drm_device *dev, void *arg)
 {
 	struct xe_device *xe = arg;
diff --git a/drivers/gpu/drm/xe/display/xe_display.h b/drivers/gpu/drm/xe/display/xe_display.h
index 256bd2d23964..64ff2d2f5270 100644
--- a/drivers/gpu/drm/xe/display/xe_display.h
+++ b/drivers/gpu/drm/xe/display/xe_display.h
@@ -46,6 +46,8 @@  void xe_display_pm_resume(struct xe_device *xe);
 void xe_display_pm_resume_noirq(struct xe_device *xe);
 void xe_display_pm_resume_noaccel(struct xe_device *xe);
 void xe_display_pm_runtime_suspend(struct xe_device *xe);
+void xe_display_pm_runtime_suspend_late(struct xe_device *xe);
+void xe_display_pm_runtime_resume_early(struct xe_device *xe);
 void xe_display_pm_runtime_resume(struct xe_device *xe);
 
 #else
diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 77eb45a641e8..4cacf4b33d83 100644
--- a/drivers/gpu/drm/xe/xe_pm.c
+++ b/drivers/gpu/drm/xe/xe_pm.c
@@ -418,8 +418,7 @@  int xe_pm_runtime_suspend(struct xe_device *xe)
 
 	xe_irq_suspend(xe);
 
-	if (xe->d3cold.allowed)
-		xe_display_pm_suspend_late(xe);
+	xe_display_pm_runtime_suspend_late(xe);
 out:
 	if (err)
 		xe_display_pm_runtime_resume(xe);
@@ -450,9 +449,11 @@  int xe_pm_runtime_resume(struct xe_device *xe)
 		err = xe_pcode_ready(xe, true);
 		if (err)
 			goto out;
+	}
 
-		xe_display_pm_resume_early(xe);
+	xe_display_pm_runtime_resume_early(xe);
 
+	if (xe->d3cold.allowed) {
 		/*
 		 * This only restores pinned memory which is the memory
 		 * required for the GT(s) to resume.