| Message ID | 20240513080243.3952292-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: adv7511: Attach next bridge without creating connector | expand |
Hi, 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. > > 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 > Indeed, looks safer finally. > Fixes: 14b3cdbd0e5b ("drm/bridge: adv7511: make it honour next bridge in DT") > Signed-off-by: Liu Ying <victor.liu@nxp.com> Acked-by: Sui Jingfeng <suijingfeng@bosc.ac.cn> > --- > 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); > if (ret) > return ret; > }
Hi, 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. > > 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.liu@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. 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. 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: <stable@vger.kernel.org> if you like. Thanks. > if (ret) > return ret; > }
On Mon, May 13, 2024 at 6:30 PM Sui Jingfeng <suijingfeng@bosc.ac.cn> wrote: > > Hi, > > > 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. > > > > 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.liu@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. > > 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. > > 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: <stable@vger.kernel.org> if you like. > Reviewed-by: Robert Foss <rfoss@kernel.org>
On Mon, 13 May 2024 at 10:55, Liu Ying <victor.liu@nxp.com> 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. > > 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.liu@nxp.com> > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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+renesas@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.liu@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: <stable@vger.kernel.org> if you like. > > > if (ret) > > return ret; > > }
Hi, On 5/14/24 23:12, Laurent Pinchart wrote: > 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. But what if there is a drm bridge prior to adv7511 but after the KMS engine? Even though we are still safe if it doesn't create connector by obeying modern rule. > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@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.liu@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. > The (next) bridge at the remote port is not necessary a display bridge. Or it is a bridge from the perspective of hardware viewpoint, but under controlled by a more complex foreign driver which generic drm bridge driver has no authority to attach. > 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. > OK, thanks for teaching us the modern way to use drm_bridge_connector helper, also thanks Ying for providing the patch. >> 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: <stable@vger.kernel.org> if you like. >> >>> if (ret) >>> return ret; >>> } >
Hi Sui, On Thu, May 16, 2024 at 12:13:03AM +0800, Sui Jingfeng wrote: > On 5/14/24 23:12, Laurent Pinchart wrote: > > 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. > > But what if there is a drm bridge prior to adv7511 but after the KMS > engine? Even though we are still safe if it doesn't create connector > by obeying modern rule. In the "old world", that bridge wouln't have created a connector, because it would detect that there's a next bridge. With modern KMS drivers, DRM_BRIDGE_ATTACH_NO_CONNECTOR is passed by the drm_bridge_connector helper to the very first bridge when attaching to it, so no bridge will create a connector. Modern bridge drivers should not support the !DRM_BRIDGE_ATTACH_NO_CONNECTOR case at all, they should not offer the option of creating a connector. > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@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.liu@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. > > The (next) bridge at the remote port is not necessary a display bridge. > Or it is a bridge from the perspective of hardware viewpoint, but under > controlled by a more complex foreign driver which generic drm bridge > driver has no authority to attach. > > > 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. > > OK, thanks for teaching us the modern way to use drm_bridge_connector > helper, also thanks Ying for providing the patch. > > >> 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: <stable@vger.kernel.org> if you like. > >> > >>> if (ret) > >>> return ret; > >>> }
On Mon, 13 May 2024 16:02:43 +0800, 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. > > [...] Applied to drm-misc-next-fixes, thanks! [1/1] drm/bridge: adv7511: Attach next bridge without creating connector commit: 20da948e3a807c67f0efe4f665e64728be370f3d Best regards,
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); if (ret) return ret; }
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. 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.liu@nxp.com> --- drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)