[PATCH v7 00/16] drm: Add DRM_MODE_ATOMIC_RESET flag

[PATCH v7 00/16] drm: Add DRM_MODE_ATOMIC_RESET flag

[Maxime Ripard]

Hi,

Userspace currently has no atomic way to bring a display pipeline back
to a pristine state. A compositor that wants to start from a known
baseline must explicitly set every property on every KMS object to its
default value, which requires tracking which properties exist and what
their defaults are. This is fragile and must be updated every time a
new property is added to the kernel.

This series introduces a new DRM_MODE_ATOMIC_RESET flag for the
atomic ioctl. When set, the kernel fills the commit with default
states for all KMS objects before applying the properties supplied in
the request. Properties not explicitly included remain at their
defaults. This allows userspace to describe the desired end state
declaratively, without caring about the current state or the full set
of properties.

The bulk of the series extracts the state insertion logic from each
drm_atomic_get_*_state() into standalone helpers, since the
fill_with_defaults path creates states through atomic_create_state()
rather than atomic_duplicate_state() and cannot reuse the existing
functions directly.

A driver-specific property is added to vkms planes for testing.
Standard properties are impractical for this: compositors like Mutter
enumerate all known properties, making it hard to isolate the reset
behavior in a test.

Mutter has a working implementation using the new flag, with a
vkms-based test exercising the reset behavior:
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/5309

And we have IGT tests as well:
https://lore.kernel.org/igt-dev/20260916095159.2896630-1-mripard@kernel.org/

This series relies on all drivers implementing atomic_create_state.
The conversion is in progress but not yet complete.

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Changes in v7:
- Fix a whole bunch of sashiko issues to deal with color_mgmt_changed,
  leases, partially leased devices (like a plane leased but it's crtc
  isn't) and the interaction with PAGE_FLIP_EVENT better
- Fix crtc create_state implementation for ssd130x
- Actually make it possible for a compositor to recreate the same whole
  state without passing ALLOW_MODESET
- Fix EDEADLK bugs with kunit tests
- Don't report DRM_CAP_ATOMIC_RESET on !atomic drivers
- Fix error handling in vkms for the new property
- Add more kunit tests for the new corner cases
- Link to v6: https://lore.kernel.org/r/20261022-drm-reset-state-flag-v6-0-f36d640b7e76@kernel.org

Changes in v6:
- If using a lease, only considered the leased objects
- Fix amdgpu plane atomic_create_state implementation
- Link to v5: https://lore.kernel.org/r/20261016-drm-reset-state-flag-v5-0-dad2d64b693b@kernel.org

Changes in v5:
- Fix bisection issue
- Improve the documentation
- Fix connector hotplugging issue
- Added kunit tests for drm_atomic_commit_fill_with_defaults()
- Link to v4: https://lore.kernel.org/r/20260918-drm-reset-state-flag-v4-0-5ad106370f05@kernel.org

Changes in v4:
- Improve the drm_atomic_get_private_obj_state and
  drm_atomic_get_connector_state array reallocations
- Return void on functions to insert an object state into a commit when
  it cannot fail
- Rename drm_atomic_can_create_state() into
  drm_atomic_implements_create_state()
- Move capability check from drm_atomic_commit_fill_with_defaults to its
  caller 
- Added IGT test
- Link to v3: https://lore.kernel.org/r/20260908-drm-reset-state-flag-v3-0-905570f387a4@kernel.org

Changes in v3:
- Rebased on top of the latest atomic_create_state series
- Sebastian Wick created and tested this work with mutter and vkms
- Fix colorop locking
- Link to v2: https://lore.kernel.org/r/20260812-drm-reset-state-flag-v2-0-e96ce13317dd@kernel.org

Changes in v2:
- Fix bisection
- Add capability to let userspace know if it can reset the state
- Link to v1: https://lore.kernel.org/r/20260708-drm-reset-state-flag-v1-0-c37dc985485d@kernel.org

---
Maxime Ripard (15):
      drm/amd/display: Fix planes atomic_create_state implementation
      drm/solomon: ssd130x: Drop the init-only assumption in ssd130x_crtc_create_state()
      drm/atomic: Switch to krealloc_array() in drm_atomic_get_private_obj_state()
      drm/atomic: Use __GFP_ZERO instead of explicit memset in drm_atomic_get_private_obj_state()
      drm/atomic: Use __GFP_ZERO instead of explicit memset in drm_atomic_get_connector_state()
      drm/atomic: Create function to insert CRTC state into a commit
      drm/atomic: Create function to insert plane state into a commit
      drm/atomic: Create function to insert colorop state into a commit
      drm/atomic: Create function to insert private obj state into a commit
      drm/atomic: Create function to insert connector state into a commit
      drm/atomic: Add drm_atomic_implements_create_state() helper
      drm/atomic: Allow filling a commit with pristine object states
      drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()
      drm/vkms: Switch container_of helpers to container_of_const
      drm/vkms: Add driver-specific plane property for testing

Sebastian Wick (1):
      drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

 .../drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c    |   3 -
 drivers/gpu/drm/drm_atomic.c                       | 524 ++++++++++++++++++---
 drivers/gpu/drm/drm_atomic_uapi.c                  |  44 +-
 drivers/gpu/drm/drm_ioctl.c                        |   5 +
 drivers/gpu/drm/solomon/ssd130x.c                  |   3 -
 drivers/gpu/drm/tests/drm_atomic_test.c            | 313 +++++++++++-
 drivers/gpu/drm/vkms/vkms_drv.h                    |  11 +-
 drivers/gpu/drm/vkms/vkms_plane.c                  |  63 ++-
 include/drm/drm_atomic.h                           |   4 +
 include/uapi/drm/drm.h                             |  10 +
 include/uapi/drm/drm_mode.h                        |  17 +-
 11 files changed, 916 insertions(+), 81 deletions(-)
---
base-commit: 128013e9fec562672562dffd4ad468492f5915c8
change-id: 20260708-drm-reset-state-flag-2fb2b5711f97

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH v7 01/16] drm/amd/display: Fix planes atomic_create_state implementation

[Maxime Ripard]

Commit 2bcec01cf105 ("drm/amdgpu_dm: Convert to atomic_create_state")
converted the amdgpu planes to use atomic_create_state instead of reset.

Unlike reset, atomic_create_state is meant to allocate a new default
state without any side effect on the hardware or software state.

However, during the conversion we forgot to remove the call to destroy
the existing plane state if any. While this was expected from a reset
implementation, it's not for an atomic_create_state one. Remove it.

Fixes: 2bcec01cf105 ("drm/amdgpu_dm: Convert to atomic_create_state")
Signed-off-by: Maxime Ripard <mripard@kernel.org>

---

To: Harry Wentland <harry.wentland@amd.com>
To: Leo Li <sunpeng.li@amd.com>
To: Rodrigo Siqueira <siqueira@igalia.com>
To: Alex Deucher <alexander.deucher@amd.com>
To: Christian König <christian.koenig@amd.com>
Cc: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
index 2f88b9d65b82..779dabb7fc51 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
@@ -1821,7 +1821,4 @@ STATIC_IFN_KUNIT struct drm_plane_state *amdgpu_dm_plane_drm_plane_create_state(
 	}
 
-	if (plane->state)
-		plane->funcs->atomic_destroy_state(plane, plane->state);
-
 	__drm_atomic_helper_plane_state_init(&amdgpu_state->base, plane);
 	amdgpu_state->degamma_tf = AMDGPU_TRANSFER_FUNCTION_DEFAULT;

-- 
2.55.0

[PATCH v7 02/16] drm/solomon: ssd130x: Drop the init-only assumption in ssd130x_crtc_create_state()

[Maxime Ripard]

The ssd130x_crtc_create_state() implementation warns when the CRTC
already has a state, assuming that atomic_create_state would only ever
be called at initialization time to allocate the initial state.

However, atomic_create_state is documented to have no side effect and
can be called at runtime, so that assumption doesn't hold and the WARN
is a side effect in itself.

Let's drop the WARN, along with the comment documenting that
assumption.

Fixes: 1345de7fe9114 ("drm/solomon: ssd130x: Convert to atomic_create_state")
Signed-off-by: Maxime Ripard <mripard@kernel.org>

---

To: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/solomon/ssd130x.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 4901a1835f44..a6dbefb37b2c 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1755,11 +1755,8 @@ static int ssd133x_crtc_atomic_check(struct drm_crtc *crtc,
 }
 
-/* Called during init to allocate the CRTC's atomic state. */
 static struct drm_crtc_state *ssd130x_crtc_create_state(struct drm_crtc *crtc)
 {
 	struct ssd130x_crtc_state *ssd130x_state;
 
-	drm_WARN_ON_ONCE(crtc->dev, crtc->state);
-
 	ssd130x_state = kzalloc_obj(*ssd130x_state);
 	if (!ssd130x_state)

-- 
2.55.0

[PATCH v7 03/16] drm/atomic: Switch to krealloc_array() in drm_atomic_get_private_obj_state()

[Maxime Ripard]

The drm_atomic_get_private_obj_state() function manually computes the
allocation size by multiplying the element count by the element size
before passing it to krealloc().

krealloc_array() handles this multiplication internally and includes
overflow checking. Switch to it and drop the now-unused size variable.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9bb761eda59f..aa47c647df31 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1080,5 +1080,4 @@ drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
 {
 	int index, num_objs, ret;
-	size_t size;
 	struct __drm_private_objs_state *arr;
 	struct drm_private_state *obj_state;
@@ -1096,6 +1095,6 @@ drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
 
 	num_objs = state->num_private_objs + 1;
-	size = sizeof(*state->private_objs) * num_objs;
-	arr = krealloc(state->private_objs, size, GFP_KERNEL);
+	arr = krealloc_array(state->private_objs, num_objs,
+			     sizeof(*state->private_objs), GFP_KERNEL);
 	if (!arr)
 		return ERR_PTR(-ENOMEM);

-- 
2.55.0

[PATCH v7 04/16] drm/atomic: Use __GFP_ZERO instead of explicit memset in drm_atomic_get_private_obj_state()

[Maxime Ripard]

The drm_atomic_get_private_obj_state() function allocates a new entry in
the private_objs array using krealloc_array(), then zeroes the new entry
with a separate memset() call.

However, since krealloc_array() already supports the __GFP_ZERO flag to
zero newly allocated memory, the explicit memset() is redundant.

Pass __GFP_ZERO to krealloc_array() and drop the manual memset().

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index aa47c647df31..43b58e9b25e4 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1096,5 +1096,5 @@ drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
 	num_objs = state->num_private_objs + 1;
 	arr = krealloc_array(state->private_objs, num_objs,
-			     sizeof(*state->private_objs), GFP_KERNEL);
+			     sizeof(*state->private_objs), GFP_KERNEL | __GFP_ZERO);
 	if (!arr)
 		return ERR_PTR(-ENOMEM);
@@ -1102,5 +1102,4 @@ drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
 	state->private_objs = arr;
 	index = state->num_private_objs;
-	memset(&state->private_objs[index], 0, sizeof(*state->private_objs));
 
 	obj_state = obj->funcs->atomic_duplicate_state(obj);

-- 
2.55.0

[PATCH v7 05/16] drm/atomic: Use __GFP_ZERO instead of explicit memset in drm_atomic_get_connector_state()

[Maxime Ripard]

The drm_atomic_get_connector_state() function grows the connectors array
with krealloc_array() when the requested connector index exceeds the
current allocation, then zeroes the new entries with a separate memset()
call.

Since krealloc_array() supports the __GFP_ZERO flag to zero newly
allocated memory, the explicit memset() is unnecessary.

Pass __GFP_ZERO to krealloc_array() and drop the manual memset().

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 43b58e9b25e4..80436adcfb7f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1391,12 +1391,9 @@ drm_atomic_get_connector_state(struct drm_atomic_commit *state,
 
 		c = krealloc_array(state->connectors, alloc,
-				   sizeof(*state->connectors), GFP_KERNEL);
+				   sizeof(*state->connectors), GFP_KERNEL | __GFP_ZERO);
 		if (!c)
 			return ERR_PTR(-ENOMEM);
 
 		state->connectors = c;
-		memset(&state->connectors[state->num_connector], 0,
-		       sizeof(*state->connectors) * (alloc - state->num_connector));
-
 		state->num_connector = alloc;
 	}

-- 
2.55.0

[PATCH v7 06/16] drm/atomic: Create function to insert CRTC state into a commit

[Maxime Ripard]

drm_atomic_get_crtc_state() allocates a new CRTC state by duplicating
the current one and inserts it into the atomic commit as a single
operation.

However, a later change will need to insert a CRTC state into a
commit without going through the full allocation and duplication path
in drm_atomic_get_crtc_state().

Extract the state insertion logic into a new static
drm_atomic_commit_set_crtc_state() helper, and convert
drm_atomic_get_crtc_state() to use it.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 80436adcfb7f..178eca95d0e3 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -434,4 +434,19 @@ void __drm_atomic_commit_free(struct kref *ref)
 EXPORT_SYMBOL(__drm_atomic_commit_free);
 
+static void drm_atomic_commit_set_crtc_state(struct drm_atomic_commit *commit,
+					     struct drm_crtc *crtc,
+					     struct drm_crtc_state *crtc_state)
+{
+	int index = drm_crtc_index(crtc);
+
+	drm_modeset_lock_assert_held(&crtc->mutex);
+
+	commit->crtcs[index].state_to_destroy = crtc_state;
+	commit->crtcs[index].old_state = crtc->state;
+	commit->crtcs[index].new_state = crtc_state;
+	commit->crtcs[index].ptr = crtc;
+	crtc_state->state = commit;
+}
+
 /**
  * drm_atomic_get_crtc_state - get CRTC state
@@ -456,5 +471,5 @@ drm_atomic_get_crtc_state(struct drm_atomic_commit *state,
 			  struct drm_crtc *crtc)
 {
-	int ret, index = drm_crtc_index(crtc);
+	int ret;
 	struct drm_crtc_state *crtc_state;
 
@@ -474,9 +489,5 @@ drm_atomic_get_crtc_state(struct drm_atomic_commit *state,
 		return ERR_PTR(-ENOMEM);
 
-	state->crtcs[index].state_to_destroy = crtc_state;
-	state->crtcs[index].old_state = crtc->state;
-	state->crtcs[index].new_state = crtc_state;
-	state->crtcs[index].ptr = crtc;
-	crtc_state->state = state;
+	drm_atomic_commit_set_crtc_state(state, crtc, crtc_state);
 
 	drm_dbg_atomic(state->dev, "Added [CRTC:%d:%s] %p state to %p\n",

-- 
2.55.0

[PATCH v7 07/16] drm/atomic: Create function to insert plane state into a commit

[Maxime Ripard]

drm_atomic_get_plane_state() allocates a new plane state by
duplicating the current one and inserts it into the atomic commit as
a single operation.

However, a later change will need to insert a plane state into a
commit without going through the full allocation and duplication path
in drm_atomic_get_plane_state().

Extract the state insertion logic into a new static
drm_atomic_commit_set_plane_state() helper, and convert
drm_atomic_get_plane_state() to use it.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 178eca95d0e3..b26458ee98d8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -631,4 +631,19 @@ static int drm_atomic_connector_check(struct drm_connector *connector,
 }
 
+static void drm_atomic_commit_set_plane_state(struct drm_atomic_commit *commit,
+					      struct drm_plane *plane,
+					      struct drm_plane_state *plane_state)
+{
+	int index = drm_plane_index(plane);
+
+	drm_modeset_lock_assert_held(&plane->mutex);
+
+	commit->planes[index].state_to_destroy = plane_state;
+	commit->planes[index].old_state = plane->state;
+	commit->planes[index].new_state = plane_state;
+	commit->planes[index].ptr = plane;
+	plane_state->state = commit;
+}
+
 /**
  * drm_atomic_get_plane_state - get plane state
@@ -649,5 +664,5 @@ drm_atomic_get_plane_state(struct drm_atomic_commit *state,
 			  struct drm_plane *plane)
 {
-	int ret, index = drm_plane_index(plane);
+	int ret;
 	struct drm_plane_state *plane_state;
 
@@ -672,9 +687,5 @@ drm_atomic_get_plane_state(struct drm_atomic_commit *state,
 		return ERR_PTR(-ENOMEM);
 
-	state->planes[index].state_to_destroy = plane_state;
-	state->planes[index].ptr = plane;
-	state->planes[index].old_state = plane->state;
-	state->planes[index].new_state = plane_state;
-	plane_state->state = state;
+	drm_atomic_commit_set_plane_state(state, plane, plane_state);
 
 	drm_dbg_atomic(plane->dev, "Added [PLANE:%d:%s] %p state to %p\n",

-- 
2.55.0

[PATCH v7 08/16] drm/atomic: Create function to insert colorop state into a commit

[Maxime Ripard]

drm_atomic_get_colorop_state() allocates a new colorop state by
duplicating the current one and inserts it into the atomic commit as
a single operation.

However, a later change will need to insert a colorop state into a
commit without going through the full allocation and duplication path
in drm_atomic_get_colorop_state().

Extract the state insertion logic into a new static
drm_atomic_commit_set_colorop_state() helper, and convert
drm_atomic_get_colorop_state() to use it.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b26458ee98d8..807b575ecc54 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -705,4 +705,19 @@ drm_atomic_get_plane_state(struct drm_atomic_commit *state,
 EXPORT_SYMBOL(drm_atomic_get_plane_state);
 
+static void drm_atomic_commit_set_colorop_state(struct drm_atomic_commit *commit,
+						struct drm_colorop *colorop,
+						struct drm_colorop_state *colorop_state)
+{
+	int index = drm_colorop_index(colorop);
+
+	drm_modeset_lock_assert_held(&colorop->plane->mutex);
+
+	commit->colorops[index].state_to_destroy = colorop_state;
+	commit->colorops[index].old_state = colorop->state;
+	commit->colorops[index].new_state = colorop_state;
+	commit->colorops[index].ptr = colorop;
+	colorop_state->state = commit;
+}
+
 /**
  * drm_atomic_get_colorop_state - get colorop state
@@ -724,5 +739,5 @@ drm_atomic_get_colorop_state(struct drm_atomic_commit *state,
 			     struct drm_colorop *colorop)
 {
-	int ret, index = drm_colorop_index(colorop);
+	int ret;
 	struct drm_colorop_state *colorop_state;
 
@@ -741,9 +756,5 @@ drm_atomic_get_colorop_state(struct drm_atomic_commit *state,
 		return ERR_PTR(-ENOMEM);
 
-	state->colorops[index].state_to_destroy = colorop_state;
-	state->colorops[index].ptr = colorop;
-	state->colorops[index].old_state = colorop->state;
-	state->colorops[index].new_state = colorop_state;
-	colorop_state->state = state;
+	drm_atomic_commit_set_colorop_state(state, colorop, colorop_state);
 
 	drm_dbg_atomic(colorop->dev, "Added [COLOROP:%d:%d] %p state to %p\n",

-- 
2.55.0

[PATCH v7 09/16] drm/atomic: Create function to insert private obj state into a commit

[Maxime Ripard]

drm_atomic_get_private_obj_state() allocates a new private object
state by duplicating the current one and inserts it into the atomic
commit as a single operation.

However, a later change will need to insert a private object state
into a commit without going through the full allocation and
duplication path in drm_atomic_get_private_obj_state().

Extract the state insertion logic, including the array reallocation,
into a new static drm_atomic_commit_set_private_obj_state() helper,
and convert drm_atomic_get_private_obj_state() to use it.

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 53 +++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 807b575ecc54..513e91ad2fdc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1096,4 +1096,33 @@ drm_atomic_private_obj_fini(struct drm_private_obj *obj)
 EXPORT_SYMBOL(drm_atomic_private_obj_fini);
 
+static int drm_atomic_commit_set_private_obj_state(struct drm_atomic_commit *commit,
+						   struct drm_private_obj *obj,
+						   struct drm_private_state *obj_state)
+{
+	struct __drm_private_objs_state *arr;
+	int index, num_objs;
+
+	drm_modeset_lock_assert_held(&obj->lock);
+
+	num_objs = commit->num_private_objs + 1;
+	arr = krealloc_array(commit->private_objs, num_objs,
+			     sizeof(*commit->private_objs), GFP_KERNEL | __GFP_ZERO);
+	if (!arr)
+		return -ENOMEM;
+
+	commit->private_objs = arr;
+	index = commit->num_private_objs;
+
+	commit->private_objs[index].state_to_destroy = obj_state;
+	commit->private_objs[index].old_state = obj->state;
+	commit->private_objs[index].new_state = obj_state;
+	commit->private_objs[index].ptr = obj;
+	obj_state->state = commit;
+
+	commit->num_private_objs = num_objs;
+
+	return 0;
+}
+
 /**
  * drm_atomic_get_private_obj_state - get private object state
@@ -1112,6 +1141,5 @@ drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
 				 struct drm_private_obj *obj)
 {
-	int index, num_objs, ret;
-	struct __drm_private_objs_state *arr;
+	int ret;
 	struct drm_private_state *obj_state;
 
@@ -1127,24 +1155,13 @@ drm_atomic_get_private_obj_state(struct drm_atomic_commit *state,
 		return ERR_PTR(ret);
 
-	num_objs = state->num_private_objs + 1;
-	arr = krealloc_array(state->private_objs, num_objs,
-			     sizeof(*state->private_objs), GFP_KERNEL | __GFP_ZERO);
-	if (!arr)
-		return ERR_PTR(-ENOMEM);
-
-	state->private_objs = arr;
-	index = state->num_private_objs;
-
 	obj_state = obj->funcs->atomic_duplicate_state(obj);
 	if (!obj_state)
 		return ERR_PTR(-ENOMEM);
 
-	state->private_objs[index].state_to_destroy = obj_state;
-	state->private_objs[index].old_state = obj->state;
-	state->private_objs[index].new_state = obj_state;
-	state->private_objs[index].ptr = obj;
-	obj_state->state = state;
-
-	state->num_private_objs = num_objs;
+	ret = drm_atomic_commit_set_private_obj_state(state, obj, obj_state);
+	if (ret) {
+		obj->funcs->atomic_destroy_state(obj, obj_state);
+		return ERR_PTR(ret);
+	}
 
 	drm_dbg_atomic(state->dev,

-- 
2.55.0

[PATCH v7 10/16] drm/atomic: Create function to insert connector state into a commit

[Maxime Ripard]

drm_atomic_get_connector_state() allocates a new connector state by
duplicating the current one and inserts it into the atomic commit as
a single operation.

However, a later change will need to insert a connector state into a
commit without going through the full allocation and duplication path
in drm_atomic_get_connector_state().

Extract the state insertion logic, including the dynamic array
reallocation, into a new static
drm_atomic_commit_set_connector_state() helper, and convert
drm_atomic_get_connector_state() to use it.

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 60 ++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 513e91ad2fdc..81591173265d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1405,4 +1405,36 @@ drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_commit *state,
 EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_encoder);
 
+static int drm_atomic_commit_set_connector_state(struct drm_atomic_commit *commit,
+						 struct drm_connector *connector,
+						 struct drm_connector_state *connector_state)
+{
+	struct drm_mode_config *config = &connector->dev->mode_config;
+	int index = drm_connector_index(connector);
+
+	drm_modeset_lock_assert_held(&config->connection_mutex);
+
+	if (index >= commit->num_connector) {
+		struct __drm_connnectors_state *c;
+		int alloc = max(index + 1, config->num_connector);
+
+		c = krealloc_array(commit->connectors, alloc,
+				   sizeof(*commit->connectors), GFP_KERNEL | __GFP_ZERO);
+		if (!c)
+			return -ENOMEM;
+
+		commit->connectors = c;
+		commit->num_connector = alloc;
+	}
+
+	drm_connector_get(connector);
+	commit->connectors[index].state_to_destroy = connector_state;
+	commit->connectors[index].old_state = connector->state;
+	commit->connectors[index].new_state = connector_state;
+	commit->connectors[index].ptr = connector;
+	connector_state->state = commit;
+
+	return 0;
+}
+
 /**
  * drm_atomic_get_connector_state - get connector state
@@ -1423,5 +1455,5 @@ drm_atomic_get_connector_state(struct drm_atomic_commit *state,
 			  struct drm_connector *connector)
 {
-	int ret, index;
+	int ret;
 	struct drm_mode_config *config = &connector->dev->mode_config;
 	struct drm_connector_state *connector_state;
@@ -1434,19 +1466,4 @@ drm_atomic_get_connector_state(struct drm_atomic_commit *state,
 		return ERR_PTR(ret);
 
-	index = drm_connector_index(connector);
-
-	if (index >= state->num_connector) {
-		struct __drm_connnectors_state *c;
-		int alloc = max(index + 1, config->num_connector);
-
-		c = krealloc_array(state->connectors, alloc,
-				   sizeof(*state->connectors), GFP_KERNEL | __GFP_ZERO);
-		if (!c)
-			return ERR_PTR(-ENOMEM);
-
-		state->connectors = c;
-		state->num_connector = alloc;
-	}
-
 	connector_state = drm_atomic_get_new_connector_state(state, connector);
 	if (connector_state)
@@ -1457,10 +1474,9 @@ drm_atomic_get_connector_state(struct drm_atomic_commit *state,
 		return ERR_PTR(-ENOMEM);
 
-	drm_connector_get(connector);
-	state->connectors[index].state_to_destroy = connector_state;
-	state->connectors[index].old_state = connector->state;
-	state->connectors[index].new_state = connector_state;
-	state->connectors[index].ptr = connector;
-	connector_state->state = state;
+	ret = drm_atomic_commit_set_connector_state(state, connector, connector_state);
+	if (ret) {
+		connector->funcs->atomic_destroy_state(connector, connector_state);
+		return ERR_PTR(ret);
+	}
 
 	drm_dbg_atomic(connector->dev, "Added [CONNECTOR:%d:%s] %p state to %p\n",

-- 
2.55.0

[PATCH v7 11/16] drm/atomic: Add drm_atomic_implements_create_state() helper

[Maxime Ripard]

The atomic reset path will need to create pristine default states from
scratch for every plane, CRTC, and connector. This requires all of them
to implement the atomic_create_state hook.

Introduce a drm_atomic_implements_create_state() helper that iterates
over all planes, CRTCs, and connectors and returns whether they all
provide the hook. Color operations are excluded since they always use
drm_atomic_helper_colorop_create_state() directly.

This will be used both as a precondition before filling a commit with
default states, and to report the device capability to userspace.

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h     |  2 ++
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 81591173265d..7a77051096c9 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1601,4 +1601,53 @@ drm_atomic_get_new_bridge_state(const struct drm_atomic_commit *state,
 EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
 
+/**
+ * drm_atomic_implements_create_state - check if a device supports creating pristine states
+ * @dev: DRM device
+ *
+ * Check whether every plane, CRTC, and connector in @dev implements the
+ * &drm_plane_funcs.atomic_create_state, &drm_crtc_funcs.atomic_create_state,
+ * and &drm_connector_funcs.atomic_create_state hooks respectively. These hooks
+ * are required to create default states from scratch rather than duplicating
+ * the current state.
+ *
+ * Color operations are not checked because they always use
+ * drm_atomic_helper_colorop_create_state() and do not have a per-driver hook.
+ *
+ * Returns:
+ * True if all objects implement atomic_create_state, false otherwise.
+ */
+bool drm_atomic_implements_create_state(struct drm_device *dev)
+{
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+
+	/*
+	 * colorops don't have an atomic_create_state hook but
+	 * drm_atomic_helper_colorop_create_state()
+	 */
+
+	drm_for_each_plane(plane, dev)
+		if (!plane->funcs->atomic_create_state)
+			return false;
+
+	drm_for_each_crtc(crtc, dev)
+		if (!crtc->funcs->atomic_create_state)
+			return false;
+
+	drm_connector_list_iter_begin(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		if (!connector->funcs->atomic_create_state) {
+			drm_connector_list_iter_end(&conn_iter);
+			return false;
+		}
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	return true;
+}
+EXPORT_SYMBOL(drm_atomic_implements_create_state);
+
 /**
  * drm_atomic_add_encoder_bridges - add bridges attached to an encoder
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 3ae35b09c0cf..72c3d420f40c 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -703,4 +703,6 @@ drm_atomic_commit_alloc(struct drm_device *dev);
 void drm_atomic_commit_clear(struct drm_atomic_commit *state);
 
+bool drm_atomic_implements_create_state(struct drm_device *dev);
+
 /**
  * drm_atomic_commit_get - acquire a reference to the atomic state

-- 
2.55.0

[PATCH v7 12/16] drm/atomic: Allow filling a commit with pristine object states

[Maxime Ripard]

The upcoming DRM_MODE_ATOMIC_RESET flag will need to create an atomic
commit that brings the entire device back to a pristine state, as if
no configuration had ever been applied.

Create drm_atomic_commit_fill_with_defaults() which iterates over all
CRTCs, planes, connectors, and color operations in the device and
inserts a fresh default state for each one into the commit. This uses
the atomic_create_state() hooks rather than atomic_duplicate_state(),
since atomic_create_state() provides exactly this pristine state on a
per-object basis.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c | 285 +++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_ioctl.c  |   1 +
 include/drm/drm_atomic.h     |   2 +
 3 files changed, 288 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7a77051096c9..6bc0d6339f14 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1650,4 +1650,289 @@ bool drm_atomic_implements_create_state(struct drm_device *dev)
 EXPORT_SYMBOL(drm_atomic_implements_create_state);
 
+/*
+ * Returns true if the colorop state @new differs from the colorop state
+ * @old.
+ */
+static bool drm_atomic_colorop_state_changed(const struct drm_colorop_state *old,
+					     const struct drm_colorop_state *new)
+{
+	return old->bypass != new->bypass ||
+	       old->curve_1d_type != new->curve_1d_type ||
+	       old->multiplier != new->multiplier ||
+	       old->data != new->data ||
+	       old->lut1d_interpolation != new->lut1d_interpolation ||
+	       old->lut3d_interpolation != new->lut3d_interpolation ||
+	       old->fixed_matrix_type != new->fixed_matrix_type;
+}
+
+/**
+ * drm_atomic_commit_fill_with_defaults - populate a commit with pristine states
+ * @file_priv: DRM file the reset was requested from, or NULL
+ * @commit: atomic commit to fill
+ *
+ * Iterate over all CRTCs, planes, connectors, and color operations in
+ * the device and insert a freshly created default state for each one
+ * into @commit. The states are created through the atomic_create_state()
+ * hooks, producing the same initial state the driver starts with rather
+ * than a copy of the current hardware state.
+ *
+ * If @file_priv is a lessee, only the CRTCs, planes and connectors part
+ * of its lease are reset, and the other ones are left untouched.
+ * @file_priv is optional: if NULL, it is considered to not hold any
+ * lease, and all objects are reset.
+ *
+ * The objects that are bound to the objects being reset are still added
+ * to the commit, with their current state: the CRTC a reset plane or
+ * connector is bound to, and the connectors bound to a reset CRTC. The
+ * atomic helpers require them to be part of the commit to update the
+ * CRTC state and the routing, and they would dereference a NULL state
+ * otherwise.
+ *
+ * In the default state, all objects are disabled: CRTCs are inactive,
+ * planes are disabled, and connectors are unbound. Driver-specific
+ * properties such as color pipeline settings or max_bpc may carry
+ * non-zero defaults appropriate for basic display output.
+ *
+ * The CRTC and plane states in the commit carry color_mgmt_changed set
+ * when the reset changes the color management properties compared to the
+ * current state, so that drivers reprogram the color management
+ * hardware.
+ *
+ * The connector states keep the encoder currently in use in their
+ * best_encoder field, like a duplicated state would, so that the atomic
+ * helpers can detect the commits that don't change the connector
+ * routing.
+ *
+ * This is meant to be used with the %DRM_MODE_ATOMIC_RESET flag, which
+ * needs to bring the device back to a known baseline before applying
+ * userspace property changes on top.
+ *
+ * Returns:
+ * 0 on success, or a negative error code on failure.
+ */
+int drm_atomic_commit_fill_with_defaults(struct drm_file *file_priv,
+					 struct drm_atomic_commit *commit)
+{
+	struct drm_device *dev = commit->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_crtc *crtc;
+	struct drm_plane *plane;
+	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_colorop *colorop;
+	int ret;
+
+	drm_WARN_ON(dev, !commit->acquire_ctx);
+
+	/*
+	 * Private objects are ignored because none have userspace
+	 * properties we might want to reset. atomic_check
+	 * implementations will derive or infer their private obj state
+	 * from the state that will end up being committed anyway.
+	 *
+	 * The CRTCs are handled before the planes and connectors so that,
+	 * when those pull the CRTC they are currently bound to into the
+	 * commit, a leased CRTC is already there with its pristine state
+	 * while an unleased one gets duplicated and keeps its current
+	 * state.
+	 */
+	drm_for_each_crtc(crtc, dev) {
+		struct drm_crtc_state *crtc_state;
+
+		if (!drm_lease_held(file_priv, crtc->base.id))
+			continue;
+
+		ret = drm_modeset_lock(&crtc->mutex, commit->acquire_ctx);
+		if (ret)
+			return ret;
+
+		crtc_state = crtc->funcs->atomic_create_state(crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		/*
+		 * The userspace property handlers only set
+		 * color_mgmt_changed when a property write changes the
+		 * value stored in the new state. Since the pristine states
+		 * we create here are already set to their default values,
+		 * restoring those defaults wouldn't set the flag, and
+		 * neither would leaving the properties untouched, even
+		 * though the commit clears whatever LUT or CTM is
+		 * currently programmed. Compare with the current state
+		 * so that the drivers reprogram the color management
+		 * hardware.
+		 */
+		crtc_state->color_mgmt_changed =
+			crtc->state->degamma_lut != crtc_state->degamma_lut ||
+			crtc->state->ctm != crtc_state->ctm ||
+			crtc->state->gamma_lut != crtc_state->gamma_lut;
+
+		drm_atomic_commit_set_crtc_state(commit, crtc, crtc_state);
+	}
+
+	drm_for_each_plane(plane, dev) {
+		struct drm_plane_state *plane_state;
+
+		if (!drm_lease_held(file_priv, plane->base.id))
+			continue;
+
+		ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx);
+		if (ret)
+			return ret;
+
+		/*
+		 * Make sure the CRTC the plane is currently enabled on is
+		 * part of the commit, like drm_atomic_set_crtc_for_plane()
+		 * does when userspace disables a plane. The atomic helpers
+		 * need the CRTC state to update planes_changed and track
+		 * the commit, and would dereference a NULL CRTC state
+		 * otherwise.
+		 */
+		if (plane->state->crtc) {
+			struct drm_crtc_state *crtc_state;
+
+			crtc_state = drm_atomic_get_crtc_state(commit,
+							       plane->state->crtc);
+			if (IS_ERR(crtc_state))
+				return PTR_ERR(crtc_state);
+		}
+
+		plane_state = plane->funcs->atomic_create_state(plane);
+		if (IS_ERR(plane_state))
+			return PTR_ERR(plane_state);
+
+		/*
+		 * Same thing for the planes: report a color pipeline change
+		 * compared to the current state.
+		 */
+		plane_state->color_mgmt_changed =
+			plane->state->color_pipeline != plane_state->color_pipeline;
+
+		drm_atomic_commit_set_plane_state(commit, plane, plane_state);
+	}
+
+	drm_for_each_colorop(colorop, dev) {
+		struct drm_plane_state *plane_state;
+		struct drm_colorop_state *colorop_state;
+
+		if (!drm_lease_held(file_priv, colorop->plane->base.id))
+			continue;
+
+		ret = drm_modeset_lock(&colorop->plane->mutex, commit->acquire_ctx);
+		if (ret)
+			return ret;
+
+		colorop_state = drm_atomic_helper_colorop_create_state(colorop);
+		if (IS_ERR(colorop_state))
+			return PTR_ERR(colorop_state);
+
+		drm_atomic_commit_set_colorop_state(commit, colorop, colorop_state);
+
+		/*
+		 * Report the colorop state changes on their plane state,
+		 * like drm_atomic_set_property() does when userspace
+		 * changes a colorop property.
+		 */
+		if (drm_atomic_colorop_state_changed(colorop->state, colorop_state)) {
+			plane_state = drm_atomic_get_new_plane_state(commit,
+								     colorop->plane);
+			if (plane_state)
+				plane_state->color_mgmt_changed = true;
+		}
+	}
+
+	ret = drm_modeset_lock(&config->connection_mutex, commit->acquire_ctx);
+	if (ret)
+		return ret;
+
+	drm_connector_list_iter_begin(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		struct drm_connector_state *connector_state;
+		struct drm_crtc_state *crtc_state;
+
+		if (!drm_lease_held(file_priv, connector->base.id)) {
+			/*
+			 * The connector isn't part of the lease so it keeps
+			 * its current state, but if it is bound to a CRTC
+			 * that is part of the commit, pull it in like
+			 * drm_atomic_add_affected_connectors() would do on a
+			 * regular commit, so that its routing and encoder
+			 * are properly handled.
+			 */
+			if (!connector->state->crtc)
+				continue;
+
+			if (!drm_atomic_get_new_crtc_state(commit,
+							   connector->state->crtc))
+				continue;
+
+			connector_state = drm_atomic_get_connector_state(commit,
+									 connector);
+			if (IS_ERR(connector_state)) {
+				drm_connector_list_iter_end(&conn_iter);
+				return PTR_ERR(connector_state);
+			}
+
+			continue;
+		}
+
+		/*
+		 * drm_atomic_implements_create_state() already checked
+		 * this, but connectors can be hotplugged between that
+		 * lockless check and the iteration here.
+		 */
+		if (!connector->funcs->atomic_create_state) {
+			drm_connector_list_iter_end(&conn_iter);
+			return -EOPNOTSUPP;
+		}
+
+		connector_state = connector->funcs->atomic_create_state(connector);
+		if (IS_ERR(connector_state)) {
+			drm_connector_list_iter_end(&conn_iter);
+			ret = PTR_ERR(connector_state);
+			return ret;
+		}
+
+		/*
+		 * best_encoder is a routing information maintained by the
+		 * atomic helpers rather than a userspace property, and it
+		 * is only carried over to a new state when that state is a
+		 * duplicate of the current one. Since
+		 * update_connector_routing() uses it to detect the commits
+		 * that keep the connector routing unchanged, let's carry
+		 * the encoder currently in use over so that a reset on
+		 * which userspace re-applies the exact same configuration
+		 * doesn't require a modeset.
+		 */
+		connector_state->best_encoder = connector->state->best_encoder;
+
+		ret = drm_atomic_commit_set_connector_state(commit, connector, connector_state);
+		if (ret) {
+			connector->funcs->atomic_destroy_state(connector, connector_state);
+			drm_connector_list_iter_end(&conn_iter);
+			return ret;
+		}
+
+		/*
+		 * Make sure the CRTC the connector is currently bound to
+		 * is part of the commit, like drm_atomic_get_connector_state()
+		 * does, so that the atomic helpers can update the CRTC
+		 * state and the connector routing.
+		 */
+		if (connector->state->crtc) {
+			crtc_state = drm_atomic_get_crtc_state(commit,
+							       connector->state->crtc);
+			if (IS_ERR(crtc_state)) {
+				drm_connector_list_iter_end(&conn_iter);
+				return PTR_ERR(crtc_state);
+			}
+		}
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_commit_fill_with_defaults);
+
 /**
  * drm_atomic_add_encoder_bridges - add bridges attached to an encoder
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 9039a39c4324..0dbf04d4aa9e 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -34,4 +34,5 @@
 #include <linux/uaccess.h>
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_auth.h>
 #include <drm/drm_crtc.h>
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 72c3d420f40c..cac74d43a34d 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -704,4 +704,6 @@ void drm_atomic_commit_clear(struct drm_atomic_commit *state);
 
 bool drm_atomic_implements_create_state(struct drm_device *dev);
+int drm_atomic_commit_fill_with_defaults(struct drm_file *file_priv,
+					 struct drm_atomic_commit *commit);
 
 /**

-- 
2.55.0

[PATCH v7 13/16] drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()

[Maxime Ripard]

drm_atomic_commit_fill_with_defaults() is used by the
DRM_MODE_ATOMIC_RESET flag to bring all objects back to their pristine
state. However, it has no dedicated test coverage.

Add two tests that verify the reset cycle. The first enables a CRTC,
plane, and connector, then calls drm_atomic_commit_fill_with_defaults()
and verifies all objects return to their disabled state: CRTC inactive,
plane disabled with no framebuffer, and connector unbound. The second
goes further and commits the default-filled state to verify the
init -> enable -> reset -> commit sequence completes without errors.

Suggested-by: Daniel Stone <daniel@fooishbar.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/tests/drm_atomic_test.c | 313 +++++++++++++++++++++++++++++++-
 1 file changed, 311 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_atomic_test.c b/drivers/gpu/drm/tests/drm_atomic_test.c
index 5e156d09e56f..160de1aa3ec2 100644
--- a/drivers/gpu/drm/tests/drm_atomic_test.c
+++ b/drivers/gpu/drm/tests/drm_atomic_test.c
@@ -9,4 +9,5 @@
 #include <drm/drm_kunit_helpers.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_property.h>
 
 #include <kunit/test.h>
@@ -143,5 +144,4 @@ static struct kunit_case drm_atomic_get_connector_for_encoder_tests[] = {
 };
 
-
 static struct kunit_suite drm_atomic_get_connector_for_encoder_test_suite = {
 	.name = "drm_test_atomic_get_connector_for_encoder",
@@ -149,5 +149,314 @@ static struct kunit_suite drm_atomic_get_connector_for_encoder_test_suite = {
 };
 
-kunit_test_suite(drm_atomic_get_connector_for_encoder_test_suite);
+/*
+ * Test that after enabling a CRTC and connector, filling with defaults
+ * produces a state where all objects are disabled: CRTC is inactive,
+ * plane is disabled, and connector is unbound.
+ *
+ * This is done without any lease, so all objects are expected to be
+ * reset.
+ *
+ * The plane is not explicitly enabled with a framebuffer here, so
+ * this validates that fill_with_defaults produces a consistent
+ * disabled state rather than testing an active plane teardown.
+ */
+static void drm_test_check_fill_with_defaults(struct kunit *test)
+{
+	struct drm_atomic_test_priv *priv;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_connector_state *new_conn_state;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_plane_state *new_plane_state;
+	struct drm_atomic_commit *state;
+	struct drm_display_mode *mode;
+	struct drm_device *drm;
+	int ret;
+
+	priv = create_device(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+
+	drm = &priv->drm;
+
+	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 16);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+retry_enable:
+	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
+						     priv->crtc,
+						     &priv->connector,
+						     mode, &ctx);
+	if (ret == -EDEADLK) {
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry_enable;
+	}
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
+retry_fill:
+	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
+	if (ret == -EDEADLK) {
+		drm_atomic_commit_clear(state);
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry_fill;
+	}
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, priv->crtc);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state);
+	KUNIT_EXPECT_FALSE(test, new_crtc_state->enable);
+	KUNIT_EXPECT_FALSE(test, new_crtc_state->active);
+
+	new_plane_state = drm_atomic_get_new_plane_state(state, priv->plane);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_plane_state);
+	KUNIT_EXPECT_NULL(test, new_plane_state->crtc);
+	KUNIT_EXPECT_NULL(test, new_plane_state->fb);
+
+	new_conn_state = drm_atomic_get_new_connector_state(state, &priv->connector);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state);
+	KUNIT_EXPECT_NULL(test, new_conn_state->crtc);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
+/*
+ * Test that after enabling a CRTC and connector, fill_with_defaults
+ * produces a state that can be successfully committed.
+ *
+ * This is done without any lease, so all objects are reset.
+ *
+ * The plane is not explicitly enabled with a framebuffer, so this
+ * validates that fill_with_defaults produces a consistent disabled
+ * state rather than testing an active plane teardown.
+ */
+static void drm_test_check_fill_with_defaults_commit(struct kunit *test)
+{
+	struct drm_atomic_test_priv *priv;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_atomic_commit *state;
+	struct drm_display_mode *mode;
+	struct drm_device *drm;
+	int ret;
+
+	priv = create_device(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+
+	drm = &priv->drm;
+
+	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 16);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+retry_enable:
+	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
+						     priv->crtc,
+						     &priv->connector,
+						     mode, &ctx);
+	if (ret == -EDEADLK) {
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry_enable;
+	}
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
+retry_fill:
+	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
+	if (ret == -EDEADLK) {
+		drm_atomic_commit_clear(state);
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry_fill;
+	}
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	state->allow_modeset = true;
+
+	ret = drm_atomic_commit(state);
+	if (ret == -EDEADLK) {
+		drm_atomic_commit_clear(state);
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry_fill;
+	}
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
+/*
+ * Test that filling with defaults reports the color management changes
+ * on the new states through color_mgmt_changed, so that drivers
+ * reprogram the color management hardware when a LUT is being cleared
+ * by the reset.
+ */
+static void drm_test_check_fill_with_defaults_color_mgmt(struct kunit *test)
+{
+	struct drm_atomic_test_priv *priv;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_plane_state *new_plane_state;
+	struct drm_atomic_commit *state;
+	struct drm_property_blob *gamma_lut;
+	struct drm_device *drm;
+	u32 lut = 0;
+	int ret;
+
+	priv = create_device(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+
+	drm = &priv->drm;
+
+	gamma_lut = drm_property_create_blob(drm, sizeof(lut), &lut);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gamma_lut);
+
+	/*
+	 * Attach a gamma LUT to the current CRTC state, like userspace
+	 * setting the GAMMA_LUT property would. The state takes ownership
+	 * of the blob reference.
+	 */
+	priv->crtc->state->gamma_lut = gamma_lut;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
+retry_fill:
+	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
+	if (ret == -EDEADLK) {
+		drm_atomic_commit_clear(state);
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry_fill;
+	}
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, priv->crtc);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state);
+	KUNIT_EXPECT_NULL(test, new_crtc_state->gamma_lut);
+	KUNIT_EXPECT_TRUE(test, new_crtc_state->color_mgmt_changed);
+
+	new_plane_state = drm_atomic_get_new_plane_state(state, priv->plane);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_plane_state);
+	KUNIT_EXPECT_FALSE(test, new_plane_state->color_mgmt_changed);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
+/*
+ * Test that a reset on which userspace re-applies the exact same
+ * configuration doesn't need a modeset, and thus doesn't require the
+ * DRM_MODE_ATOMIC_ALLOW_MODESET flag.
+ */
+static void drm_test_check_fill_with_defaults_no_modeset(struct kunit *test)
+{
+	struct drm_atomic_test_priv *priv;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_connector_state *new_conn_state;
+	struct drm_atomic_commit *state;
+	struct drm_display_mode *mode;
+	struct drm_device *drm;
+	int ret;
+
+	priv = create_device(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+
+	drm = &priv->drm;
+
+	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 16);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+retry_enable:
+	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
+						     priv->crtc,
+						     &priv->connector,
+						     mode, &ctx);
+	if (ret == -EDEADLK) {
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry_enable;
+	}
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
+retry:
+	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
+	if (ret == -EDEADLK) {
+		drm_atomic_commit_clear(state);
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry;
+	}
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/* Re-apply the exact same configuration on top of the reset */
+	new_conn_state = drm_atomic_get_new_connector_state(state, &priv->connector);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state);
+
+	ret = drm_atomic_set_crtc_for_connector(new_conn_state, priv->crtc);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, priv->crtc);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_crtc_state);
+
+	ret = drm_atomic_set_mode_for_crtc(new_crtc_state, mode);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	new_crtc_state->enable = true;
+	new_crtc_state->active = true;
+
+	/* Modesets aren't allowed and shouldn't be needed */
+	state->allow_modeset = false;
+
+	ret = drm_atomic_check_only(state);
+	if (ret == -EDEADLK) {
+		drm_atomic_commit_clear(state);
+		ret = drm_modeset_backoff(&ctx);
+		if (!ret)
+			goto retry;
+	}
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_FALSE(test, new_crtc_state->active_changed);
+	KUNIT_EXPECT_FALSE(test, new_crtc_state->connectors_changed);
+	KUNIT_EXPECT_FALSE(test, new_crtc_state->mode_changed);
+	KUNIT_EXPECT_FALSE(test, drm_atomic_crtc_needs_modeset(new_crtc_state));
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
+static struct kunit_case drm_atomic_fill_with_defaults_tests[] = {
+	KUNIT_CASE(drm_test_check_fill_with_defaults),
+	KUNIT_CASE(drm_test_check_fill_with_defaults_commit),
+	KUNIT_CASE(drm_test_check_fill_with_defaults_color_mgmt),
+	KUNIT_CASE(drm_test_check_fill_with_defaults_no_modeset),
+	{ }
+};
+
+static struct kunit_suite drm_atomic_fill_with_defaults_test_suite = {
+	.name = "drm_atomic_commit_fill_with_defaults",
+	.test_cases = drm_atomic_fill_with_defaults_tests,
+};
+
+kunit_test_suites(&drm_atomic_get_connector_for_encoder_test_suite,
+		  &drm_atomic_fill_with_defaults_test_suite);
 
 MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>");

