Hello Maxime,
On Tue Oct 14, 2025 at 11:31 AM CEST, Maxime Ripard wrote:
> The bridge implementation relies on a drm_private_obj, that is
> initialized by allocating and initializing a state, and then passing it
> to drm_private_obj_init.
>
> Since we're gradually moving away from that pattern to the more
> established one relying on a atomic_create_state implementation, let's
> migrate this instance to the new pattern.
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
>
> Cc: Andrzej Hajda <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Robert Foss <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Jonas Karlman <[email protected]>
> Cc: Jernej Skrabec <[email protected]>
> ---
> drivers/gpu/drm/drm_bridge.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index
> 630b5e6594e0affad9ba48791207c7b403da5db8..f0db891863428ee65625a6a3ed38f63ec802595e
> 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -394,11 +394,27 @@ drm_bridge_atomic_destroy_priv_state(struct
> drm_private_obj *obj,
> struct drm_bridge *bridge = drm_priv_to_bridge(obj);
>
> bridge->funcs->atomic_destroy_state(bridge, state);
> }
>
> +static struct drm_private_state *
> +drm_bridge_atomic_create_priv_state(struct drm_private_obj *obj)
> +{
> + struct drm_bridge *bridge = drm_priv_to_bridge(obj);
> + struct drm_bridge_state *state;
> +
> + state = bridge->funcs->atomic_reset(bridge);
> + if (IS_ERR(state))
> + return ERR_PTR(-ENOMEM);
This is slightly changing the behaviour, assuming that every error is
-ENOMEM, while in the current implementation any error code is just
propagated. I searched all .atomic_reset callbacks and apparently none can
return any other error, so this would not introduce a bug with current
drivers. However the atomic_reset docs say any ERR_PTR can be returned,
thus a future driver would be allowed to return another error value, even
thoug it's unlikely. The drm_bridge.c core having no control over what
other drivers do, I wonder whether we should just return ERR_PTR(state)
here, and keep the check on the drm_atomic_private_obj_init() return value
below.
I have no strong position about which direction is best however. Maybe
changing the docs to say "Return: only -ENOMEM", and add here a
WARN_ON(IS_ERR(state) && ERR_PTR(state) != -ENOMEM)?
> @@ -462,30 +478,17 @@ int drm_bridge_attach(struct drm_encoder *encoder,
> struct drm_bridge *bridge,
> ret = bridge->funcs->attach(bridge, encoder, flags);
> if (ret < 0)
> goto err_reset_bridge;
> }
>
> - if (drm_bridge_is_atomic(bridge)) {
> - struct drm_bridge_state *state;
> -
> - state = bridge->funcs->atomic_reset(bridge);
> - if (IS_ERR(state)) {
> - ret = PTR_ERR(state);
> - goto err_detach_bridge;
> - }
> -
> + if (drm_bridge_is_atomic(bridge))
> drm_atomic_private_obj_init(bridge->dev, &bridge->base,
> - &state->base,
> + NULL,
> &drm_bridge_priv_state_funcs);
> - }
>
> return 0;
>
> -err_detach_bridge:
> - if (bridge->funcs->detach)
> - bridge->funcs->detach(bridge);
> -
> err_reset_bridge:
> bridge->dev = NULL;
> bridge->encoder = NULL;
> list_del(&bridge->chain_node);
>
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
