Hi Daniel,
On Tuesday 29 Nov 2016 20:02:01 Laurent Pinchart wrote:
> On Tuesday 29 Nov 2016 11:05:27 Daniel Vetter wrote:
> > On Tue, Nov 29, 2016 at 11:43:19AM +0200, Laurent Pinchart wrote:
> >> On Tuesday 29 Nov 2016 10:35:24 Daniel Vetter wrote:
> >>> On Tue, Nov 29, 2016 at 11:04:33AM +0200, 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]
> >>>> diff --git a/drivers/gpu/drm/drm_bridge.c
> >>>> b/drivers/gpu/drm/drm_bridge.c
> >>>> index 0ee052b7c21a..850bd6509ef1 100644
> >>>> --- a/drivers/gpu/drm/drm_bridge.c
> >>>> +++ b/drivers/gpu/drm/drm_bridge.c
>
> [snip]
>
> >>>> -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge
> >>>> *bridge)
> >>>> +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
> >>>> *bridge,
> >>>> + struct drm_bridge *previous)
> >>>> {
> >>>> - if (!dev || !bridge)
> >>>> + int ret;
> >>>> +
> >>>> + if (!encoder || !bridge)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + if (previous && (!previous->dev || previous->encoder !=
> >>>> encoder))
> >>>> return -EINVAL;
> >>>
> >>> Not sure we want to allow setting both encoder and bridge for chaining.
> >>> I'd kinda expect that for chained use-case the bridge doesn't care that
> >>> much who started the chain. And if not we can always create a helper to
> >>> look up the drm_encoder.
> >>
> >> As bridge drivers currently create connectors (I'd like to change that
> >> at some point, but one thing at a time) they need to call
> >> drm_mode_connector_attach_encoder() and thus need to have access to the
> >> drm_encoder object at the beginning of the chain. The drm_bridge
> >> structure has an encoder field, it seems to me that the easiest is to
> >> always populate it, regardless of the position of the bridge in the
> >> chain.
> >
> > I mean the function inteface, not what we fill out in the drm_bridge
> > struct. I.e. instead of expecting callers to give you the encoder for
> >
> > chained bridges, look it up yourself:
> > bridge->encoder = previous : previous->encoder ? encoder;
> >
> > Or something like that. Just feels confusing to connect a bridge to both
> > a bridge _and_ the first encoder.
>
> Right. Archit proposed doing it the other way around:
>
> previous = encoder->bridge;
> while (previous && previous->next)
> previous = previous->next;
>
> That would simplify the API, at the cost of preventing us from catching
> drivers that attach bridges in the wrong order (through the !previous->dev
> check that you suggested should be turned into a WARN_ON). The previous-
>
> >encoder != encoder check is also a safety net.
>
> Any opinion on which option you like better ? I'd be very tempted to go for
> Archit's proposal as it removes the previous parameter from the API, if it
> wasn't for the loss of sanity checking.
>
> > Wrt creating the connector: Some helpers to make that easier could be
> > useful, and probably we need a separate ->register callback. That's needed
> > for proper demidlayered init/teardown sequence anyway, and then the
> > drm_bridge.c code make sure to only call ->register for the very last
> > bridge. Other bridges could still create ther connectors (less conditions
> > in the code), as long as they don't register them no one will ever see
> > them ;-)
One thing at a time, we'll get there :-)
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
