Re: [PATCH 01/14] drm/atomic: Document atomic state lifetime

[Tomi Valkeinen]

Hi,

On 10/03/2026 18:06, Maxime Ripard wrote:
> How drm_atomic_state structures and the various entity structures are
> allocated and freed isn't really trivial, so let's document it.
> 
> Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
> ---
>  Documentation/gpu/drm-kms.rst |  6 +++++
>  drivers/gpu/drm/drm_atomic.c  | 52 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 2292e65f044c3bdebafbb8f83dfe7ac12e831273..017c7b196ed7ead4cf5fa8572e1f977d9e00dda8 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -280,10 +280,16 @@ structure, ordering of committing state changes to hardware is sequenced using
>  :c:type:`struct drm_crtc_commit <drm_crtc_commit>`.
>  
>  Read on in this chapter, and also in :ref:`drm_atomic_helper` for more detailed
>  coverage of specific topics.
>  
> +Atomic State Lifetime
> +---------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_atomic.c
> +   :doc: state lifetime
> +
>  Handling Driver Private State
>  -----------------------------
>  
>  .. kernel-doc:: drivers/gpu/drm/drm_atomic.c
>     :doc: handling driver private state
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 4283ab4d06c581727cc98b1dc870bf69691ea654..92c6afc8f22c8307a59dc266aacdb8e03351409d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -45,10 +45,62 @@
>  #include <drm/drm_colorop.h>
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
>  
> +/**
> + * DOC: state lifetime
> + *
> + * &struct drm_atomic_state represents an update to video pipeline
> + * state. Despite its confusing name, it's actually a transient object
> + * that holds a state update as a collection of pointer to individual
> + * objects states. &struct drm_atomic_state has a much shorter lifetime

Hmm, I think "a collection of pointers to individual object states". Hmm
or "objects' states"? I like the former.

> + * than the objects states, since it's only allocated while preparing,

"objects' states" or "object states".

> + * checking or doing the update, while object states are allocated while
> + * the state will be, or is active in the hardware.
> + *
> + * Their respective lifetimes are:
> + *
> + * - at reset time, the object reset implementation will allocate a new,
> + *   default, state and will store it in the object state pointer.

"object's". This is the "active state", is it?

> + *
> + * - whenever a new update is needed:
> + *
> + *   + we allocate a new &struct drm_atomic_state using drm_atomic_state_alloc().
> + *
> + *   + we copy the state of each affected entity into our &struct
> + *     drm_atomic_state using drm_atomic_get_plane_state(),
> + *     drm_atomic_get_crtc_state(), drm_atomic_get_connector_state(), or
> + *     drm_atomic_get_private_obj_state(). That state can then be
> + *     modified.

Maybe clarify what is the state returned by these. It's the "active
state", isn't it, drm_crtc.state or similar?

> + *
> + *     At that point, &struct drm_atomic_state stores three state
> + *     pointers for that particular entity: the old, new, and existing
> + *     (called "state") states. The old state is the state currently
> + *     active in the hardware, which is either the one initialized by
> + *     reset() or a newer one if a commit has been made. The new state
> + *     is the state we just allocated and we might eventually commit to
> + *     the hardware. The existing state points to the state we'll
> + *     eventually have to free when the drm_atomic_state will be
> + *     destroyed, but points to the new state for now.

>From this, I don't understand the difference between the old state and
the existing state. And if the existing state is the one we'll free,
isn't that the old state, not new state? Oh, is the existing state a
state we have to free when the drm_atomic_state would is freed? And at
this point the new state is the one, as it's not committed?

> + *
> + *   + After the state is populated, it is checked. If the check is
> + *     successful, the update is committed. Part of the commit is a call
> + *     to drm_atomic_helper_swap_state() which will turn the new states
> + *     into the active states. Doing so involves updating the objects

"object's"

> + *     state pointer (&drm_crtc.state or similar) to point to the new
> + *     state, and the existing states will now point to the old states,
> + *     that used to be active but isn't anymore.

"aren't"

I think I understand this, but... It kind of brings in a new state
concept, "active state".

> + *
> + *   + When the commit is done, and when all references to our &struct
> + *     drm_atomic_state are put, drm_atomic_state_clear() runs and will
> + *     free all the old states.
> + *
> + *   + Now, we don't have any active &struct drm_atomic_state anymore,
> + *     and only the entity active states remain allocated.
> + */
> +
Even if this is a bit hard to read, I think it really clarifies the
state lifetime.

 Tomi