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.
Signed-off-by: Marek Szyprowski <[email protected]>
Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
---
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);
}
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);
exynos_dsi_set_display_enable(dsi, false);
drm_panel_unprepare(dsi->panel);
- drm_bridge_chain_post_disable(dsi->out_bridge);
+ if (dsi->out_bridge->funcs->post_disable)
+ dsi->out_bridge->funcs->post_disable(dsi->out_bridge);
dsi->state &= ~DSIM_STATE_ENABLED;
pm_runtime_put_sync(dsi->dev);
}
@@ -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);
dsi->out_bridge = out_bridge;
- list_splice(&encoder->bridge_chain, &dsi->bridge_chain);
} else {
int ret = exynos_dsi_create_connector(encoder);
@@ -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;
--
2.17.1
On Fri, 27 Dec 2019 12:01:35 +0100
Marek Szyprowski <[email protected]> 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 <[email protected]>
> 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);
> exynos_dsi_set_display_enable(dsi, false);
> drm_panel_unprepare(dsi->panel);
> - drm_bridge_chain_post_disable(dsi->out_bridge);
> + if (dsi->out_bridge->funcs->post_disable)
> + dsi->out_bridge->funcs->post_disable(dsi->out_bridge);
> dsi->state &= ~DSIM_STATE_ENABLED;
> pm_runtime_put_sync(dsi->dev);
> }
> @@ -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);
> } else {
> int ret = exynos_dsi_create_connector(encoder);
>
> @@ -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;
On Fri, 27 Dec 2019 13:00:04 +0100
Boris Brezillon <[email protected]> wrote:
> On Fri, 27 Dec 2019 12:01:35 +0100
> Marek Szyprowski <[email protected]> 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 <[email protected]>
> > 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.
The following diff should fix the problem while keeping the solution
generic enough to support chains containing more than one bridge.
--->8---
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 3955f84dc893..c861b640fc59 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, &encoder->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, &encoder->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, &encoder->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, &encoder->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);
On Fri, 27 Dec 2019 14:23:13 +0100
Boris Brezillon <[email protected]> wrote:
> On Fri, 27 Dec 2019 13:00:04 +0100
> Boris Brezillon <[email protected]> wrote:
>
> > On Fri, 27 Dec 2019 12:01:35 +0100
> > Marek Szyprowski <[email protected]> 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 <[email protected]>
> > > 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.
>
> The following diff should fix the problem while keeping the solution
> generic enough to support chains containing more than one bridge.
>
> --->8---
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 3955f84dc893..c861b640fc59 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, &encoder->bridge_chain,
^&dsi->bridge_chain
And the same applies to all list_for_each() calls in this patch.
> + 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, &encoder->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, &encoder->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, &encoder->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);
>