[PATCH v6 0/5] User readable error codes on atomic_ioctl failure

[PATCH v6 0/5] User readable error codes on atomic_ioctl failure

[Arun R Murthy]

The series focuses on providing a user readable error value on a failure
in drm_atomic_ioctl(). Usually -EINVAL is returned in most of the error
cases and it is difficult for the user to decode the error and get to
know the real cause for the error. If user gets to know the reason for
the error then corrective measurements can be taken up.

TODO: driver specific error codes are to be added and will be done in
the follow-up patches.

The IGT related changes are pushed for review @
https://patchwork.freedesktop.org/series/153330/

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
Arun R Murthy (5):
      drm: Define user readable error codes for atomic ioctl
      drm/atomic: Add error_code element in atomic_state
      drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl
      drm/atomic: Return user readable error in atomic_ioctl
      drm/i915/display: Error codes for async flip failures

 drivers/gpu/drm/drm_atomic.c                 | 31 ++++++++++
 drivers/gpu/drm/drm_atomic_uapi.c            | 92 +++++++++++++++++++---------
 drivers/gpu/drm/i915/display/intel_display.c | 25 ++++----
 include/drm/drm_atomic.h                     | 10 +++
 include/uapi/drm/drm_mode.h                  | 41 +++++++++++++
 5 files changed, 159 insertions(+), 40 deletions(-)
---
base-commit: a7101e35f29e03562f24ce1220f08350260fc0fc
change-id: 20250728-atomic-c9713fd357e4

Best regards,
-- 
Arun R Murthy <arun.r.murthy@intel.com>

[PATCH v6 1/5] drm: Define user readable error codes for atomic ioctl

[Arun R Murthy]

There can be multiple reasons for a failure in atomic_ioctl. Most often
in these error conditions -EINVAL is returned. User/Compositor would
have to blindly take a call on failure of this ioctl so as to use
ALLOW_MODESET or any. It would be good if user/compositor gets a
readable error code on failure so they can take proper corrections in
the next commit.
The struct drm_mode_atomic is being passed by the user/compositor which
holds the properties for modeset/flip. Reusing the same struct for
returning the error code in case of failure can save by creating a new
uapi/interface for returning the error code.
The element 'reserved' in the struct drm_mode_atomic is used for
returning the user readable error code. This points to the struct
drm_mode_atomic_err_code. Failure reasons have been initialized in
DRM_MODE_ATOMIC_FAILURE_REASON.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 include/uapi/drm/drm_mode.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 1e0e02a79b5c8db200cf9fd35edfe163d701cbd5..1e9eeae472e74bbd1b5e0b6f79f9782cafaf5b6e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -45,6 +45,7 @@ extern "C" {
 #define DRM_CONNECTOR_NAME_LEN	32
 #define DRM_DISPLAY_MODE_LEN	32
 #define DRM_PROP_NAME_LEN	32
+#define DRM_MODE_ATOMIC_FAILURE_STRING_LEN	128
 
 #define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
 #define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
@@ -1205,6 +1206,46 @@ struct drm_mode_destroy_dumb {
 		DRM_MODE_ATOMIC_NONBLOCK |\
 		DRM_MODE_ATOMIC_ALLOW_MODESET)
 
+/**
+ * enum drm_mode_atomic_err_code -  error codes for failures in atomic_ioctl
+ * @DRM_MODE_ATOMIC_INVALID_API_USAGE: invallid API usage(DRM_ATOMIC not
+ *				       enabled, invalid falg, page_flip event
+ *				       with test-only, etc)
+ * @DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET: Need full modeset on this crtc
+ * @DRM_MODE_ATOMIC_NEED_FULL_MODESET: Need full modeset on all connected crtc's
+ * @DRM_MODE_ATOMIC_ASYN_NOTSUPP_PLANE: Aync flip not supported on this plane
+ * DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP: Modifier not supported by async flip
+ * @DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED: Property changed in async flip
+ */
+enum drm_mode_atomic_failure_codes {
+	DRM_MODE_ATOMIC_INVALID_API_USAGE,
+	DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+	DRM_MODE_ATOMIC_NEED_FULL_MODESET,
+	DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
+	DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP,
+	DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
+};
+
+/**
+ * drm_mode_atomic_err_code - struct to store the error code
+ *
+ * pointer to this struct will be stored in reserved variable of
+ * struct drm_mode_atomic to report the failure cause to the user.
+ *
+ * @failure_code: error codes defined in enum drm_moide_atomic_failure_code
+ * @failure_string_ptr: pointer to user readable error message string
+ * @failure_obj_ptr: pointer to the drm_object that caused error
+ * @reserved: reserved for future use
+ * @count_objs: count of drm_objects if multiple drm_objects caused error
+ */
+struct drm_mode_atomic_err_code {
+	__u64 failure_code;
+	__u64 failure_objs_ptr;
+	__u64 reserved;
+	__u32 count_objs;
+	char failure_string[DRM_MODE_ATOMIC_FAILURE_STRING_LEN];
+};
+
 struct drm_mode_atomic {
 	__u32 flags;
 	__u32 count_objs;

-- 
2.25.1

[PATCH v6 2/5] drm/atomic: Add error_code element in atomic_state

[Arun R Murthy]

Now that a proper error code will be returned to the user on any failure
in atomic_ioctl() via struct drm_mode_atomic, add a new element
error_code in the struct drm_atomic_state so as to hold the error code
during the atomic_check() and atomic_commit() phases.
New function added to print the error message and fill the struct
err_code with proper error message and error code.

v5: Add a function for printing the error message and filling err_code
    struct
v6: Replace drm_err with drm_dbg_atomic print

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 31 +++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h     | 10 ++++++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 0fda567c390057b10bce691d9ddc11308088d92e..52eacb5a109d1c0f5b017e552d9f5169f8d8fea5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1897,6 +1897,37 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p)
 }
 EXPORT_SYMBOL(drm_state_dump);
 
+/**
+ * drm_mode_atomic_add_error_msg - function to add error code and error string
+ *
+ * @err_code: pointer to struct drm_mode_atomic_err_code that stores the failure
+ *	      reason
+ * @failure_code: failure code in enum drm_mode_atomic_failure_codes
+ * @failure_string: failure reason string message
+ *
+ * Returns: void
+ */
+void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
+				   __u64 failure_code, const char *format, ...)
+{
+	struct drm_atomic_state *state = container_of(err_code,
+						      struct drm_atomic_state,
+						      error_code);
+	va_list varg;
+	char *failure_string;
+
+	err_code->failure_code = failure_code;
+
+	va_start(varg, format);
+	failure_string = kvasprintf(GFP_ATOMIC, format, varg);
+
+	drm_dbg_atomic(state->dev, "%s\n", failure_string);
+	strscpy_pad(err_code->failure_string, failure_string,
+		    sizeof(err_code->failure_string));
+	va_end(varg);
+}
+EXPORT_SYMBOL(drm_mode_atomic_add_error_msg);
+
 #ifdef CONFIG_DEBUG_FS
 static int drm_state_info(struct seq_file *m, void *data)
 {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index c8ab2163bf658cd06b12a8dabada7c088a328654..205ce418da22f8cbe10ea353c62471dbb41ae2e8 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -589,6 +589,13 @@ struct drm_atomic_state {
 	 * commit without blocking.
 	 */
 	struct work_struct commit_work;
+
+	/* @error_code: pointer to struct holding failure reason and string
+	 *
+	 * struct to convey user readable error to the user.
+	 * Error codes defined in enum drm_mode_atomic_failure_flags
+	 */
+	struct drm_mode_atomic_err_code error_code;
 };
 
 void __drm_crtc_commit_free(struct kref *kref);
@@ -1259,5 +1266,8 @@ drm_atomic_get_old_bridge_state(const struct drm_atomic_state *state,
 struct drm_bridge_state *
 drm_atomic_get_new_bridge_state(const struct drm_atomic_state *state,
 				struct drm_bridge *bridge);
+__printf(3, 4)
+void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
+				   __u64 failure_code, const char *format, ...);
 
 #endif /* DRM_ATOMIC_H_ */

-- 
2.25.1

[PATCH v6 3/5] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl

[Arun R Murthy]

Moving atomic_state allocation to the beginning of the atomci_ioctl
to accommodate drm_mode_atomic_err_code usage for returning error
code on failures.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 85dbdaa4a2e25878c953b9b41539c8566d55c6d9..3ef478e717bec917d1b8803c72bbcc8d6409d745 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1394,13 +1394,21 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_out_fence_state *fence_state;
 	int ret = 0;
-	unsigned int i, j, num_fences;
+	unsigned int i, j, num_fences = 0;
 	bool async_flip = false;
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
 		return -EOPNOTSUPP;
 
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return -ENOMEM;
+
+	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
+	state->acquire_ctx = &ctx;
+	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+
 	/* disallow for userspace that has not enabled atomic cap (even
 	 * though this may be a bit overkill, since legacy userspace
 	 * wouldn't know how to call this ioctl)
@@ -1439,14 +1447,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
-
-	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
-	state->acquire_ctx = &ctx;
-	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
-
 retry:
 	copied_objs = 0;
 	copied_props = 0;
@@ -1543,7 +1543,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	}
 
 out:
-	complete_signaling(dev, state, fence_state, num_fences, !ret);
+	if (num_fences)
+		complete_signaling(dev, state, fence_state, num_fences, !ret);
 
 	if (ret == -EDEADLK) {
 		drm_atomic_state_clear(state);

-- 
2.25.1

[PATCH v6 4/5] drm/atomic: Return user readable error in atomic_ioctl

[Arun R Murthy]

Add user readable error codes for failure cases in drm_atomic_ioctl() so
that user can decode the error code and take corrective measurements.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 71 ++++++++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 3ef478e717bec917d1b8803c72bbcc8d6409d745..3d4e9709e8e289cf302413e171b1336812d65c1c 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1036,6 +1036,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 			ret = drm_atomic_connector_get_property(connector, connector_state,
 								prop, &old_val);
 			ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+			if (ret) {
+				drm_mode_atomic_add_error_msg(&state->error_code,
+							      DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
+							      "property change not allowed in async flip");
+			}
 			break;
 		}
 
@@ -1058,6 +1063,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 			ret = drm_atomic_crtc_get_property(crtc, crtc_state,
 							   prop, &old_val);
 			ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+			if (ret) {
+				drm_mode_atomic_add_error_msg(&state->error_code,
+							      DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
+							      "property change not allowed in async flip");
+			}
 			break;
 		}
 
@@ -1096,9 +1106,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 					ret = plane_funcs->atomic_async_check(plane, state, true);
 
 				if (ret) {
-					drm_dbg_atomic(prop->dev,
-						       "[PLANE:%d:%s] does not support async flips\n",
-						       obj->id, plane->name);
+					drm_mode_atomic_add_error_msg(&state->error_code,
+								      DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
+								      "[PLANE:%d:%s] does not support async flip",
+								      obj->id, plane->name);
 					break;
 				}
 			}
@@ -1393,6 +1404,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_atomic_state *state;
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_out_fence_state *fence_state;
+	struct drm_mode_atomic_err_code __user *error_code_ptr;
 	int ret = 0;
 	unsigned int i, j, num_fences = 0;
 	bool async_flip = false;
@@ -1401,6 +1413,14 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
 		return -EOPNOTSUPP;
 
+	if (!arg->reserved)
+		drm_dbg_atomic(dev,
+			       "memory not allocated for drm_atomic error reporting\n");
+	else
+		/* update the error code if any error to allow user handling it */
+		error_code_ptr = (struct drm_mode_atomic_err_code __user *)
+				 (unsigned long)arg->reserved;
+
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
 		return -ENOMEM;
@@ -1409,31 +1429,35 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	state->acquire_ctx = &ctx;
 	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
 
+	memset(&state->error_code, 0, sizeof(struct drm_mode_atomic_err_code));
+
 	/* disallow for userspace that has not enabled atomic cap (even
 	 * though this may be a bit overkill, since legacy userspace
 	 * wouldn't know how to call this ioctl)
 	 */
 	if (!file_priv->atomic) {
-		drm_dbg_atomic(dev,
-			       "commit failed: atomic cap not enabled\n");
-		return -EINVAL;
+		drm_mode_atomic_add_error_msg(&state->error_code,
+					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+					      "DRM_ATOMIC capability not enabled");
+		ret = -EINVAL;
+		goto out;
 	}
 
 	if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
-		drm_dbg_atomic(dev, "commit failed: invalid flag\n");
-		return -EINVAL;
-	}
-
-	if (arg->reserved) {
-		drm_dbg_atomic(dev, "commit failed: reserved field set\n");
-		return -EINVAL;
+		drm_mode_atomic_add_error_msg(&state->error_code,
+					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+					      "invalid flag");
+		ret = -EINVAL;
+		goto out;
 	}
 
 	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
 		if (!dev->mode_config.async_page_flip) {
-			drm_dbg_atomic(dev,
-				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
-			return -EINVAL;
+			drm_mode_atomic_add_error_msg(&state->error_code,
+						      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+						      "DRM_MODE_PAGE_FLIP_ASYNC not supported with ATOMIC ioctl");
+			ret = -EINVAL;
+			goto out;
 		}
 
 		async_flip = true;
@@ -1442,9 +1466,11 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	/* can't test and expect an event at the same time. */
 	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
 			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
-		drm_dbg_atomic(dev,
-			       "commit failed: page-flip event requested with test-only commit\n");
-		return -EINVAL;
+		drm_mode_atomic_add_error_msg(&state->error_code,
+					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+					      "page-flip event requested with test-only commit");
+		ret = -EINVAL;
+		goto out;
 	}
 
 retry:
