drm/bridge: Fix Exynos DSI after making bridge chain a double-linked list

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 <m.szyprowski@samsung.com>

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(-)

-- 
2.17.1

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);

>  	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 <boris.brezillon@collabora.com> wrote:

> 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.


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 <boris.brezillon@collabora.com> wrote:

> On Fri, 27 Dec 2019 13:00:04 +0100

> Boris Brezillon <boris.brezillon@collabora.com> wrote:

> 

> > 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.  

> 

> 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);

>