[PATCH v6 00/15] drm: Add DRM_MODE_ATOMIC_RESET flag

[PATCH v6 00/15] 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 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 (14):
      drm/amd/display: Fix planes atomic_create_state implementation
      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                       | 381 +++++++++++++++++----
 drivers/gpu/drm/drm_atomic_uapi.c                  |  18 +
 drivers/gpu/drm/drm_ioctl.c                        |   4 +
 drivers/gpu/drm/tests/drm_atomic_test.c            | 141 +++++++-
 drivers/gpu/drm/vkms/vkms_drv.h                    |  11 +-
 drivers/gpu/drm/vkms/vkms_plane.c                  |  61 +++-
 include/drm/drm_atomic.h                           |   4 +
 include/uapi/drm/drm.h                             |  10 +
 include/uapi/drm/drm_mode.h                        |  14 +-
 10 files changed, 572 insertions(+), 75 deletions(-)
---
base-commit: 489a8539ca6f347102c1334c2f08198abacc86c8
change-id: 20260708-drm-reset-state-flag-2fb2b5711f97

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

[PATCH v6 01/15] 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>
---
 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 v6 02/15] 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 v6 03/15] 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 v6 04/15] 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 v6 05/15] 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 v6 06/15] 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 v6 07/15] 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 v6 08/15] 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 v6 09/15] 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 v6 10/15] 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 v6 11/15] 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.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7a77051096c9..c3c38695814f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1650,4 +1650,146 @@ bool drm_atomic_implements_create_state(struct drm_device *dev)
 EXPORT_SYMBOL(drm_atomic_implements_create_state);
 
+/**
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+	 */
+	drm_for_each_colorop(colorop, dev) {
+		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);
+	}
+
+	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;
+
+		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_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_connector_list_iter_begin(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		struct drm_connector_state *connector_state;
+
+		if (!drm_lease_held(file_priv, connector->base.id))
+			continue;
+
+		ret = drm_modeset_lock(&config->connection_mutex, commit->acquire_ctx);
+		if (ret) {
+			drm_connector_list_iter_end(&conn_iter);
+			return ret;
+		}
+
+		/*
+		 * 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;
+		}
+
+		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;
+		}
+	}
+	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

Re: [PATCH v6 11/15] 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()
- [Medium] drm/atomic: missing color_mgmt_changed in drm_atomic_commit_fill_with_defaults()
--

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

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

This commit introduces drm_atomic_commit_fill_with_defaults() to populate an
atomic commit with freshly created default states for CRTCs, planes,
connectors, and color operations using their atomic_create_state() hooks.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7a77051096c91..c3c38695814fc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1649,6 +1649,148 @@ bool drm_atomic_implements_create_state(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_atomic_implements_create_state);
>  
[ ... ]
> +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.
> +	 */
> +	drm_for_each_colorop(colorop, dev) {
> +		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);
> +	}
> +
> +	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;
> +
> +		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_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);
> +	}

[Severity: Medium]
When drm_atomic_commit_fill_with_defaults() replaces the colorop, plane, and
CRTC states with newly created default states, both
crtc_state->color_mgmt_changed and plane_state->color_mgmt_changed are
initialized to false, with crtc_state->{degamma_lut,ctm,gamma_lut} and
plane_state->color_pipeline set to NULL and colorop_state set to defaults.

In drivers/gpu/drm/drm_atomic_uapi.c, drm_atomic_crtc_set_property(),
drm_atomic_plane_set_property(), drm_atomic_set_colorop_for_plane(), and
drm_atomic_colorop_set_property() only set color_mgmt_changed when a property
write changes the value already stored in the new state:

drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_crtc_set_property() {
	...
	ret = drm_property_replace_blob_from_id(dev,
				&state->degamma_lut,
				val,
				elem_size * lut_size, -1, elem_size,
				&replaced);
	state->color_mgmt_changed |= replaced;
	...
}

drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_set_colorop_for_plane() {
	...
	/* Color pipeline didn't change */
	if (plane_state->color_pipeline == colorop)
		return false;
	...
}

Neither drm_atomic_check_only() nor drm_atomic_helper_check_modeset() and
drm_atomic_helper_check_planes() compares old_state against new_state to set
color_mgmt_changed.

If crtc->state previously had a non-NULL degamma_lut, ctm, or gamma_lut (or
plane->state and colorop->state had an active color pipeline or non-default
colorop settings), and a reset commit either omits those properties to leave
them at defaults or explicitly sets them to 0 / default (which matches the
already-pristine new_state), won't color_mgmt_changed remain false on both
crtc_state and plane_state?

When a reset commit keeps the same mode and routing on an active CRTC so
drm_atomic_crtc_needs_modeset() is false, drivers that gate color updates on
color_mgmt_changed || drm_atomic_crtc_needs_modeset() (such as amdgpu_dm,
i915, msm/dpu1, tidss, and rockchip vop2) will skip reprogramming hardware
LUTs, CTMs, and colorops. In addition, drivers that gate hardware LUT or CTM
updates strictly on crtc_state->color_mgmt_changed (such as
mtk_crtc_atomic_flush(), vc4_hvs_atomic_flush(), omap_crtc_atomic_flush(),
malidp_atomic_commit_update_gamma(), malidp_atomic_commit_update_coloradj(),
d71_improc_update(), and armada_drm_crtc_atomic_begin()) will skip resetting
hardware color management even across a full modeset.

Should drm_atomic_commit_fill_with_defaults() compare the pristine states
against crtc->state, plane->state, and colorop->state and set
color_mgmt_changed accordingly?

> +
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *connector_state;
> +
> +		if (!drm_lease_held(file_priv, connector->base.id))
> +			continue;
> +
> +		ret = drm_modeset_lock(&config->connection_mutex, commit->acquire_ctx);
> +		if (ret) {
> +			drm_connector_list_iter_end(&conn_iter);
> +			return ret;
> +		}
> +
> +		/*
> +		 * 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;
> +		}
> +
> +		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;
> +		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);

[Severity: High]
Can this per-object lease filtering cause NULL pointer dereferences and broken
atomic state when a lease is used or revoked?

First, drm_atomic_commit_fill_with_defaults() calls drm_lease_held() in four
separate loops (for colorops, planes, CRTCs, and connectors), taking and
releasing dev->mode_config.idr_mutex on each call. Concurrent lease revocation
via drm_mode_revoke_lease_ioctl() or drm_lease_revoke() only acquires
dev->mode_config.idr_mutex (without any drm_modeset_lock) and clears
master->leases. If a lease is revoked after the plane loop adds a leased plane
(with old_plane_state->crtc != NULL and pristine new_plane_state->crtc ==
NULL) but before the CRTC loop checks old_plane_state->crtc, that CRTC is
skipped and never added to the commit. During drm_atomic_commit(),
drm_atomic_helper_plane_changed() hits WARN_ON(!crtc_state),
drm_atomic_plane_check() and drm_atomic_helper_check_plane_state() succeed
because new_plane_state->fb is NULL, and drm_atomic_helper_setup_commit() ->
crtc_or_fake_commit() dereferences a NULL new_crtc_state:

drivers/gpu/drm/drm_atomic_helper.c:crtc_or_fake_commit() {
	if (crtc) {
		struct drm_crtc_state *new_crtc_state;

		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);

		return new_crtc_state->commit;
	}
	...
}

Second, drm_mode_create_lease_ioctl() -> fill_object_idr() -> validate_lease()
does not verify that leased planes or connectors are not currently bound to an
unleased CRTC, nor does drm_lease_held() prevent the lessor (where
master->lessor == NULL) from configuring cross-lease routing. Unlike
drm_atomic_get_plane_state() and drm_atomic_get_connector_state(), which pull
old_state->crtc into the commit via drm_atomic_get_crtc_state(),
drm_atomic_commit_fill_with_defaults() resets leased planes and connectors to
crtc = NULL while skipping unleased CRTCs. If old_connector_state->crtc is not
in the commit, update_connector_routing() dereferences a NULL crtc_state:

drivers/gpu/drm/drm_atomic_helper.c:update_connector_routing() {
	...
	if (old_connector_state->crtc != new_connector_state->crtc) {
		if (old_connector_state->crtc) {
			crtc_state = drm_atomic_get_new_crtc_state(state, old_connector_state->crtc);
			crtc_state->connectors_changed = true;
		}
	...
}

Similarly, if old_plane_state->crtc is not in the commit,
crtc_or_fake_commit() dereferences a NULL new_crtc_state.

Third, when universal_planes is enabled, a lease on a CRTC does not have to
include every plane (such as overlay or cursor planes) or cloned connector
currently active on that CRTC. Resetting the leased CRTC to a pristine state
(enable = false, plane_mask = 0, connector_mask = 0) while skipping the
unleased plane or connector causes drm_atomic_helper_check_modeset() ->
drm_atomic_add_affected_planes() to duplicate the unleased plane's active
state (with crtc = leased_crtc and fb != NULL). That causes
drm_atomic_helper_check_plane_state() to fail with -EINVAL if the CRTC remains
disabled, or leaves new_crtc_state->plane_mask missing the unleased plane if
the CRTC is re-enabled in the same commit. Meanwhile,
drm_atomic_add_affected_connectors() inspects new_crtc_state->connector_mask
(which is 0), failing to pull in the unleased connector and leaving
connector->state->crtc and best_encoder dangling on the reset CRTC.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_commit_fill_with_defaults);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-reset-state-flag-v6-0-f36d640b7e76@kernel.org?part=11

[PATCH v6 12/15] 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 | 141 +++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_atomic_test.c b/drivers/gpu/drm/tests/drm_atomic_test.c
index 0aca5c1bbeae..a8e21da811aa 100644
--- a/drivers/gpu/drm/tests/drm_atomic_test.c
+++ b/drivers/gpu/drm/tests/drm_atomic_test.c
@@ -143,5 +143,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 +148,143 @@ 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);
+
+	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
+	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);
+
+	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	state->allow_modeset = true;
+
+	ret = drm_atomic_commit(state);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	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),
+	{ }
+};
+
+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

Re: [PATCH v6 12/15] 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
- [Medium] drm/tests: unhandled -EDEADLK in drm_atomic_commit_fill_with_defaults() tests
--

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

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

This commit adds two KUnit tests for drm_atomic_commit_fill_with_defaults()
to verify resetting an enabled CRTC, plane, and connector back to their
disabled state and committing the default-filled state.

> diff --git a/drivers/gpu/drm/tests/drm_atomic_test.c b/drivers/gpu/drm/tests/drm_atomic_test.c
> index ea91bec6569e6..52db445b4eadb 100644
> --- a/drivers/gpu/drm/tests/drm_atomic_test.c
> +++ b/drivers/gpu/drm/tests/drm_atomic_test.c
> @@ -140,13 +140,150 @@ 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);
> +
> +	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
Will drm_atomic_commit_fill_with_defaults() always return -EOPNOTSUPP here
and in drm_test_check_fill_with_defaults_commit() below?

create_device() initializes priv->connector with
drm_atomic_init_connector_funcs in 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 not set on drm_atomic_init_connector_funcs,
drm_atomic_commit_fill_with_defaults() in drivers/gpu/drm/drm_atomic.c locks
the plane, CRTC, and connection_mutex into state->acquire_ctx, and then hits:

    if (!connector->funcs->atomic_create_state) {
            drm_connector_list_iter_end(&conn_iter);
            return -EOPNOTSUPP;
    }

KUNIT_ASSERT_EQ(test, ret, 0) then aborts the test before
drm_modeset_drop_locks(&ctx) and drm_modeset_acquire_fini(&ctx) can run,
leaving those modeset locks held on the stack-allocated ctx when drmm
teardown runs drm_modeset_lock_fini().

Should drm_atomic_init_connector_funcs wire up .atomic_create_state to
drm_atomic_helper_connector_create_state?

> +
> +	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)
> +{
[ ... ]
> +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);
> +
> +	ret = drm_atomic_commit_fill_with_defaults(NULL, state);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	state->allow_modeset = true;
> +
> +	ret = drm_atomic_commit(state);
> +	KUNIT_EXPECT_EQ(test, ret, 0);

[Severity: Medium]
Can drm_atomic_commit_fill_with_defaults() (both here and in
drm_test_check_fill_with_defaults()) and drm_atomic_commit() fail with
-EDEADLK when CONFIG_DEBUG_WW_MUTEX_SLOWPATH is enabled?

drm_kunit_helper_enable_crtc_connector() only locks
drm->mode_config.connection_mutex and priv->crtc->mutex on ctx, leaving
priv->plane->mutex unlocked. When drm_atomic_commit_fill_with_defaults() is
subsequently called with the same ctx, it acquires priv->plane->mutex for the
first time:

    drm_for_each_plane(plane, dev) {
            ...
            ret = drm_modeset_lock(&plane->mutex, commit->acquire_ctx);
            if (ret)
                    return ret;

Under CONFIG_DEBUG_WW_MUTEX_SLOWPATH, acquiring a new ww_mutex on an acquire
context that already holds locks injects -EDEADLK when the context's
deadlock_inject_countdown reaches zero. Without a retry loop calling
drm_atomic_commit_clear(state) and drm_modeset_backoff(&ctx), will
KUNIT_ASSERT_EQ(test, ret, 0) fail spuriously and abort with ctx.contended
and held locks still active?

> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-reset-state-flag-v6-0-f36d640b7e76@kernel.org?part=12

[PATCH v6 13/15] 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.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Sebastian Wick <sebastian.wick@redhat.com>
Co-developed-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic.c      |  3 +++
 drivers/gpu/drm/drm_atomic_uapi.c | 18 ++++++++++++++++++
 drivers/gpu/drm/drm_ioctl.c       |  3 +++
 include/uapi/drm/drm.h            | 10 ++++++++++
 include/uapi/drm/drm_mode.h       | 14 +++++++++++++-
 5 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c3c38695814f..e7867da2d921 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..9ff1ddecb8ed 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1674,4 +1674,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 +1696,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..e589d93871e2 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -306,4 +306,7 @@ 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_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..32eb3f650d7a 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1329,4 +1329,15 @@ 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.).
+ *
+ * This flag cannot be combined with &DRM_MODE_PAGE_FLIP_ASYNC.
+ */
+#define DRM_MODE_ATOMIC_RESET 0x0800
 
 /**
@@ -1341,5 +1352,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

Re: [PATCH v6 13/15] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/atomic: DRM_MODE_ATOMIC_RESET with page-flip event fails on inactive CRTC
- [High] drm/atomic: NULL crtc_state dereference on DRM_MODE_ATOMIC_RESET in DRM lease
- [Medium] drm/ioctl: drm_getcap() reports DRM_CAP_ATOMIC_RESET on non-atomic drivers
- [Medium] drm/atomic: DRM_MODE_ATOMIC_RESET clears best_encoder and forces full modeset
- [Medium] drm/atomic: DRM_MODE_ATOMIC_RESET fails to set color_mgmt_changed on reset
--

commit 421841f24ef240ac1eae965a30846086114bc3d7
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 for atomic commits and
the DRM_CAP_ATOMIC_RESET capability so userspace can reset all KMS object
states to their defaults before applying the properties in the request.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1eebcf6f5e05b..9ff1ddecb8ede 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c

[ ... ]

> @@ -1688,6 +1695,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]
What happens when userspace combines DRM_MODE_ATOMIC_RESET with
DRM_MODE_PAGE_FLIP_EVENT on a device where at least one crtc is currently off
and remains off after the commit?

In drivers/gpu/drm/drm_atomic_uapi.c, drm_mode_atomic_ioctl() calls
drm_atomic_commit_fill_with_defaults(file_priv, state), which populates
state->crtcs with a default drm_crtc_state (where enable and active are
false) for every crtc on the device or lease. Any crtc not explicitly enabled
by properties in the request remains in state->crtcs with
new_crtc_state->active set to false.

Later in drm_mode_atomic_ioctl(), prepare_signaling() iterates over every
crtc in state and attaches a vblank event whenever DRM_MODE_PAGE_FLIP_EVENT
is set:

drivers/gpu/drm/drm_atomic_uapi.c:prepare_signaling() {
    ...
	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
		s32 __user *fence_ptr;

		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);

		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
			struct drm_pending_vblank_event *e;

			e = create_vblank_event(crtc, arg->user_data);
			if (!e)
				return -ENOMEM;

			crtc_state->event = e;
		}
    ...
}

When drm_atomic_commit() or drm_atomic_nonblocking_commit() subsequently runs
drm_atomic_check_only() -> drm_atomic_crtc_check() in
drivers/gpu/drm/drm_atomic.c, any crtc that was off and remains off causes the
entire commit to fail with -EINVAL:

drivers/gpu/drm/drm_atomic.c:drm_atomic_crtc_check() {
    ...
	if (new_crtc_state->event &&
	    !new_crtc_state->active && !old_crtc_state->active) {
		drm_dbg_atomic(crtc->dev,
			       "[CRTC:%d:%s] requesting event but off\n",
			       crtc->base.id, crtc->name);
		return -EINVAL;
	}
    ...
}

This also means a preceding DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_RESET
commit succeeds while the subsequent commit with DRM_MODE_PAGE_FLIP_EVENT
fails. Should prepare_signaling() avoid attaching vblank events to inactive
crtcs that were only added by drm_atomic_commit_fill_with_defaults()?

[Severity: High]
Can calling drm_atomic_commit_fill_with_defaults() from
drm_mode_atomic_ioctl() trigger a NULL crtc_state dereference in
update_connector_routing() when used by a drm lessee?

In drivers/gpu/drm/drm_atomic.c, drm_atomic_commit_fill_with_defaults()
filters crtcs, planes, and connectors independently via drm_lease_held():

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
	drm_for_each_crtc(crtc, dev) {
		struct drm_crtc_state *crtc_state;

		if (!drm_lease_held(file_priv, crtc->base.id))
			continue;
    ...
	drm_for_each_connector_iter(connector, &conn_iter) {
		struct drm_connector_state *connector_state;

		if (!drm_lease_held(file_priv, connector->base.id))
			continue;
    ...
}

Unlike drm_atomic_get_connector_state(), which pulls the connector's
previously bound crtc into the commit via drm_atomic_get_crtc_state(),
drm_atomic_commit_fill_with_defaults() resets a leased connector to
new_connector_state->crtc = NULL while skipping old_connector_state->crtc if
that crtc is not held by the lease.

When drm_atomic_check_only() -> drm_atomic_helper_check_modeset() ->
update_connector_routing() runs in drivers/gpu/drm/drm_atomic_helper.c,
drm_atomic_get_new_crtc_state(state, old_connector_state->crtc) returns NULL
and is immediately dereferenced:

drivers/gpu/drm/drm_atomic_helper.c:update_connector_routing() {
    ...
	if (old_connector_state->crtc != new_connector_state->crtc) {
		if (old_connector_state->crtc) {
			crtc_state = drm_atomic_get_new_crtc_state(state, old_connector_state->crtc);
			crtc_state->connectors_changed = true;
		}
    ...
}

Conversely, if a leased crtc currently has an unleased plane or connector
bound to it, resetting the leased crtc zeroes its plane_mask, connector_mask,
and encoder_mask without detaching the unleased object.

[Severity: Medium]
Will resetting connector state via atomic_create_state() in
drm_atomic_commit_fill_with_defaults() force a full modeset even when an
active connector remains bound to the same crtc and mode?

In drivers/gpu/drm/drm_atomic.c, drm_atomic_commit_fill_with_defaults()
creates each connector state via connector->funcs->atomic_create_state(),
which leaves new_connector_state->best_encoder as NULL (whereas
atomic_duplicate_state() preserves old_connector_state->best_encoder):

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		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;
		}
    ...
}

When userspace re-applies the same crtc to the connector in the reset commit,
new_connector_state->best_encoder is still NULL when
drm_atomic_helper_check_modeset() -> update_connector_routing() runs in
drivers/gpu/drm/drm_atomic_helper.c:

drivers/gpu/drm/drm_atomic_helper.c:update_connector_routing() {
    ...
	if (new_encoder == new_connector_state->best_encoder) {
		set_best_encoder(state, new_connector_state, new_encoder);
        ...
		return 0;
	}

	steal_encoder(state, new_encoder);

	set_best_encoder(state, new_connector_state, new_encoder);

	crtc_state->connectors_changed = true;
    ...
}

Because update_connector_routing() compares new_encoder against
new_connector_state->best_encoder rather than
old_connector_state->best_encoder, the check always fails and sets
crtc_state->connectors_changed = true. Does this cause
drm_atomic_crtc_needs_modeset() to return true unconditionally on active
crtcs, rejecting reset commits that omit DRM_MODE_ATOMIC_ALLOW_MODESET with
-EINVAL?

[Severity: Medium]
When DRM_MODE_ATOMIC_RESET clears previously active crtc color management
blobs (degamma_lut, ctm, gamma_lut) or a plane color_pipeline, how does
color_mgmt_changed get set on the new state?

In drivers/gpu/drm/drm_atomic.c, drm_atomic_commit_fill_with_defaults()
allocates fresh plane and crtc states via atomic_create_state(), which
initializes degamma_lut, ctm, gamma_lut, and color_pipeline to NULL and
color_mgmt_changed to false:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		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);
    ...
		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);
    ...
}

In drivers/gpu/drm/drm_atomic_uapi.c, drm_atomic_crtc_set_property() and
drm_atomic_plane_set_property() only update color_mgmt_changed when a property
change modifies the in-progress state:

drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_crtc_set_property() {
    ...
		ret = drm_property_replace_blob_from_id(dev,
					&state->degamma_lut,
					val,
					elem_size * lut_size, -1, elem_size,
					&replaced);
		state->color_mgmt_changed |= replaced;
		return ret;
    ...
}

drivers/gpu/drm/drm_atomic_uapi.c:drm_atomic_plane_set_property() {
    ...
		state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
    ...
}

If an active crtc or plane had a non-NULL lut, ctm, or color_pipeline in
old_crtc_state or old_plane_state and the reset commit leaves those properties
at their default NULL values (or explicitly writes 0), color_mgmt_changed
remains false. Won't drivers that gate hardware lut, ctm, or color pipeline
updates on color_mgmt_changed skip reprogramming the hardware and leave stale
color transforms active?

>  	for (i = 0; i < arg->count_objs; i++) {
>  		uint32_t obj_id, count_props;
>  		struct drm_mode_object *obj;
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 0dbf04d4aa9e0..e589d93871e27 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -305,6 +305,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  		req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) &&
>  			     dev->mode_config.async_page_flip;
>  		break;
> +	case DRM_CAP_ATOMIC_RESET:
> +		req->value = drm_atomic_implements_create_state(dev);
> +		break;
>  	default:
>  		return -EINVAL;
>  	}

[Severity: Medium]
Should DRM_CAP_ATOMIC_RESET in drm_getcap() also check
drm_core_check_feature(dev, DRIVER_ATOMIC) like DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP
does right above it?

In drivers/gpu/drm/drm_ioctl.c, this switch in drm_getcap() only checks
drm_core_check_feature(dev, DRIVER_MODESET), and
drm_atomic_implements_create_state() in drivers/gpu/drm/drm_atomic.c only
checks whether objects in plane_list, crtc_list, and connector_list have
atomic_create_state hooks. On KMS drivers where DRIVER_ATOMIC is not set (for
example, when a driver clears DRIVER_ATOMIC dynamically or when the mode
object lists are empty), drm_getcap() will report DRM_CAP_ATOMIC_RESET = 1
even though DRM_IOCTL_MODE_ATOMIC fails with -EOPNOTSUPP.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-reset-state-flag-v6-0-f36d640b7e76@kernel.org?part=13

[PATCH v6 14/15] 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 v6 15/15] 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 | 61 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 62 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..0ebc131c2788 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,13 @@ 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)
+		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 v6 15/15] drm/vkms: Add driver-specific plane property for testing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/vkms: NULL dereference in vkms_plane_create_state() if property alloc fails
- [Medium] drm/vkms: missing connector atomic_create_state breaks DRM_MODE_ATOMIC_RESET
--

commit f0d606e22df5b4b30135537f8737279247724656
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 so userspace tests can verify that
DRM_MODE_ATOMIC_RESET resets plane state properties to their default values.

> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index fd3d34dcf0fe5..0ebc131c2788f 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: High]
Can this trigger a NULL pointer dereference in
drm_object_property_get_default_value() if drm_property_create_range()
fails in vkms_plane_init()?

In drivers/gpu/drm/vkms/vkms_plane.c, vkms_plane_init() treats a
drm_property_create_range() failure as non-fatal and leaves
plane->vkms_test_do_not_use_property as NULL:

	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)
		drm_object_attach_property(&plane->base.base,
					   plane->vkms_test_do_not_use_property,
					   0);

When vkms_plane_create_state() is subsequently called during device
initialization via vkms_output_init() -> drm_mode_config_reset() (or
during a DRM_MODE_ATOMIC_RESET commit via
drm_atomic_commit_fill_with_defaults()), it unconditionally passes
vkms_plane->vkms_test_do_not_use_property to
drm_object_property_get_default_value().

However, in drivers/gpu/drm/drm_mode_object.c,
drm_object_property_get_default_value() immediately dereferences
property->dev without checking whether property is NULL:

int drm_object_property_get_default_value(struct drm_mode_object *obj,
					  struct drm_property *property,
					  uint64_t *val)
{
	WARN_ON(!drm_drv_uses_atomic_modeset(property->dev));

	return __drm_object_property_get_prop_value(obj, property, val);
}

Should vkms_plane_create_state() check that
vkms_plane->vkms_test_do_not_use_property is non-NULL before calling
drm_object_property_get_default_value(), or should vkms_plane_init()
fail when drm_property_create_range() returns NULL?

[ ... ]

> @@ -283,6 +331,15 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  					  DRM_COLOR_YCBCR_BT601,
>  					  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)
> +		drm_object_attach_property(&plane->base.base,
> +					   plane->vkms_test_do_not_use_property,
> +					   0);

[Severity: Medium]
Will userspace be able to use DRM_MODE_ATOMIC_RESET to test resetting
this property on vkms 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() in drivers/gpu/drm/drm_atomic.c
require every connector on the device to provide an atomic_create_state
callback:

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

However, vkms_connector_funcs in drivers/gpu/drm/vkms/vkms_connector.c
and vkms_wb_connector_funcs in drivers/gpu/drm/vkms/vkms_writeback.c
still define .reset = drm_atomic_helper_connector_reset without setting
.atomic_create_state:

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

Because every valid vkms device registers at least one connector,
drm_atomic_implements_create_state() always returns false on vkms, so
DRM_CAP_ATOMIC_RESET reports 0 and DRM_MODE_ATOMIC_RESET commits fail
with -EOPNOTSUPP before vkms_plane_create_state() can be reached.

Do vkms_connector_funcs and vkms_wb_connector_funcs also need to use
.atomic_create_state = drm_atomic_helper_connector_create_state so that
DRM_MODE_ATOMIC_RESET is supported on vkms?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-drm-reset-state-flag-v6-0-f36d640b7e76@kernel.org?part=15