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
