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

[Daniel Vetter <daniel@ffwll.ch>]

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"
-Daniel
-- 
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