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

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

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                 |  5 ++++
 drivers/gpu/drm/drm_atomic_uapi.c            | 20 ++++++++++----
 drivers/gpu/drm/i915/display/intel_display.c |  4 +++
 include/drm/drm_atomic.h                     |  7 +++++
 include/uapi/drm/drm_mode.h                  | 40 ++++++++++++++++++++++++++++
 5 files changed, 71 insertions(+), 5 deletions(-)
---
base-commit: 52ac98096df0784abd379f822ad14d6998b09154
change-id: 20250728-atomic-c9713fd357e4

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

[PATCH v2 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.Its a 64bit variable and
should suffice 64 error codes that should be sufficient.

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

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a122bea2559387576150236e3a88f99c24ad3138..87e8f623bfaaab09135be104db04996f0be4dcb4 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1157,6 +1157,46 @@ struct drm_mode_destroy_dumb {
 		DRM_MODE_ATOMIC_NONBLOCK |\
 		DRM_MODE_ATOMIC_ALLOW_MODESET)
 
+/* atomic not set in the drm_file */
+#define DRM_MODE_ATOMIC_CAP_NOT_ENABLED			BIT(0)
+/* atomic flag passed not in DRM_MODE_ATOMIC_FLAGS list */
+#define DRM_MODE_ATOMIC_INVALID_FLAG			BIT(1)
+/* DRM_MODE_PAGE_FLIP_LEGACY not supported with atomic ioctl  */
+#define DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC			BIT(2)
+/* flip event with atomic check only not supported */
+#define DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY	BIT(3)
+/* atomic property change requested need full crtc modeset */
+#define DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET		BIT(4)
+/* atomic property change requested has impact on all connected crtc */
+#define DRM_MODE_ATOMIC_NEED_FULL_MODESET		BIT(5)
+/* async flip supported on only primary plane */
+#define DRM_MODE_ATOMIC_ASYNC_NOT_PRIMARY		BIT(6)
+/* modifier not supported by async flip */
+#define DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED	BIT(7)
+/* async flip with pipe joiner not allowed */
+#define DRM_MODE_ATOMIC_ASYNC_PIPEJOINER_NOTALLOWED	BIT(8)
+/* No properties can be changed with async flip */
+#define DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED		BIT(9)
+
+/**
+ * DRM_MODE_ATOMIC_ERR_FLAGS
+ *
+ * Bit field of the reserved element in drm_mode_atomic will be
+ * used for returning meaningful error message to the compositor
+ * so that compositor can take necessary correction going ahead.
+ */
+#define DRM_MODE_ATOMIC_ERR_FLAGS (\
+		DRM_MODE_ATOMIC_CAP_NOT_ENABLED |\
+		DRM_MODE_ATOMIC_INVALID_FLAG |\
+		DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC |\
+		DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY |\
+		DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET |\
+		DRM_MODE_ATOMIC_NEED_FULL_MODESET |\
+		DRM_MODE_ATOMIC_ASYNC_NOT_PRIMARY |\
+		DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED |\
+		DRM_MODE_ATOMIC_ASYNC_PIPEJOINER_NOTALLOWED |\
+		DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED)
+
 struct drm_mode_atomic {
 	__u32 flags;
 	__u32 count_objs;

-- 
2.25.1

[PATCH v2 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..55799d848408e23ae5ece2dd694d119489a87ede 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
+	 *
+	 * flag to convey user readable error to the user.
+	 * Error codes defined in DRM_MODE_ATOMIC_ERR_FLAGS
+	 */
+	u64 error_code;
 };
 
 void __drm_crtc_commit_free(struct kref *kref);

-- 
2.25.1

[PATCH v2 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      |  5 +++++
 drivers/gpu/drm/drm_atomic_uapi.c | 20 +++++++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index cd15cf52f0c9144711da5879da57884674aea9e4..0cf73022955437260d138c6a1e2bb9a8a4eca2f0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1513,6 +1513,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 			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);
+				state->error_code = DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET;
+
 				return -EINVAL;
 			}
 		}
@@ -1537,8 +1539,11 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		drm_dbg_atomic(dev,
 			       "driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
 			       requested_crtc, affected_crtc);
+		state->error_code = DRM_MODE_ATOMIC_NEED_FULL_MODESET;
 		WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
 		     requested_crtc, affected_crtc);
+
+		return -EINVAL;
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index ecc73d52bfae41a7ef455a7e13649ec56c690b90..b96707ca23ac01878c3df25b8a4a026f6c6c31d2 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1058,6 +1058,8 @@ 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)
+				state->error_code = DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED;
 			break;
 		}
 
@@ -1089,6 +1091,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 
 			/* ask the driver if this non-primary plane is supported */
 			if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
+				state->error_code = DRM_MODE_ATOMIC_ASYNC_NOT_PRIMARY;
 				ret = -EINVAL;
 
 				if (plane_funcs && plane_funcs->atomic_async_check)
@@ -1400,6 +1403,11 @@ 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, "commit failed: reserved field set\n");
+		return -EINVAL;
+	}
+
 	/* 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)
@@ -1407,16 +1415,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	if (!file_priv->atomic) {
 		drm_dbg_atomic(dev,
 			       "commit failed: atomic cap not enabled\n");
+		arg->reserved = DRM_MODE_ATOMIC_CAP_NOT_ENABLED;
 		return -EINVAL;
 	}
 
 	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");
+		arg->reserved = DRM_MODE_ATOMIC_INVALID_FLAG;
 		return -EINVAL;
 	}
 
@@ -1424,6 +1429,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		if (!dev->mode_config.async_page_flip) {
 			drm_dbg_atomic(dev,
 				       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
+			arg->reserved = DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC;
 			return -EINVAL;
 		}
 
@@ -1435,6 +1441,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
 		drm_dbg_atomic(dev,
 			       "commit failed: page-flip event requested with test-only commit\n");
+		arg->reserved = DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY;
 		return -EINVAL;
 	}
 
@@ -1541,6 +1548,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 		ret = drm_atomic_commit(state);
 	}
 
+	/* update the error code if any error to allow user handling it */
+	arg->reserved = state->error_code;
+
 out:
 	complete_signaling(dev, state, fence_state, num_fences, !ret);
 

-- 
2.25.1

[PATCH v2 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 7035c1fc9033b10327be081f26715446687652c4..893e4d0eb4c3d4d2611720bc7f627b11af81ea8b 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -5944,6 +5944,7 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
 		drm_dbg_kms(display->drm,
 			    "[CRTC:%d:%s] modeset required\n",
 			    crtc->base.base.id, crtc->base.name);
+		state->base.error_code = DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET;
 		return -EINVAL;
 	}
 
@@ -5955,6 +5956,7 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
 		drm_dbg_kms(display->drm,
 			    "[CRTC:%d:%s] async flip disallowed with joiner\n",
 			    crtc->base.base.id, crtc->base.name);
+		state->base.error_code = DRM_MODE_ATOMIC_ASYNC_PIPEJOINER_NOTALLOWED;
 		return -EINVAL;
 	}
 
@@ -6013,6 +6015,7 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
 		drm_dbg_kms(display->drm,
 			    "[CRTC:%d:%s] modeset required\n",
 			    crtc->base.base.id, crtc->base.name);
+		state->base.error_code = DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET;
 		return -EINVAL;
 	}
 
@@ -6055,6 +6058,7 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
 				    plane->base.base.id, plane->base.name,
 				    &new_plane_state->hw.fb->format->format,
 				    new_plane_state->hw.fb->modifier);
+			state->base.error_code = DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED;
 			return -EINVAL;
 		}
 

-- 
2.25.1

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

[Biju Das]

Hi Arun,

> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Arun R Murthy
> Sent: 30 July 2025 11:17
> Subject: [PATCH v2 1/4] 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 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.Its a 64bit variable and should suffice 64 error codes that should be sufficient.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/uapi/drm/drm_mode.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index
> a122bea2559387576150236e3a88f99c24ad3138..87e8f623bfaaab09135be104db04996f0be4dcb4 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1157,6 +1157,46 @@ struct drm_mode_destroy_dumb {
>  		DRM_MODE_ATOMIC_NONBLOCK |\
>  		DRM_MODE_ATOMIC_ALLOW_MODESET)
> 
> +/* atomic not set in the drm_file */
> +#define DRM_MODE_ATOMIC_CAP_NOT_ENABLED			BIT(0)
> +/* atomic flag passed not in DRM_MODE_ATOMIC_FLAGS list */
> +#define DRM_MODE_ATOMIC_INVALID_FLAG			BIT(1)
> +/* DRM_MODE_PAGE_FLIP_LEGACY not supported with atomic ioctl  */
> +#define DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC			BIT(2)
> +/* flip event with atomic check only not supported */
> +#define DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY	BIT(3)
> +/* atomic property change requested need full crtc modeset */
> +#define DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET		BIT(4)
> +/* atomic property change requested has impact on all connected crtc */
> +#define DRM_MODE_ATOMIC_NEED_FULL_MODESET		BIT(5)
> +/* async flip supported on only primary plane */
> +#define DRM_MODE_ATOMIC_ASYNC_NOT_PRIMARY		BIT(6)
> +/* modifier not supported by async flip */
> +#define DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED	BIT(7)
> +/* async flip with pipe joiner not allowed */
> +#define DRM_MODE_ATOMIC_ASYNC_PIPEJOINER_NOTALLOWED	BIT(8)

For consistency, NOTALLOWED->NOT_ALLOWED ??

Cheers,
Biju

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

[Harry Wentland]

On 2025-07-30 06:16, Arun R Murthy 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.
> 

Thanks for doing this work.


> @@ -1541,6 +1548,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  		ret = drm_atomic_commit(state);
>  	}
>  
> +	/* update the error code if any error to allow user handling it */
> +	arg->reserved = state->error_code;

Once we do this we paint ourselves into a corner.

It's nice that we have this reserved field since it allows us to
extend the atomic ioctl without the need to define a new one.

When we discussed this at the Display Next Hackfest [1] we came
to the conclusion that we would want:

- an enum to show what is the issue
- a string that can be logged to display verbose information
  about the failure
- an optional array of KMS object IDs participating in the failure

We could define these in a new struct

struct drm_mode_atomic2 {
	__u64 failure_reason;
	__u64 failure_string;
	__u32 drm_object_ids[MAX_FAILURE_OBJECT_IDS]
	__u64 reserved;
}

that we link to via the drm_mode_atomic->reserved field.

Your approach of flags, as opposed to an enum, would allow reporting
of multiple failures. Do we think drivers would ever make use of it?
Normally check short-circuits when a failure/limitation is hit.

Harry

[1] https://hackmd.io/f3bDn3kyRUalLn4LbMfCVQ#Commit-Failure-Feedback

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

[Murthy, Arun R]

On 30-07-2025 16:52, Biju Das wrote:
> Hi Arun,
>
>> -----Original Message-----
>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Arun R Murthy
>> Sent: 30 July 2025 11:17
>> Subject: [PATCH v2 1/4] 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 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.Its a 64bit variable and should suffice 64 error codes that should be sufficient.
>>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>   include/uapi/drm/drm_mode.h | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index
>> a122bea2559387576150236e3a88f99c24ad3138..87e8f623bfaaab09135be104db04996f0be4dcb4 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -1157,6 +1157,46 @@ struct drm_mode_destroy_dumb {
>>   		DRM_MODE_ATOMIC_NONBLOCK |\
>>   		DRM_MODE_ATOMIC_ALLOW_MODESET)
>>
>> +/* atomic not set in the drm_file */
>> +#define DRM_MODE_ATOMIC_CAP_NOT_ENABLED			BIT(0)
>> +/* atomic flag passed not in DRM_MODE_ATOMIC_FLAGS list */
>> +#define DRM_MODE_ATOMIC_INVALID_FLAG			BIT(1)
>> +/* DRM_MODE_PAGE_FLIP_LEGACY not supported with atomic ioctl  */
>> +#define DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC			BIT(2)
>> +/* flip event with atomic check only not supported */
>> +#define DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY	BIT(3)
>> +/* atomic property change requested need full crtc modeset */
>> +#define DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET		BIT(4)
>> +/* atomic property change requested has impact on all connected crtc */
>> +#define DRM_MODE_ATOMIC_NEED_FULL_MODESET		BIT(5)
>> +/* async flip supported on only primary plane */
>> +#define DRM_MODE_ATOMIC_ASYNC_NOT_PRIMARY		BIT(6)
>> +/* modifier not supported by async flip */
>> +#define DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED	BIT(7)
>> +/* async flip with pipe joiner not allowed */
>> +#define DRM_MODE_ATOMIC_ASYNC_PIPEJOINER_NOTALLOWED	BIT(8)
> For consistency, NOTALLOWED->NOT_ALLOWED ??

Sure, will get it correct!

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

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

[Murthy, Arun R]

On 30-07-2025 18:44, Harry Wentland wrote:
>
> On 2025-07-30 06:16, Arun R Murthy 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.
>>
> Thanks for doing this work.
>
>
>> @@ -1541,6 +1548,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>   		ret = drm_atomic_commit(state);
>>   	}
>>   
>> +	/* update the error code if any error to allow user handling it */
>> +	arg->reserved = state->error_code;
> Once we do this we paint ourselves into a corner.
>
> It's nice that we have this reserved field since it allows us to
> extend the atomic ioctl without the need to define a new one.
>
> When we discussed this at the Display Next Hackfest [1] we came
> to the conclusion that we would want:
>
> - an enum to show what is the issue
> - a string that can be logged to display verbose information
>    about the failure
> - an optional array of KMS object IDs participating in the failure
>
> We could define these in a new struct
>
> struct drm_mode_atomic2 {
> 	__u64 failure_reason;
> 	__u64 failure_string;
> 	__u32 drm_object_ids[MAX_FAILURE_OBJECT_IDS]
> 	__u64 reserved;
> }
>
> that we link to via the drm_mode_atomic->reserved field.
>
> Your approach of flags, as opposed to an enum, would allow reporting
> of multiple failures. Do we think drivers would ever make use of it?
> Normally check short-circuits when a failure/limitation is hit.
Thanks for the feedback. As pointed it would be good to have a struct 
pointed by the reserved variable, so that we can further extend the scope.
W.r.t the enum, I feel its better to have bit notification as we can 
convey multiple errors if any.

Understand that the driver at present will return on the first error but 
upon adding this user readable error code can extend the driver to check 
add the properties provided by user and report error at once, so that 
multiple failure iterations can be overridden.

Having obj_id would be a good one, but when the flow goes to the vendor 
specific driver, they may not have the link to the obj_id where a 
failure is occurred. But still good to have so that at the early stage 
in set_prop, sanity check failures can be reported with obj_id so that 
it would be easy for the user to correct them in the next atomic ioctl.

Will take care of these feedback in my next revision of patch set.

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

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

[Harry Wentland]

On 2025-07-30 09:55, Murthy, Arun R wrote:
> On 30-07-2025 18:44, Harry Wentland wrote:
>>
>> On 2025-07-30 06:16, Arun R Murthy 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.
>>>
>> Thanks for doing this work.
>>
>>
>>> @@ -1541,6 +1548,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>>           ret = drm_atomic_commit(state);
>>>       }
>>>   +    /* update the error code if any error to allow user handling it */
>>> +    arg->reserved = state->error_code;
>> Once we do this we paint ourselves into a corner.
>>
>> It's nice that we have this reserved field since it allows us to
>> extend the atomic ioctl without the need to define a new one.
>>
>> When we discussed this at the Display Next Hackfest [1] we came
>> to the conclusion that we would want:
>>
>> - an enum to show what is the issue
>> - a string that can be logged to display verbose information
>>    about the failure
>> - an optional array of KMS object IDs participating in the failure
>>
>> We could define these in a new struct
>>
>> struct drm_mode_atomic2 {
>>     __u64 failure_reason;
>>     __u64 failure_string;
>>     __u32 drm_object_ids[MAX_FAILURE_OBJECT_IDS]
>>     __u64 reserved;
>> }
>>
>> that we link to via the drm_mode_atomic->reserved field.
>>
>> Your approach of flags, as opposed to an enum, would allow reporting
>> of multiple failures. Do we think drivers would ever make use of it?
>> Normally check short-circuits when a failure/limitation is hit.
> Thanks for the feedback. As pointed it would be good to have a struct pointed by the reserved variable, so that we can further extend the scope.
> W.r.t the enum, I feel its better to have bit notification as we can convey multiple errors if any.
> 
> Understand that the driver at present will return on the first error but upon adding this user readable error code can extend the driver to check add the properties provided by user and report error at once, so that multiple failure iterations can be overridden.
> 

I don't have a strong preference either way here. I'm happy with a
set of flags so we can report multiple errors at once.

> Having obj_id would be a good one, but when the flow goes to the vendor specific driver, they may not have the link to the obj_id where a failure is occurred. But still good to have so that at the early stage in set_prop, sanity check failures can be reported with obj_id so that it would be easy for the user to correct them in the next atomic ioctl.
> 

Right. This is entirely optional for scenarios where a driver (or
DRM core) can provided this additional info. It might make sense for
MST bandwidth failures where we could provide the MST connectors that
are involved, i.e., ones on the same physical connector.

It could possibly also be useful for movable HW blocks, which is not
something we can express right now. Things like a LUT that can be
assigned to any pipe. When userspace tries enabling the property that
uses the LUT but all are in use we might be able to signal what is
conflicting.

But again, entirely optional for scenarios where a driver thinks it
can provide better information. It also depends on how userspace will
implement this. I'm sure compositors don't want to overcomplicate their
failure handling.

Harry


> Will take care of these feedback in my next revision of patch set.
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> 

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

[Xaver Hugl]

Am Mi., 30. Juli 2025 um 12:36 Uhr schrieb Arun R Murthy
<arun.r.murthy@intel.com>:
>
> 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.Its a 64bit variable and
> should suffice 64 error codes that should be sufficient.
Hi, and thanks for working on this. Harry already mentioned what we
discussed at the hackfest, so I won't repeat that again :)

> +/* atomic not set in the drm_file */
> +#define DRM_MODE_ATOMIC_CAP_NOT_ENABLED                        BIT(0)
> +/* atomic flag passed not in DRM_MODE_ATOMIC_FLAGS list */
> +#define DRM_MODE_ATOMIC_INVALID_FLAG                   BIT(1)
> +/* DRM_MODE_PAGE_FLIP_LEGACY not supported with atomic ioctl  */
This should be DRM_MODE_PAGE_FLIP_ASYNC I think?
> +#define DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC                        BIT(2)
> +/* flip event with atomic check only not supported */
> +#define DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY      BIT(3)
> +/* atomic property change requested need full crtc modeset */
> +#define DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET         BIT(4)
> +/* atomic property change requested has impact on all connected crtc */
> +#define DRM_MODE_ATOMIC_NEED_FULL_MODESET              BIT(5)
> +/* async flip supported on only primary plane */
> +#define DRM_MODE_ATOMIC_ASYNC_NOT_PRIMARY              BIT(6)
This is a bit limiting when some but not all non-primary planes support async.
> +/* modifier not supported by async flip */
> +#define DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED   BIT(7)
> +/* async flip with pipe joiner not allowed */
> +#define DRM_MODE_ATOMIC_ASYNC_PIPEJOINER_NOTALLOWED    BIT(8)
I feel like this error is a bit too specific, or at least it needs
some more explanation - what will compositors do with it?

In general I want to mention that some of these errors are pretty
specific and not actionable for compositor code. Ideally the enum
would just be more generic things like
- invalid API usage
- needs modeset
- plane can't do async
- format/modifier can't do async (though with IN_FORMATS_ASYNC it's
kind of just "invalid API usage")
- scanout bandwidth
- connector bandwidth
- memory domain
- scanin bandwidth

which (except for "invalid API usage") compositor code can
automatically do something about, and the string that's passed to the
compositor can give more information for debugging and logging.

- Xaver

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

[Murthy, Arun R]

On 31-07-2025 17:02, Xaver Hugl wrote:
> Am Mi., 30. Juli 2025 um 12:36 Uhr schrieb Arun R Murthy
> <arun.r.murthy@intel.com>:
>> 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.Its a 64bit variable and
>> should suffice 64 error codes that should be sufficient.
> Hi, and thanks for working on this. Harry already mentioned what we
> discussed at the hackfest, so I won't repeat that again :)
Sure, will take care of them!
>> +/* atomic not set in the drm_file */
>> +#define DRM_MODE_ATOMIC_CAP_NOT_ENABLED                        BIT(0)
>> +/* atomic flag passed not in DRM_MODE_ATOMIC_FLAGS list */
>> +#define DRM_MODE_ATOMIC_INVALID_FLAG                   BIT(1)
>> +/* DRM_MODE_PAGE_FLIP_LEGACY not supported with atomic ioctl  */
> This should be DRM_MODE_PAGE_FLIP_ASYNC I think?
Sorry my bad, it should actually be interchanged!
>> +#define DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC                        BIT(2)
>> +/* flip event with atomic check only not supported */
>> +#define DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY      BIT(3)
>> +/* atomic property change requested need full crtc modeset */
>> +#define DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET         BIT(4)
>> +/* atomic property change requested has impact on all connected crtc */
>> +#define DRM_MODE_ATOMIC_NEED_FULL_MODESET              BIT(5)
>> +/* async flip supported on only primary plane */
>> +#define DRM_MODE_ATOMIC_ASYNC_NOT_PRIMARY              BIT(6)
> This is a bit limiting when some but not all non-primary planes support async.
>> +/* modifier not supported by async flip */
>> +#define DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED   BIT(7)
>> +/* async flip with pipe joiner not allowed */
>> +#define DRM_MODE_ATOMIC_ASYNC_PIPEJOINER_NOTALLOWED    BIT(8)
> I feel like this error is a bit too specific, or at least it needs
> some more explanation - what will compositors do with it?
Sure will add more description, compositor on getting this error will 
have to either use sync flip or reduce the resolution so as to not 
enable pipejoiner feature.
> In general I want to mention that some of these errors are pretty
> specific and not actionable for compositor code. Ideally the enum
> would just be more generic things like
> - invalid API usage
> - needs modeset
> - plane can't do async
> - format/modifier can't do async (though with IN_FORMATS_ASYNC it's
> kind of just "invalid API usage")
> - scanout bandwidth
> - connector bandwidth
> - memory domain
> - scanin bandwidth
>
> which (except for "invalid API usage") compositor code can
> automatically do something about, and the string that's passed to the
> compositor can give more information for debugging and logging.

Sure will add error string as pointer by Harry as well.

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