[PATCH 08/60] drm/bridge: Extend bridge API to disable connector creation
Daniel Vetter
daniel at ffwll.ch
Wed Aug 14 08:18:40 UTC 2019
On Thu, Aug 08, 2019 at 07:36:49PM +0200, Andrzej Hajda wrote:
> On 08.08.2019 16:25, Laurent Pinchart wrote:
> > Hi Andrzej,
> >
> > On Wed, Jul 17, 2019 at 08:39:47AM +0200, Andrzej Hajda wrote:
> >> On 07.07.2019 20:18, Laurent Pinchart wrote:
> >>> Most bridge drivers create a DRM connector to model the connector at the
> >>> output of the bridge. This model is historical and has worked pretty
> >>> well so far, but causes several issues:
> >>>
> >>> - It prevents supporting more complex display pipelines where DRM
> >>> connector operations are split over multiple components. For instance a
> >>> pipeline with a bridge connected to the DDC signals to read EDID data,
> >>> and another one connected to the HPD signal to detect connection and
> >>> disconnection, will not be possible to support through this model.
> >>>
> >>> - It requires every bridge driver to implement similar connector
> >>> handling code, resulting in code duplication.
> >>>
> >>> - It assumes that a bridge will either be wired to a connector or to
> >>> another bridge, but doesn't support bridges that can be used in both
> >>> positions very well (although there is some ad-hoc support for this in
> >>> the analogix_dp bridge driver).
> >>>
> >>> In order to solve these issues, ownership of the connector should be
> >>> moved to the display controller driver (where it can be implemented
> >>> using helpers provided by the core).
> >>>
> >>> Extend the bridge API to allow disabling connector creation in bridge
> >>> drivers as a first step towards the new model. The new create_connector
> >>> argument to the bridge .attach() operation tells the bridge driver
> >>> whether to create a connector. Set the argument to true unconditionally,
> >>> and modify all existing bridge drivers to return an error when connector
> >>> creation is not requested as they don't support this feature yet.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>> drivers/gpu/drm/arc/arcpgu_hdmi.c | 2 +-
> >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 2 +-
> >>> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++-
> >>> drivers/gpu/drm/bridge/analogix-anx78xx.c | 6 +++++-
> >>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 ++++++--
> >>> drivers/gpu/drm/bridge/cdns-dsi.c | 6 ++++--
> >>> drivers/gpu/drm/bridge/lvds-encoder.c | 4 ++--
> >>> drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 6 +++++-
> >>> drivers/gpu/drm/bridge/nxp-ptn3460.c | 6 +++++-
> >>> drivers/gpu/drm/bridge/panel.c | 5 ++++-
> >>> drivers/gpu/drm/bridge/parade-ps8622.c | 5 ++++-
> >>> drivers/gpu/drm/bridge/sii902x.c | 6 +++++-
> >>> drivers/gpu/drm/bridge/sil-sii8620.c | 2 +-
> >>> drivers/gpu/drm/bridge/simple-bridge.c | 6 +++++-
> >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++--
> >>> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 8 +++++---
> >>> drivers/gpu/drm/bridge/tc358764.c | 5 ++++-
> >>> drivers/gpu/drm/bridge/tc358767.c | 5 ++++-
> >>> drivers/gpu/drm/bridge/thc63lvd1024.c | 5 +++--
> >>> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 ++++-
> >>> drivers/gpu/drm/bridge/ti-tfp410.c | 5 ++++-
> >>> drivers/gpu/drm/drm_bridge.c | 5 +++--
> >>> drivers/gpu/drm/drm_simple_kms_helper.c | 2 +-
> >>> drivers/gpu/drm/exynos/exynos_dp.c | 3 ++-
> >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 ++--
> >>> drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +-
> >>> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 2 +-
> >>> drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 2 +-
> >>> drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++--
> >>> drivers/gpu/drm/imx/imx-ldb.c | 2 +-
> >>> drivers/gpu/drm/imx/parallel-display.c | 2 +-
> >>> drivers/gpu/drm/mcde/mcde_dsi.c | 6 +++++-
> >>> drivers/gpu/drm/mediatek/mtk_dpi.c | 2 +-
> >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +-
> >>> drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 ++++++--
> >>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 4 ++--
> >>> drivers/gpu/drm/msm/edp/edp_bridge.c | 2 +-
> >>> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +-
> >>> drivers/gpu/drm/omapdrm/omap_drv.c | 3 ++-
> >>> drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 2 +-
> >>> drivers/gpu/drm/rcar-du/rcar_lvds.c | 7 +++++--
> >>> drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 +-
> >>> drivers/gpu/drm/rockchip/rockchip_rgb.c | 2 +-
> >>> drivers/gpu/drm/sti/sti_dvo.c | 2 +-
> >>> drivers/gpu/drm/sti/sti_hda.c | 2 +-
> >>> drivers/gpu/drm/sti/sti_hdmi.c | 2 +-
> >>> drivers/gpu/drm/stm/ltdc.c | 2 +-
> >>> drivers/gpu/drm/sun4i/sun4i_lvds.c | 2 +-
> >>> drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +-
> >>> drivers/gpu/drm/tilcdc/tilcdc_external.c | 2 +-
> >>> drivers/gpu/drm/vc4/vc4_dpi.c | 2 +-
> >>> drivers/gpu/drm/vc4/vc4_dsi.c | 2 +-
> >>> include/drm/drm_bridge.h | 4 ++--
> >>> 53 files changed, 140 insertions(+), 67 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/arc/arcpgu_hdmi.c b/drivers/gpu/drm/arc/arcpgu_hdmi.c
> >>> index 98aac743cc26..739f2358f1d5 100644
> >>> --- a/drivers/gpu/drm/arc/arcpgu_hdmi.c
> >>> +++ b/drivers/gpu/drm/arc/arcpgu_hdmi.c
> >>> @@ -39,7 +39,7 @@ int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np)
> >>> return ret;
> >>>
> >>> /* Link drm_bridge to encoder */
> >>> - ret = drm_bridge_attach(encoder, bridge, NULL);
> >>> + ret = drm_bridge_attach(encoder, bridge, NULL, true);
> >> Few suggestions:
> >>
> >> 1. Maybe it would be more convenient to add flags argument instead of bool:
> >>
> >> - code should be more readable: ret = drm_bridge_attach(encoder, bridge,
> >> NULL, DRM_BRIDGE_FLAG_NO_CONNECTOR)
> >>
> >> - it can be easily expanded later with other flags, there at least two
> >> drivers which would benefit from DRM_BRIDGE_FLAG_NO_CHAINING flag.
> > Please note that I think this flag should disappear once drivers get
> > converted to the new model. This will however take some time. I'm not
> > opposed to turning the book into a flag though. I was hoping to receive
> > more review comments on this particular patch, but as that's not the
> > case, I can already proceed with the change.
> >
> > What would the DRM_BRIDGE_FLAG_NO_CHAINING flag be used for ?
>
>
> To avoid setting encoder->bridge or previous_bridge->next field to
> attached bridge (last lines of drm_bridge_attach code).
>
> This is for sure the case of exynos_dsi and vc4_dsi, but I guess it can
> affect other drivers as well, probably they just use other workarounds
> or have more flexible hardware.
>
> Generally idea that order of calling
> pre_enable/enable/disable/post_disable callbacks in chained
> encoder/bridges is fixed is wrong IMHO, only video source component
> knows in which moment it should enable its sink and if it should do
> something after. And it does not work at all with sources/sinks having
> more than one output/input.
This doesn't make sense to me ... if you don't want to have a chained
bridge, don't attach it? You can just open-code the attach and call the
bridge functions directly when appropriate. I think that's the better
long-term plan than trying to have flags for every possible topology
existing out there ...
-Daniel
>
>
> >
> >> 2. If the patch can be applied atomically it is OK as is, if not you can
> >> use preprocessor vararg magic to support new and old syntax, sth like:
> >>
> >> #define _drm_bridge_attach(encoder, bridge, prev, flags, optarg...)
> >> __drm_bridge_attach(encoder, bridge, prev, flags)
> >>
> >> #define drm_bridge_attach(encoder, bridge, prev, optarg...)
> >> _drm_bridge_attach(encoder, bridge, prev, ##optarg, 0)
> > Good point. I'll try to do this atomically, but if it fails I'll follow
> > your suggestion.
> >
> >> 3. Maybe more convenient would be to just set the flags directly before
> >> attachment:
> >>
> >> bridge->dont_create_connector = true;
> >>
> >> ret = drm_bridge_attach(encoder, bridge, NULL);
> >>
> >> This way it will be still expandable, and less changes.
> > Bridges that are chained would need to set the dont_create_connector
> > flag of the next bridge. It would be a bit ugly, but would make this
> > patch smaller. On the other hand we would need to keep the if
> > (!create_connector) check in the .attach() handlers, and it would be
> > easier to miss it in bridge drivers (current or new) than with an
> > explicit argument to the .attach() operation. I would thus have a
> > preference for the new argument to .attach(). Especially if it can help
> > you with DRM_BRIDGE_FLAG_NO_CHAINING :-)
>
> bridge->dont_chain would work as well :)
>
> Btw I wonder if it could be possible to disallow creating connectors at all by new bridges - it would speed-up transition.
>
>
> Another long term idea. Since bridges can be attached to:
> - encoder,
> - another bridge,
> - crtc (I have one example, but I guess there could be more),
> - even before crtc (image postprocessing)
> And since bridge output goes to:
> - another bridge,
> - panel.
>
> Wouldn't be better to create drm_source and drm_sink (do not respond with xkcd picture :) ):
> - drm_source will be embedded in source device context,
> - drm_sink will be embedded in sink device context.
> We could make then transitions of bridges to drm_sink with drm_source embeded in its context, and panels to drm_sink.
> This way we could drop these crazy constructs:
> - if sink is panel then do sth, elsif is bridge then do sth_else,
> - if src is bridge then do sth, elsif is encoder ... elsif ....
> - helpers of_find_panel_or_bridge,
> - drm_panel_bridge,
> Also we could implement easily multi input/output bridges/panels/crtcs whatever.
> And hpd callbacks you have proposed in another patch would fit better to drm_source.ops.
> ...
>
>
> Regards
> Andrzej
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list