[PATCH 4/5] drm/atomic: Make the drm_atomic_state documentation less ambiguous

[Pekka Paalanen]

On Mon,  4 Dec 2023 13:17:06 +0100
Maxime Ripard <mripard at kernel.org> wrote:

> The current documentation of drm_atomic_state says that it's the "global
> state object". This is confusing since, while it does contain all the
> objects affected by an update and their respective states, if an object
> isn't affected by this update it won't be part of it.
> 
> Thus, it's not truly a "global state", unlike object state structures
> that do contain the entire state of a given object.
> 
> Signed-off-by: Maxime Ripard <mripard at kernel.org>
> ---
>  include/drm/drm_atomic.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 914574b58ae7..81ad7369b90d 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -346,11 +346,19 @@ struct __drm_private_objs_state {
>  };
>  
>  /**
> - * struct drm_atomic_state - the global state object for atomic updates
> + * struct drm_atomic_state - Atomic Update structure
> + *
> + * This structure is the kernel counterpart of @drm_mode_atomic and contains
> + * all the objects affected by an atomic modeset update and their states.

Drop "modeset"? An update can be without a modeset.

>   *
>   * States are added to an atomic update by calling drm_atomic_get_crtc_state(),
>   * drm_atomic_get_plane_state(), drm_atomic_get_connector_state(), or for
>   * private state structures, drm_atomic_get_private_obj_state().
> + *
> + * NOTE: While this structure looks to be global and affecting the whole DRM
> + * device, it only contains the objects affected by the atomic commit.

This new phrasing is much more clear to me than the old one.

> + * Unaffected objects will not be part of that update, unless they have been
> + * explicitly added by either the framework or the driver.

If the framework or a driver adds an object, then it's no longer
unaffected, is it?

Should there be some discussion how this struct starts with only what
userspace mentioned, and more affected objects may be added by the
framework or a driver? And adding more objects can surprise the
userspace and cause even failures (e.g. random, hard-to-diagnose EBUSY
errors from atomic commit when a driver added a CRTC that was not
supposed to be affected)? Even unexpected failures on *future* atomic
commits, as in the CRTC example.

Was there actually a rule of when the kernel can add unmentioned
objects, like needing ALLOW_MODESET from userspace?

It's fine to leave those details for later if you want.

>   */
>  struct drm_atomic_state {
>  	/**


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231205/f004fb67/attachment.sig>