On Fri, 27 Dec 2019 12:01:35 +0100 Marek Szyprowski m.szyprowski@samsung.com wrote:
Exynos DSI DRM driver uses private calls to out bridge to force certain order of operations during init/exit sequences. This no longer works after conversion of bridge chain to a double-linked list. To fix the regression call bridge related operations manually instead of the generic drm_bridge_chain_*() operations.
I think it'd be worth explaining what the problem is (infinite loop caused by list_for_each_entry() use when the bridge is no longer part of the chain attached to the encoder).
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
We also need to fix that in VC4.
This patch is a result of the following discussion: https://www.spinics.net/lists/dri-devel/msg239256.html
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 3955f84dc893..f5905c239a86 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -255,7 +255,6 @@ struct exynos_dsi { struct mipi_dsi_host dsi_host; struct drm_connector connector; struct drm_panel *panel;
- struct list_head bridge_chain; struct drm_bridge *out_bridge; struct device *dev;
@@ -1391,7 +1390,8 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) if (ret < 0) goto err_put_sync; } else {
drm_bridge_chain_pre_enable(dsi->out_bridge);
if (dsi->out_bridge->funcs->pre_enable)dsi->out_bridge->funcs->pre_enable(dsi->out_bridge);
Okay, so you're calling ->{pre_enable,enable,disable,post_disable}() on the first bridge element which only works if the chain contains one bridge (see below). Maybe you should keep exynos_dsi.bridge_chain and create custom helpers to iterate over chain elements instead of calling those hooks only on out_bridge.
}
exynos_dsi_set_display_mode(dsi); @@ -1402,7 +1402,8 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) if (ret < 0) goto err_display_disable; } else {
drm_bridge_chain_enable(dsi->out_bridge);
if (dsi->out_bridge->funcs->enable)dsi->out_bridge->funcs->enable(dsi->out_bridge);}
dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
@@ -1427,10 +1428,12 @@ static void exynos_dsi_disable(struct drm_encoder *encoder) dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
drm_panel_disable(dsi->panel);
- drm_bridge_chain_disable(dsi->out_bridge);
- if (dsi->out_bridge->funcs->disable)
dsi->out_bridge->funcs->disable(dsi->out_bridge);
- drm_bridge_chain_post_disable(dsi->out_bridge);
- if (dsi->out_bridge->funcs->post_disable)
dsi->out_bridge->funcs->post_disable(dsi->out_bridge);} @@ -1521,9 +1524,11 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) {
drm_bridge_attach(encoder, out_bridge, NULL);
out_bridge->dev = drm;out_bridge->encoder = encoder;if (out_bridge->funcs->attach)out_bridge->funcs->attach(out_bridge);
If you don't want to keep the dsi_exynos.bridge_chain field, you should probably still use drm_bridge_attach() here and manually reset encoder->bridge_chain after checking it only contains a single element. Something like:
drm_bridge_attach(encoder, out_bridge, NULL);
/* * FIXME: we need to control the enable/disable * sequence to make sure the DSI host controller is * ready to send DSI commands before the * bridge->pre_enable()/panel->prepare() hooks are * called (those hooks are allowed to send DSI * commands). The only way to do that right now is to * reset the bridge chain so that the core thinks it's * empty and turns drm_bridge_chain_xxx() calls into * NOPs. This only works if the chain contains only one * bridge, otherwise we'd be skipping bridge->xxx() * calls on other bridges which is wrong. */ WARN_ON(!list_is_singular(&encoder->bridge_chain)); INIT_LIST_HEAD(&encoder->bridge_chain);
Calling ->attach() directly might lead to an invalid pointer dereference if out_bridge tries to attach to another bridge (out_bridge->bridge_node is uninitialized if you don't call drm_bridge_attach()).
dsi->out_bridge = out_bridge;
list_splice(&encoder->bridge_chain, &dsi->bridge_chain);@@ -1589,7 +1594,6 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, if (dsi->out_bridge->funcs->detach) dsi->out_bridge->funcs->detach(dsi->out_bridge); dsi->out_bridge = NULL;
INIT_LIST_HEAD(&dsi->bridge_chain);}
if (drm->mode_config.poll_enabled)
@@ -1737,7 +1741,6 @@ static int exynos_dsi_probe(struct platform_device *pdev) init_completion(&dsi->completed); spin_lock_init(&dsi->transfer_lock); INIT_LIST_HEAD(&dsi->transfer_list);
INIT_LIST_HEAD(&dsi->bridge_chain);
dsi->dsi_host.ops = &exynos_dsi_ops; dsi->dsi_host.dev = dev;