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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

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