[PATCH v2] drm: bridge: Allow daisy chaining of bridges

[Rob Clark]

On Fri, May 8, 2015 at 9:54 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Fri, May 08, 2015 at 09:03:19AM -0400, Rob Clark wrote:
>> On Fri, May 8, 2015 at 6:11 AM, Archit Taneja <architt at codeaurora.org> 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.
>> >
>> > Signed-off-by: Archit Taneja <architt at codeaurora.org>
>>
>> Looks good. But I guess we probably should have some headerdoc for the
>> new fxns, and somewhere a comment about not calling the bridge vfuncs
>> directly but using the new helper funcs which handle the chaining.  I
>> guess it isn't *as* critical as it would be if these were things
>> intended to be called by drivers, rather than just from core, but all
>> the same I think it would be a good idea.  With that,
>
> Yeah, at least for patches that I'll take in through drm-misc I really
> want kerneldoc. Also shouldnt' we do a cocci-run over all the drm drivers
> to make sure they use the new helpers now?

cocci might have been the more clever way to do it, but I did have a
quick check on the call-sites for bridge vfunc's with this patch
applied and didn't see any calls from drivers or unconverted call
sites in core..

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch