While running the kernel with the mutex debugging enabled, I noticed the warning reported in the first commit (that the bridge's hpd_mutex is unitialized). Additional research revealed that we do not add and remove internal drm_bridges in the msm drm driver. Fix this now.
Dmitry Baryshkov (4): drm/msm/hdmi: properly add and remove created bridges drm/msm/dsi: properly add and remove created bridges drm/msm/dp: properly add and remove created bridges drm/msm: unregister all internal bridges
drivers/gpu/drm/msm/dp/dp_drm.c | 5 +++++ drivers/gpu/drm/msm/dsi/dsi_manager.c | 3 +++ drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +++ drivers/gpu/drm/msm/msm_drv.c | 3 +++ 4 files changed, 14 insertions(+)
Add calls to drm_bridge_add()/drm_bridge_remove() for the internal HDMI bridges. This fixes the following warning.
[ 2.195003] ------------[ cut here ]------------ [ 2.195044] WARNING: CPU: 0 PID: 1 at kernel/locking/mutex.c:579 __mutex_lock+0x840/0x9f4 [ 2.198774] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [ 2.198786] Modules linked in: [ 2.211868] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-00002-g3054695a0d27-dirty #55 [ 2.214671] Hardware name: Generic DT based system [ 2.223265] unwind_backtrace from show_stack+0x10/0x14 [ 2.228036] show_stack from dump_stack_lvl+0x58/0x70 [ 2.233159] dump_stack_lvl from __warn+0xc8/0x1e8 [ 2.238367] __warn from warn_slowpath_fmt+0x78/0xa8 [ 2.243054] warn_slowpath_fmt from __mutex_lock+0x840/0x9f4 [ 2.248174] __mutex_lock from mutex_lock_nested+0x1c/0x24 [ 2.253818] mutex_lock_nested from drm_bridge_hpd_enable+0x2c/0x84 [ 2.259116] drm_bridge_hpd_enable from msm_hdmi_modeset_init+0xc0/0x21c [ 2.265279] msm_hdmi_modeset_init from mdp4_kms_init+0x53c/0x90c [ 2.272223] mdp4_kms_init from msm_drm_bind+0x514/0x698 [ 2.278212] msm_drm_bind from try_to_bring_up_aggregate_device+0x160/0x1bc [ 2.283595] try_to_bring_up_aggregate_device from component_master_add_with_match+0xc4/0xf8 [ 2.290281] component_master_add_with_match from msm_pdev_probe+0x274/0x350 [ 2.298958] msm_pdev_probe from platform_probe+0x5c/0xbc [ 2.305990] platform_probe from really_probe.part.0+0x9c/0x290 [ 2.311284] really_probe.part.0 from __driver_probe_device+0xa8/0x13c [ 2.317011] __driver_probe_device from driver_probe_device+0x34/0x10c [ 2.323610] driver_probe_device from __driver_attach+0xbc/0x178 [ 2.330122] __driver_attach from bus_for_each_dev+0x74/0xc0 [ 2.336282] bus_for_each_dev from bus_add_driver+0x160/0x1e4 [ 2.341925] bus_add_driver from driver_register+0x88/0x118 [ 2.347570] driver_register from do_one_initcall+0x6c/0x334 [ 2.352953] do_one_initcall from kernel_init_freeable+0x1bc/0x220 [ 2.358853] kernel_init_freeable from kernel_init+0x18/0x12c [ 2.364842] kernel_init from ret_from_fork+0x14/0x2c [ 2.370657] Exception stack(0xf081dfb0 to 0xf081dff8) [ 2.375693] dfa0: 00000000 00000000 00000000 00000000 [ 2.380733] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 2.388893] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 2.397170] irq event stamp: 202911 [ 2.403551] hardirqs last enabled at (202911): [<c0f86044>] _raw_spin_unlock_irqrestore+0x44/0x48 [ 2.406944] hardirqs last disabled at (202910): [<c0f85e18>] _raw_spin_lock_irqsave+0x68/0x6c [ 2.416063] softirqs last enabled at (202738): [<c03015f0>] __do_softirq+0x2f8/0x530 [ 2.424638] softirqs last disabled at (202733): [<c032bc68>] __irq_exit_rcu+0x14c/0x170 [ 2.432453] ---[ end trace 0000000000000000 ]---
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index 10ebe2089cb6..97c24010c4d1 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -15,6 +15,7 @@ void msm_hdmi_bridge_destroy(struct drm_bridge *bridge) struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
msm_hdmi_hpd_disable(hdmi_bridge); + drm_bridge_remove(bridge); }
static void msm_hdmi_power_on(struct drm_bridge *bridge) @@ -349,6 +350,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi) DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+ drm_bridge_add(bridge); + ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (ret) goto fail;
Quoting Dmitry Baryshkov (2022-04-05 16:45:48)
Add calls to drm_bridge_add()/drm_bridge_remove() for the internal HDMI bridges. This fixes the following warning.
[ 2.195003] ------------[ cut here ]------------
Usually this line is left out
[ 2.195044] WARNING: CPU: 0 PID: 1 at kernel/locking/mutex.c:579 __mutex_lock+0x840/0x9f4
And the timestamp is removed
[ 2.198774] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [ 2.198786] Modules linked in: [ 2.211868] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-00002-g3054695a0d27-dirty #55 [ 2.214671] Hardware name: Generic DT based system [ 2.223265] unwind_backtrace from show_stack+0x10/0x14 [ 2.228036] show_stack from dump_stack_lvl+0x58/0x70 [ 2.233159] dump_stack_lvl from __warn+0xc8/0x1e8 [ 2.238367] __warn from warn_slowpath_fmt+0x78/0xa8 [ 2.243054] warn_slowpath_fmt from __mutex_lock+0x840/0x9f4 [ 2.248174] __mutex_lock from mutex_lock_nested+0x1c/0x24 [ 2.253818] mutex_lock_nested from drm_bridge_hpd_enable+0x2c/0x84 [ 2.259116] drm_bridge_hpd_enable from msm_hdmi_modeset_init+0xc0/0x21c [ 2.265279] msm_hdmi_modeset_init from mdp4_kms_init+0x53c/0x90c [ 2.272223] mdp4_kms_init from msm_drm_bind+0x514/0x698 [ 2.278212] msm_drm_bind from try_to_bring_up_aggregate_device+0x160/0x1bc
I'd probably cut it off here.
Is there any Fixes tag for this? Still seems worthwhile to have one even if this is a lockdep warning.
On 06/04/2022 05:51, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-04-05 16:45:48)
Add calls to drm_bridge_add()/drm_bridge_remove() for the internal HDMI bridges. This fixes the following warning.
[ 2.195003] ------------[ cut here ]------------
Usually this line is left out
[ 2.195044] WARNING: CPU: 0 PID: 1 at kernel/locking/mutex.c:579 __mutex_lock+0x840/0x9f4
And the timestamp is removed
Ack for both notes, will fix in v2
[ 2.198774] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [ 2.198786] Modules linked in: [ 2.211868] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.18.0-rc1-00002-g3054695a0d27-dirty #55 [ 2.214671] Hardware name: Generic DT based system [ 2.223265] unwind_backtrace from show_stack+0x10/0x14 [ 2.228036] show_stack from dump_stack_lvl+0x58/0x70 [ 2.233159] dump_stack_lvl from __warn+0xc8/0x1e8 [ 2.238367] __warn from warn_slowpath_fmt+0x78/0xa8 [ 2.243054] warn_slowpath_fmt from __mutex_lock+0x840/0x9f4 [ 2.248174] __mutex_lock from mutex_lock_nested+0x1c/0x24 [ 2.253818] mutex_lock_nested from drm_bridge_hpd_enable+0x2c/0x84 [ 2.259116] drm_bridge_hpd_enable from msm_hdmi_modeset_init+0xc0/0x21c [ 2.265279] msm_hdmi_modeset_init from mdp4_kms_init+0x53c/0x90c [ 2.272223] mdp4_kms_init from msm_drm_bind+0x514/0x698 [ 2.278212] msm_drm_bind from try_to_bring_up_aggregate_device+0x160/0x1bc
I'd probably cut it off here.
ack
Is there any Fixes tag for this? Still seems worthwhile to have one even if this is a lockdep warning.
I thought about this before sending v1, but ended up not doing so. Each of these changes is not atomic. A call to drm_bridge_add() without final drm_bridge_remove() in the core msm code would leave dangling pointers in the drm core. A drm_bridge_remove() is not sensible without converting _all_ users.
So there are two alternatives: - leave this patch series as is w/o a Fixes tag - squash four patches into a single patch and add 'Fixes: a3376e3ec81c ("drm/msm: convert to drm_bridge")' tag
What would you prefer?
Quoting Dmitry Baryshkov (2022-04-06 03:21:25)
On 06/04/2022 05:51, Stephen Boyd wrote:
Is there any Fixes tag for this? Still seems worthwhile to have one even if this is a lockdep warning.
I thought about this before sending v1, but ended up not doing so. Each of these changes is not atomic. A call to drm_bridge_add() without final drm_bridge_remove() in the core msm code would leave dangling pointers in the drm core. A drm_bridge_remove() is not sensible without converting _all_ users.
So there are two alternatives:
- leave this patch series as is w/o a Fixes tag
- squash four patches into a single patch and add 'Fixes: a3376e3ec81c
("drm/msm: convert to drm_bridge")' tag
The atomic change. That's because bisection could trip over these patches and then break because it picked the middle of the patch series. One atomic commit fixes that.
Add calls to drm_bridge_add()/drm_bridge_remove() for the internal DSI bridges.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 0c1b7dde377c..918498e71255 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -665,6 +665,8 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id) bridge = &dsi_bridge->base; bridge->funcs = &dsi_mgr_bridge_funcs;
+ drm_bridge_add(bridge); + ret = drm_bridge_attach(encoder, bridge, NULL, 0); if (ret) goto fail; @@ -735,6 +737,7 @@ struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id)
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge) { + drm_bridge_remove(bridge); }
int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg)
Add calls to drm_bridge_add()/drm_bridge_remove() for the internal DP bridges.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dp/dp_drm.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 80f59cf99089..a7415f9eb80e 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -230,9 +230,13 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi bridge->funcs = &dp_bridge_ops; bridge->encoder = encoder;
+ drm_bridge_add(bridge); + rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc) { DRM_ERROR("failed to attach bridge, rc=%d\n", rc); + drm_bridge_remove(bridge); + return ERR_PTR(rc); }
@@ -242,6 +246,7 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc < 0) { DRM_ERROR("failed to attach panel bridge: %d\n", rc); + drm_bridge_detach(bridge); drm_bridge_remove(bridge); return ERR_PTR(rc); }
Unwind all drm_bridge_add() calls for internal bridges and remove all bridges added by the msm driver itself by calling drm_bridge_remove().
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/msm_drv.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index affa95eb05fc..71e1b7393f6f 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -232,6 +232,9 @@ static int msm_drm_uninit(struct device *dev)
drm_mode_config_cleanup(ddev);
+ for (i = 0; i < priv->num_bridges; i++) + drm_bridge_remove(priv->bridges[i]); + pm_runtime_get_sync(dev); msm_irq_uninstall(ddev); pm_runtime_put_sync(dev);
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
Stephen Boyd