Re: [PATCH] drm/bridge: Fix Exynos DSI after making bridge chain a double-linked list

[Boris Brezillon]

On Fri, 27 Dec 2019 14:23:13 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:

> On Fri, 27 Dec 2019 13:00:04 +0100
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:
> 
> > On Fri, 27 Dec 2019 12:01:35 +0100
> > Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > 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);
>