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 <[email protected]>
---
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;
}
--
2.34.1
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 <[email protected]>
Acked-by: Sui Jingfeng <[email protected]>
> ---
> 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;
> }
--
Best regards
Sui Jingfeng
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 <[email protected]>
> ---
> 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: <[email protected]> if you like.
Thanks.
> if (ret)
> return ret;
> }
--
Best regards
Sui Jingfeng
On Mon, May 13, 2024 at 6:30 PM Sui Jingfeng <[email protected]> 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 <[email protected]>
> > ---
> > 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: <[email protected]> if you like.
>
Reviewed-by: Robert Foss <[email protected]>
On Mon, 13 May 2024 at 10:55, Liu Ying <[email protected]> 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 <[email protected]>
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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: <[email protected]> if you like.
>
> > if (ret)
> > return ret;
> > }
--
Regards,
Laurent Pinchart
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 <[email protected]>
>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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: <[email protected]> if you like.
>>
>>> if (ret)
>>> return ret;
>>> }
>
--
Best regards
Sui
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 <[email protected]>
> >
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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: <[email protected]> if you like.
> >>
> >>> if (ret)
> >>> return ret;
> >>> }
--
Regards,
Laurent Pinchart
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,
--
With best wishes
Dmitry