* Re: drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-13 8:02 [PATCH] drm/bridge: adv7511: Attach next bridge without creating connector Liu Ying
@ 2024-05-13 15:49 ` Sui Jingfeng
2024-05-13 16:26 ` Sui Jingfeng
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Sui Jingfeng @ 2024-05-13 15:49 UTC (permalink / raw)
To: Liu Ying, dri-devel, linux-kernel
Cc: andrzej.hajda, neil.armstrong, rfoss, Laurent.pinchart, jonas,
jernej.skrabec, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, dmitry.baryshkov, biju.das.jz, aford173, bli, robh,
jani.nikula
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;
> }
--
Best regards
Sui Jingfeng
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-13 8:02 [PATCH] drm/bridge: adv7511: Attach next bridge without creating connector Liu Ying
2024-05-13 15:49 ` Sui Jingfeng
@ 2024-05-13 16:26 ` Sui Jingfeng
2024-05-13 16:49 ` Robert Foss
2024-05-14 15:12 ` Laurent Pinchart
2024-05-13 16:58 ` [PATCH] " Dmitry Baryshkov
2024-05-19 21:25 ` Dmitry Baryshkov
3 siblings, 2 replies; 9+ messages in thread
From: Sui Jingfeng @ 2024-05-13 16:26 UTC (permalink / raw)
To: Liu Ying, dri-devel, linux-kernel
Cc: andrzej.hajda, neil.armstrong, rfoss, Laurent.pinchart, jonas,
jernej.skrabec, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, dmitry.baryshkov, biju.das.jz, aford173, bli, robh,
jani.nikula
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;
> }
--
Best regards
Sui Jingfeng
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-13 16:26 ` Sui Jingfeng
@ 2024-05-13 16:49 ` Robert Foss
2024-05-14 15:12 ` Laurent Pinchart
1 sibling, 0 replies; 9+ messages in thread
From: Robert Foss @ 2024-05-13 16:49 UTC (permalink / raw)
To: Sui Jingfeng
Cc: Liu Ying, dri-devel, linux-kernel, andrzej.hajda, neil.armstrong,
Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
mripard, tzimmermann, airlied, daniel, dmitry.baryshkov,
biju.das.jz, aford173, bli, robh, jani.nikula
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-13 16:26 ` Sui Jingfeng
2024-05-13 16:49 ` Robert Foss
@ 2024-05-14 15:12 ` Laurent Pinchart
2024-05-15 16:13 ` Sui Jingfeng
1 sibling, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2024-05-14 15:12 UTC (permalink / raw)
To: Sui Jingfeng
Cc: Liu Ying, dri-devel, linux-kernel, andrzej.hajda, neil.armstrong,
rfoss, jonas, jernej.skrabec, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, dmitry.baryshkov, biju.das.jz,
aford173, bli, robh, jani.nikula
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;
> > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-14 15:12 ` Laurent Pinchart
@ 2024-05-15 16:13 ` Sui Jingfeng
2024-05-16 12:04 ` Laurent Pinchart
0 siblings, 1 reply; 9+ messages in thread
From: Sui Jingfeng @ 2024-05-15 16:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Liu Ying, dri-devel, linux-kernel, andrzej.hajda, neil.armstrong,
rfoss, jonas, jernej.skrabec, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, dmitry.baryshkov, biju.das.jz,
aford173, bli, robh, jani.nikula
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;
>>> }
>
--
Best regards
Sui
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-15 16:13 ` Sui Jingfeng
@ 2024-05-16 12:04 ` Laurent Pinchart
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2024-05-16 12:04 UTC (permalink / raw)
To: Sui Jingfeng
Cc: Liu Ying, dri-devel, linux-kernel, andrzej.hajda, neil.armstrong,
rfoss, jonas, jernej.skrabec, maarten.lankhorst, mripard,
tzimmermann, airlied, daniel, dmitry.baryshkov, biju.das.jz,
aford173, bli, robh, jani.nikula
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;
> >>> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-13 8:02 [PATCH] drm/bridge: adv7511: Attach next bridge without creating connector Liu Ying
2024-05-13 15:49 ` Sui Jingfeng
2024-05-13 16:26 ` Sui Jingfeng
@ 2024-05-13 16:58 ` Dmitry Baryshkov
2024-05-19 21:25 ` Dmitry Baryshkov
3 siblings, 0 replies; 9+ messages in thread
From: Dmitry Baryshkov @ 2024-05-13 16:58 UTC (permalink / raw)
To: Liu Ying
Cc: dri-devel, linux-kernel, andrzej.hajda, neil.armstrong, rfoss,
Laurent.pinchart, jonas, jernej.skrabec, maarten.lankhorst,
mripard, tzimmermann, airlied, daniel, biju.das.jz, aford173, bli,
robh, jani.nikula
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>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drm/bridge: adv7511: Attach next bridge without creating connector
2024-05-13 8:02 [PATCH] drm/bridge: adv7511: Attach next bridge without creating connector Liu Ying
` (2 preceding siblings ...)
2024-05-13 16:58 ` [PATCH] " Dmitry Baryshkov
@ 2024-05-19 21:25 ` Dmitry Baryshkov
3 siblings, 0 replies; 9+ messages in thread
From: Dmitry Baryshkov @ 2024-05-19 21:25 UTC (permalink / raw)
To: dri-devel, linux-kernel, Liu Ying
Cc: andrzej.hajda, neil.armstrong, rfoss, Laurent.pinchart, jonas,
jernej.skrabec, maarten.lankhorst, mripard, tzimmermann, airlied,
daniel, biju.das.jz, aford173, bli, robh, jani.nikula
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
^ permalink raw reply [flat|nested] 9+ messages in thread