Re: [PATCH 08/60] drm/bridge: Extend bridge API to disable connector creation

From: Daniel Vetter <daniel@ffwll.ch>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	dri-devel@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 08/60] drm/bridge: Extend bridge API to disable connector creation
Date: Wed, 14 Aug 2019 14:24:35 +0200	[thread overview]
Message-ID: <20190814122435.GB7444@phenom.ffwll.local> (raw)
In-Reply-To: <20190814095529.GB5009@pendragon.ideasonboard.com>

On Wed, Aug 14, 2019 at 12:55:29PM +0300, Laurent Pinchart wrote:
> On Wed, Aug 14, 2019 at 10:18:40AM +0200, Daniel Vetter wrote:
> > 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@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 ...
> 
> I think the whole topology should be known to the display driver, so the
> next (and possibly previous if we later need it) pointers should be set
> in all cases. Otherwise we can't, for instance, notify all bridges of
> HPD events. Sure, a bridge could decide not to attach at all, but then
> it would have to reimplement lots of logic to forward everything
> manually, when the need for manual forwarding is usually limited to a
> certain category of operations.

That's kinda what I meant, if you have special topology it's going to be
more typing. Once we have more examples then we can lift that to helpers
and make it generic. But setting some kind of topology that isn't correct
feels a bit fishy ...

Another option is that we have an overall function for enable/disable,
with he current helpers as default implementations. But at that point you
might as well type your own I think.
-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
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel