-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Rodrigo Vivi
Sent: Friday, August 30, 2024 11:35 AM
To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx
Cc: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Deak, Imre <imre.deak@xxxxxxxxx>
Subject: [PATCH 2/2] drm/xe/display: Avoid encoder_suspend at runtime suspend
>
> Fix circular locking dependency on runtime suspend.
>
> <4> [74.952215] ======================================================
> <4> [74.952217] WARNING: possible circular locking dependency detected
> <4> [74.952219] 6.10.0-rc7-xe #1 Not tainted
> <4> [74.952221] ------------------------------------------------------
> <4> [74.952223] kworker/7:1/82 is trying to acquire lock:
> <4> [74.952226] ffff888120548488 (&dev->mode_config.mutex){+.+.}-{3:3}, at: drm_modeset_lock_all+0x40/0x1e0 [drm]
> <4> [74.952260]
> but task is already holding lock:
> <4> [74.952262] ffffffffa0ae59c0 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}, at: xe_pm_runtime_suspend+0x2f/0x340 [xe]
> <4> [74.952322]
> which lock already depends on the new lock.
>
> The commit b1d90a86 ("drm/xe: Use the encoder suspend helper also used
> by the i915 driver") didn't do anything wrong. It actually fixed a
> critical bug, because the encoder_suspend was never getting actually
> called because it was returning if (has_display(xe)) instead of
> if (!has_display(xe)). However, this ended up introducing the encoder
> suspend calls in the runtime routines as well, causing the circular
> locking dependency.
>
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2304
> Fixes: b1d90a862c89 ("drm/xe: Use the encoder suspend helper also used by the i915 driver")
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
LGTM.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
-Jonathan Cavitt
> ---
> drivers/gpu/drm/xe/display/xe_display.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index ae92012253f8..75736faf2a80 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -345,10 +345,10 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
>
> intel_hpd_cancel_work(xe);
>
> - if (!runtime && has_display(xe))
> + if (!runtime && has_display(xe)) {
> intel_display_driver_suspend_access(xe);
> -
> - intel_encoder_suspend_all(&xe->display);
> + intel_encoder_suspend_all(&xe->display);
> + }
>
> intel_opregion_suspend(display, s2idle ? PCI_D1 : PCI_D3cold);
>
> --
> 2.46.0
>
>