@@ -1543,6 +1569,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	}
 
 out:
+	/* update the error code if any error to allow user handling it */
+	if (ret < 0 && arg->reserved) {
+		if (copy_to_user(error_code_ptr, &state->error_code,
+				 sizeof(state->error_code)))
+			return -EFAULT;
+	}
+
 	if (num_fences)
 		complete_signaling(dev, state, fence_state, num_fences, !ret);
 

-- 
2.25.1

[PATCH v6 5/5] drm/i915/display: Error codes for async flip failures

[Arun R Murthy]

For failures in async flip atomic check/commit path return user readable
error codes in struct drm_atomic_state.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b57efd8707743eb1b5a2b377fba8d6955af89825..6639f9168cf775c220fb653d69f8e27c5a4e6b88 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5911,9 +5911,10 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
 	}
 
 	if (intel_crtc_needs_modeset(new_crtc_state)) {
-		drm_dbg_kms(display->drm,
-			    "[CRTC:%d:%s] modeset required\n",
-			    crtc->base.base.id, crtc->base.name);
+		drm_mode_atomic_add_error_msg(&state->base.error_code,
+					      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+					      "[CRTC:%d:%s] requires full modeset",
+					      crtc->base.base.id, crtc->base.name);
 		return -EINVAL;
 	}
 
