Hi Daniel,
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>
> >>> ---
> >>>
> >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 4 +-
> >>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 4 +-
> >>> drivers/gpu/drm/bridge/dw-hdmi.c | 3 +-
> >>> drivers/gpu/drm/drm_bridge.c | 46 +++++++++----
> >>> drivers/gpu/drm/drm_simple_kms_helper.c | 4 +-
> >>> drivers/gpu/drm/exynos/exynos_dp.c | 5 +--
> >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 6 +--
> >>> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 5 +--
> >>> drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 5 +--
> >>> drivers/gpu/drm/imx/imx-ldb.c | 6 +--
> >>> drivers/gpu/drm/imx/parallel-display.c | 4 +-
> >>> drivers/gpu/drm/mediatek/mtk_dpi.c | 8 ++--
> >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 24 ++---------
> >>> drivers/gpu/drm/mediatek/mtk_hdmi.c | 11 +++---
> >>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 17 +++++---
> >>> drivers/gpu/drm/msm/edp/edp_bridge.c | 2 +-
> >>> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +-
> >>> drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c | 5 +--
> >>> drivers/gpu/drm/sti/sti_dvo.c | 3 +-
> >>> drivers/gpu/drm/sti/sti_hda.c | 3 +-
> >>> drivers/gpu/drm/sti/sti_hdmi.c | 3 +-
> >>> drivers/gpu/drm/sun4i/sun4i_rgb.c | 13 +++---
> >>> include/drm/drm_bridge.h | 3 +-
> >>> 23 files changed, 83 insertions(+), 103 deletions(-)
> >
> > [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 ;-)
--
Regards,
Laurent Pinchart
