Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order

Hi Dave,

Thank you for the patch.

On Wed, Feb 16, 2022 at 04:59:43PM +0000, Dave Stevenson wrote:

DSI sink devices typically want the DSI host powered up and configured before they are powered up. pre_enable is the place this would normally happen, but they are called in reverse order from panel/connector towards the encoder, which is the "wrong" order.

Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set to swap the order of pre_enable (and post_disable) so that any upstream bridges are called first to create the desired state.

eg:

• Panel
• Bridge 1
• Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
• Bridge 3
• Encoder

Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3, Bridge 2.

If there was a Bridge 4 between Bridge 3 and Encoder, would it be

Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2

? I'd capture that here, to be explicit.

Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com

drivers/gpu/drm/drm_bridge.c | 197 +++++++++++++++++++++++++++++++++++++------ include/drm/drm_bridge.h | 8 ++ 2 files changed, 180 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7c24e8340efa 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);

• 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.

Missing blank line, as well as in three locations below.

• • If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
• • that bridge will be called before the previous one to reverse the pre_enable
• • calling direction.
  • Note: the bridge passed should be the one closest to the encoder
  */

void drm_bridge_chain_post_disable(struct drm_bridge *bridge) { struct drm_encoder *encoder;

• struct drm_bridge *next, *limit;

  if (!bridge) return;

  encoder = bridge->encoder; list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {

• limit = NULL;
  

• if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
  

• 	next = list_next_entry(bridge, chain_node);
  

• 	if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
  

• 		limit = next;
  

• 		list_for_each_entry_from(next, &encoder->bridge_chain,
  

• 					 chain_node) {
  

• 			if (!(next->ops &
  

• 				DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
  

• 				next = list_prev_entry(next, chain_node);
  

• 				limit = next;
  

• 				break;
  

• 			}
  

• 		}
  

• 		list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
  

• 						 chain_node) {
  

• 			if (next == bridge)
  

• 				break;
  

• 			if (next->funcs->post_disable)
  

• 				next->funcs->post_disable(next);
  

• 		}
  

• 	}
  

• }
  

• if (bridge->funcs->post_disable) bridge->funcs->post_disable(bridge);

• if (limit)
  

• 	bridge = limit;
  

  }

} EXPORT_SYMBOL(drm_bridge_chain_post_disable);

@@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);

• 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.

• • If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
• • the previous bridge will be called before pre_enable of this bridge.
  • Note: the bridge passed should be the one closest to the encoder
  */

void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) { struct drm_encoder *encoder;

• struct drm_bridge *iter;

• struct drm_bridge *iter, *next, *limit;

  if (!bridge) return;

  encoder = bridge->encoder;

• list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {

• if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
  

• 	next = iter;
  

• 	limit = bridge;
  

• 	list_for_each_entry_from_reverse(next,
  

• 					 &encoder->bridge_chain,
  

• 					 chain_node) {
  

• 		if (next == bridge)
  

• 			break;
  

• 		if (!(next->ops &
  

• 			DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
  

• 			limit = list_prev_entry(next, chain_node);
  

• 			break;
  

• 		}
  

• 	}
  

• 	list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
  

• 		if (next == iter)
  

• 			break;
  

• 		if (next->funcs->pre_enable)
  

• 			next->funcs->pre_enable(next);
  

• 	}
  

• }
  

• if (iter->funcs->pre_enable) iter->funcs->pre_enable(iter);

• if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
  

• 	iter = limit;
  

• if (iter == bridge) break; }

@@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);

+static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,

• 				struct drm_atomic_state *old_state)
  

+{

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

• struct drm_bridge_state *old_bridge_state;
  

• old_bridge_state =
  

• 	drm_atomic_get_old_bridge_state(old_state,
  

• 					bridge);
  

• if (WARN_ON(!old_bridge_state))
  

• 	return;
  

• bridge->funcs->atomic_post_disable(bridge,
  

• 				   old_bridge_state);
  

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

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

• }

+}

/**

• drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges

• 			  in the encoder chain
  

@@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);

• &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

• • If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
• • that bridge will be called before the previous one to reverse the pre_enable
• • calling direction.
  • Note: the bridge passed should be the one closest to the encoder
  */

