[PATCH] drm/ingenic: Allocate and register the internal bridge separately

[PATCH] drm/ingenic: Allocate and register the internal bridge separately

[Florian Fainelli]

The LCD controller's internal bridge is embedded in an object allocated
with drmm_encoder_alloc(). This initializes the encoder but leaves the
bridge's reference count at zero. Since drm_bridge_attach() takes a
reference, probing the CI20 display produces addition-on-zero and
underflow warnings and fails to attach the HDMI bridge chain.

Allocate the internal bridge with devm_drm_bridge_alloc() and register
it with devm_drm_bridge_add() before attaching it. Keep the encoder in a
separate DRM-managed allocation: encoder cleanup can drop the final
bridge reference before it has finished accessing the encoder. Find
the internal bridge through the encoder's bridge chain during mode
setting, and use the bridge argument directly in bridge callbacks.

Fixes: 94d50c1a2ca3 ("drm/bridge: get/put the bridge reference in drm_bridge_attach/detach()")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 31 +++++++++++++++--------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 625ec76cf04d..520912f9ed0d 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -121,16 +121,15 @@ struct ingenic_drm {
 };
 
 struct ingenic_drm_bridge {
-	struct drm_encoder encoder;
 	struct drm_bridge bridge, *next_bridge;
 
 	struct drm_bus_cfg bus_cfg;
 };
 
 static inline struct ingenic_drm_bridge *
-to_ingenic_drm_bridge(struct drm_encoder *encoder)
+to_ingenic_drm_bridge(struct drm_bridge *bridge)
 {
-	return container_of(encoder, struct ingenic_drm_bridge, encoder);
+	return container_of(bridge, struct ingenic_drm_bridge, bridge);
 }
 
 static inline struct ingenic_drm_private_state *
@@ -738,7 +737,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 {
 	struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
-	struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
+	struct drm_bridge *first_bridge __free(drm_bridge_put) =
+		drm_bridge_chain_get_first_bridge(encoder);
+	struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(first_bridge);
 	unsigned int cfg, rgbcfg = 0;
 
 	priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
@@ -799,7 +800,7 @@ static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
 				     struct drm_encoder *encoder,
 				     enum drm_bridge_attach_flags flags)
 {
-	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(encoder);
+	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
 
 	return drm_bridge_attach(encoder, ib->next_bridge,
 				 &ib->bridge, flags);
@@ -811,7 +812,7 @@ static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
 					   struct drm_connector_state *conn_state)
 {
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
-	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
+	struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
 
 	ib->bus_cfg = bridge_state->output_bus_cfg;
 
@@ -1303,22 +1304,32 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			drm_panel_put(panel);
 		}
 
-		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
-					NULL, DRM_MODE_ENCODER_DPI, NULL);
+		ib = devm_drm_bridge_alloc(dev, struct ingenic_drm_bridge, bridge,
+					   &ingenic_drm_bridge_funcs);
 		if (IS_ERR(ib)) {
 			ret = PTR_ERR(ib);
+			dev_err(dev, "Failed to allocate bridge: %d\n", ret);
+			goto err_drvdata;
+		}
+
+		encoder = drmm_plain_encoder_alloc(drm, NULL,
+						   DRM_MODE_ENCODER_DPI, NULL);
+		if (IS_ERR(encoder)) {
+			ret = PTR_ERR(encoder);
 			dev_err(dev, "Failed to init encoder: %d\n", ret);
 			goto err_drvdata;
 		}
 
-		encoder = &ib->encoder;
 		encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
 
 		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
 
-		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
 		ib->next_bridge = bridge;
 
+		ret = devm_drm_bridge_add(dev, &ib->bridge);
+		if (ret)
+			goto err_drvdata;
+
 		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
 					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 		if (ret) {
-- 
2.53.0

Re: [PATCH] drm/ingenic: Allocate and register the internal bridge separately

[H. Nikolaus Schaller]

Hi,

> Am 05.11.2026 um 21:35 schrieb Florian Fainelli <f.fainelli@gmail.com>:
> 
> The LCD controller's internal bridge is embedded in an object allocated
> with drmm_encoder_alloc(). This initializes the encoder but leaves the
> bridge's reference count at zero. Since drm_bridge_attach() takes a
> reference, probing the CI20 display produces addition-on-zero and
> underflow warnings and fails to attach the HDMI bridge chain.
> 
> Allocate the internal bridge with devm_drm_bridge_alloc() and register
> it with devm_drm_bridge_add() before attaching it. Keep the encoder in a
> separate DRM-managed allocation: encoder cleanup can drop the final
> bridge reference before it has finished accessing the encoder. Find
> the internal bridge through the encoder's bridge chain during mode
> setting, and use the bridge argument directly in bridge callbacks.
> 
> Fixes: 94d50c1a2ca3 ("drm/bridge: get/put the bridge reference in drm_bridge_attach/detach()")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 31 +++++++++++++++--------
> 1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 625ec76cf04d..520912f9ed0d 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -121,16 +121,15 @@ struct ingenic_drm {
> };
> 
> struct ingenic_drm_bridge {
> - struct drm_encoder encoder;
> struct drm_bridge bridge, *next_bridge;
> 
> struct drm_bus_cfg bus_cfg;
> };
> 
> static inline struct ingenic_drm_bridge *
> -to_ingenic_drm_bridge(struct drm_encoder *encoder)
> +to_ingenic_drm_bridge(struct drm_bridge *bridge)
> {
> - return container_of(encoder, struct ingenic_drm_bridge, encoder);
> + return container_of(bridge, struct ingenic_drm_bridge, bridge);
> }
> 
> static inline struct ingenic_drm_private_state *
> @@ -738,7 +737,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
> {
> struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> - struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
> + struct drm_bridge *first_bridge __free(drm_bridge_put) =
> + drm_bridge_chain_get_first_bridge(encoder);
> + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(first_bridge);
> unsigned int cfg, rgbcfg = 0;
> 
> priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> @@ -799,7 +800,7 @@ static int ingenic_drm_bridge_attach(struct drm_bridge *bridge,
>     struct drm_encoder *encoder,
>     enum drm_bridge_attach_flags flags)
> {
> - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(encoder);
> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
> 
> return drm_bridge_attach(encoder, ib->next_bridge,
> &ib->bridge, flags);
> @@ -811,7 +812,7 @@ static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge,
>   struct drm_connector_state *conn_state)
> {
> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
> 
> ib->bus_cfg = bridge_state->output_bus_cfg;
> 
> @@ -1303,22 +1304,32 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> drm_panel_put(panel);
> }
> 
> - ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
> - NULL, DRM_MODE_ENCODER_DPI, NULL);
> + ib = devm_drm_bridge_alloc(dev, struct ingenic_drm_bridge, bridge,
> +   &ingenic_drm_bridge_funcs);
> if (IS_ERR(ib)) {
> ret = PTR_ERR(ib);
> + dev_err(dev, "Failed to allocate bridge: %d\n", ret);
> + goto err_drvdata;
> + }
> +
> + encoder = drmm_plain_encoder_alloc(drm, NULL,
> +   DRM_MODE_ENCODER_DPI, NULL);
> + if (IS_ERR(encoder)) {
> + ret = PTR_ERR(encoder);
> dev_err(dev, "Failed to init encoder: %d\n", ret);
> goto err_drvdata;
> }
> 
> - encoder = &ib->encoder;
> encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
> 
> drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
> 
> - ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> ib->next_bridge = bridge;
> 
> + ret = devm_drm_bridge_add(dev, &ib->bridge);
> + if (ret)
> + goto err_drvdata;
> +
> ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> if (ret) {
> -- 
> 2.53.0

Works for me on CI20 with HDMI display.

Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # CI20 V2a

Re: [PATCH] drm/ingenic: Allocate and register the internal bridge separately

[Paul Cercueil]

Nikolaus:

Le vendredi 06 novembre 2026 à 11:59 +0100, H. Nikolaus Schaller a écrit :
> Hi,
> 
> > Am 05.11.2026 um 21:35 schrieb Florian Fainelli <f.fainelli@gmail.com>:
> > 
> > The LCD controller's internal bridge is embedded in an object allocated
> > with drmm_encoder_alloc(). This initializes the encoder but leaves the
> > bridge's reference count at zero. Since drm_bridge_attach() takes a
> > reference, probing the CI20 display produces addition-on-zero and
> > underflow warnings and fails to attach the HDMI bridge chain.
> > 
> > Allocate the internal bridge with devm_drm_bridge_alloc() and register
> > it with devm_drm_bridge_add() before attaching it. Keep the encoder in a
> > separate DRM-managed allocation: encoder cleanup can drop the final
> > bridge reference before it has finished accessing the encoder. Find
> > the internal bridge through the encoder's bridge chain during mode
> > setting, and use the bridge argument directly in bridge callbacks.
> > 
> > Fixes: 94d50c1a2ca3 ("drm/bridge: get/put the bridge reference in
> > drm_bridge_attach/detach()")
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 31 +++++++++++++++--------
> > 1 file changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > index 625ec76cf04d..520912f9ed0d 100644
> > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > @@ -121,16 +121,15 @@ struct ingenic_drm {
> > };
> > 
> > struct ingenic_drm_bridge {
> > - struct drm_encoder encoder;
> > struct drm_bridge bridge, *next_bridge;
> > 
> > struct drm_bus_cfg bus_cfg;
> > };
> > 
> > static inline struct ingenic_drm_bridge *
> > -to_ingenic_drm_bridge(struct drm_encoder *encoder)
> > +to_ingenic_drm_bridge(struct drm_bridge *bridge)
> > {
> > - return container_of(encoder, struct ingenic_drm_bridge, encoder);
> > + return container_of(bridge, struct ingenic_drm_bridge, bridge);
> > }
> > 
> > static inline struct ingenic_drm_private_state *
> > @@ -738,7 +737,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct
> > drm_encoder *encoder,
> > {
> > struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
> > struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> > - struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
> > + struct drm_bridge *first_bridge __free(drm_bridge_put) =
> > + drm_bridge_chain_get_first_bridge(encoder);
> > + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(first_bridge);
> > unsigned int cfg, rgbcfg = 0;
> > 
> > priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> > @@ -799,7 +800,7 @@ static int ingenic_drm_bridge_attach(struct drm_bridge
> > *bridge,
> >     struct drm_encoder *encoder,
> >     enum drm_bridge_attach_flags flags)
> > {
> > - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(encoder);
> > + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
> > 
> > return drm_bridge_attach(encoder, ib->next_bridge,
> > &ib->bridge, flags);
> > @@ -811,7 +812,7 @@ static int ingenic_drm_bridge_atomic_check(struct
> > drm_bridge *bridge,
> >   struct drm_connector_state *conn_state)
> > {
> > struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> > - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
> > + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
> > 
> > ib->bus_cfg = bridge_state->output_bus_cfg;
> > 
> > @@ -1303,22 +1304,32 @@ static int ingenic_drm_bind(struct device *dev, bool
> > has_components)
> > drm_panel_put(panel);
> > }
> > 
> > - ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
> > - NULL, DRM_MODE_ENCODER_DPI, NULL);
> > + ib = devm_drm_bridge_alloc(dev, struct ingenic_drm_bridge, bridge,
> > +   &ingenic_drm_bridge_funcs);
> > if (IS_ERR(ib)) {
> > ret = PTR_ERR(ib);
> > + dev_err(dev, "Failed to allocate bridge: %d\n", ret);
> > + goto err_drvdata;
> > + }
> > +
> > + encoder = drmm_plain_encoder_alloc(drm, NULL,
> > +   DRM_MODE_ENCODER_DPI, NULL);
> > + if (IS_ERR(encoder)) {
> > + ret = PTR_ERR(encoder);
> > dev_err(dev, "Failed to init encoder: %d\n", ret);
> > goto err_drvdata;
> > }
> > 
> > - encoder = &ib->encoder;
> > encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
> > 
> > drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
> > 
> > - ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> > ib->next_bridge = bridge;
> > 
> > + ret = devm_drm_bridge_add(dev, &ib->bridge);
> > + if (ret)
> > + goto err_drvdata;
> > +
> > ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > if (ret) {
> > -- 
> > 2.53.0
> 
> Works for me on CI20 with HDMI display.
> 
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # CI20 V2a

This patches basically overrides your "[PATCH v6] drm/ingenic: fix bridge
allocation", correct?

-Paul

Re: [PATCH] drm/ingenic: Allocate and register the internal bridge separately

[H. Nikolaus Schaller]

> Am 06.11.2026 um 13:21 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Nikolaus:
> 
> Le vendredi 06 novembre 2026 à 11:59 +0100, H. Nikolaus Schaller a écrit :
>> Hi,
>> 
>>> Am 05.11.2026 um 21:35 schrieb Florian Fainelli <f.fainelli@gmail.com>:
>>> 
>>> The LCD controller's internal bridge is embedded in an object allocated
>>> with drmm_encoder_alloc(). This initializes the encoder but leaves the
>>> bridge's reference count at zero. Since drm_bridge_attach() takes a
>>> reference, probing the CI20 display produces addition-on-zero and
>>> underflow warnings and fails to attach the HDMI bridge chain.
>>> 
>>> Allocate the internal bridge with devm_drm_bridge_alloc() and register
>>> it with devm_drm_bridge_add() before attaching it. Keep the encoder in a
>>> separate DRM-managed allocation: encoder cleanup can drop the final
>>> bridge reference before it has finished accessing the encoder. Find
>>> the internal bridge through the encoder's bridge chain during mode
>>> setting, and use the bridge argument directly in bridge callbacks.
>>> 
>>> Fixes: 94d50c1a2ca3 ("drm/bridge: get/put the bridge reference in
>>> drm_bridge_attach/detach()")
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 31 +++++++++++++++--------
>>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> index 625ec76cf04d..520912f9ed0d 100644
>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>> @@ -121,16 +121,15 @@ struct ingenic_drm {
>>> };
>>> 
>>> struct ingenic_drm_bridge {
>>> - struct drm_encoder encoder;
>>> struct drm_bridge bridge, *next_bridge;
>>> 
>>> struct drm_bus_cfg bus_cfg;
>>> };
>>> 
>>> static inline struct ingenic_drm_bridge *
>>> -to_ingenic_drm_bridge(struct drm_encoder *encoder)
>>> +to_ingenic_drm_bridge(struct drm_bridge *bridge)
>>> {
>>> - return container_of(encoder, struct ingenic_drm_bridge, encoder);
>>> + return container_of(bridge, struct ingenic_drm_bridge, bridge);
>>> }
>>> 
>>> static inline struct ingenic_drm_private_state *
>>> @@ -738,7 +737,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct
>>> drm_encoder *encoder,
>>> {
>>> struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
>>> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>>> - struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
>>> + struct drm_bridge *first_bridge __free(drm_bridge_put) =
>>> + drm_bridge_chain_get_first_bridge(encoder);
>>> + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(first_bridge);
>>> unsigned int cfg, rgbcfg = 0;
>>> 
>>> priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
>>> @@ -799,7 +800,7 @@ static int ingenic_drm_bridge_attach(struct drm_bridge
>>> *bridge,
>>>     struct drm_encoder *encoder,
>>>     enum drm_bridge_attach_flags flags)
>>> {
>>> - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(encoder);
>>> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
>>> 
>>> return drm_bridge_attach(encoder, ib->next_bridge,
>>> &ib->bridge, flags);
>>> @@ -811,7 +812,7 @@ static int ingenic_drm_bridge_atomic_check(struct
>>> drm_bridge *bridge,
>>>   struct drm_connector_state *conn_state)
>>> {
>>> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>>> - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
>>> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
>>> 
>>> ib->bus_cfg = bridge_state->output_bus_cfg;
>>> 
>>> @@ -1303,22 +1304,32 @@ static int ingenic_drm_bind(struct device *dev, bool
>>> has_components)
>>> drm_panel_put(panel);
>>> }
>>> 
>>> - ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
>>> - NULL, DRM_MODE_ENCODER_DPI, NULL);
>>> + ib = devm_drm_bridge_alloc(dev, struct ingenic_drm_bridge, bridge,
>>> +   &ingenic_drm_bridge_funcs);
>>> if (IS_ERR(ib)) {
>>> ret = PTR_ERR(ib);
>>> + dev_err(dev, "Failed to allocate bridge: %d\n", ret);
>>> + goto err_drvdata;
>>> + }
>>> +
>>> + encoder = drmm_plain_encoder_alloc(drm, NULL,
>>> +   DRM_MODE_ENCODER_DPI, NULL);
>>> + if (IS_ERR(encoder)) {
>>> + ret = PTR_ERR(encoder);
>>> dev_err(dev, "Failed to init encoder: %d\n", ret);
>>> goto err_drvdata;
>>> }
>>> 
>>> - encoder = &ib->encoder;
>>> encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
>>> 
>>> drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>>> 
>>> - ib->bridge.funcs = &ingenic_drm_bridge_funcs;
>>> ib->next_bridge = bridge;
>>> 
>>> + ret = devm_drm_bridge_add(dev, &ib->bridge);
>>> + if (ret)
>>> + goto err_drvdata;
>>> +
>>> ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>> if (ret) {
>>> -- 
>>> 2.53.0
>> 
>> Works for me on CI20 with HDMI display.
>> 
>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # CI20 V2a
> 
> This patches basically overrides your "[PATCH v6] drm/ingenic: fix bridge
> allocation", correct?

Indeed.

Well, I am not sure which is the right one. Both work for me. So you can choose
(e.g. if struct ingenic_drm_bridge should embed an struct drm_encoder encoder
or not.

Best regards,
Nikolaus

Re: [PATCH] drm/ingenic: Allocate and register the internal bridge separately

[Luca Ceresoli]

Hello all,

On Fri Nov 6, 2026 at 1:28 PM CET, H. Nikolaus Schaller wrote:
>
>
>> Am 06.11.2026 um 13:21 schrieb Paul Cercueil <paul@crapouillou.net>:
>>
>> Nikolaus:
>>
>> Le vendredi 06 novembre 2026 à 11:59 +0100, H. Nikolaus Schaller a écrit :
>>> Hi,
>>>
>>>> Am 05.11.2026 um 21:35 schrieb Florian Fainelli <f.fainelli@gmail.com>:
>>>>
>>>> The LCD controller's internal bridge is embedded in an object allocated
>>>> with drmm_encoder_alloc(). This initializes the encoder but leaves the
>>>> bridge's reference count at zero. Since drm_bridge_attach() takes a
>>>> reference, probing the CI20 display produces addition-on-zero and
>>>> underflow warnings and fails to attach the HDMI bridge chain.
>>>>
>>>> Allocate the internal bridge with devm_drm_bridge_alloc() and register
>>>> it with devm_drm_bridge_add() before attaching it. Keep the encoder in a
>>>> separate DRM-managed allocation: encoder cleanup can drop the final
>>>> bridge reference before it has finished accessing the encoder. Find
>>>> the internal bridge through the encoder's bridge chain during mode
>>>> setting, and use the bridge argument directly in bridge callbacks.
>>>>
>>>> Fixes: 94d50c1a2ca3 ("drm/bridge: get/put the bridge reference in
>>>> drm_bridge_attach/detach()")
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 31 +++++++++++++++--------
>>>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> index 625ec76cf04d..520912f9ed0d 100644
>>>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>>> @@ -121,16 +121,15 @@ struct ingenic_drm {
>>>> };
>>>>
>>>> struct ingenic_drm_bridge {
>>>> - struct drm_encoder encoder;
>>>> struct drm_bridge bridge, *next_bridge;
>>>>
>>>> struct drm_bus_cfg bus_cfg;
>>>> };
>>>>
>>>> static inline struct ingenic_drm_bridge *
>>>> -to_ingenic_drm_bridge(struct drm_encoder *encoder)
>>>> +to_ingenic_drm_bridge(struct drm_bridge *bridge)
>>>> {
>>>> - return container_of(encoder, struct ingenic_drm_bridge, encoder);
>>>> + return container_of(bridge, struct ingenic_drm_bridge, bridge);
>>>> }
>>>>
>>>> static inline struct ingenic_drm_private_state *
>>>> @@ -738,7 +737,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct
>>>> drm_encoder *encoder,
>>>> {
>>>> struct ingenic_drm *priv = drm_device_get_priv(encoder->dev);
>>>> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>>>> - struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(encoder);
>>>> + struct drm_bridge *first_bridge __free(drm_bridge_put) =
>>>> + drm_bridge_chain_get_first_bridge(encoder);
>>>> + struct ingenic_drm_bridge *bridge = to_ingenic_drm_bridge(first_bridge);
>>>> unsigned int cfg, rgbcfg = 0;
>>>>
>>>> priv->panel_is_sharp = bridge->bus_cfg.flags & DRM_BUS_FLAG_SHARP_SIGNALS;
>>>> @@ -799,7 +800,7 @@ static int ingenic_drm_bridge_attach(struct drm_bridge
>>>> *bridge,
>>>>     struct drm_encoder *encoder,
>>>>     enum drm_bridge_attach_flags flags)
>>>> {
>>>> - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(encoder);
>>>> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
>>>>
>>>> return drm_bridge_attach(encoder, ib->next_bridge,
>>>> &ib->bridge, flags);
>>>> @@ -811,7 +812,7 @@ static int ingenic_drm_bridge_atomic_check(struct
>>>> drm_bridge *bridge,
>>>>   struct drm_connector_state *conn_state)
>>>> {
>>>> struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>>>> - struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder);
>>>> + struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge);
>>>>
>>>> ib->bus_cfg = bridge_state->output_bus_cfg;
>>>>
>>>> @@ -1303,22 +1304,32 @@ static int ingenic_drm_bind(struct device *dev, bool
>>>> has_components)
>>>> drm_panel_put(panel);
>>>> }
>>>>
>>>> - ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
>>>> - NULL, DRM_MODE_ENCODER_DPI, NULL);
>>>> + ib = devm_drm_bridge_alloc(dev, struct ingenic_drm_bridge, bridge,
>>>> +   &ingenic_drm_bridge_funcs);
>>>> if (IS_ERR(ib)) {
>>>> ret = PTR_ERR(ib);
>>>> + dev_err(dev, "Failed to allocate bridge: %d\n", ret);
>>>> + goto err_drvdata;
>>>> + }
>>>> +
>>>> + encoder = drmm_plain_encoder_alloc(drm, NULL,
>>>> +   DRM_MODE_ENCODER_DPI, NULL);
>>>> + if (IS_ERR(encoder)) {
>>>> + ret = PTR_ERR(encoder);
>>>> dev_err(dev, "Failed to init encoder: %d\n", ret);
>>>> goto err_drvdata;
>>>> }
>>>>
>>>> - encoder = &ib->encoder;
>>>> encoder->possible_crtcs = drm_crtc_mask(&priv->crtc);
>>>>
>>>> drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>>>>
>>>> - ib->bridge.funcs = &ingenic_drm_bridge_funcs;
>>>> ib->next_bridge = bridge;
>>>>
>>>> + ret = devm_drm_bridge_add(dev, &ib->bridge);
>>>> + if (ret)
>>>> + goto err_drvdata;
>>>> +
>>>> ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
>>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>> if (ret) {
>>>> --
>>>> 2.53.0
>>>
>>> Works for me on CI20 with HDMI display.
>>>
>>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com> # CI20 V2a
>>
>> This patches basically overrides your "[PATCH v6] drm/ingenic: fix bridge
>> allocation", correct?
>
> Indeed.
>
> Well, I am not sure which is the right one. Both work for me. So you can choose
> (e.g. if struct ingenic_drm_bridge should embed an struct drm_encoder encoder
> or not.

Sorry I hadn't seen the previous patches by Nikolaus. And I missed this
driver while converting bridge users to devm_drm_bridge_alloc().

Both patches look technically correct, but this one looks cleaner to me, so
I'd apply this one.

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com