Hi Andrzej, On Thu, Aug 08, 2019 at 07:36:49PM +0200, Andrzej Hajda wrote: > On 08.08.2019 16:25, Laurent Pinchart wrote: > > 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. Makes sense. I think I would however still set the ->next field, in order to track bridges, and only skip chaining of the enable/disable operations. > >> 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. If we can agree on that, that would be wonderful ! > 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. > ... My best answer to this is: one step at a time :-) Yes, that's the direction I want to take (there will of course be lots of bikeshedding on the details), and this series is one step in that direction. What I'm really trying to do here is simplify bridge implementations by creating a clear API that shrinks as much as possible the duties of bridge drivers. This moves existing complexity outside of bridges, and we of course don't want to duplicate it in all display controller drivers, so I'm also creating helpers that centralise the complexity. Their use is not mandatory (Daniel has hammered the "no midlayer" message enough), but for the vast majority of display controllers, it should greatly simplify the code. My next planned steps are: - Convert more bridge drivers to support DRM_BRIDGE_FLAG_NO_CONNECTOR - Convert more display controller drivers to use DRM_BRIDGE_FLAG_NO_CONNECTOR - Drop support for !DRM_BRIDGE_FLAG_NO_CONNECTOR in bridge drivers once they are only used by display controllers that set DRM_BRIDGE_FLAG_NO_CONNECTOR In parallel, I want to address drm_panel ad drm_panel_bridge. With the extension of the bridge API to expose connector-related operations done in this series, the distinction between panel and bridges becomes much thinner, opening the door to lots of simplifications. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
