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

[Archit Taneja <architt@codeaurora.org>]

Hi,

On 05/19/2015 03:04 PM, Daniel Vetter wrote:
> 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

I agree with this, thanks for the explanation. Although, I'm not sure 
about the pre_enable part below.

<snip>

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


The order of nesting suggested by you gives the sequence:

bridgeA_pre_enable();
bridgeB_pre_enable();
other_encoder_enable();
bridgeB_enable();
bridgeA_enable();

This won't work if the bridge A relies on bridge B to be enabled first. 
This happens in the encoder path I'm working on:

msm mdp5 encoder -> dsi bridge -> adv7511 dsi to hdmi bridge chip


The adv chip relies on dsi's clock for it to function. If dsi's 
pre_enable() isn't called first, then adv's pre_enable would fail.

We could modify the call order in drm_bridge_enable() instead to achieve:

bridgeB_pre_enable();
bridgeA_pre_enable();
other_encoder_enable();
bridgeA_enable();
bridgeB_enable();

This fixes the sequence for pre_enable() calls, but assumes that the 
configurations in the enable() don't need to follow a specific sequence 
to ensure proper behavior.

pre_enable() should ideally represent things to be done before we enable 
the next encoder in the chain. So the sequence right above seems to be 
better.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel