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)
