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

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 boris.brezillon@collabora.com Reviewed-by: Neil Armstrong narmstrong@baylibre.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

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