[PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
Abhinav Kumar
quic_abhinavk at quicinc.com
Fri Apr 22 16:04:57 UTC 2022
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 at 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 at 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
More information about the dri-devel
mailing list