| Message ID | 20241231192925.97614-3-marex@denx.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/4] drm: bridge: dw_hdmi: Add flag to indicate output port is optional | expand |
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@linaro.org> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Fabio Estevam <festevam@gmail.com> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Liu Ying <victor.liu@nxp.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Robert Foss <rfoss@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Simona Vetter <simona@ffwll.ch> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > Cc: imx@lists.linux.dev > Cc: linux-arm-kernel@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 >
On 1/2/25 6:58 PM, Dmitry Baryshkov wrote: [...] >> @@ -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? By the HDMI bridge driver, see 1/4. > 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. So what exactly should I do here ? If dw_hdmi_parse_dt() only exits with 0 if there is no connector node in DT, I don't get any output on the HDMI. I have to create a connector in the HDMI bridge driver instead and not here, right ?
On Fri, Jan 03, 2025 at 12:20:19AM +0100, Marek Vasut wrote: > On 1/2/25 6:58 PM, Dmitry Baryshkov wrote: > > [...] > > > > @@ -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? > > By the HDMI bridge driver, see 1/4. > > > 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. > So what exactly should I do here ? > > If dw_hdmi_parse_dt() only exits with 0 if there is no connector node in DT, > I don't get any output on the HDMI. I have to create a connector in the HDMI > bridge driver instead and not here, right ? No. Please make dw_hdmi_parse_dt() return 0 if there is no connector and the flag is set. Then create drm_bridge_connector here.
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 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 */ + 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;
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@linaro.org> Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: David Airlie <airlied@gmail.com> Cc: Fabio Estevam <festevam@gmail.com> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Liu Ying <victor.liu@nxp.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Robert Foss <rfoss@kernel.org> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Simona Vetter <simona@ffwll.ch> Cc: Stefan Agner <stefan@agner.ch> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org Cc: imx@lists.linux.dev Cc: linux-arm-kernel@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(-)