Re: [PATCH v3 1/2] drm: bridge: Allow daisy chaining of bridges

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 19, 2015 at 02:05:04PM +0530, Archit Taneja wrote:
> Allow drm_bridge objects to link to each other in order to form an encoder
> chain. The requirement for creating a chain of bridges comes because the
> MSM drm driver uses up its encoder and bridge objects for blocks within
> the SoC itself. There isn't anything left to use if the SoC display output
> is connected to an external encoder IC. Having an additional bridge
> connected to the existing bridge helps here. In general, it is possible for
> platforms to have  multiple devices between the encoder and the
> connector/panel that require some sort of configuration.
> 
> We create drm bridge helper functions corresponding to each op in
> 'drm_bridge_funcs'. These helpers call the corresponding
> 'drm_bridge_funcs' op for the entire chain of bridges. These helpers are
> used internally by drm_atomic_helper.c and drm_crtc_helper.c.
> 
> The drm_bridge_enable/pre_enable helpers execute enable/pre_enable ops of
> the bridge closet to the encoder, and proceed until the last bridge in the
> chain is enabled. The same holds for drm_bridge_mode_set/mode_fixup
> helpers. The drm_bridge_disable/post_disable helpers disable the last
> bridge in the chain first, and proceed until the first bridge in the chain
> is disabled.
> 
> drm_bridge_attach() remains the same. As before, the driver calling this
> function should make sure it has set the links correctly. The order in
> which the bridges are connected to each other determines the order in which
> the calls are made. One requirement is that every bridge in the chain
> should point the parent encoder object. This is required since bridge
> drivers expect a valid encoder pointer in drm_bridge. For example, consider
> a chain where an encoder's output is connected to bridge1, and bridge1's
> output is connected to bridge2:
> 
> 	/* Like before, attach bridge to an encoder */
> 	bridge1->encoder = encoder;
> 	ret = drm_bridge_attach(dev, bridge1);
> 	..
> 
> 	/*
> 	 * set the first bridge's 'next' bridge to bridge2, set its encoder
> 	 * as bridge1's encoder
> 	 */
> 	bridge1->next = bridge2
> 	bridge2->encoder = bridge1->encoder;
> 	ret = drm_bridge_attach(dev, bridge2);
> 
> 	...
> 	...
> 
> This method of bridge chaining isn't intrusive and existing drivers that
> use drm_bridge will behave the same way as before. The bridge helpers also
> cleans up the atomic and crtc helper files a bit.
> 
> Reviewed-by: Jani Nikula <jani.nikula@linux.intel.com>
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Archit Taneja <architt@codeaurora.org>

Two comments below about the nesting of callers. lgtm otherwise.
-Daniel

