Hello,
On Tue, May 14, 2024 at 12:26:15AM +0800, Sui Jingfeng wrote:
> On 5/13/24 16:02, Liu Ying wrote:
> > The connector is created by either this ADV7511 bridge driver or
> > any DRM device driver/previous bridge driver, so this ADV7511
> > bridge driver should not let the next bridge driver create connector.
> >
> > If the next bridge is a HDMI connector, the next bridge driver
> > would fail to attach bridge from display_connector_attach() without
> > the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
In theory we could have another HDMI-to-something bridge connected to
the ADV7511 output, and that bridge could create a connector. However,
before commit 14b3cdbd0e5b the adv7511 driver didn't try to attach to
the next bridge, so it's clear that no platform support in mainline had
such a setup. It should be safe to set DRM_BRIDGE_ATTACH_NO_CONNECTOR
unconditionally here.
Reviewed-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
> >
> > Add that flag to drm_bridge_attach() function call in
> > adv7511_bridge_attach() to fix the issue.
> >
> > This fixes the issue where the HDMI connector bridge fails to attach
> > to the previous ADV7535 bridge on i.MX8MP EVK platform:
> >
> > [ 2.216442] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > /hdmi-connector to encoder None-37: -22
> > [ 2.220675] mmc1: SDHCI controller on 30b50000.mmc [30b50000.mmc] using
> > ADMA
> > [ 2.226262] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > /soc@0/bus@30800000/i2c@30a30000/hdmi@3d to encoder None-37: -22
> > [ 2.245204] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> > /soc@0/bus@32c00000/dsi@32e60000 to encoder None-37: -22
> > [ 2.256445] imx-lcdif 32e80000.display-controller: error -EINVAL: Failed
> > to attach bridge for endpoint0
> > [ 2.265850] imx-lcdif 32e80000.display-controller: error -EINVAL: Cannot
> > connect bridge
> > [ 2.274009] imx-lcdif 32e80000.display-controller: probe with driver
> > imx-lcdif failed with error -22
> >
> > Fixes: 14b3cdbd0e5b ("drm/bridge: adv7511: make it honour next bridge in
> > DT")
> > Signed-off-by: Liu Ying <victor....@nxp.com>
> > ---
> > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index dd21b81bd28f..66ccb61e2a66 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -953,7 +953,8 @@ static int adv7511_bridge_attach(struct drm_bridge
> > *bridge,
> > int ret = 0;
> >
> > if (adv->next_bridge) {
> > - ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
> > bridge, flags);
> > + ret = drm_bridge_attach(bridge->encoder, adv->next_bridge,
> > bridge,
> > + flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>
> As a side note, I think, maybe you could do better in the future.
>
> If we know that the KMS display driver side has the HDMI connector
> already created for us, we should pass DRM_BRIDGE_ATTACH_NO_CONNECTOR
> from the root KMS driver side. Which is to forbidden all potential
> drm bridge drivers to create a connector in the middle.
That's the recommended way for new drivers. Using the
drm_bridge_connector helper handles all this for you.
> The KMS display driver side could parse the DT to know if there is
> a hdmi connector, or merely just hdmi connector device node, or
> something else.
No, that would violate the basic principle of not peeking into the DT of
devices you know nothing about. The display engine driver can't walk the
pipeline in DT and expect to understand all the DT nodes on the path,
and what their properties mean.
What KMS drivers should do is to use the drm_bridge_connector helper.
Calling drm_bridge_connector_init() will create a connector for a chain
of bridges. The KMS driver should then attach to the first bridge with
DRM_BRIDGE_ATTACH_NO_CONNECTOR, unconditionally.
> However, other maintainer and/or reviewer's opinion are of cause
> more valuable. I send a A-b because I thought the bug is urgency
> and it's probably more important to solve this bug first. And
> maybe you can Cc: <sta...@vger.kernel.org> if you like.
>
> > if (ret)
> > return ret;
> > }
--
Regards,
Laurent Pinchart
