[v4,04/11] drm/bridge: Make the bridge chain a double-linked list

Commit Message

Boris Brezillon Dec. 3, 2019, 2:15 p.m. UTC

So that each element in the chain can easily access its predecessor.
This will be needed to support bus format negotiation between elements
of the bridge chain.

Signed-off-by: Boris Brezillon <[e-mail]>
Reviewed-by: Neil Armstrong <[e-mail]>
Reviewed-by: Laurent Pinchart <[e-mail]>
---
Changes in v4:
* Simplify the drm_bridge_attach() logic
* Fix list iteration bugs
* Patch VC4 and Exynos DSI drivers to match core changes
* Add R-bs

Changes in v3:
* None

Changes in v2:
* Adjust things to the "dummy encoder bridge" change (patch 2 in this
  series)
---
 drivers/gpu/drm/drm_bridge.c            | 171 +++++++++++++++---------
 drivers/gpu/drm/drm_encoder.c           |  16 +--
 drivers/gpu/drm/exynos/exynos_drm_dsi.c |   5 +-
 drivers/gpu/drm/vc4/vc4_dsi.c           |  10 +-
 include/drm/drm_bridge.h                |  12 +-
 include/drm/drm_encoder.h               |   7 +-
 6 files changed, 143 insertions(+), 78 deletions(-)

Comments

Marek Szyprowski Dec. 16, 2019, 1:54 p.m. UTC | #1

Hi All,

On 03.12.2019 15:15, Boris Brezillon wrote:
> So that each element in the chain can easily access its predecessor.
> This will be needed to support bus format negotiation between elements
> of the bridge chain.
>
> Signed-off-by: Boris Brezillon <[e-mail]>
> Reviewed-by: Neil Armstrong <[e-mail]>
> Reviewed-by: Laurent Pinchart <[e-mail]>

I've noticed that this patch got merged to linux-next as commit 
05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of 
Samsung Exynos5250-based Arndale board. Booting stops after following 
messages:

[drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
[drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[drm] No driver support for vblank timestamp query.
[drm] Cannot find any crtc or sizes
[drm] Cannot find any crtc or sizes
[drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0

I will try to debug this and provide more information soon.

> ---
> Changes in v4:
> * Simplify the drm_bridge_attach() logic
> * Fix list iteration bugs
> * Patch VC4 and Exynos DSI drivers to match core changes
> * Add R-bs
>
> Changes in v3:
> * None
>
> Changes in v2:
> * Adjust things to the "dummy encoder bridge" change (patch 2 in this
>    series)
> ---
>   drivers/gpu/drm/drm_bridge.c            | 171 +++++++++++++++---------
>   drivers/gpu/drm/drm_encoder.c           |  16 +--
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c |   5 +-
>   drivers/gpu/drm/vc4/vc4_dsi.c           |  10 +-
>   include/drm/drm_bridge.h                |  12 +-
>   include/drm/drm_encoder.h               |   7 +-
>   6 files changed, 143 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 54c874493c57..b6517b4fa3d1 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -55,7 +55,7 @@
>    * just provide additional hooks to get the desired output at the end of the
>    * encoder chain.
>    *
> - * Bridges can also be chained up using the &drm_bridge.next pointer.
> + * Bridges can also be chained up using the &drm_bridge.chain_node field.
>    *
>    * Both legacy CRTC helpers and the new atomic modeset helpers support bridges.
>    */
> @@ -128,20 +128,21 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>   	bridge->dev = encoder->dev;
>   	bridge->encoder = encoder;
>   
> +	if (previous)
> +		list_add(&bridge->chain_node, &previous->chain_node);
> +	else
> +		list_add(&bridge->chain_node, &encoder->bridge_chain);
> +
>   	if (bridge->funcs->attach) {
>   		ret = bridge->funcs->attach(bridge);
>   		if (ret < 0) {
> +			list_del(&bridge->chain_node);
>   			bridge->dev = NULL;
>   			bridge->encoder = NULL;
>   			return ret;
>   		}
>   	}
>   
> -	if (previous)
> -		previous->next = bridge;
> -	else
> -		encoder->bridge = bridge;
> -
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_bridge_attach);
> @@ -157,6 +158,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>   	if (bridge->funcs->detach)
>   		bridge->funcs->detach(bridge);
>   
> +	list_del(&bridge->chain_node);
>   	bridge->dev = NULL;
>   }
>   
> @@ -190,18 +192,21 @@ bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge,
>   				 const struct drm_display_mode *mode,
>   				 struct drm_display_mode *adjusted_mode)
>   {
> -	bool ret = true;
> +	struct drm_encoder *encoder;
>   
>   	if (!bridge)
>   		return true;
>   
> -	if (bridge->funcs->mode_fixup)
> -		ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +		if (!bridge->funcs->mode_fixup)
> +			continue;
>   
> -	ret = ret && drm_bridge_chain_mode_fixup(bridge->next, mode,
> -						 adjusted_mode);
> +		if (!bridge->funcs->mode_fixup(bridge, mode, adjusted_mode))
> +			return false;
> +	}
>   
> -	return ret;
> +	return true;
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_mode_fixup);
>   
> @@ -224,18 +229,24 @@ enum drm_mode_status
>   drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
>   			    const struct drm_display_mode *mode)
>   {
> -	enum drm_mode_status ret = MODE_OK;
> +	struct drm_encoder *encoder;
>   
>   	if (!bridge)
> -		return ret;
> +		return MODE_OK;
>   
> -	if (bridge->funcs->mode_valid)
> -		ret = bridge->funcs->mode_valid(bridge, mode);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +		enum drm_mode_status ret;
> +
> +		if (!bridge->funcs->mode_valid)
> +			continue;
>   
> -	if (ret != MODE_OK)
> -		return ret;
> +		ret = bridge->funcs->mode_valid(bridge, mode);
> +		if (ret != MODE_OK)
> +			return ret;
> +	}
>   
> -	return drm_bridge_chain_mode_valid(bridge->next, mode);
> +	return MODE_OK;
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
>   
> @@ -251,13 +262,20 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
>    */
>   void drm_bridge_chain_disable(struct drm_bridge *bridge)
>   {
> +	struct drm_encoder *encoder;
> +	struct drm_bridge *iter;
> +
>   	if (!bridge)
>   		return;
>   
> -	drm_bridge_chain_disable(bridge->next);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> +		if (iter->funcs->disable)
> +			iter->funcs->disable(iter);
>   
> -	if (bridge->funcs->disable)
> -		bridge->funcs->disable(bridge);
> +		if (iter == bridge)
> +			break;
> +	}
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_disable);
>   
> @@ -274,13 +292,16 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
>    */
>   void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
>   {
> +	struct drm_encoder *encoder;
> +
>   	if (!bridge)
>   		return;
>   
> -	if (bridge->funcs->post_disable)
> -		bridge->funcs->post_disable(bridge);
> -
> -	drm_bridge_chain_post_disable(bridge->next);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +		if (bridge->funcs->post_disable)
> +			bridge->funcs->post_disable(bridge);
> +	}
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_post_disable);
>   
> @@ -300,13 +321,16 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
>   			       const struct drm_display_mode *mode,
>   			       const struct drm_display_mode *adjusted_mode)
>   {
> +	struct drm_encoder *encoder;
> +
>   	if (!bridge)
>   		return;
>   
> -	if (bridge->funcs->mode_set)
> -		bridge->funcs->mode_set(bridge, mode, adjusted_mode);
> -
> -	drm_bridge_chain_mode_set(bridge->next, mode, adjusted_mode);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +		if (bridge->funcs->mode_set)
> +			bridge->funcs->mode_set(bridge, mode, adjusted_mode);
> +	}
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>   
> @@ -323,13 +347,17 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>    */
>   void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>   {
> +	struct drm_encoder *encoder;
> +	struct drm_bridge *iter;
> +
>   	if (!bridge)
>   		return;
>   
> -	drm_bridge_chain_pre_enable(bridge->next);
> -
> -	if (bridge->funcs->pre_enable)
> -		bridge->funcs->pre_enable(bridge);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> +		if (iter->funcs->pre_enable)
> +			iter->funcs->pre_enable(iter);
> +	}
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
>   
> @@ -345,13 +373,16 @@ EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
>    */
>   void drm_bridge_chain_enable(struct drm_bridge *bridge)
>   {
> +	struct drm_encoder *encoder;
> +
>   	if (!bridge)
>   		return;
>   
> -	if (bridge->funcs->enable)
> -		bridge->funcs->enable(bridge);
> -
> -	drm_bridge_chain_enable(bridge->next);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +		if (bridge->funcs->enable)
> +			bridge->funcs->enable(bridge);
> +	}
>   }
>   EXPORT_SYMBOL(drm_bridge_chain_enable);
>   
> @@ -370,15 +401,22 @@ EXPORT_SYMBOL(drm_bridge_chain_enable);
>   void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
>   				     struct drm_atomic_state *state)
>   {
> +	struct drm_encoder *encoder;
> +	struct drm_bridge *iter;
> +
>   	if (!bridge)
>   		return;
>   
> -	drm_atomic_bridge_chain_disable(bridge->next, state);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> +		if (iter->funcs->atomic_disable)
> +			iter->funcs->atomic_disable(iter, state);
> +		else if (iter->funcs->disable)
> +			iter->funcs->disable(iter);
>   
> -	if (bridge->funcs->atomic_disable)
> -		bridge->funcs->atomic_disable(bridge, state);
> -	else if (bridge->funcs->disable)
> -		bridge->funcs->disable(bridge);
> +		if (iter == bridge)
> +			break;
> +	}
>   }
>   EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>   
> @@ -398,15 +436,18 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>   void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
>   					  struct drm_atomic_state *state)
>   {
> +	struct drm_encoder *encoder;
> +
>   	if (!bridge)
>   		return;
>   
> -	if (bridge->funcs->atomic_post_disable)
> -		bridge->funcs->atomic_post_disable(bridge, state);
> -	else if (bridge->funcs->post_disable)
> -		bridge->funcs->post_disable(bridge);
> -
> -	drm_atomic_bridge_chain_post_disable(bridge->next, state);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +		if (bridge->funcs->atomic_post_disable)
> +			bridge->funcs->atomic_post_disable(bridge, state);
> +		else if (bridge->funcs->post_disable)
> +			bridge->funcs->post_disable(bridge);
> +	}
>   }
>   EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>   
> @@ -426,15 +467,22 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>   void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>   					struct drm_atomic_state *state)
>   {
> +	struct drm_encoder *encoder;
> +	struct drm_bridge *iter;
> +
>   	if (!bridge)
>   		return;
>   
> -	drm_atomic_bridge_chain_pre_enable(bridge->next, state);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> +		if (iter->funcs->atomic_pre_enable)
> +			iter->funcs->atomic_pre_enable(iter, state);
> +		else if (iter->funcs->pre_enable)
> +			iter->funcs->pre_enable(iter);
>   
> -	if (bridge->funcs->atomic_pre_enable)
> -		bridge->funcs->atomic_pre_enable(bridge, state);
> -	else if (bridge->funcs->pre_enable)
> -		bridge->funcs->pre_enable(bridge);
> +		if (iter == bridge)
> +			break;
> +	}
>   }
>   EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>   
> @@ -453,15 +501,18 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>   void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
>   				    struct drm_atomic_state *state)
>   {
> +	struct drm_encoder *encoder;
> +
>   	if (!bridge)
>   		return;
>   
> -	if (bridge->funcs->atomic_enable)
> -		bridge->funcs->atomic_enable(bridge, state);
> -	else if (bridge->funcs->enable)
> -		bridge->funcs->enable(bridge);
> -
> -	drm_atomic_bridge_chain_enable(bridge->next, state);
> +	encoder = bridge->encoder;
> +	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> +		if (bridge->funcs->atomic_enable)
> +			bridge->funcs->atomic_enable(bridge, state);
> +		else if (bridge->funcs->enable)
> +			bridge->funcs->enable(bridge);
> +	}
>   }
>   EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
>   
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index a2cc7e7241a9..e555281f43d4 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -140,6 +140,7 @@ int drm_encoder_init(struct drm_device *dev,
>   		goto out_put;
>   	}
>   
> +	INIT_LIST_HEAD(&encoder->bridge_chain);
>   	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
>   	encoder->index = dev->mode_config.num_encoder++;
>   
> @@ -160,23 +161,16 @@ EXPORT_SYMBOL(drm_encoder_init);
>   void drm_encoder_cleanup(struct drm_encoder *encoder)
>   {
>   	struct drm_device *dev = encoder->dev;
> +	struct drm_bridge *bridge, *next;
>   
>   	/* Note that the encoder_list is considered to be static; should we
>   	 * remove the drm_encoder at runtime we would have to decrement all
>   	 * the indices on the drm_encoder after us in the encoder_list.
>   	 */
>   
> -	if (encoder->bridge) {
> -		struct drm_bridge *bridge;
> -		struct drm_bridge *next;
> -
> -		bridge = drm_bridge_chain_get_first_bridge(encoder);
> -		while (bridge) {
> -			next = drm_bridge_get_next_bridge(bridge);
> -			drm_bridge_detach(bridge);
> -			bridge = next;
> -		}
> -	}
> +	list_for_each_entry_safe(bridge, next, &encoder->bridge_chain,
> +				 chain_node)
> +		drm_bridge_detach(bridge);
>   
>   	drm_mode_object_unregister(dev, &encoder->base);
>   	kfree(encoder->name);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index d984097704b8..7de82e22252a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -255,6 +255,7 @@ 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;
>   
> @@ -1522,7 +1523,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;
> -		encoder->bridge = NULL;
> +		list_splice(&encoder->bridge_chain, &dsi->bridge_chain);
>   	} else {
>   		int ret = exynos_dsi_create_connector(encoder);
>   
> @@ -1588,6 +1589,7 @@ 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)
> @@ -1735,6 +1737,7 @@ 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;
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index ff81b54ea281..6c5b80ad6154 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -499,6 +499,7 @@ struct vc4_dsi {
>   	struct mipi_dsi_host dsi_host;
>   	struct drm_encoder *encoder;
>   	struct drm_bridge *bridge;
> +	struct list_head bridge_chain;
>   
>   	void __iomem *regs;
>   
> @@ -1460,6 +1461,8 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>   				       GFP_KERNEL);
>   	if (!vc4_dsi_encoder)
>   		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&dsi->bridge_chain);
>   	vc4_dsi_encoder->base.type = VC4_ENCODER_TYPE_DSI1;
>   	vc4_dsi_encoder->dsi = dsi;
>   	dsi->encoder = &vc4_dsi_encoder->base.base;
> @@ -1610,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>   	 * from our driver, since we need to sequence them within the
>   	 * encoder's enable/disable paths.
>   	 */
> -	dsi->encoder->bridge = NULL;
> +	list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
>   
>   	if (dsi->port == 0)
>   		vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
> @@ -1632,6 +1635,11 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
>   	if (dsi->bridge)
>   		pm_runtime_disable(dev);
>   
> +	/*
> +	 * Restore the bridge_chain so the bridge detach procedure can happen
> +	 * normally.
> +	 */
> +	list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
>   	vc4_dsi_encoder_destroy(dsi->encoder);
>   
>   	if (dsi->port == 1)
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index bd78c256b1ed..c118726469ee 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -384,8 +384,8 @@ struct drm_bridge {
>   	struct drm_device *dev;
>   	/** @encoder: encoder to which this bridge is connected */
>   	struct drm_encoder *encoder;
> -	/** @next: the next bridge in the encoder chain */
> -	struct drm_bridge *next;
> +	/** @chain_node: used to form a bridge chain */
> +	struct list_head chain_node;
>   #ifdef CONFIG_OF
>   	/** @of_node: device node pointer to the bridge */
>   	struct device_node *of_node;
> @@ -420,7 +420,10 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>   static inline struct drm_bridge *
>   drm_bridge_get_next_bridge(struct drm_bridge *bridge)
>   {
> -	return bridge->next;
> +	if (list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain))
> +		return NULL;
> +
> +	return list_next_entry(bridge, chain_node);
>   }
>   
>   /**
> @@ -434,7 +437,8 @@ drm_bridge_get_next_bridge(struct drm_bridge *bridge)
>   static inline struct drm_bridge *
>   drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
>   {
> -	return encoder->bridge;
> +	return list_first_entry_or_null(&encoder->bridge_chain,
> +					struct drm_bridge, chain_node);
>   }
>   
>   bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge,
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index f06164f44efe..5623994b6e9e 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -172,7 +172,12 @@ struct drm_encoder {
>   	 * &drm_connector_state.crtc.
>   	 */
>   	struct drm_crtc *crtc;
> -	struct drm_bridge *bridge;
> +
> +	/**
> +	 * @bridge_chain: Bridges attached to this encoder.
> +	 */
> +	struct list_head bridge_chain;
> +
>   	const struct drm_encoder_funcs *funcs;
>   	const struct drm_encoder_helper_funcs *helper_private;
>   };

Best regards

Boris Brezillon Dec. 16, 2019, 2:55 p.m. UTC | #2

Hello Marek,

On Mon, 16 Dec 2019 14:54:25 +0100
Marek Szyprowski <[e-mail]> wrote:

> Hi All,
> 
> On 03.12.2019 15:15, Boris Brezillon wrote:
> > So that each element in the chain can easily access its predecessor.
> > This will be needed to support bus format negotiation between elements
> > of the bridge chain.
> >
> > Signed-off-by: Boris Brezillon <[e-mail]>
> > Reviewed-by: Neil Armstrong <[e-mail]>
> > Reviewed-by: Laurent Pinchart <[e-mail]>  
> 
> I've noticed that this patch got merged to linux-next as commit 
> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of 
> Samsung Exynos5250-based Arndale board. Booting stops after following 
> messages:
> 
> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [drm] No driver support for vblank timestamp query.
> [drm] Cannot find any crtc or sizes
> [drm] Cannot find any crtc or sizes
> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> 
> I will try to debug this and provide more information soon.
> 

Can you try with this diff applied?

--->8---
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 3955f84dc893..118ecedc7621 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1523,7 +1523,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);
 
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 6c5b80ad6154..e1378d48210f 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
         * from our driver, since we need to sequence them within the
         * encoder's enable/disable paths.
         */
