Hi Boris,

Thank you for the patch.

On Wed, Oct 23, 2019 at 05:44:52PM +0200, Boris Brezillon wrote:

Encoder drivers will progressively transition to the drm_bridge interface in place of the drm_encoder one.

Let's start with the VC4 driver, and use the ->pre_{enable,disable}() hooks to get rid of the hack resetting encoder->bridge.next (which was needed to control the encoder/bridge enable/disable sequence).

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v3:

• Embed a drm_bridge object in vc4_dsi since drm_encoder no longer has a dummy bridge

Changes in v2:

• New patch (replaces "drm/vc4: Get rid of the dsi->bridge field")

---

drivers/gpu/drm/vc4/vc4_dsi.c | 88 +++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index c9ba83ed49b9..49f8a313e759 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -498,7 +498,11 @@ struct vc4_dsi {

struct mipi_dsi_host dsi_host; struct drm_encoder *encoder;

• struct drm_bridge *bridge;

• /* Embed a bridge object so we can implement bridge funcs instead of

• * encoder ones.
  

• */
  

• struct drm_bridge bridge;

  void __iomem *regs;

@@ -543,6 +547,11 @@ struct vc4_dsi { struct debugfs_regset32 regset; };

+static inline struct vc4_dsi *bridge_to_vc4_dsi(struct drm_bridge *bridge) +{

• return container_of(bridge, struct vc4_dsi, bridge);

+}

#define host_to_dsi(host) container_of(host, struct vc4_dsi, dsi_host)

static inline void @@ -747,16 +756,11 @@ dsi_esc_timing(u32 ns) return DIV_ROUND_UP(ns, ESC_TIME_NS); }

-static void vc4_dsi_encoder_disable(struct drm_encoder *encoder) +static void vc4_dsi_bridge_post_disable(struct drm_bridge *bridge) {

• struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
• struct vc4_dsi *dsi = vc4_encoder->dsi;

• struct vc4_dsi *dsi = bridge_to_vc4_dsi(bridge); struct device *dev = &dsi->pdev->dev;

• drm_bridge_disable(dsi->bridge);
• vc4_dsi_ulps(dsi, true);
• drm_bridge_post_disable(dsi->bridge);
• clk_disable_unprepare(dsi->pll_phy_clock); clk_disable_unprepare(dsi->escape_clock); clk_disable_unprepare(dsi->pixel_clock);

@@ -764,6 +768,13 @@ static void vc4_dsi_encoder_disable(struct drm_encoder *encoder) pm_runtime_put(dev); }

+static void vc4_dsi_bridge_disable(struct drm_bridge *bridge) +{

• struct vc4_dsi *dsi = bridge_to_vc4_dsi(bridge);
• vc4_dsi_ulps(dsi, true);

+}

/* Extends the mode's blank intervals to handle BCM2835's integer-only

• DSI PLL divider.

@@ -777,12 +788,11 @@ static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)

• higher-than-expected clock rate to the panel, but that's what the
• firmware does too.

*/ -static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,

• 		       const struct drm_display_mode *mode,
  

• 		       struct drm_display_mode *adjusted_mode)
  

+static bool vc4_dsi_bridge_mode_fixup(struct drm_bridge *bridge,

• 		      const struct drm_display_mode *mode,
  

• 		      struct drm_display_mode *adjusted_mode)
  

{

• struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
• struct vc4_dsi *dsi = vc4_encoder->dsi;

• struct vc4_dsi *dsi = bridge_to_vc4_dsi(bridge); struct clk *phy_parent = clk_get_parent(dsi->pll_phy_clock); unsigned long parent_rate = clk_get_rate(phy_parent); unsigned long pixel_clock_hz = mode->clock * 1000;

@@ -816,11 +826,11 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder, return true; }

-static void vc4_dsi_encoder_enable(struct drm_encoder *encoder) +static void vc4_dsi_bridge_pre_enable(struct drm_bridge *bridge) {

• struct drm_encoder *encoder = bridge->encoder; struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;

• struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
• struct vc4_dsi *dsi = vc4_encoder->dsi;

• struct vc4_dsi *dsi = bridge_to_vc4_dsi(bridge); struct device *dev = &dsi->pdev->dev; bool debug_dump_regs = false; unsigned long hs_clock;

@@ -1054,8 +1064,12 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder) }

vc4_dsi_ulps(dsi, false); +}

• drm_bridge_pre_enable(dsi->bridge);

If I'm not mistaken this switches the order of the DSI's encoder pre-enable and the next bridge's pre-enable. I think it's true for post-disable too. It may not be a problem, but have this been tested ?

+static void vc4_dsi_bridge_enable(struct drm_bridge *bridge) +{

• struct vc4_dsi *dsi = bridge_to_vc4_dsi(bridge);

• bool debug_dump_regs = false;

  if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) { DSI_PORT_WRITE(DISP0_CTRL,

@@ -1072,8 +1086,6 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder) DSI_DISP0_ENABLE); }

• drm_bridge_enable(dsi->bridge);
• if (debug_dump_regs) { struct drm_printer p = drm_info_printer(&dsi->pdev->dev); dev_info(&dsi->pdev->dev, "DSI regs after:\n");

@@ -1290,10 +1302,12 @@ static const struct mipi_dsi_host_ops vc4_dsi_host_ops = { .transfer = vc4_dsi_host_transfer, };

-static const struct drm_encoder_helper_funcs vc4_dsi_encoder_helper_funcs = {

• .disable = vc4_dsi_encoder_disable,
• .enable = vc4_dsi_encoder_enable,
• .mode_fixup = vc4_dsi_encoder_mode_fixup,

+static const struct drm_bridge_funcs vc4_dsi_bridge_funcs = {

• .pre_enable = vc4_dsi_bridge_pre_enable,
• .enable = vc4_dsi_bridge_enable,
• .disable = vc4_dsi_bridge_disable,
• .post_disable = vc4_dsi_bridge_post_disable,
• .mode_fixup = vc4_dsi_bridge_mode_fixup,

};

static const struct of_device_id vc4_dsi_dt_match[] = { @@ -1445,6 +1459,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_dsi *dsi = dev_get_drvdata(dev); struct vc4_dsi_encoder *vc4_dsi_encoder;

• struct drm_bridge *next_bridge; struct drm_panel *panel; const struct of_device_id *match; dma_cap_mask_t dma_mask;

@@ -1561,7 +1576,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) }

ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,

• 			  &panel, &dsi->bridge);
  

• 			  &panel, &next_bridge);
  

  if (ret) { /* If the bridge or panel pointed by dev->of_node is not
  • enabled, just return 0 here so that we don't prevent the DRM

@@ -1576,10 +1591,10 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) }

if (panel) {

• dsi->bridge = devm_drm_panel_bridge_add_typed(dev, panel,
  

• next_bridge = devm_drm_panel_bridge_add_typed(dev, panel,
  				      DRM_MODE_CONNECTOR_DSI);
  

• if (IS_ERR(dsi->bridge))
  

• 	return PTR_ERR(dsi->bridge);
  

• if (IS_ERR(next_bridge))
  

• 	return PTR_ERR(next_bridge);
  

  }

  /* The esc clock rate is supposed to always be 100Mhz. */

@@ -1598,19 +1613,20 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)

drm_encoder_init(drm, dsi->encoder, &vc4_dsi_encoder_funcs, DRM_MODE_ENCODER_DSI, NULL);

• drm_encoder_helper_add(dsi->encoder, &vc4_dsi_encoder_helper_funcs);

• ret = drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);

• /* Declare ourself as the first bridge element. */
• dsi->bridge.funcs = &vc4_dsi_bridge_funcs;
• ret = drm_bridge_attach(dsi->encoder, &dsi->bridge, NULL);
• if (ret) {

• dev_err(dev, "bridge attach failed: %d\n", ret);
  

• return ret;
  

• }
• ret = drm_bridge_attach(dsi->encoder, next_bridge, &dsi->bridge); if (ret) { dev_err(dev, "bridge attach failed: %d\n", ret); return ret; }

This is usually done in the bridge attach operation. As we're in control we can attach the next bridge here, but I think the driver would look more standard if you moved the second attach call to this bridge's attach operation.

With this fixed, and if the driver has been tested and the enable/disable order change doesn't cause issues,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

• /* Disable the atomic helper calls into the bridge. We

• * manually call the bridge pre_enable / enable / etc. calls
  

• * from our driver, since we need to sequence them within the
  

• * encoder's enable/disable paths.
  

• */
  

• dsi->encoder->bridge = NULL;

  if (dsi->port == 0) vc4_debugfs_add_regset32(drm, "dsi0_regs", &dsi->regset);

@@ -1629,7 +1645,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_dsi *dsi = dev_get_drvdata(dev);

• if (dsi->bridge)

• if (dsi->bridge.next) pm_runtime_disable(dev);

  vc4_dsi_encoder_destroy(dsi->encoder);

Hi Laurent,

On Sun, 24 Nov 2019 12:24:33 +0200 Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Boris,

Thank you for the patch.

On Wed, Oct 23, 2019 at 05:44:54PM +0200, Boris Brezillon wrote:

...

Encoder drivers will progressively transition to the drm_bridge interface in place of the drm_encoder one.

Converting the Exynos DSI encoder driver to this approach allows us to use the ->pre_{enable,disable}() hooks and get rid of the hack resetting encoder->bridge.next (which was needed to control the encoder/bridge enable/disable sequence).

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v3:

• Embed a drm_bridge object in exynos_dsi since drm_encoder no longer has a dummy bridge

Changes in v2:

• New patch (replacement for "drm/exynos: Get rid of exynos_dsi->out_bridge")

---

drivers/gpu/drm/exynos/exynos_drm_dsi.c | 89 +++++++++++++++---------- 1 file changed, 55 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 72726f2c7a9f..3915f50b005e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -252,10 +252,10 @@ struct exynos_dsi_driver_data {

struct exynos_dsi { struct drm_encoder encoder;

• struct drm_bridge bridge; struct mipi_dsi_host dsi_host; struct drm_connector connector; struct drm_panel *panel;

• struct drm_bridge *out_bridge; struct device *dev;

  void __iomem *reg_base;

@@ -291,6 +291,11 @@ static inline struct exynos_dsi *encoder_to_dsi(struct drm_encoder *e) return container_of(e, struct exynos_dsi, encoder); }

+static inline struct exynos_dsi *bridge_to_dsi(struct drm_bridge *b) +{

• return container_of(b, struct exynos_dsi, bridge);

+}

enum reg_idx { DSIM_STATUS_REG, /* Status register */ DSIM_SWRST_REG, /* Software reset register */ @@ -1374,25 +1379,38 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi) } }

-static void exynos_dsi_enable(struct drm_encoder *encoder) +static void exynos_dsi_pre_enable(struct drm_bridge *bridge) {

• struct exynos_dsi *dsi = encoder_to_dsi(encoder);

• struct exynos_dsi *dsi = bridge_to_dsi(bridge); int ret;

  if (dsi->state & DSIM_STATE_ENABLED) return;

This can probably be removed now as the core should ensure that double-enable or double-disable never occurs, but it can be done in a separate patch.

Except the enable/disable() implementations handle failures (the framework does not expect those to fails BTW), and I guess it's important to know the actual HW state in order to keep runtime PM get/put calls balanced.

...

pm_runtime_get_sync(dsi->dev);

• dsi->state |= DSIM_STATE_ENABLED;

  if (dsi->panel) { ret = drm_panel_prepare(dsi->panel); if (ret < 0) goto err_put_sync;

• } else {

• drm_bridge_pre_enable(dsi->out_bridge);
  

  }

It would be nice to switch to the drm panel bridge, but that can also be done on top of this series.

I agree, just didn't want to add more stuff to this series.

...

• dsi->state |= DSIM_STATE_ENABLED;
• return;

+err_put_sync:

• pm_runtime_put(dsi->dev);

+}

+static void exynos_dsi_enable(struct drm_bridge *bridge) +{

• struct exynos_dsi *dsi = bridge_to_dsi(bridge);
• int ret;
• if (!(dsi->state & DSIM_STATE_ENABLED) ||

•    (dsi->state & DSIM_STATE_VIDOUT_AVAILABLE))
  

• return;
  

• exynos_dsi_set_display_mode(dsi); exynos_dsi_set_display_enable(dsi, true);

@@ -1400,8 +1418,6 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) ret = drm_panel_enable(dsi->panel); if (ret < 0) goto err_display_disable;

• } else {

• drm_bridge_enable(dsi->out_bridge);
  

  }

  dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;

@@ -1410,28 +1426,30 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) err_display_disable: exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel);

Does this belong here, as drm_panel_prepare() was called in exynos_dsi_pre_enable() ?

Nope, this one should be dropped.

...

-err_put_sync:

• dsi->state &= ~DSIM_STATE_ENABLED;
• pm_runtime_put(dsi->dev);

}

-static void exynos_dsi_disable(struct drm_encoder *encoder) +static void exynos_dsi_disable(struct drm_bridge *bridge) {

• struct exynos_dsi *dsi = encoder_to_dsi(encoder);

• struct exynos_dsi *dsi = bridge_to_dsi(bridge);
• if (!(dsi->state & DSIM_STATE_VIDOUT_AVAILABLE))

• return;
  

• drm_panel_disable(dsi->panel);
• exynos_dsi_set_display_enable(dsi, false);
• dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;

+}

+static void exynos_dsi_post_disable(struct drm_bridge *bridge) +{

• struct exynos_dsi *dsi = bridge_to_dsi(bridge);

  if (!(dsi->state & DSIM_STATE_ENABLED)) return;

• dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
• drm_panel_disable(dsi->panel);
• drm_bridge_disable(dsi->out_bridge);
• exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel);
• drm_bridge_post_disable(dsi->out_bridge);
• dsi->state &= ~DSIM_STATE_ENABLED; pm_runtime_put_sync(dsi->dev);

• dsi->state &= ~DSIM_STATE_ENABLED;

}

static enum drm_connector_status @@ -1499,9 +1517,11 @@ static int exynos_dsi_create_connector(struct drm_encoder *encoder) return 0; }

-static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = { +static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {

• .pre_enable = exynos_dsi_pre_enable, .enable = exynos_dsi_enable, .disable = exynos_dsi_disable,
• .post_disable = exynos_dsi_post_disable,

};

static const struct drm_encoder_funcs exynos_dsi_encoder_funcs = { @@ -1520,9 +1540,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,

out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) {

• drm_bridge_attach(encoder, out_bridge, NULL);
  

• dsi->out_bridge = out_bridge;
  

• encoder->bridge = NULL;
  

• drm_bridge_attach(encoder, out_bridge, &dsi->bridge);
  

  } else { int ret = exynos_dsi_create_connector(encoder);

@@ -1575,19 +1593,19 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host);

• struct drm_bridge *out_bridge = dsi->bridge.next; struct drm_device *drm = dsi->encoder.dev;

  if (dsi->panel) { mutex_lock(&drm->mode_config.mutex);

• exynos_dsi_disable(&dsi->encoder);
  

• exynos_dsi_disable(&dsi->bridge);
  

• exynos_dsi_post_disable(&dsi->bridge);
  

  drm_panel_detach(dsi->panel); dsi->panel = NULL; dsi->connector.status = connector_status_disconnected; mutex_unlock(&drm->mode_config.mutex);

• } else {

• if (dsi->out_bridge->funcs->detach)
  

• 	dsi->out_bridge->funcs->detach(dsi->out_bridge);
  

• dsi->out_bridge = NULL;
  

• } else if (out_bridge && out_bridge->funcs->detach) {

• out_bridge->funcs->detach(out_bridge);  
  

Maybe drm_bridge_detach() ?

This function is not exported, and I suppose that's why they used the function pointer in this driver. I bet there's a good reason for not exposing this function...

...

}

if (drm->mode_config.poll_enabled) @@ -1687,16 +1705,18 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, drm_encoder_init(drm_dev, encoder, &exynos_dsi_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);

• drm_encoder_helper_add(encoder, &exynos_dsi_encoder_helper_funcs);
• ret = exynos_drm_set_possible_crtcs(encoder, EXYNOS_DISPLAY_TYPE_LCD); if (ret < 0) return ret;

• /* Declare ourself as the first bridge element. */
• dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
• drm_bridge_attach(encoder, &dsi->bridge, NULL);
• if (dsi->in_bridge_node) { in_bridge = of_drm_find_bridge(dsi->in_bridge_node); if (in_bridge)

• 	drm_bridge_attach(encoder, in_bridge, NULL);
  

• 	drm_bridge_attach(encoder, in_bridge, &dsi->bridge);
  

  }

Same as for patch 01/21, maybe this could be moved to this bridge's attach operation ? Actually, now that I've read the code, this in_bridge part looks weird. Why would the DSI encoder have an input bridge that is has to manage itself ?

Yes, I know, it doesn't make any sense. Either we're dealing with a bridge which can be chained to other bridges (can be placed in the middle of a chain as well), or we're dealing with an encoder which precedes any bridges. In the latter case (which is how exynos_dsi is implemented) in_bridge doesn't have any meaning, and that's even worse since we're placing the so-called input bridge (AKA previous bridge) after our encoder (that's what drm_bridge_attach(encoder, in_bridge, NULL) does).

TBH, I didn't want to go that far and fix existing drivers when I started this series, so I think I'll rework the patchset to get rid of the VC4 and exynos patches, even if that means having 2 drivers that mess up with the encoder->bridge_chain list.

Regards,

Boris

Hi Boris,

On Sun, Nov 24, 2019 at 02:17:27PM +0100, Boris Brezillon wrote:

On Sun, 24 Nov 2019 12:24:33 +0200 Laurent Pinchart wrote:

...

On Wed, Oct 23, 2019 at 05:44:54PM +0200, Boris Brezillon wrote:

...

Encoder drivers will progressively transition to the drm_bridge interface in place of the drm_encoder one.

Converting the Exynos DSI encoder driver to this approach allows us to use the ->pre_{enable,disable}() hooks and get rid of the hack resetting encoder->bridge.next (which was needed to control the encoder/bridge enable/disable sequence).

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v3:

• Embed a drm_bridge object in exynos_dsi since drm_encoder no longer has a dummy bridge

Changes in v2:

• New patch (replacement for "drm/exynos: Get rid of exynos_dsi->out_bridge")

---

drivers/gpu/drm/exynos/exynos_drm_dsi.c | 89 +++++++++++++++---------- 1 file changed, 55 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 72726f2c7a9f..3915f50b005e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -252,10 +252,10 @@ struct exynos_dsi_driver_data {

struct exynos_dsi { struct drm_encoder encoder;

• struct drm_bridge bridge; struct mipi_dsi_host dsi_host; struct drm_connector connector; struct drm_panel *panel;

• struct drm_bridge *out_bridge; struct device *dev;

  void __iomem *reg_base;

@@ -291,6 +291,11 @@ static inline struct exynos_dsi *encoder_to_dsi(struct drm_encoder *e) return container_of(e, struct exynos_dsi, encoder); }

+static inline struct exynos_dsi *bridge_to_dsi(struct drm_bridge *b) +{

• return container_of(b, struct exynos_dsi, bridge);

+}

enum reg_idx { DSIM_STATUS_REG, /* Status register */ DSIM_SWRST_REG, /* Software reset register */ @@ -1374,25 +1379,38 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi) } }

-static void exynos_dsi_enable(struct drm_encoder *encoder) +static void exynos_dsi_pre_enable(struct drm_bridge *bridge) {

• struct exynos_dsi *dsi = encoder_to_dsi(encoder);

• struct exynos_dsi *dsi = bridge_to_dsi(bridge); int ret;

  if (dsi->state & DSIM_STATE_ENABLED) return;

This can probably be removed now as the core should ensure that double-enable or double-disable never occurs, but it can be done in a separate patch.

Except the enable/disable() implementations handle failures (the framework does not expect those to fails BTW), and I guess it's important to know the actual HW state in order to keep runtime PM get/put calls balanced.

...

...

pm_runtime_get_sync(dsi->dev);

• dsi->state |= DSIM_STATE_ENABLED;

  if (dsi->panel) { ret = drm_panel_prepare(dsi->panel); if (ret < 0) goto err_put_sync;

• } else {

• drm_bridge_pre_enable(dsi->out_bridge);
  

  }

It would be nice to switch to the drm panel bridge, but that can also be done on top of this series.

I agree, just didn't want to add more stuff to this series.

...

...

• dsi->state |= DSIM_STATE_ENABLED;
• return;

+err_put_sync:

• pm_runtime_put(dsi->dev);

+}

+static void exynos_dsi_enable(struct drm_bridge *bridge) +{

• struct exynos_dsi *dsi = bridge_to_dsi(bridge);
• int ret;
• if (!(dsi->state & DSIM_STATE_ENABLED) ||

•    (dsi->state & DSIM_STATE_VIDOUT_AVAILABLE))
  

• return;
  

• exynos_dsi_set_display_mode(dsi); exynos_dsi_set_display_enable(dsi, true);

@@ -1400,8 +1418,6 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) ret = drm_panel_enable(dsi->panel); if (ret < 0) goto err_display_disable;

• } else {

• drm_bridge_enable(dsi->out_bridge);
  

  }

  dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;

@@ -1410,28 +1426,30 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) err_display_disable: exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel);

Does this belong here, as drm_panel_prepare() was called in exynos_dsi_pre_enable() ?

Nope, this one should be dropped.

...

...

-err_put_sync:

• dsi->state &= ~DSIM_STATE_ENABLED;
• pm_runtime_put(dsi->dev);

}

-static void exynos_dsi_disable(struct drm_encoder *encoder) +static void exynos_dsi_disable(struct drm_bridge *bridge) {

• struct exynos_dsi *dsi = encoder_to_dsi(encoder);

• struct exynos_dsi *dsi = bridge_to_dsi(bridge);
• if (!(dsi->state & DSIM_STATE_VIDOUT_AVAILABLE))

• return;
  

• drm_panel_disable(dsi->panel);
• exynos_dsi_set_display_enable(dsi, false);
• dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;

+}

+static void exynos_dsi_post_disable(struct drm_bridge *bridge) +{

• struct exynos_dsi *dsi = bridge_to_dsi(bridge);

  if (!(dsi->state & DSIM_STATE_ENABLED)) return;

• dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
• drm_panel_disable(dsi->panel);
• drm_bridge_disable(dsi->out_bridge);
• exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel);
• drm_bridge_post_disable(dsi->out_bridge);
• dsi->state &= ~DSIM_STATE_ENABLED; pm_runtime_put_sync(dsi->dev);

• dsi->state &= ~DSIM_STATE_ENABLED;

}

static enum drm_connector_status @@ -1499,9 +1517,11 @@ static int exynos_dsi_create_connector(struct drm_encoder *encoder) return 0; }

-static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = { +static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {

• .pre_enable = exynos_dsi_pre_enable, .enable = exynos_dsi_enable, .disable = exynos_dsi_disable,
• .post_disable = exynos_dsi_post_disable,

};

static const struct drm_encoder_funcs exynos_dsi_encoder_funcs = { @@ -1520,9 +1540,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,

out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) {

• drm_bridge_attach(encoder, out_bridge, NULL);
  

• dsi->out_bridge = out_bridge;
  

• encoder->bridge = NULL;
  

• drm_bridge_attach(encoder, out_bridge, &dsi->bridge);
  

  } else { int ret = exynos_dsi_create_connector(encoder);

@@ -1575,19 +1593,19 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host);

• struct drm_bridge *out_bridge = dsi->bridge.next; struct drm_device *drm = dsi->encoder.dev;

  if (dsi->panel) { mutex_lock(&drm->mode_config.mutex);

• exynos_dsi_disable(&dsi->encoder);
  

• exynos_dsi_disable(&dsi->bridge);
  

• exynos_dsi_post_disable(&dsi->bridge);
  

  drm_panel_detach(dsi->panel); dsi->panel = NULL; dsi->connector.status = connector_status_disconnected; mutex_unlock(&drm->mode_config.mutex);

• } else {

• if (dsi->out_bridge->funcs->detach)
  

• 	dsi->out_bridge->funcs->detach(dsi->out_bridge);
  

• dsi->out_bridge = NULL;
  

• } else if (out_bridge && out_bridge->funcs->detach) {

• out_bridge->funcs->detach(out_bridge);  
  

Maybe drm_bridge_detach() ?

This function is not exported, and I suppose that's why they used the function pointer in this driver. I bet there's a good reason for not exposing this function...

Indeed, my bad. It's called by drm_encoder_cleanup(), I assume it's not enough ? If there's a good use case for exporting it, then I think it should be exported.

...

...

}

if (drm->mode_config.poll_enabled) @@ -1687,16 +1705,18 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, drm_encoder_init(drm_dev, encoder, &exynos_dsi_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL);

• drm_encoder_helper_add(encoder, &exynos_dsi_encoder_helper_funcs);
• ret = exynos_drm_set_possible_crtcs(encoder, EXYNOS_DISPLAY_TYPE_LCD); if (ret < 0) return ret;

• /* Declare ourself as the first bridge element. */
• dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
• drm_bridge_attach(encoder, &dsi->bridge, NULL);
• if (dsi->in_bridge_node) { in_bridge = of_drm_find_bridge(dsi->in_bridge_node); if (in_bridge)

• 	drm_bridge_attach(encoder, in_bridge, NULL);
  

• 	drm_bridge_attach(encoder, in_bridge, &dsi->bridge);
  

  }

Same as for patch 01/21, maybe this could be moved to this bridge's attach operation ? Actually, now that I've read the code, this in_bridge part looks weird. Why would the DSI encoder have an input bridge that is has to manage itself ?

Yes, I know, it doesn't make any sense. Either we're dealing with a bridge which can be chained to other bridges (can be placed in the middle of a chain as well), or we're dealing with an encoder which precedes any bridges. In the latter case (which is how exynos_dsi is implemented) in_bridge doesn't have any meaning, and that's even worse since we're placing the so-called input bridge (AKA previous bridge) after our encoder (that's what drm_bridge_attach(encoder, in_bridge, NULL) does).

Can we get input from the exynos maintainers ? Or is the driver not actively maintained anymore ?

TBH, I didn't want to go that far and fix existing drivers when I started this series, so I think I'll rework the patchset to get rid of the VC4 and exynos patches, even if that means having 2 drivers that mess up with the encoder->bridge_chain list.

I don't mind the above changes really (and the one for VC4 seems pretty fine so far).

On 23/10/2019 17:44, Boris Brezillon wrote:

Change the prefix of bridge helpers targeting a bridge chain from drm_bridge_ to drm_bridge_chain_ to better reflect the fact that the operation will happen on all elements of chain, starting at the bridge passed in argument.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v3:

• None

Changes in v2:

• Pass te bridge, not the encoder, so we can later act on a sub-chain instead of the whole chain

---

drivers/gpu/drm/drm_atomic_helper.c | 19 +++-- drivers/gpu/drm/drm_bridge.c | 125 ++++++++++++++-------------- drivers/gpu/drm/drm_probe_helper.c | 2 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +- include/drm/drm_bridge.h | 64 +++++++------- 5 files changed, 112 insertions(+), 102 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 587052751b48..cf678be58fa4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -435,8 +435,9 @@ mode_fixup(struct drm_atomic_state *state) encoder = new_conn_state->best_encoder; funcs = encoder->helper_private;

• ret = drm_bridge_mode_fixup(encoder->bridge, &new_crtc_state->mode,
  

• 		&new_crtc_state->adjusted_mode);
  

• ret = drm_bridge_chain_mode_fixup(encoder->bridge,
  

• 			&new_crtc_state->mode,
  

• 			&new_crtc_state->adjusted_mode);
  

  if (!ret) { DRM_DEBUG_ATOMIC("Bridge fixup failed\n"); return -EINVAL;

@@ -501,7 +502,7 @@ static enum drm_mode_status mode_valid_path(struct drm_connector *connector, return ret; }

• ret = drm_bridge_mode_valid(encoder->bridge, mode);

• ret = drm_bridge_chain_mode_valid(encoder->bridge, mode); if (ret != MODE_OK) { DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); return ret;

@@ -1020,7 +1021,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) * Each encoder has at most one connector (since we always steal * it away), so we won't call disable hooks twice. */

• drm_atomic_bridge_disable(encoder->bridge, old_state);
  

• drm_atomic_bridge_chain_disable(encoder->bridge, old_state);
  
  

  /* Right function depends upon target state. */ if (funcs) {

@@ -1034,7 +1035,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) funcs->dpms(encoder, DRM_MODE_DPMS_OFF); }

• drm_atomic_bridge_post_disable(encoder->bridge, old_state);
  

• drm_atomic_bridge_chain_post_disable(encoder->bridge,
  

• 				     old_state);
  

  }

  for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {

@@ -1215,7 +1217,8 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) funcs->mode_set(encoder, mode, adjusted_mode); }

• drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
  

• drm_bridge_chain_mode_set(encoder->bridge, mode,
  

• 			  adjusted_mode);
  

  }

}

@@ -1332,7 +1335,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, * Each encoder has at most one connector (since we always steal * it away), so we won't call enable hooks twice. */

• drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
  

• drm_atomic_bridge_chain_pre_enable(encoder->bridge, old_state);
  
  

  if (funcs) { if (funcs->atomic_enable)

@@ -1343,7 +1346,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, funcs->commit(encoder); }

• drm_atomic_bridge_enable(encoder->bridge, old_state);
  

• drm_atomic_bridge_chain_enable(encoder->bridge, old_state);
  

  }

  drm_atomic_helper_commit_writebacks(dev, old_state);

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..54c874493c57 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -172,8 +172,8 @@ void drm_bridge_detach(struct drm_bridge *bridge) */

/**

• • drm_bridge_mode_fixup - fixup proposed mode for all bridges in the

• • 	   encoder chain
    

• • drm_bridge_chain_mode_fixup - fixup proposed mode for all bridges in the

• • 		 encoder chain
    

  • @bridge: bridge control structure
  • @mode: desired mode to be set for the bridge
  • @adjusted_mode: updated mode that works for this bridge

@@ -186,9 +186,9 @@ void drm_bridge_detach(struct drm_bridge *bridge)

• RETURNS:
• true on success, false on failure

*/ -bool drm_bridge_mode_fixup(struct drm_bridge *bridge,

• 	const struct drm_display_mode *mode,
  

• 	struct drm_display_mode *adjusted_mode)
  

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

@@ -198,15 +198,16 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, if (bridge->funcs->mode_fixup) ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);

• ret = ret && drm_bridge_mode_fixup(bridge->next, mode, adjusted_mode);

• ret = ret && drm_bridge_chain_mode_fixup(bridge->next, mode,

• 				 adjusted_mode);
  
  

  return ret;

} -EXPORT_SYMBOL(drm_bridge_mode_fixup); +EXPORT_SYMBOL(drm_bridge_chain_mode_fixup);

/**

• • drm_bridge_mode_valid - validate the mode against all bridges in the

• • 	   encoder chain.
    

• • drm_bridge_chain_mode_valid - validate the mode against all bridges in the

• • 		 encoder chain.
    

  • @bridge: bridge control structure
  • @mode: desired mode to be validated

@@ -219,8 +220,9 @@ EXPORT_SYMBOL(drm_bridge_mode_fixup);

• RETURNS:
• MODE_OK on success, drm_mode_status Enum error code on failure

*/ -enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,

• 			   const struct drm_display_mode *mode)
  

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

@@ -233,12 +235,12 @@ enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge, if (ret != MODE_OK) return ret;

• return drm_bridge_mode_valid(bridge->next, mode);

• return drm_bridge_chain_mode_valid(bridge->next, mode);

} -EXPORT_SYMBOL(drm_bridge_mode_valid); +EXPORT_SYMBOL(drm_bridge_chain_mode_valid);

/**

• • drm_bridge_disable - disables all bridges in the encoder chain

• • drm_bridge_chain_disable - disables all bridges in the encoder chain
  • @bridge: bridge control structure
  • Calls &drm_bridge_funcs.disable op for all the bridges in the encoder

@@ -247,20 +249,21 @@ EXPORT_SYMBOL(drm_bridge_mode_valid);

• Note: the bridge passed should be the one closest to the encoder

*/ -void drm_bridge_disable(struct drm_bridge *bridge) +void drm_bridge_chain_disable(struct drm_bridge *bridge) { if (!bridge) return;

• drm_bridge_disable(bridge->next);

• drm_bridge_chain_disable(bridge->next);

  if (bridge->funcs->disable) bridge->funcs->disable(bridge);

} -EXPORT_SYMBOL(drm_bridge_disable); +EXPORT_SYMBOL(drm_bridge_chain_disable);

/**

• • drm_bridge_post_disable - cleans up after disabling all bridges in the encoder chain

• • drm_bridge_chain_post_disable - cleans up after disabling all bridges in the

• • 		   encoder chain
    

  • @bridge: bridge control structure
  • Calls &drm_bridge_funcs.post_disable op for all the bridges in the

@@ -269,7 +272,7 @@ EXPORT_SYMBOL(drm_bridge_disable);

• Note: the bridge passed should be the one closest to the encoder

*/ -void drm_bridge_post_disable(struct drm_bridge *bridge) +void drm_bridge_chain_post_disable(struct drm_bridge *bridge) { if (!bridge) return; @@ -277,25 +280,25 @@ void drm_bridge_post_disable(struct drm_bridge *bridge) if (bridge->funcs->post_disable) bridge->funcs->post_disable(bridge);

• drm_bridge_post_disable(bridge->next);

• drm_bridge_chain_post_disable(bridge->next);

} -EXPORT_SYMBOL(drm_bridge_post_disable); +EXPORT_SYMBOL(drm_bridge_chain_post_disable);

/**

• • drm_bridge_mode_set - set proposed mode for all bridges in the

• • 	 encoder chain
    

• • drm_bridge_chain_mode_set - set proposed mode for all bridges in the

• • 	       encoder chain
    

  • @bridge: bridge control structure

• • @mode: desired mode to be set for the bridge
• • @adjusted_mode: updated mode that works for this bridge

• • @mode: desired mode to be set for the encoder chain
• • @adjusted_mode: updated mode that works for this encoder chain
  • Calls &drm_bridge_funcs.mode_set op for all the bridges in the
  • encoder chain, starting from the first bridge to the last.
  • Note: the bridge passed should be the one closest to the encoder
  */

-void drm_bridge_mode_set(struct drm_bridge *bridge,

• 	 const struct drm_display_mode *mode,
  

• 	 const struct drm_display_mode *adjusted_mode)
  

+void drm_bridge_chain_mode_set(struct drm_bridge *bridge,

• 	       const struct drm_display_mode *mode,
  

• 	       const struct drm_display_mode *adjusted_mode)
  

{ if (!bridge) return; @@ -303,13 +306,13 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, if (bridge->funcs->mode_set) bridge->funcs->mode_set(bridge, mode, adjusted_mode);

• drm_bridge_mode_set(bridge->next, mode, adjusted_mode);

• drm_bridge_chain_mode_set(bridge->next, mode, adjusted_mode);

} -EXPORT_SYMBOL(drm_bridge_mode_set); +EXPORT_SYMBOL(drm_bridge_chain_mode_set);

/**

• • drm_bridge_pre_enable - prepares for enabling all

• • 	   bridges in the encoder chain
    

• • drm_bridge_chain_pre_enable - prepares for enabling all bridges in the

• • 		 encoder chain
    

  • @bridge: bridge control structure
  • Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder

@@ -318,20 +321,20 @@ EXPORT_SYMBOL(drm_bridge_mode_set);

• Note: the bridge passed should be the one closest to the encoder

*/ -void drm_bridge_pre_enable(struct drm_bridge *bridge) +void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) { if (!bridge) return;

• drm_bridge_pre_enable(bridge->next);

• drm_bridge_chain_pre_enable(bridge->next);

  if (bridge->funcs->pre_enable) bridge->funcs->pre_enable(bridge);

} -EXPORT_SYMBOL(drm_bridge_pre_enable); +EXPORT_SYMBOL(drm_bridge_chain_pre_enable);

/**

• • drm_bridge_enable - enables all bridges in the encoder chain

• • drm_bridge_chain_enable - enables all bridges in the encoder chain
  • @bridge: bridge control structure
  • Calls &drm_bridge_funcs.enable op for all the bridges in the encoder

@@ -340,7 +343,7 @@ EXPORT_SYMBOL(drm_bridge_pre_enable);

• Note that the bridge passed should be the one closest to the encoder

*/ -void drm_bridge_enable(struct drm_bridge *bridge) +void drm_bridge_chain_enable(struct drm_bridge *bridge) { if (!bridge) return; @@ -348,12 +351,12 @@ void drm_bridge_enable(struct drm_bridge *bridge) if (bridge->funcs->enable) bridge->funcs->enable(bridge);

• drm_bridge_enable(bridge->next);

• drm_bridge_chain_enable(bridge->next);

} -EXPORT_SYMBOL(drm_bridge_enable); +EXPORT_SYMBOL(drm_bridge_chain_enable);

/**

• • drm_atomic_bridge_disable - disables all bridges in the encoder chain

• • drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
  • @bridge: bridge control structure
  • @state: atomic state being committed

@@ -364,24 +367,24 @@ EXPORT_SYMBOL(drm_bridge_enable);

• Note: the bridge passed should be the one closest to the encoder

*/ -void drm_atomic_bridge_disable(struct drm_bridge *bridge,

• 	       struct drm_atomic_state *state)
  

+void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,

• 		     struct drm_atomic_state *state)
  

{ if (!bridge) return;

• drm_atomic_bridge_disable(bridge->next, state);

• drm_atomic_bridge_chain_disable(bridge->next, state);

  if (bridge->funcs->atomic_disable) bridge->funcs->atomic_disable(bridge, state); else if (bridge->funcs->disable) bridge->funcs->disable(bridge);

} -EXPORT_SYMBOL(drm_atomic_bridge_disable); +EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);

/**

• • drm_atomic_bridge_post_disable - cleans up after disabling all bridges in the

• • 		    encoder chain
    

• • drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges

• • 			  in the encoder chain
    

  • @bridge: bridge control structure
  • @state: atomic state being committed

@@ -392,8 +395,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_disable);

• Note: the bridge passed should be the one closest to the encoder

*/ -void drm_atomic_bridge_post_disable(struct drm_bridge *bridge,

• 		    struct drm_atomic_state *state)
  

+void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,

• 			  struct drm_atomic_state *state)
  

{ if (!bridge) return; @@ -403,13 +406,13 @@ void drm_atomic_bridge_post_disable(struct drm_bridge *bridge, else if (bridge->funcs->post_disable) bridge->funcs->post_disable(bridge);

• drm_atomic_bridge_post_disable(bridge->next, state);

• drm_atomic_bridge_chain_post_disable(bridge->next, state);

} -EXPORT_SYMBOL(drm_atomic_bridge_post_disable); +EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);

/**

• • drm_atomic_bridge_pre_enable - prepares for enabling all bridges in the

• • 		  encoder chain
    

• • drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in

• • 			the encoder chain
    

  • @bridge: bridge control structure
  • @state: atomic state being committed

@@ -420,23 +423,23 @@ EXPORT_SYMBOL(drm_atomic_bridge_post_disable);

• Note: the bridge passed should be the one closest to the encoder

*/ -void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,

• 		  struct drm_atomic_state *state)
  

+void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,

• 			struct drm_atomic_state *state)
  

{ if (!bridge) return;

• drm_atomic_bridge_pre_enable(bridge->next, state);

• drm_atomic_bridge_chain_pre_enable(bridge->next, state);

  if (bridge->funcs->atomic_pre_enable) bridge->funcs->atomic_pre_enable(bridge, state); else if (bridge->funcs->pre_enable) bridge->funcs->pre_enable(bridge);

} -EXPORT_SYMBOL(drm_atomic_bridge_pre_enable); +EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);

/**

• • drm_atomic_bridge_enable - enables all bridges in the encoder chain

• • drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
  • @bridge: bridge control structure
  • @state: atomic state being committed

@@ -447,8 +450,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_pre_enable);

• Note: the bridge passed should be the one closest to the encoder

*/ -void drm_atomic_bridge_enable(struct drm_bridge *bridge,

• 	      struct drm_atomic_state *state)
  

+void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,

• 		    struct drm_atomic_state *state)
  

{ if (!bridge) return; @@ -458,9 +461,9 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge, else if (bridge->funcs->enable) bridge->funcs->enable(bridge);

• drm_atomic_bridge_enable(bridge->next, state);

• drm_atomic_bridge_chain_enable(bridge->next, state);

} -EXPORT_SYMBOL(drm_atomic_bridge_enable); +EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

#ifdef CONFIG_OF /** diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index a7c87abe88d0..c3ea722065c4 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -112,7 +112,7 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode, continue; }

• ret = drm_bridge_mode_valid(encoder->bridge, mode);
  

• ret = drm_bridge_chain_mode_valid(encoder->bridge, mode);
  

  if (ret != MODE_OK) { /* There is also no point in continuing for crtc check * here. */

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index c79b1f855d89..ea68b5adccbe 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1247,8 +1247,8 @@ static int mtk_hdmi_conn_mode_valid(struct drm_connector *conn, struct drm_display_mode adjusted_mode;

drm_mode_copy(&adjusted_mode, mode);

• if (!drm_bridge_mode_fixup(hdmi->bridge.next, mode,
  

• 			   &adjusted_mode))
  

• if (!drm_bridge_chain_mode_fixup(hdmi->bridge.next, mode,
  

• 				 &adjusted_mode))
  return MODE_BAD;
  

  }

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c0a2286a81e9..726435baf4ad 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -254,9 +254,10 @@ struct drm_bridge_funcs { * there is one) when this callback is called. * * Note that this function will only be invoked in the context of an

• * atomic commit. It will not be invoked from &drm_bridge_pre_enable. It
  

• * would be prudent to also provide an implementation of @pre_enable if
  

• * you are expecting driver calls into &drm_bridge_pre_enable.
  

• * atomic commit. It will not be invoked from
  

• * &drm_bridge_chain_pre_enable. It would be prudent to also provide an
  

• * implementation of @pre_enable if you are expecting driver calls into
  

• * &drm_bridge_chain_pre_enable.
  

  • The @atomic_pre_enable callback is optional.
  */

@@ -279,9 +280,9 @@ struct drm_bridge_funcs { * chain if there is one. * * Note that this function will only be invoked in the context of an

• * atomic commit. It will not be invoked from &drm_bridge_enable. It
  

• * would be prudent to also provide an implementation of @enable if
  

• * you are expecting driver calls into &drm_bridge_enable.
  

• * atomic commit. It will not be invoked from &drm_bridge_chain_enable.
  

• * It would be prudent to also provide an implementation of @enable if
  

• * you are expecting driver calls into &drm_bridge_chain_enable.
  

  • The @atomic_enable callback is optional.
  */

@@ -301,9 +302,10 @@ struct drm_bridge_funcs { * signals) feeding it is still running when this callback is called. * * Note that this function will only be invoked in the context of an

• * atomic commit. It will not be invoked from &drm_bridge_disable. It
  

• * would be prudent to also provide an implementation of @disable if
  

• * you are expecting driver calls into &drm_bridge_disable.
  

• * atomic commit. It will not be invoked from
  

• * &drm_bridge_chain_disable. It would be prudent to also provide an
  

• * implementation of @disable if you are expecting driver calls into
  

• * &drm_bridge_chain_disable.
  

  • The @atomic_disable callback is optional.
  */

@@ -325,10 +327,11 @@ struct drm_bridge_funcs { * called. * * Note that this function will only be invoked in the context of an

• * atomic commit. It will not be invoked from &drm_bridge_post_disable.
  

• * atomic commit. It will not be invoked from
  

• * &drm_bridge_chain_post_disable.
  

  • It would be prudent to also provide an implementation of
  • @post_disable if you are expecting driver calls into

• * &drm_bridge_post_disable.
  

• * &drm_bridge_chain_post_disable.
  

  • The @atomic_post_disable callback is optional.
  */

@@ -406,27 +409,28 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);

-bool drm_bridge_mode_fixup(struct drm_bridge *bridge,

• 	   const struct drm_display_mode *mode,
  

• 	   struct drm_display_mode *adjusted_mode);
  

-enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,

• 			   const struct drm_display_mode *mode);
  

-void drm_bridge_disable(struct drm_bridge *bridge); -void drm_bridge_post_disable(struct drm_bridge *bridge); -void drm_bridge_mode_set(struct drm_bridge *bridge,

• 	 const struct drm_display_mode *mode,
  

• 	 const struct drm_display_mode *adjusted_mode);
  

-void drm_bridge_pre_enable(struct drm_bridge *bridge); -void drm_bridge_enable(struct drm_bridge *bridge); +bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge,

• 		 const struct drm_display_mode *mode,
  

• 		 struct drm_display_mode *adjusted_mode);
  

+enum drm_mode_status +drm_bridge_chain_mode_valid(struct drm_bridge *bridge,

• 	    const struct drm_display_mode *mode);
  

+void drm_bridge_chain_disable(struct drm_bridge *bridge); +void drm_bridge_chain_post_disable(struct drm_bridge *bridge); +void drm_bridge_chain_mode_set(struct drm_bridge *bridge,

• 	       const struct drm_display_mode *mode,
  

• 	       const struct drm_display_mode *adjusted_mode);
  

+void drm_bridge_chain_pre_enable(struct drm_bridge *bridge); +void drm_bridge_chain_enable(struct drm_bridge *bridge);

-void drm_atomic_bridge_disable(struct drm_bridge *bridge,

• 	       struct drm_atomic_state *state);
  

-void drm_atomic_bridge_post_disable(struct drm_bridge *bridge, +void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,

• 		     struct drm_atomic_state *state);
  

+void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,

• 			  struct drm_atomic_state *state);
  

+void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,

• 			struct drm_atomic_state *state);
  

+void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, struct drm_atomic_state *state); -void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,

• 		  struct drm_atomic_state *state);
  

-void drm_atomic_bridge_enable(struct drm_bridge *bridge,

• 	      struct drm_atomic_state *state);
  
  

#ifdef CONFIG_DRM_PANEL_BRIDGE struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);

Reviewed-by: Neil Armstrong narmstrong@baylibre.com

Hi Boris,

Thank you for the patch.

On Wed, Oct 23, 2019 at 05:44:56PM +0200, Boris Brezillon wrote:

And use it in drivers accessing the bridge->next field directly. This is part of our attempt to make the bridge chain a double-linked list based on the generic list helpers.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v3:

• Inline drm_bridge_chain_get_next_bridge() (Suggested by Laurent)

Changes in v2:

• Kill the last/first helpers (they're not really needed)
• Drop the !bridge || !bridge->encoder test

---

drivers/gpu/drm/exynos/exynos_drm_dsi.c | 3 ++- drivers/gpu/drm/mediatek/mtk_hdmi.c | 6 ++++-- drivers/gpu/drm/omapdrm/omap_drv.c | 4 ++-- drivers/gpu/drm/omapdrm/omap_encoder.c | 3 ++- drivers/gpu/drm/vc4/vc4_dsi.c | 4 +++- include/drm/drm_bridge.h | 13 +++++++++++++ 6 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 3915f50b005e..005c67894b78 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1593,9 +1593,10 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host);

• struct drm_bridge *out_bridge = dsi->bridge.next; struct drm_device *drm = dsi->encoder.dev;

• struct drm_bridge *out_bridge;

• out_bridge = drm_bridge_chain_get_next_bridge(&dsi->bridge);

You may want to store this in the exynos_dsi structure in the previous patch where you rework this driver.

if (dsi->panel) { mutex_lock(&drm->mode_config.mutex); exynos_dsi_disable(&dsi->bridge); diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index ea68b5adccbe..cfaa5aab8876 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1238,16 +1238,18 @@ static int mtk_hdmi_conn_mode_valid(struct drm_connector *conn, struct drm_display_mode *mode) { struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);

• struct drm_bridge *next_bridge;

  dev_dbg(hdmi->dev, "xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n", mode->hdisplay, mode->vdisplay, mode->vrefresh, !!(mode->flags & DRM_MODE_FLAG_INTERLACE), mode->clock * 1000);

• if (hdmi->bridge.next) {

• next_bridge = drm_bridge_chain_get_next_bridge(&hdmi->bridge);

• if (next_bridge) { struct drm_display_mode adjusted_mode;

  drm_mode_copy(&adjusted_mode, mode);

• if (!drm_bridge_chain_mode_fixup(hdmi->bridge.next, mode,
  

• if (!drm_bridge_chain_mode_fixup(next_bridge, mode,
  			 &adjusted_mode))
  return MODE_BAD;
  

  }

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index b3e22c890c51..865164fe28dc 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -217,8 +217,8 @@ static int omap_display_id(struct omap_dss_device *output) } else if (output->bridge) { struct drm_bridge *bridge = output->bridge;

• while (bridge->next)
  

• 	bridge = bridge->next;
  

• while (drm_bridge_chain_get_next_bridge(bridge))
  

• 	bridge = drm_bridge_chain_get_next_bridge(bridge);
  
  

  node = bridge->of_node; } else if (output->panel) {

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index 24bbe9f2a32e..8ca54081997e 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -126,7 +126,8 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder, for (dssdev = output; dssdev; dssdev = dssdev->next) omap_encoder_update_videomode_flags(&vm, dssdev->bus_flags);

• for (bridge = output->bridge; bridge; bridge = bridge->next) {

• for (bridge = output->bridge; bridge;

•     bridge = drm_bridge_chain_get_next_bridge(bridge)) {
  

A for_each_bridge() macro would be nice (in a separate patch). It could be used in omap_drv.c above too.

if (!bridge->timings)
	continue;

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 49f8a313e759..49c47185aff0 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1644,8 +1644,10 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, struct drm_device *drm = dev_get_drvdata(master); struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_dsi *dsi = dev_get_drvdata(dev);

• struct drm_bridge *bridge;

• if (dsi->bridge.next)

• bridge = drm_bridge_chain_get_next_bridge(&dsi->bridge);

• if (bridge) pm_runtime_disable(dev);

  vc4_dsi_encoder_destroy(dsi->encoder);

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 726435baf4ad..8aeba83fcf31 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -409,6 +409,19 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);

+/**

• • drm_bridge_chain_get_next_bridge() - Get the next bridge in the chain
• • @bridge: bridge object
• • RETURNS:
• • the next bridge in the chain, or NULL if @bridge is the last.

Maybe "the next bridge in the chain after @bridge, ..." ?

• */

+static inline struct drm_bridge * +drm_bridge_chain_get_next_bridge(struct drm_bridge *bridge)

Technically speaking this doesn't operate on a chain but on a bridge, so I'd name is drm_bridge_get_next_bridge(). I will not insist to the way of nacking the series for this, so with the rename, but also without,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

+{

• return bridge->next;

+}

bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode);

Hi Boris,

On Sun, Nov 24, 2019 at 11:56:16AM +0100, Boris Brezillon wrote:

On Sun, 24 Nov 2019 12:33:35 +0200 Laurent Pinchart wrote:

...

On Wed, Oct 23, 2019 at 05:44:56PM +0200, Boris Brezillon wrote:

...

And use it in drivers accessing the bridge->next field directly. This is part of our attempt to make the bridge chain a double-linked list based on the generic list helpers.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v3:

• Inline drm_bridge_chain_get_next_bridge() (Suggested by Laurent)

Changes in v2:

• Kill the last/first helpers (they're not really needed)
• Drop the !bridge || !bridge->encoder test

---

drivers/gpu/drm/exynos/exynos_drm_dsi.c | 3 ++- drivers/gpu/drm/mediatek/mtk_hdmi.c | 6 ++++-- drivers/gpu/drm/omapdrm/omap_drv.c | 4 ++-- drivers/gpu/drm/omapdrm/omap_encoder.c | 3 ++- drivers/gpu/drm/vc4/vc4_dsi.c | 4 +++- include/drm/drm_bridge.h | 13 +++++++++++++ 6 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 3915f50b005e..005c67894b78 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1593,9 +1593,10 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host);

• struct drm_bridge *out_bridge = dsi->bridge.next; struct drm_device *drm = dsi->encoder.dev;

• struct drm_bridge *out_bridge;

• out_bridge = drm_bridge_chain_get_next_bridge(&dsi->bridge);

You may want to store this in the exynos_dsi structure in the previous patch where you rework this driver.

Do we really need to store it there, since we already have a simple way to retrieve the next bridge in the chain?

We don't have to indeed. I thought it would be simpler, but that's probably subjective.

...

...

if (dsi->panel) { mutex_lock(&drm->mode_config.mutex); exynos_dsi_disable(&dsi->bridge); diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index ea68b5adccbe..cfaa5aab8876 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1238,16 +1238,18 @@ static int mtk_hdmi_conn_mode_valid(struct drm_connector *conn, struct drm_display_mode *mode) { struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);

• struct drm_bridge *next_bridge;

  dev_dbg(hdmi->dev, "xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n", mode->hdisplay, mode->vdisplay, mode->vrefresh, !!(mode->flags & DRM_MODE_FLAG_INTERLACE), mode->clock * 1000);

• if (hdmi->bridge.next) {

• next_bridge = drm_bridge_chain_get_next_bridge(&hdmi->bridge);

• if (next_bridge) { struct drm_display_mode adjusted_mode;

  drm_mode_copy(&adjusted_mode, mode);

• if (!drm_bridge_chain_mode_fixup(hdmi->bridge.next, mode,
  

• if (!drm_bridge_chain_mode_fixup(next_bridge, mode,
  			 &adjusted_mode))
  return MODE_BAD;
  

  }

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index b3e22c890c51..865164fe28dc 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -217,8 +217,8 @@ static int omap_display_id(struct omap_dss_device *output) } else if (output->bridge) { struct drm_bridge *bridge = output->bridge;

• while (bridge->next)
  

• 	bridge = bridge->next;
  

• while (drm_bridge_chain_get_next_bridge(bridge))
  

• 	bridge = drm_bridge_chain_get_next_bridge(bridge);
  
  

  node = bridge->of_node; } else if (output->panel) {

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index 24bbe9f2a32e..8ca54081997e 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -126,7 +126,8 @@ static void omap_encoder_mode_set(struct drm_encoder *encoder, for (dssdev = output; dssdev; dssdev = dssdev->next) omap_encoder_update_videomode_flags(&vm, dssdev->bus_flags);

• for (bridge = output->bridge; bridge; bridge = bridge->next) {

• for (bridge = output->bridge; bridge;

•     bridge = drm_bridge_chain_get_next_bridge(bridge)) {  
  

A for_each_bridge() macro would be nice (in a separate patch). It could be used in omap_drv.c above too.

It's coming later in the series (patch 8).

...

...

if (!bridge->timings)
	continue;

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 49f8a313e759..49c47185aff0 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1644,8 +1644,10 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, struct drm_device *drm = dev_get_drvdata(master); struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_dsi *dsi = dev_get_drvdata(dev);

• struct drm_bridge *bridge;

• if (dsi->bridge.next)

• bridge = drm_bridge_chain_get_next_bridge(&dsi->bridge);

• if (bridge) pm_runtime_disable(dev);

  vc4_dsi_encoder_destroy(dsi->encoder);

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 726435baf4ad..8aeba83fcf31 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -409,6 +409,19 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous);

+/**

• • drm_bridge_chain_get_next_bridge() - Get the next bridge in the chain
• • @bridge: bridge object
• • RETURNS:
• • the next bridge in the chain, or NULL if @bridge is the last.

Maybe "the next bridge in the chain after @bridge, ..." ?

Agreed.

...

...

• */

+static inline struct drm_bridge * +drm_bridge_chain_get_next_bridge(struct drm_bridge *bridge)

Technically speaking this doesn't operate on a chain but on a bridge, so I'd name is drm_bridge_get_next_bridge(). I will not insist to the way of nacking the series for this, so with the rename, but also without,

You're absolutely right, I'll rename the function as suggested.

...

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

...

+{

• return bridge->next;

+}

bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode);

On 23/10/2019 17:44, Boris Brezillon wrote:

We are about to replace the single-linked bridge list by a double-linked one based on list.h, leading to the suppression of the encoder->bridge field. But before we can do that we must provide a drm_bridge_chain_get_first_bridge() bridge helper and patch all drivers and core helpers to use it instead of directly accessing encoder->bridge.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

drivers/gpu/drm/drm_atomic_helper.c | 25 +++++++++++++++++-------- drivers/gpu/drm/drm_encoder.c | 3 ++- drivers/gpu/drm/drm_probe_helper.c | 4 +++- drivers/gpu/drm/msm/edp/edp_bridge.c | 10 ++++++++-- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 11 ++++++++--- include/drm/drm_bridge.h | 15 +++++++++++++++ 6 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index cf678be58fa4..f02ddffd4960 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -419,6 +419,7 @@ mode_fixup(struct drm_atomic_state *state) for_each_new_connector_in_state(state, connector, new_conn_state, i) { const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;

• struct drm_bridge *bridge;
  
  

  WARN_ON(!!new_conn_state->best_encoder != !!new_conn_state->crtc);

@@ -435,7 +436,8 @@ mode_fixup(struct drm_atomic_state *state) encoder = new_conn_state->best_encoder; funcs = encoder->helper_private;

• ret = drm_bridge_chain_mode_fixup(encoder->bridge,
  

• bridge = drm_bridge_chain_get_first_bridge(encoder);
  

• ret = drm_bridge_chain_mode_fixup(bridge,
  		&new_crtc_state->mode,
  		&new_crtc_state->adjusted_mode);
  

  if (!ret) {

@@ -493,6 +495,7 @@ static enum drm_mode_status mode_valid_path(struct drm_connector *connector, struct drm_crtc *crtc, const struct drm_display_mode *mode) {

• struct drm_bridge *bridge; enum drm_mode_status ret;

  ret = drm_encoder_mode_valid(encoder, mode);

@@ -502,7 +505,8 @@ static enum drm_mode_status mode_valid_path(struct drm_connector *connector, return ret; }

• ret = drm_bridge_chain_mode_valid(encoder->bridge, mode);

• bridge = drm_bridge_chain_get_first_bridge(encoder);
• ret = drm_bridge_chain_mode_valid(bridge, mode); if (ret != MODE_OK) { DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); return ret;

@@ -985,6 +989,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) { const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;

• struct drm_bridge *bridge;
  
  

  /* Shut down everything that's in the changeset and currently

  • still on. So need to check the old, saved state. */

@@ -1021,7 +1026,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) * Each encoder has at most one connector (since we always steal * it away), so we won't call disable hooks twice. */

• drm_atomic_bridge_chain_disable(encoder->bridge, old_state);
  

• bridge = drm_bridge_chain_get_first_bridge(encoder);
  

• drm_atomic_bridge_chain_disable(bridge, old_state);
  
  

  /* Right function depends upon target state. */ if (funcs) {

@@ -1035,7 +1041,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) funcs->dpms(encoder, DRM_MODE_DPMS_OFF); }

• drm_atomic_bridge_chain_post_disable(encoder->bridge,
  

• drm_atomic_bridge_chain_post_disable(bridge,
  			     old_state);
  

  }

@@ -1190,6 +1196,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder; struct drm_display_mode *mode, *adjusted_mode;

• struct drm_bridge *bridge;
  
  

  if (!new_conn_state->best_encoder) continue;

@@ -1217,8 +1224,8 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) funcs->mode_set(encoder, mode, adjusted_mode); }

• drm_bridge_chain_mode_set(encoder->bridge, mode,
  

• 			  adjusted_mode);
  

• bridge = drm_bridge_chain_get_first_bridge(encoder);
  

• drm_bridge_chain_mode_set(bridge, mode, adjusted_mode);
  

  }

}

@@ -1317,6 +1324,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, for_each_new_connector_in_state(old_state, connector, new_conn_state, i) { const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;

• struct drm_bridge *bridge;
  
  

  if (!new_conn_state->best_encoder) continue;

@@ -1335,7 +1343,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, * Each encoder has at most one connector (since we always steal * it away), so we won't call enable hooks twice. */

• drm_atomic_bridge_chain_pre_enable(encoder->bridge, old_state);
  

• bridge = drm_bridge_chain_get_first_bridge(encoder);
  

• drm_atomic_bridge_chain_pre_enable(bridge, old_state);
  
  

  if (funcs) { if (funcs->atomic_enable)

@@ -1346,7 +1355,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, funcs->commit(encoder); }

• drm_atomic_bridge_chain_enable(encoder->bridge, old_state);
  

• drm_atomic_bridge_chain_enable(bridge, old_state);
  

  }

  drm_atomic_helper_commit_writebacks(dev, old_state);

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index 80d88a55302e..4fe9e723e227 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -167,9 +167,10 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) */

if (encoder->bridge) {

• struct drm_bridge *bridge = encoder->bridge;
  

• struct drm_bridge *bridge;
  

  struct drm_bridge *next;

• bridge = drm_bridge_chain_get_first_bridge(encoder);
  

  while (bridge) { next = bridge->next; drm_bridge_detach(bridge);

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index c3ea722065c4..576b4b7dcd89 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -101,6 +101,7 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode,

/* Step 2: Validate against encoders and crtcs */ drm_connector_for_each_possible_encoder(connector, encoder) {

• struct drm_bridge *bridge;
  

  struct drm_crtc *crtc;

  ret = drm_encoder_mode_valid(encoder, mode);

@@ -112,7 +113,8 @@ drm_mode_validate_pipeline(struct drm_display_mode *mode, continue; }

• ret = drm_bridge_chain_mode_valid(encoder->bridge, mode);
  

• bridge = drm_bridge_chain_get_first_bridge(encoder);
  

• ret = drm_bridge_chain_mode_valid(bridge, mode);
  

  if (ret != MODE_OK) { /* There is also no point in continuing for crtc check * here. */

diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c b/drivers/gpu/drm/msm/edp/edp_bridge.c index 2950bba4aca9..b65b5cc2dba2 100644 --- a/drivers/gpu/drm/msm/edp/edp_bridge.c +++ b/drivers/gpu/drm/msm/edp/edp_bridge.c @@ -55,8 +55,14 @@ static void edp_bridge_mode_set(struct drm_bridge *bridge, DBG("set mode: " DRM_MODE_FMT, DRM_MODE_ARG(mode));

list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

• if ((connector->encoder != NULL) &&
  

• 	(connector->encoder->bridge == bridge)) {
  

• struct drm_encoder *encoder = connector->encoder;
  

• struct drm_bridge *first_bridge;
  

• if (!connector->encoder)
  

• 	continue;
  

• first_bridge = drm_bridge_chain_get_first_bridge(encoder);
  

• if (bridge == first_bridge) {
  msm_edp_ctrl_timing_cfg(edp->ctrl,
  	adjusted_mode, &connector->display_info);
  break;
  

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 2da46e3dc4ae..7a1f1e5f0326 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -14,6 +14,7 @@

#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> #include <drm/drm_fb_cma_helper.h> @@ -680,9 +681,10 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index]; const struct drm_display_mode *mode = &crtc->state->adjusted_mode;

• struct drm_bridge *bridge;
  
  

• rcar_lvds_clk_enable(encoder->base.bridge,
  

• 		     mode->clock * 1000);
  

• bridge = drm_bridge_chain_get_first_bridge(&encoder->base);
  

• rcar_lvds_clk_enable(bridge, mode->clock * 1000);
  

  }

  rcar_du_crtc_start(rcrtc);

@@ -702,12 +704,15 @@ static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc, rstate->outputs == BIT(RCAR_DU_OUTPUT_DPAD0)) { struct rcar_du_encoder *encoder = rcdu->encoders[RCAR_DU_OUTPUT_LVDS0 + rcrtc->index];

• struct drm_bridge *bridge;
  

• /*

  • Disable the LVDS clock output, see
  • rcar_du_crtc_atomic_enable().

  */

• rcar_lvds_clk_disable(encoder->base.bridge);
  

• bridge = drm_bridge_chain_get_first_bridge(&encoder->base);
  

• rcar_lvds_clk_disable(bridge);
  

  }

  spin_lock_irq(&crtc->dev->event_lock);

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 8aeba83fcf31..27eef63ce0ff 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -25,6 +25,7 @@

#include <linux/list.h> #include <linux/ctype.h> +#include <drm/drm_encoder.h> #include <drm/drm_mode_object.h> #include <drm/drm_modes.h>

@@ -422,6 +423,20 @@ drm_bridge_chain_get_next_bridge(struct drm_bridge *bridge) return bridge->next; }

+/**

• • drm_bridge_chain_get_first_bridge() - Get the first bridge in the chain
• • @encoder: encoder object
• • RETURNS:
• • the first bridge in the chain, or NULL if @encoder has no bridge attached
• • to it.
• */

+static inline struct drm_bridge * +drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder) +{

• return encoder->bridge;

+}

bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode);

Reviewed-by: Neil Armstrong narmstrong@baylibre.com

On Sun, 24 Nov 2019 12:39:26 +0200 Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

...

@@ -1346,7 +1355,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, funcs->commit(encoder); }

• drm_atomic_bridge_chain_enable(encoder->bridge, old_state);
  

• drm_atomic_bridge_chain_enable(bridge, old_state);
  

  }

  drm_atomic_helper_commit_writebacks(dev, old_state);

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index 80d88a55302e..4fe9e723e227 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -167,9 +167,10 @@ void drm_encoder_cleanup(struct drm_encoder *encoder) */

if (encoder->bridge) {

• struct drm_bridge *bridge = encoder->bridge;
  

• struct drm_bridge *bridge;
  

  struct drm_bridge *next;

• bridge = drm_bridge_chain_get_first_bridge(encoder);
  

  while (bridge) { next = bridge->next;

Shouldn't this have been addressed in the previous patch ?

It should and I will update patch 6 accordingly. This being said, it's not a big deal since the code is patched to use list_for_each_entry_safe() in patch 7 ;-).

On 23/10/2019 17:44, 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

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 +--- include/drm/drm_bridge.h | 12 ++- include/drm/drm_encoder.h | 9 +- 4 files changed, 135 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 54c874493c57..c5cf8a9c4237 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.
  */

@@ -114,6 +114,7 @@ EXPORT_SYMBOL(drm_bridge_remove); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous) {

• LIST_HEAD(tmp_list); int ret;

  if (!encoder || !bridge)

@@ -127,6 +128,7 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,

bridge->dev = encoder->dev; bridge->encoder = encoder;

• list_add_tail(&bridge->chain_node, &tmp_list);

  if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge);

@@ -138,9 +140,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, }

if (previous)

• previous->next = bridge;
  

• list_splice(&tmp_list, &previous->chain_node);
  

  else

• encoder->bridge = bridge;
  

• list_splice(&tmp_list, &encoder->bridge_chain);
  
  

  return 0;

} @@ -157,6 +159,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 +193,22 @@ 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 +231,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;
  

• 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 (bridge->funcs->mode_valid) ret = bridge->funcs->mode_valid(bridge, mode);

• if (ret != MODE_OK)
  

• 	return ret;
  

• }

• 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 +264,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 +294,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 +323,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 +349,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 +375,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 +403,23 @@ 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 +439,19 @@ 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 +471,23 @@ 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, &bridge->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 +506,19 @@ 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, &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 4fe9e723e227..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 = bridge->next;
  

• 	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/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 27eef63ce0ff..3ab16c95e59e 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_chain_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_chain_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..9b3dde177c81 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -172,7 +172,14 @@ struct drm_encoder { * &drm_connector_state.crtc. */ struct drm_crtc *crtc;

• struct drm_bridge *bridge;

• /**

• * @bridge_chain: Bridges attached to this encoder. The first entry of
  

• * this list is always &drm_encoder.bridge. It may be followed by other
  

• * bridge entities.
  

• */
  

• struct list_head bridge_chain;
• const struct drm_encoder_funcs *funcs; const struct drm_encoder_helper_funcs *helper_private;

};

Reviewed-by: Neil Armstrong narmstrong@baylibre.com

Hi Neil,

Sorry for the late reply.

On Tue, 5 Nov 2019 17:02:30 +0100 Neil Armstrong narmstrong@baylibre.com wrote:

Hi,

On 25/10/2019 15:29, Neil Armstrong wrote:

...

On 23/10/2019 17:44, 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

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 +--- include/drm/drm_bridge.h | 12 ++- include/drm/drm_encoder.h | 9 +- 4 files changed, 135 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 54c874493c57..c5cf8a9c4237 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c

[...]

...

...

@@ -426,15 +471,23 @@ 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, &bridge->encoder->bridge_chain,

• 		    chain_node) {  
  

This should use the encoder local variable in list_for_each_entry_reverse()

...

...

• 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 +506,19 @@ 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, &bridge->encoder->bridge_chain,

• 		 chain_node) {  
  

This should use encoder instead of bridge->encoder otherwise bridge will change and bridge->encoder->bridge_chain won't be valid during the for_each and cause the following :

Oops, indeed. I fixed the very same problem in another helper but somehow missed those 2. Thanks for testing/reporting the bug.

Boris

Hi Boris,

Thank you for the patch. On Wed, Oct 23, 2019 at 05:44:59PM +0200, Boris Brezillon wrote:

To iterate over all bridges attached to a specific encoder.

Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

---

Changes in v3:

• None

Changes in v2:

• New patch

---

include/drm/drm_bridge.h | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 3ab16c95e59e..238e84ab63a3 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -441,6 +441,17 @@ drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder) struct drm_bridge, chain_node); }

+/**

• • for_each_bridge_in_chain() - Iterate over all bridges present in a chain

The name doesn't match the macro below. Apart from that,

Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

• • @encoder: the encoder to iterate bridges on
• • @bridge: a bridge pointer updated to point to the current bridge at each

• •    iteration
    

• • Iterate over all bridges present in the bridge chain attached to @encoder.
• */

+#define drm_for_each_bridge_in_chain(encoder, bridge) \

• list_for_each_entry(bridge, &(encoder)->bridge_chain, chain_node)

bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode);

On 23/10/2019 17:45, Boris Brezillon wrote:

One of the last remaining objects to not have its atomic state.

This is being motivated by our attempt to support runtime bus-format negotiation between elements of the bridge chain. This patch just paves the road for such a feature by adding a new drm_bridge_state object inheriting from drm_private_obj so we can re-use some of the existing state initialization/tracking logic.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

Changes in v3:

• None

Changes in v2:

• Use drm_for_each_bridge_in_chain()
• Rename helpers to be more consistent with the rest of the DRM API
• Improve/fix the doc

---

drivers/gpu/drm/drm_atomic.c | 39 +++++++ drivers/gpu/drm/drm_atomic_helper.c | 20 ++++ drivers/gpu/drm/drm_bridge.c | 168 +++++++++++++++++++++++++++- include/drm/drm_atomic.h | 3 + include/drm/drm_bridge.h | 118 +++++++++++++++++++ 5 files changed, 343 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 419381abbdd1..6c249ce39380 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -30,6 +30,7 @@

#include <drm/drm_atomic.h> #include <drm/drm_atomic_uapi.h> +#include <drm/drm_bridge.h> #include <drm/drm_debugfs.h> #include <drm/drm_device.h> #include <drm/drm_drv.h> @@ -1010,6 +1011,44 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, connector->funcs->atomic_print_state(p, state); }

+/**

• • drm_atomic_add_encoder_bridges - add bridges attached to an encoder
• • @state: atomic state
• • @encoder: DRM encoder
• • This function adds all bridges attached to @encoder. This is needed to add
• • bridge states to @state and make them available when
• • &bridge_funcs.atomic_{check,pre_enable,enable,disable_post_disable}() are
• • called
• • Returns:
• • 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
• • then the w/w mutex code has detected a deadlock and the entire atomic
• • sequence must be restarted. All other errors are fatal.
• */

+int +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,

• 	       struct drm_encoder *encoder)
  

+{

• struct drm_bridge_state *bridge_state;
• struct drm_bridge *bridge;
• if (!encoder)

• return 0;
  

• DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n",

• 	 encoder->base.id, encoder->name, state);
  

• drm_for_each_bridge_in_chain(encoder, bridge) {

• bridge_state = drm_atomic_get_bridge_state(state, bridge);
  

• if (IS_ERR(bridge_state))
  

• 	return PTR_ERR(bridge_state);
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_atomic_add_encoder_bridges);

/**

• drm_atomic_add_affected_connectors - add connectors for crtc
• @state: atomic state

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index f02ddffd4960..de985ba7ce2d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -730,6 +730,26 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, return ret; }

• /*

• * Iterate over all connectors again, and add all affected bridges to
  

• * the state.
  

• */
  

• for_each_oldnew_connector_in_state(state, connector,

• 			   old_connector_state,
  

• 			   new_connector_state, i) {
  

• struct drm_encoder *encoder;
  

• encoder = old_connector_state->best_encoder;
  

• ret = drm_atomic_add_encoder_bridges(state, encoder);
  

• if (ret)
  

• 	return ret;
  

• encoder = new_connector_state->best_encoder;
  

• ret = drm_atomic_add_encoder_bridges(state, encoder);
  

• if (ret)
  

• 	return ret;
  

• }
• ret = mode_valid(state); if (ret) return ret;

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c5cf8a9c4237..c53966767782 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -25,6 +25,7 @@ #include <linux/module.h> #include <linux/mutex.h>

+#include <drm/drm_atomic_state_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_encoder.h>

@@ -89,6 +90,38 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove);

+static struct drm_private_state * +drm_bridge_atomic_duplicate_priv_state(struct drm_private_obj *obj) +{

• struct drm_bridge *bridge = drm_priv_to_bridge(obj);
• struct drm_bridge_state *state;
• if (bridge->funcs->atomic_duplicate_state)

• state = bridge->funcs->atomic_duplicate_state(bridge);
  

• else

• state = drm_atomic_helper_bridge_duplicate_state(bridge);
  

• return &state->base;

+}

+static void +drm_bridge_atomic_destroy_priv_state(struct drm_private_obj *obj,

• 		     struct drm_private_state *s)
  

+{

• struct drm_bridge_state *state = drm_priv_to_bridge_state(s);
• struct drm_bridge *bridge = drm_priv_to_bridge(obj);
• if (bridge->funcs->atomic_destroy_state)

• bridge->funcs->atomic_destroy_state(bridge, state);
  

• else

• drm_atomic_helper_bridge_destroy_state(bridge, state);
  

+}

