In order to enable drivers to fill their initial state from the hardware
state, we need to provide an alternative atomic_reset helper.
This helper relies on each state having its own atomic_state_readout()
hooks. Each component will thus be able to fill the initial state based
on what they can figure out from the hardware.
It also allocates a dummy drm_atomic_state to glue the whole thing
together so atomic_state_readout implementations can still figure out
the state of other related entities.
Link: https://lore.kernel.org/dri-devel/CAKMK7uHtqHy_oz4W7F+hmp9iqp7W5Ra8CxPvJ=9BwmvfU-O0gg@mail.gmail.com/
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
drivers/gpu/drm/drm_atomic_helper.c | 382 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_mode_config.c | 1 +
include/drm/drm_atomic_helper.h | 1 +
include/drm/drm_bridge.h | 21 ++
include/drm/drm_connector.h | 26 +++
include/drm/drm_crtc.h | 19 ++
include/drm/drm_plane.h | 27 +++
7 files changed, 477 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d5ebe6ea0acbc5a08aef7fa41ecb9ed5d8fa8e80..f59512476ebf2b48e1c7034950bcaf99237f03c6 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -45,10 +45,392 @@
#include <drm/drm_vblank.h>
#include <drm/drm_writeback.h>
#include "drm_crtc_helper_internal.h"
#include "drm_crtc_internal.h"
+#include "drm_internal.h"
+
+static void drm_atomic_set_old_plane_state(struct drm_atomic_state *state,
+ struct drm_plane *plane,
+ struct drm_plane_state *plane_state)
+{
+ int idx = drm_plane_index(plane);
+
+ state->planes[idx].old_state = plane_state;
+ state->planes[idx].ptr = plane;
+
+ drm_dbg_atomic(plane->dev, "Added [PLANE:%d:%s] %p state to %p\n",
+ plane->base.id, plane->name, plane_state, state);
+}
+
+static void drm_atomic_set_old_crtc_state(struct drm_atomic_state *state,
+ struct drm_crtc *crtc,
+ struct drm_crtc_state *crtc_state)
+{
+ int idx = drm_crtc_index(crtc);
+
+ state->crtcs[idx].old_state = crtc_state;
+ state->crtcs[idx].ptr = crtc;
+
+ drm_dbg_atomic(state->dev, "Added [CRTC:%d:%s] %p state to %p\n",
+ crtc->base.id, crtc->name, crtc_state, state);
+}
+
+static void drm_atomic_set_old_connector_state(struct drm_atomic_state *state,
+ struct drm_connector *conn,
+ struct drm_connector_state *conn_state)
+{
+ int idx = drm_connector_index(conn);
+
+ drm_connector_get(conn);
+ state->connectors[idx].old_state = conn_state;
+ state->connectors[idx].ptr = conn;
+ state->num_connector++;
+
+ drm_dbg_atomic(conn->dev,
+ "Added [CONNECTOR:%d:%s] %p state to %p\n",
+ conn->base.id, conn->name, conn_state, state);
+}
+
+static void drm_atomic_set_old_private_obj_state(struct drm_atomic_state *state,
+ struct drm_private_obj *obj,
+ struct drm_private_state *obj_state)
+{
+ int idx = state->num_private_objs;
+
+ memset(&state->private_objs[idx], 0, sizeof(*state->private_objs));
+ state->private_objs[idx].old_state = obj_state;
+ state->private_objs[idx].ptr = obj;
+ state->num_private_objs++;
+
+ drm_dbg_atomic(state->dev,
+ "Added new private object %p state %p to %p\n", obj,
+ obj_state, state);
+}
+
+static void drm_atomic_set_old_bridge_state(struct drm_atomic_state *state,
+ struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state)
+{
+ drm_atomic_set_old_private_obj_state(state, &bridge->base, &bridge_state->base);
+}
+
+static struct drm_connector_state *
+find_connector_state_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder)
+{
+ struct drm_connector_list_iter conn_iter;
+ struct drm_connector *connector;
+
+ drm_connector_list_iter_begin(state->dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ struct drm_connector_state *conn_state =
+ drm_atomic_get_old_connector_state(state, connector);
+
+ if (WARN_ON(!conn_state))
+ continue;
+
+ if (encoder == conn_state->best_encoder)
+ return conn_state;
+ }
+ drm_connector_list_iter_end(&conn_iter);
+
+ return NULL;
+}
+
+/**
+ * drm_atomic_helper_install_readout_state - Sets the state pointer from their readout state
+ * @state: drm_atomic_state to initialize the DRM device with.
+ *
+ * This function takes a &struct drm_atomic_state initialized by
+ * drm_atomic_build_readout_state() and sets up the entities state
+ * pointers (ie, &drm_crtc.state and similar) to properly setup the
+ * initial state.
+ */
+static void
+drm_atomic_helper_install_readout_state(struct drm_atomic_state *state)
+{
+ struct drm_connector_state *old_conn_state;
+ struct drm_private_state *old_obj_state;
+ struct drm_plane_state *old_plane_state;
+ struct drm_crtc_state *old_crtc_state;
+ struct drm_connector *connector;
+ struct drm_private_obj *obj;
+ struct drm_plane *plane;
+ struct drm_crtc *crtc;
+ unsigned int i;
+
+ for_each_old_connector_in_state(state, connector, old_conn_state, i) {
+ connector->state = old_conn_state;
+
+ drm_connector_put(state->connectors[i].ptr);
+ state->connectors[i].ptr = NULL;
+ state->connectors[i].old_state = NULL;
+ }
+
+ for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+ crtc->state = old_crtc_state;
+
+ state->crtcs[i].ptr = NULL;
+ state->crtcs[i].old_state = NULL;
+ }
+
+ for_each_old_plane_in_state(state, plane, old_plane_state, i) {
+ plane->state = old_plane_state;
+
+ state->planes[i].ptr = NULL;
+ state->planes[i].old_state = NULL;
+ }
+
+ for_each_old_private_obj_in_state(state, obj, old_obj_state, i) {
+ obj->state = old_obj_state;
+
+ state->private_objs[i].ptr = NULL;
+ state->private_objs[i].old_state = NULL;
+ }
+}
+
+static unsigned int count_private_obj(struct drm_device *dev)
+{
+ struct drm_mode_config *config = &dev->mode_config;
+ struct drm_private_obj *obj;
+ unsigned int count = 0;
+
+ list_for_each_entry(obj, &config->privobj_list, head)
+ count++;
+
+ return count;
+}
+
+/**
+ * drm_atomic_build_readout_state - Creates an initial state from the hardware
+ * @dev: DRM device to build the state for
+ *
+ * This function allocates a &struct drm_atomic_state, calls the
+ * atomic_readout_state callbacks, and fills the global state old states
+ * by what the callbacks returned.
+ *
+ * Returns:
+ *
+ * A partially initialized &struct drm_atomic_state on success, an error
+ * pointer otherwise.
+ */
+static struct drm_atomic_state *
+drm_atomic_build_readout_state(struct drm_device *dev)
+{
+ struct drm_connector_list_iter conn_iter;
+ struct drm_atomic_state *state;
+ struct drm_mode_config *config =
+ &dev->mode_config;
+ struct drm_connector *connector;
+ struct drm_printer p =
+ drm_info_printer(dev->dev);
+ struct drm_encoder *encoder;
+ struct drm_plane *plane;
+ struct drm_crtc *crtc;
+ int ret;
+
+ drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
+
+ state = drm_atomic_state_alloc(dev);
+ if (WARN_ON(!state))
+ return ERR_PTR(-ENOMEM);
+
+ state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
+ if (WARN_ON(!state->connectors)) {
+ ret = -ENOMEM;
+ goto err_state_put;
+ }
+
+ state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
+ if (WARN_ON(!state->private_objs)) {
+ ret = -ENOMEM;
+ goto err_state_put;
+ }
+
+ drm_for_each_crtc(crtc, dev) {
+ const struct drm_crtc_funcs *crtc_funcs =
+ crtc->funcs;
+ struct drm_crtc_state *crtc_state;
+
+ drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
+
+ if (crtc_funcs->atomic_readout_state) {
+ crtc_state = crtc_funcs->atomic_readout_state(crtc);
+ } else if (crtc_funcs->reset) {
+ crtc_funcs->reset(crtc);
+
+ /*
+ * We don't want to set crtc->state field yet. Let's save and clear it up.
+ */
+ crtc_state = crtc->state;
+ crtc->state = NULL;
+ } else {
+ drm_warn(dev, "No CRTC readout or reset implementation.");
+ continue;
+ }
+
+ if (WARN_ON(IS_ERR(crtc_state))) {
+ ret = PTR_ERR(crtc_state);
+ goto err_state_put;
+ }
+
+ drm_atomic_set_old_crtc_state(state, crtc, crtc_state);
+ }
+
+ drm_connector_list_iter_begin(dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ const struct drm_connector_funcs *conn_funcs =
+ connector->funcs;
+ struct drm_connector_state *conn_state;
+
+ drm_dbg_kms(dev, "Initializing Connector %s state.\n", connector->name);
+
+ if (conn_funcs->atomic_readout_state) {
+ conn_state = conn_funcs->atomic_readout_state(connector, state);
+ } else if (conn_funcs->reset) {
+ conn_funcs->reset(connector);
+
+ /*
+ * We don't want to set connector->state field yet. Let's save and clear it
+ * up.
+ */
+ conn_state = connector->state;
+ connector->state = NULL;
+ } else {
+ drm_warn(dev, "No Connector readout or reset implementation.");
+ continue;
+ }
+
+ if (WARN_ON(IS_ERR(conn_state))) {
+ ret = PTR_ERR(conn_state);
+ goto err_state_put;
+ }
+
+ drm_atomic_set_old_connector_state(state, connector, conn_state);
+ }
+ drm_connector_list_iter_end(&conn_iter);
+
+ WARN_ON(state->num_connector != config->num_connector);
+
+ drm_for_each_encoder(encoder, dev) {
+ struct drm_connector_state *enc_conn_state;
+ struct drm_crtc_state *enc_crtc_state;
+ struct drm_bridge *bridge;
+
+ /*
+ * It works a bit differently for bridges. Because they are
+ * using a drm_private_state, and because
+ * drm_atomic_private_obj_init() asks for its initial state when
+ * initializing, instead of doing it later on through a reset
+ * call like the other entities, we can't have reset xor
+ * readout.
+ *
+ * We'll need a mandatory reset to create that initial, blank,
+ * state, and then readout will fill that state later on if the
+ * driver implements it.
+ *
+ * This also means we don't need to call the readout state
+ * function if we don't have the bridge enabled (ie, if no
+ * drm_connector_state->best_encoder points to bridge->encoder,
+ * and / or if drm_connector_state->crtc is NULL).
+ *
+ * In such a case, we would get the blank state reset created
+ * during registration.
+ */
+
+ enc_conn_state = find_connector_state_for_encoder(state, encoder);
+ if (!enc_conn_state)
+ continue;
+
+ enc_crtc_state = drm_atomic_get_old_crtc_state(state, enc_conn_state->crtc);
+ if (!enc_crtc_state)
+ continue;
+
+ list_for_each_entry(bridge, &encoder->bridge_chain, chain_node) {
+ const struct drm_bridge_funcs *bridge_funcs = bridge->funcs;
+ struct drm_bridge_state *bridge_state;
+
+ bridge_state = drm_bridge_get_current_state(bridge);
+ if (WARN_ON(!bridge_state)) {
+ ret = -EINVAL;
+ goto err_state_put;
+ }
+
+ if (bridge_funcs->atomic_readout_state) {
+ ret = bridge_funcs->atomic_readout_state(bridge,
+ bridge_state,
+ enc_crtc_state,
+ enc_conn_state);
+ if (WARN_ON(ret))
+ goto err_state_put;
+ }
+
+ drm_atomic_set_old_bridge_state(state, bridge, bridge_state);
+ }
+ }
+
+ drm_for_each_plane(plane, dev) {
+ const struct drm_plane_funcs *plane_funcs =
+ plane->funcs;
+ struct drm_plane_state *plane_state;
+
+ drm_dbg_kms(dev, "Initializing Plane %s state.\n", plane->name);
+
+ if (plane_funcs->atomic_readout_state) {
+ plane_state = plane_funcs->atomic_readout_state(plane, state);
+ } else if (plane_funcs->reset) {
+ plane_funcs->reset(plane);
+
+ /*
+ * We don't want to set conn->state field yet. Let's save and clear it up.
+ */
+ plane_state = plane->state;
+ plane->state = NULL;
+ } else {
+ drm_warn(dev, "No plane readout or reset implementation.");
+ continue;
+ }
+
+ if (WARN_ON(IS_ERR(plane_state))) {
+ ret = PTR_ERR(plane_state);
+ goto err_state_put;
+ }
+
+ drm_atomic_set_old_plane_state(state, plane, plane_state);
+ }
+
+ drm_atomic_print_old_state(state, &p);
+
+ return state;
+
+err_state_put:
+ drm_atomic_state_put(state);
+ return ERR_PTR(ret);
+}
+
+/**
+ * drm_atomic_helper_readout_state - Builds an initial state from hardware state
+ * @dev: DRM device to build the state for
+ *
+ * This function creates the initial state for all the entities on a
+ * @dev. Drivers can use this as their
+ * &drm_mode_config_helper_funcs.atomic_reset callback to implement
+ * hardware state readout suppport.
+ */
+void drm_atomic_helper_readout_state(struct drm_device *dev)
+{
+ struct drm_atomic_state *state;
+
+ state = drm_atomic_build_readout_state(dev);
+ if (IS_ERR(state))
+ return;
+
+ drm_atomic_helper_install_readout_state(state);
+ drm_atomic_state_put(state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_readout_state);
/**
* DOC: overview
*
* This helper library provides implementations of check and commit functions on
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 82180760032d3490d63fe83136465d2c26551d08..96d38a49be501a0090457cbe96135f82bb1358b5 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -26,10 +26,11 @@
#include <drm/drm_drv.h>
#include <drm/drm_encoder.h>
#include <drm/drm_file.h>
#include <drm/drm_framebuffer.h>
#include <drm/drm_managed.h>
+#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_mode_config.h>
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_print.h>
#include <linux/dma-resv.h>
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 53382fe93537bbcda9cee8827bc95de9a515efb5..47902a9181727a08581fb808faabe67d92a755cf 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -45,10 +45,11 @@
struct drm_atomic_state;
struct drm_private_obj;
struct drm_private_state;
+void drm_atomic_helper_readout_state(struct drm_device *dev);
int drm_atomic_helper_check_modeset(struct drm_device *dev,
struct drm_atomic_state *state);
int drm_atomic_helper_check_wb_connector_state(struct drm_connector *connector,
struct drm_atomic_state *state);
int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 8d9d4fd078e72977677fd992d725261232754e3e..15b63053f01869786831936ba28b7efc1e55e2e8 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -490,10 +490,31 @@ struct drm_bridge_funcs {
* The @atomic_post_disable callback is optional.
*/
void (*atomic_post_disable)(struct drm_bridge *bridge,
struct drm_atomic_state *state);
+ /**
+ * @atomic_readout_state:
+ *
+ * Initializes,this bridge atomic state.
+ *
+ * It's meant to be used by drivers that wants to implement fast
+ * / flicker-free boot and allows to initialize the atomic state
+ * from the hardware state left by the firmware.
+ *
+ * It's used at initialization time, so drivers must make sure
+ * that the power state is sensible when accessing the hardware.
+ *
+ * RETURNS:
+ *
+ * 0 on success, an error code otherwise.
+ */
+ int (*atomic_readout_state)(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state);
+
/**
* @atomic_duplicate_state:
*
* Duplicate the current bridge state object (which is guaranteed to be
* non-NULL).
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 8f34f4b8183d83dccd3e820a444fbf74fb6c16f2..f68bd9627c085c6d2463b847aaa245ccc651f27b 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1464,10 +1464,36 @@ 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_readout_state:
+ *
+ * Allocates, initializes, and returns an atomic state for this
+ * connector.
+ *
+ * It's meant to be used by drivers that wants to implement fast
+ * / flicker-free boot and allows to initialize the atomic state
+ * from the hardware state left by the firmware.
+ *
+ * It's used at initialization time, so drivers must make sure
+ * that the power state is sensible when accessing the hardware.
+ *
+ * The drm_atomic_state being passed is not fully filled. Only
+ * the CRTC state are there when this hooks is called, and only
+ * their old state. The only safe operation one can do on this
+ * state in this hook is calling
+ * drm_atomic_get_old_crtc_state().
+ *
+ * RETURNS:
+ *
+ * An atomic state on success, an error pointer otherwise.
+ */
+ struct drm_connector_state *(*atomic_readout_state)(struct drm_connector *connector,
+ struct drm_atomic_state *state);
+
/**
* @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
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index caa56e039da2a748cf40ebf45b37158acda439d9..c462bd9b2f7d3ae08e669463717002e5f78122fe 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -613,10 +613,29 @@ struct drm_crtc_funcs {
* 0 on success or a negative error code on failure.
*/
int (*set_property)(struct drm_crtc *crtc,
struct drm_property *property, uint64_t val);
+ /**
+ * @atomic_readout_state:
+ *
+ * Allocates, initializes, and returns an atomic state for this
+ * CRTC.
+ *
+ * It's meant to be used by drivers that wants to implement fast
+ * / flicker-free boot and allows to initialize the atomic state
+ * from the hardware state left by the firmware.
+ *
+ * It's used at initialization time, so drivers must make sure
+ * that the power state is sensible when accessing the hardware.
+ *
+ * RETURNS:
+ *
+ * An atomic state on success, an error pointer otherwise.
+ */
+ struct drm_crtc_state *(*atomic_readout_state)(struct drm_crtc *crtc);
+
/**
* @atomic_duplicate_state:
*
* Duplicate the current atomic state for this CRTC and return it.
* The core and helpers guarantee that any atomic state duplicated with
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 01479dd94e76a8389a0c9e9d6744400aa2291064..691a267c857a228f674ef02a63fb6d1ff9e379a8 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -378,10 +378,37 @@ struct drm_plane_funcs {
* 0 on success or a negative error code on failure.
*/
int (*set_property)(struct drm_plane *plane,
struct drm_property *property, uint64_t val);
+ /**
+ * @atomic_readout_state:
+ *
+ * Allocates, initializes, and returns an atomic state for this
+ * plane.
+ *
+ * It's meant to be used by drivers that wants to implement fast
+ * / flicker-free boot and allows to initialize the atomic state
+ * from the hardware state left by the firmware.
+ *
+ * It's used at initialization time, so drivers must make sure
+ * that the power state is sensible when accessing the hardware.
+ *
+ * The drm_atomic_state being passed is not fully filled. Only
+ * the CRTC and connector states are there when this hooks is
+ * called, and only their old state. The only safe operation one
+ * can do on this state in this hook is calling
+ * drm_atomic_get_old_crtc_state() and
+ * drm_atomic_get_old_connector_state().
+ *
+ * RETURNS:
+ *
+ * An atomic state on success, an error pointer otherwise.
+ */
+ struct drm_plane_state *(*atomic_readout_state)(struct drm_plane *plane,
+ struct drm_atomic_state *state);
+
/**
* @atomic_duplicate_state:
*
* Duplicate the current atomic state for this plane and return it.
* The core and helpers guarantee that any atomic state duplicated with
--
2.50.1On Tue, Sep 02, 2025 at 10:32:38AM +0200, Maxime Ripard wrote:
> In order to enable drivers to fill their initial state from the hardware
> state, we need to provide an alternative atomic_reset helper.
>
> This helper relies on each state having its own atomic_state_readout()
> hooks. Each component will thus be able to fill the initial state based
> on what they can figure out from the hardware.
>
> It also allocates a dummy drm_atomic_state to glue the whole thing
> together so atomic_state_readout implementations can still figure out
> the state of other related entities.
>
> Link: https://lore.kernel.org/dri-devel/CAKMK7uHtqHy_oz4W7F+hmp9iqp7W5Ra8CxPvJ=9BwmvfU-O0gg@mail.gmail.com/
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 382 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_mode_config.c | 1 +
> include/drm/drm_atomic_helper.h | 1 +
> include/drm/drm_bridge.h | 21 ++
> include/drm/drm_connector.h | 26 +++
> include/drm/drm_crtc.h | 19 ++
> include/drm/drm_plane.h | 27 +++
> 7 files changed, 477 insertions(+)
>
> + drm_for_each_encoder(encoder, dev) {
> + struct drm_connector_state *enc_conn_state;
> + struct drm_crtc_state *enc_crtc_state;
> + struct drm_bridge *bridge;
> +
> + /*
> + * It works a bit differently for bridges. Because they are
> + * using a drm_private_state, and because
> + * drm_atomic_private_obj_init() asks for its initial state when
> + * initializing, instead of doing it later on through a reset
> + * call like the other entities, we can't have reset xor
> + * readout.
Would it make sense to unify the way the bridges / priv_obj handle the
state with the rest of the object types?
> + *
> + * We'll need a mandatory reset to create that initial, blank,
> + * state, and then readout will fill that state later on if the
> + * driver implements it.
> + *
> + * This also means we don't need to call the readout state
> + * function if we don't have the bridge enabled (ie, if no
> + * drm_connector_state->best_encoder points to bridge->encoder,
> + * and / or if drm_connector_state->crtc is NULL).
> + *
> + * In such a case, we would get the blank state reset created
> + * during registration.
> + */
> +
> + enc_conn_state = find_connector_state_for_encoder(state, encoder);
> + if (!enc_conn_state)
> + continue;
> +
> + enc_crtc_state = drm_atomic_get_old_crtc_state(state, enc_conn_state->crtc);
> + if (!enc_crtc_state)
> + continue;
> +
> + list_for_each_entry(bridge, &encoder->bridge_chain, chain_node) {
> + const struct drm_bridge_funcs *bridge_funcs = bridge->funcs;
> + struct drm_bridge_state *bridge_state;
> +
> + bridge_state = drm_bridge_get_current_state(bridge);
> + if (WARN_ON(!bridge_state)) {
> + ret = -EINVAL;
> + goto err_state_put;
> + }
> +
> + if (bridge_funcs->atomic_readout_state) {
> + ret = bridge_funcs->atomic_readout_state(bridge,
> + bridge_state,
> + enc_crtc_state,
> + enc_conn_state);
> + if (WARN_ON(ret))
> + goto err_state_put;
> + }
> +
> + drm_atomic_set_old_bridge_state(state, bridge, bridge_state);
> + }
> + }
> +
--
With best wishes
Dmitry
On Mon, Sep 15, 2025 at 09:40:57PM +0300, Dmitry Baryshkov wrote:
> On Tue, Sep 02, 2025 at 10:32:38AM +0200, Maxime Ripard wrote:
> > In order to enable drivers to fill their initial state from the hardware
> > state, we need to provide an alternative atomic_reset helper.
> >
> > This helper relies on each state having its own atomic_state_readout()
> > hooks. Each component will thus be able to fill the initial state based
> > on what they can figure out from the hardware.
> >
> > It also allocates a dummy drm_atomic_state to glue the whole thing
> > together so atomic_state_readout implementations can still figure out
> > the state of other related entities.
> >
> > Link: https://lore.kernel.org/dri-devel/CAKMK7uHtqHy_oz4W7F+hmp9iqp7W5Ra8CxPvJ=9BwmvfU-O0gg@mail.gmail.com/
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 382 ++++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/drm_mode_config.c | 1 +
> > include/drm/drm_atomic_helper.h | 1 +
> > include/drm/drm_bridge.h | 21 ++
> > include/drm/drm_connector.h | 26 +++
> > include/drm/drm_crtc.h | 19 ++
> > include/drm/drm_plane.h | 27 +++
> > 7 files changed, 477 insertions(+)
> >
> > + drm_for_each_encoder(encoder, dev) {
> > + struct drm_connector_state *enc_conn_state;
> > + struct drm_crtc_state *enc_crtc_state;
> > + struct drm_bridge *bridge;
> > +
> > + /*
> > + * It works a bit differently for bridges. Because they are
> > + * using a drm_private_state, and because
> > + * drm_atomic_private_obj_init() asks for its initial state when
> > + * initializing, instead of doing it later on through a reset
> > + * call like the other entities, we can't have reset xor
> > + * readout.
>
> Would it make sense to unify the way the bridges / priv_obj handle the
> state with the rest of the object types?
I would be all for it, but I think this is pretty much the same
conversation we had in my recent bridge improvement series. Aren't
bridges not assumed to have atomic support and thus we can't really do
something better here?
Or should we move all bridges to be atomic?
Maxime
On Tue, Sep 23, 2025 at 11:45:49AM +0200, Maxime Ripard wrote:
> On Mon, Sep 15, 2025 at 09:40:57PM +0300, Dmitry Baryshkov wrote:
> > On Tue, Sep 02, 2025 at 10:32:38AM +0200, Maxime Ripard wrote:
> > > In order to enable drivers to fill their initial state from the hardware
> > > state, we need to provide an alternative atomic_reset helper.
> > >
> > > This helper relies on each state having its own atomic_state_readout()
> > > hooks. Each component will thus be able to fill the initial state based
> > > on what they can figure out from the hardware.
> > >
> > > It also allocates a dummy drm_atomic_state to glue the whole thing
> > > together so atomic_state_readout implementations can still figure out
> > > the state of other related entities.
> > >
> > > Link: https://lore.kernel.org/dri-devel/CAKMK7uHtqHy_oz4W7F+hmp9iqp7W5Ra8CxPvJ=9BwmvfU-O0gg@mail.gmail.com/
> > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 382 ++++++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/drm_mode_config.c | 1 +
> > > include/drm/drm_atomic_helper.h | 1 +
> > > include/drm/drm_bridge.h | 21 ++
> > > include/drm/drm_connector.h | 26 +++
> > > include/drm/drm_crtc.h | 19 ++
> > > include/drm/drm_plane.h | 27 +++
> > > 7 files changed, 477 insertions(+)
> > >
> > > + drm_for_each_encoder(encoder, dev) {
> > > + struct drm_connector_state *enc_conn_state;
> > > + struct drm_crtc_state *enc_crtc_state;
> > > + struct drm_bridge *bridge;
> > > +
> > > + /*
> > > + * It works a bit differently for bridges. Because they are
> > > + * using a drm_private_state, and because
> > > + * drm_atomic_private_obj_init() asks for its initial state when
> > > + * initializing, instead of doing it later on through a reset
> > > + * call like the other entities, we can't have reset xor
> > > + * readout.
> >
> > Would it make sense to unify the way the bridges / priv_obj handle the
> > state with the rest of the object types?
>
> I would be all for it, but I think this is pretty much the same
> conversation we had in my recent bridge improvement series. Aren't
> bridges not assumed to have atomic support and thus we can't really do
> something better here?
>
> Or should we move all bridges to be atomic?
I think I had something smaller on my mind: make drm_private_obj /
drm_bride provide the initial state during the reset call, like all
other entities.
--
With best wishes
Dmitry
Hi
Am 02.09.25 um 10:32 schrieb Maxime Ripard:
> In order to enable drivers to fill their initial state from the hardware
> state, we need to provide an alternative atomic_reset helper.
>
> This helper relies on each state having its own atomic_state_readout()
> hooks. Each component will thus be able to fill the initial state based
> on what they can figure out from the hardware.
>
> It also allocates a dummy drm_atomic_state to glue the whole thing
> together so atomic_state_readout implementations can still figure out
> the state of other related entities.
>
> Link: https://lore.kernel.org/dri-devel/CAKMK7uHtqHy_oz4W7F+hmp9iqp7W5Ra8CxPvJ=9BwmvfU-O0gg@mail.gmail.com/
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 382 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_mode_config.c | 1 +
> include/drm/drm_atomic_helper.h | 1 +
> include/drm/drm_bridge.h | 21 ++
> include/drm/drm_connector.h | 26 +++
> include/drm/drm_crtc.h | 19 ++
> include/drm/drm_plane.h | 27 +++
> 7 files changed, 477 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d5ebe6ea0acbc5a08aef7fa41ecb9ed5d8fa8e80..f59512476ebf2b48e1c7034950bcaf99237f03c6 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -45,10 +45,392 @@
> #include <drm/drm_vblank.h>
> #include <drm/drm_writeback.h>
>
> #include "drm_crtc_helper_internal.h"
> #include "drm_crtc_internal.h"
> +#include "drm_internal.h"
> +
> +static void drm_atomic_set_old_plane_state(struct drm_atomic_state *state,
> + struct drm_plane *plane,
> + struct drm_plane_state *plane_state)
> +{
> + int idx = drm_plane_index(plane);
> +
> + state->planes[idx].old_state = plane_state;
> + state->planes[idx].ptr = plane;
> +
> + drm_dbg_atomic(plane->dev, "Added [PLANE:%d:%s] %p state to %p\n",
> + plane->base.id, plane->name, plane_state, state);
> +}
> +
> +static void drm_atomic_set_old_crtc_state(struct drm_atomic_state *state,
> + struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state)
> +{
> + int idx = drm_crtc_index(crtc);
> +
> + state->crtcs[idx].old_state = crtc_state;
> + state->crtcs[idx].ptr = crtc;
> +
> + drm_dbg_atomic(state->dev, "Added [CRTC:%d:%s] %p state to %p\n",
> + crtc->base.id, crtc->name, crtc_state, state);
> +}
> +
> +static void drm_atomic_set_old_connector_state(struct drm_atomic_state *state,
> + struct drm_connector *conn,
> + struct drm_connector_state *conn_state)
> +{
> + int idx = drm_connector_index(conn);
> +
> + drm_connector_get(conn);
> + state->connectors[idx].old_state = conn_state;
> + state->connectors[idx].ptr = conn;
> + state->num_connector++;
> +
> + drm_dbg_atomic(conn->dev,
> + "Added [CONNECTOR:%d:%s] %p state to %p\n",
> + conn->base.id, conn->name, conn_state, state);
> +}
> +
> +static void drm_atomic_set_old_private_obj_state(struct drm_atomic_state *state,
> + struct drm_private_obj *obj,
> + struct drm_private_state *obj_state)
> +{
> + int idx = state->num_private_objs;
> +
> + memset(&state->private_objs[idx], 0, sizeof(*state->private_objs));
> + state->private_objs[idx].old_state = obj_state;
> + state->private_objs[idx].ptr = obj;
> + state->num_private_objs++;
> +
> + drm_dbg_atomic(state->dev,
> + "Added new private object %p state %p to %p\n", obj,
> + obj_state, state);
> +}
> +
> +static void drm_atomic_set_old_bridge_state(struct drm_atomic_state *state,
> + struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state)
> +{
> + drm_atomic_set_old_private_obj_state(state, &bridge->base, &bridge_state->base);
> +}
> +
> +static struct drm_connector_state *
> +find_connector_state_for_encoder(struct drm_atomic_state *state,
> + struct drm_encoder *encoder)
> +{
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *connector;
> +
> + drm_connector_list_iter_begin(state->dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + struct drm_connector_state *conn_state =
> + drm_atomic_get_old_connector_state(state, connector);
> +
> + if (WARN_ON(!conn_state))
> + continue;
> +
> + if (encoder == conn_state->best_encoder)
> + return conn_state;
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +
> + return NULL;
> +}
> +
> +/**
> + * drm_atomic_helper_install_readout_state - Sets the state pointer from their readout state
> + * @state: drm_atomic_state to initialize the DRM device with.
> + *
> + * This function takes a &struct drm_atomic_state initialized by
> + * drm_atomic_build_readout_state() and sets up the entities state
> + * pointers (ie, &drm_crtc.state and similar) to properly setup the
> + * initial state.
> + */
> +static void
> +drm_atomic_helper_install_readout_state(struct drm_atomic_state *state)
> +{
> + struct drm_connector_state *old_conn_state;
> + struct drm_private_state *old_obj_state;
> + struct drm_plane_state *old_plane_state;
> + struct drm_crtc_state *old_crtc_state;
> + struct drm_connector *connector;
> + struct drm_private_obj *obj;
> + struct drm_plane *plane;
> + struct drm_crtc *crtc;
> + unsigned int i;
> +
> + for_each_old_connector_in_state(state, connector, old_conn_state, i) {
> + connector->state = old_conn_state;
> +
> + drm_connector_put(state->connectors[i].ptr);
> + state->connectors[i].ptr = NULL;
> + state->connectors[i].old_state = NULL;
> + }
> +
> + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> + crtc->state = old_crtc_state;
> +
> + state->crtcs[i].ptr = NULL;
> + state->crtcs[i].old_state = NULL;
> + }
> +
> + for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> + plane->state = old_plane_state;
> +
> + state->planes[i].ptr = NULL;
> + state->planes[i].old_state = NULL;
> + }
> +
> + for_each_old_private_obj_in_state(state, obj, old_obj_state, i) {
> + obj->state = old_obj_state;
> +
> + state->private_objs[i].ptr = NULL;
> + state->private_objs[i].old_state = NULL;
> + }
> +}
> +
> +static unsigned int count_private_obj(struct drm_device *dev)
> +{
> + struct drm_mode_config *config = &dev->mode_config;
> + struct drm_private_obj *obj;
> + unsigned int count = 0;
> +
> + list_for_each_entry(obj, &config->privobj_list, head)
> + count++;
> +
> + return count;
> +}
> +
> +/**
> + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> + * @dev: DRM device to build the state for
> + *
> + * This function allocates a &struct drm_atomic_state, calls the
> + * atomic_readout_state callbacks, and fills the global state old states
> + * by what the callbacks returned.
> + *
> + * Returns:
> + *
> + * A partially initialized &struct drm_atomic_state on success, an error
> + * pointer otherwise.
> + */
> +static struct drm_atomic_state *
> +drm_atomic_build_readout_state(struct drm_device *dev)
> +{
> + struct drm_connector_list_iter conn_iter;
> + struct drm_atomic_state *state;
> + struct drm_mode_config *config =
> + &dev->mode_config;
> + struct drm_connector *connector;
> + struct drm_printer p =
> + drm_info_printer(dev->dev);
> + struct drm_encoder *encoder;
> + struct drm_plane *plane;
> + struct drm_crtc *crtc;
> + int ret;
> +
> + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> +
> + state = drm_atomic_state_alloc(dev);
> + if (WARN_ON(!state))
> + return ERR_PTR(-ENOMEM);
> +
> + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> + if (WARN_ON(!state->connectors)) {
> + ret = -ENOMEM;
> + goto err_state_put;
> + }
> +
> + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> + if (WARN_ON(!state->private_objs)) {
> + ret = -ENOMEM;
> + goto err_state_put;
> + }
> +
> + drm_for_each_crtc(crtc, dev) {
> + const struct drm_crtc_funcs *crtc_funcs =
> + crtc->funcs;
> + struct drm_crtc_state *crtc_state;
> +
> + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> +
> + if (crtc_funcs->atomic_readout_state) {
> + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> + } else if (crtc_funcs->reset) {
> + crtc_funcs->reset(crtc);
> +
> + /*
> + * We don't want to set crtc->state field yet. Let's save and clear it up.
> + */
> + crtc_state = crtc->state;
> + crtc->state = NULL;
Chancing the crtc->state pointer behind the back of the reset callback
seems fragile. We never how if some other piece of the driver refers to
it (although illegally).
For now, wouldn't it be better to require a read-out helper for all
elements of the driver's mode-setting pipeline? The trivial
implementation would copy the existing reset function and keep
crtc->state to NULL.
> + } else {
> + drm_warn(dev, "No CRTC readout or reset implementation.");
> + continue;
> + }
> +
> + if (WARN_ON(IS_ERR(crtc_state))) {
> + ret = PTR_ERR(crtc_state);
> + goto err_state_put;
> + }
> +
> + drm_atomic_set_old_crtc_state(state, crtc, crtc_state);
> + }
> +
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + const struct drm_connector_funcs *conn_funcs =
> + connector->funcs;
> + struct drm_connector_state *conn_state;
> +
> + drm_dbg_kms(dev, "Initializing Connector %s state.\n", connector->name);
> +
> + if (conn_funcs->atomic_readout_state) {
> + conn_state = conn_funcs->atomic_readout_state(connector, state);
> + } else if (conn_funcs->reset) {
> + conn_funcs->reset(connector);
> +
> + /*
> + * We don't want to set connector->state field yet. Let's save and clear it
> + * up.
> + */
> + conn_state = connector->state;
> + connector->state = NULL;
> + } else {
> + drm_warn(dev, "No Connector readout or reset implementation.");
> + continue;
> + }
> +
> + if (WARN_ON(IS_ERR(conn_state))) {
> + ret = PTR_ERR(conn_state);
> + goto err_state_put;
> + }
> +
> + drm_atomic_set_old_connector_state(state, connector, conn_state);
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +
> + WARN_ON(state->num_connector != config->num_connector);
drm_WARN_ON() here and elsewhere.
> +
> + drm_for_each_encoder(encoder, dev) {
> + struct drm_connector_state *enc_conn_state;
> + struct drm_crtc_state *enc_crtc_state;
> + struct drm_bridge *bridge;
> +
> + /*
> + * It works a bit differently for bridges. Because they are
> + * using a drm_private_state, and because
> + * drm_atomic_private_obj_init() asks for its initial state when
> + * initializing, instead of doing it later on through a reset
> + * call like the other entities, we can't have reset xor
> + * readout.
> + *
> + * We'll need a mandatory reset to create that initial, blank,
> + * state, and then readout will fill that state later on if the
> + * driver implements it.
> + *
> + * This also means we don't need to call the readout state
> + * function if we don't have the bridge enabled (ie, if no
> + * drm_connector_state->best_encoder points to bridge->encoder,
> + * and / or if drm_connector_state->crtc is NULL).
> + *
> + * In such a case, we would get the blank state reset created
> + * during registration.
> + */
> +
> + enc_conn_state = find_connector_state_for_encoder(state, encoder);
> + if (!enc_conn_state)
> + continue;
> +
> + enc_crtc_state = drm_atomic_get_old_crtc_state(state, enc_conn_state->crtc);
> + if (!enc_crtc_state)
> + continue;
> +
> + list_for_each_entry(bridge, &encoder->bridge_chain, chain_node) {
> + const struct drm_bridge_funcs *bridge_funcs = bridge->funcs;
> + struct drm_bridge_state *bridge_state;
> +
> + bridge_state = drm_bridge_get_current_state(bridge);
> + if (WARN_ON(!bridge_state)) {
> + ret = -EINVAL;
> + goto err_state_put;
> + }
> +
> + if (bridge_funcs->atomic_readout_state) {
> + ret = bridge_funcs->atomic_readout_state(bridge,
> + bridge_state,
> + enc_crtc_state,
> + enc_conn_state);
> + if (WARN_ON(ret))
> + goto err_state_put;
> + }
> +
> + drm_atomic_set_old_bridge_state(state, bridge, bridge_state);
> + }
> + }
> +
> + drm_for_each_plane(plane, dev) {
> + const struct drm_plane_funcs *plane_funcs =
> + plane->funcs;
> + struct drm_plane_state *plane_state;
> +
> + drm_dbg_kms(dev, "Initializing Plane %s state.\n", plane->name);
> +
> + if (plane_funcs->atomic_readout_state) {
> + plane_state = plane_funcs->atomic_readout_state(plane, state);
> + } else if (plane_funcs->reset) {
> + plane_funcs->reset(plane);
> +
> + /*
> + * We don't want to set conn->state field yet. Let's save and clear it up.
> + */
> + plane_state = plane->state;
> + plane->state = NULL;
> + } else {
> + drm_warn(dev, "No plane readout or reset implementation.");
> + continue;
> + }
> +
> + if (WARN_ON(IS_ERR(plane_state))) {
> + ret = PTR_ERR(plane_state);
> + goto err_state_put;
> + }
> +
> + drm_atomic_set_old_plane_state(state, plane, plane_state);
> + }
> +
> + drm_atomic_print_old_state(state, &p);
> +
> + return state;
> +
> +err_state_put:
> + drm_atomic_state_put(state);
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * drm_atomic_helper_readout_state - Builds an initial state from hardware state
> + * @dev: DRM device to build the state for
> + *
> + * This function creates the initial state for all the entities on a
> + * @dev. Drivers can use this as their
> + * &drm_mode_config_helper_funcs.atomic_reset callback to implement
> + * hardware state readout suppport.
> + */
> +void drm_atomic_helper_readout_state(struct drm_device *dev)
> +{
> + struct drm_atomic_state *state;
> +
> + state = drm_atomic_build_readout_state(dev);
> + if (IS_ERR(state))
> + return;
> +
> + drm_atomic_helper_install_readout_state(state);
> + drm_atomic_state_put(state);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_readout_state);
>
> /**
> * DOC: overview
> *
> * This helper library provides implementations of check and commit functions on
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 82180760032d3490d63fe83136465d2c26551d08..96d38a49be501a0090457cbe96135f82bb1358b5 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -26,10 +26,11 @@
> #include <drm/drm_drv.h>
> #include <drm/drm_encoder.h>
> #include <drm/drm_file.h>
> #include <drm/drm_framebuffer.h>
> #include <drm/drm_managed.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> #include <drm/drm_mode_config.h>
> #include <drm/drm_modeset_helper_vtables.h>
> #include <drm/drm_print.h>
> #include <linux/dma-resv.h>
>
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 53382fe93537bbcda9cee8827bc95de9a515efb5..47902a9181727a08581fb808faabe67d92a755cf 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -45,10 +45,11 @@
>
> struct drm_atomic_state;
> struct drm_private_obj;
> struct drm_private_state;
>
> +void drm_atomic_helper_readout_state(struct drm_device *dev);
> int drm_atomic_helper_check_modeset(struct drm_device *dev,
> struct drm_atomic_state *state);
> int drm_atomic_helper_check_wb_connector_state(struct drm_connector *connector,
> struct drm_atomic_state *state);
> int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 8d9d4fd078e72977677fd992d725261232754e3e..15b63053f01869786831936ba28b7efc1e55e2e8 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -490,10 +490,31 @@ struct drm_bridge_funcs {
> * The @atomic_post_disable callback is optional.
> */
> void (*atomic_post_disable)(struct drm_bridge *bridge,
> struct drm_atomic_state *state);
>
> + /**
> + * @atomic_readout_state:
> + *
> + * Initializes,this bridge atomic state.
> + *
> + * It's meant to be used by drivers that wants to implement fast
'want'
> + * / flicker-free boot and allows to initialize the atomic state
I think we should only call it flicker-free boot. Fast boot is misleading.
> + * from the hardware state left by the firmware.
> + *
> + * It's used at initialization time, so drivers must make sure
> + * that the power state is sensible when accessing the hardware.
> + *
> + * RETURNS:
> + *
> + * 0 on success, an error code otherwise.
> + */
> + int (*atomic_readout_state)(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state);
> +
> /**
> * @atomic_duplicate_state:
> *
> * Duplicate the current bridge state object (which is guaranteed to be
> * non-NULL).
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f34f4b8183d83dccd3e820a444fbf74fb6c16f2..f68bd9627c085c6d2463b847aaa245ccc651f27b 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1464,10 +1464,36 @@ 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_readout_state:
> + *
> + * Allocates, initializes, and returns an atomic state for this
> + * connector.
> + *
> + * It's meant to be used by drivers that wants to implement fast
> + * / flicker-free boot and allows to initialize the atomic state
> + * from the hardware state left by the firmware.
> + *
> + * It's used at initialization time, so drivers must make sure
> + * that the power state is sensible when accessing the hardware.
> + *
> + * The drm_atomic_state being passed is not fully filled. Only
> + * the CRTC state are there when this hooks is called, and only
> + * their old state. The only safe operation one can do on this
> + * state in this hook is calling
> + * drm_atomic_get_old_crtc_state().
> + *
> + * RETURNS:
> + *
> + * An atomic state on success, an error pointer otherwise.
> + */
> + struct drm_connector_state *(*atomic_readout_state)(struct drm_connector *connector,
> + struct drm_atomic_state *state);
> +
> /**
> * @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
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index caa56e039da2a748cf40ebf45b37158acda439d9..c462bd9b2f7d3ae08e669463717002e5f78122fe 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -613,10 +613,29 @@ struct drm_crtc_funcs {
> * 0 on success or a negative error code on failure.
> */
> int (*set_property)(struct drm_crtc *crtc,
> struct drm_property *property, uint64_t val);
>
> + /**
> + * @atomic_readout_state:
> + *
> + * Allocates, initializes, and returns an atomic state for this
> + * CRTC.
> + *
> + * It's meant to be used by drivers that wants to implement fast
> + * / flicker-free boot and allows to initialize the atomic state
> + * from the hardware state left by the firmware.
> + *
> + * It's used at initialization time, so drivers must make sure
> + * that the power state is sensible when accessing the hardware.
> + *
> + * RETURNS:
> + *
> + * An atomic state on success, an error pointer otherwise.
> + */
> + struct drm_crtc_state *(*atomic_readout_state)(struct drm_crtc *crtc);
> +
> /**
> * @atomic_duplicate_state:
> *
> * Duplicate the current atomic state for this CRTC and return it.
> * The core and helpers guarantee that any atomic state duplicated with
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 01479dd94e76a8389a0c9e9d6744400aa2291064..691a267c857a228f674ef02a63fb6d1ff9e379a8 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -378,10 +378,37 @@ struct drm_plane_funcs {
> * 0 on success or a negative error code on failure.
> */
> int (*set_property)(struct drm_plane *plane,
> struct drm_property *property, uint64_t val);
>
> + /**
> + * @atomic_readout_state:
> + *
> + * Allocates, initializes, and returns an atomic state for this
> + * plane.
> + *
> + * It's meant to be used by drivers that wants to implement fast
> + * / flicker-free boot and allows to initialize the atomic state
> + * from the hardware state left by the firmware.
> + *
> + * It's used at initialization time, so drivers must make sure
> + * that the power state is sensible when accessing the hardware.
> + *
> + * The drm_atomic_state being passed is not fully filled. Only
> + * the CRTC and connector states are there when this hooks is
> + * called, and only their old state. The only safe operation one
> + * can do on this state in this hook is calling
> + * drm_atomic_get_old_crtc_state() and
> + * drm_atomic_get_old_connector_state().
> + *
> + * RETURNS:
> + *
> + * An atomic state on success, an error pointer otherwise.
> + */
> + struct drm_plane_state *(*atomic_readout_state)(struct drm_plane *plane,
> + struct drm_atomic_state *state);
> +
> /**
> * @atomic_duplicate_state:
> *
> * Duplicate the current atomic state for this plane and return it.
> * The core and helpers guarantee that any atomic state duplicated with
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Hi Tohmas,
On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > +/**
> > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > + * @dev: DRM device to build the state for
> > + *
> > + * This function allocates a &struct drm_atomic_state, calls the
> > + * atomic_readout_state callbacks, and fills the global state old states
> > + * by what the callbacks returned.
> > + *
> > + * Returns:
> > + *
> > + * A partially initialized &struct drm_atomic_state on success, an error
> > + * pointer otherwise.
> > + */
> > +static struct drm_atomic_state *
> > +drm_atomic_build_readout_state(struct drm_device *dev)
> > +{
> > + struct drm_connector_list_iter conn_iter;
> > + struct drm_atomic_state *state;
> > + struct drm_mode_config *config =
> > + &dev->mode_config;
> > + struct drm_connector *connector;
> > + struct drm_printer p =
> > + drm_info_printer(dev->dev);
> > + struct drm_encoder *encoder;
> > + struct drm_plane *plane;
> > + struct drm_crtc *crtc;
> > + int ret;
> > +
> > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > +
> > + state = drm_atomic_state_alloc(dev);
> > + if (WARN_ON(!state))
> > + return ERR_PTR(-ENOMEM);
> > +
> > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > + if (WARN_ON(!state->connectors)) {
> > + ret = -ENOMEM;
> > + goto err_state_put;
> > + }
> > +
> > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > + if (WARN_ON(!state->private_objs)) {
> > + ret = -ENOMEM;
> > + goto err_state_put;
> > + }
> > +
> > + drm_for_each_crtc(crtc, dev) {
> > + const struct drm_crtc_funcs *crtc_funcs =
> > + crtc->funcs;
> > + struct drm_crtc_state *crtc_state;
> > +
> > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > +
> > + if (crtc_funcs->atomic_readout_state) {
> > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > + } else if (crtc_funcs->reset) {
> > + crtc_funcs->reset(crtc);
> > +
> > + /*
> > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > + */
> > + crtc_state = crtc->state;
> > + crtc->state = NULL;
>
> Chancing the crtc->state pointer behind the back of the reset callback seems
> fragile. We never how if some other piece of the driver refers to it
> (although illegally).
I agree that it's clunky. I'm not sure who would use it at this point
though: we're in the middle of the drm_mode_config_reset(), so the
drivers' involvement is pretty minimal.
I did wonder if changing reset to return the object instead of setting
$OBJECT->state would be a better interface?
> For now, wouldn't it be better to require a read-out helper for all elements
> of the driver's mode-setting pipeline? The trivial implementation would
> copy the existing reset function and keep crtc->state to NULL.
I also considered that, but I'm not sure we can expect bridges to have
readout hooks filled for every configuration in the wild.
But maybe we can look during drm_mode_config_reset() at whether all the
objects have their hook filled, and if not fall back on reset for
everything.
It would make the implementation easier, but missing bridges
implementations would trigger a mode change when it might actually work
just fine since bridge state is pretty minimal.
Idk.
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -490,10 +490,31 @@ struct drm_bridge_funcs {
> > * The @atomic_post_disable callback is optional.
> > */
> > void (*atomic_post_disable)(struct drm_bridge *bridge,
> > struct drm_atomic_state *state);
> > + /**
> > + * @atomic_readout_state:
> > + *
> > + * Initializes,this bridge atomic state.
> > + *
> > + * It's meant to be used by drivers that wants to implement fast
>
> 'want'
>
> > + * / flicker-free boot and allows to initialize the atomic state
>
> I think we should only call it flicker-free boot. Fast boot is misleading.
I agree, but it's also how it's been called by the only implementation
of it we have so far (i915), and the name of the module parameter that
controls it.
Maxime
On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> Hi Tohmas,
>
> On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > +/**
> > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > + * @dev: DRM device to build the state for
> > > + *
> > > + * This function allocates a &struct drm_atomic_state, calls the
> > > + * atomic_readout_state callbacks, and fills the global state old states
> > > + * by what the callbacks returned.
> > > + *
> > > + * Returns:
> > > + *
> > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > + * pointer otherwise.
> > > + */
> > > +static struct drm_atomic_state *
> > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > +{
> > > + struct drm_connector_list_iter conn_iter;
> > > + struct drm_atomic_state *state;
> > > + struct drm_mode_config *config =
> > > + &dev->mode_config;
> > > + struct drm_connector *connector;
> > > + struct drm_printer p =
> > > + drm_info_printer(dev->dev);
> > > + struct drm_encoder *encoder;
> > > + struct drm_plane *plane;
> > > + struct drm_crtc *crtc;
> > > + int ret;
> > > +
> > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > +
> > > + state = drm_atomic_state_alloc(dev);
> > > + if (WARN_ON(!state))
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > + if (WARN_ON(!state->connectors)) {
> > > + ret = -ENOMEM;
> > > + goto err_state_put;
> > > + }
> > > +
> > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > + if (WARN_ON(!state->private_objs)) {
> > > + ret = -ENOMEM;
> > > + goto err_state_put;
> > > + }
> > > +
> > > + drm_for_each_crtc(crtc, dev) {
> > > + const struct drm_crtc_funcs *crtc_funcs =
> > > + crtc->funcs;
> > > + struct drm_crtc_state *crtc_state;
> > > +
> > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > +
> > > + if (crtc_funcs->atomic_readout_state) {
> > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > + } else if (crtc_funcs->reset) {
> > > + crtc_funcs->reset(crtc);
> > > +
> > > + /*
> > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > + */
> > > + crtc_state = crtc->state;
> > > + crtc->state = NULL;
> >
> > Chancing the crtc->state pointer behind the back of the reset callback seems
> > fragile. We never how if some other piece of the driver refers to it
> > (although illegally).
>
> I agree that it's clunky. I'm not sure who would use it at this point
> though: we're in the middle of the drm_mode_config_reset(), so the
> drivers' involvement is pretty minimal.
>
> I did wonder if changing reset to return the object instead of setting
> $OBJECT->state would be a better interface?
>
> > For now, wouldn't it be better to require a read-out helper for all elements
> > of the driver's mode-setting pipeline? The trivial implementation would
> > copy the existing reset function and keep crtc->state to NULL.
>
> I also considered that, but I'm not sure we can expect bridges to have
> readout hooks filled for every configuration in the wild.
>
> But maybe we can look during drm_mode_config_reset() at whether all the
> objects have their hook filled, and if not fall back on reset for
> everything.
>
> It would make the implementation easier, but missing bridges
> implementations would trigger a mode change when it might actually work
> just fine since bridge state is pretty minimal.
DP bridge drivers have a pretty big state (DPCD and all the features).
Other bridge drivers randomly leak state to the non-state structs.
>
> Idk.
>
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -490,10 +490,31 @@ struct drm_bridge_funcs {
> > > * The @atomic_post_disable callback is optional.
> > > */
> > > void (*atomic_post_disable)(struct drm_bridge *bridge,
> > > struct drm_atomic_state *state);
> > > + /**
> > > + * @atomic_readout_state:
> > > + *
> > > + * Initializes,this bridge atomic state.
> > > + *
> > > + * It's meant to be used by drivers that wants to implement fast
> >
> > 'want'
> >
> > > + * / flicker-free boot and allows to initialize the atomic state
> >
> > I think we should only call it flicker-free boot. Fast boot is misleading.
>
> I agree, but it's also how it's been called by the only implementation
> of it we have so far (i915), and the name of the module parameter that
> controls it.
I hope to be able to try implementing this for drm/msm (at least it will
allow us to check how does that play with bridges).
>
> Maxime
--
With best wishes
Dmitry
On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > Hi Tohmas,
> >
> > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > +/**
> > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > + * @dev: DRM device to build the state for
> > > > + *
> > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > + * by what the callbacks returned.
> > > > + *
> > > > + * Returns:
> > > > + *
> > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > + * pointer otherwise.
> > > > + */
> > > > +static struct drm_atomic_state *
> > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > +{
> > > > + struct drm_connector_list_iter conn_iter;
> > > > + struct drm_atomic_state *state;
> > > > + struct drm_mode_config *config =
> > > > + &dev->mode_config;
> > > > + struct drm_connector *connector;
> > > > + struct drm_printer p =
> > > > + drm_info_printer(dev->dev);
> > > > + struct drm_encoder *encoder;
> > > > + struct drm_plane *plane;
> > > > + struct drm_crtc *crtc;
> > > > + int ret;
> > > > +
> > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > +
> > > > + state = drm_atomic_state_alloc(dev);
> > > > + if (WARN_ON(!state))
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > + if (WARN_ON(!state->connectors)) {
> > > > + ret = -ENOMEM;
> > > > + goto err_state_put;
> > > > + }
> > > > +
> > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > + if (WARN_ON(!state->private_objs)) {
> > > > + ret = -ENOMEM;
> > > > + goto err_state_put;
> > > > + }
> > > > +
> > > > + drm_for_each_crtc(crtc, dev) {
> > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > + crtc->funcs;
> > > > + struct drm_crtc_state *crtc_state;
> > > > +
> > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > +
> > > > + if (crtc_funcs->atomic_readout_state) {
> > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > + } else if (crtc_funcs->reset) {
> > > > + crtc_funcs->reset(crtc);
> > > > +
> > > > + /*
> > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > + */
> > > > + crtc_state = crtc->state;
> > > > + crtc->state = NULL;
> > >
> > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > fragile. We never how if some other piece of the driver refers to it
> > > (although illegally).
> >
> > I agree that it's clunky. I'm not sure who would use it at this point
> > though: we're in the middle of the drm_mode_config_reset(), so the
> > drivers' involvement is pretty minimal.
> >
> > I did wonder if changing reset to return the object instead of setting
> > $OBJECT->state would be a better interface?
> >
> > > For now, wouldn't it be better to require a read-out helper for all elements
> > > of the driver's mode-setting pipeline? The trivial implementation would
> > > copy the existing reset function and keep crtc->state to NULL.
> >
> > I also considered that, but I'm not sure we can expect bridges to have
> > readout hooks filled for every configuration in the wild.
> >
> > But maybe we can look during drm_mode_config_reset() at whether all the
> > objects have their hook filled, and if not fall back on reset for
> > everything.
> >
> > It would make the implementation easier, but missing bridges
> > implementations would trigger a mode change when it might actually work
> > just fine since bridge state is pretty minimal.
>
> DP bridge drivers have a pretty big state (DPCD and all the features).
I meant drm_bridge_state. Subclasses would have their own implementation
anyway.
> Other bridge drivers randomly leak state to the non-state structs.
I'm not sure what you mean by that though. Can you expand?
Thanks!
Maxime
On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > Hi Tohmas,
> > >
> > > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > > +/**
> > > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > > + * @dev: DRM device to build the state for
> > > > > + *
> > > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > > + * by what the callbacks returned.
> > > > > + *
> > > > > + * Returns:
> > > > > + *
> > > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > > + * pointer otherwise.
> > > > > + */
> > > > > +static struct drm_atomic_state *
> > > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > > +{
> > > > > + struct drm_connector_list_iter conn_iter;
> > > > > + struct drm_atomic_state *state;
> > > > > + struct drm_mode_config *config =
> > > > > + &dev->mode_config;
> > > > > + struct drm_connector *connector;
> > > > > + struct drm_printer p =
> > > > > + drm_info_printer(dev->dev);
> > > > > + struct drm_encoder *encoder;
> > > > > + struct drm_plane *plane;
> > > > > + struct drm_crtc *crtc;
> > > > > + int ret;
> > > > > +
> > > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > > +
> > > > > + state = drm_atomic_state_alloc(dev);
> > > > > + if (WARN_ON(!state))
> > > > > + return ERR_PTR(-ENOMEM);
> > > > > +
> > > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > > + if (WARN_ON(!state->connectors)) {
> > > > > + ret = -ENOMEM;
> > > > > + goto err_state_put;
> > > > > + }
> > > > > +
> > > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > > + if (WARN_ON(!state->private_objs)) {
> > > > > + ret = -ENOMEM;
> > > > > + goto err_state_put;
> > > > > + }
> > > > > +
> > > > > + drm_for_each_crtc(crtc, dev) {
> > > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > > + crtc->funcs;
> > > > > + struct drm_crtc_state *crtc_state;
> > > > > +
> > > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > > +
> > > > > + if (crtc_funcs->atomic_readout_state) {
> > > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > > + } else if (crtc_funcs->reset) {
> > > > > + crtc_funcs->reset(crtc);
> > > > > +
> > > > > + /*
> > > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > > + */
> > > > > + crtc_state = crtc->state;
> > > > > + crtc->state = NULL;
> > > >
> > > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > > fragile. We never how if some other piece of the driver refers to it
> > > > (although illegally).
> > >
> > > I agree that it's clunky. I'm not sure who would use it at this point
> > > though: we're in the middle of the drm_mode_config_reset(), so the
> > > drivers' involvement is pretty minimal.
> > >
> > > I did wonder if changing reset to return the object instead of setting
> > > $OBJECT->state would be a better interface?
> > >
> > > > For now, wouldn't it be better to require a read-out helper for all elements
> > > > of the driver's mode-setting pipeline? The trivial implementation would
> > > > copy the existing reset function and keep crtc->state to NULL.
> > >
> > > I also considered that, but I'm not sure we can expect bridges to have
> > > readout hooks filled for every configuration in the wild.
> > >
> > > But maybe we can look during drm_mode_config_reset() at whether all the
> > > objects have their hook filled, and if not fall back on reset for
> > > everything.
> > >
> > > It would make the implementation easier, but missing bridges
> > > implementations would trigger a mode change when it might actually work
> > > just fine since bridge state is pretty minimal.
> >
> > DP bridge drivers have a pretty big state (DPCD and all the features).
>
> I meant drm_bridge_state. Subclasses would have their own implementation
> anyway.
>
> > Other bridge drivers randomly leak state to the non-state structs.
>
> I'm not sure what you mean by that though. Can you expand?
I think I've seen bridge drivers which stored subclassed drm_bridge
instead of drm_bridge_state or stored the data in the long-living data
structures. YEs, that's a bug, which should be fixed on its own.
--
With best wishes
Dmitry
On Tue, Sep 23, 2025 at 01:28:57PM +0300, Dmitry Baryshkov wrote:
> On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> > On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > > Hi Tohmas,
> > > >
> > > > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > > > +/**
> > > > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > > > + * @dev: DRM device to build the state for
> > > > > > + *
> > > > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > > > + * by what the callbacks returned.
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + *
> > > > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > > > + * pointer otherwise.
> > > > > > + */
> > > > > > +static struct drm_atomic_state *
> > > > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > > > +{
> > > > > > + struct drm_connector_list_iter conn_iter;
> > > > > > + struct drm_atomic_state *state;
> > > > > > + struct drm_mode_config *config =
> > > > > > + &dev->mode_config;
> > > > > > + struct drm_connector *connector;
> > > > > > + struct drm_printer p =
> > > > > > + drm_info_printer(dev->dev);
> > > > > > + struct drm_encoder *encoder;
> > > > > > + struct drm_plane *plane;
> > > > > > + struct drm_crtc *crtc;
> > > > > > + int ret;
> > > > > > +
> > > > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > > > +
> > > > > > + state = drm_atomic_state_alloc(dev);
> > > > > > + if (WARN_ON(!state))
> > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > +
> > > > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > > > + if (WARN_ON(!state->connectors)) {
> > > > > > + ret = -ENOMEM;
> > > > > > + goto err_state_put;
> > > > > > + }
> > > > > > +
> > > > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > > > + if (WARN_ON(!state->private_objs)) {
> > > > > > + ret = -ENOMEM;
> > > > > > + goto err_state_put;
> > > > > > + }
> > > > > > +
> > > > > > + drm_for_each_crtc(crtc, dev) {
> > > > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > > > + crtc->funcs;
> > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > +
> > > > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > > > +
> > > > > > + if (crtc_funcs->atomic_readout_state) {
> > > > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > > > + } else if (crtc_funcs->reset) {
> > > > > > + crtc_funcs->reset(crtc);
> > > > > > +
> > > > > > + /*
> > > > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > > > + */
> > > > > > + crtc_state = crtc->state;
> > > > > > + crtc->state = NULL;
> > > > >
> > > > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > > > fragile. We never how if some other piece of the driver refers to it
> > > > > (although illegally).
> > > >
> > > > I agree that it's clunky. I'm not sure who would use it at this point
> > > > though: we're in the middle of the drm_mode_config_reset(), so the
> > > > drivers' involvement is pretty minimal.
> > > >
> > > > I did wonder if changing reset to return the object instead of setting
> > > > $OBJECT->state would be a better interface?
> > > >
> > > > > For now, wouldn't it be better to require a read-out helper for all elements
> > > > > of the driver's mode-setting pipeline? The trivial implementation would
> > > > > copy the existing reset function and keep crtc->state to NULL.
> > > >
> > > > I also considered that, but I'm not sure we can expect bridges to have
> > > > readout hooks filled for every configuration in the wild.
> > > >
> > > > But maybe we can look during drm_mode_config_reset() at whether all the
> > > > objects have their hook filled, and if not fall back on reset for
> > > > everything.
> > > >
> > > > It would make the implementation easier, but missing bridges
> > > > implementations would trigger a mode change when it might actually work
> > > > just fine since bridge state is pretty minimal.
> > >
> > > DP bridge drivers have a pretty big state (DPCD and all the features).
> >
> > I meant drm_bridge_state. Subclasses would have their own implementation
> > anyway.
> >
> > > Other bridge drivers randomly leak state to the non-state structs.
> >
> > I'm not sure what you mean by that though. Can you expand?
>
> I think I've seen bridge drivers which stored subclassed drm_bridge
> instead of drm_bridge_state or stored the data in the long-living data
> structures. YEs, that's a bug, which should be fixed on its own.
Yeah, I'm not sure how we can defend against that. If the driver doesn't
handle the state well, then it's a driver's problem, and it would
probably create other problems anyway
Maxime
On Tue, Sep 23, 2025 at 01:28:57PM +0300, Dmitry Baryshkov wrote:
> On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> > On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > > > +/**
> > > > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > > > + * @dev: DRM device to build the state for
> > > > > > + *
> > > > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > > > + * by what the callbacks returned.
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + *
> > > > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > > > + * pointer otherwise.
> > > > > > + */
> > > > > > +static struct drm_atomic_state *
> > > > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > > > +{
> > > > > > + struct drm_connector_list_iter conn_iter;
> > > > > > + struct drm_atomic_state *state;
> > > > > > + struct drm_mode_config *config =
> > > > > > + &dev->mode_config;
> > > > > > + struct drm_connector *connector;
> > > > > > + struct drm_printer p =
> > > > > > + drm_info_printer(dev->dev);
> > > > > > + struct drm_encoder *encoder;
> > > > > > + struct drm_plane *plane;
> > > > > > + struct drm_crtc *crtc;
> > > > > > + int ret;
> > > > > > +
> > > > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > > > +
> > > > > > + state = drm_atomic_state_alloc(dev);
> > > > > > + if (WARN_ON(!state))
> > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > +
> > > > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > > > + if (WARN_ON(!state->connectors)) {
> > > > > > + ret = -ENOMEM;
> > > > > > + goto err_state_put;
> > > > > > + }
> > > > > > +
> > > > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > > > + if (WARN_ON(!state->private_objs)) {
> > > > > > + ret = -ENOMEM;
> > > > > > + goto err_state_put;
> > > > > > + }
> > > > > > +
> > > > > > + drm_for_each_crtc(crtc, dev) {
> > > > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > > > + crtc->funcs;
> > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > +
> > > > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > > > +
> > > > > > + if (crtc_funcs->atomic_readout_state) {
> > > > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > > > + } else if (crtc_funcs->reset) {
> > > > > > + crtc_funcs->reset(crtc);
> > > > > > +
> > > > > > + /*
> > > > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > > > + */
> > > > > > + crtc_state = crtc->state;
> > > > > > + crtc->state = NULL;
> > > > >
> > > > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > > > fragile. We never how if some other piece of the driver refers to it
> > > > > (although illegally).
> > > >
> > > > I agree that it's clunky. I'm not sure who would use it at this point
> > > > though: we're in the middle of the drm_mode_config_reset(), so the
> > > > drivers' involvement is pretty minimal.
> > > >
> > > > I did wonder if changing reset to return the object instead of setting
> > > > $OBJECT->state would be a better interface?
> > > >
> > > > > For now, wouldn't it be better to require a read-out helper for all elements
> > > > > of the driver's mode-setting pipeline? The trivial implementation would
> > > > > copy the existing reset function and keep crtc->state to NULL.
> > > >
> > > > I also considered that, but I'm not sure we can expect bridges to have
> > > > readout hooks filled for every configuration in the wild.
> > > >
> > > > But maybe we can look during drm_mode_config_reset() at whether all the
> > > > objects have their hook filled, and if not fall back on reset for
> > > > everything.
> > > >
> > > > It would make the implementation easier, but missing bridges
> > > > implementations would trigger a mode change when it might actually work
> > > > just fine since bridge state is pretty minimal.
> > >
> > > DP bridge drivers have a pretty big state (DPCD and all the features).
> >
> > I meant drm_bridge_state. Subclasses would have their own implementation
> > anyway.
> >
> > > Other bridge drivers randomly leak state to the non-state structs.
> >
> > I'm not sure what you mean by that though. Can you expand?
>
> I think I've seen bridge drivers which stored subclassed drm_bridge
> instead of drm_bridge_state or stored the data in the long-living data
> structures. YEs, that's a bug, which should be fixed on its own.
There's one exception to the "all state in state structure" rules if I
understand things correctly (and I'm mentioning it here partly to be
corrected if my understanding is wrong). Active state data that needs to
be accessed from a non-sleepable context need to be copied to the
device-specific structure when the state is applied, as we can't take
the mutex protecting state access there. I'd expect that to be uncommon
for bridges.
--
Regards,
Laurent Pinchart
On Tue, Sep 23, 2025 at 01:32:23PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 23, 2025 at 01:28:57PM +0300, Dmitry Baryshkov wrote:
> > On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> > > On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > > > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > > > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > > > > +/**
> > > > > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > > > > + * @dev: DRM device to build the state for
> > > > > > > + *
> > > > > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > > > > + * by what the callbacks returned.
> > > > > > > + *
> > > > > > > + * Returns:
> > > > > > > + *
> > > > > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > > > > + * pointer otherwise.
> > > > > > > + */
> > > > > > > +static struct drm_atomic_state *
> > > > > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > > > > +{
> > > > > > > + struct drm_connector_list_iter conn_iter;
> > > > > > > + struct drm_atomic_state *state;
> > > > > > > + struct drm_mode_config *config =
> > > > > > > + &dev->mode_config;
> > > > > > > + struct drm_connector *connector;
> > > > > > > + struct drm_printer p =
> > > > > > > + drm_info_printer(dev->dev);
> > > > > > > + struct drm_encoder *encoder;
> > > > > > > + struct drm_plane *plane;
> > > > > > > + struct drm_crtc *crtc;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > > > > +
> > > > > > > + state = drm_atomic_state_alloc(dev);
> > > > > > > + if (WARN_ON(!state))
> > > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > > +
> > > > > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > > > > + if (WARN_ON(!state->connectors)) {
> > > > > > > + ret = -ENOMEM;
> > > > > > > + goto err_state_put;
> > > > > > > + }
> > > > > > > +
> > > > > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > > > > + if (WARN_ON(!state->private_objs)) {
> > > > > > > + ret = -ENOMEM;
> > > > > > > + goto err_state_put;
> > > > > > > + }
> > > > > > > +
> > > > > > > + drm_for_each_crtc(crtc, dev) {
> > > > > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > > > > + crtc->funcs;
> > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > +
> > > > > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > > > > +
> > > > > > > + if (crtc_funcs->atomic_readout_state) {
> > > > > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > > > > + } else if (crtc_funcs->reset) {
> > > > > > > + crtc_funcs->reset(crtc);
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > > > > + */
> > > > > > > + crtc_state = crtc->state;
> > > > > > > + crtc->state = NULL;
> > > > > >
> > > > > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > > > > fragile. We never how if some other piece of the driver refers to it
> > > > > > (although illegally).
> > > > >
> > > > > I agree that it's clunky. I'm not sure who would use it at this point
> > > > > though: we're in the middle of the drm_mode_config_reset(), so the
> > > > > drivers' involvement is pretty minimal.
> > > > >
> > > > > I did wonder if changing reset to return the object instead of setting
> > > > > $OBJECT->state would be a better interface?
> > > > >
> > > > > > For now, wouldn't it be better to require a read-out helper for all elements
> > > > > > of the driver's mode-setting pipeline? The trivial implementation would
> > > > > > copy the existing reset function and keep crtc->state to NULL.
> > > > >
> > > > > I also considered that, but I'm not sure we can expect bridges to have
> > > > > readout hooks filled for every configuration in the wild.
> > > > >
> > > > > But maybe we can look during drm_mode_config_reset() at whether all the
> > > > > objects have their hook filled, and if not fall back on reset for
> > > > > everything.
> > > > >
> > > > > It would make the implementation easier, but missing bridges
> > > > > implementations would trigger a mode change when it might actually work
> > > > > just fine since bridge state is pretty minimal.
> > > >
> > > > DP bridge drivers have a pretty big state (DPCD and all the features).
> > >
> > > I meant drm_bridge_state. Subclasses would have their own implementation
> > > anyway.
> > >
> > > > Other bridge drivers randomly leak state to the non-state structs.
> > >
> > > I'm not sure what you mean by that though. Can you expand?
> >
> > I think I've seen bridge drivers which stored subclassed drm_bridge
> > instead of drm_bridge_state or stored the data in the long-living data
> > structures. YEs, that's a bug, which should be fixed on its own.
>
> There's one exception to the "all state in state structure" rules if I
> understand things correctly (and I'm mentioning it here partly to be
> corrected if my understanding is wrong). Active state data that needs to
> be accessed from a non-sleepable context need to be copied to the
> device-specific structure when the state is applied, as we can't take
> the mutex protecting state access there. I'd expect that to be uncommon
> for bridges.
It's not really about non-sleepable, it's more about everything that is
outside of DRM's locking scheme. So interrupt handlers are one of those
indeed, but CEC, ALSA, debugfs, etc. also are.
Maxime
On Tue, Sep 23, 2025 at 01:32:23PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 23, 2025 at 01:28:57PM +0300, Dmitry Baryshkov wrote:
> > On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> > > On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > > > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > > > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > > > > +/**
> > > > > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > > > > + * @dev: DRM device to build the state for
> > > > > > > + *
> > > > > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > > > > + * by what the callbacks returned.
> > > > > > > + *
> > > > > > > + * Returns:
> > > > > > > + *
> > > > > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > > > > + * pointer otherwise.
> > > > > > > + */
> > > > > > > +static struct drm_atomic_state *
> > > > > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > > > > +{
> > > > > > > + struct drm_connector_list_iter conn_iter;
> > > > > > > + struct drm_atomic_state *state;
> > > > > > > + struct drm_mode_config *config =
> > > > > > > + &dev->mode_config;
> > > > > > > + struct drm_connector *connector;
> > > > > > > + struct drm_printer p =
> > > > > > > + drm_info_printer(dev->dev);
> > > > > > > + struct drm_encoder *encoder;
> > > > > > > + struct drm_plane *plane;
> > > > > > > + struct drm_crtc *crtc;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > > > > +
> > > > > > > + state = drm_atomic_state_alloc(dev);
> > > > > > > + if (WARN_ON(!state))
> > > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > > +
> > > > > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > > > > + if (WARN_ON(!state->connectors)) {
> > > > > > > + ret = -ENOMEM;
> > > > > > > + goto err_state_put;
> > > > > > > + }
> > > > > > > +
> > > > > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > > > > + if (WARN_ON(!state->private_objs)) {
> > > > > > > + ret = -ENOMEM;
> > > > > > > + goto err_state_put;
> > > > > > > + }
> > > > > > > +
> > > > > > > + drm_for_each_crtc(crtc, dev) {
> > > > > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > > > > + crtc->funcs;
> > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > +
> > > > > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > > > > +
> > > > > > > + if (crtc_funcs->atomic_readout_state) {
> > > > > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > > > > + } else if (crtc_funcs->reset) {
> > > > > > > + crtc_funcs->reset(crtc);
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > > > > + */
> > > > > > > + crtc_state = crtc->state;
> > > > > > > + crtc->state = NULL;
> > > > > >
> > > > > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > > > > fragile. We never how if some other piece of the driver refers to it
> > > > > > (although illegally).
> > > > >
> > > > > I agree that it's clunky. I'm not sure who would use it at this point
> > > > > though: we're in the middle of the drm_mode_config_reset(), so the
> > > > > drivers' involvement is pretty minimal.
> > > > >
> > > > > I did wonder if changing reset to return the object instead of setting
> > > > > $OBJECT->state would be a better interface?
> > > > >
> > > > > > For now, wouldn't it be better to require a read-out helper for all elements
> > > > > > of the driver's mode-setting pipeline? The trivial implementation would
> > > > > > copy the existing reset function and keep crtc->state to NULL.
> > > > >
> > > > > I also considered that, but I'm not sure we can expect bridges to have
> > > > > readout hooks filled for every configuration in the wild.
> > > > >
> > > > > But maybe we can look during drm_mode_config_reset() at whether all the
> > > > > objects have their hook filled, and if not fall back on reset for
> > > > > everything.
> > > > >
> > > > > It would make the implementation easier, but missing bridges
> > > > > implementations would trigger a mode change when it might actually work
> > > > > just fine since bridge state is pretty minimal.
> > > >
> > > > DP bridge drivers have a pretty big state (DPCD and all the features).
> > >
> > > I meant drm_bridge_state. Subclasses would have their own implementation
> > > anyway.
> > >
> > > > Other bridge drivers randomly leak state to the non-state structs.
> > >
> > > I'm not sure what you mean by that though. Can you expand?
> >
> > I think I've seen bridge drivers which stored subclassed drm_bridge
> > instead of drm_bridge_state or stored the data in the long-living data
> > structures. YEs, that's a bug, which should be fixed on its own.
>
> There's one exception to the "all state in state structure" rules if I
> understand things correctly (and I'm mentioning it here partly to be
> corrected if my understanding is wrong). Active state data that needs to
> be accessed from a non-sleepable context need to be copied to the
> device-specific structure when the state is applied, as we can't take
> the mutex protecting state access there.
I'd even relax this slightly (please correct me if I'm wrong here): the
state can be extracted to the device-specific structure in
atomic_pre_enable() and cleared in atomic_post_disable().
Likewise it's generally fine to store non-state changing data in the
private data (e.g. hw params or infoframe from the HDMI codec data).
> I'd expect that to be uncommon
> for bridges.
--
With best wishes
Dmitry
On Tue, Sep 23, 2025 at 01:43:29PM +0300, Dmitry Baryshkov wrote:
> On Tue, Sep 23, 2025 at 01:32:23PM +0300, Laurent Pinchart wrote:
> > On Tue, Sep 23, 2025 at 01:28:57PM +0300, Dmitry Baryshkov wrote:
> > > On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> > > > On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > > > > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > > > > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > > > > > +/**
> > > > > > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > > > > > + * @dev: DRM device to build the state for
> > > > > > > > + *
> > > > > > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > > > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > > > > > + * by what the callbacks returned.
> > > > > > > > + *
> > > > > > > > + * Returns:
> > > > > > > > + *
> > > > > > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > > > > > + * pointer otherwise.
> > > > > > > > + */
> > > > > > > > +static struct drm_atomic_state *
> > > > > > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > > > > > +{
> > > > > > > > + struct drm_connector_list_iter conn_iter;
> > > > > > > > + struct drm_atomic_state *state;
> > > > > > > > + struct drm_mode_config *config =
> > > > > > > > + &dev->mode_config;
> > > > > > > > + struct drm_connector *connector;
> > > > > > > > + struct drm_printer p =
> > > > > > > > + drm_info_printer(dev->dev);
> > > > > > > > + struct drm_encoder *encoder;
> > > > > > > > + struct drm_plane *plane;
> > > > > > > > + struct drm_crtc *crtc;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > > > > > +
> > > > > > > > + state = drm_atomic_state_alloc(dev);
> > > > > > > > + if (WARN_ON(!state))
> > > > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > > > +
> > > > > > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > > > > > + if (WARN_ON(!state->connectors)) {
> > > > > > > > + ret = -ENOMEM;
> > > > > > > > + goto err_state_put;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > > > > > + if (WARN_ON(!state->private_objs)) {
> > > > > > > > + ret = -ENOMEM;
> > > > > > > > + goto err_state_put;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + drm_for_each_crtc(crtc, dev) {
> > > > > > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > > > > > + crtc->funcs;
> > > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > > +
> > > > > > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > > > > > +
> > > > > > > > + if (crtc_funcs->atomic_readout_state) {
> > > > > > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > > > > > + } else if (crtc_funcs->reset) {
> > > > > > > > + crtc_funcs->reset(crtc);
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > > > > > + */
> > > > > > > > + crtc_state = crtc->state;
> > > > > > > > + crtc->state = NULL;
> > > > > > >
> > > > > > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > > > > > fragile. We never how if some other piece of the driver refers to it
> > > > > > > (although illegally).
> > > > > >
> > > > > > I agree that it's clunky. I'm not sure who would use it at this point
> > > > > > though: we're in the middle of the drm_mode_config_reset(), so the
> > > > > > drivers' involvement is pretty minimal.
> > > > > >
> > > > > > I did wonder if changing reset to return the object instead of setting
> > > > > > $OBJECT->state would be a better interface?
> > > > > >
> > > > > > > For now, wouldn't it be better to require a read-out helper for all elements
> > > > > > > of the driver's mode-setting pipeline? The trivial implementation would
> > > > > > > copy the existing reset function and keep crtc->state to NULL.
> > > > > >
> > > > > > I also considered that, but I'm not sure we can expect bridges to have
> > > > > > readout hooks filled for every configuration in the wild.
> > > > > >
> > > > > > But maybe we can look during drm_mode_config_reset() at whether all the
> > > > > > objects have their hook filled, and if not fall back on reset for
> > > > > > everything.
> > > > > >
> > > > > > It would make the implementation easier, but missing bridges
> > > > > > implementations would trigger a mode change when it might actually work
> > > > > > just fine since bridge state is pretty minimal.
> > > > >
> > > > > DP bridge drivers have a pretty big state (DPCD and all the features).
> > > >
> > > > I meant drm_bridge_state. Subclasses would have their own implementation
> > > > anyway.
> > > >
> > > > > Other bridge drivers randomly leak state to the non-state structs.
> > > >
> > > > I'm not sure what you mean by that though. Can you expand?
> > >
> > > I think I've seen bridge drivers which stored subclassed drm_bridge
> > > instead of drm_bridge_state or stored the data in the long-living data
> > > structures. YEs, that's a bug, which should be fixed on its own.
> >
> > There's one exception to the "all state in state structure" rules if I
> > understand things correctly (and I'm mentioning it here partly to be
> > corrected if my understanding is wrong). Active state data that needs to
> > be accessed from a non-sleepable context need to be copied to the
> > device-specific structure when the state is applied, as we can't take
> > the mutex protecting state access there.
>
> I'd even relax this slightly (please correct me if I'm wrong here): the
> state can be extracted to the device-specific structure in
> atomic_pre_enable() and cleared in atomic_post_disable().
Overall I agree. I think this should be minimized when reasonably
possible to avoid data duplication though.
> Likewise it's generally fine to store non-state changing data in the
> private data (e.g. hw params or infoframe from the HDMI codec data).
Most hardware parameters are a translation of the software state. I
would store them in the state when possible (e.g. when they need to be
computed at atomic check time, and when the computation is expensive
enough to make caching useful), and store in private data only the data
that needs to also be accessed from a location where state access is not
easily possible.
> > I'd expect that to be uncommon
> > for bridges.
--
Regards,
Laurent Pinchart
On Tue, Sep 23, 2025 at 02:41:16PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 23, 2025 at 01:43:29PM +0300, Dmitry Baryshkov wrote:
> > On Tue, Sep 23, 2025 at 01:32:23PM +0300, Laurent Pinchart wrote:
> > > On Tue, Sep 23, 2025 at 01:28:57PM +0300, Dmitry Baryshkov wrote:
> > > > On Tue, Sep 23, 2025 at 11:38:17AM +0200, Maxime Ripard wrote:
> > > > > On Mon, Sep 15, 2025 at 09:38:44PM +0300, Dmitry Baryshkov wrote:
> > > > > > On Mon, Sep 15, 2025 at 10:42:22AM +0200, Maxime Ripard wrote:
> > > > > > > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > > > > > > +/**
> > > > > > > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > > > > > > + * @dev: DRM device to build the state for
> > > > > > > > > + *
> > > > > > > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > > > > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > > > > > > + * by what the callbacks returned.
> > > > > > > > > + *
> > > > > > > > > + * Returns:
> > > > > > > > > + *
> > > > > > > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > > > > > > + * pointer otherwise.
> > > > > > > > > + */
> > > > > > > > > +static struct drm_atomic_state *
> > > > > > > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > > > > > > +{
> > > > > > > > > + struct drm_connector_list_iter conn_iter;
> > > > > > > > > + struct drm_atomic_state *state;
> > > > > > > > > + struct drm_mode_config *config =
> > > > > > > > > + &dev->mode_config;
> > > > > > > > > + struct drm_connector *connector;
> > > > > > > > > + struct drm_printer p =
> > > > > > > > > + drm_info_printer(dev->dev);
> > > > > > > > > + struct drm_encoder *encoder;
> > > > > > > > > + struct drm_plane *plane;
> > > > > > > > > + struct drm_crtc *crtc;
> > > > > > > > > + int ret;
> > > > > > > > > +
> > > > > > > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > > > > > > +
> > > > > > > > > + state = drm_atomic_state_alloc(dev);
> > > > > > > > > + if (WARN_ON(!state))
> > > > > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > > > > +
> > > > > > > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > > > > > > + if (WARN_ON(!state->connectors)) {
> > > > > > > > > + ret = -ENOMEM;
> > > > > > > > > + goto err_state_put;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > > > > > > + if (WARN_ON(!state->private_objs)) {
> > > > > > > > > + ret = -ENOMEM;
> > > > > > > > > + goto err_state_put;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + drm_for_each_crtc(crtc, dev) {
> > > > > > > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > > > > > > + crtc->funcs;
> > > > > > > > > + struct drm_crtc_state *crtc_state;
> > > > > > > > > +
> > > > > > > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > > > > > > +
> > > > > > > > > + if (crtc_funcs->atomic_readout_state) {
> > > > > > > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > > > > > > + } else if (crtc_funcs->reset) {
> > > > > > > > > + crtc_funcs->reset(crtc);
> > > > > > > > > +
> > > > > > > > > + /*
> > > > > > > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > > > > > > + */
> > > > > > > > > + crtc_state = crtc->state;
> > > > > > > > > + crtc->state = NULL;
> > > > > > > >
> > > > > > > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > > > > > > fragile. We never how if some other piece of the driver refers to it
> > > > > > > > (although illegally).
> > > > > > >
> > > > > > > I agree that it's clunky. I'm not sure who would use it at this point
> > > > > > > though: we're in the middle of the drm_mode_config_reset(), so the
> > > > > > > drivers' involvement is pretty minimal.
> > > > > > >
> > > > > > > I did wonder if changing reset to return the object instead of setting
> > > > > > > $OBJECT->state would be a better interface?
> > > > > > >
> > > > > > > > For now, wouldn't it be better to require a read-out helper for all elements
> > > > > > > > of the driver's mode-setting pipeline? The trivial implementation would
> > > > > > > > copy the existing reset function and keep crtc->state to NULL.
> > > > > > >
> > > > > > > I also considered that, but I'm not sure we can expect bridges to have
> > > > > > > readout hooks filled for every configuration in the wild.
> > > > > > >
> > > > > > > But maybe we can look during drm_mode_config_reset() at whether all the
> > > > > > > objects have their hook filled, and if not fall back on reset for
> > > > > > > everything.
> > > > > > >
> > > > > > > It would make the implementation easier, but missing bridges
> > > > > > > implementations would trigger a mode change when it might actually work
> > > > > > > just fine since bridge state is pretty minimal.
> > > > > >
> > > > > > DP bridge drivers have a pretty big state (DPCD and all the features).
> > > > >
> > > > > I meant drm_bridge_state. Subclasses would have their own implementation
> > > > > anyway.
> > > > >
> > > > > > Other bridge drivers randomly leak state to the non-state structs.
> > > > >
> > > > > I'm not sure what you mean by that though. Can you expand?
> > > >
> > > > I think I've seen bridge drivers which stored subclassed drm_bridge
> > > > instead of drm_bridge_state or stored the data in the long-living data
> > > > structures. YEs, that's a bug, which should be fixed on its own.
> > >
> > > There's one exception to the "all state in state structure" rules if I
> > > understand things correctly (and I'm mentioning it here partly to be
> > > corrected if my understanding is wrong). Active state data that needs to
> > > be accessed from a non-sleepable context need to be copied to the
> > > device-specific structure when the state is applied, as we can't take
> > > the mutex protecting state access there.
> >
> > I'd even relax this slightly (please correct me if I'm wrong here): the
> > state can be extracted to the device-specific structure in
> > atomic_pre_enable() and cleared in atomic_post_disable().
>
> Overall I agree. I think this should be minimized when reasonably
> possible to avoid data duplication though.
>
> > Likewise it's generally fine to store non-state changing data in the
> > private data (e.g. hw params or infoframe from the HDMI codec data).
>
> Most hardware parameters are a translation of the software state. I
> would store them in the state when possible (e.g. when they need to be
> computed at atomic check time, and when the computation is expensive
> enough to make caching useful), and store in private data only the data
> that needs to also be accessed from a location where state access is not
> easily possible.
I'd agree here.
>
> > > I'd expect that to be uncommon
> > > for bridges.
--
With best wishes
Dmitry
Hi
Am 15.09.25 um 10:42 schrieb Maxime Ripard:
> Hi Tohmas,
>
> On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
>>> +/**
>>> + * drm_atomic_build_readout_state - Creates an initial state from the hardware
>>> + * @dev: DRM device to build the state for
>>> + *
>>> + * This function allocates a &struct drm_atomic_state, calls the
>>> + * atomic_readout_state callbacks, and fills the global state old states
>>> + * by what the callbacks returned.
>>> + *
>>> + * Returns:
>>> + *
>>> + * A partially initialized &struct drm_atomic_state on success, an error
>>> + * pointer otherwise.
>>> + */
>>> +static struct drm_atomic_state *
>>> +drm_atomic_build_readout_state(struct drm_device *dev)
>>> +{
>>> + struct drm_connector_list_iter conn_iter;
>>> + struct drm_atomic_state *state;
>>> + struct drm_mode_config *config =
>>> + &dev->mode_config;
>>> + struct drm_connector *connector;
>>> + struct drm_printer p =
>>> + drm_info_printer(dev->dev);
>>> + struct drm_encoder *encoder;
>>> + struct drm_plane *plane;
>>> + struct drm_crtc *crtc;
>>> + int ret;
>>> +
>>> + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
>>> +
>>> + state = drm_atomic_state_alloc(dev);
>>> + if (WARN_ON(!state))
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
>>> + if (WARN_ON(!state->connectors)) {
>>> + ret = -ENOMEM;
>>> + goto err_state_put;
>>> + }
>>> +
>>> + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
>>> + if (WARN_ON(!state->private_objs)) {
>>> + ret = -ENOMEM;
>>> + goto err_state_put;
>>> + }
>>> +
>>> + drm_for_each_crtc(crtc, dev) {
>>> + const struct drm_crtc_funcs *crtc_funcs =
>>> + crtc->funcs;
>>> + struct drm_crtc_state *crtc_state;
>>> +
>>> + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
>>> +
>>> + if (crtc_funcs->atomic_readout_state) {
>>> + crtc_state = crtc_funcs->atomic_readout_state(crtc);
>>> + } else if (crtc_funcs->reset) {
>>> + crtc_funcs->reset(crtc);
>>> +
>>> + /*
>>> + * We don't want to set crtc->state field yet. Let's save and clear it up.
>>> + */
>>> + crtc_state = crtc->state;
>>> + crtc->state = NULL;
>> Chancing the crtc->state pointer behind the back of the reset callback seems
>> fragile. We never how if some other piece of the driver refers to it
>> (although illegally).
> I agree that it's clunky. I'm not sure who would use it at this point
> though: we're in the middle of the drm_mode_config_reset(), so the
> drivers' involvement is pretty minimal.
>
> I did wonder if changing reset to return the object instead of setting
> $OBJECT->state would be a better interface?
Probably not. The reset helper is supposed to initialize the object's
software and hardware state. But in most drivers, we're currently mostly
setting the minimal software state here and simply assume that hardware
is off. Returning the state would water down semantics even further.
Having said that, I could imaging building an atomic_clean_state
callback that replaces the reset callback. It would work alongside the
new atomic_readout_state callback. Current reset could be build upon
that callback. The atomic_clean_state would intentionally only take care
of the software state and leave hardware state undefined. This reflects
the current realities of most DRM drivers. From that clean state, DRM
could do an atomic commit that also initializes the hardware.
>
>> For now, wouldn't it be better to require a read-out helper for all elements
>> of the driver's mode-setting pipeline? The trivial implementation would
>> copy the existing reset function and keep crtc->state to NULL.
> I also considered that, but I'm not sure we can expect bridges to have
> readout hooks filled for every configuration in the wild.
>
> But maybe we can look during drm_mode_config_reset() at whether all the
> objects have their hook filled, and if not fall back on reset for
> everything.
That's what I meant, I think.
>
> It would make the implementation easier, but missing bridges
> implementations would trigger a mode change when it might actually work
> just fine since bridge state is pretty minimal.
If there's an element in the pipeline that's missing the readout helper,
it might be safer to fallback to that modeset instead of ending up with
inconsistent state.
Best regards
Thomas
>
> Idk.
>
>>> --- a/include/drm/drm_bridge.h
>>> +++ b/include/drm/drm_bridge.h
>>> @@ -490,10 +490,31 @@ struct drm_bridge_funcs {
>>> * The @atomic_post_disable callback is optional.
>>> */
>>> void (*atomic_post_disable)(struct drm_bridge *bridge,
>>> struct drm_atomic_state *state);
>>> + /**
>>> + * @atomic_readout_state:
>>> + *
>>> + * Initializes,this bridge atomic state.
>>> + *
>>> + * It's meant to be used by drivers that wants to implement fast
>> 'want'
>>
>>> + * / flicker-free boot and allows to initialize the atomic state
>> I think we should only call it flicker-free boot. Fast boot is misleading.
> I agree, but it's also how it's been called by the only implementation
> of it we have so far (i915), and the name of the module parameter that
> controls it.
>
> Maxime
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
On Mon, Sep 15, 2025 at 11:11:39AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 15.09.25 um 10:42 schrieb Maxime Ripard:
> > Hi Tohmas,
> >
> > On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
> > > > +/**
> > > > + * drm_atomic_build_readout_state - Creates an initial state from the hardware
> > > > + * @dev: DRM device to build the state for
> > > > + *
> > > > + * This function allocates a &struct drm_atomic_state, calls the
> > > > + * atomic_readout_state callbacks, and fills the global state old states
> > > > + * by what the callbacks returned.
> > > > + *
> > > > + * Returns:
> > > > + *
> > > > + * A partially initialized &struct drm_atomic_state on success, an error
> > > > + * pointer otherwise.
> > > > + */
> > > > +static struct drm_atomic_state *
> > > > +drm_atomic_build_readout_state(struct drm_device *dev)
> > > > +{
> > > > + struct drm_connector_list_iter conn_iter;
> > > > + struct drm_atomic_state *state;
> > > > + struct drm_mode_config *config =
> > > > + &dev->mode_config;
> > > > + struct drm_connector *connector;
> > > > + struct drm_printer p =
> > > > + drm_info_printer(dev->dev);
> > > > + struct drm_encoder *encoder;
> > > > + struct drm_plane *plane;
> > > > + struct drm_crtc *crtc;
> > > > + int ret;
> > > > +
> > > > + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> > > > +
> > > > + state = drm_atomic_state_alloc(dev);
> > > > + if (WARN_ON(!state))
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
> > > > + if (WARN_ON(!state->connectors)) {
> > > > + ret = -ENOMEM;
> > > > + goto err_state_put;
> > > > + }
> > > > +
> > > > + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
> > > > + if (WARN_ON(!state->private_objs)) {
> > > > + ret = -ENOMEM;
> > > > + goto err_state_put;
> > > > + }
> > > > +
> > > > + drm_for_each_crtc(crtc, dev) {
> > > > + const struct drm_crtc_funcs *crtc_funcs =
> > > > + crtc->funcs;
> > > > + struct drm_crtc_state *crtc_state;
> > > > +
> > > > + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
> > > > +
> > > > + if (crtc_funcs->atomic_readout_state) {
> > > > + crtc_state = crtc_funcs->atomic_readout_state(crtc);
> > > > + } else if (crtc_funcs->reset) {
> > > > + crtc_funcs->reset(crtc);
> > > > +
> > > > + /*
> > > > + * We don't want to set crtc->state field yet. Let's save and clear it up.
> > > > + */
> > > > + crtc_state = crtc->state;
> > > > + crtc->state = NULL;
> > > Chancing the crtc->state pointer behind the back of the reset callback seems
> > > fragile. We never how if some other piece of the driver refers to it
> > > (although illegally).
> > I agree that it's clunky. I'm not sure who would use it at this point
> > though: we're in the middle of the drm_mode_config_reset(), so the
> > drivers' involvement is pretty minimal.
> >
> > I did wonder if changing reset to return the object instead of setting
> > $OBJECT->state would be a better interface?
>
> Probably not. The reset helper is supposed to initialize the object's
> software and hardware state. But in most drivers, we're currently mostly
> setting the minimal software state here and simply assume that hardware is
> off. Returning the state would water down semantics even further.
>
> Having said that, I could imaging building an atomic_clean_state callback
> that replaces the reset callback. It would work alongside the new
> atomic_readout_state callback. Current reset could be build upon that
> callback. The atomic_clean_state would intentionally only take care of the
> software state and leave hardware state undefined. This reflects the current
> realities of most DRM drivers. From that clean state, DRM could do an
> atomic commit that also initializes the hardware.
That sounds like a good idea, but I wonder how we would deal with reset
then? Should we remove it entirely? Still call it? What do you think?
> > > For now, wouldn't it be better to require a read-out helper for all elements
> > > of the driver's mode-setting pipeline? The trivial implementation would
> > > copy the existing reset function and keep crtc->state to NULL.
> > I also considered that, but I'm not sure we can expect bridges to have
> > readout hooks filled for every configuration in the wild.
> >
> > But maybe we can look during drm_mode_config_reset() at whether all the
> > objects have their hook filled, and if not fall back on reset for
> > everything.
>
> That's what I meant, I think.
>
> > It would make the implementation easier, but missing bridges
> > implementations would trigger a mode change when it might actually work
> > just fine since bridge state is pretty minimal.
>
> If there's an element in the pipeline that's missing the readout helper, it
> might be safer to fallback to that modeset instead of ending up with
> inconsistent state.
Yeah, that sounds fair.
Maxime
Hi
Am 23.09.25 um 11:37 schrieb Maxime Ripard:
> On Mon, Sep 15, 2025 at 11:11:39AM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 15.09.25 um 10:42 schrieb Maxime Ripard:
>>> Hi Tohmas,
>>>
>>> On Tue, Sep 02, 2025 at 03:44:54PM +0200, Thomas Zimmermann wrote:
>>>>> +/**
>>>>> + * drm_atomic_build_readout_state - Creates an initial state from the hardware
>>>>> + * @dev: DRM device to build the state for
>>>>> + *
>>>>> + * This function allocates a &struct drm_atomic_state, calls the
>>>>> + * atomic_readout_state callbacks, and fills the global state old states
>>>>> + * by what the callbacks returned.
>>>>> + *
>>>>> + * Returns:
>>>>> + *
>>>>> + * A partially initialized &struct drm_atomic_state on success, an error
>>>>> + * pointer otherwise.
>>>>> + */
>>>>> +static struct drm_atomic_state *
>>>>> +drm_atomic_build_readout_state(struct drm_device *dev)
>>>>> +{
>>>>> + struct drm_connector_list_iter conn_iter;
>>>>> + struct drm_atomic_state *state;
>>>>> + struct drm_mode_config *config =
>>>>> + &dev->mode_config;
>>>>> + struct drm_connector *connector;
>>>>> + struct drm_printer p =
>>>>> + drm_info_printer(dev->dev);
>>>>> + struct drm_encoder *encoder;
>>>>> + struct drm_plane *plane;
>>>>> + struct drm_crtc *crtc;
>>>>> + int ret;
>>>>> +
>>>>> + drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
>>>>> +
>>>>> + state = drm_atomic_state_alloc(dev);
>>>>> + if (WARN_ON(!state))
>>>>> + return ERR_PTR(-ENOMEM);
>>>>> +
>>>>> + state->connectors = kcalloc(config->num_connector, sizeof(*state->connectors), GFP_KERNEL);
>>>>> + if (WARN_ON(!state->connectors)) {
>>>>> + ret = -ENOMEM;
>>>>> + goto err_state_put;
>>>>> + }
>>>>> +
>>>>> + state->private_objs = kcalloc(count_private_obj(dev), sizeof(*state->private_objs), GFP_KERNEL);
>>>>> + if (WARN_ON(!state->private_objs)) {
>>>>> + ret = -ENOMEM;
>>>>> + goto err_state_put;
>>>>> + }
>>>>> +
>>>>> + drm_for_each_crtc(crtc, dev) {
>>>>> + const struct drm_crtc_funcs *crtc_funcs =
>>>>> + crtc->funcs;
>>>>> + struct drm_crtc_state *crtc_state;
>>>>> +
>>>>> + drm_dbg_kms(dev, "Initializing CRTC %s state.\n", crtc->name);
>>>>> +
>>>>> + if (crtc_funcs->atomic_readout_state) {
>>>>> + crtc_state = crtc_funcs->atomic_readout_state(crtc);
>>>>> + } else if (crtc_funcs->reset) {
>>>>> + crtc_funcs->reset(crtc);
>>>>> +
>>>>> + /*
>>>>> + * We don't want to set crtc->state field yet. Let's save and clear it up.
>>>>> + */
>>>>> + crtc_state = crtc->state;
>>>>> + crtc->state = NULL;
>>>> Chancing the crtc->state pointer behind the back of the reset callback seems
>>>> fragile. We never how if some other piece of the driver refers to it
>>>> (although illegally).
>>> I agree that it's clunky. I'm not sure who would use it at this point
>>> though: we're in the middle of the drm_mode_config_reset(), so the
>>> drivers' involvement is pretty minimal.
>>>
>>> I did wonder if changing reset to return the object instead of setting
>>> $OBJECT->state would be a better interface?
>> Probably not. The reset helper is supposed to initialize the object's
>> software and hardware state. But in most drivers, we're currently mostly
>> setting the minimal software state here and simply assume that hardware is
>> off. Returning the state would water down semantics even further.
>>
>> Having said that, I could imaging building an atomic_clean_state callback
>> that replaces the reset callback. It would work alongside the new
>> atomic_readout_state callback. Current reset could be build upon that
>> callback. The atomic_clean_state would intentionally only take care of the
>> software state and leave hardware state undefined. This reflects the current
>> realities of most DRM drivers. From that clean state, DRM could do an
>> atomic commit that also initializes the hardware.
> That sounds like a good idea, but I wonder how we would deal with reset
> then? Should we remove it entirely? Still call it? What do you think?
I think in that design, a reset would be atomic_clean_state +
atomic_commit. The clean_state step would likely always happen, were we
currently do the reset. In the long term, the commit step could be moved
into drm_dev_register() or even the DRM client setup. The later place
should make it easy for in-kernel DRM clients to adopt the current
display config. If there's no client enabled, a plain atomic_commit
would set the HW to whatever is in the state.
The state could, of course, also come from atomic_readout_state in which
case in which the display would just keep running as-is on the
atomic_commit.
Best regards
Thomas
>
>>>> For now, wouldn't it be better to require a read-out helper for all elements
>>>> of the driver's mode-setting pipeline? The trivial implementation would
>>>> copy the existing reset function and keep crtc->state to NULL.
>>> I also considered that, but I'm not sure we can expect bridges to have
>>> readout hooks filled for every configuration in the wild.
>>>
>>> But maybe we can look during drm_mode_config_reset() at whether all the
>>> objects have their hook filled, and if not fall back on reset for
>>> everything.
>> That's what I meant, I think.
>>
>>> It would make the implementation easier, but missing bridges
>>> implementations would trigger a mode change when it might actually work
>>> just fine since bridge state is pretty minimal.
>> If there's an element in the pipeline that's missing the readout helper, it
>> might be safer to fallback to that modeset instead of ending up with
>> inconsistent state.
> Yeah, that sounds fair.
>
> Maxime
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
© 2016 - 2025 Red Hat, Inc.