[PATCH 3/3] drm/exynos: dsi: Fix bridge chain handling

[Marek Szyprowski]

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 at samsung.com>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>

Works fine on Exynos5250-based Arndale board.

Tested-by: Marek Szyprowski <m.szyprowski at 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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland