[PATCH v4 0/4] User readable error codes on atomic_ioctl failure

[PATCH v4 0/4] 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/

v4: Removed XMacros and added enum for failure codes
    Failure strings added in respective failure place

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

 drivers/gpu/drm/drm_atomic.c                 | 23 +++++---
 drivers/gpu/drm/drm_atomic_uapi.c            | 83 ++++++++++++++++++++++------
 drivers/gpu/drm/i915/display/intel_display.c | 31 ++++++++---
 include/drm/drm_atomic.h                     | 26 +++++++++
 include/uapi/drm/drm_mode.h                  | 41 ++++++++++++++
 5 files changed, 171 insertions(+), 33 deletions(-)
---
base-commit: 21147fac313e561dcce8e18363c8de995f3ad4cd
change-id: 20250728-atomic-c9713fd357e4

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

[PATCH v4 1/4] 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 a122bea2559387576150236e3a88f99c24ad3138..6c4929e591b30b7a5eac472dadeeb3c9400e36df 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	64
 
 #define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
 #define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
@@ -1157,6 +1158,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 v4 2/4] 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.

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

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 38636a593c9d98cadda85ccd67326cb152f0dd27..d380001b24b4223baa54dae6c3c43e19dfb1958d 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -524,6 +524,13 @@ struct drm_atomic_state {
 	 * commit without blocking.
 	 */
 	struct work_struct commit_work;
+
+	/* @error_code
+	 *
+	 * 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);

-- 
2.25.1

[PATCH v4 3/4] 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.c      | 23 +++++++----
 drivers/gpu/drm/drm_atomic_uapi.c | 83 ++++++++++++++++++++++++++++++---------
 include/drm/drm_atomic.h          | 19 +++++++++
 3 files changed, 100 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index cd15cf52f0c9144711da5879da57884674aea9e4..4f6c7e659b362f0887ffcc85dade1122fd30df3d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1463,6 +1463,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	unsigned int requested_crtc = 0;
 	unsigned int affected_crtc = 0;
 	int i, ret = 0;
+	char *err_string;
 
 	drm_dbg_atomic(dev, "checking %p\n", state);
 
@@ -1511,8 +1512,13 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	if (!state->allow_modeset) {
 		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
-				drm_dbg_atomic(dev, "[CRTC:%d:%s] requires full modeset\n",
-					       crtc->base.id, crtc->name);
+				err_string = "requires full modeset";
+				drm_dbg_atomic(dev, "[CRTC:%d:%s] %s\n",
+					       crtc->base.id, crtc->name, err_string);
+				drm_mode_atomic_add_error_msg(state->error_code,
+							      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+							      err_string);
+
 				return -EINVAL;
 			}
 		}
@@ -1534,11 +1540,14 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	 * so compositors know what's going on.
 	 */
 	if (affected_crtc != requested_crtc) {
-		drm_dbg_atomic(dev,
-			       "driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
-			       requested_crtc, affected_crtc);
-		WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
-		     requested_crtc, affected_crtc);
+		err_string = "adding CRTC not allowed without modesets";
+		drm_dbg_atomic(dev, "%s: requested 0x%x, affected 0x%0x\n",
+			       err_string, requested_crtc, affected_crtc);
+		drm_mode_atomic_add_error_msg(state->error_code,
+					      DRM_MODE_ATOMIC_NEED_FULL_MODESET,
+					      err_string);
+
+		return -EINVAL;
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 85dbdaa4a2e25878c953b9b41539c8566d55c6d9..60f1b8baebce0db1ce984c8cda56039261b519e8 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1017,6 +1017,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 	struct drm_mode_object *ref;
 	u64 old_val;
 	int ret;
+	char *err_string;
 
 	if (!drm_property_change_valid_get(prop, prop_value, &ref))
 		return -EINVAL;
@@ -1058,6 +1059,12 @@ 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) {
+				err_string = "property change not allowed in async flip";
+				drm_mode_atomic_add_error_msg(state->error_code,
+							      DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
+							      err_string);
+			}
 			break;
 		}
 
@@ -1096,9 +1103,14 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 					ret = plane_funcs->atomic_async_check(plane, state, true);
 
 				if (ret) {
+					err_string = "plane does not support async flip";
+					drm_mode_atomic_add_error_msg(state->error_code,
+								      DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
+								      err_string);
 					drm_dbg_atomic(prop->dev,
-						       "[PLANE:%d:%s] does not support async flips\n",
-						       obj->id, plane->name);
+							"[PLANE:%d:%s] %s\n",
+						       obj->id, plane->name,
+						       err_string);
 					break;
 				}
 			}
@@ -1390,42 +1402,63 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
 	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
 	unsigned int copied_objs, copied_props;
-	struct drm_atomic_state *state;
+	struct drm_atomic_state *state = NULL;
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_out_fence_state *fence_state;
+	struct drm_mode_atomic_err_code error_code;
+	struct drm_mode_atomic_err_code __user *error_code_ptr;
 	int ret = 0;
 	unsigned int i, j, num_fences;
 	bool async_flip = false;
+	char *err_string;
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
 		return -EOPNOTSUPP;
 
+	if (!arg->reserved)
+		drm_err(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;
+
+	memset(&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) {
+		err_string = "DRM_ATOMIC capability not enabled";
 		drm_dbg_atomic(dev,
-			       "commit failed: atomic cap not enabled\n");
-		return -EINVAL;
+			       "commit failed: %s\n", err_string);
+		drm_mode_atomic_add_error_msg(&error_code,
+					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+					      err_string);
+		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;
+		err_string = "invalid flag";
+		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
+		drm_mode_atomic_add_error_msg(&error_code,
+					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+					      err_string);
+		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;
+			err_string = "DRM_MODE_PAGE_FLIP_ASYNC not supported with ATOMIC ioctl";
+			drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
+			drm_mode_atomic_add_error_msg(&error_code,
+						      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+						      err_string);
+			ret = -EINVAL;
+			goto out;
 		}
 
 		async_flip = true;
@@ -1434,9 +1467,13 @@ 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;
+		err_string = "page-flip event requested with test-only commit";
+		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
+		drm_mode_atomic_add_error_msg(&error_code,
+					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
+					      err_string);
+		ret = -EINVAL;
+		goto out;
 	}
 
 	state = drm_atomic_state_alloc(dev);
@@ -1447,6 +1484,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	state->acquire_ctx = &ctx;
 	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
 
+	state->error_code = &error_code;
+
 retry:
 	copied_objs = 0;
 	copied_props = 0;
@@ -1543,6 +1582,14 @@ 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, &error_code, sizeof(error_code)))
+			return -EFAULT;
+
+	if (!state)
+		return ret;
+
 	complete_signaling(dev, state, fence_state, num_fences, !ret);
 
 	if (ret == -EDEADLK) {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d380001b24b4223baa54dae6c3c43e19dfb1958d..8956b175795128422eefc2bc047b320b80aedc3f 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -1255,4 +1255,23 @@ struct drm_bridge_state *
 drm_atomic_get_new_bridge_state(const struct drm_atomic_state *state,
 				struct drm_bridge *bridge);
 
+/**
+ * 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
+ */
+static inline void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
+						 __u64 failure_code,
+						 char *failure_string)
+{
+	err_code->failure_code = failure_code;
+	strscpy_pad(err_code->failure_string, failure_string,
+		    strlen(err_code->failure_string));
+}
+
 #endif /* DRM_ATOMIC_H_ */

-- 
2.25.1

[PATCH v4 4/4] 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 | 31 +++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c1a3a95c65f0b66c24ddd64f47dfdc67bbde86c9..41b054764fdec37e4d9410fa7fbe62dbcbd3a11f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5935,6 +5935,7 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
 	struct intel_plane_state *new_plane_state;
 	struct intel_plane *plane;
 	int i;
+	char *err_string;
 
 	if (!new_crtc_state->uapi.async_flip)
 		return 0;
@@ -5947,9 +5948,13 @@ 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);
+		err_string = "requires full modeset";
+		drm_dbg_kms(display->drm, "[CRTC:%d:%s] %s\n",
+			    crtc->base.base.id, crtc->base.name,
+			    err_string);
+		drm_mode_atomic_add_error_msg(state->base.error_code,
+					      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+					      err_string);
 		return -EINVAL;
 	}
 
@@ -6001,6 +6006,7 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
 	const struct intel_plane_state *new_plane_state, *old_plane_state;
 	struct intel_plane *plane;
 	int i;
+	char *err_string;
 
 	old_crtc_state = intel_atomic_get_old_crtc_state(state, crtc);
 	new_crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
@@ -6016,9 +6022,13 @@ 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);
+		err_string = "requires full modeset";
+		drm_dbg_kms(display->drm, "[CRTC:%d:%s] %s\n",
+			    crtc->base.base.id, crtc->base.name,
+			    err_string);
+		drm_mode_atomic_add_error_msg(state->base.error_code,
+					      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+					      err_string);
 		return -EINVAL;
 	}
 
