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 <[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)?
No, it's a good catch, we should totally return state and not ignore it. Thanks! Maxime
signature.asc
Description: PGP signature
