Hi Boris.

Good to see that you kept the alphabetical order in the include files. One nit below. With this fixed: Reviewed-by: Sam Ravnborg sam@ravnborg.org

Sam

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h index 982865866a29..c681a9e22484 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.h +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h @@ -7,6 +7,8 @@ #ifndef __HDMI_CONNECTOR_H__ #define __HDMI_CONNECTOR_H__

+#include <drm/drm_bridge.h>

#include <linux/i2c.h> #include <linux/clk.h> #include <linux/platform_device.h>

Please follow the same order of include blocks as we see in other files:

#include <linux/*.h>

#include <drm/*.h>

#include "*.h"

And with an empty line between the blocks, and sorted withint the blocks.

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote:

The VC4 and Exynos DSI encoder drivers need a custom enable/disable sequence and manually set encoder->bridge to NULL to prevent the core from calling the bridge hooks.

Let's provide a more official way to support custom bridge/encoder enable/disable sequences.

So, while I'm not opposed to this, I think it's a bit of a hack, and I'd like to share my vision of how I'd like to see this being implemented in the more distant future (and thus possibly on top of this change).

It has been established for quite some time now that exposing drm_encoder to userspace was likely a mistake, and that it should have stayed a kernel-only object. With the generalised usage of drm_bridge, I would go one step further: drm_encoder shouldn't matter for DRM/KMS drivers at all.

drm_bridge has been introduced to model chained encoders, where the second (and subsequent) encoders couldn't easily be supported in a standard and reusable way with just drm_encoder. A bridge (with the exception of the panel bridge) is just an encoder. It shouldn't matter whether encoders are internal to the display controller, separate IPs in the SoC or external components, they could all be modelled as bridges. We do have bridges for DSI encoder IPs, and DSI (and other) bridges potentially need similar custom enable/disable sequences. I would thus ideally like to see the VC4 and Exynos DSI encoders being implemented as bridges, with support for custom enable/disable sequences in bridges, and drop support for custom enable/disable sequences in drm_encoder.

Does this make sense to you ? While I would love to see this being implemented right away, it may be too much work as a prerequisite for this bus format negotiation series, so I don't want to reject this patch. I would however like to already make sure we agree on the way forward for the next steps.

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

drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++--- drivers/gpu/drm/drm_crtc_helper.c | 38 ++++++++++++++++++----------- include/drm/drm_encoder.h | 9 +++++++ 3 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4706439fb490..15ea61877122 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1030,7 +1030,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_disable(encoder->bridge, old_state);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_disable(encoder->bridge, old_state);
  
  

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

@@ -1044,7 +1045,9 @@ 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);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_post_disable(encoder->bridge,
  

• 				       old_state);
  

  }

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

@@ -1342,7 +1345,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_pre_enable(encoder->bridge, old_state);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
  
  

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

@@ -1353,7 +1357,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, funcs->commit(encoder); }

• drm_atomic_bridge_enable(encoder->bridge, old_state);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_enable(encoder->bridge, old_state);
  

  }

  drm_atomic_helper_commit_writebacks(dev, old_state);

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index fa3694836c22..87dae37fec12 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -160,14 +160,16 @@ drm_encoder_disable(struct drm_encoder *encoder) if (!encoder_funcs) return;

• drm_bridge_disable(encoder->bridge);

• if (!encoder->custom_bridge_enable_disable_seq)

• drm_bridge_disable(encoder->bridge);
  
  

  if (encoder_funcs->disable) (*encoder_funcs->disable)(encoder); else if (encoder_funcs->dpms) (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);

• drm_bridge_post_disable(encoder->bridge);

• if (!encoder->custom_bridge_enable_disable_seq)

• drm_bridge_post_disable(encoder->bridge);
  

}

static void __drm_helper_disable_unused_functions(struct drm_device *dev) @@ -365,13 +367,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!encoder_funcs) continue;

• drm_bridge_disable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_disable(encoder->bridge);
  
  

  /* Disable the encoders as the first thing we do. */ if (encoder_funcs->prepare) encoder_funcs->prepare(encoder);

• drm_bridge_post_disable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_post_disable(encoder->bridge);
  

  }

  drm_crtc_prepare_encoders(dev);

@@ -414,12 +418,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!encoder_funcs) continue;

• drm_bridge_pre_enable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_pre_enable(encoder->bridge);
  
  

  if (encoder_funcs->commit) encoder_funcs->commit(encoder);

• drm_bridge_enable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_enable(encoder->bridge);
  

  }

  /* Calculate and store various constants which

@@ -825,18 +831,22 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) if (!encoder_funcs) return;

• if (mode == DRM_MODE_DPMS_ON)

• drm_bridge_pre_enable(bridge);
  

• else

• drm_bridge_disable(bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq) {

• if (mode == DRM_MODE_DPMS_ON)
  

• 	drm_bridge_pre_enable(bridge);
  

• else
  

• 	drm_bridge_disable(bridge);
  

• }

  if (encoder_funcs->dpms) encoder_funcs->dpms(encoder, mode);

• if (mode == DRM_MODE_DPMS_ON)

• drm_bridge_enable(bridge);
  

• else

• drm_bridge_post_disable(bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq) {

• if (mode == DRM_MODE_DPMS_ON)
  

• 	drm_bridge_enable(bridge);
  

• else
  

• 	drm_bridge_post_disable(bridge);
  

• }

}

static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 70cfca03d812..d986ff1197ce 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -175,6 +175,15 @@ struct drm_encoder { struct drm_bridge *bridge; const struct drm_encoder_funcs *funcs; const struct drm_encoder_helper_funcs *helper_private;

• /**

• * @custom_bridge_enable_disable_seq: Set to true if the encoder needs
  

• * a custom bridge/encoder enable/disable sequence. In that case the
  

• * driver is responsible for calling the
  

• * drm[_atomic]_bridge_{pre_enable,enable,disable,post_disable}()
  

• * functions as part of its encoder enable/disable handling.
  

• */
  

• uint32_t custom_bridge_enable_disable_seq : 1;

};

#define obj_to_encoder(x) container_of(x, struct drm_encoder, base)

Hi Daniel,

On Wed, Aug 21, 2019 at 10:21:07AM +0200, Daniel Vetter wrote:

On Tue, Aug 20, 2019 at 10:05:05PM +0300, Laurent Pinchart wrote:

...

On Thu, Aug 08, 2019 at 05:11:33PM +0200, Boris Brezillon wrote:

...

The VC4 and Exynos DSI encoder drivers need a custom enable/disable sequence and manually set encoder->bridge to NULL to prevent the core from calling the bridge hooks.

Let's provide a more official way to support custom bridge/encoder enable/disable sequences.

So, while I'm not opposed to this, I think it's a bit of a hack, and I'd like to share my vision of how I'd like to see this being implemented in the more distant future (and thus possibly on top of this change).

I think it's also counter to how the atomic helpers are meant to be used. I mean you're adding a pile new hooks here all for essentially not having to type a few lines of helper code. If you look around at big drivers (i915, amdgpu, nouveau) _none_ of them use the modesets_enables/disables helpers. Exactly for reasons like this where you need a custom sequence.

So proper recommendation is to just type your own, you'll be happier.

...

It has been established for quite some time now that exposing drm_encoder to userspace was likely a mistake, and that it should have stayed a kernel-only object. With the generalised usage of drm_bridge, I would go one step further: drm_encoder shouldn't matter for DRM/KMS drivers at all.

drm_bridge has been introduced to model chained encoders, where the second (and subsequent) encoders couldn't easily be supported in a standard and reusable way with just drm_encoder. A bridge (with the exception of the panel bridge) is just an encoder. It shouldn't matter whether encoders are internal to the display controller, separate IPs in the SoC or external components, they could all be modelled as bridges. We do have bridges for DSI encoder IPs, and DSI (and other) bridges potentially need similar custom enable/disable sequences. I would thus ideally like to see the VC4 and Exynos DSI encoders being implemented as bridges, with support for custom enable/disable sequences in bridges, and drop support for custom enable/disable sequences in drm_encoder.

Does this make sense to you ? While I would love to see this being implemented right away, it may be too much work as a prerequisite for this bus format negotiation series, so I don't want to reject this patch. I would however like to already make sure we agree on the way forward for the next steps.

Yeah the other bit is that bridges are supposed to be some kind of standard. I do wonder (I haven't looked at the series) whether the problem here is that encoders don't have their own full set of pre/post enable hooks like bridges (essentially that's what you're adding here), or whether the idea behind pre/post enable/disable hooks is not good enough.

It's a mostly-good idea :-) DSI is a bit more troublesome as it often requires enable/disable sequences that are a bit more entangled between the source and the sink. That's why the omapdrm driver originally controlled enabling from sink to source, it would locate the bridge (or the omapdrm internal equivalent of bridge to be more accurate) at the end of the chain, and enable it. The bridge would then be responsible for forwarding the enable call to its source, broken in several fine-grained calls for DSI. That allowed a DSI sink to implement completely custom enable/disable sequences, and enable the source accordingly (usually by first enabling clocks, then enabling data transmission but without actual display data, and finally enabling transfer of display data). The cost was a more complex enable/disable implementation for each bridge.

I think the pre/post enable/disable operations we have now are mostly good, but for some links they need to be manually controlled. Of course I can't rule out a new display bus in the future that will shatter this down to pieces by requiring more complex enable/disable sequences.

Anyway, this here is imo not the right thing to do. If setting encoder->bridge to NULL doesn't work for you then just type your own enable/disable implementations. There's explicitly no requirement to use all parts of atomic helpers, au contraire, it's explicitly discouraged.

...

...

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

drivers/gpu/drm/drm_atomic_helper.c | 13 +++++++--- drivers/gpu/drm/drm_crtc_helper.c | 38 ++++++++++++++++++----------- include/drm/drm_encoder.h | 9 +++++++ 3 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4706439fb490..15ea61877122 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1030,7 +1030,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_disable(encoder->bridge, old_state);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_disable(encoder->bridge, old_state);
  
  

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

@@ -1044,7 +1045,9 @@ 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);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_post_disable(encoder->bridge,
  

• 				       old_state);
  

  }

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