@@ -6056,11 +6066,16 @@ 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)) {
+			err_string = "modifier does not support async flip";
 			drm_dbg_kms(display->drm,
-				    "[PLANE:%d:%s] pixel format %p4cc / modifier 0x%llx does not support async flip\n",
+				    "[PLANE:%d:%s] pixel format %p4cc / 0x%llx %s\n",
 				    plane->base.base.id, plane->base.name,
 				    &new_plane_state->hw.fb->format->format,
-				    new_plane_state->hw.fb->modifier);
+				    new_plane_state->hw.fb->modifier,
+				    err_string);
+			drm_mode_atomic_add_error_msg(state->base.error_code,
+						      DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP,
+						      err_string);
 			return -EINVAL;
 		}
 

-- 
2.25.1

Re: [PATCH v4 1/4] drm: Define user readable error codes for atomic ioctl

[Jani Nikula]

On Tue, 02 Sep 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> 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>

Please run 'scripts/kernel-doc -Wall -none path/to/file' on all changes,
and make sure you're not adding new errors. There are plenty, and I'm
not going to iterate to you separately what the tool can tell you.

> ---
>  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 a122bea2559387576150236e3a88f99c24ad3138..6c4929e591b30b7a5eac472dadeeb3c9400e36df 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	64

64 is enough for everyone?

>  
>  #define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
>  #define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
> @@ -1157,6 +1158,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;

-- 
Jani Nikula, Intel

Re: [PATCH v4 3/4] drm/atomic: Return user readable error in atomic_ioctl

[Jani Nikula]

On Tue, 02 Sep 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> 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.c      | 23 +++++++----
>  drivers/gpu/drm/drm_atomic_uapi.c | 83 ++++++++++++++++++++++++++++++---------
>  include/drm/drm_atomic.h          | 19 +++++++++
>  3 files changed, 100 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index cd15cf52f0c9144711da5879da57884674aea9e4..4f6c7e659b362f0887ffcc85dade1122fd30df3d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1463,6 +1463,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	unsigned int requested_crtc = 0;
>  	unsigned int affected_crtc = 0;
>  	int i, ret = 0;
> +	char *err_string;
>  
>  	drm_dbg_atomic(dev, "checking %p\n", state);
>  
> @@ -1511,8 +1512,13 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	if (!state->allow_modeset) {
>  		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>  			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> -				drm_dbg_atomic(dev, "[CRTC:%d:%s] requires full modeset\n",
> -					       crtc->base.id, crtc->name);
> +				err_string = "requires full modeset";

This points a non-const pointer to const data.

Anyway none of this should require or start a precendent of using local
variables all over the place for the error strings. It's ugly. Ditto
below for all the places.

If all the places calling drm_mode_atomic_add_error_msg() do debug
logging, maybe that's where the debug logging should be?

> +				drm_dbg_atomic(dev, "[CRTC:%d:%s] %s\n",
> +					       crtc->base.id, crtc->name, err_string);
> +				drm_mode_atomic_add_error_msg(state->error_code,
> +							      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
> +							      err_string);

I guess you didn't want to follow what Maarten suggested.

> +
>  				return -EINVAL;

Adding the return is a functional change, and I mentioned it
earlier. Ignoring review is one of the certain ways to stop receiving
review in the future.

>  			}
>  		}
> @@ -1534,11 +1540,14 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	 * so compositors know what's going on.
>  	 */
>  	if (affected_crtc != requested_crtc) {
> -		drm_dbg_atomic(dev,
> -			       "driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
> -			       requested_crtc, affected_crtc);
> -		WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
> -		     requested_crtc, affected_crtc);
> +		err_string = "adding CRTC not allowed without modesets";
> +		drm_dbg_atomic(dev, "%s: requested 0x%x, affected 0x%0x\n",
> +			       err_string, requested_crtc, affected_crtc);
> +		drm_mode_atomic_add_error_msg(state->error_code,
> +					      DRM_MODE_ATOMIC_NEED_FULL_MODESET,
> +					      err_string);
> +
> +		return -EINVAL;

Adding the return is a functional change, and I mentioned it earlier.