@@ -5980,9 +5981,10 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
 	}
 
 	if (intel_crtc_needs_modeset(new_crtc_state)) {
-		drm_dbg_kms(display->drm,
-			    "[CRTC:%d:%s] modeset required\n",
-			    crtc->base.base.id, crtc->base.name);
+		drm_mode_atomic_add_error_msg(&state->base.error_code,
+					      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+					      "[CRTC:%d:%s] requires full modeset",
+					      crtc->base.base.id, crtc->base.name);
 		return -EINVAL;
 	}
 
@@ -6020,11 +6022,12 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
 
 		if (!intel_plane_can_async_flip(plane, new_plane_state->hw.fb->format->format,
 						new_plane_state->hw.fb->modifier)) {
-			drm_dbg_kms(display->drm,
-				    "[PLANE:%d:%s] pixel format %p4cc / modifier 0x%llx does not support async flip\n",
-				    plane->base.base.id, plane->base.name,
-				    &new_plane_state->hw.fb->format->format,
-				    new_plane_state->hw.fb->modifier);
+			drm_mode_atomic_add_error_msg(&state->base.error_code,
+						      DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP,
+						      "[PLANE:%d:%s] pixel format %p4cc / 0x%llx modifier does not support async flip",
+						      plane->base.base.id, plane->base.name,
+						      &new_plane_state->hw.fb->format->format,
+						      new_plane_state->hw.fb->modifier);
 			return -EINVAL;
 		}
 

-- 
2.25.1

RE: [PATCH v6 0/5] User readable error codes on atomic_ioctl failure

[Murthy, Arun R]

Gentle Reminder!
Any comments on this?

Thanks and Regards,
Arun R Murthy
-------------------

> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@intel.com>
> Sent: Thursday, October 9, 2025 3:03 PM
> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David
> Airlie <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> <jani.nikula@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Joonas
> Lahtinen <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> <tursulin@ursulin.net>; xaver.hugl@kde.org; harry.wentland@amd.com;
> Shankar, Uma <uma.shankar@intel.com>; louis.chauvet@bootlin.com
> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org; Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCH v6 0/5] User readable error codes on atomic_ioctl failure
> 
> The series focuses on providing a user readable error value on a failure in
> drm_atomic_ioctl(). Usually -EINVAL is returned in most of the error cases and
> it is difficult for the user to decode the error and get to know the real cause for
> the error. If user gets to know the reason for the error then corrective
> measurements can be taken up.
> 
> TODO: driver specific error codes are to be added and will be done in the
> follow-up patches.
> 
> The IGT related changes are pushed for review @
> https://patchwork.freedesktop.org/series/153330/
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
> Arun R Murthy (5):
>       drm: Define user readable error codes for atomic ioctl
>       drm/atomic: Add error_code element in atomic_state
>       drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl
>       drm/atomic: Return user readable error in atomic_ioctl
>       drm/i915/display: Error codes for async flip failures
> 
>  drivers/gpu/drm/drm_atomic.c                 | 31 ++++++++++
>  drivers/gpu/drm/drm_atomic_uapi.c            | 92 +++++++++++++++++++--------
> -
>  drivers/gpu/drm/i915/display/intel_display.c | 25 ++++----
>  include/drm/drm_atomic.h                     | 10 +++
>  include/uapi/drm/drm_mode.h                  | 41 +++++++++++++
>  5 files changed, 159 insertions(+), 40 deletions(-)
> ---
> base-commit: a7101e35f29e03562f24ce1220f08350260fc0fc
> change-id: 20250728-atomic-c9713fd357e4
> 
> Best regards,
> --
> Arun R Murthy <arun.r.murthy@intel.com>

RE: [PATCH v6 1/5] drm: Define user readable error codes for atomic ioctl

[Kandpal, Suraj]

> Subject: [PATCH v6 1/5] drm: Define user readable error codes for atomic ioctl
> 
> There can be multiple reasons for a failure in atomic_ioctl. Most often in these
> error conditions -EINVAL is returned. User/Compositor would have to blindly
> take a call on failure of this ioctl so as to use ALLOW_MODESET or any. It would

Or Any ? I think you need to rephrase

> be good if user/compositor gets a readable error code on failure so they can
> take proper corrections in the next commit.
> The struct drm_mode_atomic is being passed by the user/compositor which
> holds the properties for modeset/flip. Reusing the same struct for returning
> the error code in case of failure can save by creating a new uapi/interface for
> returning the error code.

This sentence seems a little weird can you correct this.

> The element 'reserved' in the struct drm_mode_atomic is used for returning
> the user readable error code. This points to the struct

In that case why leave the element name as 'reserved' should that be changed

> drm_mode_atomic_err_code. Failure reasons have been initialized in
> DRM_MODE_ATOMIC_FAILURE_REASON.

I don't see the code for this here.

> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/uapi/drm/drm_mode.h | 41
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index
> 1e0e02a79b5c8db200cf9fd35edfe163d701cbd5..1e9eeae472e74bbd1b5e0b6f7
> 9f9782cafaf5b6e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -45,6 +45,7 @@ extern "C" {
>  #define DRM_CONNECTOR_NAME_LEN	32
>  #define DRM_DISPLAY_MODE_LEN	32
>  #define DRM_PROP_NAME_LEN	32
> +#define DRM_MODE_ATOMIC_FAILURE_STRING_LEN	128
> 
>  #define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
>  #define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN)
> /* deprecated */
> @@ -1205,6 +1206,46 @@ struct drm_mode_destroy_dumb {
>  		DRM_MODE_ATOMIC_NONBLOCK |\
>  		DRM_MODE_ATOMIC_ALLOW_MODESET)
> 
> +/**
> + * enum drm_mode_atomic_err_code -  error codes for failures in
> +atomic_ioctl
> + * @DRM_MODE_ATOMIC_INVALID_API_USAGE: invallid API
> usage(DRM_ATOMIC not
> + *				       enabled, invalid falg, page_flip event
> + *				       with test-only, etc)
> + * @DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET: Need full modeset

Should this be just be CRTC_NEED_MODESET to make it differ from the below definition.

> on this
> +crtc
> + * @DRM_MODE_ATOMIC_NEED_FULL_MODESET: Need full modeset on all
> +connected crtc's
> + * @DRM_MODE_ATOMIC_ASYN_NOTSUPP_PLANE: Aync flip not supported

Typos here
* DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE
* Async

Regards,
Suraj Kandpal

> on this
> +plane
> + * DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP: Modifier not
> supported by
> +async flip
> + * @DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED: Property changed in
> async flip
> +*/ enum drm_mode_atomic_failure_codes {
> +	DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +	DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
> +	DRM_MODE_ATOMIC_NEED_FULL_MODESET,
> +	DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
> +	DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP,
> +	DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> +};
> +
> +/**
> + * drm_mode_atomic_err_code - struct to store the error code
> + *
> + * pointer to this struct will be stored in reserved variable of
> + * struct drm_mode_atomic to report the failure cause to the user.
> + *
> + * @failure_code: error codes defined in enum
> +drm_moide_atomic_failure_code
> + * @failure_string_ptr: pointer to user readable error message string
> + * @failure_obj_ptr: pointer to the drm_object that caused error
> + * @reserved: reserved for future use
> + * @count_objs: count of drm_objects if multiple drm_objects caused
> +error  */ struct drm_mode_atomic_err_code {
> +	__u64 failure_code;
> +	__u64 failure_objs_ptr;
> +	__u64 reserved;
> +	__u32 count_objs;
> +	char failure_string[DRM_MODE_ATOMIC_FAILURE_STRING_LEN];
> +};
> +
>  struct drm_mode_atomic {
>  	__u32 flags;
>  	__u32 count_objs;
> 
> --
> 2.25.1

RE: [PATCH v6 3/5] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl

[Kandpal, Suraj]

> Subject: [PATCH v6 3/5] drm/atomic: Allocate atomic_state at the beginning of
> atomic_ioctl
> 
> Moving atomic_state allocation to the beginning of the atomci_ioctl to

This needs to be in imerative language so something like "Move ..."
Also Typo *atomic_ioctl

> accommodate drm_mode_atomic_err_code usage for returning error code on
> failures.
> 

Also maybe mention why drm_mode_atomic_err_code cannot be accommodated at previous place

> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index
> 85dbdaa4a2e25878c953b9b41539c8566d55c6d9..3ef478e717bec917d1b8803c
> 72bbcc8d6409d745 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1394,13 +1394,21 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_out_fence_state *fence_state;
>  	int ret = 0;
> -	unsigned int i, j, num_fences;
> +	unsigned int i, j, num_fences = 0;
>  	bool async_flip = false;
> 
>  	/* disallow for drivers not supporting atomic: */
>  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>  		return -EOPNOTSUPP;
> 
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	drm_modeset_acquire_init(&ctx,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> +	state->acquire_ctx = &ctx;
> +	state->allow_modeset = !!(arg->flags &
> DRM_MODE_ATOMIC_ALLOW_MODESET);
> +
>  	/* disallow for userspace that has not enabled atomic cap (even
>  	 * though this may be a bit overkill, since legacy userspace
>  	 * wouldn't know how to call this ioctl) @@ -1439,14 +1447,6 @@ int
> drm_mode_atomic_ioctl(struct drm_device *dev,
>  		return -EINVAL;
>  	}
> 
> -	state = drm_atomic_state_alloc(dev);
> -	if (!state)
> -		return -ENOMEM;
> -
> -	drm_modeset_acquire_init(&ctx,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> -	state->acquire_ctx = &ctx;
> -	state->allow_modeset = !!(arg->flags &
> DRM_MODE_ATOMIC_ALLOW_MODESET);
> -
>  retry:
>  	copied_objs = 0;
>  	copied_props = 0;
> @@ -1543,7 +1543,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	}
> 
>  out:
> -	complete_signaling(dev, state, fence_state, num_fences, !ret);
> +	if (num_fences)
> +		complete_signaling(dev, state, fence_state, num_fences, !ret);

Why the need to check num_fences before we complete signalling
Also this seems like a separate change maybe should be in its own patch

Regards,
Suraj Kandpal

> 
>  	if (ret == -EDEADLK) {
>  		drm_atomic_state_clear(state);
> 
> --
> 2.25.1

RE: [PATCH v6 2/5] drm/atomic: Add error_code element in atomic_state

[Kandpal, Suraj]

> Subject: [PATCH v6 2/5] drm/atomic: Add error_code element in atomic_state
> 
> Now that a proper error code will be returned to the user on any failure in
> atomic_ioctl() via struct drm_mode_atomic, add a new element error_code in
> the struct drm_atomic_state so as to hold the error code during the
> atomic_check() and atomic_commit() phases.
> New function added to print the error message and fill the struct err_code
> with proper error message and error code.
> 
> v5: Add a function for printing the error message and filling err_code
>     struct
> v6: Replace drm_err with drm_dbg_atomic print
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 31 +++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     | 10 ++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index
> 0fda567c390057b10bce691d9ddc11308088d92e..52eacb5a109d1c0f5b017e
> 552d9f5169f8d8fea5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1897,6 +1897,37 @@ void drm_state_dump(struct drm_device *dev,
> struct drm_printer *p)  }  EXPORT_SYMBOL(drm_state_dump);
> 
> +/**
> + * drm_mode_atomic_add_error_msg - function to add error code and error
> +string
> + *
> + * @err_code: pointer to struct drm_mode_atomic_err_code that stores the
> failure
> + *	      reason
> + * @failure_code: failure code in enum drm_mode_atomic_failure_codes
> + * @failure_string: failure reason string message
> + *
> + * Returns: void
> + */
> +void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code
> *err_code,
> +				   __u64 failure_code, const char *format, ...) {
> +	struct drm_atomic_state *state = container_of(err_code,
> +						      struct drm_atomic_state,
> +						      error_code);
> +	va_list varg;
> +	char *failure_string;
> +
> +	err_code->failure_code = failure_code;
> +
> +	va_start(varg, format);
> +	failure_string = kvasprintf(GFP_ATOMIC, format, varg);
> +
> +	drm_dbg_atomic(state->dev, "%s\n", failure_string);

In the next patches we endup changing the dbg message format from
"Commit failed: <failure reason>" to just "<failure reason>"
Maybe this should be "Commit failed: %s"

Regards,
Suraj Kandpal

> +	strscpy_pad(err_code->failure_string, failure_string,
> +		    sizeof(err_code->failure_string));
> +	va_end(varg);
> +}
> +EXPORT_SYMBOL(drm_mode_atomic_add_error_msg);
> +
>  #ifdef CONFIG_DEBUG_FS
>  static int drm_state_info(struct seq_file *m, void *data)  { diff --git
> a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index
> c8ab2163bf658cd06b12a8dabada7c088a328654..205ce418da22f8cbe10ea3
> 53c62471dbb41ae2e8 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -589,6 +589,13 @@ struct drm_atomic_state {
>  	 * commit without blocking.
>  	 */
>  	struct work_struct commit_work;
> +
> +	/* @error_code: pointer to struct holding failure reason and string
> +	 *
> +	 * struct to convey user readable error to the user.
> +	 * Error codes defined in enum drm_mode_atomic_failure_flags
> +	 */
> +	struct drm_mode_atomic_err_code error_code;
>  };
> 
>  void __drm_crtc_commit_free(struct kref *kref); @@ -1259,5 +1266,8 @@
> drm_atomic_get_old_bridge_state(const struct drm_atomic_state *state,
> struct drm_bridge_state *  drm_atomic_get_new_bridge_state(const struct
> drm_atomic_state *state,
>  				struct drm_bridge *bridge);
> +__printf(3, 4)
> +void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code
> *err_code,
> +				   __u64 failure_code, const char *format, ...);
> 
>  #endif /* DRM_ATOMIC_H_ */
> 
> --
> 2.25.1

RE: [PATCH v6 4/5] drm/atomic: Return user readable error in atomic_ioctl

[Kandpal, Suraj]

> Subject: [PATCH v6 4/5] drm/atomic: Return user readable error in
> atomic_ioctl
> 
> Add user readable error codes for failure cases in drm_atomic_ioctl() so that
> user can decode the error code and take corrective measurements.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c | 71
> ++++++++++++++++++++++++++++-----------
>  1 file changed, 52 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index
> 3ef478e717bec917d1b8803c72bbcc8d6409d745..3d4e9709e8e289cf302413
> e171b1336812d65c1c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1036,6 +1036,11 @@ int drm_atomic_set_property(struct
> drm_atomic_state *state,
>  			ret = drm_atomic_connector_get_property(connector,
> connector_state,
>  								prop,
> &old_val);
>  			ret = drm_atomic_check_prop_changes(ret, old_val,
> prop_value, prop);
> +			if (ret) {
> +				drm_mode_atomic_add_error_msg(&state-
> >error_code,
> +
> DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> +							      "property change
> not allowed in async flip");
> +			}
>  			break;
>  		}
> 
> @@ -1058,6 +1063,11 @@ int drm_atomic_set_property(struct
> drm_atomic_state *state,
>  			ret = drm_atomic_crtc_get_property(crtc, crtc_state,
>  							   prop, &old_val);
>  			ret = drm_atomic_check_prop_changes(ret, old_val,
> prop_value, prop);
> +			if (ret) {
> +				drm_mode_atomic_add_error_msg(&state-
> >error_code,
> +
> DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> +							      "property change
> not allowed in async flip");
> +			}
>  			break;
>  		}
> 
> @@ -1096,9 +1106,10 @@ int drm_atomic_set_property(struct
> drm_atomic_state *state,
>  					ret = plane_funcs-
> >atomic_async_check(plane, state, true);
> 
>  				if (ret) {
> -					drm_dbg_atomic(prop->dev,
> -						       "[PLANE:%d:%s] does not
> support async flips\n",
> -						       obj->id, plane->name);
> +
> 	drm_mode_atomic_add_error_msg(&state->error_code,
> +
> DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
> +
> "[PLANE:%d:%s] does not support async flip",
> +								      obj->id,
> plane->name);
>  					break;
>  				}
>  			}
> @@ -1393,6 +1404,7 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	struct drm_atomic_state *state;
>  	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_out_fence_state *fence_state;
> +	struct drm_mode_atomic_err_code __user *error_code_ptr;
>  	int ret = 0;
>  	unsigned int i, j, num_fences = 0;
>  	bool async_flip = false;
> @@ -1401,6 +1413,14 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>  		return -EOPNOTSUPP;
> 
> +	if (!arg->reserved)
> +		drm_dbg_atomic(dev,
> +			       "memory not allocated for drm_atomic error
> reporting\n");
> +	else
> +		/* update the error code if any error to allow user handling it
> */

First letter in uppercase.

> +		error_code_ptr = (struct drm_mode_atomic_err_code __user
> *)
> +				 (unsigned long)arg->reserved;
> +
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state)
>  		return -ENOMEM;
> @@ -1409,31 +1429,35 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	state->acquire_ctx = &ctx;
>  	state->allow_modeset = !!(arg->flags &
> DRM_MODE_ATOMIC_ALLOW_MODESET);
> 
> +	memset(&state->error_code, 0, sizeof(struct
> +drm_mode_atomic_err_code));

Isn't is better to have it as
sizeof(*error_code_ptr)

> +
>  	/* disallow for userspace that has not enabled atomic cap (even
>  	 * though this may be a bit overkill, since legacy userspace
>  	 * wouldn't know how to call this ioctl)
>  	 */
>  	if (!file_priv->atomic) {
> -		drm_dbg_atomic(dev,
> -			       "commit failed: atomic cap not enabled\n");
> -		return -EINVAL;
> +		drm_mode_atomic_add_error_msg(&state->error_code,
> +
> DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +					      "DRM_ATOMIC capability not
> enabled");

I'd rather have it say drm atomic capability not enabled

> +		ret = -EINVAL;
> +		goto out;
>  	}
> 
>  	if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
> -		drm_dbg_atomic(dev, "commit failed: invalid flag\n");
> -		return -EINVAL;
> -	}
> -
> -	if (arg->reserved) {
> -		drm_dbg_atomic(dev, "commit failed: reserved field set\n");
> -		return -EINVAL;
> +		drm_mode_atomic_add_error_msg(&state->error_code,
> +
> DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +					      "invalid flag");
> +		ret = -EINVAL;
> +		goto out;
>  	}
> 
>  	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
>  		if (!dev->mode_config.async_page_flip) {
> -			drm_dbg_atomic(dev,
> -				       "commit failed:
> DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
> -			return -EINVAL;
> +			drm_mode_atomic_add_error_msg(&state-
> >error_code,
> +
> DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +
> "DRM_MODE_PAGE_FLIP_ASYNC not supported with ATOMIC ioctl");
> +			ret = -EINVAL;
> +			goto out;
>  		}
> 
>  		async_flip = true;
> @@ -1442,9 +1466,11 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	/* can't test and expect an event at the same time. */
>  	if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
>  			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
> -		drm_dbg_atomic(dev,
> -			       "commit failed: page-flip event requested with
> test-only commit\n");
> -		return -EINVAL;
> +		drm_mode_atomic_add_error_msg(&state->error_code,
> +
> DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +					      "page-flip event requested with
> test-only commit");
> +		ret = -EINVAL;
> +		goto out;
>  	}
> 
>  retry:
> @@ -1543,6 +1569,13 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>  	}
> 
>  out:
> +	/* update the error code if any error to allow user handling it */

First letter in uppercase here

Regards,
Suraj Kandpal

> +	if (ret < 0 && arg->reserved) {
> +		if (copy_to_user(error_code_ptr, &state->error_code,
> +				 sizeof(state->error_code)))
> +			return -EFAULT;
> +	}
> +
>  	if (num_fences)
>  		complete_signaling(dev, state, fence_state, num_fences, !ret);
> 
> 
> --
> 2.25.1