Re: [PATCH v6 15/19] drm/connector: Add new atomic_create_state callback

[Maxime Ripard]

On Fri, Jun 19, 2026 at 06:24:46PM +0200, Luca Ceresoli wrote:
> Hello Maxime, Dmitry, all,
> 
> On Tue May 26, 2026 at 6:46 PM CEST, Maxime Ripard wrote:
> > Commit 47b5ac7daa46 ("drm/atomic: Add new atomic_create_state callback
> > to drm_private_obj") introduced a new pattern for allocating drm object
> > states.
> >
> > Instead of relying on the reset() callback, it created a new
> > atomic_create_state hook. This is helpful because reset is a bit
> > overloaded: it's used to create the initial software state, reset it,
> > but also reset the hardware.
> >
> > It can also be used either at probe time, to create the initial state
> > and possibly reset the hardware to an expected default, but also during
> > suspend/resume.
> >
> > Both these cases come with different expectations too: during the
> > initialization, we want to initialize all states, but during
> > suspend/resume, drm_private_states for example are expected to be kept
> > around.
> >
> > reset() also isn't fallible, which makes it harder to handle
> > initialization errors properly. This is only really relevant for some
> > drivers though, since all the helpers for reset only create a new
> > state, and don't touch the hardware at all.
> >
> > It was thus decided to create a new hook that would allocate and
> > initialize a pristine state without any side effect:
> > atomic_create_state to untangle a bit some of it, and to separate the
> > initialization with the actual reset one might need during a
> > suspend/resume.
> >
> > Continue the transition to the new pattern with connectors.
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
> 
> As I'm rebasing another series on current drm-misc-next, which now includes
> this patch, I ran into troubles and I'm not sure what is the right thing to
> do. I hope you can help me clarify this. See below for my question.
> 
> FTR the series I'm rebasing is "drm bridge hotplug", but the question is
> not specific to that series.
> 
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -616,11 +616,19 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
> >
> >  	/*
> >  	 * drm_connector_attach_max_bpc_property() requires the
> >  	 * connector to have a state.
> >  	 */
> > -	if (connector->funcs->reset)
> > +	if (connector->funcs->atomic_create_state) {
> > +		struct drm_connector_state *state;
> > +
> > +		state = connector->funcs->atomic_create_state(connector);
> > +		if (IS_ERR(state))
> > +			return PTR_ERR(state);
> > +
> > +		connector->state = state;
> > +	} else if (connector->funcs->reset)
> >  		connector->funcs->reset(connector);
> 
> Here a state is added to connector->state, and that's fine.
> 
> However non-HDMI connectors don't get a state created by default.

That's true, but I don't see how this particular patch affects it? The
call sites of reset are now falling back to atomic_create_state, but it
doesn't change anything wrt when reset is (or was?) called, which seems
to be what you're talking about.

> I was hit by this with the drm_bridge_connector which it can add either an
> HDMI or a non-HDMI connector [0]. In the former case it calls
> drmm_connector_hdmi_init(), which creates the state (in the hunk quoted
> above). In the latter case, as I experienced at runtime and confirmed by
> code inspection, it does not create a state: no one calls
> connector->funcs->atomic_create_state.
> 
> I suspect this is related to patch 19/19 which converted the
> drm_bridge_connector from drm_atomic_helper_connector_reset() to
> drm_atomic_helper_connector_create_state(), and only the former sets
> 'connector->state = conn_state'.

But it's pretty much the same story here? it changes the implementation,
but it should be called at the same time it used to.

> Generally speaking, looks like a state is created only for HDMI
> connectors.
> 
> The hardware I have uses the drm_bridge_connector in the non-HDMI case, so
> the state is not created and this results in a NULL pointer deref later on,
> in my case it's in in drm_atomic_connector_get_property().
> 
> Am I missing anything obvious?
> 
> For now I've come up with a quick workaround, adding (roughly after
> connector init at [1]):
> 
>         if (!connector->state)
>                 connector->state = drm_bridge_connector_create_state(connector);
> 
> I'm not sure which would be the best solution. Maybe taking the whole
> atomic_create_state/reset state creation calls [2] from
> drmm_connector_hdmi_init() and hoist them up into
> drmm_connector_init(), so all connectors benefit?

Generally speaking, either drm_mode_config_reset() or
drm_mode_config_create_initial_state will fill $object->state on most
drivers. For dynamic connectors, you'll need to create the initial state
when creating the new connector.

That's what intel (in intel_connector_alloc()) is doing
https://elixir.bootlin.com/linux/v7.0.11/source/drivers/gpu/drm/i915/display/intel_dp_mst.c#L1684

amdgpu through the call to amdgpu_dm_connector_funcs_reset():
https://elixir.bootlin.com/linux/v7.0.11/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c#L632

nouveau through the call to mstc->connector.funcs->reset()
https://elixir.bootlin.com/linux/v7.0.11/source/drivers/gpu/drm/nouveau/dispnv50/disp.c#L1262

Would it be possible that it's not a regression but rather that you just noticed it?

Maxime

Attachment: signature.asc
Description: PGP signature