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@xxxxxxxxxxxxxxxx> > >>>>> --- [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