@@ -1342,7 +1345,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_pre_enable(encoder->bridge, old_state);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
  
  

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

@@ -1353,7 +1357,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, funcs->commit(encoder); }

• drm_atomic_bridge_enable(encoder->bridge, old_state);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_atomic_bridge_enable(encoder->bridge, old_state);
  

  }

  drm_atomic_helper_commit_writebacks(dev, old_state);

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index fa3694836c22..87dae37fec12 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -160,14 +160,16 @@ drm_encoder_disable(struct drm_encoder *encoder) if (!encoder_funcs) return;

• drm_bridge_disable(encoder->bridge);

• if (!encoder->custom_bridge_enable_disable_seq)

• drm_bridge_disable(encoder->bridge);
  
  

  if (encoder_funcs->disable) (*encoder_funcs->disable)(encoder); else if (encoder_funcs->dpms) (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);

• drm_bridge_post_disable(encoder->bridge);

• if (!encoder->custom_bridge_enable_disable_seq)

• drm_bridge_post_disable(encoder->bridge);
  

}

static void __drm_helper_disable_unused_functions(struct drm_device *dev) @@ -365,13 +367,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!encoder_funcs) continue;

• drm_bridge_disable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_disable(encoder->bridge);
  
  

  /* Disable the encoders as the first thing we do. */ if (encoder_funcs->prepare) encoder_funcs->prepare(encoder);

• drm_bridge_post_disable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_post_disable(encoder->bridge);
  

  }

  drm_crtc_prepare_encoders(dev);

@@ -414,12 +418,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!encoder_funcs) continue;

• drm_bridge_pre_enable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_pre_enable(encoder->bridge);
  
  

  if (encoder_funcs->commit) encoder_funcs->commit(encoder);

• drm_bridge_enable(encoder->bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq)
  

• 	drm_bridge_enable(encoder->bridge);
  

  }

  /* Calculate and store various constants which

@@ -825,18 +831,22 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) if (!encoder_funcs) return;

• if (mode == DRM_MODE_DPMS_ON)

• drm_bridge_pre_enable(bridge);
  

• else

• drm_bridge_disable(bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq) {

• if (mode == DRM_MODE_DPMS_ON)
  

• 	drm_bridge_pre_enable(bridge);
  

• else
  

• 	drm_bridge_disable(bridge);
  

• }

  if (encoder_funcs->dpms) encoder_funcs->dpms(encoder, mode);

• if (mode == DRM_MODE_DPMS_ON)

• drm_bridge_enable(bridge);
  

• else

• drm_bridge_post_disable(bridge);
  

• if (!encoder->custom_bridge_enable_disable_seq) {

• if (mode == DRM_MODE_DPMS_ON)
  

• 	drm_bridge_enable(bridge);
  

• else
  

• 	drm_bridge_post_disable(bridge);
  

• }

}

static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 70cfca03d812..d986ff1197ce 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -175,6 +175,15 @@ struct drm_encoder { struct drm_bridge *bridge; const struct drm_encoder_funcs *funcs; const struct drm_encoder_helper_funcs *helper_private;

• /**

• * @custom_bridge_enable_disable_seq: Set to true if the encoder needs
  

• * a custom bridge/encoder enable/disable sequence. In that case the
  

• * driver is responsible for calling the
  

• * drm[_atomic]_bridge_{pre_enable,enable,disable,post_disable}()
  

• * functions as part of its encoder enable/disable handling.
  

• */
  

• uint32_t custom_bridge_enable_disable_seq : 1;

};

#define obj_to_encoder(x) container_of(x, struct drm_encoder, base)

Hi Boris,

On Thu, Aug 08, 2019 at 05:11:36PM +0200, Boris Brezillon wrote:

bridge->next is only points to the new bridge if drm_bridge_attach() succeeds. No need to reset it manually here.

Note that this change is part of the attempt to make the bridge chain a double-linked list. In order to do that we must patch all drivers manipulating the bridge->next field.

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

This looks good to me as a cleanup, and I think you can already push it without waiting for the whole series to be ready.

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

---

drivers/gpu/drm/exynos/exynos_dp.c | 1 - 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c index 1e6aa24bf45e..4785885c0f4f 100644 --- a/drivers/gpu/drm/exynos/exynos_dp.c +++ b/drivers/gpu/drm/exynos/exynos_dp.c @@ -110,7 +110,6 @@ static int exynos_dp_bridge_attach(struct analogix_dp_plat_data *plat_data, if (ret) { DRM_DEV_ERROR(dp->dev, "Failed to attach bridge to drm\n");

• 	bridge->next = NULL;
  return ret;
  

  } }

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:37PM +0200, Boris Brezillon wrote:

DRM bridges should not be operated independently. Let's rename the drm_bridge_xxx() helpers that take the first bridge of the chain and iterate over the whole chain into drm_bridge_chain_xx(). We also pass it the encoder containing the bridge chain instead of the dereferencing encoder->bridge.

I'm not sure about this. I do agree that the helpers operate on a chain, but I think they're use for calling them on any bridge, especially in conjunction with your work. The way I see it is that when a bridge in the chain needs a custom enable/disable sequence (flagged by some kind of flag in the bridge structure), the helpers will not automatically propagate the calls down the chain, and let the bridge call the pre/post enable/disable operations on the downstream bridge. This means that those bridges with special needs will have to call the helpers on the next bridge down the chain, and thus require keeping the ability to do so.

We could of course propose two sets of helpers, one taking a bridge pointer, and another one taking an encoder pointer, but I think it's a bit overkill. Especially if you consider my comments earlier in this series where I propose moving the custom sequence feature to bridges instead of encoders, I don't think this patch will be needed.

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

drivers/gpu/drm/drm_atomic_helper.c | 19 +- drivers/gpu/drm/drm_bridge.c | 365 +++++++++++++----------- drivers/gpu/drm/drm_crtc_helper.c | 27 +- drivers/gpu/drm/drm_probe_helper.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +- drivers/gpu/drm/vc4/vc4_dsi.c | 8 +- include/drm/drm_bridge.h | 64 +++-- 8 files changed, 273 insertions(+), 224 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 15ea61877122..0deb54099570 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -445,8 +445,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,
  

• 			&new_crtc_state->mode,
  

• 			&new_crtc_state->adjusted_mode);
  

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

@@ -511,7 +512,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, mode); if (ret != MODE_OK) { DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n"); return ret;

@@ -1031,7 +1032,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) * it away), so we won't call disable hooks twice. */ if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_atomic_bridge_disable(encoder->bridge, old_state);
  

• 	drm_atomic_bridge_chain_disable(encoder, old_state);
  
  

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

@@ -1046,8 +1047,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) }

if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_atomic_bridge_post_disable(encoder->bridge,
  

• 				       old_state);
  

• 	drm_atomic_bridge_chain_post_disable(encoder,
  

• 					     old_state);
  

  }

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

@@ -1228,7 +1229,7 @@ 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, mode, adjusted_mode);
  

  }

}

@@ -1346,7 +1347,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, * it away), so we won't call enable hooks twice. */ if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_atomic_bridge_pre_enable(encoder->bridge, old_state);
  

• 	drm_atomic_bridge_chain_pre_enable(encoder, old_state);
  
  

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

@@ -1358,7 +1359,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, }

if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_atomic_bridge_enable(encoder->bridge, old_state);
  