@@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge, struct drm_atomic_state *old_state) { struct drm_encoder *encoder;

• struct drm_bridge *next, *limit;

  if (!bridge) return;

  encoder = bridge->encoder;

• list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {

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

• 	struct drm_bridge_state *old_bridge_state;
  

• limit = NULL;
  

• if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
  

• 	next = list_next_entry(bridge, chain_node);
  

• 	if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
  

• 		limit = next;
  

• 		list_for_each_entry_from(next, &encoder->bridge_chain,
  

• 					 chain_node) {
  

• 			if (!(next->ops &
  

• 				DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
  

• 				next = list_prev_entry(next, chain_node);
  

• 				limit = next;
  

• 				break;
  

• 			}
  

• 		}
  

• 		list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
  

• 						 chain_node) {
  

• 			if (next == bridge)
  

• 				break;
  

• 			drm_atomic_bridge_call_post_disable(next,
  

• 							    old_state);
  

• 		}
  

• 	}
  

• }
  
  

• 	old_bridge_state =
  

• 		drm_atomic_get_old_bridge_state(old_state,
  

• 						bridge);
  

• 	if (WARN_ON(!old_bridge_state))
  

• 		return;
  

• drm_atomic_bridge_call_post_disable(bridge, old_state);
  
  

• 	bridge->funcs->atomic_post_disable(bridge,
  

• 					   old_bridge_state);
  

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

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

• }
  

• if (limit)
  

• 	bridge = limit;
  

  }

} EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);

+static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,

• 			      struct drm_atomic_state *old_state)
  

+{

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

• struct drm_bridge_state *old_bridge_state;
  

• old_bridge_state =
  

• 	drm_atomic_get_old_bridge_state(old_state,
  

• 					bridge);
  

• if (WARN_ON(!old_bridge_state))
  

• 	return;
  

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

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

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

• }

+}

/**

• drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in

• 			the encoder chain
  

@@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);

• &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

• • If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
• • the previous bridge will be called before pre_enable of this bridge.
  • Note: the bridge passed should be the one closest to the encoder
  */

@@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *old_state) { struct drm_encoder *encoder;

• struct drm_bridge *iter;

• struct drm_bridge *iter, *next, *limit;

  if (!bridge) return;

  encoder = bridge->encoder;

• list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {

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

• 	struct drm_bridge_state *old_bridge_state;
  

• if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
  

• 	next = iter;
  

• 	limit = bridge;
  

• 	list_for_each_entry_from_reverse(next,
  

• 					 &encoder->bridge_chain,
  

• 					 chain_node) {
  

• 		if (next == bridge)
  

• 			break;
  

• 		if (!(next->ops &
  

• 			DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
  

• 			limit = list_prev_entry(next, chain_node);
  

• 			break;
  

• 		}
  

• 	}
  

• 	list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
  

• 		if (next == iter)
  

• 			break;
  

• 		drm_atomic_bridge_call_pre_enable(next, old_state);
  

• 	}
  

• }
  

This is hard to understand, I have trouble figuring out if it does the right thing when multiple bridges set the DRM_BRIDGE_OP_UPSTREAM_FIRST flag (or actually even when a single bridge does so). Comments would help, but I wonder if it wouldn't be simpler to switch to a recursive implementation.

It also seems that merging the legacy and atomic versions of the code would be a good idea. They could both call into a shared implementation, with the legacy version passing a NULL state, and the atomic op being considered only if the state is not NULL.

• 	old_bridge_state =
  

• 		drm_atomic_get_old_bridge_state(old_state,
  

• 						iter);
  

• 	if (WARN_ON(!old_bridge_state))
  

• 		return;
  

• drm_atomic_bridge_call_pre_enable(iter, old_state);
  
  

• 	iter->funcs->atomic_pre_enable(iter, old_bridge_state);
  

• } else if (iter->funcs->pre_enable) {
  

• 	iter->funcs->pre_enable(iter);
  

• }
  

• if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
  

• 	iter = limit;
  
  

  if (iter == bridge) break;

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index f27b4060faa2..523ec9d8f3f8 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -725,6 +725,14 @@ enum drm_bridge_ops { * this flag shall implement the &drm_bridge_funcs->get_modes callback. */ DRM_BRIDGE_OP_MODES = BIT(3),

• /**

• * @DRM_BRIDGE_OP_UPSTREAM_FIRST: The bridge can requires
  

s/can //

• * that the upstream node pre_enable is called before its pre_enable,
  

s/node/bridge/ ?

• * and conversely for post_disables. This is most frequently a
  

s/post_disables/post_disable/

Bonus points if you use the correct markup to link to those operations.

• * requirement for DSI devices which need the host to be initialised
  

• * before them.
  

• */
  

• DRM_BRIDGE_OP_UPSTREAM_FIRST = BIT(4),

};

/**