[PATCH v2 1/5] drm/atomic: Move the drm_atomic_state field doc inline

[PATCH v2 1/5] drm/atomic: Move the drm_atomic_state field doc inline

[Maxime Ripard]

Some fields of drm_atomic_state have been documented in-line, but some
were documented in the main kerneldoc block before the structure.

Since the former is the preferred option in DRM, let's move all the
fields to an inline documentation.

Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 include/drm/drm_atomic.h | 50 ++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index cf8e1220a4ac..2a08030fcd75 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -347,24 +347,22 @@ struct __drm_private_objs_state {
 
 /**
  * struct drm_atomic_state - the global state object for atomic updates
- * @ref: count of all references to this state (will not be freed until zero)
- * @dev: parent DRM device
- * @async_update: hint for asynchronous plane update
- * @planes: pointer to array of structures with per-plane data
- * @crtcs: pointer to array of CRTC pointers
- * @num_connector: size of the @connectors and @connector_states arrays
- * @connectors: pointer to array of structures with per-connector data
- * @num_private_objs: size of the @private_objs array
- * @private_objs: pointer to array of private object pointers
- * @acquire_ctx: acquire context for this atomic modeset state update
  *
  * 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().
  */
 struct drm_atomic_state {
+	/**
+	 * @ref:
+	 *
+	 * Count of all references to this update (will not be freed until zero).
+	 */
 	struct kref ref;
 
+	/**
+	 * @dev: Parent DRM Device.
+	 */
 	struct drm_device *dev;
 
 	/**
@@ -388,7 +386,12 @@ struct drm_atomic_state {
 	 * flag are not allowed.
 	 */
 	bool legacy_cursor_update : 1;
+
+	/**
+	 * @async_update: hint for asynchronous plane update
+	 */
 	bool async_update : 1;
+
 	/**
 	 * @duplicated:
 	 *
@@ -398,13 +401,40 @@ struct drm_atomic_state {
 	 * states.
 	 */
 	bool duplicated : 1;
+
+	/**
+	 * @planes: pointer to array of structures with per-plane data
+	 */
 	struct __drm_planes_state *planes;
+
+	/**
+	 * @crtcs: pointer to array of CRTC pointers
+	 */
 	struct __drm_crtcs_state *crtcs;
+
+	/**
+	 * @num_connector: size of the @connectors and @connector_states arrays
+	 */
 	int num_connector;
+
+	/**
+	 * @connectors: pointer to array of structures with per-connector data
+	 */
 	struct __drm_connnectors_state *connectors;
+
+	/**
+	 * @num_private_objs: size of the @private_objs array
+	 */
 	int num_private_objs;
+
+	/**
+	 * @private_objs: pointer to array of private object pointers
+	 */
 	struct __drm_private_objs_state *private_objs;
 
+	/**
+	 * @acquire_ctx: acquire context for this atomic modeset state update
+	 */
 	struct drm_modeset_acquire_ctx *acquire_ctx;
 
 	/**
-- 
2.43.0

[PATCH v2 2/5] drm/atomic: Remove inexistent reference

[Maxime Ripard]

Commit 63e83c1dba54 ("drm: Consolidate connector arrays in
drm_atomic_state") removed the connector_states field but didn't remove
its mention in the num_connectors documentation.

Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 include/drm/drm_atomic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 2a08030fcd75..13cecdc1257d 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -413,7 +413,7 @@ struct drm_atomic_state {
 	struct __drm_crtcs_state *crtcs;
 
 	/**
-	 * @num_connector: size of the @connectors and @connector_states arrays
+	 * @num_connector: size of the @connectors array
 	 */
 	int num_connector;
 
-- 
2.43.0

[PATCH v2 3/5] drm/atomic: Rework the object doc a bit

[Maxime Ripard]

Commits 63e83c1dba54 ("drm: Consolidate connector arrays in
drm_atomic_state"), b8b5342b699b ("drm: Consolidate plane arrays in
drm_atomic_state") and 5d943aa6c0d4 ("drm: Consolidate crtc arrays in
drm_atomic_state") moved the object pointer and their state pointer to
an intermediate structure storing both.

The CRTC commit didn't update the doc of the crtcs field to reflect
that, and the doc for the planes and connectors fields mention that they
are pointers to an array of structures with per-$OBJECT data.

The private_objs field was added later on by commit b430c27a7de3 ("drm:
Add driver-private objects to atomic state") reusing the same sentence
than the crtcs field, probably due to copy and paste.

While these fields are indeed pointers to an array, each item of that
array contain a pointer to the object structure affected by the update,
and its old and new state. There's no per-object data there, and there's
more than just a pointer to the objects.

Let's rephrase those fields a bit to better match the current situation.

Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 include/drm/drm_atomic.h | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 13cecdc1257d..914574b58ae7 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -403,12 +403,18 @@ struct drm_atomic_state {
 	bool duplicated : 1;
 
 	/**
-	 * @planes: pointer to array of structures with per-plane data
+	 * @planes:
+	 *
+	 * Pointer to array of @drm_plane and @drm_plane_state part of this
+	 * update.
 	 */
 	struct __drm_planes_state *planes;
 
 	/**
-	 * @crtcs: pointer to array of CRTC pointers
+	 * @crtcs:
+	 *
+	 * Pointer to array of @drm_crtc and @drm_crtc_state part of this
+	 * update.
 	 */
 	struct __drm_crtcs_state *crtcs;
 
@@ -418,7 +424,10 @@ struct drm_atomic_state {
 	int num_connector;
 
 	/**
-	 * @connectors: pointer to array of structures with per-connector data
+	 * @connectors:
+	 *
+	 * Pointer to array of @drm_connector and @drm_connector_state part of
+	 * this update.
 	 */
 	struct __drm_connnectors_state *connectors;
 
@@ -428,7 +437,10 @@ struct drm_atomic_state {
 	int num_private_objs;
 
 	/**
-	 * @private_objs: pointer to array of private object pointers
+	 * @private_objs:
+	 *
+	 * Pointer to array of @drm_private_obj and @drm_private_obj_state part
+	 * of this update.
 	 */
 	struct __drm_private_objs_state *private_objs;
 
-- 
2.43.0

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

[Maxime Ripard]

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@kernel.org>
---
 include/drm/drm_atomic.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 914574b58ae7..5df67e587816 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -346,7 +346,13 @@ struct __drm_private_objs_state {
 };
 
 /**
- * struct drm_atomic_state - the global state object for atomic updates
+ * struct drm_atomic_state - Atomic commit structure
+ *
+ * This structure is the kernel counterpart of @drm_mode_atomic and represents
+ * an atomic commit that transitions from an old to a new display state. It
+ * contains all the objects affected by an atomic commits and both the new
+ * state structures and pointers to the old state structures for
+ * these.
  *
  * 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
-- 
2.43.0

[PATCH v2 5/5] drm/todo: Add entry to rename drm_atomic_state

[Maxime Ripard]

The name of the structure drm_atomic_state is confusing. Let's add an
entry to our todo list to rename it.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 Documentation/gpu/todo.rst | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 41a264bf84ce..fb9ad120b141 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -120,6 +120,29 @@ Contact: Daniel Vetter, respective driver maintainers
 
 Level: Advanced
 
+Rename drm_atomic_state
+-----------------------
+
+The KMS framework uses two slightly different definitions for the ``state``
+concept. For a given object (plane, CRTC, encoder, etc., so
+``drm_$OBJECT_state``), the state is the entire state of that object. However,
+at the device level, ``drm_atomic_state`` refers to a state update for a
+limited number of objects.
+
+The state isn't the entire device state, but only the full state of some
+objects in that device. This is confusing to newcomers, and
+``drm_atomic_state`` should be renamed to something clearer like
+``drm_atomic_commit``.
+
+In addition to renaming the structure itself, it would also imply renaming some
+related functions (``drm_atomic_state_alloc``, ``drm_atomic_state_get``,
+``drm_atomic_state_put``, ``drm_atomic_state_init``,
+``__drm_atomic_state_free``, etc.).
+
+Contact: Maxime Ripard <mripard@kernel.org>
+
+Level: Advanced
+
 Fallout from atomic KMS
 -----------------------
 
-- 
2.43.0

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

[Hamza Mahfooz]

On 12/14/23 05:09, Maxime Ripard 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@kernel.org>

Reviewed-by: Hamza Mahfooz <hamza.mahfooz@amd.com>

> ---
>   include/drm/drm_atomic.h | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 914574b58ae7..5df67e587816 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -346,7 +346,13 @@ struct __drm_private_objs_state {
>   };
>   
>   /**
> - * struct drm_atomic_state - the global state object for atomic updates
> + * struct drm_atomic_state - Atomic commit structure
> + *
> + * This structure is the kernel counterpart of @drm_mode_atomic and represents
> + * an atomic commit that transitions from an old to a new display state. It
> + * contains all the objects affected by an atomic commits and both the new
> + * state structures and pointers to the old state structures for
> + * these.
>    *
>    * 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
-- 
Hamza

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Thu, 14 Dec 2023 11:09:15 +0100
Maxime Ripard <mripard@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@kernel.org>
> ---
>  include/drm/drm_atomic.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 914574b58ae7..5df67e587816 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -346,7 +346,13 @@ struct __drm_private_objs_state {
>  };
>  
>  /**
> - * struct drm_atomic_state - the global state object for atomic updates
> + * struct drm_atomic_state - Atomic commit structure
> + *
> + * This structure is the kernel counterpart of @drm_mode_atomic and represents
> + * an atomic commit that transitions from an old to a new display state. It
> + * contains all the objects affected by an atomic commits and both the new

*by the atomic commit and

> + * state structures and pointers to the old state structures for
> + * these.
>   *
>   * 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

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

Hi,

On Fri, Dec 15, 2023 at 04:54:28PM +0200, Pekka Paalanen wrote:
> On Thu, 14 Dec 2023 11:09:15 +0100
> Maxime Ripard <mripard@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@kernel.org>
> > ---
> >  include/drm/drm_atomic.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 914574b58ae7..5df67e587816 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -346,7 +346,13 @@ struct __drm_private_objs_state {
> >  };
> >  
> >  /**
> > - * struct drm_atomic_state - the global state object for atomic updates
> > + * struct drm_atomic_state - Atomic commit structure
> > + *
> > + * This structure is the kernel counterpart of @drm_mode_atomic and represents
> > + * an atomic commit that transitions from an old to a new display state. It
> > + * contains all the objects affected by an atomic commits and both the new
> 
> *by the atomic commit and
> 
> > + * state structures and pointers to the old state structures for
> > + * these.
> >   *
> >   * 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
> 
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>

I've applied the series with the typos changes you asked for

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]