[v4,15/20] drm/connector: Add new atomic_create_state callback
Commit Message
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@oss.qualcomm.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/drm_atomic_state_helper.c | 26 ++++++++++++++++++++++++++
drivers/gpu/drm/drm_mode_config.c | 31 ++++++++++++++++++++++++++++++-
include/drm/drm_atomic_state_helper.h | 2 ++
include/drm/drm_connector.h | 16 ++++++++++++++++
4 files changed, 74 insertions(+), 1 deletion(-)
Comments
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing fallback to `atomic_create_state` in explicit `reset()` callers causes kernel panics (e.g., in `drmm_connector_hdmi_init`).
- [High] TV connector reset helpers are structurally incompatible with `atomic_create_state`, preventing driver migration.
- [High] Missing NULL check for `atomic_destroy_state` causes kernel panic on driver omission.
--
commit 9c7dcc4b2ba2f3c09ed3926c7e0db8c1a524dd31
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/connector: Add new atomic_create_state callback
This commit introduces the atomic_create_state callback for drm_connectors.
It allows drivers to allocate a pristine state without side-effects, separating
initialization from hardware reset operations during suspend and resume.
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index e2e5a1b8a8202..07686e94aae00 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
[ ... ]
> @@ -534,6 +534,32 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector)
> }
> EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
>
> +/**
> + * drm_atomic_helper_connector_create_state - default &drm_connector_funcs.atomic_create_state hook for connectors
> + * @connector: connector object
> + *
> + * Allocates and initializes pristine @drm_connector_state.
> + *
> + * This is useful for drivers that don't subclass @drm_connector_state.
> + *
> + * RETURNS:
> + * Pointer to new connector state, or ERR_PTR on failure.
> + */
> +struct drm_connector_state *
> +drm_atomic_helper_connector_create_state(struct drm_connector *connector)
> +{
> + struct drm_connector_state *state;
> +
> + state = kzalloc_obj(*state);
> + if (!state)
> + return ERR_PTR(-ENOMEM);
> +
> + __drm_atomic_helper_connector_state_init(state, connector);
> +
> + return state;
> +}
How should TV drivers migrating to atomic_create_state handle initialization?
The new pattern requires drivers to allocate and return a pristine state
without assigning it to connector->state. However, existing initialization
helpers like drm_atomic_helper_connector_tv_reset() and
drm_atomic_helper_connector_tv_margins_reset() are hardcoded to read and
modify connector->state directly.
If a TV driver attempts to use these helpers on its newly allocated state,
will the helpers dereference a NULL or stale connector->state and crash?
Would it make sense to introduce state-based variants of these helpers
that take a struct drm_connector_state pointer?
> +EXPORT_SYMBOL(drm_atomic_helper_connector_create_state);
[ ... ]
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 2e2cd18a14b43..9d240817f8b64 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
[ ... ]
> +static int drm_mode_config_connector_reset_with_create_state(struct drm_connector *connector)
> +{
> + if (connector->state) {
> + connector->funcs->atomic_destroy_state(connector, connector->state);
Is there a risk of a NULL pointer dereference here if a driver migrating to
atomic_create_state omits the atomic_destroy_state hook?
While omitting the hook is a driver bug, other DRM core functions like
drm_connector_cleanup() gracefully guard against this by checking
if (connector->funcs->atomic_destroy_state) before calling it. Should this
wrapper include a similar defensive guard to prevent system crashes?
> + connector->state = NULL;
> + }
> +
> + return drm_mode_config_connector_create_state(connector);
> +}
[ ... ]
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5ad62c207d009..529755c2e8620 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
[ ... ]
> @@ -1571,6 +1571,22 @@ struct drm_connector_funcs {
> */
> void (*destroy)(struct drm_connector *connector);
>
> + /**
> + * @atomic_create_state:
> + *
> + * Allocate a pristine, initialized, state for the connector
Does the introduction of atomic_create_state require updates to other core
functions that explicitly call the reset callback to allocate state?
For example, drmm_connector_hdmi_init() checks for connector->funcs->reset
and calls it. If a driver uses atomic_create_state instead, the reset check
fails and the state is never allocated.
Immediately after, drmm_connector_hdmi_init() calls
drm_connector_attach_max_bpc_property(), which dereferences the uninitialized
connector->state. Can this cause a kernel panic on boot for drivers migrating
to atomic_create_state?
> + * object and return it. This callback must have no side
> + * effects: in particular, the returned state must not be
> + * assigned to the object's state pointer and it must not affect
> + * the hardware state.
@@ -532,10 +532,36 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector)
kfree(connector->state);
__drm_atomic_helper_connector_reset(connector, conn_state);
}
EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
+/**
+ * drm_atomic_helper_connector_create_state - default &drm_connector_funcs.atomic_create_state hook for connectors
+ * @connector: connector object
+ *
+ * Allocates and initializes pristine @drm_connector_state.
+ *
+ * This is useful for drivers that don't subclass @drm_connector_state.
+ *
+ * RETURNS:
+ * Pointer to new connector state, or ERR_PTR on failure.
+ */
+struct drm_connector_state *
+drm_atomic_helper_connector_create_state(struct drm_connector *connector)
+{
+ struct drm_connector_state *state;
+
+ state = kzalloc_obj(*state);
+ if (!state)
+ return ERR_PTR(-ENOMEM);
+
+ __drm_atomic_helper_connector_state_init(state, connector);
+
+ return state;
+}
+EXPORT_SYMBOL(drm_atomic_helper_connector_create_state);
+
/**
* drm_atomic_helper_connector_tv_margins_reset - Resets TV connector properties
* @connector: DRM connector
*
* Resets the TV-related properties attached to a connector.
@@ -235,10 +235,36 @@ static int drm_mode_config_crtc_reset_with_create_state(struct drm_crtc *crtc)
}
return drm_mode_config_crtc_create_state(crtc);
}
+static int drm_mode_config_connector_create_state(struct drm_connector *connector)
+{
+ struct drm_connector_state *conn_state;
+
+ if (!connector->funcs->atomic_create_state)
+ return 0;
+
+ conn_state = connector->funcs->atomic_create_state(connector);
+ if (IS_ERR(conn_state))
+ return PTR_ERR(conn_state);
+
+ connector->state = conn_state;
+
+ return 0;
+}
+
+static int drm_mode_config_connector_reset_with_create_state(struct drm_connector *connector)
+{
+ if (connector->state) {
+ connector->funcs->atomic_destroy_state(connector, connector->state);
+ connector->state = NULL;
+ }
+
+ return drm_mode_config_connector_create_state(connector);
+}
+
/**
* drm_mode_config_reset - call ->reset callbacks
* @dev: drm device
*
* This functions calls all the crtc's, encoder's and connector's ->reset
@@ -278,13 +304,16 @@ void drm_mode_config_reset(struct drm_device *dev)
drm_for_each_encoder(encoder, dev)
if (encoder->funcs && encoder->funcs->reset)
encoder->funcs->reset(encoder);
drm_connector_list_iter_begin(dev, &conn_iter);
- drm_for_each_connector_iter(connector, &conn_iter)
+ drm_for_each_connector_iter(connector, &conn_iter) {
if (connector->funcs->reset)
connector->funcs->reset(connector);
+ else if (connector->funcs->atomic_create_state)
+ drm_mode_config_connector_reset_with_create_state(connector);
+ }
drm_connector_list_iter_end(&conn_iter);
}
EXPORT_SYMBOL(drm_mode_config_reset);
/*
@@ -73,10 +73,12 @@ void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
void __drm_atomic_helper_connector_state_init(struct drm_connector_state *conn_state,
struct drm_connector *connector);
void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
struct drm_connector_state *conn_state);
void drm_atomic_helper_connector_reset(struct drm_connector *connector);
+struct drm_connector_state *
+drm_atomic_helper_connector_create_state(struct drm_connector *connector);
void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector);
int drm_atomic_helper_connector_tv_check(struct drm_connector *connector,
struct drm_atomic_commit *state);
void drm_atomic_helper_connector_tv_margins_reset(struct drm_connector *connector);
void
@@ -1569,10 +1569,26 @@ struct drm_connector_funcs {
* when a connector is being hot-unplugged for drivers that support
* connector hotplugging (e.g. DisplayPort MST).
*/
void (*destroy)(struct drm_connector *connector);
+ /**
+ * @atomic_create_state:
+ *
+ * Allocate a pristine, initialized, state for the connector
+ * object and return it. This callback must have no side
+ * effects: in particular, the returned state must not be
+ * assigned to the object's state pointer and it must not affect
+ * the hardware state.
+ *
+ * RETURNS:
+ *
+ * A new, pristine, connector state instance or an error pointer
+ * on failure.
+ */
+ struct drm_connector_state *(*atomic_create_state)(struct drm_connector *connector);
+
/**
* @atomic_duplicate_state:
*
* Duplicate the current atomic state for this connector and return it.
* The core and helpers guarantee that any atomic state duplicated with