Messages in this thread | | | Date | Fri, 22 Apr 2022 09:04:57 -0700 | Subject | Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP | From | Abhinav Kumar <> |
| |
Hi Doug
For the lockdep error, the splat looks similar to what kuogee fixed recently.
Can you please check if below patch is present in your tree?
https://patchwork.freedesktop.org/patch/481396/
Thanks
Abhinav On 4/22/2022 8:55 AM, Doug Anderson wrote: > Hi, > > On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti > <quic_sbillaka@quicinc.com> wrote: >> >> The panel-edp enables the eDP panel power during probe, get_modes >> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP >> controller are directly dependent on panel power. As eDP display can be >> assumed as always connected, the controller driver can skip the eDP >> connect and disconnect interrupts. Any disruption in the link status >> will be indicated via the IRQ_HPD interrupts. >> >> So, the eDP controller driver can just enable the IRQ_HPD and replug >> interrupts. The DP controller driver still needs to enable all the >> interrupts. >> >> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> >> --- >> Changes in v9: >> - add comment explaining the interrupt status register >> >> Changes in v8: >> - add comment explaining the interrupt status return >> >> Changes in v7: >> - reordered the patch in the series >> - modified the return statement for isr >> - connector check modified to just check for eDP >> >> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------ >> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++- >> 2 files changed, 31 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c >> index fac815f..df9670d 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c >> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) >> >> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); >> >> - /* enable HPD plug and unplug interrupts */ >> - dp_catalog_hpd_config_intr(dp_catalog, >> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true); >> - >> /* Configure REFTIMER and enable it */ >> reftimer |= DP_DP_HPD_REFTIMER_ENABLE; >> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); >> @@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) >> { >> struct dp_catalog_private *catalog = container_of(dp_catalog, >> struct dp_catalog_private, dp_catalog); >> - int isr = 0; >> + int isr, mask; >> >> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); >> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, >> (isr & DP_DP_HPD_INT_MASK)); >> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); >> >> - return isr; >> + /* >> + * We only want to return interrupts that are unmasked to the caller. >> + * However, the interrupt status field also contains other >> + * informational bits about the HPD state status, so we only mask >> + * out the part of the register that tells us about which interrupts >> + * are pending. >> + */ >> + return isr & (mask | ~DP_DP_HPD_INT_MASK); >> } >> >> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index 055681a..dea4de9 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) >> dp_display_handle_plugged_change(&dp->dp_display, false); >> >> /* enable HDP plug interrupt to prepare for next plugin */ >> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); >> + if (!dp->dp_display.is_edp) >> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); >> >> DRM_DEBUG_DP("After, type=%d hpd_state=%d\n", >> dp->dp_display.connector_type, state); >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp) >> dp_display_host_init(dp); >> dp_catalog_ctrl_hpd_config(dp->catalog); >> >> + /* Enable plug and unplug interrupts only for external DisplayPort */ >> + if (!dp->dp_display.is_edp) >> + dp_catalog_hpd_config_intr(dp->catalog, >> + DP_DP_HPD_PLUG_INT_MASK | >> + DP_DP_HPD_UNPLUG_INT_MASK, >> + true); >> + >> /* Enable interrupt first time >> * we are leaving dp clocks on during disconnect >> * and never disable interrupt >> @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev) >> dp_catalog_ctrl_hpd_config(dp->catalog); >> >> >> + if (!dp->dp_display.is_edp) >> + dp_catalog_hpd_config_intr(dp->catalog, >> + DP_DP_HPD_PLUG_INT_MASK | >> + DP_DP_HPD_UNPLUG_INT_MASK, >> + true); >> + >> if (dp_catalog_link_is_connected(dp->catalog)) { >> /* >> * set sink to normal operation mode -- D0 >> @@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) >> return; >> } >> >> + if (dp->is_edp) >> + dp_hpd_plug_handle(dp_display, 0); > > So I finally got a chance to test and unfortunately this is getting a > lockdep error. :( Here's the crawl with my current set of patches > (which, admittedly is on the chromeos-5.15 tree) instead of pure > upstream. I avoid the errors with this (sorry for the whitespace > damage, but it's really just a one-line change): > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -582,7 +582,8 @@ static int dp_hpd_plug_handle(struct > dp_display_private *dp, u32 data) > * add fail safe mode outside event_mutex scope > * to avoid potiential circular lock with drm thread > */ > - dp_panel_add_fail_safe_mode(dp->dp_display.connector); > + if (!dp->dp_display.is_edp) > + dp_panel_add_fail_safe_mode(dp->dp_display.connector); > > /* uevent will complete connection part */ > return 0; > > That's a bit gross, but at least shows the problem. It's not a > _terrible_ fix because the failsafe modes don't make sense for eDP, > right? That being said, why are we hacking this in here? Shouldn't > this be in the core? ...or at least we should just be providing them > in get_modes()? > > FWIW: the error was: > > ====================================================== > WARNING: possible circular locking dependency detected > 5.15.35-lockdep #6 Tainted: G W > ------------------------------------------------------ > frecon/429 is trying to acquire lock: > ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at: > dp_panel_add_fail_safe_mode+0x4c/0xa0 > > but task is already holding lock: > ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124 > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #3 (&kms->commit_lock[i]){+.+.}-{3:3}: > __mutex_lock_common+0x174/0x1a64 > mutex_lock_nested+0x98/0xac > lock_crtcs+0xb4/0x124 > msm_atomic_commit_tail+0x330/0x748 > commit_tail+0x19c/0x278 > drm_atomic_helper_commit+0x1dc/0x1f0 > drm_atomic_commit+0xc0/0xd8 > drm_atomic_helper_set_config+0xb4/0x134 > drm_mode_setcrtc+0x688/0x1248 > drm_ioctl_kernel+0x1e4/0x338 > drm_ioctl+0x3a4/0x684 > __arm64_sys_ioctl+0x118/0x154 > invoke_syscall+0x78/0x224 > el0_svc_common+0x178/0x200 > do_el0_svc+0x94/0x13c > el0_svc+0x5c/0xec > el0t_64_sync_handler+0x78/0x108 > el0t_64_sync+0x1a4/0x1a8 > > -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}: > __mutex_lock_common+0x174/0x1a64 > ww_mutex_lock+0xb8/0x278 > modeset_lock+0x304/0x4ac > drm_modeset_lock+0x4c/0x7c > drmm_mode_config_init+0x4a8/0xc50 > msm_drm_init+0x274/0xac0 > msm_drm_bind+0x20/0x2c > try_to_bring_up_master+0x3dc/0x470 > __component_add+0x18c/0x3c0 > component_add+0x1c/0x28 > dp_display_probe+0x954/0xa98 > platform_probe+0x124/0x15c > really_probe+0x1b0/0x5f8 > __driver_probe_device+0x174/0x20c > driver_probe_device+0x70/0x134 > __device_attach_driver+0x130/0x1d0 > bus_for_each_drv+0xfc/0x14c > __device_attach+0x1bc/0x2bc > device_initial_probe+0x1c/0x28 > bus_probe_device+0x94/0x178 > deferred_probe_work_func+0x1a4/0x1f0 > process_one_work+0x5d4/0x9dc > worker_thread+0x898/0xccc > kthread+0x2d4/0x3d4 > ret_from_fork+0x10/0x20 > > -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}: > ww_acquire_init+0x1c4/0x2c8 > drm_modeset_acquire_init+0x44/0xc8 > drm_helper_probe_single_connector_modes+0xb0/0x12dc > drm_mode_getconnector+0x5dc/0xfe8 > drm_ioctl_kernel+0x1e4/0x338 > drm_ioctl+0x3a4/0x684 > __arm64_sys_ioctl+0x118/0x154 > invoke_syscall+0x78/0x224 > el0_svc_common+0x178/0x200 > do_el0_svc+0x94/0x13c > el0_svc+0x5c/0xec > el0t_64_sync_handler+0x78/0x108 > el0t_64_sync+0x1a4/0x1a8 > > -> #0 (&dev->mode_config.mutex){+.+.}-{3:3}: > __lock_acquire+0x2650/0x672c > lock_acquire+0x1b4/0x4ac > __mutex_lock_common+0x174/0x1a64 > mutex_lock_nested+0x98/0xac > dp_panel_add_fail_safe_mode+0x4c/0xa0 > dp_hpd_plug_handle+0x1f0/0x280 > dp_bridge_enable+0x94/0x2b8 > drm_atomic_bridge_chain_enable+0x11c/0x168 > drm_atomic_helper_commit_modeset_enables+0x500/0x740 > msm_atomic_commit_tail+0x3e4/0x748 > commit_tail+0x19c/0x278 > drm_atomic_helper_commit+0x1dc/0x1f0 > drm_atomic_commit+0xc0/0xd8 > drm_atomic_helper_set_config+0xb4/0x134 > drm_mode_setcrtc+0x688/0x1248 > drm_ioctl_kernel+0x1e4/0x338 > drm_ioctl+0x3a4/0x684 > __arm64_sys_ioctl+0x118/0x154 > invoke_syscall+0x78/0x224 > el0_svc_common+0x178/0x200 > do_el0_svc+0x94/0x13c > el0_svc+0x5c/0xec > el0t_64_sync_handler+0x78/0x108 > el0t_64_sync+0x1a4/0x1a8 > > other info that might help us debug this: > > Chain exists of: > &dev->mode_config.mutex --> crtc_ww_class_mutex --> &kms->commit_lock[i] > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(&kms->commit_lock[i]); > lock(crtc_ww_class_mutex); > lock(&kms->commit_lock[i]); > lock(&dev->mode_config.mutex); > > *** DEADLOCK *** > > 3 locks held by frecon/429: > #0: ffffffc00e197ab0 (crtc_ww_class_acquire){+.+.}-{0:0}, at: > drm_modeset_acquire_init+0x44/0xc8 > #1: ffffff808dc3c588 (crtc_ww_class_mutex){+.+.}-{3:3}, at: > modeset_lock+0x18c/0x4ac > #2: ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: > lock_crtcs+0xb4/0x124 > > stack backtrace: > CPU: 5 PID: 429 Comm: frecon Tainted: G W > 5.15.35-lockdep #6 9ba2ecd8f15354021fe165873da3aaa99f5b6798 > Hardware name: Google Herobrine (rev1+) (DT) > Call trace: > dump_backtrace+0x0/0x3c4 > show_stack+0x20/0x2c > dump_stack_lvl+0x78/0x9c > dump_stack+0x18/0x44 > print_circular_bug+0x17c/0x1a8 > check_noncircular+0x260/0x30c > __lock_acquire+0x2650/0x672c > lock_acquire+0x1b4/0x4ac > __mutex_lock_common+0x174/0x1a64 > mutex_lock_nested+0x98/0xac > dp_panel_add_fail_safe_mode+0x4c/0xa0 > dp_hpd_plug_handle+0x1f0/0x280 > dp_bridge_enable+0x94/0x2b8 > drm_atomic_bridge_chain_enable+0x11c/0x168 > drm_atomic_helper_commit_modeset_enables+0x500/0x740 > msm_atomic_commit_tail+0x3e4/0x748 > commit_tail+0x19c/0x278 > drm_atomic_helper_commit+0x1dc/0x1f0 > drm_atomic_commit+0xc0/0xd8 > drm_atomic_helper_set_config+0xb4/0x134 > drm_mode_setcrtc+0x688/0x1248 > drm_ioctl_kernel+0x1e4/0x338 > drm_ioctl+0x3a4/0x684 > __arm64_sys_ioctl+0x118/0x154 > invoke_syscall+0x78/0x224 > el0_svc_common+0x178/0x200 > do_el0_svc+0x94/0x13c > el0_svc+0x5c/0xec > el0t_64_sync_handler+0x78/0x108 > el0t_64_sync+0x1a4/0x1a8
| |