-       list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
+       list_splice_init(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
 
        if (dsi->port == 0)
                vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
@@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
         * Restore the bridge_chain so the bridge detach procedure can happen
         * normally.
         */
-       list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
+       list_splice_init(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
        vc4_dsi_encoder_destroy(dsi->encoder);
 
        if (dsi->port == 1)

Marek Szyprowski Dec. 16, 2019, 3:02 p.m. UTC | #3

Hi Boris,

On 16.12.2019 15:55, Boris Brezillon wrote:
> On Mon, 16 Dec 2019 14:54:25 +0100
> Marek Szyprowski <[e-mail]> wrote:
>> On 03.12.2019 15:15, Boris Brezillon wrote:
>>> So that each element in the chain can easily access its predecessor.
>>> This will be needed to support bus format negotiation between elements
>>> of the bridge chain.
>>>
>>> Signed-off-by: Boris Brezillon <[e-mail]>
>>> Reviewed-by: Neil Armstrong <[e-mail]>
>>> Reviewed-by: Laurent Pinchart <[e-mail]>
>> I've noticed that this patch got merged to linux-next as commit
>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
>> Samsung Exynos5250-based Arndale board. Booting stops after following
>> messages:
>>
>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> [drm] No driver support for vblank timestamp query.
>> [drm] Cannot find any crtc or sizes
>> [drm] Cannot find any crtc or sizes
>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
>>
>> I will try to debug this and provide more information soon.
>>
> Can you try with this diff applied?

This patch doesn't change anything.

> --->8---
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 3955f84dc893..118ecedc7621 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1523,7 +1523,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);
>   
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 6c5b80ad6154..e1378d48210f 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>           * from our driver, since we need to sequence them within the
>           * encoder's enable/disable paths.
>           */
> -       list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
> +       list_splice_init(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
>   
>          if (dsi->port == 0)
>                  vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
> @@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
>           * Restore the bridge_chain so the bridge detach procedure can happen
>           * normally.
>           */
> -       list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
> +       list_splice_init(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
>          vc4_dsi_encoder_destroy(dsi->encoder);
>   
>          if (dsi->port == 1)
>
>
Best regards

Boris Brezillon Dec. 16, 2019, 3:25 p.m. UTC | #4

On Mon, 16 Dec 2019 16:02:36 +0100
Marek Szyprowski <[e-mail]> wrote:

> Hi Boris,
> 
> On 16.12.2019 15:55, Boris Brezillon wrote:
> > On Mon, 16 Dec 2019 14:54:25 +0100
> > Marek Szyprowski <[e-mail]> wrote:  
> >> On 03.12.2019 15:15, Boris Brezillon wrote:  
> >>> So that each element in the chain can easily access its predecessor.
> >>> This will be needed to support bus format negotiation between elements
> >>> of the bridge chain.
> >>>
> >>> Signed-off-by: Boris Brezillon <[e-mail]>
> >>> Reviewed-by: Neil Armstrong <[e-mail]>
> >>> Reviewed-by: Laurent Pinchart <[e-mail]>  
> >> I've noticed that this patch got merged to linux-next as commit
> >> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
> >> Samsung Exynos5250-based Arndale board. Booting stops after following
> >> messages:
> >>
> >> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> >> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> >> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> >> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> >> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> >> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> >> [drm] No driver support for vblank timestamp query.
> >> [drm] Cannot find any crtc or sizes
> >> [drm] Cannot find any crtc or sizes
> >> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> >>
> >> I will try to debug this and provide more information soon.
> >>  
> > Can you try with this diff applied?  
> 
> This patch doesn't change anything.

Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
after the list_splice_init() call?

> 
> > --->8---  
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index 3955f84dc893..118ecedc7621 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -1523,7 +1523,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);
> >   
> > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > index 6c5b80ad6154..e1378d48210f 100644
> > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > @@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
> >           * from our driver, since we need to sequence them within the
> >           * encoder's enable/disable paths.
> >           */
> > -       list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
> > +       list_splice_init(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
> >   
> >          if (dsi->port == 0)
> >                  vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
> > @@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
> >           * Restore the bridge_chain so the bridge detach procedure can happen
> >           * normally.
> >           */
> > -       list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
> > +       list_splice_init(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
> >          vc4_dsi_encoder_destroy(dsi->encoder);
> >   
> >          if (dsi->port == 1)
> >
> >  
> Best regards

Marek Szyprowski Dec. 23, 2019, 9:55 a.m. UTC | #5

Hi Boris,

On 16.12.2019 16:25, Boris Brezillon wrote:
> On Mon, 16 Dec 2019 16:02:36 +0100
> Marek Szyprowski <[e-mail]> wrote:
>> Hi Boris,
>>
>> On 16.12.2019 15:55, Boris Brezillon wrote:
>>> On Mon, 16 Dec 2019 14:54:25 +0100
>>> Marek Szyprowski <[e-mail]> wrote:
>>>> On 03.12.2019 15:15, Boris Brezillon wrote:
>>>>> So that each element in the chain can easily access its predecessor.
>>>>> This will be needed to support bus format negotiation between elements
>>>>> of the bridge chain.
>>>>>
>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>
>>>> I've noticed that this patch got merged to linux-next as commit
>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
>>>> messages:
>>>>
>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>>>> [drm] No driver support for vblank timestamp query.
>>>> [drm] Cannot find any crtc or sizes
>>>> [drm] Cannot find any crtc or sizes
>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
>>>>
>>>> I will try to debug this and provide more information soon.
>>>>   
>>> Can you try with this diff applied?
>> This patch doesn't change anything.
> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
> after the list_splice_init() call?

encoder->bridge_chain contains only one element. dsi->drive_chain is empty.

Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
fixed the boot issue. It looks that this is related with the way the 
Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
will give a bit more detailed comment and spread some light on this.

I can send a formal patch fixing this if You want.

>>> --->8---
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 3955f84dc893..118ecedc7621 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -1523,7 +1523,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);
>>>    
>>> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
>>> index 6c5b80ad6154..e1378d48210f 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
>>> @@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>>>            * from our driver, since we need to sequence them within the
>>>            * encoder's enable/disable paths.
>>>            */
>>> -       list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
>>> +       list_splice_init(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
>>>    
>>>           if (dsi->port == 0)
>>>                   vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
>>> @@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
>>>            * Restore the bridge_chain so the bridge detach procedure can happen
>>>            * normally.
>>>            */
>>> -       list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
>>> +       list_splice_init(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
>>>           vc4_dsi_encoder_destroy(dsi->encoder);
>>>    
>>>           if (dsi->port == 1)
>>>
>>>
Best regards

Andrzej Hajda Dec. 24, 2019, 9:16 a.m. UTC | #6

On 23.12.2019 10:55, Marek Szyprowski wrote:
> Hi Boris,
>
> On 16.12.2019 16:25, Boris Brezillon wrote:
>> On Mon, 16 Dec 2019 16:02:36 +0100
>> Marek Szyprowski <[e-mail]> wrote:
>>> Hi Boris,
>>>
>>> On 16.12.2019 15:55, Boris Brezillon wrote:
>>>> On Mon, 16 Dec 2019 14:54:25 +0100
>>>> Marek Szyprowski <[e-mail]> wrote:
>>>>> On 03.12.2019 15:15, Boris Brezillon wrote:
>>>>>> So that each element in the chain can easily access its predecessor.
>>>>>> This will be needed to support bus format negotiation between elements
>>>>>> of the bridge chain.
>>>>>>
>>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
>>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
>>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>
>>>>> I've noticed that this patch got merged to linux-next as commit
>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
>>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
>>>>> messages:
>>>>>
>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>>>>> [drm] No driver support for vblank timestamp query.
>>>>> [drm] Cannot find any crtc or sizes
>>>>> [drm] Cannot find any crtc or sizes
>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
>>>>>
>>>>> I will try to debug this and provide more information soon.
>>>>>   
>>>> Can you try with this diff applied?
>>> This patch doesn't change anything.
>> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
>> after the list_splice_init() call?
> encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
>
> Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
> fixed the boot issue. It looks that this is related with the way the 
> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
> will give a bit more detailed comment and spread some light on this.


Hi Marek, Boris,


I have not followed latest patches due to high work load, my bad. Marek
thanks from pointing

About ExynosDSI bridge handling:

The order of calling encoder, bridge (and consequently panel) ops
enforced by DRM core (bridge->pre_enable, encoder->enable,
bridge->enable) does not fit to ExynosDSI hardware initialization
sequence, if I remember correctly it does not fit to whole MIPI DSI
standard (I think similar situation is with eDP). As a result DSI
drivers must use some ugly workarounds, rely on HW properly coping with
incorrect sequences, or, as in case of ExynosDSI driver, just avoid
using encoder->bridge chaining and call bridge ops by itself when suitable.

So proper patch converting to double-linked list should not try to
splice ExynosDSI private bridge list with with encoder's, encoder's list
should be always empty, as Marek suggested.


Regards

Andrzej


>
> I can send a formal patch fixing this if You want.
>
>>>> --->8---
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> index 3955f84dc893..118ecedc7621 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> @@ -1523,7 +1523,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);
>>>>    
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
>>>> index 6c5b80ad6154..e1378d48210f 100644
>>>> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
>>>> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
>>>> @@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>>>>            * from our driver, since we need to sequence them within the
>>>>            * encoder's enable/disable paths.
>>>>            */
>>>> -       list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
>>>> +       list_splice_init(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
>>>>    
>>>>           if (dsi->port == 0)
>>>>                   vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
>>>> @@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
>>>>            * Restore the bridge_chain so the bridge detach procedure can happen
>>>>            * normally.
>>>>            */
>>>> -       list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
>>>> +       list_splice_init(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
>>>>           vc4_dsi_encoder_destroy(dsi->encoder);
>>>>    
>>>>           if (dsi->port == 1)
>>>>
>>>>
> Best regards

Boris Brezillon Dec. 24, 2019, 9:44 a.m. UTC | #7

On Tue, 24 Dec 2019 10:16:49 +0100
Andrzej Hajda <[e-mail]> wrote:

> On 23.12.2019 10:55, Marek Szyprowski wrote:
> > Hi Boris,
> >
> > On 16.12.2019 16:25, Boris Brezillon wrote:  
> >> On Mon, 16 Dec 2019 16:02:36 +0100
> >> Marek Szyprowski <[e-mail]> wrote:  
> >>> Hi Boris,
> >>>
> >>> On 16.12.2019 15:55, Boris Brezillon wrote:  
> >>>> On Mon, 16 Dec 2019 14:54:25 +0100
> >>>> Marek Szyprowski <[e-mail]> wrote:  
> >>>>> On 03.12.2019 15:15, Boris Brezillon wrote:  
> >>>>>> So that each element in the chain can easily access its predecessor.
> >>>>>> This will be needed to support bus format negotiation between elements
> >>>>>> of the bridge chain.
> >>>>>>
> >>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
> >>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
> >>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>  
> >>>>> I've noticed that this patch got merged to linux-next as commit
> >>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
> >>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
> >>>>> messages:
> >>>>>
> >>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> >>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> >>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> >>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> >>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> >>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> >>>>> [drm] No driver support for vblank timestamp query.
> >>>>> [drm] Cannot find any crtc or sizes
> >>>>> [drm] Cannot find any crtc or sizes
> >>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> >>>>>
> >>>>> I will try to debug this and provide more information soon.
> >>>>>     
> >>>> Can you try with this diff applied?  
> >>> This patch doesn't change anything.  
> >> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
> >> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
> >> after the list_splice_init() call?  
> > encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
> >
> > Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
> > fixed the boot issue.

If INIT_LIST_HEAD() worked, I don't understand why replacing the
list_splice() call by a list_splice_init() (which doing a list_splice()
+ INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
list_splice_init() version doesn't work?

> > It looks that this is related with the way the 
> > Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
> > will give a bit more detailed comment and spread some light on this.  
> 
> 
> Hi Marek, Boris,
> 
> 
> I have not followed latest patches due to high work load, my bad. Marek
> thanks from pointing
> 
> About ExynosDSI bridge handling:
> 
> The order of calling encoder, bridge (and consequently panel) ops
> enforced by DRM core (bridge->pre_enable, encoder->enable,
> bridge->enable) does not fit to ExynosDSI hardware initialization
> sequence, if I remember correctly it does not fit to whole MIPI DSI
> standard (I think similar situation is with eDP). As a result DSI
> drivers must use some ugly workarounds, rely on HW properly coping with
> incorrect sequences, or, as in case of ExynosDSI driver, just avoid
> using encoder->bridge chaining and call bridge ops by itself when suitable.

Yes, that's definitely hack-ish, and I proposed 2 solutions to address
that in previous versions of this patchset, unfortunately I didn't get
any feedback so I went for the less invasive option (keep the hack but
adapt it to the double-linked list changes), which still lead to
regressions :-/.

Just a reminder of my 2 proposals:

1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
   split your enable/disable logic in 2 parts and make sure things are
   ready when the panel/next bridge tries to send DSI commands
2/ move everything that's needed to send DSI commands out of the
   ->enable() path (maybe in runtime PM resume/suspend hooks) so you
   can call that in the DSI transfer path too

As pointed out by Laurent, #1 doesn't work because some panel drivers
send DSI commands in their ->prepare() hook, and ->pre_enable() methods
are called in reverse order, meaning that the DRM panel bridge driver
would try to issue DSI commands before the DSI host controllers is ready
to send them. I still thing #2 is a good option.

> 
> So proper patch converting to double-linked list should not try to
> splice ExynosDSI private bridge list with with encoder's, encoder's list
> should be always empty, as Marek suggested.

That's exactly what I wanted to do: make the encoder's list empty after
attach() and restore it to its initial state before unregistering
the bridge, except I forgot that list_splice() doesn't call
INIT_LIST_HEAD(). It's still not clear to me why replacing the
list_splice() call by a list_splice_init() didn't work.
Also note that calling INIT_LIST_HEAD() only works if you have one
bridge in the chain, so if we go for that option we need a comment
explaining the limitations of this approach.

Boris Brezillon Dec. 24, 2019, 9:49 a.m. UTC | #8

On Tue, 24 Dec 2019 10:44:22 +0100
Boris Brezillon <[e-mail]> wrote:

> On Tue, 24 Dec 2019 10:16:49 +0100
> Andrzej Hajda <[e-mail]> wrote:
> 
> > On 23.12.2019 10:55, Marek Szyprowski wrote:  
> > > Hi Boris,
> > >
> > > On 16.12.2019 16:25, Boris Brezillon wrote:    
> > >> On Mon, 16 Dec 2019 16:02:36 +0100
> > >> Marek Szyprowski <[e-mail]> wrote:    
> > >>> Hi Boris,
> > >>>
> > >>> On 16.12.2019 15:55, Boris Brezillon wrote:    
> > >>>> On Mon, 16 Dec 2019 14:54:25 +0100
> > >>>> Marek Szyprowski <[e-mail]> wrote:    
> > >>>>> On 03.12.2019 15:15, Boris Brezillon wrote:    
> > >>>>>> So that each element in the chain can easily access its predecessor.
> > >>>>>> This will be needed to support bus format negotiation between elements
> > >>>>>> of the bridge chain.
> > >>>>>>
> > >>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
> > >>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
> > >>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>    
> > >>>>> I've noticed that this patch got merged to linux-next as commit
> > >>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
> > >>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
> > >>>>> messages:
> > >>>>>
> > >>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> > >>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> > >>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> > >>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> > >>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> > >>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> > >>>>> [drm] No driver support for vblank timestamp query.
> > >>>>> [drm] Cannot find any crtc or sizes
> > >>>>> [drm] Cannot find any crtc or sizes
> > >>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> > >>>>>
> > >>>>> I will try to debug this and provide more information soon.
> > >>>>>       
> > >>>> Can you try with this diff applied?    
> > >>> This patch doesn't change anything.    
> > >> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
> > >> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
> > >> after the list_splice_init() call?    
> > > encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
> > >
> > > Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
> > > fixed the boot issue.  
> 
> If INIT_LIST_HEAD() worked, I don't understand why replacing the
> list_splice() call by a list_splice_init() (which doing a list_splice()
> + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
> list_splice_init() version doesn't work?
> 
> > > It looks that this is related with the way the 
> > > Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
> > > will give a bit more detailed comment and spread some light on this.    
> > 
> > 
> > Hi Marek, Boris,
> > 
> > 
> > I have not followed latest patches due to high work load, my bad. Marek
> > thanks from pointing
> > 
> > About ExynosDSI bridge handling:
> > 
> > The order of calling encoder, bridge (and consequently panel) ops
> > enforced by DRM core (bridge->pre_enable, encoder->enable,
> > bridge->enable) does not fit to ExynosDSI hardware initialization
> > sequence, if I remember correctly it does not fit to whole MIPI DSI
> > standard (I think similar situation is with eDP). As a result DSI
> > drivers must use some ugly workarounds, rely on HW properly coping with
> > incorrect sequences, or, as in case of ExynosDSI driver, just avoid
> > using encoder->bridge chaining and call bridge ops by itself when suitable.  
> 
> Yes, that's definitely hack-ish, and I proposed 2 solutions to address
> that in previous versions of this patchset, unfortunately I didn't get
> any feedback so I went for the less invasive option (keep the hack but
> adapt it to the double-linked list changes), which still lead to
> regressions :-/.
> 
> Just a reminder of my 2 proposals:
> 
> 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
>    split your enable/disable logic in 2 parts and make sure things are
>    ready when the panel/next bridge tries to send DSI commands
> 2/ move everything that's needed to send DSI commands out of the
>    ->enable() path (maybe in runtime PM resume/suspend hooks) so you  
>    can call that in the DSI transfer path too
> 
> As pointed out by Laurent, #1 doesn't work because some panel drivers
> send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> are called in reverse order, meaning that the DRM panel bridge driver
> would try to issue DSI commands before the DSI host controllers is ready
> to send them. I still thing #2 is a good option.
> 
> > 
> > So proper patch converting to double-linked list should not try to
> > splice ExynosDSI private bridge list with with encoder's, encoder's list
> > should be always empty, as Marek suggested.  
> 
> That's exactly what I wanted to do: make the encoder's list empty after
> attach() and restore it to its initial state before unregistering
> the bridge, except I forgot that list_splice() doesn't call
> INIT_LIST_HEAD(). It's still not clear to me why replacing the
> list_splice() call by a list_splice_init() didn't work.

Okay, I think I figured it out: drm_bridge_chain_xx() helpers use
encoder->bridge_chain as their list head, and you'll never hit the 'elem
is list head' condition since we moved all elems from
encoder->bridge_chain to exynos_dsi->bridge_chain. The only way this
can work is if we stop using the helpers and implement our own list
iterators.

Boris Brezillon Dec. 24, 2019, 10:03 a.m. UTC | #9

On Tue, 24 Dec 2019 10:49:36 +0100
Boris Brezillon <[e-mail]> wrote:

> On Tue, 24 Dec 2019 10:44:22 +0100
> Boris Brezillon <[e-mail]> wrote:
> 
> > On Tue, 24 Dec 2019 10:16:49 +0100
> > Andrzej Hajda <[e-mail]> wrote:
> >   
> > > On 23.12.2019 10:55, Marek Szyprowski wrote:    
> > > > Hi Boris,
> > > >
> > > > On 16.12.2019 16:25, Boris Brezillon wrote:      
> > > >> On Mon, 16 Dec 2019 16:02:36 +0100
> > > >> Marek Szyprowski <[e-mail]> wrote:      
> > > >>> Hi Boris,
> > > >>>
> > > >>> On 16.12.2019 15:55, Boris Brezillon wrote:      
> > > >>>> On Mon, 16 Dec 2019 14:54:25 +0100
> > > >>>> Marek Szyprowski <[e-mail]> wrote:      
> > > >>>>> On 03.12.2019 15:15, Boris Brezillon wrote:      
> > > >>>>>> So that each element in the chain can easily access its predecessor.
> > > >>>>>> This will be needed to support bus format negotiation between elements
> > > >>>>>> of the bridge chain.
> > > >>>>>>
> > > >>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
> > > >>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
> > > >>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>      
> > > >>>>> I've noticed that this patch got merged to linux-next as commit
> > > >>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
> > > >>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
> > > >>>>> messages:
> > > >>>>>
> > > >>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> > > >>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> > > >>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> > > >>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> > > >>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> > > >>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> > > >>>>> [drm] No driver support for vblank timestamp query.
> > > >>>>> [drm] Cannot find any crtc or sizes
> > > >>>>> [drm] Cannot find any crtc or sizes
> > > >>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> > > >>>>>
> > > >>>>> I will try to debug this and provide more information soon.
> > > >>>>>         
> > > >>>> Can you try with this diff applied?      
> > > >>> This patch doesn't change anything.      
> > > >> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
> > > >> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
> > > >> after the list_splice_init() call?      
> > > > encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
> > > >
> > > > Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
> > > > fixed the boot issue.    
> > 
> > If INIT_LIST_HEAD() worked, I don't understand why replacing the
> > list_splice() call by a list_splice_init() (which doing a list_splice()
> > + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
> > list_splice_init() version doesn't work?
> >   
> > > > It looks that this is related with the way the 
> > > > Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
> > > > will give a bit more detailed comment and spread some light on this.      
> > > 
> > > 
> > > Hi Marek, Boris,
> > > 
> > > 
> > > I have not followed latest patches due to high work load, my bad. Marek
> > > thanks from pointing
> > > 
> > > About ExynosDSI bridge handling:
> > > 
> > > The order of calling encoder, bridge (and consequently panel) ops
> > > enforced by DRM core (bridge->pre_enable, encoder->enable,
> > > bridge->enable) does not fit to ExynosDSI hardware initialization
> > > sequence, if I remember correctly it does not fit to whole MIPI DSI
> > > standard (I think similar situation is with eDP). As a result DSI
> > > drivers must use some ugly workarounds, rely on HW properly coping with
> > > incorrect sequences, or, as in case of ExynosDSI driver, just avoid
> > > using encoder->bridge chaining and call bridge ops by itself when suitable.    
> > 
> > Yes, that's definitely hack-ish, and I proposed 2 solutions to address
> > that in previous versions of this patchset, unfortunately I didn't get
> > any feedback so I went for the less invasive option (keep the hack but
> > adapt it to the double-linked list changes), which still lead to
> > regressions :-/.
> > 
> > Just a reminder of my 2 proposals:
> > 
> > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
> >    split your enable/disable logic in 2 parts and make sure things are
> >    ready when the panel/next bridge tries to send DSI commands
> > 2/ move everything that's needed to send DSI commands out of the  
> >    ->enable() path (maybe in runtime PM resume/suspend hooks) so you    
> >    can call that in the DSI transfer path too
> > 
> > As pointed out by Laurent, #1 doesn't work because some panel drivers
> > send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> > are called in reverse order, meaning that the DRM panel bridge driver
> > would try to issue DSI commands before the DSI host controllers is ready
> > to send them. I still thing #2 is a good option.
> >   
> > > 
> > > So proper patch converting to double-linked list should not try to
> > > splice ExynosDSI private bridge list with with encoder's, encoder's list
> > > should be always empty, as Marek suggested.    
> > 
> > That's exactly what I wanted to do: make the encoder's list empty after
> > attach() and restore it to its initial state before unregistering
> > the bridge, except I forgot that list_splice() doesn't call
> > INIT_LIST_HEAD(). It's still not clear to me why replacing the
> > list_splice() call by a list_splice_init() didn't work.  
> 
> Okay, I think I figured it out: drm_bridge_chain_xx() helpers use
> encoder->bridge_chain as their list head, and you'll never hit the 'elem
> is list head' condition since we moved all elems from
> encoder->bridge_chain to exynos_dsi->bridge_chain. The only way this
> can work is if we stop using the helpers and implement our own list
> iterators.

Just to make it clear, calling INIT_LIST_HEAD(encoder->bridge_chain)
doesn't really fix the bug, it just prevents the hang (infinite loop)
and turn all drm_bridge_chain_xx() calls into NOPs.

Sam Ravnborg Dec. 24, 2019, 11:31 a.m. UTC | #10

Hi Boris.

> Just a reminder of my 2 proposals:
> 
> 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
>    split your enable/disable logic in 2 parts and make sure things are
>    ready when the panel/next bridge tries to send DSI commands
> 2/ move everything that's needed to send DSI commands out of the
>    ->enable() path (maybe in runtime PM resume/suspend hooks) so you
>    can call that in the DSI transfer path too
> 
> As pointed out by Laurent, #1 doesn't work because some panel drivers
> send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> are called in reverse order, meaning that the DRM panel bridge driver
> would try to issue DSI commands before the DSI host controllers is ready
> to send them. I still thing #2 is a good option.

Jitao Shi suggested to extend panels so we had a sequence of:

  prepare_power()  <= new callback,
                   here one should NOT be allowed to send
                   DSI commands
  prepare()
  enable()

   #
   # panel is now ready to show your favourite christmas movie
   #

  disable()
  unprepare()
  unprepare_power()  <= new callback


Would this help implement what you suggest above?
Relevant panels would then have to be updated - but this
is doable.

	Sam

Laurent Pinchart Dec. 25, 2019, 1:36 a.m. UTC | #11

Hi Sam,

On Tue, Dec 24, 2019 at 12:31:11PM +0100, Sam Ravnborg wrote:
> > Just a reminder of my 2 proposals:
> > 
> > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
> >    split your enable/disable logic in 2 parts and make sure things are
> >    ready when the panel/next bridge tries to send DSI commands
> > 2/ move everything that's needed to send DSI commands out of the
> >    ->enable() path (maybe in runtime PM resume/suspend hooks) so you
> >    can call that in the DSI transfer path too
> > 
> > As pointed out by Laurent, #1 doesn't work because some panel drivers
> > send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> > are called in reverse order, meaning that the DRM panel bridge driver
> > would try to issue DSI commands before the DSI host controllers is ready
> > to send them. I still thing #2 is a good option.
> 
> Jitao Shi suggested to extend panels so we had a sequence of:
> 
>   prepare_power()  <= new callback,
>                    here one should NOT be allowed to send
>                    DSI commands
>   prepare()
>   enable()
> 
>    #
>    # panel is now ready to show your favourite christmas movie
>    #
> 
>   disable()
>   unprepare()
>   unprepare_power()  <= new callback

Please note that you will need corresponding bridge operations in order
to implement the DRM panel bridge.

> Would this help implement what you suggest above?
> Relevant panels would then have to be updated - but this
> is doable.

The fundamental issue is that the enable/disable sequence requirements
are inherently driven by sinks, while the bridge API goes from source to
sink. A DSI panel could for instance require the following enable
sequence.

0. DSI bus disabled (LP-00 state)
1. Power up the panel
2. Enable the DSI bus (LP-11 state)
3. Configure the panel (through DCS LP mode writes, SPI, GPIOs, ...)
4. Transition the source to HS mode, sending sync packets, but no data
5. Perform additional configuration for the panel
6. Enable transmission of video from the source

For a given bus type there are thus well-specified states for a source
(in this case disabled, enabled in LP mode, in HS mode, sending sync
packets, sending video packets).

As our API operates from source to sink, fine-grained control of bridges
is difficult. We can of course always make bridge and panel operations
more fine-grained, but that's more of a ad-hoc solution that may lead to
abuse. We would at the very least document very explicitly what each
operation would be allowed to do for every bus type.

Andrzej Hajda Dec. 27, 2019, 9:42 a.m. UTC | #12

On 24.12.2019 10:44, Boris Brezillon wrote:
> On Tue, 24 Dec 2019 10:16:49 +0100
> Andrzej Hajda <[e-mail]> wrote:
>
>> On 23.12.2019 10:55, Marek Szyprowski wrote:
>>> Hi Boris,
>>>
>>> On 16.12.2019 16:25, Boris Brezillon wrote:  
>>>> On Mon, 16 Dec 2019 16:02:36 +0100
>>>> Marek Szyprowski <[e-mail]> wrote:  
>>>>> Hi Boris,
>>>>>
>>>>> On 16.12.2019 15:55, Boris Brezillon wrote:  
>>>>>> On Mon, 16 Dec 2019 14:54:25 +0100
>>>>>> Marek Szyprowski <[e-mail]> wrote:  
>>>>>>> On 03.12.2019 15:15, Boris Brezillon wrote:  
>>>>>>>> So that each element in the chain can easily access its predecessor.
>>>>>>>> This will be needed to support bus format negotiation between elements
>>>>>>>> of the bridge chain.
>>>>>>>>
>>>>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
>>>>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
>>>>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>  
>>>>>>> I've noticed that this patch got merged to linux-next as commit
>>>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
>>>>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
>>>>>>> messages:
>>>>>>>
>>>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
>>>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
>>>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
>>>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
>>>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
>>>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>>>>>>> [drm] No driver support for vblank timestamp query.
>>>>>>> [drm] Cannot find any crtc or sizes
>>>>>>> [drm] Cannot find any crtc or sizes
>>>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
>>>>>>>
>>>>>>> I will try to debug this and provide more information soon.
>>>>>>>     
>>>>>> Can you try with this diff applied?  
>>>>> This patch doesn't change anything.  
>>>> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
>>>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
>>>> after the list_splice_init() call?  
>>> encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
>>>
>>> Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
>>> fixed the boot issue.
> If INIT_LIST_HEAD() worked, I don't understand why replacing the
> list_splice() call by a list_splice_init() (which doing a list_splice()
> + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
> list_splice_init() version doesn't work?
>
>>> It looks that this is related with the way the 
>>> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
>>> will give a bit more detailed comment and spread some light on this.  
>>
>> Hi Marek, Boris,
>>
>>
>> I have not followed latest patches due to high work load, my bad. Marek
>> thanks from pointing
>>
>> About ExynosDSI bridge handling:
>>
>> The order of calling encoder, bridge (and consequently panel) ops
>> enforced by DRM core (bridge->pre_enable, encoder->enable,
>> bridge->enable) does not fit to ExynosDSI hardware initialization
>> sequence, if I remember correctly it does not fit to whole MIPI DSI
>> standard (I think similar situation is with eDP). As a result DSI
>> drivers must use some ugly workarounds, rely on HW properly coping with
>> incorrect sequences, or, as in case of ExynosDSI driver, just avoid
>> using encoder->bridge chaining and call bridge ops by itself when suitable.
> Yes, that's definitely hack-ish, and I proposed 2 solutions to address
> that in previous versions of this patchset, unfortunately I didn't get
> any feedback so I went for the less invasive option (keep the hack but
> adapt it to the double-linked list changes), which still lead to
> regressions :-/.
>
> Just a reminder of my 2 proposals:
>
> 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
>    split your enable/disable logic in 2 parts and make sure things are
>    ready when the panel/next bridge tries to send DSI commands


If it means 'convert exynos_dsi to bridge' I do not think it will help -

- pre_enable op will be still called after pre_enable op of downstream
bridge - and this is the main reason why exynos_dsi do not use encoder
bridge chain - it needs to perform some operations BEFORE (pre)enabling
downstream devices.


> 2/ move everything that's needed to send DSI commands out of the
>    ->enable() path (maybe in runtime PM resume/suspend hooks) so you
>    can call that in the DSI transfer path too


It looks like a solution for DSI protocol, where control bus is shared
with data bus, but the problem is more general - we have source and sink
connected with some local bus, which has some negotiation/enable/disable
protocol/requirements. And drm_core/bridge framework enforces us to fit
every such protocol to 'drm_bridge protocol' with few opses called in
fixed order, without clearly defined purpose of each ops. That does not
sound generic and results in multiple issues:

- different drivers uses different opses to perform the same thing,

- different drivers assumes different things about their sinks/sources
in their opses,

- more complicated sequences does not fit at all to this model.

All this results in incompatibilities between drivers which become
visible with devices used in different configurations/platforms.


Regards

Andrzej


>
> As pointed out by Laurent, #1 doesn't work because some panel drivers
> send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> are called in reverse order, meaning that the DRM panel bridge driver
> would try to issue DSI commands before the DSI host controllers is ready
> to send them. I still thing #2 is a good option.
>
>> So proper patch converting to double-linked list should not try to
>> splice ExynosDSI private bridge list with with encoder's, encoder's list
>> should be always empty, as Marek suggested.
> That's exactly what I wanted to do: make the encoder's list empty after
> attach() and restore it to its initial state before unregistering
> the bridge, except I forgot that list_splice() doesn't call
> INIT_LIST_HEAD(). It's still not clear to me why replacing the
> list_splice() call by a list_splice_init() didn't work.
> Also note that calling INIT_LIST_HEAD() only works if you have one
> bridge in the chain, so if we go for that option we need a comment
> explaining the limitations of this approach.
>

Marek Szyprowski Dec. 27, 2019, 10:25 a.m. UTC | #13

Hi Boris,

On 24.12.2019 11:03, Boris Brezillon wrote:
> On Tue, 24 Dec 2019 10:49:36 +0100
> Boris Brezillon <[e-mail]> wrote:
>> On Tue, 24 Dec 2019 10:44:22 +0100
>> Boris Brezillon <[e-mail]> wrote:
>>> On Tue, 24 Dec 2019 10:16:49 +0100
>>> Andrzej Hajda <[e-mail]> wrote:
>>>> On 23.12.2019 10:55, Marek Szyprowski wrote:
>>>>> On 16.12.2019 16:25, Boris Brezillon wrote:
>>>>>> On Mon, 16 Dec 2019 16:02:36 +0100
>>>>>> Marek Szyprowski <[e-mail]> wrote:
>>>>>>> On 16.12.2019 15:55, Boris Brezillon wrote:
>>>>>>>> On Mon, 16 Dec 2019 14:54:25 +0100
>>>>>>>> Marek Szyprowski <[e-mail]> wrote:
>>>>>>>>> On 03.12.2019 15:15, Boris Brezillon wrote:
>>>>>>>>>> So that each element in the chain can easily access its predecessor.
>>>>>>>>>> This will be needed to support bus format negotiation between elements
>>>>>>>>>> of the bridge chain.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
>>>>>>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
>>>>>>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>
>>>>>>>>> I've noticed that this patch got merged to linux-next as commit
>>>>>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
>>>>>>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
>>>>>>>>> messages:
>>>>>>>>>
>>>>>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
>>>>>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
>>>>>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
>>>>>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
>>>>>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
>>>>>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>>>>>>>>> [drm] No driver support for vblank timestamp query.
>>>>>>>>> [drm] Cannot find any crtc or sizes
>>>>>>>>> [drm] Cannot find any crtc or sizes
>>>>>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
>>>>>>>>>
>>>>>>>>> I will try to debug this and provide more information soon.
>>>>>>>>>          
>>>>>>>> Can you try with this diff applied?
>>>>>>> This patch doesn't change anything.
>>>>>> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
>>>>>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
>>>>>> after the list_splice_init() call?
>>>>> encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
>>>>>
>>>>> Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain)
>>>>> fixed the boot issue.
>>> If INIT_LIST_HEAD() worked, I don't understand why replacing the
>>> list_splice() call by a list_splice_init() (which doing a list_splice()
>>> + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
>>> list_splice_init() version doesn't work?
>>>    
>>>>> It looks that this is related with the way the
>>>>> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej
>>>>> will give a bit more detailed comment and spread some light on this.
>>>>
>>>> Hi Marek, Boris,
>>>>
>>>>
>>>> I have not followed latest patches due to high work load, my bad. Marek
>>>> thanks from pointing
>>>>
>>>> About ExynosDSI bridge handling:
>>>>
>>>> The order of calling encoder, bridge (and consequently panel) ops
>>>> enforced by DRM core (bridge->pre_enable, encoder->enable,
>>>> bridge->enable) does not fit to ExynosDSI hardware initialization
>>>> sequence, if I remember correctly it does not fit to whole MIPI DSI
>>>> standard (I think similar situation is with eDP). As a result DSI
>>>> drivers must use some ugly workarounds, rely on HW properly coping with
>>>> incorrect sequences, or, as in case of ExynosDSI driver, just avoid
>>>> using encoder->bridge chaining and call bridge ops by itself when suitable.
>>> Yes, that's definitely hack-ish, and I proposed 2 solutions to address
>>> that in previous versions of this patchset, unfortunately I didn't get
>>> any feedback so I went for the less invasive option (keep the hack but
>>> adapt it to the double-linked list changes), which still lead to
>>> regressions :-/.
>>>
>>> Just a reminder of my 2 proposals:
>>>
>>> 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
>>>     split your enable/disable logic in 2 parts and make sure things are
>>>     ready when the panel/next bridge tries to send DSI commands
>>> 2/ move everything that's needed to send DSI commands out of the
>>>     ->enable() path (maybe in runtime PM resume/suspend hooks) so you
>>>     can call that in the DSI transfer path too
>>>
>>> As pointed out by Laurent, #1 doesn't work because some panel drivers
>>> send DSI commands in their ->prepare() hook, and ->pre_enable() methods
>>> are called in reverse order, meaning that the DRM panel bridge driver
>>> would try to issue DSI commands before the DSI host controllers is ready
>>> to send them. I still thing #2 is a good option.
>>>    
>>>> So proper patch converting to double-linked list should not try to
>>>> splice ExynosDSI private bridge list with with encoder's, encoder's list
>>>> should be always empty, as Marek suggested.
>>> That's exactly what I wanted to do: make the encoder's list empty after
>>> attach() and restore it to its initial state before unregistering
>>> the bridge, except I forgot that list_splice() doesn't call
>>> INIT_LIST_HEAD(). It's still not clear to me why replacing the
>>> list_splice() call by a list_splice_init() didn't work.
>> Okay, I think I figured it out: drm_bridge_chain_xx() helpers use
>> encoder->bridge_chain as their list head, and you'll never hit the 'elem
>> is list head' condition since we moved all elems from
>> encoder->bridge_chain to exynos_dsi->bridge_chain. The only way this
>> can work is if we stop using the helpers and implement our own list
>> iterators.
> Just to make it clear, calling INIT_LIST_HEAD(encoder->bridge_chain)
> doesn't really fix the bug, it just prevents the hang (infinite loop)
> and turn all drm_bridge_chain_xx() calls into NOPs.

Right, I've just checked it and indeed the display chain outputs nothing 
after such 'fix'. To get it finally working I've replaced 
drm_bridge_chain_*() operations for exynos_dsi 'out_bridge' by a direct 
calls. I will submit a patch in a few minutes. I hope that such 
workaround can be used now to fix the regression until a better solution 
is agreed.

Best regards

Laurent Pinchart Dec. 27, 2019, 10:51 a.m. UTC | #14

Hi Andrzej,

On Fri, Dec 27, 2019 at 10:42:25AM +0100, Andrzej Hajda wrote:
> On 24.12.2019 10:44, Boris Brezillon wrote:
> > On Tue, 24 Dec 2019 10:16:49 +0100 Andrzej Hajda wrote:
> >> On 23.12.2019 10:55, Marek Szyprowski wrote:
> >>> On 16.12.2019 16:25, Boris Brezillon wrote:  
> >>>> On Mon, 16 Dec 2019 16:02:36 +0100 Marek Szyprowski wrote:  
> >>>>> On 16.12.2019 15:55, Boris Brezillon wrote:  
> >>>>>> On Mon, 16 Dec 2019 14:54:25 +0100
> >>>>>> Marek Szyprowski <[e-mail]> wrote:  
> >>>>>>> On 03.12.2019 15:15, Boris Brezillon wrote:  
> >>>>>>>> So that each element in the chain can easily access its predecessor.
> >>>>>>>> This will be needed to support bus format negotiation between elements
> >>>>>>>> of the bridge chain.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
> >>>>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
> >>>>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>  
> >>>>>>>
> >>>>>>> I've noticed that this patch got merged to linux-next as commit
> >>>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
> >>>>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
> >>>>>>> messages:
> >>>>>>>
> >>>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> >>>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> >>>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> >>>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> >>>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> >>>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> >>>>>>> [drm] No driver support for vblank timestamp query.
> >>>>>>> [drm] Cannot find any crtc or sizes
> >>>>>>> [drm] Cannot find any crtc or sizes
> >>>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> >>>>>>>
> >>>>>>> I will try to debug this and provide more information soon.
> >>>>>>
> >>>>>> Can you try with this diff applied?  
> >>>>>
> >>>>> This patch doesn't change anything.  
> >>>>
> >>>> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
> >>>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
> >>>> after the list_splice_init() call?  
> >>>
> >>> encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
> >>>
> >>> Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
> >>> fixed the boot issue.
> >
> > If INIT_LIST_HEAD() worked, I don't understand why replacing the
> > list_splice() call by a list_splice_init() (which doing a list_splice()
> > + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
> > list_splice_init() version doesn't work?
> >
> >>> It looks that this is related with the way the 
> >>> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
> >>> will give a bit more detailed comment and spread some light on this.  
> >>
> >> Hi Marek, Boris,
> >>
> >> I have not followed latest patches due to high work load, my bad. Marek
> >> thanks from pointing
> >>
> >> About ExynosDSI bridge handling:
> >>
> >> The order of calling encoder, bridge (and consequently panel) ops
> >> enforced by DRM core (bridge->pre_enable, encoder->enable,
> >> bridge->enable) does not fit to ExynosDSI hardware initialization
> >> sequence, if I remember correctly it does not fit to whole MIPI DSI
> >> standard (I think similar situation is with eDP). As a result DSI
> >> drivers must use some ugly workarounds, rely on HW properly coping with
> >> incorrect sequences, or, as in case of ExynosDSI driver, just avoid
> >> using encoder->bridge chaining and call bridge ops by itself when suitable.
> >
> > Yes, that's definitely hack-ish, and I proposed 2 solutions to address
> > that in previous versions of this patchset, unfortunately I didn't get
> > any feedback so I went for the less invasive option (keep the hack but
> > adapt it to the double-linked list changes), which still lead to
> > regressions :-/.
> >
> > Just a reminder of my 2 proposals:
> >
> > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
> >    split your enable/disable logic in 2 parts and make sure things are
> >    ready when the panel/next bridge tries to send DSI commands
> 
> If it means 'convert exynos_dsi to bridge' I do not think it will help -
> 
> - pre_enable op will be still called after pre_enable op of downstream
> bridge - and this is the main reason why exynos_dsi do not use encoder
> bridge chain - it needs to perform some operations BEFORE (pre)enabling
> downstream devices.
> 
> > 2/ move everything that's needed to send DSI commands out of the
> >    ->enable() path (maybe in runtime PM resume/suspend hooks) so you
> >    can call that in the DSI transfer path too
> 
> It looks like a solution for DSI protocol, where control bus is shared
> with data bus, but the problem is more general - we have source and sink
> connected with some local bus, which has some negotiation/enable/disable
> protocol/requirements. And drm_core/bridge framework enforces us to fit
> every such protocol to 'drm_bridge protocol' with few opses called in
> fixed order, without clearly defined purpose of each ops. That does not
> sound generic and results in multiple issues:
> 
> - different drivers uses different opses to perform the same thing,
> 
> - different drivers assumes different things about their sinks/sources
> in their opses,
> 
> - more complicated sequences does not fit at all to this model.
> 
> All this results in incompatibilities between drivers which become
> visible with devices used in different configurations/platforms.

I fully agree with you, not defining the semantics of the bridge
operations precisely was I believe a mistake, and we're paying the price
now. That's OK, we "just" need to fix it :-)

> > As pointed out by Laurent, #1 doesn't work because some panel drivers
> > send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> > are called in reverse order, meaning that the DRM panel bridge driver
> > would try to issue DSI commands before the DSI host controllers is ready
> > to send them. I still thing #2 is a good option.
> >
> >> So proper patch converting to double-linked list should not try to
> >> splice ExynosDSI private bridge list with with encoder's, encoder's list
> >> should be always empty, as Marek suggested.
> >
> > That's exactly what I wanted to do: make the encoder's list empty after
> > attach() and restore it to its initial state before unregistering
> > the bridge, except I forgot that list_splice() doesn't call
> > INIT_LIST_HEAD(). It's still not clear to me why replacing the
> > list_splice() call by a list_splice_init() didn't work.
> > Also note that calling INIT_LIST_HEAD() only works if you have one
> > bridge in the chain, so if we go for that option we need a comment
> > explaining the limitations of this approach.

Marek Szyprowski Dec. 27, 2019, 11:03 a.m. UTC | #15

Hi All,

On 27.12.2019 11:25, Marek Szyprowski wrote:
> On 24.12.2019 11:03, Boris Brezillon wrote:
>> On Tue, 24 Dec 2019 10:49:36 +0100
>> Boris Brezillon <[e-mail]> wrote:
>>> On Tue, 24 Dec 2019 10:44:22 +0100
>>> Boris Brezillon <[e-mail]> wrote:
>>>> On Tue, 24 Dec 2019 10:16:49 +0100
>>>> Andrzej Hajda <[e-mail]> wrote:
>>>>> On 23.12.2019 10:55, Marek Szyprowski wrote:
>>>>>> On 16.12.2019 16:25, Boris Brezillon wrote:
>>>>>>> On Mon, 16 Dec 2019 16:02:36 +0100
>>>>>>> Marek Szyprowski <[e-mail]> wrote:
>>>>>>>> On 16.12.2019 15:55, Boris Brezillon wrote:
>>>>>>>>> On Mon, 16 Dec 2019 14:54:25 +0100
>>>>>>>>> Marek Szyprowski <[e-mail]> wrote:
>>>>>>>>>> On 03.12.2019 15:15, Boris Brezillon wrote:
>>>>>>>>>>> So that each element in the chain can easily access its 
>>>>>>>>>>> predecessor.
>>>>>>>>>>> This will be needed to support bus format negotiation 
>>>>>>>>>>> between elements
>>>>>>>>>>> of the bridge chain.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
>>>>>>>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
>>>>>>>>>>> Reviewed-by: Laurent Pinchart 
>>>>>>>>>>> <[e-mail]>
>>>>>>>>>> I've noticed that this patch got merged to linux-next as commit
>>>>>>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks 
>>>>>>>>>> booting of
>>>>>>>>>> Samsung Exynos5250-based Arndale board. Booting stops after 
>>>>>>>>>> following
>>>>>>>>>> messages:
>>>>>>>>>>
>>>>>>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping 
>>>>>>>>>> operations
>>>>>>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops 
>>>>>>>>>> fimd_component_ops)
>>>>>>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops 
>>>>>>>>>> mixer_component_ops)
>>>>>>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops 
>>>>>>>>>> exynos_dsi_component_ops)
>>>>>>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops 
>>>>>>>>>> hdmi_component_ops)
>>>>>>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>>>>>>>>>> [drm] No driver support for vblank timestamp query.
>>>>>>>>>> [drm] Cannot find any crtc or sizes
>>>>>>>>>> [drm] Cannot find any crtc or sizes
>>>>>>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on 
>>>>>>>>>> minor 0
>>>>>>>>>>
>>>>>>>>>> I will try to debug this and provide more information soon.
>>>>>>>>> Can you try with this diff applied?
>>>>>>>> This patch doesn't change anything.
>>>>>>> Okay. Can you do a list_for_each_entry() on both 
>>>>>>> encoder->bridge_chain
>>>>>>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) 
>>>>>>> before and
>>>>>>> after the list_splice_init() call?
>>>>>> encoder->bridge_chain contains only one element. dsi->drive_chain 
>>>>>> is empty.
>>>>>>
>>>>>> Replacing that list_splice() with 
>>>>>> INIT_LIST_HEAD(&encoder->bridge_chain)
>>>>>> fixed the boot issue.
>>>> If INIT_LIST_HEAD() worked, I don't understand why replacing the
>>>> list_splice() call by a list_splice_init() (which doing a 
>>>> list_splice()
>>>> + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
>>>> list_splice_init() version doesn't work?
>>>>>> It looks that this is related with the way the
>>>>>> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej
>>>>>> will give a bit more detailed comment and spread some light on this.
>>>>>
>>>>> Hi Marek, Boris,
>>>>>
>>>>>
>>>>> I have not followed latest patches due to high work load, my bad. 
>>>>> Marek
>>>>> thanks from pointing
>>>>>
>>>>> About ExynosDSI bridge handling:
>>>>>
>>>>> The order of calling encoder, bridge (and consequently panel) ops
>>>>> enforced by DRM core (bridge->pre_enable, encoder->enable,
>>>>> bridge->enable) does not fit to ExynosDSI hardware initialization
>>>>> sequence, if I remember correctly it does not fit to whole MIPI DSI
>>>>> standard (I think similar situation is with eDP). As a result DSI
>>>>> drivers must use some ugly workarounds, rely on HW properly coping 
>>>>> with
>>>>> incorrect sequences, or, as in case of ExynosDSI driver, just avoid
>>>>> using encoder->bridge chaining and call bridge ops by itself when 
>>>>> suitable.
>>>> Yes, that's definitely hack-ish, and I proposed 2 solutions to address
>>>> that in previous versions of this patchset, unfortunately I didn't get
>>>> any feedback so I went for the less invasive option (keep the hack but
>>>> adapt it to the double-linked list changes), which still lead to
>>>> regressions :-/.
>>>>
>>>> Just a reminder of my 2 proposals:
>>>>
>>>> 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you 
>>>> can
>>>>     split your enable/disable logic in 2 parts and make sure things 
>>>> are
>>>>     ready when the panel/next bridge tries to send DSI commands
>>>> 2/ move everything that's needed to send DSI commands out of the
>>>>     ->enable() path (maybe in runtime PM resume/suspend hooks) so you
>>>>     can call that in the DSI transfer path too
>>>>
>>>> As pointed out by Laurent, #1 doesn't work because some panel drivers
>>>> send DSI commands in their ->prepare() hook, and ->pre_enable() 
>>>> methods
>>>> are called in reverse order, meaning that the DRM panel bridge driver
>>>> would try to issue DSI commands before the DSI host controllers is 
>>>> ready
>>>> to send them. I still thing #2 is a good option.
>>>>> So proper patch converting to double-linked list should not try to
>>>>> splice ExynosDSI private bridge list with with encoder's, 
>>>>> encoder's list
>>>>> should be always empty, as Marek suggested.
>>>> That's exactly what I wanted to do: make the encoder's list empty 
>>>> after
>>>> attach() and restore it to its initial state before unregistering
>>>> the bridge, except I forgot that list_splice() doesn't call
>>>> INIT_LIST_HEAD(). It's still not clear to me why replacing the
>>>> list_splice() call by a list_splice_init() didn't work.
>>> Okay, I think I figured it out: drm_bridge_chain_xx() helpers use
>>> encoder->bridge_chain as their list head, and you'll never hit the 
>>> 'elem
>>> is list head' condition since we moved all elems from
>>> encoder->bridge_chain to exynos_dsi->bridge_chain. The only way this
>>> can work is if we stop using the helpers and implement our own list
>>> iterators.
>> Just to make it clear, calling INIT_LIST_HEAD(encoder->bridge_chain)
>> doesn't really fix the bug, it just prevents the hang (infinite loop)
>> and turn all drm_bridge_chain_xx() calls into NOPs.
>
> Right, I've just checked it and indeed the display chain outputs 
> nothing after such 'fix'. To get it finally working I've replaced 
> drm_bridge_chain_*() operations for exynos_dsi 'out_bridge' by a 
> direct calls. I will submit a patch in a few minutes. I hope that such 
> workaround can be used now to fix the regression until a better 
> solution is agreed.

I've posted a 'quick & working' fix for Exynos DRM DSI driver:

https://lkml.org/lkml/2019/12/27/121

Best regards

Boris Brezillon Dec. 27, 2019, 12:21 p.m. UTC | #16

On Fri, 27 Dec 2019 12:51:54 +0200
Laurent Pinchart <[e-mail]> wrote:

> Hi Andrzej,
> 
> On Fri, Dec 27, 2019 at 10:42:25AM +0100, Andrzej Hajda wrote:
> > On 24.12.2019 10:44, Boris Brezillon wrote:  
> > > On Tue, 24 Dec 2019 10:16:49 +0100 Andrzej Hajda wrote:  
> > >> On 23.12.2019 10:55, Marek Szyprowski wrote:  
> > >>> On 16.12.2019 16:25, Boris Brezillon wrote:    
> > >>>> On Mon, 16 Dec 2019 16:02:36 +0100 Marek Szyprowski wrote:    
> > >>>>> On 16.12.2019 15:55, Boris Brezillon wrote:    
> > >>>>>> On Mon, 16 Dec 2019 14:54:25 +0100
> > >>>>>> Marek Szyprowski <[e-mail]> wrote:    
> > >>>>>>> On 03.12.2019 15:15, Boris Brezillon wrote:    
> > >>>>>>>> So that each element in the chain can easily access its predecessor.
> > >>>>>>>> This will be needed to support bus format negotiation between elements
> > >>>>>>>> of the bridge chain.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
> > >>>>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
> > >>>>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>    
> > >>>>>>>
> > >>>>>>> I've noticed that this patch got merged to linux-next as commit
> > >>>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
> > >>>>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
> > >>>>>>> messages:
> > >>>>>>>
> > >>>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> > >>>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> > >>>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> > >>>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> > >>>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> > >>>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> > >>>>>>> [drm] No driver support for vblank timestamp query.
> > >>>>>>> [drm] Cannot find any crtc or sizes
> > >>>>>>> [drm] Cannot find any crtc or sizes
> > >>>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> > >>>>>>>
> > >>>>>>> I will try to debug this and provide more information soon.  
> > >>>>>>
> > >>>>>> Can you try with this diff applied?    
> > >>>>>
> > >>>>> This patch doesn't change anything.    
> > >>>>
> > >>>> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
> > >>>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
> > >>>> after the list_splice_init() call?    
> > >>>
> > >>> encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
> > >>>
> > >>> Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
> > >>> fixed the boot issue.  
> > >
> > > If INIT_LIST_HEAD() worked, I don't understand why replacing the
> > > list_splice() call by a list_splice_init() (which doing a list_splice()
> > > + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
> > > list_splice_init() version doesn't work?
> > >  
> > >>> It looks that this is related with the way the 
> > >>> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
> > >>> will give a bit more detailed comment and spread some light on this.    
> > >>
> > >> Hi Marek, Boris,
> > >>
> > >> I have not followed latest patches due to high work load, my bad. Marek
> > >> thanks from pointing
> > >>
> > >> About ExynosDSI bridge handling:
> > >>
> > >> The order of calling encoder, bridge (and consequently panel) ops
> > >> enforced by DRM core (bridge->pre_enable, encoder->enable,
> > >> bridge->enable) does not fit to ExynosDSI hardware initialization
> > >> sequence, if I remember correctly it does not fit to whole MIPI DSI
> > >> standard (I think similar situation is with eDP). As a result DSI
> > >> drivers must use some ugly workarounds, rely on HW properly coping with
> > >> incorrect sequences, or, as in case of ExynosDSI driver, just avoid
> > >> using encoder->bridge chaining and call bridge ops by itself when suitable.  
> > >
> > > Yes, that's definitely hack-ish, and I proposed 2 solutions to address
> > > that in previous versions of this patchset, unfortunately I didn't get
> > > any feedback so I went for the less invasive option (keep the hack but
> > > adapt it to the double-linked list changes), which still lead to
> > > regressions :-/.
> > >
> > > Just a reminder of my 2 proposals:
> > >
> > > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
> > >    split your enable/disable logic in 2 parts and make sure things are
> > >    ready when the panel/next bridge tries to send DSI commands  
> > 
> > If it means 'convert exynos_dsi to bridge' I do not think it will help -
> > 
> > - pre_enable op will be still called after pre_enable op of downstream
> > bridge - and this is the main reason why exynos_dsi do not use encoder
> > bridge chain - it needs to perform some operations BEFORE (pre)enabling
> > downstream devices.

Yep, I figured that after Laurent's review.

> >   
> > > 2/ move everything that's needed to send DSI commands out of the  
> > >    ->enable() path (maybe in runtime PM resume/suspend hooks) so you  
> > >    can call that in the DSI transfer path too  
> > 
> > It looks like a solution for DSI protocol, where control bus is shared
> > with data bus, but the problem is more general - we have source and sink
> > connected with some local bus, which has some negotiation/enable/disable
> > protocol/requirements. And drm_core/bridge framework enforces us to fit
> > every such protocol to 'drm_bridge protocol' with few opses called in
> > fixed order, without clearly defined purpose of each ops. That does not
> > sound generic and results in multiple issues:
> > 
> > - different drivers uses different opses to perform the same thing,
> > 
> > - different drivers assumes different things about their sinks/sources
> > in their opses,
> > 
> > - more complicated sequences does not fit at all to this model.
> > 
> > All this results in incompatibilities between drivers which become
> > visible with devices used in different configurations/platforms.

That's true, drm_bridge_funcs semantics is rather vague and probably
doesn't fit all needs, but that's not the only problem we have when it
comes to DSI IMHO. I mean, I couldn't find any doc in drm_mipi_dsi.h
explaining when panel/bridge drivers are allowed to send DCS commands.
I was personally assuming that we were allowed to send such commands as
soon as mipi_dsi_attach() was called, but that's not clearly not
possible with VC4 and Exynos. This part should be clarified too.

> 
> I fully agree with you, not defining the semantics of the bridge
> operations precisely was I believe a mistake, and we're paying the price
> now. That's OK, we "just" need to fix it :-)

Okay, so how do we fix that? :-)

I'm not a big fan of specializing the drm_bridge_funcs interface to fit
protocol X or protocol Y needs. Sounds like a never ending story,
protocol Z might require something slightly different, and we're likely
to end up with an interface that's not generic at all.

Maybe I'm wrong, but it sounds like all DSI ordering issues could be
addressed at the mipi_dsi_host_ops level if we define a new
->enter_power_state() hook and have the DSI framework keep track of the
host power state (LP,HS,PD,...). The framework would then make sure we
are in a valid state (LP or HS) before calling dsi_host->transfer(). If
we have that in place, I don't think we need new hooks at the bridge
level, at least not for the DSI case. Please let me know if I'm missing
something.

Boris Brezillon Dec. 27, 2019, 12:39 p.m. UTC | #17

On Tue, 24 Dec 2019 12:31:11 +0100
Sam Ravnborg <[e-mail]> wrote:

> Hi Boris.
> 
> > Just a reminder of my 2 proposals:
> > 
> > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
> >    split your enable/disable logic in 2 parts and make sure things are
> >    ready when the panel/next bridge tries to send DSI commands
> > 2/ move everything that's needed to send DSI commands out of the  
> >    ->enable() path (maybe in runtime PM resume/suspend hooks) so you  
> >    can call that in the DSI transfer path too
> > 
> > As pointed out by Laurent, #1 doesn't work because some panel drivers
> > send DSI commands in their ->prepare() hook, and ->pre_enable() methods
> > are called in reverse order, meaning that the DRM panel bridge driver
> > would try to issue DSI commands before the DSI host controllers is ready
> > to send them. I still thing #2 is a good option.  
> 
> Jitao Shi suggested to extend panels so we had a sequence of:
> 
>   prepare_power()  <= new callback,
>                    here one should NOT be allowed to send
>                    DSI commands
>   prepare()
>   enable()
> 
>    #
>    # panel is now ready to show your favourite christmas movie
>    #
> 
>   disable()
>   unprepare()
>   unprepare_power()  <= new callback
> 
> 
> Would this help implement what you suggest above?
> Relevant panels would then have to be updated - but this
> is doable.

I didn't look at Jitao's proposal but it looks like it's addressing a
similar issue on the DSI slave/device side: the device probably needs
to be powered before the host can interact with it through the DSI+DPHY
bus. I'm not entirely sure why we'd need another hook to do that since
we already have the ->prepare() one.

Laurent Pinchart Jan. 1, 2020, 5:13 p.m. UTC | #18

Hi Boris,

On Fri, Dec 27, 2019 at 01:21:31PM +0100, Boris Brezillon wrote:
> On Fri, 27 Dec 2019 12:51:54 +0200 Laurent Pinchart wrote:
> > On Fri, Dec 27, 2019 at 10:42:25AM +0100, Andrzej Hajda wrote:
> > > On 24.12.2019 10:44, Boris Brezillon wrote:  
> > > > On Tue, 24 Dec 2019 10:16:49 +0100 Andrzej Hajda wrote:  
> > > >> On 23.12.2019 10:55, Marek Szyprowski wrote:  
> > > >>> On 16.12.2019 16:25, Boris Brezillon wrote:    
> > > >>>> On Mon, 16 Dec 2019 16:02:36 +0100 Marek Szyprowski wrote:    
> > > >>>>> On 16.12.2019 15:55, Boris Brezillon wrote:    
> > > >>>>>> On Mon, 16 Dec 2019 14:54:25 +0100
> > > >>>>>> Marek Szyprowski <[e-mail]> wrote:    
> > > >>>>>>> On 03.12.2019 15:15, Boris Brezillon wrote:    
> > > >>>>>>>> So that each element in the chain can easily access its predecessor.
> > > >>>>>>>> This will be needed to support bus format negotiation between elements
> > > >>>>>>>> of the bridge chain.
> > > >>>>>>>>
> > > >>>>>>>> Signed-off-by: Boris Brezillon <[e-mail]>
> > > >>>>>>>> Reviewed-by: Neil Armstrong <[e-mail]>
> > > >>>>>>>> Reviewed-by: Laurent Pinchart <[e-mail]>    
> > > >>>>>>>
> > > >>>>>>> I've noticed that this patch got merged to linux-next as commit
> > > >>>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of
> > > >>>>>>> Samsung Exynos5250-based Arndale board. Booting stops after following
> > > >>>>>>> messages:
> > > >>>>>>>
> > > >>>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations
> > > >>>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops)
> > > >>>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops)
> > > >>>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops)
> > > >>>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops)
> > > >>>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> > > >>>>>>> [drm] No driver support for vblank timestamp query.
> > > >>>>>>> [drm] Cannot find any crtc or sizes
> > > >>>>>>> [drm] Cannot find any crtc or sizes
> > > >>>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
> > > >>>>>>>
> > > >>>>>>> I will try to debug this and provide more information soon.  
> > > >>>>>>
> > > >>>>>> Can you try with this diff applied?    
> > > >>>>>
> > > >>>>> This patch doesn't change anything.    
> > > >>>>
> > > >>>> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain
> > > >>>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and
> > > >>>> after the list_splice_init() call?    
> > > >>>
> > > >>> encoder->bridge_chain contains only one element. dsi->drive_chain is empty.
> > > >>>
> > > >>> Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) 
> > > >>> fixed the boot issue.  
> > > >
> > > > If INIT_LIST_HEAD() worked, I don't understand why replacing the
> > > > list_splice() call by a list_splice_init() (which doing a list_splice()
> > > > + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the
> > > > list_splice_init() version doesn't work?
> > > >  
> > > >>> It looks that this is related with the way the 
> > > >>> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej 
> > > >>> will give a bit more detailed comment and spread some light on this.    
> > > >>
> > > >> Hi Marek, Boris,
> > > >>
> > > >> I have not followed latest patches due to high work load, my bad. Marek
> > > >> thanks from pointing
> > > >>
> > > >> About ExynosDSI bridge handling:
> > > >>
> > > >> The order of calling encoder, bridge (and consequently panel) ops
> > > >> enforced by DRM core (bridge->pre_enable, encoder->enable,
> > > >> bridge->enable) does not fit to ExynosDSI hardware initialization
> > > >> sequence, if I remember correctly it does not fit to whole MIPI DSI
> > > >> standard (I think similar situation is with eDP). As a result DSI
> > > >> drivers must use some ugly workarounds, rely on HW properly coping with
> > > >> incorrect sequences, or, as in case of ExynosDSI driver, just avoid
> > > >> using encoder->bridge chaining and call bridge ops by itself when suitable.  
> > > >
> > > > Yes, that's definitely hack-ish, and I proposed 2 solutions to address
> > > > that in previous versions of this patchset, unfortunately I didn't get
> > > > any feedback so I went for the less invasive option (keep the hack but
> > > > adapt it to the double-linked list changes), which still lead to
> > > > regressions :-/.
> > > >
> > > > Just a reminder of my 2 proposals:
> > > >
> > > > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can
> > > >    split your enable/disable logic in 2 parts and make sure things are
> > > >    ready when the panel/next bridge tries to send DSI commands  
> > > 
> > > If it means 'convert exynos_dsi to bridge' I do not think it will help -
> > > 
> > > - pre_enable op will be still called after pre_enable op of downstream
> > > bridge - and this is the main reason why exynos_dsi do not use encoder
> > > bridge chain - it needs to perform some operations BEFORE (pre)enabling
> > > downstream devices.
> 
> Yep, I figured that after Laurent's review.
> 
> > > > 2/ move everything that's needed to send DSI commands out of the  
> > > >    ->enable() path (maybe in runtime PM resume/suspend hooks) so you  
> > > >    can call that in the DSI transfer path too  
> > > 
> > > It looks like a solution for DSI protocol, where control bus is shared
> > > with data bus, but the problem is more general - we have source and sink
> > > connected with some local bus, which has some negotiation/enable/disable
> > > protocol/requirements. And drm_core/bridge framework enforces us to fit
> > > every such protocol to 'drm_bridge protocol' with few opses called in
> > > fixed order, without clearly defined purpose of each ops. That does not
> > > sound generic and results in multiple issues:
> > > 
> > > - different drivers uses different opses to perform the same thing,
> > > 
> > > - different drivers assumes different things about their sinks/sources
> > > in their opses,
> > > 
> > > - more complicated sequences does not fit at all to this model.
> > > 
> > > All this results in incompatibilities between drivers which become
> > > visible with devices used in different configurations/platforms.
> 
> That's true, drm_bridge_funcs semantics is rather vague and probably
> doesn't fit all needs, but that's not the only problem we have when it
> comes to DSI IMHO. I mean, I couldn't find any doc in drm_mipi_dsi.h
> explaining when panel/bridge drivers are allowed to send DCS commands.
> I was personally assuming that we were allowed to send such commands as
> soon as mipi_dsi_attach() was called, but that's not clearly not
> possible with VC4 and Exynos. This part should be clarified too.
> 
> > I fully agree with you, not defining the semantics of the bridge
> > operations precisely was I believe a mistake, and we're paying the price
> > now. That's OK, we "just" need to fix it :-)
> 
> Okay, so how do we fix that? :-)
> 
> I'm not a big fan of specializing the drm_bridge_funcs interface to fit
> protocol X or protocol Y needs. Sounds like a never ending story,
> protocol Z might require something slightly different, and we're likely
> to end up with an interface that's not generic at all.
> 
> Maybe I'm wrong, but it sounds like all DSI ordering issues could be
> addressed at the mipi_dsi_host_ops level if we define a new
> ->enter_power_state() hook and have the DSI framework keep track of the
> host power state (LP,HS,PD,...). The framework would then make sure we
> are in a valid state (LP or HS) before calling dsi_host->transfer(). If
> we have that in place, I don't think we need new hooks at the bridge
> level, at least not for the DSI case. Please let me know if I'm missing
> something.

I'll have to review the mipi_dsi_host_ops in details to answer this, but
I think there will be at least two potential issues.

- The complexity on the DSI host side will increase due to the need to
  handle both the DSI host and bridge operations. We will have to define
  precise semantics for the bridge operations anyway to ensure they work
  properly with the DSI host operations, and that may just defeat the
  purpose of your proposal altogether.

- The issue isn't limited to ->transfer(), and DSI sink needs finer
  control of the source for the purpose of video transfer as well. As
  explained in another e-mail in this thread, it's not uncommon for DSI
  sinks to first require the source to start sending H/V sync packets
  without sending any video data, then get configured (through MIPI
  commands or an out-of-band channel such as I2C or SPI), and finally
  require the source to send video data.

Patch

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 54c874493c57..b6517b4fa3d1 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -55,7 +55,7 @@ 
  * just provide additional hooks to get the desired output at the end of the
  * encoder chain.
  *
- * Bridges can also be chained up using the &drm_bridge.next pointer.
+ * Bridges can also be chained up using the &drm_bridge.chain_node field.
  *
  * Both legacy CRTC helpers and the new atomic modeset helpers support bridges.
  */
@@ -128,20 +128,21 @@  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 	bridge->dev = encoder->dev;
 	bridge->encoder = encoder;
 
+	if (previous)
+		list_add(&bridge->chain_node, &previous->chain_node);
+	else
+		list_add(&bridge->chain_node, &encoder->bridge_chain);
+
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge);
 		if (ret < 0) {
+			list_del(&bridge->chain_node);
 			bridge->dev = NULL;
 			bridge->encoder = NULL;
 			return ret;
 		}
 	}
 
-	if (previous)
-		previous->next = bridge;
-	else
-		encoder->bridge = bridge;
-
 	return 0;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
@@ -157,6 +158,7 @@  void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	list_del(&bridge->chain_node);
 	bridge->dev = NULL;
 }
 
@@ -190,18 +192,21 @@  bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge,
 				 const struct drm_display_mode *mode,
 				 struct drm_display_mode *adjusted_mode)
 {
-	bool ret = true;
+	struct drm_encoder *encoder;
 
 	if (!bridge)
 		return true;
 
-	if (bridge->funcs->mode_fixup)
-		ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (!bridge->funcs->mode_fixup)
+			continue;
 
-	ret = ret && drm_bridge_chain_mode_fixup(bridge->next, mode,
-						 adjusted_mode);
+		if (!bridge->funcs->mode_fixup(bridge, mode, adjusted_mode))
+			return false;
+	}
 
-	return ret;
+	return true;
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_fixup);
 
@@ -224,18 +229,24 @@  enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 			    const struct drm_display_mode *mode)
 {
-	enum drm_mode_status ret = MODE_OK;
+	struct drm_encoder *encoder;
 
 	if (!bridge)
-		return ret;
+		return MODE_OK;
 
-	if (bridge->funcs->mode_valid)
-		ret = bridge->funcs->mode_valid(bridge, mode);
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		enum drm_mode_status ret;
+
+		if (!bridge->funcs->mode_valid)
+			continue;
 
-	if (ret != MODE_OK)
-		return ret;
+		ret = bridge->funcs->mode_valid(bridge, mode);
+		if (ret != MODE_OK)
+			return ret;
+	}
 
-	return drm_bridge_chain_mode_valid(bridge->next, mode);
+	return MODE_OK;
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
 
@@ -251,13 +262,20 @@  EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
  */
 void drm_bridge_chain_disable(struct drm_bridge *bridge)
 {
+	struct drm_encoder *encoder;
+	struct drm_bridge *iter;
+
 	if (!bridge)
 		return;
 
-	drm_bridge_chain_disable(bridge->next);
+	encoder = bridge->encoder;
+	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
+		if (iter->funcs->disable)
+			iter->funcs->disable(iter);
 
-	if (bridge->funcs->disable)
-		bridge->funcs->disable(bridge);
+		if (iter == bridge)
+			break;
+	}
 }
 EXPORT_SYMBOL(drm_bridge_chain_disable);
 
@@ -274,13 +292,16 @@  EXPORT_SYMBOL(drm_bridge_chain_disable);
  */
 void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
 {
+	struct drm_encoder *encoder;
+
 	if (!bridge)
 		return;
 
-	if (bridge->funcs->post_disable)
-		bridge->funcs->post_disable(bridge);
-
-	drm_bridge_chain_post_disable(bridge->next);
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (bridge->funcs->post_disable)
+			bridge->funcs->post_disable(bridge);
+	}
 }
 EXPORT_SYMBOL(drm_bridge_chain_post_disable);
 
@@ -300,13 +321,16 @@  void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 			       const struct drm_display_mode *mode,
 			       const struct drm_display_mode *adjusted_mode)
 {
+	struct drm_encoder *encoder;
+
 	if (!bridge)
 		return;
 
-	if (bridge->funcs->mode_set)
-		bridge->funcs->mode_set(bridge, mode, adjusted_mode);
-
-	drm_bridge_chain_mode_set(bridge->next, mode, adjusted_mode);
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (bridge->funcs->mode_set)
+			bridge->funcs->mode_set(bridge, mode, adjusted_mode);
+	}
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_set);
 
@@ -323,13 +347,17 @@  EXPORT_SYMBOL(drm_bridge_chain_mode_set);
  */
 void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
 {
+	struct drm_encoder *encoder;
+	struct drm_bridge *iter;
+
 	if (!bridge)
 		return;
 
-	drm_bridge_chain_pre_enable(bridge->next);
-
-	if (bridge->funcs->pre_enable)
-		bridge->funcs->pre_enable(bridge);
+	encoder = bridge->encoder;
+	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
+		if (iter->funcs->pre_enable)
+			iter->funcs->pre_enable(iter);
+	}
 }
 EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
 
@@ -345,13 +373,16 @@  EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
  */
 void drm_bridge_chain_enable(struct drm_bridge *bridge)
 {
+	struct drm_encoder *encoder;
+
 	if (!bridge)
 		return;
 
-	if (bridge->funcs->enable)
-		bridge->funcs->enable(bridge);
-
-	drm_bridge_chain_enable(bridge->next);
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (bridge->funcs->enable)
+			bridge->funcs->enable(bridge);
+	}
 }
 EXPORT_SYMBOL(drm_bridge_chain_enable);
 