> ---
> v3:
> - Add headerdocs for the new functions
> 
> v2:
> - Add EXPORT_SYMBOL for the new functions
> - Fix logic issue in mode_fixup()
> 
>  drivers/gpu/drm/drm_atomic_helper.c |  29 +++-----
>  drivers/gpu/drm/drm_bridge.c        | 144 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_helper.c   |  54 +++++---------
>  include/drm/drm_crtc.h              |  14 ++++
>  4 files changed, 188 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1d2ca52..d6c85c0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -281,14 +281,11 @@ mode_fixup(struct drm_atomic_state *state)
>  		encoder = conn_state->best_encoder;
>  		funcs = encoder->helper_private;
>  
> -		if (encoder->bridge && encoder->bridge->funcs->mode_fixup) {
> -			ret = encoder->bridge->funcs->mode_fixup(
> -					encoder->bridge, &crtc_state->mode,
> -					&crtc_state->adjusted_mode);
> -			if (!ret) {
> -				DRM_DEBUG_ATOMIC("Bridge fixup failed\n");
> -				return -EINVAL;
> -			}
> +		ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
> +				&crtc_state->adjusted_mode);
> +		if (!ret) {
> +			DRM_DEBUG_ATOMIC("Bridge fixup failed\n");
> +			return -EINVAL;
>  		}
>  
>  		if (funcs->atomic_check) {
> @@ -578,8 +575,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		 * Each encoder has at most one connector (since we always steal
>  		 * it away), so we won't call disable hooks twice.
>  		 */
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->disable(encoder->bridge);
> +		drm_bridge_disable(encoder->bridge);
>  
>  		/* Right function depends upon target state. */
>  		if (connector->state->crtc && funcs->prepare)
> @@ -589,8 +585,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		else
>  			funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>  
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->post_disable(encoder->bridge);
> +		drm_bridge_post_disable(encoder->bridge);
>  	}
>  
>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> @@ -713,9 +708,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		if (funcs->mode_set)
>  			funcs->mode_set(encoder, mode, adjusted_mode);
>  
> -		if (encoder->bridge && encoder->bridge->funcs->mode_set)
> -			encoder->bridge->funcs->mode_set(encoder->bridge,
> -							 mode, adjusted_mode);
> +		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>  	}
>  }
>  
> @@ -809,16 +802,14 @@ 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.
>  		 */
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->pre_enable(encoder->bridge);
> +		drm_bridge_pre_enable(encoder->bridge);
>  
>  		if (funcs->enable)
>  			funcs->enable(encoder);
>  		else
>  			funcs->commit(encoder);
>  
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->enable(encoder->bridge);
> +		drm_bridge_enable(encoder->bridge);
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index eaa5790..f70e617 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -66,6 +66,150 @@ int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
>  }
>  EXPORT_SYMBOL(drm_bridge_attach);
>  
> +/**
> + * 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 'mode_fixup' drm_bridge_funcs 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
> + */
> +bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
> +			const struct drm_display_mode *mode,
> +			struct drm_display_mode *adjusted_mode)
> +{
> +	bool ret = true;
> +
> +	if (!bridge)
> +		return true;
> +
> +	if (bridge->funcs->mode_fixup)
> +		ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);
> +
> +	ret = ret && drm_bridge_mode_fixup(bridge->next, mode, adjusted_mode);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_bridge_mode_fixup);
> +
> +/**
> + * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all
> + *			bridges in the encoder chain.
> + * @bridge: bridge control structure
> + *
> + * Calls 'disable' drm_bridge_funcs 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)
> +{
> +	if (!bridge)
> +		return;
> +
> +	drm_bridge_disable(bridge->next);
> +
> +	bridge->funcs->disable(bridge);
> +}
> +EXPORT_SYMBOL(drm_bridge_disable);
> +
> +/**
> + * drm_bridge_post_disable - calls 'post_disable' drm_bridge_funcs op for
> + *			     all bridges in the encoder chain.
> + * @bridge: bridge control structure
> + *
> + * Calls 'post_disable' drm_bridge_funcs op for all the bridges in the
> + * encoder chain, starting from the last bridge to the first. 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)
> +{
> +	if (!bridge)
> +		return;
> +
> +	drm_bridge_post_disable(bridge->next);
> +
> +	bridge->funcs->post_disable(bridge);

For symmetry I'd call the post_disable hook for the next brigde _after_
this one. Otherwise we break abstraction. E.g. with 1 bridge we'd have

bridgeA_disable();
encoder_disable();
bridgeA_post_disable();

But if on some other part bridge A is connected to bridge B (i.e. we
replace the encoder with a combo of other_encoder+bridgeA) with your code
the post_disable is suddenly interleaved with the preceeding stages in the
pipeline:


bridgeA_disable();
bridgeB_disable();
other_encoder_disable();
bridgeA_post_disable();
bridgeB_post_disable();

Which means from the pov of bridgeA there's a difference between whether
it's connected to "encoder" or "other_encoder+bridgeB". But if we reorder
the post_disable sequence like I suggest we'll get:

bridgeA_disable();
bridgeB_disable();
other_encoder_disable();
bridgeB_post_disable();
bridgeA_post_disable();

Which means that "encoder" and "other_encoder+bridgeB" look the same for
bridgeA. To avoid confusion the two pipelines in hw are:

encoder ---> bridgeA

other_encoder ---> bridgeB ---> bridgeA

> +}
> +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 'mode_set' drm_bridge_funcs 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,
> +			struct drm_display_mode *mode,
> +			struct drm_display_mode *adjusted_mode)
> +{
> +	if (!bridge)
> +		return;
> +
> +	if (bridge->funcs->mode_set)
> +		bridge->funcs->mode_set(bridge, mode, adjusted_mode);
> +
> +	drm_bridge_mode_set(bridge->next, mode, adjusted_mode);
> +}
> +EXPORT_SYMBOL(drm_bridge_mode_set);
> +
> +/**
> + * drm_bridge_pre_enable - calls 'pre_enable' drm_bridge_funcs op for all
> + *			   bridges in the encoder chain.
> + * @bridge: bridge control structure
> + *
> + * Calls 'pre_enable' drm_bridge_funcs op for all the bridges in the encoder
> + * chain, starting from the first bridge to the last. 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)
> +{
> +	if (!bridge)
> +		return;
> +
> +	bridge->funcs->pre_enable(bridge);
> +
> +	drm_bridge_pre_enable(bridge->next);

Same as with post_disable, pre_enable for bridge->next should be called
_before_ the pre_enable for this bridge.

> +}
> +EXPORT_SYMBOL(drm_bridge_pre_enable);
> +
> +/**
> + * drm_bridge_enable - calls 'enable' drm_bridge_funcs op for all bridges
> + *		       in the encoder chain.
> + * @bridge: bridge control structure
> + *
> + * Calls 'enable' drm_bridge_funcs 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)
> +{
> +	if (!bridge)
> +		return;
> +
> +	bridge->funcs->enable(bridge);
> +
> +	drm_bridge_enable(bridge->next);
> +}
> +EXPORT_SYMBOL(drm_bridge_enable);
> +
>  #ifdef CONFIG_OF
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  {
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index ab00286..ff9b338 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -163,16 +163,14 @@ drm_encoder_disable(struct drm_encoder *encoder)
>  {
>  	const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
>  
> -	if (encoder->bridge)
> -		encoder->bridge->funcs->disable(encoder->bridge);
> +	drm_bridge_disable(encoder->bridge);
>  
>  	if (encoder_funcs->disable)
>  		(*encoder_funcs->disable)(encoder);
>  	else
>  		(*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF);
>  
> -	if (encoder->bridge)
> -		encoder->bridge->funcs->post_disable(encoder->bridge);
> +	drm_bridge_post_disable(encoder->bridge);
>  }
>  
>  static void __drm_helper_disable_unused_functions(struct drm_device *dev)
> @@ -312,13 +310,11 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		if (encoder->crtc != crtc)
>  			continue;
>  
> -		if (encoder->bridge && encoder->bridge->funcs->mode_fixup) {
> -			ret = encoder->bridge->funcs->mode_fixup(
> -					encoder->bridge, mode, adjusted_mode);
> -			if (!ret) {
> -				DRM_DEBUG_KMS("Bridge fixup failed\n");
> -				goto done;
> -			}
> +		ret = drm_bridge_mode_fixup(encoder->bridge,
> +			mode, adjusted_mode);
> +		if (!ret) {
> +			DRM_DEBUG_KMS("Bridge fixup failed\n");
> +			goto done;
>  		}
>  
>  		encoder_funcs = encoder->helper_private;
> @@ -343,15 +339,13 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		if (encoder->crtc != crtc)
>  			continue;
>  
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->disable(encoder->bridge);
> +		drm_bridge_disable(encoder->bridge);
>  
>  		encoder_funcs = encoder->helper_private;
>  		/* Disable the encoders as the first thing we do. */
>  		encoder_funcs->prepare(encoder);
>  
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->post_disable(encoder->bridge);
> +		drm_bridge_post_disable(encoder->bridge);
>  	}
>  
>  	drm_crtc_prepare_encoders(dev);
> @@ -376,9 +370,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		encoder_funcs = encoder->helper_private;
>  		encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>  
> -		if (encoder->bridge && encoder->bridge->funcs->mode_set)
> -			encoder->bridge->funcs->mode_set(encoder->bridge, mode,
> -					adjusted_mode);
> +		drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -389,14 +381,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>  		if (encoder->crtc != crtc)
>  			continue;
>  
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->pre_enable(encoder->bridge);
> +		drm_bridge_pre_enable(encoder->bridge);
>  
>  		encoder_funcs = encoder->helper_private;
>  		encoder_funcs->commit(encoder);
>  
> -		if (encoder->bridge)
> -			encoder->bridge->funcs->enable(encoder->bridge);
> +		drm_bridge_enable(encoder->bridge);
>  	}
>  
>  	/* Calculate and store various constants which
> @@ -735,23 +725,19 @@ 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;
>  
> -	if (bridge) {
> -		if (mode == DRM_MODE_DPMS_ON)
> -			bridge->funcs->pre_enable(bridge);
> -		else
> -			bridge->funcs->disable(bridge);
> -	}
> +	if (mode == DRM_MODE_DPMS_ON)
> +		drm_bridge_pre_enable(bridge);
> +	else
> +		drm_bridge_disable(bridge);
>  
>  	encoder_funcs = encoder->helper_private;
>  	if (encoder_funcs->dpms)
>  		encoder_funcs->dpms(encoder, mode);
>  
> -	if (bridge) {
> -		if (mode == DRM_MODE_DPMS_ON)
> -			bridge->funcs->enable(bridge);
> -		else
> -			bridge->funcs->post_disable(bridge);
> -	}
> +	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_crtc.h b/include/drm/drm_crtc.h
> index ca71c03..db830f4 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -895,6 +895,8 @@ struct drm_bridge_funcs {
>  /**
>   * struct drm_bridge - central DRM bridge control structure
>   * @dev: DRM device this bridge belongs to
> + * @encoder: encoder to which this bridge is connected
> + * @next: the next bridge in the encoder chain
>   * @of_node: device node pointer to the bridge
>   * @list: to keep track of all added bridges
>   * @base: base mode object
> @@ -904,6 +906,7 @@ struct drm_bridge_funcs {
>  struct drm_bridge {
>  	struct drm_device *dev;
>  	struct drm_encoder *encoder;
> +	struct drm_bridge *next;
>  #ifdef CONFIG_OF
>  	struct device_node *of_node;
>  #endif
> @@ -1230,6 +1233,17 @@ extern void drm_bridge_remove(struct drm_bridge *bridge);
>  extern struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>  extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge);
>  
> +bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
> +			const struct drm_display_mode *mode,
> +			struct drm_display_mode *adjusted_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,
> +			struct drm_display_mode *mode,
> +			struct drm_display_mode *adjusted_mode);
> +void drm_bridge_pre_enable(struct drm_bridge *bridge);
> +void drm_bridge_enable(struct drm_bridge *bridge);
> +
>  extern int drm_encoder_init(struct drm_device *dev,
>  			    struct drm_encoder *encoder,
>  			    const struct drm_encoder_funcs *funcs,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel