| Message ID | 20191227144124.210294-3-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/bridge: Fix drm_bridge_chain_pre_enable() | expand |
On Fri, 27 Dec 2019 15:41:24 +0100 Boris Brezillon <boris.brezillon@collabora.com> wrote: > Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked > list") patched the bridge chain logic to use a double-linked list instead > of a single-linked list. This change induced changes to the Exynos driver > which was manually resetting the encoder->bridge element to NULL to > control the enable/disable sequence of the bridge chain. During this > conversion, 2 bugs were introduced: > > 1/ list_splice() was used to move chain elements to our own internal > chain, but list_splice() does not reset the source list to an empty > state, leading to unexpected bridge hook calls when > drm_bridge_chain_xxx() helpers were called by the core. Replacing > the list_splice() call by list_splice_init() fixes this problem. > > 2/ drm_bridge_chain_xxx() helpers operate on the > bridge->encoder->bridge_chain list, which is now empty. When the > helper uses list_for_each_entry_reverse() we end up with no operation > done which is not what we want. But that's even worse when the helper > uses list_for_each_entry_from(), because in that case we end up in > an infinite loop searching for the list head element which is no > longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address > that problem we stop using the bridge chain helpers and call the > hooks directly. > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > Hello Marek, > > I'm perfectly fine applying your patch instead of this one if you prefer > to restrict the logic to a single bridge per chain. I just sent this > patch in case your okay with the slightly different version I propose > here. Marek, Andrzej, did you have time to look at this patch (or respin "drm/bridge: Fix Exynos DSI after making bridge chain a double-linked list" if you don't like this version)? I'd really like to apply this fix (and its vc4 equivalent) to drm-misc-next as soon as possible. Thanks, Boris
Hi Boris, Sorry, for the late reply, I've just got back from my prolonged Chrismas holidays. On 27.12.2019 15:41, Boris Brezillon wrote: > Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked > list") patched the bridge chain logic to use a double-linked list instead > of a single-linked list. This change induced changes to the Exynos driver > which was manually resetting the encoder->bridge element to NULL to > control the enable/disable sequence of the bridge chain. During this > conversion, 2 bugs were introduced: > > 1/ list_splice() was used to move chain elements to our own internal > chain, but list_splice() does not reset the source list to an empty > state, leading to unexpected bridge hook calls when > drm_bridge_chain_xxx() helpers were called by the core. Replacing > the list_splice() call by list_splice_init() fixes this problem. > > 2/ drm_bridge_chain_xxx() helpers operate on the > bridge->encoder->bridge_chain list, which is now empty. When the > helper uses list_for_each_entry_reverse() we end up with no operation > done which is not what we want. But that's even worse when the helper > uses list_for_each_entry_from(), because in that case we end up in > an infinite loop searching for the list head element which is no > longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address > that problem we stop using the bridge chain helpers and call the > hooks directly. > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Works fine on Exynos5250-based Arndale board. Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > Hello Marek, > > I'm perfectly fine applying your patch instead of this one if you prefer > to restrict the logic to a single bridge per chain. I just sent this > patch in case your okay with the slightly different version I propose > here. > > Let me know what you want to do. I'm fine with your approach. > Regards, > > Boris > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 29 ++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index 3955f84dc893..33628d85edad 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1378,6 +1378,7 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi) > static void exynos_dsi_enable(struct drm_encoder *encoder) > { > struct exynos_dsi *dsi = encoder_to_dsi(encoder); > + struct drm_bridge *iter; > int ret; > > if (dsi->state & DSIM_STATE_ENABLED) > @@ -1391,7 +1392,11 @@ 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); > + list_for_each_entry_reverse(iter, &dsi->bridge_chain, > + chain_node) { > + if (iter->funcs->pre_enable) > + iter->funcs->pre_enable(iter); > + } > } > > exynos_dsi_set_display_mode(dsi); > @@ -1402,7 +1407,10 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) > if (ret < 0) > goto err_display_disable; > } else { > - drm_bridge_chain_enable(dsi->out_bridge); > + list_for_each_entry(iter, &dsi->bridge_chain, chain_node) { > + if (iter->funcs->enable) > + iter->funcs->enable(iter); > + } > } > > dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE; > @@ -1420,6 +1428,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) > static void exynos_dsi_disable(struct drm_encoder *encoder) > { > struct exynos_dsi *dsi = encoder_to_dsi(encoder); > + struct drm_bridge *iter; > > if (!(dsi->state & DSIM_STATE_ENABLED)) > return; > @@ -1427,10 +1436,20 @@ 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); > + > + list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) { > + if (iter->funcs->disable) > + iter->funcs->disable(iter); > + } > + > exynos_dsi_set_display_enable(dsi, false); > drm_panel_unprepare(dsi->panel); > - drm_bridge_chain_post_disable(dsi->out_bridge); > + > + list_for_each_entry(iter, &dsi->bridge_chain, chain_node) { > + if (iter->funcs->post_disable) > + iter->funcs->post_disable(iter); > + } > + > dsi->state &= ~DSIM_STATE_ENABLED; > pm_runtime_put_sync(dsi->dev); > } > @@ -1523,7 +1542,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, > if (out_bridge) { > drm_bridge_attach(encoder, out_bridge, NULL); > dsi->out_bridge = out_bridge; > - list_splice(&encoder->bridge_chain, &dsi->bridge_chain); > + list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain); > } else { > int ret = exynos_dsi_create_connector(encoder); > Best regards
Hi Marek, On Tue, 7 Jan 2020 10:11:43 +0100 Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hi Boris, > > Sorry, for the late reply, I've just got back from my prolonged Chrismas > holidays. > > On 27.12.2019 15:41, Boris Brezillon wrote: > > Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked > > list") patched the bridge chain logic to use a double-linked list instead > > of a single-linked list. This change induced changes to the Exynos driver > > which was manually resetting the encoder->bridge element to NULL to > > control the enable/disable sequence of the bridge chain. During this > > conversion, 2 bugs were introduced: > > > > 1/ list_splice() was used to move chain elements to our own internal > > chain, but list_splice() does not reset the source list to an empty > > state, leading to unexpected bridge hook calls when > > drm_bridge_chain_xxx() helpers were called by the core. Replacing > > the list_splice() call by list_splice_init() fixes this problem. > > > > 2/ drm_bridge_chain_xxx() helpers operate on the > > bridge->encoder->bridge_chain list, which is now empty. When the > > helper uses list_for_each_entry_reverse() we end up with no operation > > done which is not what we want. But that's even worse when the helper > > uses list_for_each_entry_from(), because in that case we end up in > > an infinite loop searching for the list head element which is no > > longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address > > that problem we stop using the bridge chain helpers and call the > > hooks directly. > > > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > Works fine on Exynos5250-based Arndale board. > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Thanks for testing, but can you also add your R-b (I need it to make dim happy)? While you're at it, maybe you can review patch 2 (it's very similar to this patch).
On 27.12.2019 15:41, Boris Brezillon wrote: > Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked > list") patched the bridge chain logic to use a double-linked list instead > of a single-linked list. This change induced changes to the Exynos driver > which was manually resetting the encoder->bridge element to NULL to > control the enable/disable sequence of the bridge chain. During this > conversion, 2 bugs were introduced: > > 1/ list_splice() was used to move chain elements to our own internal > chain, but list_splice() does not reset the source list to an empty > state, leading to unexpected bridge hook calls when > drm_bridge_chain_xxx() helpers were called by the core. Replacing > the list_splice() call by list_splice_init() fixes this problem. > > 2/ drm_bridge_chain_xxx() helpers operate on the > bridge->encoder->bridge_chain list, which is now empty. When the > helper uses list_for_each_entry_reverse() we end up with no operation > done which is not what we want. But that's even worse when the helper > uses list_for_each_entry_from(), because in that case we end up in > an infinite loop searching for the list head element which is no > longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address > that problem we stop using the bridge chain helpers and call the > hooks directly. > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 3955f84dc893..33628d85edad 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1378,6 +1378,7 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi) static void exynos_dsi_enable(struct drm_encoder *encoder) { struct exynos_dsi *dsi = encoder_to_dsi(encoder); + struct drm_bridge *iter; int ret; if (dsi->state & DSIM_STATE_ENABLED) @@ -1391,7 +1392,11 @@ 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); + list_for_each_entry_reverse(iter, &dsi->bridge_chain, + chain_node) { + if (iter->funcs->pre_enable) + iter->funcs->pre_enable(iter); + } } exynos_dsi_set_display_mode(dsi); @@ -1402,7 +1407,10 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) if (ret < 0) goto err_display_disable; } else { - drm_bridge_chain_enable(dsi->out_bridge); + list_for_each_entry(iter, &dsi->bridge_chain, chain_node) { + if (iter->funcs->enable) + iter->funcs->enable(iter); + } } dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE; @@ -1420,6 +1428,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) static void exynos_dsi_disable(struct drm_encoder *encoder) { struct exynos_dsi *dsi = encoder_to_dsi(encoder); + struct drm_bridge *iter; if (!(dsi->state & DSIM_STATE_ENABLED)) return; @@ -1427,10 +1436,20 @@ 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); + + list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) { + if (iter->funcs->disable) + iter->funcs->disable(iter); + } + exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel); - drm_bridge_chain_post_disable(dsi->out_bridge); + + list_for_each_entry(iter, &dsi->bridge_chain, chain_node) { + if (iter->funcs->post_disable) + iter->funcs->post_disable(iter); + } + dsi->state &= ~DSIM_STATE_ENABLED; pm_runtime_put_sync(dsi->dev); } @@ -1523,7 +1542,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, if (out_bridge) { drm_bridge_attach(encoder, out_bridge, NULL); dsi->out_bridge = out_bridge; - list_splice(&encoder->bridge_chain, &dsi->bridge_chain); + list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain); } else { int ret = exynos_dsi_create_connector(encoder);
Commit 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") patched the bridge chain logic to use a double-linked list instead of a single-linked list. This change induced changes to the Exynos driver which was manually resetting the encoder->bridge element to NULL to control the enable/disable sequence of the bridge chain. During this conversion, 2 bugs were introduced: 1/ list_splice() was used to move chain elements to our own internal chain, but list_splice() does not reset the source list to an empty state, leading to unexpected bridge hook calls when drm_bridge_chain_xxx() helpers were called by the core. Replacing the list_splice() call by list_splice_init() fixes this problem. 2/ drm_bridge_chain_xxx() helpers operate on the bridge->encoder->bridge_chain list, which is now empty. When the helper uses list_for_each_entry_reverse() we end up with no operation done which is not what we want. But that's even worse when the helper uses list_for_each_entry_from(), because in that case we end up in an infinite loop searching for the list head element which is no longer encoder->bridge_chain but exynos_dsi->bridge_chain. To address that problem we stop using the bridge chain helpers and call the hooks directly. Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list") Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- Hello Marek, I'm perfectly fine applying your patch instead of this one if you prefer to restrict the logic to a single bridge per chain. I just sent this patch in case your okay with the slightly different version I propose here. Let me know what you want to do. Regards, Boris --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 29 ++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)