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

[Paul Cercueil <paul@crapouillou.net>]

Hi,

Le jeudi 05 novembre 2026 à 12:35 -0800, Florian Fainelli a écrit :
> 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>

Looks like I lost my commit rights to drm-misc-next, I guess for
inactivity.

So I'll ack it (LGTM) and let someone else (Maxime?) commit it.

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  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_CONNECT
> OR);
>  		if (ret) {