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