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

[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?

-Paul