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

[PATCH 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.

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 2/5] drm/atomic: Remove inexistent reference

[Maxime Ripard]

The num_connectors field documentation mentions a connector_states field
that has never been part of this structure.

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

Re: [2/5] drm/atomic: Remove inexistent reference

[Sui Jingfeng]

Hi,


On 2023/12/4 20:17, Maxime Ripard wrote:
> The num_connectors field documentation mentions a connector_states field
> that has never been part of this structure.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>


> ---
>   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;
>   

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

[Daniel Vetter]

On Mon, Dec 04, 2023 at 01:17:04PM +0100, Maxime Ripard wrote:
> The num_connectors field documentation mentions a connector_states field
> that has never been part of this structure.

Not entirely correct, this is an oversight from 63e83c1dba54 ("drm:
Consolidate connector arrays in drm_atomic_state"). With the commit
message suitably updated.

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Maxime Ripard]

The doc for the planes, crtcs, connectors and private_objs fields
mention that they are pointers to an array of structures with
per-$OBJECT data.

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.

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

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

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

[Sui Jingfeng]

Hi,


On 2023/12/4 20:17, Maxime Ripard wrote:
> The doc for the planes, crtcs, connectors and private_objs fields
> mention that they are pointers to an array of structures with
> per-$OBJECT data.
>
> 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.
>
> Let's rephrase those fields a bit to better match the current situation.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>

> ---
>   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;
>   

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

[Daniel Vetter]

On Mon, Dec 04, 2023 at 01:17:05PM +0100, Maxime Ripard wrote:
> The doc for the planes, crtcs, connectors and private_objs fields
> mention that they are pointers to an array of structures with
> per-$OBJECT data.
> 
> 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.
> 
> Let's rephrase those fields a bit to better match the current situation.

Yeah that wasn't updated as part of 5d943aa6c0d4 ("drm: Consolidate crtc
arrays in drm_atomic_state") and b8b5342b699b ("drm: Consolidate plane
arrays in drm_atomic_state"). With that added to the commit message:

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 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 | 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.
  *
  * 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.
+ * Unaffected objects will not be part of that update, unless they have been
+ * explicitly added by either the framework or the driver.
  */
 struct drm_atomic_state {
 	/**
-- 
2.43.0

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

[Pekka Paalanen]

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

On Mon,  4 Dec 2023 13:17:06 +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 | 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

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

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

[Pekka Paalanen]

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

On Tue, 5 Dec 2023 10:51:06 +0200
Pekka Paalanen <pekka.paalanen@collabora.com> wrote:

> On Mon,  4 Dec 2023 13:17:06 +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>

Hi Maxime,

could you figure out how your email got the linux.ie address for Dave
Airlie?

I got a bounce from skynet.ie when replying to all. Maybe there is a
leftover email address for Dave still somewhere?


Thanks,
pq

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

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

[Maxime Ripard]

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

On Tue, Dec 05, 2023 at 11:15:29AM +0200, Pekka Paalanen wrote:
> On Tue, 5 Dec 2023 10:51:06 +0200
> Pekka Paalanen <pekka.paalanen@collabora.com> wrote:
> 
> > On Mon,  4 Dec 2023 13:17:06 +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>
>
> could you figure out how your email got the linux.ie address for Dave
> Airlie?
> 
> I got a bounce from skynet.ie when replying to all. Maybe there is a
> leftover email address for Dave still somewhere?

Yep. In my mutt aliases file :)

It's fixed now, thanks

Maxime

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

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

[Maxime Ripard]

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

On Tue, Dec 05, 2023 at 10:51:06AM +0200, Pekka Paalanen wrote:
> On Mon,  4 Dec 2023 13:17:06 +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 | 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.

Yeah, good point

> >   *
> >   * 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.

Great

> > + * 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?

I guess what I meant to say is that it's affected as a side effect that
the userspace cannot anticipate.

The typical example being that changing a property on a connector would
need to pull the CRTC into the update to disable / enable the CRTC,
encoder and connector.

As far as userspace is concerned, only the connector will be affected,
and only the connector will initially be part of the drm_atomic_state.

But then some part of the atomic_check logic will pull the CRTC into the
update.

It's still invisible to userspace, so I guess
"unaffected-but-turns-out-to-be-affected" would be the right term :)

Would something like:

Unaffected objects will not be part of the initial update, but might be
added explicitly by either the framework or the driver during
atomic_check.

be better?

> 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?

Sima (iirc?) recently made that explicit, yeah, but there's plenty of
commits that need at the very least the encoder and connector to be
disabled and enabled again to handle them.

Maxime

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

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

[Pekka Paalanen]

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

On Thu, 7 Dec 2023 15:27:33 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Dec 05, 2023 at 10:51:06AM +0200, Pekka Paalanen wrote:
> > On Mon,  4 Dec 2023 13:17:06 +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 | 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.  
> 
> Yeah, good point
> 
> > >   *
> > >   * 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.  
> 
> Great
> 
> > > + * 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?  
> 
> I guess what I meant to say is that it's affected as a side effect that
> the userspace cannot anticipate.
> 
> The typical example being that changing a property on a connector would
> need to pull the CRTC into the update to disable / enable the CRTC,
> encoder and connector.

Right, that's the easy case. This can even be documented and therefore
become expected by userspace. The associations between CRTC and
connector are published, e.g. the current routing.

What I was thinking of was much more hidden:

Userspace explicitly programs CRTC A and the connector connected to it.
None of the mentioned KMS objects in that atomic commit refer to CRTC B
in any way, not in old nor in new device state. Still, the driver
decides to pull CRTC B in, because it needs to adjust something there
(ISTR hearing "watermarks" in some conversation) in order to
accommodate changes in CRTC A.

So there are two categories of pulling in extra KMS objects: knowable
and unknowable. Or expected and unexpected.

If userspace changes things on a connector connected to CRTC A, you can
expect to pull in CRTC A. That's knowable. If the driver unexpectedly
also pulls in CRTC B while userspace made absolutely no explicit nor
implicit reference to it, that's unknowable.

The unknowable/unexpected additions are very hardware and driver
specific and not reliably determinable from an atomic commit UAPI
structure.

> As far as userspace is concerned, only the connector will be affected,
> and only the connector will initially be part of the drm_atomic_state.
> 
> But then some part of the atomic_check logic will pull the CRTC into the
> update.

Is this a rule that happens always? If so, it can be documented to make
it expected.

> It's still invisible to userspace, so I guess
> "unaffected-but-turns-out-to-be-affected" would be the right term :)
> 
> Would something like:
> 
> Unaffected objects will not be part of the initial update, but might be
> added explicitly by either the framework or the driver during
> atomic_check.
> 
> be better?

I'm not comfortable with the use of "unaffected" here. I'd be more in
the direction of: More objects can be affected than explicitly
mentioned.


Thanks,
pq

> > 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?  
> 
> Sima (iirc?) recently made that explicit, yeah, but there's plenty of
> commits that need at the very least the encoder and connector to be
> disabled and enabled again to handle them.
> 
> Maxime


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

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

[Maxime Ripard]

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

On Fri, Dec 08, 2023 at 10:08:28AM +0200, Pekka Paalanen wrote:
> On Thu, 7 Dec 2023 15:27:33 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Tue, Dec 05, 2023 at 10:51:06AM +0200, Pekka Paalanen wrote:
> > > On Mon,  4 Dec 2023 13:17:06 +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 | 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.  
> > 
> > Yeah, good point
> > 
> > > >   *
> > > >   * 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.  
> > 
> > Great
> > 
> > > > + * 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?  
> > 
> > I guess what I meant to say is that it's affected as a side effect that
> > the userspace cannot anticipate.
> > 
> > The typical example being that changing a property on a connector would
> > need to pull the CRTC into the update to disable / enable the CRTC,
> > encoder and connector.
> 
> Right, that's the easy case. This can even be documented and therefore
> become expected by userspace. The associations between CRTC and
> connector are published, e.g. the current routing.
> 
> What I was thinking of was much more hidden:
> 
> Userspace explicitly programs CRTC A and the connector connected to it.
> None of the mentioned KMS objects in that atomic commit refer to CRTC B
> in any way, not in old nor in new device state. Still, the driver
> decides to pull CRTC B in, because it needs to adjust something there
> (ISTR hearing "watermarks" in some conversation) in order to
> accommodate changes in CRTC A.
> 
> So there are two categories of pulling in extra KMS objects: knowable
> and unknowable. Or expected and unexpected.
> 
> If userspace changes things on a connector connected to CRTC A, you can
> expect to pull in CRTC A. That's knowable. If the driver unexpectedly
> also pulls in CRTC B while userspace made absolutely no explicit nor
> implicit reference to it, that's unknowable.

> The unknowable/unexpected additions are very hardware and driver
> specific and not reliably determinable from an atomic commit UAPI
> structure.

So I had a quick look at all the drivers we have in-tree, and it looks
to be either a plane or a connector pulling its CRTC in. I guess they
would all qualify as knowable.

For unknowable, yeah, it's kind of bad, but at the same time you have to
deal with the hardware you have. Like, for vc4 for example, there's a
single controller before the CRTCs that deals with the blending, scaling
and all the other stuff. That controller has a limited number of FIFOs
and muxes to output the result of the blending to the right CRTC.

So if you commit something on one CRTC, you might very well wait for
another one to complete because some hidden state (to userspace) is
shared between the two and you just can't run them in parallel.

So yeah... We should strive to make it as transparent to userspace as
possible, but I also don't think we can express all the variations we
support.

> > As far as userspace is concerned, only the connector will be affected,
> > and only the connector will initially be part of the drm_atomic_state.
> > 
> > But then some part of the atomic_check logic will pull the CRTC into the
> > update.
> 
> Is this a rule that happens always? If so, it can be documented to make
> it expected.
> 
> > It's still invisible to userspace, so I guess
> > "unaffected-but-turns-out-to-be-affected" would be the right term :)
> > 
> > Would something like:
> > 
> > Unaffected objects will not be part of the initial update, but might be
> > added explicitly by either the framework or the driver during
> > atomic_check.
> > 
> > be better?
> 
> I'm not comfortable with the use of "unaffected" here. I'd be more in
> the direction of: More objects can be affected than explicitly
> mentioned.

I think Sima's suggestion uses a different phrasing that should be much
better. Can you check if it works for you?

Maxime

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

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

[Pekka Paalanen]

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

On Fri, 8 Dec 2023 13:25:22 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Dec 08, 2023 at 10:08:28AM +0200, Pekka Paalanen wrote:
> > On Thu, 7 Dec 2023 15:27:33 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > On Tue, Dec 05, 2023 at 10:51:06AM +0200, Pekka Paalanen wrote:  
> > > > On Mon,  4 Dec 2023 13:17:06 +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 | 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.    
> > > 
> > > Yeah, good point
> > >   
> > > > >   *
> > > > >   * 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.    
> > > 
> > > Great
> > >   
> > > > > + * 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?    
> > > 
> > > I guess what I meant to say is that it's affected as a side effect that
> > > the userspace cannot anticipate.
> > > 
> > > The typical example being that changing a property on a connector would
> > > need to pull the CRTC into the update to disable / enable the CRTC,
> > > encoder and connector.  
> > 
> > Right, that's the easy case. This can even be documented and therefore
> > become expected by userspace. The associations between CRTC and
> > connector are published, e.g. the current routing.
> > 
> > What I was thinking of was much more hidden:
> > 
> > Userspace explicitly programs CRTC A and the connector connected to it.
> > None of the mentioned KMS objects in that atomic commit refer to CRTC B
> > in any way, not in old nor in new device state. Still, the driver
> > decides to pull CRTC B in, because it needs to adjust something there
> > (ISTR hearing "watermarks" in some conversation) in order to
> > accommodate changes in CRTC A.
> > 
> > So there are two categories of pulling in extra KMS objects: knowable
> > and unknowable. Or expected and unexpected.
> > 
> > If userspace changes things on a connector connected to CRTC A, you can
> > expect to pull in CRTC A. That's knowable. If the driver unexpectedly
> > also pulls in CRTC B while userspace made absolutely no explicit nor
> > implicit reference to it, that's unknowable.  
> 
> > The unknowable/unexpected additions are very hardware and driver
> > specific and not reliably determinable from an atomic commit UAPI
> > structure.  
> 
> So I had a quick look at all the drivers we have in-tree, and it looks
> to be either a plane or a connector pulling its CRTC in. I guess they
> would all qualify as knowable.

Yes.

> For unknowable, yeah, it's kind of bad, but at the same time you have to
> deal with the hardware you have. Like, for vc4 for example, there's a
> single controller before the CRTCs that deals with the blending, scaling
> and all the other stuff. That controller has a limited number of FIFOs
> and muxes to output the result of the blending to the right CRTC.
> 
> So if you commit something on one CRTC, you might very well wait for
> another one to complete because some hidden state (to userspace) is
> shared between the two and you just can't run them in parallel.
> 
> So yeah... We should strive to make it as transparent to userspace as
> possible, but I also don't think we can express all the variations we
> support.

I do not expect this could be prevented. It's important to acknowledge
that this can happen, even if it cannot be specified when it happens.
It's also important to document the requirement, like maybe it's not ok
to pull in unexpected CRTCs without ALLOW_MODESET? Maybe there is
already a check in DRM core for this?

If userspace is using per-CRTC flip completion events, then userspace
must always know before-hand which flip events it's going to get
eventually.

If userspace is using non-blocking commits (async is yet another
dimension), then the kernel pulling in an unexpected CRTC will risk
userspace failing its next commit on the unexpected CRTC with EBUSY.

When userspace uses non-blocking commits, it practically always also
expects flip events.

I don't know how ALLOW_MODESET + non-blocking should work. If it pulls
in unexpected CRTCs, will userspace also get unexpected flip events, or
is it only prone to EBUSY?

I would guess that unexpected flip events are avoided, and the expected
flip event is just delayed until unexpected CRTCs have completed as
well?

> > > As far as userspace is concerned, only the connector will be affected,
> > > and only the connector will initially be part of the drm_atomic_state.
> > > 
> > > But then some part of the atomic_check logic will pull the CRTC into the
> > > update.  
> > 
> > Is this a rule that happens always? If so, it can be documented to make
> > it expected.
> >   
> > > It's still invisible to userspace, so I guess
> > > "unaffected-but-turns-out-to-be-affected" would be the right term :)
> > > 
> > > Would something like:
> > > 
> > > Unaffected objects will not be part of the initial update, but might be
> > > added explicitly by either the framework or the driver during
> > > atomic_check.
> > > 
> > > be better?  
> > 
> > I'm not comfortable with the use of "unaffected" here. I'd be more in
> > the direction of: More objects can be affected than explicitly
> > mentioned.  
> 
> I think Sima's suggestion uses a different phrasing that should be much
> better. Can you check if it works for you?

Sima's phrasing is an addition, not a replacement, to this. There are
two things:

a) An atomic commit does not need to contain all the objects of a
   drm_device.

b) An atomic commit may affect more objects than it originally
   contained. (from userspace perspective)

Here b) is important for userspace to know, because it can be
surprising, but I understand that this patch is not userspace doc.


Thanks,
pq

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

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

[Maxime Ripard]

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

On Fri, Dec 08, 2023 at 03:59:46PM +0200, Pekka Paalanen wrote:
> On Fri, 8 Dec 2023 13:25:22 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > On Fri, Dec 08, 2023 at 10:08:28AM +0200, Pekka Paalanen wrote:
> > > On Thu, 7 Dec 2023 15:27:33 +0100
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >   
> > > > On Tue, Dec 05, 2023 at 10:51:06AM +0200, Pekka Paalanen wrote:  
> > > > > On Mon,  4 Dec 2023 13:17:06 +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 | 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.    
> > > > 
> > > > Yeah, good point
> > > >   
> > > > > >   *
> > > > > >   * 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.    
> > > > 
> > > > Great
> > > >   
> > > > > > + * 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?    
> > > > 
> > > > I guess what I meant to say is that it's affected as a side effect that
> > > > the userspace cannot anticipate.
> > > > 
> > > > The typical example being that changing a property on a connector would
> > > > need to pull the CRTC into the update to disable / enable the CRTC,
> > > > encoder and connector.  
> > > 
> > > Right, that's the easy case. This can even be documented and therefore
> > > become expected by userspace. The associations between CRTC and
> > > connector are published, e.g. the current routing.
> > > 
> > > What I was thinking of was much more hidden:
> > > 
> > > Userspace explicitly programs CRTC A and the connector connected to it.
> > > None of the mentioned KMS objects in that atomic commit refer to CRTC B
> > > in any way, not in old nor in new device state. Still, the driver
> > > decides to pull CRTC B in, because it needs to adjust something there
> > > (ISTR hearing "watermarks" in some conversation) in order to
> > > accommodate changes in CRTC A.
> > > 
> > > So there are two categories of pulling in extra KMS objects: knowable
> > > and unknowable. Or expected and unexpected.
> > > 
> > > If userspace changes things on a connector connected to CRTC A, you can
> > > expect to pull in CRTC A. That's knowable. If the driver unexpectedly
> > > also pulls in CRTC B while userspace made absolutely no explicit nor
> > > implicit reference to it, that's unknowable.  
> > 
> > > The unknowable/unexpected additions are very hardware and driver
> > > specific and not reliably determinable from an atomic commit UAPI
> > > structure.  
> > 
> > So I had a quick look at all the drivers we have in-tree, and it looks
> > to be either a plane or a connector pulling its CRTC in. I guess they
> > would all qualify as knowable.
> 
> Yes.
> 
> > For unknowable, yeah, it's kind of bad, but at the same time you have to
> > deal with the hardware you have. Like, for vc4 for example, there's a
> > single controller before the CRTCs that deals with the blending, scaling
> > and all the other stuff. That controller has a limited number of FIFOs
> > and muxes to output the result of the blending to the right CRTC.
> > 
> > So if you commit something on one CRTC, you might very well wait for
> > another one to complete because some hidden state (to userspace) is
> > shared between the two and you just can't run them in parallel.
> > 
> > So yeah... We should strive to make it as transparent to userspace as
> > possible, but I also don't think we can express all the variations we
> > support.
> 
> I do not expect this could be prevented. It's important to acknowledge
> that this can happen, even if it cannot be specified when it happens.

Ack.

> It's also important to document the requirement, like maybe it's not ok
> to pull in unexpected CRTCs without ALLOW_MODESET? Maybe there is
> already a check in DRM core for this?
> 
> If userspace is using per-CRTC flip completion events, then userspace
> must always know before-hand which flip events it's going to get
> eventually.
> 
> If userspace is using non-blocking commits (async is yet another
> dimension), then the kernel pulling in an unexpected CRTC will risk
> userspace failing its next commit on the unexpected CRTC with EBUSY.
> 
> When userspace uses non-blocking commits, it practically always also
> expects flip events.
> 
> I don't know how ALLOW_MODESET + non-blocking should work. If it pulls
> in unexpected CRTCs, will userspace also get unexpected flip events, or
> is it only prone to EBUSY?
> 
> I would guess that unexpected flip events are avoided, and the expected
> flip event is just delayed until unexpected CRTCs have completed as
> well?

Those are all good questions, and the answer is... I don't know, sorry :/

> > > > As far as userspace is concerned, only the connector will be affected,
> > > > and only the connector will initially be part of the drm_atomic_state.
> > > > 
> > > > But then some part of the atomic_check logic will pull the CRTC into the
> > > > update.  
> > > 
> > > Is this a rule that happens always? If so, it can be documented to make
> > > it expected.
> > >   
> > > > It's still invisible to userspace, so I guess
> > > > "unaffected-but-turns-out-to-be-affected" would be the right term :)
> > > > 
> > > > Would something like:
> > > > 
> > > > Unaffected objects will not be part of the initial update, but might be
> > > > added explicitly by either the framework or the driver during
> > > > atomic_check.
> > > > 
> > > > be better?  
> > > 
> > > I'm not comfortable with the use of "unaffected" here. I'd be more in
> > > the direction of: More objects can be affected than explicitly
> > > mentioned.  
> > 
> > I think Sima's suggestion uses a different phrasing that should be much
> > better. Can you check if it works for you?
> 
> Sima's phrasing is an addition, not a replacement, to this. There are
> two things:
> 
> a) An atomic commit does not need to contain all the objects of a
>    drm_device.
> 
> b) An atomic commit may affect more objects than it originally
>    contained. (from userspace perspective)
> 
> Here b) is important for userspace to know, because it can be
> surprising, but I understand that this patch is not userspace doc.

Right, so my initial goal from this series was to make sure the
ambiguous meaning of what a state was was (hopefully) lifted, and thus
we could progress on your userspace doc patch. So I want to address
point A here.

B is also important, but maybe we should discuss it into a separate
patch?

Maxime

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

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

[Pekka Paalanen]

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

On Fri, 8 Dec 2023 16:20:37 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, Dec 08, 2023 at 03:59:46PM +0200, Pekka Paalanen wrote:
> > On Fri, 8 Dec 2023 13:25:22 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > On Fri, Dec 08, 2023 at 10:08:28AM +0200, Pekka Paalanen wrote:  
> > > > On Thu, 7 Dec 2023 15:27:33 +0100
> > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > >     
> > > > > On Tue, Dec 05, 2023 at 10:51:06AM +0200, Pekka Paalanen wrote:    
> > > > > > On Mon,  4 Dec 2023 13:17:06 +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 | 10 +++++++++-
> > > > > > >  1 file changed, 9 insertions(+), 1 deletion(-)

...

> > Sima's phrasing is an addition, not a replacement, to this. There are
> > two things:
> > 
> > a) An atomic commit does not need to contain all the objects of a
> >    drm_device.
> > 
> > b) An atomic commit may affect more objects than it originally
> >    contained. (from userspace perspective)
> > 
> > Here b) is important for userspace to know, because it can be
> > surprising, but I understand that this patch is not userspace doc.  
> 
> Right, so my initial goal from this series was to make sure the
> ambiguous meaning of what a state was was (hopefully) lifted, and thus
> we could progress on your userspace doc patch. So I want to address
> point A here.
> 
> B is also important, but maybe we should discuss it into a separate
> patch?

Sure!

Just mind both cases to not accidentally imply something about b) when
talking about a).


Thanks,
pq

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

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

[Daniel Vetter]

On Mon, Dec 04, 2023 at 01:17:06PM +0100, 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>

So this is probably the biggest naming fumble I've committed, because this
is the drm_atomic_commit structure: It's not just a state structure, but
it represents the transition from a set of old to new states. Which is
also why we have both old and new state pointers in it.

> ---
>  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

I think we're using "commit" more often than "update"

> + *
> + * This structure is the kernel counterpart of @drm_mode_atomic and contains
> + * all the objects affected by an atomic modeset update and their states.

My suggestion:

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
>   * 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.
> + * Unaffected objects will not be part of that update, unless they have been
> + * explicitly added by either the framework or the driver.

Since you remove the global in the header summary I wouldn't reintroduce
it here. Seems to just add to the confusion again instead of clarifying.

If you want maybe clarify that an atomic commit does not need to contain
all the objects of a &drm_device, or something like that.

With the comments suitably addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


>   */
>  struct drm_atomic_state {
>  	/**
> -- 
> 2.43.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 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.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 Documentation/gpu/todo.rst | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index b62c7fa0c2bc..fe95aea89d67 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -120,6 +120,24 @@ 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 anymore, 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_update``.
+
+Contact: Maxime Ripard <mripard@kernel.org>
+
+Level: Advanced
+
 Fallout from atomic KMS
 -----------------------
 
-- 
2.43.0

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

[Daniel Vetter]

On Mon, Dec 04, 2023 at 01:17:07PM +0100, Maxime Ripard wrote:
> The name of the structure drm_atomic_state is confusing. Let's add an
> entry to our todo list to rename it.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  Documentation/gpu/todo.rst | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index b62c7fa0c2bc..fe95aea89d67 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -120,6 +120,24 @@ 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.

That's a very generous description of my screw-up of calling a commit a
state and making a big mess out of a lot of concepts :-)

> +
> +The state isn't the entire device state anymore, but only the full state of

s/anymore// since it was always meant to be an incremental/partial
update/commit structure.

> +some objects in that device. This is confusing to newcomers, and
> +``drm_atomic_state`` should be renamed to something clearer like
> +``drm_atomic_update``.

My choice would be drm_atomic_commit, because that's what we're calling
these everywhere else in the code. See drm_crtc_commit for the per-crtc
tracking thing of a drm_atomic_commit. If you want update, there's quite a
lot of other things we also need to rename to the _update suffix.

Also this should have some pointers to the functions that need adjusting,
like drm_atomic_state_alloc|get/put/init/.... since without also renaming
those this is just going to create even more confusion.

With my comments addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
> +Contact: Maxime Ripard <mripard@kernel.org>
> +
> +Level: Advanced
> +
>  Fallout from atomic KMS
>  -----------------------
>  
> -- 
> 2.43.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Sui Jingfeng]

Hi,


On 2023/12/4 20:17, Maxime Ripard wrote:
> 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.
>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>

Very nice cleanup, but I see a very very tiny concern, see below.

> ---
>   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.
> +	 */

Well, I noticed that the first letter of 'Parent' get capitalized.
Do we also need capitalize the reset of documentation to keep consistent?


>   	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;
>   
>   	/**

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

[Daniel Vetter]

On Mon, Dec 04, 2023 at 01:17:03PM +0100, Maxime Ripard wrote:
> 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.
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch