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@xxxxxxxxxxxxxxxx> > >>> --- > >>> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