>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 85dbdaa4a2e25878c953b9b41539c8566d55c6d9..60f1b8baebce0db1ce984c8cda56039261b519e8 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1017,6 +1017,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  	struct drm_mode_object *ref;
>  	u64 old_val;
>  	int ret;
> +	char *err_string;
>  
>  	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>  		return -EINVAL;
> @@ -1058,6 +1059,12 @@ 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) {
> +				err_string = "property change not allowed in async flip";
> +				drm_mode_atomic_add_error_msg(state->error_code,
> +							      DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> +							      err_string);
> +			}
>  			break;
>  		}
>  
> @@ -1096,9 +1103,14 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>  					ret = plane_funcs->atomic_async_check(plane, state, true);
>  
>  				if (ret) {
> +					err_string = "plane does not support async flip";
> +					drm_mode_atomic_add_error_msg(state->error_code,
> +								      DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
> +								      err_string);
>  					drm_dbg_atomic(prop->dev,
> -						       "[PLANE:%d:%s] does not support async flips\n",
> -						       obj->id, plane->name);
> +							"[PLANE:%d:%s] %s\n",
> +						       obj->id, plane->name,
> +						       err_string);
>  					break;
>  				}
>  			}
> @@ -1390,42 +1402,63 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
>  	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
>  	unsigned int copied_objs, copied_props;
> -	struct drm_atomic_state *state;
> +	struct drm_atomic_state *state = NULL;

Unrelated change.

>  	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_out_fence_state *fence_state;
> +	struct drm_mode_atomic_err_code error_code;
> +	struct drm_mode_atomic_err_code __user *error_code_ptr;
>  	int ret = 0;
>  	unsigned int i, j, num_fences;
>  	bool async_flip = false;
> +	char *err_string;
>  
>  	/* disallow for drivers not supporting atomic: */
>  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>  		return -EOPNOTSUPP;
>  
> +	if (!arg->reserved)
> +		drm_err(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;
> +
> +	memset(&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) {
> +		err_string = "DRM_ATOMIC capability not enabled";

Yeah, just repeating, don't start using this err_string local variable
idea.

>  		drm_dbg_atomic(dev,
> -			       "commit failed: atomic cap not enabled\n");
> -		return -EINVAL;
> +			       "commit failed: %s\n", err_string);
> +		drm_mode_atomic_add_error_msg(&error_code,
> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +					      err_string);
> +		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;
> +		err_string = "invalid flag";
> +		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
> +		drm_mode_atomic_add_error_msg(&error_code,
> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +					      err_string);
> +		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;
> +			err_string = "DRM_MODE_PAGE_FLIP_ASYNC not supported with ATOMIC ioctl";
> +			drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
> +			drm_mode_atomic_add_error_msg(&error_code,
> +						      DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +						      err_string);
> +			ret = -EINVAL;
> +			goto out;
>  		}
>  
>  		async_flip = true;
> @@ -1434,9 +1467,13 @@ 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;
> +		err_string = "page-flip event requested with test-only commit";
> +		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
> +		drm_mode_atomic_add_error_msg(&error_code,
> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +					      err_string);
> +		ret = -EINVAL;
> +		goto out;
>  	}
>  
>  	state = drm_atomic_state_alloc(dev);
> @@ -1447,6 +1484,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	state->acquire_ctx = &ctx;
>  	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
>  
> +	state->error_code = &error_code;
> +
>  retry:
>  	copied_objs = 0;
>  	copied_props = 0;
> @@ -1543,6 +1582,14 @@ 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, &error_code, sizeof(error_code)))
> +			return -EFAULT;
> +
> +	if (!state)
> +		return ret;
> +
>  	complete_signaling(dev, state, fence_state, num_fences, !ret);
>  
>  	if (ret == -EDEADLK) {
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d380001b24b4223baa54dae6c3c43e19dfb1958d..8956b175795128422eefc2bc047b320b80aedc3f 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1255,4 +1255,23 @@ struct drm_bridge_state *
>  drm_atomic_get_new_bridge_state(const struct drm_atomic_state *state,
>  				struct drm_bridge *bridge);
>  
> +/**
> + * 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
> + */
> +static inline void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
> +						 __u64 failure_code,
> +						 char *failure_string)

So this should be "const char *format, ..." with printf format, with
printf format annotation, to help callers pass in other stuff than just
fixed strings.

This should be a proper function instead of static inline.

> +{
> +	err_code->failure_code = failure_code;
> +	strscpy_pad(err_code->failure_string, failure_string,
> +		    strlen(err_code->failure_string));

If the size is fixed, why do we just silently truncate the string? If
the message is for the user, it'll be ugly to emit truncated strings.

> +}
> +
>  #endif /* DRM_ATOMIC_H_ */

-- 
Jani Nikula, Intel

Re: [PATCH v4 1/4] drm: Define user readable error codes for atomic ioctl

[Murthy, Arun R]

On 02-09-2025 15:36, Jani Nikula wrote:
> On Tue, 02 Sep 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> 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>
> Please run 'scripts/kernel-doc -Wall -none path/to/file' on all changes,
> and make sure you're not adding new errors. There are plenty, and I'm
> not going to iterate to you separately what the tool can tell you.
Ok
>> ---
>>   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 a122bea2559387576150236e3a88f99c24ad3138..6c4929e591b30b7a5eac472dadeeb3c9400e36df 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	64
> 64 is enough for everyone?
Should be sufficient, as we just need small meaningful error messages 
that can be used for logging in user space.

To accommodate long ones, can change it to 128.

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

>>   
>>   #define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
>>   #define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
>> @@ -1157,6 +1158,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;

Re: [PATCH v4 3/4] drm/atomic: Return user readable error in atomic_ioctl

[Murthy, Arun R]

On 02-09-2025 15:46, Jani Nikula wrote:
> On Tue, 02 Sep 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> 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.c      | 23 +++++++----
>>   drivers/gpu/drm/drm_atomic_uapi.c | 83 ++++++++++++++++++++++++++++++---------
>>   include/drm/drm_atomic.h          | 19 +++++++++
>>   3 files changed, 100 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index cd15cf52f0c9144711da5879da57884674aea9e4..4f6c7e659b362f0887ffcc85dade1122fd30df3d 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1463,6 +1463,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>   	unsigned int requested_crtc = 0;
>>   	unsigned int affected_crtc = 0;
>>   	int i, ret = 0;
>> +	char *err_string;
>>   
>>   	drm_dbg_atomic(dev, "checking %p\n", state);
>>   
>> @@ -1511,8 +1512,13 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>   	if (!state->allow_modeset) {
>>   		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>>   			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>> -				drm_dbg_atomic(dev, "[CRTC:%d:%s] requires full modeset\n",
>> -					       crtc->base.id, crtc->name);
>> +				err_string = "requires full modeset";
> This points a non-const pointer to const data.
>
> Anyway none of this should require or start a precendent of using local
> variables all over the place for the error strings. It's ugly. Ditto
> below for all the places.
>
> If all the places calling drm_mode_atomic_add_error_msg() do debug
> logging, maybe that's where the debug logging should be?
>
>> +				drm_dbg_atomic(dev, "[CRTC:%d:%s] %s\n",
>> +					       crtc->base.id, crtc->name, err_string);
>> +				drm_mode_atomic_add_error_msg(state->error_code,
>> +							      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
>> +							      err_string);
> I guess you didn't want to follow what Maarten suggested.
Yes, felt so, as adding a new function doesnt make much sense just for 
filling the struct drm_mode_atomic_err_code.

Will remove this function and fill the struct drm_mode_atomic_err_code 
in place, thereby the below comment also should be handled.

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

>
>> +
>>   				return -EINVAL;
> Adding the return is a functional change, and I mentioned it
> earlier. Ignoring review is one of the certain ways to stop receiving
> review in the future.
Will take it out of this series.
>>   			}
>>   		}
>> @@ -1534,11 +1540,14 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>   	 * so compositors know what's going on.
>>   	 */
>>   	if (affected_crtc != requested_crtc) {
>> -		drm_dbg_atomic(dev,
>> -			       "driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
>> -			       requested_crtc, affected_crtc);
>> -		WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
>> -		     requested_crtc, affected_crtc);
>> +		err_string = "adding CRTC not allowed without modesets";
>> +		drm_dbg_atomic(dev, "%s: requested 0x%x, affected 0x%0x\n",
>> +			       err_string, requested_crtc, affected_crtc);
>> +		drm_mode_atomic_add_error_msg(state->error_code,
>> +					      DRM_MODE_ATOMIC_NEED_FULL_MODESET,
>> +					      err_string);
>> +
>> +		return -EINVAL;
> Adding the return is a functional change, and I mentioned it earlier.
Will take it out of this series.
>
>>   	}
>>   
>>   	return 0;
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 85dbdaa4a2e25878c953b9b41539c8566d55c6d9..60f1b8baebce0db1ce984c8cda56039261b519e8 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1017,6 +1017,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>   	struct drm_mode_object *ref;
>>   	u64 old_val;
>>   	int ret;
>> +	char *err_string;
>>   
>>   	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>   		return -EINVAL;
>> @@ -1058,6 +1059,12 @@ 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) {
>> +				err_string = "property change not allowed in async flip";
>> +				drm_mode_atomic_add_error_msg(state->error_code,
>> +							      DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
>> +							      err_string);
>> +			}
>>   			break;
>>   		}
>>   
>> @@ -1096,9 +1103,14 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>   					ret = plane_funcs->atomic_async_check(plane, state, true);
>>   
>>   				if (ret) {
>> +					err_string = "plane does not support async flip";
>> +					drm_mode_atomic_add_error_msg(state->error_code,
>> +								      DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
>> +								      err_string);
>>   					drm_dbg_atomic(prop->dev,
>> -						       "[PLANE:%d:%s] does not support async flips\n",
>> -						       obj->id, plane->name);
>> +							"[PLANE:%d:%s] %s\n",
>> +						       obj->id, plane->name,
>> +						       err_string);
>>   					break;
>>   				}
>>   			}
>> @@ -1390,42 +1402,63 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
>>   	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
>>   	unsigned int copied_objs, copied_props;
>> -	struct drm_atomic_state *state;
>> +	struct drm_atomic_state *state = NULL;
> Unrelated change.
Noted!
>
>>   	struct drm_modeset_acquire_ctx ctx;
>>   	struct drm_out_fence_state *fence_state;
>> +	struct drm_mode_atomic_err_code error_code;
>> +	struct drm_mode_atomic_err_code __user *error_code_ptr;
>>   	int ret = 0;
>>   	unsigned int i, j, num_fences;
>>   	bool async_flip = false;
>> +	char *err_string;
>>   
>>   	/* disallow for drivers not supporting atomic: */
>>   	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>>   		return -EOPNOTSUPP;
>>   
>> +	if (!arg->reserved)
>> +		drm_err(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;
>> +
>> +	memset(&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) {
>> +		err_string = "DRM_ATOMIC capability not enabled";
> Yeah, just repeating, don't start using this err_string local variable
> idea.
>
>>   		drm_dbg_atomic(dev,
>> -			       "commit failed: atomic cap not enabled\n");
>> -		return -EINVAL;
>> +			       "commit failed: %s\n", err_string);
>> +		drm_mode_atomic_add_error_msg(&error_code,
>> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +					      err_string);
>> +		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;
>> +		err_string = "invalid flag";
>> +		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
>> +		drm_mode_atomic_add_error_msg(&error_code,
>> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +					      err_string);
>> +		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;
>> +			err_string = "DRM_MODE_PAGE_FLIP_ASYNC not supported with ATOMIC ioctl";
>> +			drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
>> +			drm_mode_atomic_add_error_msg(&error_code,
>> +						      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +						      err_string);
>> +			ret = -EINVAL;
>> +			goto out;
>>   		}
>>   
>>   		async_flip = true;
>> @@ -1434,9 +1467,13 @@ 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;
>> +		err_string = "page-flip event requested with test-only commit";
>> +		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
>> +		drm_mode_atomic_add_error_msg(&error_code,
>> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +					      err_string);
>> +		ret = -EINVAL;
>> +		goto out;
>>   	}
>>   
>>   	state = drm_atomic_state_alloc(dev);
>> @@ -1447,6 +1484,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   	state->acquire_ctx = &ctx;
>>   	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
>>   
>> +	state->error_code = &error_code;
>> +
>>   retry:
>>   	copied_objs = 0;
>>   	copied_props = 0;
>> @@ -1543,6 +1582,14 @@ 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, &error_code, sizeof(error_code)))
>> +			return -EFAULT;
>> +
>> +	if (!state)
>> +		return ret;
>> +
>>   	complete_signaling(dev, state, fence_state, num_fences, !ret);
>>   
>>   	if (ret == -EDEADLK) {
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index d380001b24b4223baa54dae6c3c43e19dfb1958d..8956b175795128422eefc2bc047b320b80aedc3f 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -1255,4 +1255,23 @@ struct drm_bridge_state *
>>   drm_atomic_get_new_bridge_state(const struct drm_atomic_state *state,
>>   				struct drm_bridge *bridge);
>>   
>> +/**
>> + * 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
>> + */
>> +static inline void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
>> +						 __u64 failure_code,
>> +						 char *failure_string)
> So this should be "const char *format, ..." with printf format, with
> printf format annotation, to help callers pass in other stuff than just
> fixed strings.
>
> This should be a proper function instead of static inline.
>
>> +{
>> +	err_code->failure_code = failure_code;
>> +	strscpy_pad(err_code->failure_string, failure_string,
>> +		    strlen(err_code->failure_string));
> If the size is fixed, why do we just silently truncate the string? If
> the message is for the user, it'll be ugly to emit truncated strings.
>
>> +}
>> +
>>   #endif /* DRM_ATOMIC_H_ */

Re: [PATCH v4 3/4] drm/atomic: Return user readable error in atomic_ioctl

[Jani Nikula]

On Wed, 03 Sep 2025, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
> On 02-09-2025 15:46, Jani Nikula wrote:
>> On Tue, 02 Sep 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>>> 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.c      | 23 +++++++----
>>>   drivers/gpu/drm/drm_atomic_uapi.c | 83 ++++++++++++++++++++++++++++++---------
>>>   include/drm/drm_atomic.h          | 19 +++++++++
>>>   3 files changed, 100 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index cd15cf52f0c9144711da5879da57884674aea9e4..4f6c7e659b362f0887ffcc85dade1122fd30df3d 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -1463,6 +1463,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>>   	unsigned int requested_crtc = 0;
>>>   	unsigned int affected_crtc = 0;
>>>   	int i, ret = 0;
>>> +	char *err_string;
>>>   
>>>   	drm_dbg_atomic(dev, "checking %p\n", state);
>>>   
>>> @@ -1511,8 +1512,13 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>>   	if (!state->allow_modeset) {
>>>   		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>>>   			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>>> -				drm_dbg_atomic(dev, "[CRTC:%d:%s] requires full modeset\n",
>>> -					       crtc->base.id, crtc->name);
>>> +				err_string = "requires full modeset";
>> This points a non-const pointer to const data.
>>
>> Anyway none of this should require or start a precendent of using local
>> variables all over the place for the error strings. It's ugly. Ditto
>> below for all the places.
>>
>> If all the places calling drm_mode_atomic_add_error_msg() do debug
>> logging, maybe that's where the debug logging should be?
>>
>>> +				drm_dbg_atomic(dev, "[CRTC:%d:%s] %s\n",
>>> +					       crtc->base.id, crtc->name, err_string);
>>> +				drm_mode_atomic_add_error_msg(state->error_code,
>>> +							      DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
>>> +							      err_string);
>> I guess you didn't want to follow what Maarten suggested.
> Yes, felt so, as adding a new function doesnt make much sense just for 
> filling the struct drm_mode_atomic_err_code.
>
> Will remove this function and fill the struct drm_mode_atomic_err_code 
> in place, thereby the below comment also should be handled.

Nobody suggested that! Keep the function.


>
> Thanks and Regards,
> Arun R Murthy
> -------------------
>
>>
>>> +
>>>   				return -EINVAL;
>> Adding the return is a functional change, and I mentioned it
>> earlier. Ignoring review is one of the certain ways to stop receiving
>> review in the future.
> Will take it out of this series.
>>>   			}
>>>   		}
>>> @@ -1534,11 +1540,14 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>>   	 * so compositors know what's going on.
>>>   	 */
>>>   	if (affected_crtc != requested_crtc) {
>>> -		drm_dbg_atomic(dev,
>>> -			       "driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
>>> -			       requested_crtc, affected_crtc);
>>> -		WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
>>> -		     requested_crtc, affected_crtc);
>>> +		err_string = "adding CRTC not allowed without modesets";
>>> +		drm_dbg_atomic(dev, "%s: requested 0x%x, affected 0x%0x\n",
>>> +			       err_string, requested_crtc, affected_crtc);
>>> +		drm_mode_atomic_add_error_msg(state->error_code,
>>> +					      DRM_MODE_ATOMIC_NEED_FULL_MODESET,
>>> +					      err_string);
>>> +
>>> +		return -EINVAL;
>> Adding the return is a functional change, and I mentioned it earlier.
> Will take it out of this series.
>>
>>>   	}
>>>   
>>>   	return 0;
>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>> index 85dbdaa4a2e25878c953b9b41539c8566d55c6d9..60f1b8baebce0db1ce984c8cda56039261b519e8 100644
>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>> @@ -1017,6 +1017,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>>   	struct drm_mode_object *ref;
>>>   	u64 old_val;
>>>   	int ret;
>>> +	char *err_string;
>>>   
>>>   	if (!drm_property_change_valid_get(prop, prop_value, &ref))
>>>   		return -EINVAL;
>>> @@ -1058,6 +1059,12 @@ 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) {
>>> +				err_string = "property change not allowed in async flip";
>>> +				drm_mode_atomic_add_error_msg(state->error_code,
>>> +							      DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
>>> +							      err_string);
>>> +			}
>>>   			break;
>>>   		}
>>>   
>>> @@ -1096,9 +1103,14 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>>>   					ret = plane_funcs->atomic_async_check(plane, state, true);
>>>   
>>>   				if (ret) {
>>> +					err_string = "plane does not support async flip";
>>> +					drm_mode_atomic_add_error_msg(state->error_code,
>>> +								      DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
>>> +								      err_string);
>>>   					drm_dbg_atomic(prop->dev,
>>> -						       "[PLANE:%d:%s] does not support async flips\n",
>>> -						       obj->id, plane->name);
>>> +							"[PLANE:%d:%s] %s\n",
>>> +						       obj->id, plane->name,
>>> +						       err_string);
>>>   					break;
>>>   				}
>>>   			}
>>> @@ -1390,42 +1402,63 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>>   	uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
>>>   	uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
>>>   	unsigned int copied_objs, copied_props;
>>> -	struct drm_atomic_state *state;
>>> +	struct drm_atomic_state *state = NULL;
>> Unrelated change.
> Noted!
>>
>>>   	struct drm_modeset_acquire_ctx ctx;
>>>   	struct drm_out_fence_state *fence_state;
>>> +	struct drm_mode_atomic_err_code error_code;
>>> +	struct drm_mode_atomic_err_code __user *error_code_ptr;
>>>   	int ret = 0;
>>>   	unsigned int i, j, num_fences;
>>>   	bool async_flip = false;
>>> +	char *err_string;
>>>   
>>>   	/* disallow for drivers not supporting atomic: */
>>>   	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>>>   		return -EOPNOTSUPP;
>>>   
>>> +	if (!arg->reserved)
>>> +		drm_err(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;
>>> +
>>> +	memset(&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) {
>>> +		err_string = "DRM_ATOMIC capability not enabled";
>> Yeah, just repeating, don't start using this err_string local variable
>> idea.
>>
>>>   		drm_dbg_atomic(dev,
>>> -			       "commit failed: atomic cap not enabled\n");
>>> -		return -EINVAL;
>>> +			       "commit failed: %s\n", err_string);
>>> +		drm_mode_atomic_add_error_msg(&error_code,
>>> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>>> +					      err_string);
>>> +		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;
>>> +		err_string = "invalid flag";
>>> +		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
>>> +		drm_mode_atomic_add_error_msg(&error_code,
>>> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>>> +					      err_string);
>>> +		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;
>>> +			err_string = "DRM_MODE_PAGE_FLIP_ASYNC not supported with ATOMIC ioctl";
>>> +			drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
>>> +			drm_mode_atomic_add_error_msg(&error_code,
>>> +						      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>>> +						      err_string);
>>> +			ret = -EINVAL;
>>> +			goto out;
>>>   		}
>>>   
>>>   		async_flip = true;
>>> @@ -1434,9 +1467,13 @@ 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;
>>> +		err_string = "page-flip event requested with test-only commit";
>>> +		drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
>>> +		drm_mode_atomic_add_error_msg(&error_code,
>>> +					      DRM_MODE_ATOMIC_INVALID_API_USAGE,
>>> +					      err_string);
>>> +		ret = -EINVAL;
>>> +		goto out;
>>>   	}
>>>   
>>>   	state = drm_atomic_state_alloc(dev);
>>> @@ -1447,6 +1484,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>>   	state->acquire_ctx = &ctx;
>>>   	state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
>>>   
>>> +	state->error_code = &error_code;
>>> +
>>>   retry:
>>>   	copied_objs = 0;
>>>   	copied_props = 0;
>>> @@ -1543,6 +1582,14 @@ 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, &error_code, sizeof(error_code)))
>>> +			return -EFAULT;
>>> +
>>> +	if (!state)
>>> +		return ret;
>>> +
>>>   	complete_signaling(dev, state, fence_state, num_fences, !ret);
>>>   
>>>   	if (ret == -EDEADLK) {
>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>> index d380001b24b4223baa54dae6c3c43e19dfb1958d..8956b175795128422eefc2bc047b320b80aedc3f 100644
>>> --- a/include/drm/drm_atomic.h
>>> +++ b/include/drm/drm_atomic.h
>>> @@ -1255,4 +1255,23 @@ struct drm_bridge_state *
>>>   drm_atomic_get_new_bridge_state(const struct drm_atomic_state *state,
>>>   				struct drm_bridge *bridge);
>>>   
>>> +/**
>>> + * 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
>>> + */
>>> +static inline void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
>>> +						 __u64 failure_code,
>>> +						 char *failure_string)
>> So this should be "const char *format, ..." with printf format, with
>> printf format annotation, to help callers pass in other stuff than just
>> fixed strings.
>>
>> This should be a proper function instead of static inline.
>>
>>> +{
>>> +	err_code->failure_code = failure_code;
>>> +	strscpy_pad(err_code->failure_string, failure_string,
>>> +		    strlen(err_code->failure_string));
>> If the size is fixed, why do we just silently truncate the string? If
>> the message is for the user, it'll be ugly to emit truncated strings.
>>
>>> +}
>>> +
>>>   #endif /* DRM_ATOMIC_H_ */

-- 
Jani Nikula, Intel