[PATCH 1/2] drm/xe: Avoid fbdev suspend at runtime pm suspend

[Francois Dugast]

On Mon, Jun 03, 2024 at 05:52:17PM -0400, Rodrigo Vivi wrote:
> There's no need to suspend fbdev upon runtime_pm, and it is upseting
> lockdep:
> 
> [  313.150411] ======================================================
> [  313.150412] WARNING: possible circular locking dependency detected
> [  313.150413] 6.10.0-rc1pz+ #99 Tainted: G            E
> [  313.150414] ------------------------------------------------------
> [  313.150414] kworker/2:0/33 is trying to acquire lock:
> [  313.150415] ffffffffb5f61340 (console_lock){+.+.}-{0:0}, at: intel_fbdev_set_suspend+0x115/0x1e0 [xe]
> [  313.150495]
>                but task is already holding lock:
> [  313.150495] ffffffffc0ce74a0 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}, at: xe_pm_runtime_suspend+0x2a/0x2e0 [xe]
> [  313.150561]
>                which lock already depends on the new lock.
> [snip]
> 
>                -> #2 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}:
> [  313.150563]        xe_pm_runtime_resume_and_get+0x44/0xb0 [xe]
> [  313.150626]        intelfb_create+0x150/0x360 [xe]
> [snip]
> [  313.150932]        xe_device_probe+0x439/0x4a0 [xe]
>                -> #1 (&helper->lock){+.+.}-{3:3}:
> [snip]
> [  313.151097]        intel_fbdev_set_par+0x16/0x60 [xe]
> [snip]
> [  313.151402]        xe_device_probe+0x439/0x4a0 [xe]
> [snip]
>                -> #0 (console_lock){+.+.}-{0:0}:
> [snip]
> [  313.151548]        intel_fbdev_set_suspend+0x115/0x1e0 [xe]
> [  313.151607]        xe_display_pm_suspend+0xd1/0x100 [xe]
> [  313.151679]        xe_pm_runtime_suspend+0x2bf/0x2e0 [xe]
> [snip]
> [  313.151817] Chain exists of:
>                  console_lock --> &helper->lock --> xe_pm_runtime_lockdep_map
> [  313.151819]  Possible unsafe locking scenario:
> [  313.151819]        CPU0                    CPU1
> [  313.151820]        ----                    ----
> [  313.151820]   lock(xe_pm_runtime_lockdep_map);
> [  313.151821]                                lock(&helper->lock);
> [  313.151821]                                lock(xe_pm_runtime_lockdep_map);
> [  313.151822]   lock(console_lock);
> [  313.151823]
>                 *** DEADLOCK ***
> 
> Although we know that this scenario is a false positive because
> runtime_pm suspend can never happen while the probe hasn't finished,
> the best approach is to avoid the unnecessary calls.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Anshuman Gupta <anshuman.gupta at intel.com>
> Cc: Francois Dugast <francois.dugast at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

LGTM, maybe add kernel doc to those 2 functions so that the meaning
of the "runtime" argument is explicit:

	Reviewed-by: Francois Dugast <francois.dugast at intel.com>

> ---
>  drivers/gpu/drm/xe/display/xe_display.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 2a9b4a4f2e71..cd97975344f1 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -333,7 +333,8 @@ void xe_display_pm_suspend(struct xe_device *xe, bool runtime)
>  
>  	intel_opregion_suspend(xe, s2idle ? PCI_D1 : PCI_D3cold);
>  
> -	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
> +	if (!runtime)
> +		intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_SUSPENDED, true);
>  
>  	intel_dmc_suspend(xe);
>  }
> @@ -383,7 +384,8 @@ void xe_display_pm_resume(struct xe_device *xe, bool runtime)
>  
>  	intel_opregion_resume(xe);
>  
> -	intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false);
> +	if (!runtime)
> +		intel_fbdev_set_suspend(&xe->drm, FBINFO_STATE_RUNNING, false);
>  
>  	intel_power_domains_enable(xe);
>  }
> -- 
> 2.45.1
>