@@ -370,15 +401,22 @@  EXPORT_SYMBOL(drm_bridge_chain_enable);
 void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 				     struct drm_atomic_state *state)
 {
+	struct drm_encoder *encoder;
+	struct drm_bridge *iter;
+
 	if (!bridge)
 		return;
 
-	drm_atomic_bridge_chain_disable(bridge->next, state);
+	encoder = bridge->encoder;
+	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
+		if (iter->funcs->atomic_disable)
+			iter->funcs->atomic_disable(iter, state);
+		else if (iter->funcs->disable)
+			iter->funcs->disable(iter);
 
-	if (bridge->funcs->atomic_disable)
-		bridge->funcs->atomic_disable(bridge, state);
-	else if (bridge->funcs->disable)
-		bridge->funcs->disable(bridge);
+		if (iter == bridge)
+			break;
+	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
 
@@ -398,15 +436,18 @@  EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
 void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 					  struct drm_atomic_state *state)
 {
+	struct drm_encoder *encoder;
+
 	if (!bridge)
 		return;
 
-	if (bridge->funcs->atomic_post_disable)
-		bridge->funcs->atomic_post_disable(bridge, state);
-	else if (bridge->funcs->post_disable)
-		bridge->funcs->post_disable(bridge);
-
-	drm_atomic_bridge_chain_post_disable(bridge->next, state);
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (bridge->funcs->atomic_post_disable)
+			bridge->funcs->atomic_post_disable(bridge, state);
+		else if (bridge->funcs->post_disable)
+			bridge->funcs->post_disable(bridge);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
 
@@ -426,15 +467,22 @@  EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
 void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 					struct drm_atomic_state *state)
 {
+	struct drm_encoder *encoder;
+	struct drm_bridge *iter;
+
 	if (!bridge)
 		return;
 
-	drm_atomic_bridge_chain_pre_enable(bridge->next, state);
+	encoder = bridge->encoder;
+	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
+		if (iter->funcs->atomic_pre_enable)
+			iter->funcs->atomic_pre_enable(iter, state);
+		else if (iter->funcs->pre_enable)
+			iter->funcs->pre_enable(iter);
 
-	if (bridge->funcs->atomic_pre_enable)
-		bridge->funcs->atomic_pre_enable(bridge, state);
-	else if (bridge->funcs->pre_enable)
-		bridge->funcs->pre_enable(bridge);
+		if (iter == bridge)
+			break;
+	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
 
@@ -453,15 +501,18 @@  EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 				    struct drm_atomic_state *state)
 {
+	struct drm_encoder *encoder;
+
 	if (!bridge)
 		return;
 
-	if (bridge->funcs->atomic_enable)
-		bridge->funcs->atomic_enable(bridge, state);
-	else if (bridge->funcs->enable)
-		bridge->funcs->enable(bridge);
-
-	drm_atomic_bridge_chain_enable(bridge->next, state);
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (bridge->funcs->atomic_enable)
+			bridge->funcs->atomic_enable(bridge, state);
+		else if (bridge->funcs->enable)
+			bridge->funcs->enable(bridge);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
 
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index a2cc7e7241a9..e555281f43d4 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -140,6 +140,7 @@  int drm_encoder_init(struct drm_device *dev,
 		goto out_put;
 	}
 
+	INIT_LIST_HEAD(&encoder->bridge_chain);
 	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
 	encoder->index = dev->mode_config.num_encoder++;
 
@@ -160,23 +161,16 @@  EXPORT_SYMBOL(drm_encoder_init);
 void drm_encoder_cleanup(struct drm_encoder *encoder)
 {
 	struct drm_device *dev = encoder->dev;
+	struct drm_bridge *bridge, *next;
 
 	/* Note that the encoder_list is considered to be static; should we
 	 * remove the drm_encoder at runtime we would have to decrement all
 	 * the indices on the drm_encoder after us in the encoder_list.
 	 */
 
-	if (encoder->bridge) {
-		struct drm_bridge *bridge;
-		struct drm_bridge *next;
-
-		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		while (bridge) {
-			next = drm_bridge_get_next_bridge(bridge);
-			drm_bridge_detach(bridge);
-			bridge = next;
-		}
-	}
+	list_for_each_entry_safe(bridge, next, &encoder->bridge_chain,
+				 chain_node)
+		drm_bridge_detach(bridge);
 
 	drm_mode_object_unregister(dev, &encoder->base);
 	kfree(encoder->name);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index d984097704b8..7de82e22252a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -255,6 +255,7 @@  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;
 
@@ -1522,7 +1523,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;
-		encoder->bridge = NULL;
+		list_splice(&encoder->bridge_chain, &dsi->bridge_chain);
 	} else {
 		int ret = exynos_dsi_create_connector(encoder);
 
@@ -1588,6 +1589,7 @@  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)
@@ -1735,6 +1737,7 @@  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;
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index ff81b54ea281..6c5b80ad6154 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -499,6 +499,7 @@  struct vc4_dsi {
 	struct mipi_dsi_host dsi_host;
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
+	struct list_head bridge_chain;
 
 	void __iomem *regs;
 
@@ -1460,6 +1461,8 @@  static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 				       GFP_KERNEL);
 	if (!vc4_dsi_encoder)
 		return -ENOMEM;
+
+	INIT_LIST_HEAD(&dsi->bridge_chain);
 	vc4_dsi_encoder->base.type = VC4_ENCODER_TYPE_DSI1;
 	vc4_dsi_encoder->dsi = dsi;
 	dsi->encoder = &vc4_dsi_encoder->base.base;
@@ -1610,7 +1613,7 @@  static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	 * from our driver, since we need to sequence them within the
 	 * encoder's enable/disable paths.
 	 */
-	dsi->encoder->bridge = NULL;
+	list_splice(&dsi->encoder->bridge_chain, &dsi->bridge_chain);
 
 	if (dsi->port == 0)
 		vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);
@@ -1632,6 +1635,11 @@  static void vc4_dsi_unbind(struct device *dev, struct device *master,
 	if (dsi->bridge)
 		pm_runtime_disable(dev);
 
+	/*
+	 * Restore the bridge_chain so the bridge detach procedure can happen
+	 * normally.
+	 */
+	list_splice(&dsi->bridge_chain, &dsi->encoder->bridge_chain);
 	vc4_dsi_encoder_destroy(dsi->encoder);
 
 	if (dsi->port == 1)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bd78c256b1ed..c118726469ee 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -384,8 +384,8 @@  struct drm_bridge {
 	struct drm_device *dev;
 	/** @encoder: encoder to which this bridge is connected */
 	struct drm_encoder *encoder;
-	/** @next: the next bridge in the encoder chain */
-	struct drm_bridge *next;
+	/** @chain_node: used to form a bridge chain */
+	struct list_head chain_node;
 #ifdef CONFIG_OF
 	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
@@ -420,7 +420,10 @@  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 static inline struct drm_bridge *
 drm_bridge_get_next_bridge(struct drm_bridge *bridge)
 {
-	return bridge->next;
+	if (list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain))
+		return NULL;
+
+	return list_next_entry(bridge, chain_node);
 }
 
 /**
@@ -434,7 +437,8 @@  drm_bridge_get_next_bridge(struct drm_bridge *bridge)
 static inline struct drm_bridge *
 drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
 {
-	return encoder->bridge;
+	return list_first_entry_or_null(&encoder->bridge_chain,
+					struct drm_bridge, chain_node);
 }
 
 bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge,
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index f06164f44efe..5623994b6e9e 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -172,7 +172,12 @@  struct drm_encoder {
 	 * &drm_connector_state.crtc.
 	 */
 	struct drm_crtc *crtc;
-	struct drm_bridge *bridge;
+
+	/**
+	 * @bridge_chain: Bridges attached to this encoder.
+	 */
+	struct list_head bridge_chain;
+
 	const struct drm_encoder_funcs *funcs;
 	const struct drm_encoder_helper_funcs *helper_private;
 };