• 	drm_atomic_bridge_chain_enable(encoder, 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..2bf9a19e11bf 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -171,24 +171,9 @@ void drm_bridge_detach(struct drm_bridge *bridge)

• For detailed specification of the bridge callbacks see &drm_bridge_funcs.

*/

-/**

• • drm_bridge_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
• • Calls &drm_bridge_funcs.mode_fixup 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
• • 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)
  

+static bool drm_bridge_mode_fixup(struct drm_bridge *bridge,

• 		  const struct drm_display_mode *mode,
  

• 		  struct drm_display_mode *adjusted_mode)
  

{ bool ret = true;

@@ -202,25 +187,10 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,

return ret; } -EXPORT_SYMBOL(drm_bridge_mode_fixup);

-/**

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

• • 	   encoder chain.
    

• • @bridge: bridge control structure
• • @mode: desired mode to be validated
• • Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
• • chain, starting from the first bridge to the last. If at least one bridge
• • does not accept the mode the function returns the error code.
• • Note: the bridge passed should be the one closest to the encoder.
• • 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)
  

+static enum drm_mode_status +drm_bridge_mode_valid(struct drm_bridge *bridge,

•       const struct drm_display_mode *mode)
  

{ enum drm_mode_status ret = MODE_OK;

@@ -235,19 +205,8 @@ enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,

return drm_bridge_mode_valid(bridge->next, mode); } -EXPORT_SYMBOL(drm_bridge_mode_valid);

-/**

• • drm_bridge_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
• • chain, starting from the last bridge to the first. These are called before
• • calling the encoder's prepare op.
• • Note: the bridge passed should be the one closest to the encoder
• */

-void drm_bridge_disable(struct drm_bridge *bridge) +static void drm_bridge_disable(struct drm_bridge *bridge) { if (!bridge) return; @@ -257,19 +216,8 @@ void drm_bridge_disable(struct drm_bridge *bridge) if (bridge->funcs->disable) bridge->funcs->disable(bridge); } -EXPORT_SYMBOL(drm_bridge_disable);

-/**

• • drm_bridge_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
• • encoder chain, starting from the first bridge to the last. These are called
• • after completing the encoder's prepare op.
• • Note: the bridge passed should be the one closest to the encoder
• */

-void drm_bridge_post_disable(struct drm_bridge *bridge) +static void drm_bridge_post_disable(struct drm_bridge *bridge) { if (!bridge) return; @@ -279,23 +227,10 @@ void drm_bridge_post_disable(struct drm_bridge *bridge)

drm_bridge_post_disable(bridge->next); } -EXPORT_SYMBOL(drm_bridge_post_disable);

-/**

• • drm_bridge_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
• • 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)
  

+static void drm_bridge_mode_set(struct drm_bridge *bridge,

• 		const struct drm_display_mode *mode,
  

• 		const struct drm_display_mode *adjusted_mode)
  

{ if (!bridge) return; @@ -305,20 +240,8 @@ void drm_bridge_mode_set(struct drm_bridge *bridge,

drm_bridge_mode_set(bridge->next, mode, adjusted_mode); } -EXPORT_SYMBOL(drm_bridge_mode_set);

-/**

• • drm_bridge_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
• • chain, starting from the last bridge to the first. These are called
• • before calling the encoder's commit op.
• • Note: the bridge passed should be the one closest to the encoder
• */

-void drm_bridge_pre_enable(struct drm_bridge *bridge) +static void drm_bridge_pre_enable(struct drm_bridge *bridge) { if (!bridge) return; @@ -328,19 +251,8 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge) if (bridge->funcs->pre_enable) bridge->funcs->pre_enable(bridge); } -EXPORT_SYMBOL(drm_bridge_pre_enable);

-/**

• • drm_bridge_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
• • chain, starting from the first bridge to the last. These are called
• • after completing the encoder's commit op.
• • Note that the bridge passed should be the one closest to the encoder
• */

-void drm_bridge_enable(struct drm_bridge *bridge) +static void drm_bridge_enable(struct drm_bridge *bridge) { if (!bridge) return; @@ -350,22 +262,127 @@ void drm_bridge_enable(struct drm_bridge *bridge)

drm_bridge_enable(bridge->next); } -EXPORT_SYMBOL(drm_bridge_enable);

/**

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

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

• • 		 encoder chain
    

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

• • Calls &drm_bridge_funcs.atomic_disable (falls back on
• • &drm_bridge_funcs.disable) op for all the bridges in the encoder chain,
• • starting from the last bridge to the first. These are called before calling
• • &drm_encoder_helper_funcs.atomic_disable

• • Calls &drm_bridge_funcs.mode_fixup 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

• • RETURNS:
• • true on success, false on failure
  */

-void drm_atomic_bridge_disable(struct drm_bridge *bridge,

• 	       struct drm_atomic_state *state)
  

+bool drm_bridge_chain_mode_fixup(struct drm_encoder *encoder,

• 		 const struct drm_display_mode *mode,
  

• 		 struct drm_display_mode *adjusted_mode)
  

+{

• return drm_bridge_mode_fixup(encoder->bridge, mode, adjusted_mode);

+} +EXPORT_SYMBOL(drm_bridge_chain_mode_fixup);

+/**

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

• • 		 encoder chain.
    

• • @encoder: encoder object
• • @mode: desired mode to be validated
• • Calls &drm_bridge_funcs.mode_valid for all the bridges in the encoder
• • chain, starting from the first bridge to the last. If at least one bridge
• • does not accept the mode the function returns the error code.
• • RETURNS:
• • MODE_OK on success, drm_mode_status Enum error code on failure
• */

+enum drm_mode_status +drm_bridge_chain_mode_valid(struct drm_encoder *encoder,

• 	    const struct drm_display_mode *mode)
  

+{

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

+} +EXPORT_SYMBOL(drm_bridge_chain_mode_valid);

+/**

• • drm_bridge_chain_disable - disables all bridges in the encoder chain
• • @encoder: encoder object
• • Calls &drm_bridge_funcs.disable op for all the bridges in the encoder
• • chain, starting from the last bridge to the first. These are called before
• • calling the encoder's prepare op.
• */

+void drm_bridge_chain_disable(struct drm_encoder *encoder) +{

• drm_bridge_disable(encoder->bridge);

+} +EXPORT_SYMBOL(drm_bridge_chain_disable);

+/**

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

• • 		   encoder chain
    

• • @encoder: encoder object
• • Calls &drm_bridge_funcs.post_disable op for all the bridges in the
• • encoder chain, starting from the first bridge to the last. These are called
• • after completing the encoder's prepare op.
• */

+void drm_bridge_chain_post_disable(struct drm_encoder *encoder) +{

• drm_bridge_post_disable(encoder->bridge);

+} +EXPORT_SYMBOL(drm_bridge_chain_post_disable);

+/**

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

• • 	       encoder chain
    

• • @encoder: encoder object
• • @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.
• */

+void drm_bridge_chain_mode_set(struct drm_encoder *encoder,

• 	       const struct drm_display_mode *mode,
  

• 	       const struct drm_display_mode *adjusted_mode)
  

+{

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

+} +EXPORT_SYMBOL(drm_bridge_chain_mode_set);

+/**

• • drm_bridge_chain_pre_enable - prepares for enabling all bridges in the

• • 		 encoder chain
    

• • @encoder: encoder object
• • Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
• • chain, starting from the last bridge to the first. These are called
• • before calling the encoder's commit op.
• */

+void drm_bridge_chain_pre_enable(struct drm_encoder *encoder) +{

• drm_bridge_pre_enable(encoder->bridge);

+} +EXPORT_SYMBOL(drm_bridge_chain_pre_enable);

+/**

• • drm_bridge_chain_enable - enables all bridges in the encoder chain
• • @encoder: encoder object
• • Calls &drm_bridge_funcs.enable op for all the bridges in the encoder
• • chain, starting from the first bridge to the last. These are called
• • after completing the encoder's commit op.
• */

+void drm_bridge_chain_enable(struct drm_encoder *encoder) +{

• drm_bridge_enable(encoder->bridge);

+} +EXPORT_SYMBOL(drm_bridge_chain_enable);

+static void drm_atomic_bridge_disable(struct drm_bridge *bridge,

• 		      struct drm_atomic_state *state)
  

{ if (!bridge) return; @@ -377,23 +394,9 @@ void drm_atomic_bridge_disable(struct drm_bridge *bridge, else if (bridge->funcs->disable) bridge->funcs->disable(bridge); } -EXPORT_SYMBOL(drm_atomic_bridge_disable);

-/**

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

• • 		    encoder chain
    

• • @bridge: bridge control structure
• • @state: atomic state being committed
• • Calls &drm_bridge_funcs.atomic_post_disable (falls back on
• • &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
• • starting from the first bridge to the last. These are called after completing
• • &drm_encoder_helper_funcs.atomic_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)
  

+static void drm_atomic_bridge_post_disable(struct drm_bridge *bridge,

• 			   struct drm_atomic_state *state)
  

{ if (!bridge) return; @@ -405,23 +408,9 @@ void drm_atomic_bridge_post_disable(struct drm_bridge *bridge,

drm_atomic_bridge_post_disable(bridge->next, state); } -EXPORT_SYMBOL(drm_atomic_bridge_post_disable);

-/**

• • drm_atomic_bridge_pre_enable - prepares for enabling all bridges in the

• • 		  encoder chain
    

• • @bridge: bridge control structure
• • @state: atomic state being committed
• • Calls &drm_bridge_funcs.atomic_pre_enable (falls back on
• • &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
• • starting from the last bridge to the first. These are called before calling
• • &drm_encoder_helper_funcs.atomic_enable
• • 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)
  

+static void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge,

• 			 struct drm_atomic_state *state)
  

{ if (!bridge) return; @@ -433,22 +422,9 @@ void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge, else if (bridge->funcs->pre_enable) bridge->funcs->pre_enable(bridge); } -EXPORT_SYMBOL(drm_atomic_bridge_pre_enable);

-/**

• • drm_atomic_bridge_enable - enables all bridges in the encoder chain
• • @bridge: bridge control structure
• • @state: atomic state being committed
• • Calls &drm_bridge_funcs.atomic_enable (falls back on
• • &drm_bridge_funcs.enable) op for all the bridges in the encoder chain,
• • starting from the first bridge to the last. These are called after completing
• • &drm_encoder_helper_funcs.atomic_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)
  

+static void drm_atomic_bridge_enable(struct drm_bridge *bridge,

• 		     struct drm_atomic_state *state)
  

{ if (!bridge) return; @@ -460,7 +436,76 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,

drm_atomic_bridge_enable(bridge->next, state); } -EXPORT_SYMBOL(drm_atomic_bridge_enable);

+/**

• • drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
• • @encoder: encoder object
• • @state: atomic state being committed
• • Calls &drm_bridge_funcs.atomic_disable (falls back on
• • &drm_bridge_funcs.disable) op for all the bridges in the encoder chain,
• • starting from the last bridge to the first. These are called before calling
• • &drm_encoder_helper_funcs.atomic_disable
• */

+void drm_atomic_bridge_chain_disable(struct drm_encoder *encoder,

• 		     struct drm_atomic_state *state)
  

+{

• drm_atomic_bridge_disable(encoder->bridge, state);

+} +EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);

+/**

• • drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges

• • 			  in the encoder chain
    

• • @encoder: encoder object
• • @state: atomic state being committed
• • Calls &drm_bridge_funcs.atomic_post_disable (falls back on
• • &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
• • starting from the first bridge to the last. These are called after completing
• • &drm_encoder_helper_funcs.atomic_disable
• */

+void drm_atomic_bridge_chain_post_disable(struct drm_encoder *encoder,

• 			  struct drm_atomic_state *state)
  

+{

• drm_atomic_bridge_post_disable(encoder->bridge, state);

+} +EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);

+/**

• • drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in

• • 			the encoder chain
    

• • @encoder: encoder object
• • @state: atomic state being committed
• • Calls &drm_bridge_funcs.atomic_pre_enable (falls back on
• • &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
• • starting from the last bridge to the first. These are called before calling
• • &drm_encoder_helper_funcs.atomic_enable
• */

+void drm_atomic_bridge_chain_pre_enable(struct drm_encoder *encoder,

• 			struct drm_atomic_state *state)
  

+{

• drm_atomic_bridge_pre_enable(encoder->bridge, state);

+} +EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);

+/**

• • drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
• • @encoder: encoder object
• • @state: atomic state being committed
• • Calls &drm_bridge_funcs.atomic_enable (falls back on
• • &drm_bridge_funcs.enable) op for all the bridges in the encoder chain,
• • starting from the first bridge to the last. These are called after completing
• • &drm_encoder_helper_funcs.atomic_enable
• */

+void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder,

• 		    struct drm_atomic_state *state)
  

+{

• drm_atomic_bridge_enable(encoder->bridge, state);

+} +EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

#ifdef CONFIG_OF /** diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 87dae37fec12..f8a361d396df 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -161,7 +161,7 @@ drm_encoder_disable(struct drm_encoder *encoder) return;

if (!encoder->custom_bridge_enable_disable_seq)

• drm_bridge_disable(encoder->bridge);
  

• drm_bridge_chain_disable(encoder);
  
  

  if (encoder_funcs->disable) (*encoder_funcs->disable)(encoder);

@@ -169,7 +169,7 @@ drm_encoder_disable(struct drm_encoder *encoder) (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);

if (!encoder->custom_bridge_enable_disable_seq)

• drm_bridge_post_disable(encoder->bridge);
  

• drm_bridge_chain_post_disable(encoder);
  

}

static void __drm_helper_disable_unused_functions(struct drm_device *dev) @@ -329,8 +329,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (!encoder_funcs) continue;

• ret = drm_bridge_mode_fixup(encoder->bridge,
  

• 	mode, adjusted_mode);
  

• ret = drm_bridge_chain_mode_fixup(encoder, mode,
  

• 				  adjusted_mode);
  

  if (!ret) { DRM_DEBUG_KMS("Bridge fixup failed\n"); goto done;

@@ -368,14 +368,14 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, continue;

if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_bridge_disable(encoder->bridge);
  

• 	drm_bridge_chain_disable(encoder);
  
  

  /* Disable the encoders as the first thing we do. */ if (encoder_funcs->prepare) encoder_funcs->prepare(encoder);

  if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_bridge_post_disable(encoder->bridge);
  

• 	drm_bridge_chain_post_disable(encoder);
  

  }

  drm_crtc_prepare_encoders(dev);

@@ -403,7 +403,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (encoder_funcs->mode_set) encoder_funcs->mode_set(encoder, mode, adjusted_mode);

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

• drm_bridge_chain_mode_set(encoder, mode, adjusted_mode);
  

  }

  /* Now enable the clocks, plane, pipe, and connectors that we set up. */

@@ -419,13 +419,13 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, continue;

if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_bridge_pre_enable(encoder->bridge);
  

• 	drm_bridge_chain_pre_enable(encoder);
  
  

  if (encoder_funcs->commit) encoder_funcs->commit(encoder);

  if (!encoder->custom_bridge_enable_disable_seq)

• 	drm_bridge_enable(encoder->bridge);
  

• 	drm_bridge_chain_enable(encoder);
  

  }

  /* Calculate and store various constants which

@@ -824,7 +824,6 @@ static int drm_helper_choose_encoder_dpms(struct drm_encoder *encoder) /* Helper which handles bridge ordering around encoder dpms */ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) {

• struct drm_bridge *bridge = encoder->bridge; const struct drm_encoder_helper_funcs *encoder_funcs;

  encoder_funcs = encoder->helper_private;

@@ -833,9 +832,9 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)

if (!encoder->custom_bridge_enable_disable_seq) { if (mode == DRM_MODE_DPMS_ON)

• 	drm_bridge_pre_enable(bridge);
  

• 	drm_bridge_chain_pre_enable(encoder);
  

  else

• 	drm_bridge_disable(bridge);
  

• 	drm_bridge_chain_disable(encoder);
  

  }

  if (encoder_funcs->dpms)

@@ -843,9 +842,9 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)

if (!encoder->custom_bridge_enable_disable_seq) { if (mode == DRM_MODE_DPMS_ON)

• 	drm_bridge_enable(bridge);
  

• 	drm_bridge_chain_enable(encoder);
  

  else

• 	drm_bridge_post_disable(bridge);
  

• 	drm_bridge_chain_post_disable(encoder);
  

  }

}

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 351cbc40f0f8..461199bc28c0 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -113,7 +113,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, mode);
  

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

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 8593c87bdf96..9a96bc48b81f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1389,7 +1389,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) if (ret < 0) goto err_put_sync; } else {

• drm_bridge_pre_enable(encoder->bridge);
  

• drm_bridge_chain_pre_enable(encoder);
  

  }

  exynos_dsi_set_display_mode(dsi);

@@ -1400,7 +1400,7 @@ static void exynos_dsi_enable(struct drm_encoder *encoder) if (ret < 0) goto err_display_disable; } else {

• drm_bridge_enable(encoder->bridge);
  

• drm_bridge_chain_enable(encoder);
  

  }

  dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;

@@ -1425,10 +1425,10 @@ static void exynos_dsi_disable(struct drm_encoder *encoder) dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;

drm_panel_disable(dsi->panel);

• drm_bridge_disable(encoder->bridge);

• drm_bridge_chain_disable(encoder); exynos_dsi_set_display_enable(dsi, false); drm_panel_unprepare(dsi->panel);

• drm_bridge_post_disable(encoder->bridge);

• drm_bridge_chain_post_disable(encoder); dsi->state &= ~DSIM_STATE_ENABLED; pm_runtime_put_sync(dsi->dev);

} diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index c79b1f855d89..6ec3d2584539 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.encoder, mode,
  

• 				 &adjusted_mode))
  return MODE_BAD;
  

  }

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index b670ca473786..d7cd720c4efa 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -752,9 +752,9 @@ static void vc4_dsi_encoder_disable(struct drm_encoder *encoder) struct vc4_dsi *dsi = vc4_encoder->dsi; struct device *dev = &dsi->pdev->dev;

• drm_bridge_disable(encoder->bridge);

• drm_bridge_chain_disable(encoder); vc4_dsi_ulps(dsi, true);

• drm_bridge_post_disable(encoder->bridge);

• drm_bridge_chain_post_disable(encoder);

  clk_disable_unprepare(dsi->pll_phy_clock); clk_disable_unprepare(dsi->escape_clock);

@@ -1054,7 +1054,7 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)

vc4_dsi_ulps(dsi, false);

• drm_bridge_pre_enable(encoder->bridge);

• drm_bridge_chain_pre_enable(encoder);

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

@@ -1071,7 +1071,7 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder) DSI_DISP0_ENABLE); }

• drm_bridge_enable(encoder->bridge);

• drm_bridge_chain_enable(encoder);

  if (debug_dump_regs) { struct drm_printer p = drm_info_printer(&dsi->pdev->dev);

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 7616f6562fe4..3057e2153f62 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 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 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 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_encoder *encoder,

• 		 const struct drm_display_mode *mode,
  

• 		 struct drm_display_mode *adjusted_mode);
  

+enum drm_mode_status +drm_bridge_chain_mode_valid(struct drm_encoder *encoder,

• 	    const struct drm_display_mode *mode);
  

+void drm_bridge_chain_disable(struct drm_encoder *encoder); +void drm_bridge_chain_post_disable(struct drm_encoder *encoder); +void drm_bridge_chain_mode_set(struct drm_encoder *encoder,

• 	       const struct drm_display_mode *mode,
  

• 	       const struct drm_display_mode *adjusted_mode);
  

+void drm_bridge_chain_pre_enable(struct drm_encoder *encoder); +void drm_bridge_chain_enable(struct drm_encoder *encoder);

-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_encoder *encoder,

• 		     struct drm_atomic_state *state);
  

+void drm_atomic_bridge_chain_post_disable(struct drm_encoder *encoder,

• 			  struct drm_atomic_state *state);
  

+void drm_atomic_bridge_chain_pre_enable(struct drm_encoder *encoder,

• 			struct drm_atomic_state *state);
  

+void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, 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, -- 2.21.0

On Mon, Aug 19, 2019 at 07:19:39PM +0200, Sam Ravnborg wrote:

On Thu, Aug 08, 2019 at 05:11:38PM +0200, Boris Brezillon wrote:

...

This is part of our attempt to make the bridge chain a double-linked list based on the generic list helpers. In order to do that, we must patch all drivers manipulating the encoder->bridge field directly.

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

Reviewed-by: Sam Ravnborg sam@ravnborg.org

Sean, this patch looks like a nice cleanup we can apply outside the series. It would be good that drivers do not poke direct in the encoder data that this patch fixes.

Agreed, and

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

...

---

drivers/gpu/drm/msm/edp/edp.c | 4 +++- drivers/gpu/drm/msm/hdmi/hdmi.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/edp/edp.c b/drivers/gpu/drm/msm/edp/edp.c index 0f312ac5b624..ad4e963ccd9b 100644 --- a/drivers/gpu/drm/msm/edp/edp.c +++ b/drivers/gpu/drm/msm/edp/edp.c @@ -178,7 +178,9 @@ int msm_edp_modeset_init(struct msm_edp *edp, struct drm_device *dev, goto fail; }

• encoder->bridge = edp->bridge;

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

• if (ret)

• goto fail;
  
  

  priv->bridges[priv->num_bridges++] = edp->bridge; priv->connectors[priv->num_connectors++] = edp->connector;

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index 0e4217be3f00..55b9a8c8312b 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -327,7 +327,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi, goto fail; }

• encoder->bridge = hdmi->bridge;

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

• if (ret)

• goto fail;
  
  

  priv->bridges[priv->num_bridges++] = hdmi->bridge; priv->connectors[priv->num_connectors++] = hdmi->connector;

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:41PM +0200, Boris Brezillon wrote:

One of the last remaining object 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 state initialization/tracking logic.

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

drivers/gpu/drm/drm_atomic.c | 40 ++++++++ drivers/gpu/drm/drm_atomic_helper.c | 18 ++++ drivers/gpu/drm/drm_bridge.c | 141 +++++++++++++++++++++++++++- include/drm/drm_atomic.h | 3 + include/drm/drm_bridge.h | 99 +++++++++++++++++++ 5 files changed, 296 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 419381abbdd1..ed6fd3e31b56 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,45 @@ 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);
  

• for (bridge = drm_bridge_chain_get_first_bridge(encoder); bridge;

•     bridge = drm_bridge_chain_get_next_bridge(bridge)) {
  

Would a for_each_bridge() macro make sense ? I'd implement it on top of list_for_each_entry() to make it simple.

• 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 0deb54099570..e76ec9648b6f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -736,6 +736,24 @@ 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 ed2410f49581..0280da6be1e6 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,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_duplicate_bridge_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_destroy_bridge_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 +146,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 +165,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_duplicate_state)

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

• else

• state = drm_atomic_helper_duplicate_bridge_state(bridge);
  

• if (!state) {

• ret = -ENOMEM;
  

• 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 +209,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);

@@ -495,6 +550,82 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

+/**

• • drm_atomic_helper_init_bridge_state() - Initializes a bridge state

s/Initializes/Initialize/

• • @bridge this state is referring to

Did you mean

* @bridge: the bridge this state is referring 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_init_bridge_state(struct drm_bridge *bridge,

• 			 struct drm_bridge_state *state)
  

+{

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

+} +EXPORT_SYMBOL(drm_atomic_helper_init_bridge_state);

Other objects don't export a state init function helper, but state reset and state duplicate helpers. Is there a reason to depart from that model ? Same for the copy helper below.

+/**

• • drm_atomic_helper_copy_bridge_state() - Copy the content of a bridge state
• • @bridge: bridge the old and new state are referring to
• • @old: previous bridge state to copy from
• • @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_copy_bridge_state(struct drm_bridge *bridge,

• 			 const struct drm_bridge_state *old,
  

• 			 struct drm_bridge_state *new)
  

+{

• *new = *old;

+} +EXPORT_SYMBOL(drm_atomic_helper_copy_bridge_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_duplicate_bridge_state(struct drm_bridge *bridge) +{

• struct drm_bridge_state *old = NULL, *new;
• new = kzalloc(sizeof(*new), GFP_KERNEL);
• if (!new)

• return NULL;
  

• if (bridge->base.state) {

• old = drm_priv_to_bridge_state(bridge->base.state);
  

• drm_atomic_helper_copy_bridge_state(bridge, old, new);
  

• } else {

• drm_atomic_helper_init_bridge_state(bridge, new);
  

• }
• return new;

+} +EXPORT_SYMBOL(drm_atomic_helper_duplicate_bridge_state);

+/**

• • drm_atomic_helper_destroy_bridge_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_destroy_bridge_state(struct drm_bridge *bridge,

• 			    struct drm_bridge_state *state)
  

+{

• kfree(state);

+} +EXPORT_SYMBOL(drm_atomic_helper_destroy_bridge_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..5ee25fd51e80 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 f40181274ed7..8ca25d266d0c 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_mode_object.h> #include <drm/drm_modes.h>

@@ -32,6 +33,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

*/ @@ -337,6 +355,30 @@ 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.
  

• *
  

• * Note that this function can be called when current bridge state is
  

• * NULL. In this case, implementations should either implement HW
  

• * readback or initialize the state with sensible default values.
  

Related to the question above, shouldn't we have a reset state operation instead ?

• *
  

• * 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_duplicate_state:
  

• *
  

• * Destroy a bridge state object previously allocated by
  

• * &drm_bridge_funcs.atomic_duplicate_state() or
  

• * &drm_bridge_funcs.atomic_get_initial_state().
  

• */
  

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

• 		     struct drm_bridge_state *state);
  

};

/** @@ -379,6 +421,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 */

@@ -403,6 +447,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); @@ -438,6 +488,55 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_encoder *encoder, void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, struct drm_atomic_state *state);

+void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge,

• 			 struct drm_bridge_state *state);
  

+void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge,

• 			 const struct drm_bridge_state *old,
  

• 			 struct drm_bridge_state *new);
  

+struct drm_bridge_state * +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge); +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge,

• 			    struct drm_bridge_state *state);
  

+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, u32 connector_type);

Hi Boris,

On Thu, Aug 22, 2019 at 11:00:56AM +0200, Boris Brezillon wrote:

On Wed, 21 Aug 2019 23:14:56 +0300 Laurent Pinchart wrote:

...

...

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

• for (bridge = drm_bridge_chain_get_first_bridge(encoder); bridge;

•     bridge = drm_bridge_chain_get_next_bridge(bridge)) {  
  

Would a for_each_bridge() macro make sense ? I'd implement it on top of list_for_each_entry() to make it simple.

I can add this helper. Note that for_each_bridge() will iterate over the whole bridge chain, and we might need a for_each_bridge_from() at some point if we want to iterate over a sub-chain.

...

...

• 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_helper_init_bridge_state() - Initializes a bridge state

s/Initializes/Initialize/

...

• • @bridge this state is referring to

Did you mean

• @bridge: the bridge this state is referring to ?

Yes.

...

...

• • @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_init_bridge_state(struct drm_bridge *bridge,

• 			 struct drm_bridge_state *state)
  

+{

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

+} +EXPORT_SYMBOL(drm_atomic_helper_init_bridge_state);

Other objects don't export a state init function helper, but state reset and state duplicate helpers. Is there a reason to depart from that model ? Same for the copy helper below.

I thought those names better reflect what the helpers do. __drm_atomic_helper_crtc_duplicate_state() for instance, it actually does not duplicate the state, but just copies current state info into the new state object which has been allocated by the caller. Same for the reset helpers, they actually do not reset anything but just initialize the state to a default. I also try to avoid prefixing functions with __ when I can (names can be clarified/extended to avoid conflicts most of the time). Will switch back to reset/dup names if you prefer.

I think consistency would help. Regarding the reset operation, unless I'm mistaken, it got its name from the fact that it should create a software state that reflects the current hardware state, in order to support fastboot (taking over a running display controller started by the boot loader without causing any glitch). That being said, I'm not sure reset was the best name even for that :-)

...

...

+/**

• • drm_atomic_helper_copy_bridge_state() - Copy the content of a bridge state
• • @bridge: bridge the old and new state are referring to
• • @old: previous bridge state to copy from
• • @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_copy_bridge_state(struct drm_bridge *bridge,

• 			 const struct drm_bridge_state *old,
  

• 			 struct drm_bridge_state *new)
  

+{

• *new = *old;

+} +EXPORT_SYMBOL(drm_atomic_helper_copy_bridge_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_duplicate_bridge_state(struct drm_bridge *bridge) +{

• struct drm_bridge_state *old = NULL, *new;
• new = kzalloc(sizeof(*new), GFP_KERNEL);
• if (!new)

• return NULL;
  

• if (bridge->base.state) {

• old = drm_priv_to_bridge_state(bridge->base.state);
  

• drm_atomic_helper_copy_bridge_state(bridge, old, new);
  

• } else {

• drm_atomic_helper_init_bridge_state(bridge, new);
  

• }
• return new;

+} +EXPORT_SYMBOL(drm_atomic_helper_duplicate_bridge_state);

+/**

• • drm_atomic_helper_destroy_bridge_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_destroy_bridge_state(struct drm_bridge *bridge,

• 			    struct drm_bridge_state *state)
  

+{

• kfree(state);

+} +EXPORT_SYMBOL(drm_atomic_helper_destroy_bridge_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..5ee25fd51e80 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 f40181274ed7..8ca25d266d0c 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_mode_object.h> #include <drm/drm_modes.h>

@@ -32,6 +33,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

*/ @@ -337,6 +355,30 @@ 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.
  

• *
  

• * Note that this function can be called when current bridge state is
  

• * NULL. In this case, implementations should either implement HW
  

• * readback or initialize the state with sensible default values.  
  

Related to the question above, shouldn't we have a reset state operation instead ?

I had a version with a reset hook and a helper that was falling back to ->duplicate_state() when not specified, but I decided to drop it to simplify things (I think most drivers will just use the default helper, and those that actually want to implement HW readback can easily do it from their ->duplicate_state() hook by testing bridge->state value). I can re-introduce it if you like.

It would help with consistency, but I'll leave that to you.

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:42PM +0200, Boris Brezillon wrote:

Instead of a drm_atomic_state. The only driver implementing those hooks is patched too.

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

.../drm/bridge/analogix/analogix_dp_core.c | 12 ++-- drivers/gpu/drm/drm_bridge.c | 57 +++++++++++++++---- include/drm/drm_bridge.h | 8 +-- 3 files changed, 57 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index b096a7a75c14..02b9ee0e4f7a 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1290,8 +1290,9 @@ struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp, }

static void analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,

• 				 struct drm_atomic_state *state)
  

• 				 struct drm_bridge_state *bstate)
  

{

• struct drm_atomic_state *state = bstate->base.state; struct analogix_dp_device *dp = bridge->driver_private; struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state;

@@ -1367,8 +1368,9 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp) }

static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,

• 			     struct drm_atomic_state *state)
  

• 			     struct drm_bridge_state *bstate)
  

{

• struct drm_atomic_state *state = bstate->base.state; struct analogix_dp_device *dp = bridge->driver_private; struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state;

@@ -1441,8 +1443,9 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge) }

static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,

• 			      struct drm_atomic_state *state)
  

• 			      struct drm_bridge_state *bstate)
  

{

• struct drm_atomic_state *state = bstate->base.state; struct analogix_dp_device *dp = bridge->driver_private; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state = NULL;

@@ -1465,8 +1468,9 @@ static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,

static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,

• 			    struct drm_atomic_state *state)
  

• 			    struct drm_bridge_state *bstate)
  

{

• struct drm_atomic_state *state = bstate->base.state; struct analogix_dp_device *dp = bridge->driver_private; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state;

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 0280da6be1e6..0528ca941855 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -467,10 +467,18 @@ void drm_atomic_bridge_chain_disable(struct drm_encoder *encoder,

list_for_each_entry_reverse(bridge, &encoder->bridge_chain, chain_node) {

• if (bridge->funcs->atomic_disable)
  

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

• else if (bridge->funcs->disable)
  

• if (bridge->funcs->atomic_disable) {
  

• 	struct drm_bridge_state *bridge_state;
  

• 	bridge_state = drm_atomic_get_old_bridge_state(state,
  

• 						       bridge);
  

• 	if (WARN_ON(!bridge_state))
  

• 		return;
  

• 	bridge->funcs->atomic_disable(bridge, bridge_state);
  

• } else if (bridge->funcs->disable) {
  bridge->funcs->disable(bridge);
  

• }
  

  }

} EXPORT_SYMBOL(drm_atomic_bridge_chain_disable); @@ -492,10 +500,19 @@ void drm_atomic_bridge_chain_post_disable(struct drm_encoder *encoder, struct drm_bridge *bridge;

list_for_each_entry(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)
  

• if (bridge->funcs->atomic_post_disable) {
  

• 	struct drm_bridge_state *bridge_state;
  

• 	bridge_state = drm_atomic_get_old_bridge_state(state,
  

• 						       bridge);
  

• 	if (WARN_ON(!bridge_state))
  

• 		return;
  

• 	bridge->funcs->atomic_post_disable(bridge,
  

• 					   bridge_state);
  

• } else if (bridge->funcs->post_disable) {
  bridge->funcs->post_disable(bridge);
  

• }
  

  }

} EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable); @@ -518,10 +535,18 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_encoder *encoder,

list_for_each_entry_reverse(bridge, &encoder->bridge_chain, chain_node) {

• if (bridge->funcs->atomic_pre_enable)
  

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

• else if (bridge->funcs->pre_enable)
  

• if (bridge->funcs->atomic_pre_enable) {
  

• 	struct drm_bridge_state *bridge_state;
  

• 	bridge_state = drm_atomic_get_new_bridge_state(state,
  

• 						       bridge);
  

Shouldn't you get the old state here ? The new state in commit-related operations is supposed to be obtained from the object itself, and the old state is passed to the function. See how the CRTC and encoder .atomic_enable() are called in drm_atomic_helper_commit_modeset_enables (drm_atomic_helper.c) for instance.

You should update the documentation of the bridge atomic operations to makes this explicit. The documentation of the bridge helpers (drm_atomic_bridge_enable() & co.) is also misleading, they get passed the old state, while the documentation states "atomic state being committed". I think you should rename all those state parameters to old_state to make this clearer.

Last but not least, the implementation in the analogix bridge driver seems to expect the new state, so I think it's buggy.

• 	if (WARN_ON(!bridge_state))
  

• 		return;
  

• 	bridge->funcs->atomic_pre_enable(bridge, bridge_state);
  

• } else if (bridge->funcs->pre_enable) {
  bridge->funcs->pre_enable(bridge);
  

• }
  

  }

} EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable); @@ -542,10 +567,18 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, struct drm_bridge *bridge;

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

• if (bridge->funcs->atomic_enable)
  

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

• else if (bridge->funcs->enable)
  

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

• 	struct drm_bridge_state *bridge_state;
  

• 	bridge_state = drm_atomic_get_new_bridge_state(state,
  

• 						       bridge);
  

• 	if (WARN_ON(!bridge_state))
  

• 		return;
  

• 	bridge->funcs->atomic_enable(bridge, bridge_state);
  

• } else if (bridge->funcs->enable) {
  bridge->funcs->enable(bridge);
  

• }
  

  }

} EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 8ca25d266d0c..9383b4e4b853 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -280,7 +280,7 @@ struct drm_bridge_funcs { * The @atomic_pre_enable callback is optional. */ void (*atomic_pre_enable)(struct drm_bridge *bridge,

• 		  struct drm_atomic_state *state);
  

• 		  struct drm_bridge_state *state);
  
  

  /**

  • @atomic_enable:

@@ -305,7 +305,7 @@ struct drm_bridge_funcs { * The enable callback is optional. */ void (*atomic_enable)(struct drm_bridge *bridge,

• 	      struct drm_atomic_state *state);
  

• 	      struct drm_bridge_state *state);
  

  /**
  • @atomic_disable:

@@ -328,7 +328,7 @@ struct drm_bridge_funcs { * The disable callback is optional. */ void (*atomic_disable)(struct drm_bridge *bridge,

• 	       struct drm_atomic_state *state);
  

• 	       struct drm_bridge_state *state);
  
  

  /**

  • @atomic_post_disable:

@@ -354,7 +354,7 @@ struct drm_bridge_funcs { * The post_disable callback is optional. */ void (*atomic_post_disable)(struct drm_bridge *bridge,

• 		    struct drm_atomic_state *state);
  

• 		    struct drm_bridge_state *state);
  
  

  /**

  • @atomic_duplicate_state:

On Thu, 22 Aug 2019 03:12:07 +0300 Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:43PM +0200, Boris Brezillon wrote:

...

So that bridge drivers have a way to check/reject an atomic operation. The drm_atomic_bridge_chain_check() (which is just a wrapper around the ->atomic_check() hook) is called in place of drm_bridge_chain_mode_fixup() (when ->atomic_check() is not implemented, the core falls back to ->mode_fixup(), so the behavior should stay the same for existing bridge drivers).

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

drivers/gpu/drm/drm_atomic_helper.c | 11 +++--- drivers/gpu/drm/drm_bridge.c | 59 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 23 +++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e76ec9648b6f..3fadde38ead7 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -445,12 +445,11 @@ mode_fixup(struct drm_atomic_state *state) encoder = new_conn_state->best_encoder; funcs = encoder->helper_private;

• ret = drm_bridge_chain_mode_fixup(encoder,
  

• 			&new_crtc_state->mode,
  

• 			&new_crtc_state->adjusted_mode);
  

• if (!ret) {
  

• 	DRM_DEBUG_ATOMIC("Bridge fixup failed\n");
  

• 	return -EINVAL;
  

• ret = drm_atomic_bridge_chain_check(encoder, new_crtc_state,
  

• 				    new_conn_state);
  

• if (ret) {
  

• 	DRM_DEBUG_ATOMIC("Bridge atomic check failed\n");
  

• 	return ret;
  

  }

  if (funcs && funcs->atomic_check) {

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 0528ca941855..dcad661daa74 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -583,6 +583,65 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

+static int drm_atomic_bridge_check(struct drm_bridge *bridge,

• 		   struct drm_crtc_state *crtc_state,
  

• 		   struct drm_connector_state *conn_state)
  

+{

• if (bridge->funcs->atomic_check) {

• struct drm_bridge_state *bridge_state;
  

• int ret;
  

• bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
  

• 					       bridge);
  

• if (WARN_ON(!bridge_state))
  

• 	return -EINVAL;
  

• ret = bridge->funcs->atomic_check(bridge, bridge_state,
  

• 				  crtc_state, conn_state);
  

• if (ret)
  

• 	return ret;
  

• } else if (bridge->funcs->mode_fixup) {

• if (!bridge->funcs->mode_fixup(bridge, &crtc_state->mode,
  

• 			       &crtc_state->adjusted_mode))
  

• 	return -EINVAL;
  

• }
• return 0;

+}

+/**

• • drm_atomic_bridge_chain_check() - Do an atomic check on the bridge chain
• • @encoder: encoder object
• • @crtc_state: new CRTC state
• • @conn_state: new connector state
• • Calls &drm_bridge_funcs.atomic_check() (falls back on
• • &drm_bridge_funcs.mode_fixup()) op for all the bridges in the encoder chain,
• • starting from the last bridge to the first. These are called before calling
• • &drm_encoder_helper_funcs.atomic_check()
• • RETURNS:
• • 0 on success, a negative error code on failure
• */

+int drm_atomic_bridge_chain_check(struct drm_encoder *encoder,

• 		  struct drm_crtc_state *crtc_state,
  

• 		  struct drm_connector_state *conn_state)
  

+{

• struct drm_bridge *bridge;
• list_for_each_entry_reverse(bridge, &encoder->bridge_chain,

• 		    chain_node) {
  

• int ret;
  

• ret = drm_atomic_bridge_check(bridge, crtc_state, conn_state);
  

• if (ret)
  

• 	return ret;
  

• }
• return 0;

+} +EXPORT_SYMBOL(drm_atomic_bridge_chain_check);

/**

• drm_atomic_helper_init_bridge_state() - Initializes a bridge state
• @bridge this state is referring to

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 9383b4e4b853..5d8fe3709bde 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -379,6 +379,26 @@ struct drm_bridge_funcs { */ void (*atomic_destroy_state)(struct drm_bridge *bridge, struct drm_bridge_state *state);

• /**

• * @atomic_check:
  

• *
  

• * This method is responsible for checking bridge state correctness.
  

• * It can also check the state of the surrounding components in chain
  

• * to make sure the whole pipeline can work properly.  
  

As this is meant to replace .mode_fixup() you should document that here and in the .mode_fixup() operation.

There's also the additional question of what parameters can be modified. Is a bridge .atomic_check() allowed to modify the CRTC or connector state ? Of so, what are the restrictions on how those can be modified ? For instance .mode_fixup() can change the pixel clock but isn't allowed to modify the h/v active values. Those restrictions were not clearly documented for .mode_fixup() and have led to grey areas. I don't want to repeat the same mistake for bridges, we need to be very explicit here.

Any recommendation? I didn't face this problem myself, so I'd need inputs from those who did to take a decision.

In the list of open questions, what if a bridge changes the pixel clock to a value it supports (as .mode_fixup() does), and then the previous bridge in the chain (the next one in traversal order for the drm_atomic_bridge_chain_check() function) also changes the pixel clock, to a value that the current bridge doesn't support ? I think we really need to think about the semantic of this operation.

Don't we have the exact same problem right now with the ->mode_fixup() hook when bridges are chained? Not sure why it suddenly becomes a priority to clarify that, but if we quickly settle on this new semantic, I'm fine integrating those changes in my patchset.

As the goal of your series is to implement configuration of bus formats between bridges, shouldn't a bridge .atomic_check() also receive the state of the next bridge in the chain ?

I thought we had enough arguments passed to the atomic_check() hook and decided to let the driver retrieve the next state. I can pass the next bridge state if you prefer.

...

• *
  

• * &drm_bridge_funcs.atomic_check() hooks are called in reverse
  

• * order (from the last to the first bridge).
  

• *
  

• * This method is optional.
  

• *
  

• * RETURNS:
  

• * zero if the check passed, a negative error code otherwise.
  

• */
  

• int (*atomic_check)(struct drm_bridge *bridge,

• 	    struct drm_bridge_state *bridge_state,
  

• 	    struct drm_crtc_state *crtc_state,
  

• 	    struct drm_connector_state *conn_state);
  

};

/** @@ -479,6 +499,9 @@ void drm_bridge_chain_mode_set(struct drm_encoder *encoder, void drm_bridge_chain_pre_enable(struct drm_encoder *encoder); void drm_bridge_chain_enable(struct drm_encoder *encoder);

+int drm_atomic_bridge_chain_check(struct drm_encoder *encoder,

• 		  struct drm_crtc_state *crtc_state,
  

• 		  struct drm_connector_state *conn_state);
  

void drm_atomic_bridge_chain_disable(struct drm_encoder *encoder, struct drm_atomic_state *state); void drm_atomic_bridge_chain_post_disable(struct drm_encoder *encoder,

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:44PM +0200, Boris Brezillon wrote:

Will be useful for bridge drivers that want to do bus format negotiation with their neighbours.

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

drivers/gpu/drm/drm_bridge.c | 19 +++++++++++++++++++ include/drm/drm_bridge.h | 2 ++ 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index dcad661daa74..9efb27087e70 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -271,6 +271,25 @@ drm_bridge_chain_get_next_bridge(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_chain_get_next_bridge);

+/**

• • drm_bridge_chain_get_prev_bridge() - Get the previous bridge in the chain
• • @bridge: bridge object
• • RETURNS:
• • the previous bridge in the chain, or NULL if there's @bridge is the
• • last.

Did you mean "if the @bridge is the first" ?

• */

+struct drm_bridge * +drm_bridge_chain_get_prev_bridge(struct drm_bridge *bridge) +{

• if (!bridge || !bridge->encoder ||

Do we really need to protect against !bridge and !bridge->encoder, can that happen ? Especially !bridge, are there use cases for potentially calling this function (and the other list traversal helpers in your previous patches) with a NULL bridge ?

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

•            return NULL;
  

• return list_prev_entry(bridge, chain_node);

+} +EXPORT_SYMBOL(drm_bridge_chain_get_prev_bridge);

/**

• DOC: bridge callbacks

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 5d8fe3709bde..2f69adb7b0f3 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -485,6 +485,8 @@ struct drm_bridge * drm_bridge_chain_get_last_bridge(struct drm_encoder *encoder); struct drm_bridge * drm_bridge_chain_get_next_bridge(struct drm_bridge *bridge); +struct drm_bridge * +drm_bridge_chain_get_prev_bridge(struct drm_bridge *bridge); bool drm_bridge_chain_mode_fixup(struct drm_encoder *encoder, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode);

On Wed, 14 Aug 2019 09:51:07 +0200 Neil Armstrong narmstrong@baylibre.com wrote:

On 08/08/2019 17:11, Boris Brezillon wrote:

...

This takes the form of various helpers, plus the addition of 2 new structs:

• drm_bus_caps: describe the bus capabilities of a bridge/encoder. For bridges we have one for the input port and one for the output port. Encoders just have one output port.
• drm_bus_cfg: added to the drm_bridge_state. It's supposed to store bus configuration information. For now we just have format and flags but more fields might be added in the future. Again, each drm_bridge_state contains 2 drm_bus_cfg elements: one for the output port and one for the input port

Helpers can be used from bridge drivers' ->atomic_check() implementation to negotiate the bus format to use. Negotiation happens in reserve order, starting from the last element of the bridge chain (the one directly connected to the display) to the first element of the chain (the one connected to the encoder).

Negotiation usually takes place in 2 steps:

• make sure the input end of the next element in the chain picked a format that's supported by the output end of our bridge. That's done with drm_atomic_bridge_choose_output_bus_cfg().
• choose a format for the input end of our bridge based on what's supported by the previous bridge in the chain. This is achieved by calling drm_atomic_bridge_choose_input_bus_cfg()

Note that those helpers are a bit lax when it comes to unitialized bus format validation. That's intentional. If we don't do that we'll basically break things if we start adding support for bus format negotiation to some elements of the pipeline leaving others on the side, and that's likely to happen for all external bridges since we can't necessarily tell where they are used.

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

drivers/gpu/drm/drm_bridge.c | 187 +++++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 46 +++++++++ include/drm/drm_encoder.h | 6 ++ 3 files changed, 239 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 9efb27087e70..ef072df42422 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -602,6 +602,193 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

+int drm_find_best_bus_format(const struct drm_bus_caps *a,

• 	     const struct drm_bus_caps *b,
  

• 	     const struct drm_display_mode *mode,
  

• 	     u32 *selected_bus_fmt)
  

+{

• unsigned int i, j;
• /*

• * Some drm_bridge/drm_encoder don't care about the input/output bus
  

• * format, let's set the format to zero in that case (this is only
  

• * valid if both side of the link don't care).
  

• */
  

• if (!a->num_supported_fmts && !b->num_supported_fmts) {

• *selected_bus_fmt = 0;
  

• return 0;
  

• } else if (b->num_supported_fmts > 1 && b->supported_fmts) {

• *selected_bus_fmt = b->supported_fmts[0];
  

• return 0;  
  

Here, `!a->num_supported_fmts &&` is missing otherwise this code will select b->supported_fmts[0] whatever the supported formats of a.

...

• } else if (a->num_supported_fmts > 1 && a->supported_fmts) {

• *selected_bus_fmt = a->supported_fmts[0];
  

• return 0;  
  

Here, `!b->num_supported_fmts &&` is missing otherwise this code will select a->supported_fmts[0] whatever the supported formats of b.

...

• } else if (!a->num_supported_fmts || !a->supported_fmts ||

•    !b->num_supported_fmts || !b->supported_fmts) {
  

• return -EINVAL;
  

• }
• /*

• * TODO:
  

• * Dummy algo picking the first match. We probably want something
  

• * smarter where the narrowest format (in term of bus width) that
  

• * does not incur data loss is picked, and if all possible formats
  

• * are lossy, pick the one that's less lossy among all the choices
  

• * we have. In order to do that we'd need to convert MEDIA_BUS_FMT_
  

• * modes into something like drm_format_info.
  

• */
  

• for (i = 0; i < a->num_supported_fmts; i++) {

• for (j = 0; j < b->num_supported_fmts; j++) {
  

• 	if (a->supported_fmts[i] == b->supported_fmts[j]) {
  

• 		*selected_bus_fmt = a->supported_fmts[i];
  

• 		return 0;
  

• 	}
  

• }
  

• }
• return -EINVAL;

+} +EXPORT_SYMBOL(drm_find_best_bus_format);

[...]

With that fixed, this works with basic negociation of the single MEDIA_BUS_FMT_YUV8_1X24 exposed by the meson hdmi encoder, otherwise it takes the first default MEDIA_BUS_FMT_RGB888_1X24 format exposed by dw-hdmi.

Thanks for the report. Will be fixed in v2.

Boris

On Thu, 22 Aug 2019 03:55:56 +0300 Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:

Hi Boris,

Thank you for the patch.

On Thu, Aug 08, 2019 at 05:11:45PM +0200, Boris Brezillon wrote:

...

This takes the form of various helpers, plus the addition of 2 new structs:

• drm_bus_caps: describe the bus capabilities of a bridge/encoder. For bridges we have one for the input port and one for the output port. Encoders just have one output port.
• drm_bus_cfg: added to the drm_bridge_state. It's supposed to store bus configuration information. For now we just have format and flags but more fields might be added in the future. Again, each drm_bridge_state contains 2 drm_bus_cfg elements: one for the output port and one for the input port

Helpers can be used from bridge drivers' ->atomic_check() implementation to negotiate the bus format to use. Negotiation happens in reserve order,

s/reserve/reverse/

...

starting from the last element of the bridge chain (the one directly connected to the display) to the first element of the chain (the one connected to the encoder).

Negotiation usually takes place in 2 steps:

• make sure the input end of the next element in the chain picked a format that's supported by the output end of our bridge. That's done with drm_atomic_bridge_choose_output_bus_cfg().
• choose a format for the input end of our bridge based on what's supported by the previous bridge in the chain. This is achieved by calling drm_atomic_bridge_choose_input_bus_cfg()

Note that those helpers are a bit lax when it comes to unitialized bus format validation. That's intentional. If we don't do that we'll basically break things if we start adding support for bus format negotiation to some elements of the pipeline leaving others on the side, and that's likely to happen for all external bridges since we can't necessarily tell where they are used.

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

drivers/gpu/drm/drm_bridge.c | 187 +++++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 46 +++++++++ include/drm/drm_encoder.h | 6 ++ 3 files changed, 239 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 9efb27087e70..ef072df42422 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -602,6 +602,193 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

+int drm_find_best_bus_format(const struct drm_bus_caps *a,

• 	     const struct drm_bus_caps *b,
  

• 	     const struct drm_display_mode *mode,
  

• 	     u32 *selected_bus_fmt)
  

+{

• unsigned int i, j;
• /*

• * Some drm_bridge/drm_encoder don't care about the input/output bus
  

• * format, let's set the format to zero in that case (this is only
  

• * valid if both side of the link don't care).
  

• */
  

• if (!a->num_supported_fmts && !b->num_supported_fmts) {

• *selected_bus_fmt = 0;
  

• return 0;
  

• } else if (b->num_supported_fmts > 1 && b->supported_fmts) {

• *selected_bus_fmt = b->supported_fmts[0];
  

• return 0;
  

• } else if (a->num_supported_fmts > 1 && a->supported_fmts) {

• *selected_bus_fmt = a->supported_fmts[0];
  

• return 0;
  

• } else if (!a->num_supported_fmts || !a->supported_fmts ||

•    !b->num_supported_fmts || !b->supported_fmts) {
  

• return -EINVAL;
  

• }
• /*

• * TODO:
  

• * Dummy algo picking the first match. We probably want something
  

• * smarter where the narrowest format (in term of bus width) that
  

• * does not incur data loss is picked, and if all possible formats
  

• * are lossy, pick the one that's less lossy among all the choices
  

• * we have. In order to do that we'd need to convert MEDIA_BUS_FMT_
  

• * modes into something like drm_format_info.
  

• */
  

• for (i = 0; i < a->num_supported_fmts; i++) {

• for (j = 0; j < b->num_supported_fmts; j++) {
  

• 	if (a->supported_fmts[i] == b->supported_fmts[j]) {
  

• 		*selected_bus_fmt = a->supported_fmts[i];
  

• 		return 0;
  

• 	}
  

• }
  

• }
• return -EINVAL;

+} +EXPORT_SYMBOL(drm_find_best_bus_format);

+/**

• • drm_atomic_bridge_choose_input_bus_cfg() - Choose bus config for the input

• • 			      end
    

• • @bridge_state: bridge state
• • @crtc_state: CRTC state
• • @conn_state: connector state
• • Choose a bus format for the input side of a bridge based on what the
• • previous bridge in the chain and this bridge support. Can be called from
• • bridge drivers' &drm_bridge_funcs.atomic_check() implementation.
• • RETURNS:
• • 0 if a matching format was found, a negative error code otherwise
• */

+int +drm_atomic_bridge_choose_input_bus_cfg(struct drm_bridge_state *bridge_state,

• 		       struct drm_crtc_state *crtc_state,
  

• 		       struct drm_connector_state *conn_state)
  

+{

• struct drm_bridge *self = bridge_state->bridge;
• struct drm_bus_caps *prev_output_bus_caps;
• struct drm_bridge *prev;
• int ret;
• prev = drm_bridge_chain_get_prev_bridge(self);
• if (!prev)

• prev_output_bus_caps = &self->encoder->output_bus_caps;
  

• else

• prev_output_bus_caps = &prev->output_bus_caps;
  

• ret = drm_find_best_bus_format(prev_output_bus_caps,

• 		       &self->input_bus_caps, &crtc_state->mode,
  

• 		       &bridge_state->input_bus_cfg.fmt);
  

• if (ret)

• return ret;  
  

I think most bridges that support multiple input and output formats will not necessarily be able to transcode, meaning that for a particular output format a particular corresponding input format will need to be picked (or possibly one of a subset of input formats). I'm thus not sure how useful this helper will be in practice.

I also fear that this negotiation mechanism is too simple and will lock is with support for very simple systems only :-S Let's assume a system with a display device (D), two bridges (B0 and B1) and a connector (C). Let's further assume that the bus format required by the connector is fixed. With this patch series, B1 will pick an output format identical to the format on the connector, and will then pick an input format among the ones supported by both B0 and B1. Let's assume there are multiple options, Fa and Fb, and drm_find_best_bus_format() will pick one of them, say Fa. Then B0 will configure its output with that format, and will proceed with finding an input format supported by D. It could be that the output format Fa for B0 requires an input format that is not supported by D, while Fb would have produced a working pipeline.

Yes, I assumed all pipeline elements would be able to convert any of their supported input formats to any of the output ones, which might not be the case in practice. I could add a matrix to expose the supported conversions and add a pass of 'format eviction' at bridge attach time, but I'm not sure that's enough.

I don't think this example is really far-fetched, and while we don't need to support it immediately, we need an API that makes it possible to support it, as changing the API later would require reworking all the drivers that use it.

...

• /*

• * TODO:
  

• * Should we fill/check the ->flag field too?
  

• */
  

• return 0;

+} +EXPORT_SYMBOL(drm_atomic_bridge_choose_input_bus_cfg);

+/**

• • drm_atomic_bridge_choose_output_bus_cfg() - Choose bus config for the output

• • 			       end
    

• • @bridge_state: bridge state
• • @crtc_state: CRTC state
• • @conn_state: connector state
• • Choose a bus format for the output side of a bridge based on what the next
• • bridge in the chain and this bridge support. Can be called from bridge
• • drivers' &drm_bridge_funcs.atomic_check() implementation.
• • RETURNS:
• • 0 if a matching format was found, a negative error code otherwise
• */

+int +drm_atomic_bridge_choose_output_bus_cfg(struct drm_bridge_state *bridge_state,

• 			struct drm_crtc_state *crtc_state,
  

• 			struct drm_connector_state *conn_state)
  

+{

• struct drm_atomic_state *state = crtc_state->state;
• struct drm_bridge *self = bridge_state->bridge;
• struct drm_bridge_state *next_bridge_state;
• struct drm_bridge *next;
• u32 fmt;
• next = drm_bridge_chain_get_next_bridge(self);
• if (!next) {

• struct drm_connector *conn = conn_state->connector;
  

• struct drm_display_info *di = &conn->display_info;
  

• /*
  

•  * FIXME:
  

•  * We currently pick the first supported format
  

•  * unconditionally. It seems to fit all current use cases but
  

•  * might be too limited for panels/displays that can configure
  

•  * the bus format dynamically.
  

•  */
  

• if (di->num_bus_formats && di->bus_formats)
  

• 	bridge_state->output_bus_cfg.fmt = di->bus_formats[0];
  

• else
  

• 	bridge_state->output_bus_cfg.fmt = 0;  
  

What is the bridge driver supposed to do in this case ?