Re: [PATCH 1/2] drm/xe/display: Do not suspend resume dp mst during runtime

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Sep 13, 2024 at 04:38:58PM -0400, Rodrigo Vivi wrote:
> On Thu, Sep 12, 2024 at 06:55:44AM +0530, Suraj Kandpal wrote:
> > Remove intel_dp_mst_suspend/resume from runtime suspend resume
> > sequences. It is incorrect as it depends on AUX transfers which
> > itself depend on the device being runtime resumed. This is
> > also why we see a lock_dep splat here.
> > 
> > <4> [76.011119] kworker/4:2/192 is trying to acquire lock:
> > <4> [76.011122] ffff8881120b3210 (&mgr->lock#2){+.+.}-{3:3}, at:
> > drm_dp_mst_topology_mgr_suspend+0x33/0xd0 [drm_display_helper]
> > <4> [76.011142]
> > but task is already holding lock:
> > <4> [76.011144] ffffffffa0bc3420
> > (xe_pm_runtime_lockdep_map){+.+.}-{0:0}, at:
> > xe_pm_runtime_suspend+0x51/0x3f0 [xe]
> > <4> [76.011223]
> > which lock already depends on the new lock.
> > <4> [76.011226]
> > the existing dependency chain (in reverse order) is:
> > <4> [76.011229]
> > -> #2 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}:
> > <4> [76.011233]        pm_runtime_lockdep_prime+0x2f/0x50 [xe]
> > <4> [76.011306]        xe_pm_runtime_resume_and_get+0x29/0x90 [xe]
> > <4> [76.011377]        intel_display_power_get+0x24/0x70 [xe]
> > <4> [76.011466]        intel_digital_port_connected_locked+0x4c/0xf0
> > [xe]
> > <4> [76.011551]        intel_dp_aux_xfer+0xb8/0x7c0 [xe]
> > <4> [76.011633]        intel_dp_aux_transfer+0x166/0x2e0 [xe]
> > <4> [76.011715]        drm_dp_dpcd_access+0x87/0x150
> > [drm_display_helper]
> > <4> [76.011726]        drm_dp_dpcd_probe+0x3d/0xf0 [drm_display_helper]
> > <4> [76.011737]        drm_dp_dpcd_read+0xdd/0x130 [drm_display_helper]
> > <4> [76.011747]        intel_dp_get_colorimetry_status+0x3a/0x70 [xe]
> > <4> [76.011886]        intel_dp_init_connector+0x4ff/0x1030 [xe]
> > <4> [76.011969]        intel_ddi_init+0xc5b/0x1030 [xe]
> > <4> [76.012058]        intel_bios_for_each_encoder+0x36/0x60 [xe]
> > <4> [76.012145]        intel_setup_outputs+0x201/0x460 [xe]
> > <4> [76.012233]        intel_display_driver_probe_nogem+0x155/0x1e0 [xe]
> > <4> [76.012320]        xe_display_init_noaccel+0x27/0x70 [xe]
> 
> could you please share the full in a link with me?
> I believe we can improve this. This seems incomplete...

Thanks for sharing the link:
https://intel-gfx-ci.01.org/tree/intel-xe/xe-1775-f0a6824d9e4ba2e1beabab4e3eeb195aa7fea167/re-dg2-17/igt@xe_pm@d3cold-mmap-vram.html#dmesg-warnings266

What about something like this:

WARNING: possible circular locking dependency detected
 6.11.0-rc3-xe #1 Not tainted
 ------------------------------------------------------
 kworker/4:2/192 is trying to acquire lock:
 ffff8881120b3210 (&mgr->lock#2){+.+.}-{3:3}, at: drm_dp_mst_topology_mgr_suspend+0x33/0xd0 [drm_display_helper]
but task is already holding lock:
ffffffffa0bc3420 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}, at: xe_pm_runtime_suspend+0x51/0x3f0 [xe]

-> #2 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}:
        drm_dp_dpcd_access+0x87/0x150 [drm_display_helper]
        xe_device_probe+0x8dd/0xbf0 [xe]

-> #1 (&aux->hw_mutex){+.+.}-{3:3}:
        drm_dp_dpcd_access+0x55/0x150 [drm_display_helper]
	drm_dp_mst_topology_mgr_set_mst+0x8c/0x2e0 [drm_display_helper]
        i915_hpd_poll_init_work+0x1d1/0x290 [xe]

-> #0 (&mgr->lock#2){+.+.}-{3:3}:
        drm_dp_mst_topology_mgr_suspend+0x33/0xd0 [drm_display_helper]
        intel_dp_mst_suspend+0x7e/0xa0 [xe]
        xe_display_pm_suspend+0x59/0xc0 [xe]

Possible unsafe locking scenario:
      CPU0                    CPU1
      ----                    ----
 lock(xe_pm_runtime_lockdep_map);
                              lock(&aux->hw_mutex);
                              lock(xe_pm_runtime_lockdep_map);
 lock(&mgr->lock#2);
 *** DEADLOCK ***
 3 locks held by kworker/4:2/192:
  #0: ffff888101546348 ((wq_completion)pm){+.+.}-{0:0}, at: process_scheduled_works+0x5bb/0x730
  #1: ffffc900005b3e48 ((work_completion)(&dev->power.work)){+.+.}-{0:0}, at: process_scheduled_works+0x35b/0x730
  #2: ffffffffa0bc3420 (xe_pm_runtime_lockdep_map){+.+.}-{0:0}, at: xe_pm_runtime_suspend+0x51/0x3f0 [xe]

 Call Trace:
[snip]
  intel_dp_mst_suspend+0x7e/0xa0 [xe]

> 
> > 
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/display/xe_display.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> > index e65e7d70f989..39da79069b5a 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > @@ -331,7 +331,8 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> >  
> >  	xe_display_flush_cleanup_work(xe);
> >  
> > -	intel_dp_mst_suspend(xe);
> > +	if (!runtime)
> > +		intel_dp_mst_suspend(xe);
> >  
> >  	intel_hpd_cancel_work(xe);
> >  
> > @@ -444,7 +445,9 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime)
> >  		intel_display_driver_resume_access(xe);
> >  
> >  	/* MST sideband requires HPD interrupts enabled */
> > -	intel_dp_mst_resume(xe);
> > +	if (!runtime)
> > +		intel_dp_mst_resume(xe);
> > +
> >  	if (!runtime && has_display(xe)) {
> >  		intel_display_driver_resume(xe);
> >  		drm_kms_helper_poll_enable(&xe->drm);
> > -- 
> > 2.43.2
> >