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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2913 bytes --]

On Tue, Oct 21, 2025 at 10:17:13AM +0200, Luca Ceresoli wrote:
> 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)?

No, it's a good catch, we should totally return state and not ignore it.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]