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

[Luca Ceresoli]

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.

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'.

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?

Let me know what you think.

[0] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/7a921d111810652672e02c392b35fdcefa4d5030/drivers/gpu/drm/display/drm_bridge_connector.c#L995-1029
[1] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/7a921d111810652672e02c392b35fdcefa4d5030/drivers/gpu/drm/display/drm_bridge_connector.c#L1030
[2] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/7a921d111810652672e02c392b35fdcefa4d5030/drivers/gpu/drm/drm_connector.c#L617-631

Kind regards,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com