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

[Archit Taneja]

On 05/19/2015 09:00 PM, Daniel Vetter wrote:
> On Tue, May 19, 2015 at 04:37:44PM +0530, Archit Taneja wrote:
>> On 05/19/2015 03:04 PM, Daniel Vetter wrote:
>>> On Tue, May 19, 2015 at 02:05:04PM +0530, Archit Taneja wrote:
>>>> +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.
>
> If you need the clock, then imo that's code which should be in the enable
> hook, and not in the pre-enable hook. At least thus far the rules have
> roughly been:
> - pre_enable: anything that needs to be done _before_ clock+timings are
>    enabled by the source for this bridge.
> - enable: anything that needs clock+timings to be on from the source for
>    this bridge.

Okay. I think I got the rules wrong. I assumed that pre_enable() should 
do the heavy enabling. This happened because I was looking at the 
bridges(dsi/hdmi/edp) in msm as reference. Those were actually a special 
case, where the bridge feeds back the pixel clock to the mdp encoder. 
That's why everything was stuffed within pre_enable. They can afford to 
not comply to the rules since they are tightly coupled with the msm driver.

>
> Similar for the disable side:
> - disable still has clocks+timing on.
> - post_disable should be called after clocks are off.
>
> In i915 we once had a callback in between clock enabling and timing
> enabling, but we've removed that again after some restructuring. Maybe we
> need such a hook bridges? But that also means we need to split up
> encoder/crtc callbacks, and that will get nasty real fast.

Yeah, let's stay away from that!

>
>> 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.
>
> Nah, that's even more backwards imo. From bridgeA's pov it really should
> make no difference at all whether the input is directly from an encoder or
> whether it's "other_encoder+bridgeB". If we leak this detail down the
> bridge chain, then the abstraction is leaky and bridge drivers will be
> impossible to be reused.

Right. That makes sense. I came up with that to under the assumption 
that the bridge's source should be ready with clocks and timings, which 
was wrong. I'll stick to the order you mentioned.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project