-- 
2.55.0

[PATCH v7 14/16] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

[Maxime Ripard]

From: Sebastian Wick <sebastian.wick@redhat.com>

Userspace currently has no atomic way to reset all KMS object states
to their defaults. To bring a display pipeline to a known state, a
compositor must explicitly set every property on every object, which
requires tracking which properties exist and what their defaults are.

Introduce DRM_MODE_ATOMIC_RESET (0x0800) which, when passed to the
atomic ioctl, fills the commit with default states for all KMS
objects before applying the properties supplied in the request.
Properties not explicitly included in the commit remain at their
defaults (CRTCs inactive, planes disabled, connectors unbound, and
so on).

The flag cannot be combined with DRM_MODE_PAGE_FLIP_ASYNC, since a
full state reset is incompatible with an async flip.

Since the commit contains every CRTC of the device or lease, including
the ones that are disabled before and after it, no page flip event is
attached to those: they can't report a flip, and drm_atomic_crtc_check()
would reject the whole commit. Such a CRTC doesn't count towards the
CRTCs that can signal either, so a commit that leaves them all disabled
while asking for an event is still rejected instead of waiting for an
event that would never come.

Signed-off-by: Sebastian Wick <sebastian.wick@redhat.com>
Co-developed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c      |  3 +++
 drivers/gpu/drm/drm_atomic_uapi.c | 44 ++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_ioctl.c       |  4 ++++
 include/uapi/drm/drm.h            | 10 +++++++++
 include/uapi/drm/drm_mode.h       | 17 ++++++++++++++-
 5 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6bc0d6339f14..16530ce62455 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1614,4 +1614,7 @@ EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
  * drm_atomic_helper_colorop_create_state() and do not have a per-driver hook.
  *
+ * This is used to report the %DRM_CAP_ATOMIC_RESET capability to userspace,
+ * and as a precondition in drm_atomic_commit_fill_with_defaults().
+ *
  * Returns:
  * True if all objects implement atomic_create_state, false otherwise.
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 1eebcf6f5e05..77d0713c7e2d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1441,8 +1441,27 @@ static int prepare_signaling(struct drm_device *dev,
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		s32 __user *fence_ptr;
+		bool page_flip_event;
 
 		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
 
-		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
+		page_flip_event = arg->flags & DRM_MODE_PAGE_FLIP_EVENT;
+
+		/*
+		 * A commit with the DRM_MODE_ATOMIC_RESET flag contains
+		 * every CRTC of the device or lease, even the ones that are
+		 * disabled before and after the commit. Those can't report
+		 * a page flip, and drm_atomic_crtc_check() would reject the
+		 * whole commit, so don't attach any event to them.
+		 */
+		if (page_flip_event && !fence_ptr &&
+		    (arg->flags & DRM_MODE_ATOMIC_RESET) &&
+		    !crtc_state->active) {
+			struct drm_crtc_state *old_crtc_state;
+
+			old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+			page_flip_event = old_crtc_state->active;
+		}
+
+		if (page_flip_event || fence_ptr) {
 			struct drm_pending_vblank_event *e;
 
@@ -1454,5 +1473,5 @@ static int prepare_signaling(struct drm_device *dev,
 		}
 
-		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
+		if (page_flip_event) {
 			struct drm_pending_vblank_event *e = crtc_state->event;
 
@@ -1495,5 +1514,6 @@ static int prepare_signaling(struct drm_device *dev,
 		}
 
-		c++;
+		if (page_flip_event)
+			c++;
 	}
 
@@ -1674,4 +1694,11 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	}
 
+	if ((arg->flags & DRM_MODE_ATOMIC_RESET) &&
+			(arg->flags & DRM_MODE_PAGE_FLIP_ASYNC)) {
+		drm_dbg_atomic(dev,
+			       "commit failed: reset cannot be combined with async flip\n");
+		return -EINVAL;
+	}
+
 	state = drm_atomic_commit_alloc(dev);
 	if (!state)
@@ -1689,4 +1716,15 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	num_fences = 0;
 
+	if (arg->flags & DRM_MODE_ATOMIC_RESET) {
+		if (!drm_atomic_implements_create_state(dev)) {
+			ret = -EOPNOTSUPP;
+			goto out;
+		}
+
+		ret = drm_atomic_commit_fill_with_defaults(file_priv, state);
+		if (ret)
+			goto out;
+	}
+
 	for (i = 0; i < arg->count_objs; i++) {
 		uint32_t obj_id, count_props;
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 0dbf04d4aa9e..f666a13da6bf 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -306,4 +306,8 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 			     dev->mode_config.async_page_flip;
 		break;
+	case DRM_CAP_ATOMIC_RESET:
+		req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) &&
+			     drm_atomic_implements_create_state(dev);
+		break;
 	default:
 		return -EINVAL;
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index bc7ef7684099..b6e2f2edd122 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -796,4 +796,14 @@ struct drm_gem_change_handle {
 #define DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP	0x15
 
+/**
+ * DRM_CAP_ATOMIC_RESET
+ *
+ * If set to 1, the driver supports the &DRM_MODE_ATOMIC_RESET flag in
+ * &DRM_IOCTL_MODE_ATOMIC commits. When supported, userspace can pass that
+ * flag to reset all KMS object states to their defaults before applying
+ * property changes.
+ */
+#define DRM_CAP_ATOMIC_RESET	0x16
+
 /* DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index ed1a660a3dfd..1ab5c0fb3804 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1329,4 +1329,18 @@ struct drm_mode_destroy_dumb {
  */
 #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
+/**
+ * DRM_MODE_ATOMIC_RESET
+ *
+ * Reset all KMS object states (CRTCs, planes, connectors, color operations)
+ * to their default values before applying the properties in this commit.
+ * Properties not explicitly included in the commit will remain at their
+ * defaults (CRTCs inactive, planes disabled, connectors unbound, etc.).
+ *
+ * CRTCs that are disabled before and after the commit won't send any
+ * &DRM_MODE_PAGE_FLIP_EVENT.
+ *
+ * This flag cannot be combined with &DRM_MODE_PAGE_FLIP_ASYNC.
+ */
+#define DRM_MODE_ATOMIC_RESET 0x0800
 
 /**
@@ -1341,5 +1355,6 @@ struct drm_mode_destroy_dumb {
 		DRM_MODE_ATOMIC_TEST_ONLY |\
 		DRM_MODE_ATOMIC_NONBLOCK |\
-		DRM_MODE_ATOMIC_ALLOW_MODESET)
+		DRM_MODE_ATOMIC_ALLOW_MODESET |\
+		DRM_MODE_ATOMIC_RESET)
 
 struct drm_mode_atomic {

-- 
2.55.0

[PATCH v7 15/16] drm/vkms: Switch container_of helpers to container_of_const

[Maxime Ripard]

The vkms container_of helpers use container_of() which discards the
const qualifier from the input pointer. This prevents passing a const
state pointer to to_vkms_plane_state() without a cast, for instance
in atomic_get_property which receives a const drm_plane_state.

Switch all four helpers to container_of_const() so the const qualifier
is properly propagated to the result type.

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vkms/vkms_drv.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 381483aa4fb0..dea6cad3f1b9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -251,14 +251,14 @@ struct vkms_device {
 
 #define drm_crtc_to_vkms_output(target) \
-	container_of(target, struct vkms_output, crtc)
+	container_of_const(target, struct vkms_output, crtc)
 
 #define drm_device_to_vkms_device(target) \
-	container_of(target, struct vkms_device, drm)
+	container_of_const(target, struct vkms_device, drm)
 
 #define to_vkms_crtc_state(target)\
-	container_of(target, struct vkms_crtc_state, base)
+	container_of_const(target, struct vkms_crtc_state, base)
 
 #define to_vkms_plane_state(target)\
-	container_of(target, struct vkms_plane_state, base.base)
+	container_of_const(target, struct vkms_plane_state, base.base)
 
 /**

-- 
2.55.0

[PATCH v7 16/16] drm/vkms: Add driver-specific plane property for testing

[Maxime Ripard]

The DRM_MODE_ATOMIC_RESET flag allows userspace to reset all object
states to their defaults before applying new property values. Testing
that this mechanism works correctly requires a property that the test
can set to a non-default value and then verify is properly reset.

Using a standard property for this is problematic: compositors like
Mutter enumerate and track all known properties, making it difficult
to isolate the reset behavior from the compositor's own property
management.

Add a "vkms-test-do-not-use-in-prod" range property to vkms planes.
The property is stored in vkms_plane_state, duplicated across state
copies, and initialized from its registered default in
atomic_create_state via drm_object_property_get_default_value().

Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vkms/vkms_drv.h   |  3 ++
 drivers/gpu/drm/vkms/vkms_plane.c | 63 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index dea6cad3f1b9..384616095b45 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -147,4 +147,5 @@ struct conversion_matrix {
  *		     struct vkms_plane_state must ensure that this pointer is valid
  * @conversion_matrix: matrix used for yuv formats to convert to rgb
+ * @vkms_test_do_not_use_value: driver-specific property for testing only
  */
 struct vkms_plane_state {
@@ -153,8 +154,10 @@ struct vkms_plane_state {
 	pixel_read_line_t pixel_read_line;
 	struct conversion_matrix conversion_matrix;
+	unsigned int vkms_test_do_not_use_value;
 };
 
 struct vkms_plane {
 	struct drm_plane base;
+	struct drm_property *vkms_test_do_not_use_property;
 };
 
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index fd3d34dcf0fe..42513cb462ac 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -12,8 +12,12 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_print.h>
+#include <drm/drm_property.h>
 
 #include "vkms_drv.h"
 #include "vkms_formats.h"
 
+#define to_vkms_plane(target) \
+	container_of_const(target, struct vkms_plane, base)
+
 static const u32 vkms_formats[] = {
 	DRM_FORMAT_ARGB8888,
@@ -55,4 +59,5 @@ static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
+	struct vkms_plane_state *old_state = to_vkms_plane_state(plane->state);
 	struct vkms_plane_state *vkms_state;
 
@@ -61,4 +66,6 @@ vkms_plane_duplicate_state(struct drm_plane *plane)
 		return NULL;
 
+	vkms_state->vkms_test_do_not_use_value = old_state->vkms_test_do_not_use_value;
+
 	__drm_gem_duplicate_shadow_plane_state(plane, &vkms_state->base);
 
@@ -86,5 +93,7 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 static struct drm_plane_state *vkms_plane_create_state(struct drm_plane *plane)
 {
+	struct vkms_plane *vkms_plane = to_vkms_plane(plane);
 	struct vkms_plane_state *vkms_state;
+	u64 val;
 
 	vkms_state = kzalloc_obj(*vkms_state);
@@ -96,12 +105,51 @@ static struct drm_plane_state *vkms_plane_create_state(struct drm_plane *plane)
 	__drm_gem_shadow_plane_state_init(plane, &vkms_state->base);
 
+	if (!drm_object_property_get_default_value(&plane->base,
+						   vkms_plane->vkms_test_do_not_use_property,
+						   &val))
+		vkms_state->vkms_test_do_not_use_value = val;
+
 	return &(&vkms_state->base)->base;
 }
 
+static int vkms_plane_atomic_set_property(struct drm_plane *plane,
+					  struct drm_plane_state *state,
+					  struct drm_property *property,
+					  uint64_t val)
+{
+	struct vkms_plane *vkms_plane = to_vkms_plane(plane);
+	struct vkms_plane_state *vkms_state = to_vkms_plane_state(state);
+
+	if (property == vkms_plane->vkms_test_do_not_use_property) {
+		vkms_state->vkms_test_do_not_use_value = val;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int vkms_plane_atomic_get_property(struct drm_plane *plane,
+					  const struct drm_plane_state *state,
+					  struct drm_property *property,
+					  uint64_t *val)
+{
+	const struct vkms_plane_state *vkms_state = to_vkms_plane_state(state);
+	const struct vkms_plane *vkms_plane = to_vkms_plane(plane);
+
+	if (property == vkms_plane->vkms_test_do_not_use_property) {
+		*val = vkms_state->vkms_test_do_not_use_value;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static const struct drm_plane_funcs vkms_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
 	.disable_plane		= drm_atomic_helper_disable_plane,
-	.atomic_create_state = vkms_plane_create_state,
-	.atomic_duplicate_state = vkms_plane_duplicate_state,
+	.atomic_set_property	= vkms_plane_atomic_set_property,
+	.atomic_get_property	= vkms_plane_atomic_get_property,
+	.atomic_create_state	= vkms_plane_create_state,
+	.atomic_duplicate_state	= vkms_plane_duplicate_state,
 	.atomic_destroy_state	= vkms_plane_destroy_state,
 };
@@ -284,4 +332,15 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 					  DRM_COLOR_YCBCR_FULL_RANGE);
 
+	plane->vkms_test_do_not_use_property =
+		drm_property_create_range(dev, 0,
+					  "vkms-test-do-not-use-in-prod",
+					  0, U32_MAX);
+	if (!plane->vkms_test_do_not_use_property)
+		return ERR_PTR(-ENOMEM);
+
+	drm_object_attach_property(&plane->base.base,
+				   plane->vkms_test_do_not_use_property,
+				   0);
+
 	if (vkms_config_plane_get_default_pipeline(plane_cfg))
 		vkms_initialize_colorops(&plane->base);

-- 
2.55.0

Re: [PATCH v7 02/16] drm/solomon: ssd130x: Drop the init-only assumption in ssd130x_crtc_create_state()

[Javier Martinez Canillas]

Maxime Ripard <mripard@kernel.org> writes:

Hello Maxime,

> The ssd130x_crtc_create_state() implementation warns when the CRTC
> already has a state, assuming that atomic_create_state would only ever
> be called at initialization time to allocate the initial state.
>
> However, atomic_create_state is documented to have no side effect and
> can be called at runtime, so that assumption doesn't hold and the WARN
> is a side effect in itself.
>
> Let's drop the WARN, along with the comment documenting that
> assumption.
>
> Fixes: 1345de7fe9114 ("drm/solomon: ssd130x: Convert to atomic_create_state")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
>
> ---
>
> To: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/gpu/drm/solomon/ssd130x.c | 3 ---
>  1 file changed, 3 deletions(-)
>

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v7 13/16] drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/tests: missing atomic_create_state in drm_atomic_init_connector_funcs
- [Low] drm/tests: commit message does not match added tests in drm_atomic_test.c
--

commit 0885e84f724b909721af519687158e094325a32c
Author: Maxime Ripard <mripard@kernel.org>

drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()

This commit adds a KUnit test suite in drivers/gpu/drm/tests/drm_atomic_test.c
to exercise drm_atomic_commit_fill_with_defaults().

>     drm/tests: atomic: Add tests for drm_atomic_commit_fill_with_defaults()
>     
>     drm_atomic_commit_fill_with_defaults() is used by the
>     DRM_MODE_ATOMIC_RESET flag to bring all objects back to their pristine
>     state. However, it has no dedicated test coverage.
>     
>     Add two tests that verify the reset cycle. The first enables a CRTC,
>     plane, and connector, then calls drm_atomic_commit_fill_with_defaults()
>     and verifies all objects return to their disabled state: CRTC inactive,
>     plane disabled with no framebuffer, and connector unbound. The second
>     goes further and commits the default-filled state to verify the
>     init -> enable -> reset -> commit sequence completes without errors.
>     
>     Suggested-by: Daniel Stone <daniel@fooishbar.org>
>     Signed-off-by: Maxime Ripard <mripard@kernel.org>

[Severity: Low]
This isn't a bug, but does the commit message match the tests added by this
patch?

The commit message states that two tests are added and that the first test
enables a CRTC, plane, and connector. However, drm_atomic_fill_with_defaults_tests
adds four test cases (also including drm_test_check_fill_with_defaults_color_mgmt
and drm_test_check_fill_with_defaults_no_modeset), and as noted in the comment
above drm_test_check_fill_with_defaults(), the test only enables the CRTC and
connector via drm_kunit_helper_enable_crtc_connector() without enabling the
plane.

> diff --git a/drivers/gpu/drm/tests/drm_atomic_test.c b/drivers/gpu/drm/tests/drm_atomic_test.c
> index ea91bec6569e6..e8f18b8f6361e 100644
> --- a/drivers/gpu/drm/tests/drm_atomic_test.c
> +++ b/drivers/gpu/drm/tests/drm_atomic_test.c

[ ... ]

> @@ -140,13 +141,321 @@ static struct kunit_case drm_atomic_get_connector_for_encoder_tests[] = {
>  	{ }
>  };
>  
> -
>  static struct kunit_suite drm_atomic_get_connector_for_encoder_test_suite = {
>  	.name = "drm_test_atomic_get_connector_for_encoder",
>  	.test_cases = drm_atomic_get_connector_for_encoder_tests,
>  };
>  
> -kunit_test_suite(drm_atomic_get_connector_for_encoder_test_suite);
> +/*
> + * Test that after enabling a CRTC and connector, filling with defaults
> + * produces a state where all objects are disabled: CRTC is inactive,
> + * plane is disabled, and connector is unbound.
> + *
> + * This is done without any lease, so all objects are expected to be
> + * reset.
> + *
> + * The plane is not explicitly enabled with a framebuffer here, so
> + * this validates that fill_with_defaults produces a consistent
> + * disabled state rather than testing an active plane teardown.
> + */
> +static void drm_test_check_fill_with_defaults(struct kunit *test)
> +{
> +	struct drm_atomic_test_priv *priv;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector_state *new_conn_state;
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_plane_state *new_plane_state;
> +	struct drm_atomic_commit *state;
> +	struct drm_display_mode *mode;
> +	struct drm_device *drm;
> +	int ret;
> +
> +	priv = create_device(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> +
> +	drm = &priv->drm;
> +
> +	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 16);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +retry_enable:
> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
> +						     priv->crtc,
> +						     &priv->connector,
> +						     mode, &ctx);
> +	if (ret == -EDEADLK) {
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry_enable;
> +	}
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
> +
> +retry_fill:
> +	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
> +	if (ret == -EDEADLK) {
> +		drm_atomic_commit_clear(state);
> +		ret = drm_modeset_backoff(&ctx);
> +		if (!ret)
> +			goto retry_fill;
> +	}
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
As reported by Sashiko on v6 [1], won't drm_atomic_commit_fill_with_defaults()
fail with -EOPNOTSUPP in all four tests here?

Each test calls create_device(), which initializes priv->connector with
drm_atomic_init_connector_funcs:

drivers/gpu/drm/tests/drm_atomic_test.c:
static const struct drm_connector_funcs drm_atomic_init_connector_funcs = {
	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
	.reset			= drm_atomic_helper_connector_reset,
};

Because .atomic_create_state is left NULL on drm_atomic_init_connector_funcs,
drm_atomic_commit_fill_with_defaults() acquires the modeset locks on
state->acquire_ctx and then returns -EOPNOTSUPP when iterating over connectors:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults()
		if (!connector->funcs->atomic_create_state) {
			drm_connector_list_iter_end(&conn_iter);
			return -EOPNOTSUPP;
		}

This causes KUNIT_ASSERT_EQ(test, ret, 0) in drm_test_check_fill_with_defaults(),
drm_test_check_fill_with_defaults_commit(),
drm_test_check_fill_with_defaults_color_mgmt(), and
drm_test_check_fill_with_defaults_no_modeset() to fail and abort the test
before drm_modeset_drop_locks(&ctx) and drm_modeset_acquire_fini(&ctx) are
reached, leaving the acquired modeset locks linked to the stack-allocated ctx
during KUnit device teardown.

Should drm_atomic_init_connector_funcs set .atomic_create_state to
drm_atomic_helper_connector_create_state?

[1] https://lore.kernel.org/r/20261022-drm-reset-state-flag-v6-12-f36d640b7e76@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-drm-reset-state-flag-v7-0-4e55eaeae683@kernel.org?part=13

Re: [PATCH v7 16/16] drm/vkms: Add driver-specific plane property for testing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/vkms: missing connector atomic_create_state breaks DRM_MODE_ATOMIC_RESET
--

commit dd5905026f6d7cc05c849f79e8c81bca404953ae
Author: Maxime Ripard <mripard@kernel.org>

drm/vkms: Add driver-specific plane property for testing

This commit adds a driver-specific "vkms-test-do-not-use-in-prod" range
property to vkms planes to allow userspace to test DRM_MODE_ATOMIC_RESET
without interfering with standard compositor properties.

> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index fd3d34dcf0fe5..42513cb462ace 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c

[ ... ]

> @@ -85,7 +92,9 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
>  
>  static struct drm_plane_state *vkms_plane_create_state(struct drm_plane *plane)
>  {
> +	struct vkms_plane *vkms_plane = to_vkms_plane(plane);
>  	struct vkms_plane_state *vkms_state;
> +	u64 val;
>  
>  	vkms_state = kzalloc_obj(*vkms_state);
>  	if (!vkms_state) {
> @@ -95,14 +104,53 @@ static struct drm_plane_state *vkms_plane_create_state(struct drm_plane *plane)
>  
>  	__drm_gem_shadow_plane_state_init(plane, &vkms_state->base);
>  
> +	if (!drm_object_property_get_default_value(&plane->base,
> +						   vkms_plane->vkms_test_do_not_use_property,
> +						   &val))
> +		vkms_state->vkms_test_do_not_use_value = val;

[Severity: Medium]
As reported by Sashiko on v6 [1], can vkms_plane_create_state() actually be
reached via DRM_MODE_ATOMIC_RESET while vkms connectors do not implement
atomic_create_state?

When userspace queries DRM_CAP_ATOMIC_RESET in drm_getcap() or submits an
atomic commit with DRM_MODE_ATOMIC_RESET in drm_mode_atomic_ioctl(),
drm_atomic_implements_create_state() and
drm_atomic_commit_fill_with_defaults() check all connectors on the device:

drivers/gpu/drm/drm_atomic.c:drm_atomic_implements_create_state() {
    ...
	drm_connector_list_iter_begin(dev, &conn_iter);
	drm_for_each_connector_iter(connector, &conn_iter) {
		if (!connector->funcs->atomic_create_state) {
			drm_connector_list_iter_end(&conn_iter);
			return false;
		}
	}
	drm_connector_list_iter_end(&conn_iter);
    ...
}

In vkms, both connector function tables still set .reset instead of
.atomic_create_state:

drivers/gpu/drm/vkms/vkms_connector.c:
static const struct drm_connector_funcs vkms_connector_funcs = {
	.detect = vkms_connector_detect,
	.fill_modes = drm_helper_probe_single_connector_modes,
	.reset = drm_atomic_helper_connector_reset,
	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};

drivers/gpu/drm/vkms/vkms_writeback.c:
static const struct drm_connector_funcs vkms_wb_connector_funcs = {
	.fill_modes = drm_helper_probe_single_connector_modes,
	.reset = drm_atomic_helper_connector_reset,
	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};

Because every valid vkms device requires at least one connector,
drm_atomic_implements_create_state() returns false and DRM_MODE_ATOMIC_RESET
commits fail with -EOPNOTSUPP before vkms_plane_create_state() can be invoked.

Should vkms_connector_funcs and vkms_wb_connector_funcs also be converted to
use .atomic_create_state = drm_atomic_helper_connector_create_state so this
property can be used to test DRM_MODE_ATOMIC_RESET?

[1] https://lore.kernel.org/r/20261022-drm-reset-state-flag-v6-15-f36d640b7e76@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-drm-reset-state-flag-v7-0-4e55eaeae683@kernel.org?part=16

Re: [PATCH v7 12/16] drm/atomic: Allow filling a commit with pristine object states

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/atomic: broken lease handling in drm_atomic_commit_fill_with_defaults()
- [High] drm/atomic: uninitialized adjusted_mode on no-modeset reset commits
--

commit bec50b903063597d979230c48b9e1dfe0d54aa1f
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Allow filling a commit with pristine object states

This commit adds drm_atomic_commit_fill_with_defaults() to populate an
atomic commit with pristine default states for all leased KMS objects in
the device using their atomic_create_state() hooks.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7a77051096c91..6bc0d6339f142 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
[ ... ]
> +int drm_atomic_commit_fill_with_defaults(struct drm_file *file_priv,
> +					 struct drm_atomic_commit *commit)
> +{
> +	struct drm_device *dev = commit->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
> +	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_colorop *colorop;
> +	int ret;
> +
> +	drm_WARN_ON(dev, !commit->acquire_ctx);
> +
> +	/*
> +	 * Private objects are ignored because none have userspace
> +	 * properties we might want to reset. atomic_check
> +	 * implementations will derive or infer their private obj state
> +	 * from the state that will end up being committed anyway.
> +	 *
> +	 * The CRTCs are handled before the planes and connectors so that,
> +	 * when those pull the CRTC they are currently bound to into the
> +	 * commit, a leased CRTC is already there with its pristine state
> +	 * while an unleased one gets duplicated and keeps its current
> +	 * state.
> +	 */
> +	drm_for_each_crtc(crtc, dev) {
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (!drm_lease_held(file_priv, crtc->base.id))
> +			continue;
> +
> +		ret = drm_modeset_lock(&crtc->mutex, commit->acquire_ctx);
> +		if (ret)
> +			return ret;
> +
> +		crtc_state = crtc->funcs->atomic_create_state(crtc);
> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
[ ... ]
> +		drm_atomic_commit_set_crtc_state(commit, crtc, crtc_state);
> +	}
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *plane_state;
> +
> +		if (!drm_lease_held(file_priv, plane->base.id))
> +			continue;
> +
> +		ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Make sure the CRTC the plane is currently enabled on is
> +		 * part of the commit, like drm_atomic_set_crtc_for_plane()
> +		 * does when userspace disables a plane. The atomic helpers
> +		 * need the CRTC state to update planes_changed and track
> +		 * the commit, and would dereference a NULL CRTC state
> +		 * otherwise.
> +		 */
> +		if (plane->state->crtc) {
> +			struct drm_crtc_state *crtc_state;
> +
> +			crtc_state = drm_atomic_get_crtc_state(commit,
> +							       plane->state->crtc);
> +			if (IS_ERR(crtc_state))
> +				return PTR_ERR(crtc_state);
> +		}
> +
> +		plane_state = plane->funcs->atomic_create_state(plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);
[ ... ]
> +		drm_atomic_commit_set_plane_state(commit, plane, plane_state);
> +	}
[ ... ]
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *connector_state;
> +		struct drm_crtc_state *crtc_state;
> +
> +		if (!drm_lease_held(file_priv, connector->base.id)) {
> +			/*
> +			 * The connector isn't part of the lease so it keeps
> +			 * its current state, but if it is bound to a CRTC
> +			 * that is part of the commit, pull it in like
> +			 * drm_atomic_add_affected_connectors() would do on a
> +			 * regular commit, so that its routing and encoder
> +			 * are properly handled.
> +			 */
> +			if (!connector->state->crtc)
> +				continue;
> +
> +			if (!drm_atomic_get_new_crtc_state(commit,
> +							   connector->state->crtc))
> +				continue;
> +
> +			connector_state = drm_atomic_get_connector_state(commit,
> +									 connector);
> +			if (IS_ERR(connector_state)) {
> +				drm_connector_list_iter_end(&conn_iter);
> +				return PTR_ERR(connector_state);
> +			}
> +
> +			continue;
> +		}
> +
> +		/*
> +		 * drm_atomic_implements_create_state() already checked
> +		 * this, but connectors can be hotplugged between that
> +		 * lockless check and the iteration here.
> +		 */
> +		if (!connector->funcs->atomic_create_state) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return -EOPNOTSUPP;
> +		}
> +
> +		connector_state = connector->funcs->atomic_create_state(connector);
> +		if (IS_ERR(connector_state)) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			ret = PTR_ERR(connector_state);
> +			return ret;
> +		}
> +
> +		/*
> +		 * best_encoder is a routing information maintained by the
> +		 * atomic helpers rather than a userspace property, and it
> +		 * is only carried over to a new state when that state is a
> +		 * duplicate of the current one. Since
> +		 * update_connector_routing() uses it to detect the commits
> +		 * that keep the connector routing unchanged, let's carry
> +		 * the encoder currently in use over so that a reset on
> +		 * which userspace re-applies the exact same configuration
> +		 * doesn't require a modeset.
> +		 */
> +		connector_state->best_encoder = connector->state->best_encoder;

[Severity: High]
When connector_state->best_encoder is carried over so that re-applying the
same active configuration on a reset commit avoids a modeset, does
new_crtc_state->adjusted_mode remain all zeroes?

In drm_atomic_commit_fill_with_defaults(), crtc->funcs->atomic_create_state()
creates a pristine state with a zeroed adjusted_mode. If userspace then
re-applies the current mode and connector routing in the same commit,
drm_atomic_set_mode_prop_for_crtc() updates new_crtc_state->mode and sets
new_crtc_state->enable = true, while both new_crtc_state->mode_changed and
new_crtc_state->connectors_changed remain false in
drm_atomic_helper_check_modeset().

In drivers/gpu/drm/drm_atomic_helper.c:mode_fixup():

	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
		if (!new_crtc_state->mode_changed &&
		    !new_crtc_state->connectors_changed)
			continue;

		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
	}

Because neither flag is set, mode_fixup() skips copying mode into
adjusted_mode. That leaves new_crtc_state->adjusted_mode zeroed (with
crtc_clock == 0) on an enabled crtc, passing a zeroed adjusted_mode to
bridge, encoder, and crtc check hooks and causing
drm_atomic_helper_calc_timestamping_constants() ->
drm_calc_timestamping_constants() to warn and zero out vblank timing
constants during commit.

Should crtc_state->adjusted_mode also be copied from
crtc->state->adjusted_mode in drm_atomic_commit_fill_with_defaults(), or
should mode_fixup() populate adjusted_mode when resetting?

> +
> +		ret = drm_atomic_commit_set_connector_state(commit, connector, connector_state);
> +		if (ret) {
> +			connector->funcs->atomic_destroy_state(connector, connector_state);
> +			drm_connector_list_iter_end(&conn_iter);
> +			return ret;
> +		}
> +
> +		/*
> +		 * Make sure the CRTC the connector is currently bound to
> +		 * is part of the commit, like drm_atomic_get_connector_state()
> +		 * does, so that the atomic helpers can update the CRTC
> +		 * state and the connector routing.
> +		 */
> +		if (connector->state->crtc) {
> +			crtc_state = drm_atomic_get_crtc_state(commit,
> +							       connector->state->crtc);
> +			if (IS_ERR(crtc_state)) {
> +				drm_connector_list_iter_end(&conn_iter);
> +				return PTR_ERR(crtc_state);
> +			}
> +		}
> +	}

[Severity: High]
As reported by Sashiko on v6 [1], can resetting a subset of objects for a
lessee leave crtc_state->plane_mask, crtc_state->connector_mask, and
cross-lease object routing inconsistent in
drm_atomic_commit_fill_with_defaults()?

First, when a leased plane or leased connector is currently bound to an
unleased crtc, drm_atomic_get_crtc_state() duplicates the unleased crtc's
current state into the commit, keeping drm_plane_mask(plane) in
crtc_state->plane_mask and drm_connector_mask(connector) in
crtc_state->connector_mask. The leased plane or connector is then given a
pristine state with crtc = NULL via atomic_create_state(), but unlike
drm_atomic_set_crtc_for_plane() and drm_atomic_set_crtc_for_connector(),
nothing clears those mask bits from the unleased crtc_state. In addition, if
an unleased connector cloned onto that same unleased crtc is visited earlier
in the connector loop than the leased connector,
drm_atomic_get_new_crtc_state(commit, connector->state->crtc) returns NULL
because the unleased crtc is only added to the commit later when the leased
connector is processed.

Second, when an unleased connector or unleased plane is currently bound to a
leased crtc, the leased crtc receives a pristine state with enable = false,
plane_mask = 0, and connector_mask = 0. For an unleased connector bound to
that leased crtc, the !drm_lease_held() branch calls
drm_atomic_get_connector_state(), duplicating connector->state with
connector_state->crtc still pointing to the disabled pristine crtc while
crtc_state->connector_mask is 0. In drm_atomic_helper_check_modeset(),
update_connector_routing() keeps the connector routed to the disabled crtc,
causing mode_valid() and mode_fixup() to validate against a zeroed mode or
leaving crtc_state->connector_mask missing the unleased connector if the
crtc is re-enabled. Likewise, an unleased plane bound to the leased crtc is
skipped in the plane loop and later pulled in by
drm_atomic_add_affected_planes() with its active state still bound to the
reset crtc, failing drm_atomic_helper_check_plane_state() when the crtc is
disabled or missing from crtc_state->plane_mask if re-enabled.

Third, drm_lease_held() acquires and releases mode_config.idr_mutex on each
individual call across the four loops (crtcs, planes, colorops, and
connectors). If drm_mode_revoke_lease_ioctl() -> drm_lease_revoke() runs
concurrently after the crtc loop has already inserted pristine disabled crtc
states, the subsequent plane and connector loops see drm_lease_held() return
false and treat the lessee's planes and connectors as unleased objects bound
to those reset crtcs.

[1] https://lore.kernel.org/r/20261022-drm-reset-state-flag-v6-11-f36d640b7e76@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-drm-reset-state-flag-v7-0-4e55eaeae683@kernel.org?part=12

Re: [PATCH v7 14/16] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/atomic: zeroed adjusted_mode on DRM_MODE_ATOMIC_RESET without modeset
- [High] drm/atomic: cross-lease state corruption and events on DRM_MODE_ATOMIC_RESET
--

commit c819cce4a5454d4a632c82a26b77df3ff48e4432
Author: Sebastian Wick <sebastian.wick@redhat.com>

drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

This commit introduces the DRM_MODE_ATOMIC_RESET flag and DRM_CAP_ATOMIC_RESET
capability so userspace can reset all KMS object states to their defaults
before applying the properties in an atomic commit. It also updates
prepare_signaling() to avoid attaching page flip events to CRTCs that remain
disabled across a reset commit.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1eebcf6f5e05b..77d0713c7e2d6 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1440,10 +1440,29 @@ static int prepare_signaling(struct drm_device *dev,
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		s32 __user *fence_ptr;
> +		bool page_flip_event;
>  
>  		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
>  
> -		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> +		page_flip_event = arg->flags & DRM_MODE_PAGE_FLIP_EVENT;
> +
> +		/*
> +		 * A commit with the DRM_MODE_ATOMIC_RESET flag contains
> +		 * every CRTC of the device or lease, even the ones that are
> +		 * disabled before and after the commit. Those can't report
> +		 * a page flip, and drm_atomic_crtc_check() would reject the
> +		 * whole commit, so don't attach any event to them.
> +		 */
> +		if (page_flip_event && !fence_ptr &&
> +		    (arg->flags & DRM_MODE_ATOMIC_RESET) &&
> +		    !crtc_state->active) {
> +			struct drm_crtc_state *old_crtc_state;
> +
> +			old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> +			page_flip_event = old_crtc_state->active;
> +		}

[Severity: High]
As reported by Sashiko on v6 [1], what happens when a DRM lessee calls
drm_mode_atomic_ioctl() with DRM_MODE_ATOMIC_RESET while KMS objects are
bound across the lease boundary?

In drm_atomic_commit_fill_with_defaults(), when a leased plane or leased
connector is currently bound to an unleased CRTC, the unleased CRTC is pulled
into the commit with its current state duplicated:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		if (plane->state->crtc) {
			struct drm_crtc_state *crtc_state;

			crtc_state = drm_atomic_get_crtc_state(commit,
							       plane->state->crtc);
			if (IS_ERR(crtc_state))
				return PTR_ERR(crtc_state);
		}

		plane_state = plane->funcs->atomic_create_state(plane);
    ...
}

Because prepare_signaling() iterates over all CRTCs in state without checking
drm_lease_held(file_priv, crtc->base.id), any active unleased CRTC pulled into
the commit has crtc_state->active set to true and will have a page flip event
attached for the lessee's file_priv and increment c. Doesn't this leak vblank
events for an unleased CRTC to the lessee, allow a commit that leaves all of
the lessee's own CRTCs disabled to bypass the c == 0 check, and risk failing
with -EBUSY in stall_checks() if the lessor has a flip in flight on that CRTC?

In addition, when detaching a leased plane or connector from an unleased CRTC
via atomic_create_state(), drm_atomic_commit_fill_with_defaults() does not
clear drm_plane_mask(plane) from crtc_state->plane_mask or
drm_connector_mask(connector) from crtc_state->connector_mask on the
duplicated unleased CRTC state. Conversely, if an unleased connector or plane
is bound to a leased CRTC that gets reset, the leased CRTC's connector_mask
and plane_mask are zeroed while the unleased connector or plane state still
points to that CRTC:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		if (!drm_lease_held(file_priv, connector->base.id)) {
			if (!connector->state->crtc)
				continue;

			if (!drm_atomic_get_new_crtc_state(commit,
							   connector->state->crtc))
				continue;

			connector_state = drm_atomic_get_connector_state(commit,
									 connector);
    ...
}

Won't these cross-lease bindings corrupt the CRTC state masks or cause
drm_atomic_helper_check_modeset() and drm_atomic_helper_check_planes() to
reject the lessee's reset commit with -EINVAL?

[ ... ]

> @@ -1688,6 +1715,17 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	fence_state = NULL;
>  	num_fences = 0;
>  
> +	if (arg->flags & DRM_MODE_ATOMIC_RESET) {
> +		if (!drm_atomic_implements_create_state(dev)) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		ret = drm_atomic_commit_fill_with_defaults(file_priv, state);
> +		if (ret)
> +			goto out;
> +	}

[Severity: High]
When userspace issues a commit with DRM_MODE_ATOMIC_RESET that re-applies the
same mode and connector routing on an already-enabled CRTC, does
new_crtc_state->adjusted_mode get left all-zeroed?

In drm_atomic_commit_fill_with_defaults(), the fresh CRTC state is allocated
via crtc->funcs->atomic_create_state(crtc) with adjusted_mode zeroed, while
connector_state->best_encoder is preserved from connector->state->best_encoder
so that re-applying the current configuration avoids a modeset:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		crtc_state = crtc->funcs->atomic_create_state(crtc);
    ...
		connector_state->best_encoder = connector->state->best_encoder;
    ...
}

When userspace supplies the existing MODE_ID, ACTIVE = 1, and CRTC_ID
properties, new_crtc_state->mode and new_crtc_state->enable match
old_crtc_state, and update_connector_routing() sees new_encoder ==
new_connector_state->best_encoder, leaving both mode_changed and
connectors_changed false. In drm_atomic_helper_check_modeset() -> mode_fixup(),
copying mode into adjusted_mode is then skipped:

drivers/gpu/drm/drm_atomic_helper.c:mode_fixup() {
    ...
	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
		if (!new_crtc_state->mode_changed &&
		    !new_crtc_state->connectors_changed)
			continue;

		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
	}
    ...
}

With new_crtc_state->adjusted_mode remaining all zeros on an enabled CRTC,
won't subsequent checks and commit steps fail or crash—for example, dividing
by zero on mode->htotal in vc4_crtc_atomic_check(), rejecting valid plane
coordinates against adjusted_mode.hdisplay in omap_plane_atomic_check(), and
corrupting vblank timing constants in
drm_atomic_helper_calc_timestamping_constants()?

[1] https://lore.kernel.org/r/20261022-drm-reset-state-flag-v6-13-f36d640b7e76@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-drm-reset-state-flag-v7-0-4e55eaeae683@kernel.org?part=14