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

[Archit Taneja]

On 05/08/2015 08:00 PM, Rob Clark wrote:
> 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..

I'll repost with headerdoc for the new functions.

Thanks,
Archit

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