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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
> ---
>  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),
>  };
>  
>  /**

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux