[PATCH v3 3/4] drm/lcdif: add DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to drm_bridge_attach

[Dmitry Baryshkov]

On Tue, Dec 31, 2024 at 08:28:50PM +0100, Marek Vasut wrote:
> Commit a25b988ff83f ("drm/bridge: Extend bridge API to disable connector creation")
> added DRM_BRIDGE_ATTACH_NO_CONNECTOR bridge flag and all bridges handle
> this flag in some way since then.
> Newly added bridge drivers must no longer contain the connector creation and
> will fail probing if this flag isn't set.
> 
> In order to be able to connect to those newly added bridges as well,
> make use of drm_bridge_connector API and have the connector initialized
> by the display controller.
> 
> Based on 2e87bf389e13 ("drm/rockchip: add DRM_BRIDGE_ATTACH_NO_CONNECTOR flag to drm_bridge_attach")
> 
> This makes LT9611 work with i.MX8M Plus.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> Signed-off-by: Marek Vasut <marex at denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
> Cc: David Airlie <airlied at gmail.com>
> Cc: Fabio Estevam <festevam at gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec at gmail.com>
> Cc: Jonas Karlman <jonas at kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> Cc: Liu Ying <victor.liu at nxp.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Neil Armstrong <neil.armstrong at linaro.org>
> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
> Cc: Robert Foss <rfoss at kernel.org>
> Cc: Sascha Hauer <s.hauer at pengutronix.de>
> Cc: Shawn Guo <shawnguo at kernel.org>
> Cc: Simona Vetter <simona at ffwll.ch>
> Cc: Stefan Agner <stefan at agner.ch>
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: dri-devel at lists.freedesktop.org
> Cc: imx at lists.linux.dev
> Cc: linux-arm-kernel at lists.infradead.org
> ---
> V2: Add RB from Dmitry
> V3: - Select DRM_DISPLAY_HELPER
>     - Use return dev_err_probe() directly
>     - Add missing of_node_put(ep)
>     - Add test using drm_bridge_get_next_bridge() to try and determine
>       if the HDMI connector was missing in DT or not, and if it was
>       missing, if it was created by the HDMI bridge driver.
> ---
>  drivers/gpu/drm/mxsfb/Kconfig     |  2 ++
>  drivers/gpu/drm/mxsfb/lcdif_drv.c | 30 ++++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
> index 264e74f455547..fe98674d5a298 100644
> --- a/drivers/gpu/drm/mxsfb/Kconfig
> +++ b/drivers/gpu/drm/mxsfb/Kconfig
> @@ -12,6 +12,7 @@ config DRM_MXSFB
>  	select DRM_CLIENT_SELECTION
>  	select DRM_MXS
>  	select DRM_KMS_HELPER
> +	select DRM_BRIDGE_CONNECTOR

Shouldn't this chunk go to another patch?

>  	select DRM_GEM_DMA_HELPER
>  	select DRM_PANEL
>  	select DRM_PANEL_BRIDGE
> @@ -28,6 +29,7 @@ config DRM_IMX_LCDIF
>  	depends on COMMON_CLK
>  	depends on ARCH_MXC || COMPILE_TEST
>  	select DRM_CLIENT_SELECTION
> +	select DRM_DISPLAY_HELPER
>  	select DRM_MXS
>  	select DRM_KMS_HELPER
>  	select DRM_GEM_DMA_HELPER
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> index 8ee00f59ca821..dc39adabb3cd9 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -17,6 +17,7 @@
>  #include <drm/clients/drm_client_setup.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_fbdev_dma.h>
> @@ -48,8 +49,10 @@ static const struct drm_encoder_funcs lcdif_encoder_funcs = {
>  static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
>  {
>  	struct device *dev = lcdif->drm->dev;
> +	struct drm_device *drm = lcdif->drm;
> +	struct drm_connector *connector;
>  	struct device_node *ep;
> -	struct drm_bridge *bridge;
> +	struct drm_bridge *bridge, *nextbridge;
>  	int ret;
>  
>  	for_each_endpoint_of_node(dev->of_node, ep) {
> @@ -97,13 +100,36 @@ static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
>  			return ret;
>  		}
>  
> -		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> +		ret = drm_bridge_attach(encoder, bridge, NULL,
> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>  		if (ret) {
>  			of_node_put(ep);
>  			return dev_err_probe(dev, ret,
>  					     "Failed to attach bridge for endpoint%u\n",
>  					     of_ep.id);
>  		}
> +
> +		nextbridge = drm_bridge_get_next_bridge(bridge);
> +		nextbridge = drm_bridge_get_next_bridge(nextbridge);
> +		/* Test if connector node in DT, if not, it was created already */

By whom? And why? There is no display-connector bridge, but there is a
normal bridge chain, you have passed DRM_BRIDGE_ATTACH_NO_CONNECTOR, so
now it's a proper time to create drm_bridge_connector. You have added
the next_bridge_optional flag, but it should just prevent the dw driver
from returning the error if there is no next_bridge.

> +		if (!nextbridge)
> +			continue;
> +
> +		connector = drm_bridge_connector_init(drm, encoder);
> +		if (IS_ERR(connector)) {
> +			of_node_put(ep);
> +			return dev_err_probe(drm->dev, PTR_ERR(connector),
> +					     "Failed to initialize bridge connector: %pe\n",
> +					     connector);
> +		}
> +
> +		ret = drm_connector_attach_encoder(connector, encoder);
> +		if (ret < 0) {
> +			of_node_put(ep);
> +			drm_connector_cleanup(connector);
> +			return dev_err_probe(drm->dev, ret,
> +					     "Failed to attach encoder.\n");
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.45.2
> 

-- 
With best wishes
Dmitry