On 05/20/2015 12:05 PM, Daniel Vetter wrote:
> On Wed, May 20, 2015 at 10:49:38AM +0530, Archit Taneja wrote:
>> 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.
>
> Since this (how pre/post are wrapped around clock+timing enabling from the
> source) caused confusion I think we should document this in the DOC
> kerneldoc for drm_bridge. Can you please create a small patch for that?
> Maybe even do a new DOC: section with it's own subtitle like "Default
> sequence for bridge callbacks"
Yes, I was planning to add this in the second patch itself.
Where do you think drm_bridge documentation fits? I was considering
putting it under 'KMS initialization and setup', while pointing out that
it isn't exactly a drm_mode_object entity like crtcs or encoders etc.
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
