Re: [PATCH v2 05/16] drm/bridge: Switch private_obj initialization to atomic_create_state

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

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 <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  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