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 <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > Reviewed-by: Thomas Zimmermann <[email protected]>
> > Signed-off-by: Maxime Ripard <[email protected]>
>
> 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
signature.asc
Description: PGP signature