+static const struct drm_private_state_funcs drm_bridge_priv_state_funcs = {

• .atomic_duplicate_state = drm_bridge_atomic_duplicate_priv_state,
• .atomic_destroy_state = drm_bridge_atomic_destroy_priv_state,

+};

/**

• drm_bridge_attach - attach the bridge to an encoder's chain

@@ -114,6 +147,7 @@ EXPORT_SYMBOL(drm_bridge_remove); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous) {

• struct drm_bridge_state *state; LIST_HEAD(tmp_list); int ret;

@@ -132,19 +166,39 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,

if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge);

• if (ret < 0) {
  

• 	bridge->dev = NULL;
  

• 	bridge->encoder = NULL;
  

• 	return ret;
  

• }
  

• if (ret < 0)
  

• 	goto err_reset_bridge;
  

  }

• if (bridge->funcs->atomic_reset)

• state = bridge->funcs->atomic_reset(bridge);
  

• else

• state = drm_atomic_helper_bridge_reset(bridge);
  

• if (IS_ERR(state)) {

• ret = PTR_ERR(state);
  

• goto err_detach_bridge;
  

• }

• drm_atomic_private_obj_init(bridge->dev, &bridge->base,

• 		    &state->base,
  

• 		    &drm_bridge_priv_state_funcs);
  

• if (previous) list_splice(&tmp_list, &previous->chain_node); else list_splice(&tmp_list, &encoder->bridge_chain);

  return 0;

+err_detach_bridge:

• if (bridge->funcs->detach)

• bridge->funcs->detach(bridge);
  

+err_reset_bridge:

• bridge->dev = NULL;
• bridge->encoder = NULL;
• return ret;

} EXPORT_SYMBOL(drm_bridge_attach);

@@ -156,6 +210,8 @@ void drm_bridge_detach(struct drm_bridge *bridge) if (WARN_ON(!bridge->dev)) return;

• drm_atomic_private_obj_fini(&bridge->base);
• if (bridge->funcs->detach) bridge->funcs->detach(bridge);

@@ -522,6 +578,108 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

+/**

• • drm_atomic_helper_bridge_destroy_state() - Default destroy state helper
• • @bridge: the bridge this state refers to
• • @state: state object to destroy
• • Just a simple kfree() for now.
• */

+void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,

• 			    struct drm_bridge_state *state)
  

+{

• kfree(state);

+} +EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);

+/**

• • __drm_atomic_helper_bridge_reset() - Initialize a bridge state to its

• • 			default
    

• • @bridge: the bridge this state is refers to
• • @state: bridge state to initialize
• • For now it's just a memset(0) plus a state->bridge assignment. Might
• • be extended in the future.
• */

+void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,

• 		      struct drm_bridge_state *state)
  

+{

• memset(state, 0, sizeof(*state));
• state->bridge = bridge;

+} +EXPORT_SYMBOL(__drm_atomic_helper_bridge_reset);

+/**

• • drm_atomic_helper_bridge_reset() - default &drm_plane_funcs.reset hook for

• • 		      bridges
    

• • @bridge: the bridge to reset state on
• • Resets the atomic state for @bridge by freeing the state pointer (which
• • might be NULL, e.g. at driver load time) and allocating a new empty state
• • object.
• • RETURNS:
• • A valid drm_bridge_state object in case of success, an ERR_PTR()
• • giving the reaon of the failure otherwise.
• */

+struct drm_bridge_state * +drm_atomic_helper_bridge_reset(struct drm_bridge *bridge) +{

• struct drm_bridge_state *bridge_state;
• bridge_state = kzalloc(sizeof(*bridge_state), GFP_KERNEL);
• if (!bridge_state)

• return ERR_PTR(-ENOMEM);
  

• __drm_atomic_helper_bridge_reset(bridge, bridge_state);
• return bridge_state;

+} +EXPORT_SYMBOL(drm_atomic_helper_bridge_reset);

+/**

• • __drm_atomic_helper_bridge_duplicate_state() - Copy the content of the

• • 				  current bridge state into a
    

• • 				  new one
    

• • @bridge: bridge element the old and new states are referring to
• • @new: new bridge state to copy to
• • Should be used by custom &drm_bridge_funcs.atomic_duplicate() implementation
• • to copy the previous state into the new object.
• */

+void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge,

• 				struct drm_bridge_state *new)
  

+{

• __drm_atomic_helper_private_obj_duplicate_state(&bridge->base,

• 					&new->base);
  

• new->bridge = bridge;

+} +EXPORT_SYMBOL(__drm_atomic_helper_bridge_duplicate_state);

+/**

• • drm_atomic_helper_duplicate_bridge_state() - Default duplicate state helper
• • @bridge: bridge containing the state to duplicate
• • Default implementation of &drm_bridge_funcs.atomic_duplicate().
• • RETURNS:
• • a valid state object or NULL if the allocation fails.
• */

+struct drm_bridge_state * +drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge) +{

• struct drm_bridge_state *new;
• if (WARN_ON(!bridge->base.state))

• return NULL;
  

• new = kzalloc(sizeof(*new), GFP_KERNEL);
• if (new)

• __drm_atomic_helper_bridge_duplicate_state(bridge, new);
  

• return new;

+} +EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);

#ifdef CONFIG_OF /**

• of_drm_find_bridge - find the bridge corresponding to the device node in

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 927e1205d7aa..1c0a08217712 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -660,6 +660,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, return plane->state; }

+int __must_check +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,

• 	       struct drm_encoder *encoder);
  

int __must_check drm_atomic_add_affected_connectors(struct drm_atomic_state *state, struct drm_crtc *crtc); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 238e84ab63a3..a608c47d1de5 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -25,6 +25,7 @@

#include <linux/list.h> #include <linux/ctype.h> +#include <drm/drm_atomic.h> #include <drm/drm_encoder.h> #include <drm/drm_mode_object.h> #include <drm/drm_modes.h> @@ -33,6 +34,23 @@ struct drm_bridge; struct drm_bridge_timings; struct drm_panel;

+/**

• • struct drm_bridge_state - Atomic bridge state object
• • @base: inherit from &drm_private_state
• • @bridge: the bridge this state refers to
• */

+struct drm_bridge_state {

• struct drm_private_state base;
• struct drm_bridge *bridge;

+};

+static inline struct drm_bridge_state * +drm_priv_to_bridge_state(struct drm_private_state *priv) +{

• return container_of(priv, struct drm_bridge_state, base);

+}

/**

• struct drm_bridge_funcs - drm_bridge control functions

*/ @@ -338,6 +356,48 @@ struct drm_bridge_funcs { */ void (*atomic_post_disable)(struct drm_bridge *bridge, struct drm_atomic_state *state);

• /**

• * @atomic_duplicate_state:
  

• *
  

• * Duplicate the current bridge state object (which is guaranteed to be
  

• * non-NULL).
  

• *
  

• * The atomic_duplicate_state() is optional, the core falls back on
  

• * &drm_atomic_helper_bridge_duplicate_state() when not implemented.
  

• *
  

• * RETURNS:
  

• * A valid drm_bridge_state object or NULL if the allocation fails.
  

• */
  

• struct drm_bridge_state *(*atomic_duplicate_state)(struct drm_bridge *bridge);
• /**

• * @atomic_destroy_state:
  

• *
  

• * Destroy a bridge state object previously allocated by
  

• * &drm_bridge_funcs.atomic_duplicate_state().
  

• *
  

• * The atomic_destroy_state hook is optional, the coref falls back on
  

• * &drm_atomic_helper_bridge_destroy_state() when not implemented.
  

• */
  

• void (*atomic_destroy_state)(struct drm_bridge *bridge,

• 		     struct drm_bridge_state *state);
  

• /**

• * @atomic_reset:
  

• *
  

• * Reset the bridge to a predefined state (or retrieve its current
  

• * state) and return a &drm_bridge_state object matching this state.
  

• * This function is called at attach time.
  

• *
  

• * The atomic_reset hook is optional, the core falls back on
  

• * &drm_atomic_helper_bridge_reset() when not implemented.
  

• *
  

• * RETURNS:
  

• * A valid drm_bridge_state object in case of success, an ERR_PTR()
  

• * giving the reaon of the failure otherwise.
  

• */
  

• struct drm_bridge_state *(*atomic_reset)(struct drm_bridge *bridge);

};

/** @@ -380,6 +440,8 @@ struct drm_bridge_timings {

• struct drm_bridge - central DRM bridge control structure

*/ struct drm_bridge {

• /** @base: inherit from &drm_private_object */
• struct drm_private_obj base; /** @dev: DRM device this bridge belongs to */ struct drm_device *dev; /** @encoder: encoder to which this bridge is connected */

@@ -404,6 +466,12 @@ struct drm_bridge { void *driver_private; };

+static inline struct drm_bridge * +drm_priv_to_bridge(struct drm_private_obj *priv) +{

• return container_of(priv, struct drm_bridge, base);

+}

void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); struct drm_bridge *of_drm_find_bridge(struct device_node *np); @@ -475,6 +543,56 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge, void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, struct drm_atomic_state *state);

+void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,

• 		      struct drm_bridge_state *state);
  

+struct drm_bridge_state * +drm_atomic_helper_bridge_reset(struct drm_bridge *bridge); +void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,

• 			    struct drm_bridge_state *state);
  

+void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge,

• 				struct drm_bridge_state *new);
  

+struct drm_bridge_state * +drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge);

+static inline struct drm_bridge_state * +drm_atomic_get_bridge_state(struct drm_atomic_state *state,

• 	    struct drm_bridge *bridge)
  

+{

• struct drm_private_state *obj_state;
• obj_state = drm_atomic_get_private_obj_state(state, &bridge->base);
• if (!obj_state)

• return NULL;
  

• return drm_priv_to_bridge_state(obj_state);

+}

+static inline struct drm_bridge_state * +drm_atomic_get_old_bridge_state(struct drm_atomic_state *state,

• 		struct drm_bridge *bridge)
  

+{

• struct drm_private_state *obj_state;
• obj_state = drm_atomic_get_old_private_obj_state(state, &bridge->base);
• if (!obj_state)

• return NULL;
  

• return drm_priv_to_bridge_state(obj_state);

+}

+static inline struct drm_bridge_state * +drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,

• 		struct drm_bridge *bridge)
  

+{

• struct drm_private_state *obj_state;
• obj_state = drm_atomic_get_new_private_obj_state(state, &bridge->base);
• if (!obj_state)

• return NULL;
  

• return drm_priv_to_bridge_state(obj_state);

+}

#ifdef CONFIG_DRM_PANEL_BRIDGE struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel); struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,

Reviewed-by: Neil Armstrong narmstrong@baylibre.com