[PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Laurent Pinchart]

Hi Archit,

On Wednesday 30 Nov 2016 16:30:53 Archit Taneja wrote:
> On 11/30/2016 03:53 PM, Laurent Pinchart wrote:
> > On Wednesday 30 Nov 2016 10:35:02 Archit Taneja wrote:
> >> On 11/29/2016 11:27 PM, Laurent Pinchart wrote:
> >>> On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:
> >>>> On 11/29/2016 02:34 PM, Laurent Pinchart wrote:
> >>>>> Instead of linking encoders and bridges in every driver (and getting
> >>>>> it wrong half of the time, as many drivers forget to set the
> >>>>> drm_bridge encoder pointer), do so in core code. The
> >>>>> drm_bridge_attach() function needs the encoder and optional previous
> >>>>> bridge to perform that task, update all the callers.
> >>>>> 
> >>>>> Signed-off-by: Laurent Pinchart
> >>>>> <laurent.pinchart+renesas at ideasonboard.com>
> >>>>> ---

[snip]

> >>>> I think we could derive previous from the encoder itself. Something
> >>>> like:
> >>>>
> >>>> 	previous = encoder->bridge;
> >>>> 	while (previous && previous->next)
> >>>> 		previous = previous->next;
> >>> 
> >>> That's a very good point. It would however prevent us from catching
> >>> drivers that attach bridges in the wrong order, which the !previous->dev
> >>> currently allows us to do (and it should be turned into a WARN_ON as
> >>> Daniel proposed).
> >> 
> >> With the simpler API, I don't think we will ever hit the case of
> >> !previous->dev. The previous bridge (if it exists) in the chain would
> >> already have a dev attached to it. In other words, we would remove the
> >> risk of the chance of the 'previous' bridge being unattached.
> >> 
> >> I'm a bit unclear about what you mean about the order part. If a kms
> >> driver
> >> wants to create a chain: encoder->bridge1->bridge2, it should ideally do:
> >> 
> >> drm_bridge_attach(encoder, bridge1, NULL);
> >> drm_bridge_attach(encoder, bridge2, bridge1);
> > 
> > Correct.
> > 
> >> We can't do much if the kms driver does the opposite:
> >> 
> >> drm_bridge_attach(encoder, bridge2, NULL);
> >> drm_bridge_attach(encoder, bridge2, bridge1);
> > 
> > That would certainly be a very stupid thing for a driver to do :-) The
> > problem that we could catch with my current proposal is
> > 
> > drm_bridge_attach(encoder, bridge2, bridge1);
> > ...
> > drm_bridge_attach(encoder, bridge1, NULL);
> > 
> > which I expect to happen from time to time as the two bridge can be
> > attached through separate code paths sometimes a bit difficult to trace.
> > It's not a big deal though, you could convince me that the advantages of
> > a simpler API exceed its drawbacks.
> 
> Having no 'previous' argument would prevent the possibility of this
> altogether, won't it?
> 
> With no 'previous' arg in the API, the driver can only do:
> 
> drm_bridge_attach(encoder, bridge1);
> drm_bridge_attach(encoder, bridge2);
> 
> or
> 
> drm_bridge_attach(encoder, bridge2);
> drm_bridge_attach(encoder, bridge1);

Correct.

> For the latter, we can't do much as discussed above.

Except that with the currently proposed API the code would be

drm_bridge_attach(encoder, bridge2, bridge1);
drm_bridge_attach(encoder, bridge1, NULL);

(correct case)

or

drm_bridge_attach(encoder, bridge2, bridge1);
drm_bridge_attach(encoder, bridge1, NULL);

(incorrect case)

The second one could be caught by the drm_bridge_attach() function as bridge1-
>dev will be NULL when attaching bridge2 in the incorrect case.

-- 
Regards,

Laurent Pinchart