* [PATCH v10 1/7] drm: Define user readable error codes for atomic ioctl
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
@ 2026-02-23 9:15 ` Arun R Murthy
2026-02-24 0:26 ` Claude review: " Claude Code Review Bot
2026-02-23 9:16 ` [PATCH v10 2/7] drm/atomic: Add error_code element in atomic_state Arun R Murthy
` (7 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-02-23 9:15 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: Imported from, dri-devel, intel-gfx, intel-xe, Arun R Murthy,
Suraj Kandpal
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 retry. 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, thereby creation of new
uapi/interface for returning the error code is not required.
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 as a string can also be added
on need basis by the variable failure_string in the same struct
drm_mode_atomic_err_code.
v3: Remove fixed error (Jani/Xaver)
v5: Fix kernel-doc (Jani)
v7: Rephrase the kernel doc description (Suraj)
v8: Removed the below enum and suggest to use INVALID_API_USAGE (Xaver)
DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE
DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
include/uapi/drm/drm_mode.h | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 3693d82b5279f1cf14fc2adb538ea830591cc598..71e91d4f05ee8d556e8e7137a6cdcc7f3b261a75 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -45,6 +45,7 @@ extern "C" {
#define DRM_CONNECTOR_NAME_LEN 32
#define DRM_DISPLAY_MODE_LEN 32
#define DRM_PROP_NAME_LEN 32
+#define DRM_MODE_ATOMIC_FAILURE_STRING_LEN 128
#define DRM_MODE_TYPE_BUILTIN (1<<0) /* deprecated */
#define DRM_MODE_TYPE_CLOCK_C ((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
@@ -1343,6 +1344,42 @@ struct drm_mode_destroy_dumb {
DRM_MODE_ATOMIC_NONBLOCK |\
DRM_MODE_ATOMIC_ALLOW_MODESET)
+/**
+ * enum drm_mode_atomic_failure_codes - 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_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_PROP_CHANGED,
+};
+
+/**
+ * struct 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_objs_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
+ * @failure_string: user readable error message string
+ */
+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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: drm: Define user readable error codes for atomic ioctl
2026-02-23 9:15 ` [PATCH v10 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
@ 2026-02-24 0:26 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:26 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This patch defines the UAPI structures and enums for error reporting.
> +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_PROP_CHANGED,
> +};
`DRM_MODE_ATOMIC_INVALID_API_USAGE` has the value 0. Since the error struct is zero-initialized, userspace cannot distinguish "no error code was set" from "the error was invalid API usage." The enum should start at 1, or a sentinel/none value should be added at 0.
`DRM_MODE_ATOMIC_NEED_FULL_MODESET` is defined here but never used in any patch in this series. Is it intended for future use? If so, it should not be part of the initial UAPI -- adding unused UAPI values creates a commitment without any implementation to validate the semantics.
> + * @DRM_MODE_ATOMIC_INVALID_API_USAGE: invallid API usage(DRM_ATOMIC not
> + * enabled, invalid falg, page_flip event
Typos: "invallid" should be "invalid", "falg" should be "flag."
> + * @failure_code: error codes defined in enum drm_moide_atomic_failure_code
Typo: "drm_moide_atomic_failure_code" should be "drm_mode_atomic_failure_codes."
> +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];
> +};
`failure_objs_ptr`, `count_objs`, and `reserved` are defined here but never populated by any patch in this series. For UAPI, it's better to add fields when they are actually implemented. Shipping dead fields creates an API contract without any way to validate it.
The `reserved` field in a new UAPI struct must be validated to be zero by the kernel if it's intended for future extension. No such validation exists.
The struct has a `__u32 count_objs` followed by a `char` array, which means the total struct size is 156 bytes (not a multiple of 8). While this works, adding an explicit `__u32 pad` after `count_objs` would be more consistent with DRM UAPI conventions.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 2/7] drm/atomic: Add error_code element in atomic_state
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
2026-02-23 9:15 ` [PATCH v10 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
@ 2026-02-23 9:16 ` Arun R Murthy
2026-02-24 0:26 ` Claude review: " Claude Code Review Bot
2026-02-23 9:16 ` [PATCH v10 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done Arun R Murthy
` (6 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-02-23 9:16 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: Imported from, dri-devel, intel-gfx, intel-xe, Arun R Murthy,
Suraj Kandpal
Now that a proper error code will be returned to the user on any failure
in atomic_ioctl() via struct drm_mode_atomic, add a new element
error_code in the struct drm_atomic_state so as to hold the error code
during the atomic_check() and atomic_commit() phases.
New function added to print the error message and fill the struct
err_code with proper error message and error code.
v5: Add a function for printing the error message and filling err_code
struct
v6: Replace drm_err with drm_dbg_atomic print
v6: Add keyword "commit failed" in dbg msg (Suraj)
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
drivers/gpu/drm/drm_atomic.c | 31 +++++++++++++++++++++++++++++++
include/drm/drm_atomic.h | 10 ++++++++++
2 files changed, 41 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 04925166df989bcb30b111739aa4ed5c84f3a5ae..5fd5cc4da99cea51c06da3e5f5603d9c018d4600 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2122,6 +2122,37 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p)
}
EXPORT_SYMBOL(drm_state_dump);
+/**
+ * drm_mode_atomic_add_error_msg - function to add error code and error string
+ *
+ * @err_code: pointer to struct drm_mode_atomic_err_code that stores the failure
+ * reason
+ * @failure_code: failure code in enum drm_mode_atomic_failure_codes
+ * @failure_string: failure reason string message
+ *
+ * Returns: void
+ */
+void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
+ __u64 failure_code, const char *format, ...)
+{
+ struct drm_atomic_state *state = container_of(err_code,
+ struct drm_atomic_state,
+ error_code);
+ va_list varg;
+ char *failure_string;
+
+ err_code->failure_code = failure_code;
+
+ va_start(varg, format);
+ failure_string = kvasprintf(GFP_ATOMIC, format, varg);
+
+ drm_dbg_atomic(state->dev, "Commit failed: %s\n", failure_string);
+ strscpy_pad(err_code->failure_string, failure_string,
+ sizeof(err_code->failure_string));
+ va_end(varg);
+}
+EXPORT_SYMBOL(drm_mode_atomic_add_error_msg);
+
#ifdef CONFIG_DEBUG_FS
static int drm_state_info(struct seq_file *m, void *data)
{
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 0b1b32bcd2bda1b92299fd369ba7c23b1c2d3dfa..e26e68a955d33c2f7caa50b89ccb7d66c2c430d8 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -646,6 +646,13 @@ struct drm_atomic_state {
* commit without blocking.
*/
struct work_struct commit_work;
+
+ /* @error_code: pointer to struct holding failure reason and string
+ *
+ * struct to convey user readable error to the user.
+ * Error codes defined in enum drm_mode_atomic_failure_flags
+ */
+ struct drm_mode_atomic_err_code error_code;
};
void __drm_crtc_commit_free(struct kref *kref);
@@ -1373,5 +1380,8 @@ drm_atomic_get_old_bridge_state(const struct drm_atomic_state *state,
struct drm_bridge_state *
drm_atomic_get_new_bridge_state(const struct drm_atomic_state *state,
struct drm_bridge *bridge);
+__printf(3, 4)
+void drm_mode_atomic_add_error_msg(struct drm_mode_atomic_err_code *err_code,
+ __u64 failure_code, const char *format, ...);
#endif /* DRM_ATOMIC_H_ */
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: drm/atomic: Add error_code element in atomic_state
2026-02-23 9:16 ` [PATCH v10 2/7] drm/atomic: Add error_code element in atomic_state Arun R Murthy
@ 2026-02-24 0:26 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:26 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
> + failure_string = kvasprintf(GFP_ATOMIC, format, varg);
> +
> + drm_dbg_atomic(state->dev, "Commit failed: %s\n", failure_string);
> + strscpy_pad(err_code->failure_string, failure_string,
> + sizeof(err_code->failure_string));
> + va_end(varg);
Two bugs here. First, `kvasprintf()` allocates memory that is **never freed with `kfree()`**. Every call to this function leaks memory. Second, if `kvasprintf()` returns NULL (allocation failure), both `drm_dbg_atomic` and `strscpy_pad` will dereference a NULL pointer.
The fix is to add `kfree(failure_string)` after `strscpy_pad`, and to handle the NULL case. Alternatively, since the goal is just to format into a fixed-size buffer, `vsnprintf` directly into `err_code->failure_string` would avoid the allocation entirely and be simpler.
> + failure_string = kvasprintf(GFP_ATOMIC, format, varg);
`GFP_ATOMIC` is for interrupt/spinlock context. This function is called from ioctl handlers (process context), so `GFP_KERNEL` would be appropriate. But as noted above, the allocation can be avoided entirely.
> + /* @error_code: pointer to struct holding failure reason and string
> + *
> + * struct to convey user readable error to the user.
> + * Error codes defined in enum drm_mode_atomic_failure_flags
> + */
> + struct drm_mode_atomic_err_code error_code;
The kernel-doc says "pointer to struct" but this is an embedded struct, not a pointer. Also references `drm_mode_atomic_failure_flags` which doesn't exist -- it should be `drm_mode_atomic_failure_codes`. The comment is missing the `/**` kernel-doc opening marker (uses `/*` instead).
Embedding a UAPI struct (`struct drm_mode_atomic_err_code` from `drm_mode.h`) directly in a kernel-internal struct (`struct drm_atomic_state`) is unusual. The UAPI types (`__u64`, `char[]`) are unnecessary for kernel-internal storage. A more conventional approach would be to use kernel types internally and convert when copying to userspace.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
2026-02-23 9:15 ` [PATCH v10 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
2026-02-23 9:16 ` [PATCH v10 2/7] drm/atomic: Add error_code element in atomic_state Arun R Murthy
@ 2026-02-23 9:16 ` Arun R Murthy
2026-02-24 0:26 ` Claude review: " Claude Code Review Bot
2026-02-23 9:16 ` [PATCH v10 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl Arun R Murthy
` (5 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-02-23 9:16 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: Imported from, dri-devel, intel-gfx, intel-xe, Arun R Murthy,
Suraj Kandpal
Upon returning valid error code on atomic_ioctl failure, changes have
been done to goto error/out in cases of error instead of returining to
accommodate returning the failure codes. As part of this change
complete_signaling() will be called as part of cleanup. Check if the
fences are initialized/prepared before completing.
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
drivers/gpu/drm/drm_atomic_uapi.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 87de41fb445931ff8383b45839fca63123026f8e..ac84b9a9c25844985816b9c0843c44e6e763d048 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1569,7 +1569,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_modeset_acquire_ctx ctx;
struct drm_out_fence_state *fence_state;
int ret = 0;
- unsigned int i, j, num_fences;
+ unsigned int i, j, num_fences = 0;
bool async_flip = false;
/* disallow for drivers not supporting atomic: */
@@ -1719,7 +1719,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
}
out:
- complete_signaling(dev, state, fence_state, num_fences, !ret);
+ if (num_fences)
+ complete_signaling(dev, state, fence_state, num_fences, !ret);
if (ret == -EDEADLK) {
drm_atomic_state_clear(state);
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: drm/atomic: Call complete_signaling only if prepare_signaling is done
2026-02-23 9:16 ` [PATCH v10 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done Arun R Murthy
@ 2026-02-24 0:26 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:26 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
> - unsigned int i, j, num_fences;
> + unsigned int i, j, num_fences = 0;
> - complete_signaling(dev, state, fence_state, num_fences, !ret);
> + if (num_fences)
> + complete_signaling(dev, state, fence_state, num_fences, !ret);
This change is correct in isolation. Before this series, `complete_signaling` was always called after `prepare_signaling`. With the state allocation moved earlier (patch 4), early `goto out` paths can reach the cleanup before `prepare_signaling` runs. Initializing `num_fences = 0` and guarding the call is the right approach.
I verified that `complete_signaling` cleans up both fence state and crtc events. When `prepare_signaling` was never called, there are no events or fences to clean up, so skipping the call when `num_fences == 0` is safe. The events and fences are only created together in `prepare_signaling`, and `num_fences` is incremented for each one.
No issues found in this patch.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (2 preceding siblings ...)
2026-02-23 9:16 ` [PATCH v10 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done Arun R Murthy
@ 2026-02-23 9:16 ` Arun R Murthy
2026-02-24 0:27 ` Claude review: " Claude Code Review Bot
2026-02-23 9:16 ` [PATCH v10 5/7] drm/atomic: Return user readable error in atomic_ioctl Arun R Murthy
` (4 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-02-23 9:16 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: Imported from, dri-devel, intel-gfx, intel-xe, Arun R Murthy,
Suraj Kandpal
Move atomic_state allocation to the beginning of the atomic_ioctl
to accommodate drm_mode_atomic_err_code usage for returning error
code on failures.
As atomic state is required for drm_mode_atomic_err_code to store the
error codes.
v7: Reframe commit message (Suraj)
v8: Moved the clearing fence change to a different patch (Suraj/Louis)
v9: Free allocated atomic_state before return on error, move this change
from patch 5 (Suraj)
v10: Re-order, exchange 3 and 4th patch to ensure cleanup is done in
order (Suraj)
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
drivers/gpu/drm/drm_atomic_uapi.c | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index ac84b9a9c25844985816b9c0843c44e6e763d048..f295a397c26c24c6414a25bacff051bd694260c5 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1576,6 +1576,14 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
return -EOPNOTSUPP;
+ state = drm_atomic_state_alloc(dev);
+ if (!state)
+ return -ENOMEM;
+
+ drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
+ state->acquire_ctx = &ctx;
+ state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+
/* disallow for userspace that has not enabled atomic cap (even
* though this may be a bit overkill, since legacy userspace
* wouldn't know how to call this ioctl)
@@ -1583,24 +1591,28 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
if (!file_priv->atomic) {
drm_dbg_atomic(dev,
"commit failed: atomic cap not enabled\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
drm_dbg_atomic(dev, "commit failed: invalid flag\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
if (arg->reserved) {
drm_dbg_atomic(dev, "commit failed: reserved field set\n");
- return -EINVAL;
+ 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;
+ ret = -EINVAL;
+ goto out;
}
async_flip = true;
@@ -1611,16 +1623,10 @@ 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");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
- state = drm_atomic_state_alloc(dev);
- if (!state)
- return -ENOMEM;
-
- drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
- state->acquire_ctx = &ctx;
- state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
state->plane_color_pipeline = file_priv->plane_color_pipeline;
retry:
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl
2026-02-23 9:16 ` [PATCH v10 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl Arun R Murthy
@ 2026-02-24 0:27 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:27 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
> + state = drm_atomic_state_alloc(dev);
> + if (!state)
> + return -ENOMEM;
> +
> + drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> + state->acquire_ctx = &ctx;
> + state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
> +
> /* disallow for userspace that has not enabled atomic cap (even
Moving the allocation earlier means every ioctl call that fails early validation (bad flags, atomic cap not enabled, etc.) now incurs the overhead of allocating and freeing an atomic state and initializing/finalizing the acquire context. Previously these were cheap early returns. This is a performance regression on common error paths, though probably not significant in practice.
The `goto out` cleanup path calls `drm_atomic_state_put(state)` and `drm_modeset_drop_locks(&ctx)` / `drm_modeset_acquire_fini(&ctx)`. I verified that these are safe to call even when no locks were acquired. This is correct.
No correctness bugs found here.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 5/7] drm/atomic: Return user readable error in atomic_ioctl
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (3 preceding siblings ...)
2026-02-23 9:16 ` [PATCH v10 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl Arun R Murthy
@ 2026-02-23 9:16 ` Arun R Murthy
2026-02-24 0:27 ` Claude review: " Claude Code Review Bot
2026-02-23 9:16 ` [PATCH v10 6/7] drm/i915/display: Error codes for async flip failures Arun R Murthy
` (3 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-02-23 9:16 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: Imported from, dri-devel, intel-gfx, intel-xe, Arun R Murthy,
Suraj Kandpal
Add user readable error codes for failure cases in drm_atomic_ioctl() so
that user can decode the error code and take corrective measurements.
v8: Replaced DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP with INVALID_API_USAGE
(Xaver)
v9: Move free atomic_state on error to patch 3 (Suraj)
v10: on copy_to_user error free atomic_state and drop acquired locks
(Suraj)
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
drivers/gpu/drm/drm_atomic_uapi.c | 58 +++++++++++++++++++++++++++++----------
1 file changed, 44 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index f295a397c26c24c6414a25bacff051bd694260c5..0a4a8e8033d21a78efaeda0dca3e018dc3ae542a 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1196,6 +1196,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
ret = drm_atomic_connector_get_property(connector, connector_state,
prop, &old_val);
ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ if (ret) {
+ drm_mode_atomic_add_error_msg(&state->error_code,
+ DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
+ "property change not allowed in async flip");
+ }
break;
}
@@ -1218,6 +1223,11 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
ret = drm_atomic_crtc_get_property(crtc, crtc_state,
prop, &old_val);
ret = drm_atomic_check_prop_changes(ret, old_val, prop_value, prop);
+ if (ret) {
+ drm_mode_atomic_add_error_msg(&state->error_code,
+ DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
+ "property change not allowed in async flip");
+ }
break;
}
@@ -1256,9 +1266,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
ret = plane_funcs->atomic_async_check(plane, state, true);
if (ret) {
- drm_dbg_atomic(prop->dev,
- "[PLANE:%d:%s] does not support async flips\n",
- obj->id, plane->name);
+ drm_mode_atomic_add_error_msg(&state->error_code,
+ DRM_MODE_ATOMIC_INVALID_API_USAGE,
+ "[PLANE:%d:%s] does not support async flip",
+ obj->id, plane->name);
break;
}
}
@@ -1568,6 +1579,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_atomic_state *state;
struct drm_modeset_acquire_ctx ctx;
struct drm_out_fence_state *fence_state;
+ struct drm_mode_atomic_err_code __user *error_code_ptr;
int ret = 0;
unsigned int i, j, num_fences = 0;
bool async_flip = false;
@@ -1576,6 +1588,14 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
return -EOPNOTSUPP;
+ if (!arg->reserved)
+ drm_dbg_atomic(dev,
+ "memory not allocated for drm_atomic error reporting\n");
+ else
+ /* Update the error code if any error to allow user handling it */
+ error_code_ptr = (struct drm_mode_atomic_err_code __user *)
+ (unsigned long)arg->reserved;
+
state = drm_atomic_state_alloc(dev);
if (!state)
return -ENOMEM;
@@ -1584,11 +1604,16 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
state->acquire_ctx = &ctx;
state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+ memset(&state->error_code, 0, sizeof(*error_code_ptr));
+
/* disallow for userspace that has not enabled atomic cap (even
* though this may be a bit overkill, since legacy userspace
* wouldn't know how to call this ioctl)
*/
if (!file_priv->atomic) {
+ drm_mode_atomic_add_error_msg(&state->error_code,
+ DRM_MODE_ATOMIC_INVALID_API_USAGE,
+ "drm atomic capability not enabled");
drm_dbg_atomic(dev,
"commit failed: atomic cap not enabled\n");
ret = -EINVAL;
@@ -1596,21 +1621,18 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
}
if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
- drm_dbg_atomic(dev, "commit failed: invalid flag\n");
- ret = -EINVAL;
- goto out;
- }
-
- if (arg->reserved) {
- drm_dbg_atomic(dev, "commit failed: reserved field set\n");
+ drm_mode_atomic_add_error_msg(&state->error_code,
+ DRM_MODE_ATOMIC_INVALID_API_USAGE,
+ "invalid flag");
ret = -EINVAL;
goto out;
}
if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
if (!dev->mode_config.async_page_flip) {
- drm_dbg_atomic(dev,
- "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not supported\n");
+ drm_mode_atomic_add_error_msg(&state->error_code,
+ DRM_MODE_ATOMIC_INVALID_API_USAGE,
+ "DRM_MODE_PAGE_FLIP_ASYNC not supported with ATOMIC ioctl");
ret = -EINVAL;
goto out;
}
@@ -1621,8 +1643,9 @@ 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");
+ drm_mode_atomic_add_error_msg(&state->error_code,
+ DRM_MODE_ATOMIC_INVALID_API_USAGE,
+ "page-flip event requested with test-only commit");
ret = -EINVAL;
goto out;
}
@@ -1725,6 +1748,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
}
out:
+ /* Update the error code if any error to allow user handling it */
+ if (ret < 0 && arg->reserved) {
+ if (copy_to_user(error_code_ptr, &state->error_code,
+ sizeof(state->error_code)))
+ ret = -EFAULT;
+ }
+
if (num_fences)
complete_signaling(dev, state, fence_state, num_fences, !ret);
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: drm/atomic: Return user readable error in atomic_ioctl
2026-02-23 9:16 ` [PATCH v10 5/7] drm/atomic: Return user readable error in atomic_ioctl Arun R Murthy
@ 2026-02-24 0:27 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:27 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This is the main functional patch and has several issues.
> + if (!arg->reserved)
> + drm_dbg_atomic(dev,
> + "memory not allocated for drm_atomic error reporting\n");
> + else
> + /* Update the error code if any error to allow user handling it */
> + error_code_ptr = (struct drm_mode_atomic_err_code __user *)
> + (unsigned long)arg->reserved;
Emitting a debug message every time userspace does NOT opt into error reporting seems backwards. Most userspace (including all existing userspace) will not set `reserved`, so this will spam the debug log on every successful atomic ioctl call. This debug message should be removed.
> + memset(&state->error_code, 0, sizeof(*error_code_ptr));
When `arg->reserved` is 0, `error_code_ptr` is uninitialized. Using `sizeof(*error_code_ptr)` works because `sizeof` is evaluated at compile time, but this is confusing. It should be `sizeof(state->error_code)` for clarity.
> + if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
> + drm_mode_atomic_add_error_msg(&state->error_code,
> + DRM_MODE_ATOMIC_INVALID_API_USAGE,
> + "invalid flag");
> ret = -EINVAL;
> goto out;
> }
The patch removes the `drm_dbg_atomic` call for the invalid flags case, replacing it solely with the error_msg function. But `drm_mode_atomic_add_error_msg` calls `drm_dbg_atomic` internally, so the debug output is preserved. This is fine, though the other error paths (e.g., "atomic cap not enabled") keep both the explicit `drm_dbg_atomic` AND the error_msg call, which results in double debug output for those paths. The approach should be consistent.
> - if (arg->reserved) {
> - drm_dbg_atomic(dev, "commit failed: reserved field set\n");
> - ret = -EINVAL;
> - goto out;
> - }
The `reserved` field validation is removed because the field is now repurposed. This relies on old userspace always passing 0 in `reserved`. Since the old kernel rejected non-zero values, well-behaved userspace should indeed pass 0. However, the safer pattern used elsewhere in DRM is to add a new flag to opt into the new behavior, rather than silently reinterpreting a previously-validated field.
> +out:
> + /* Update the error code if any error to allow user handling it */
> + if (ret < 0 && arg->reserved) {
> + if (copy_to_user(error_code_ptr, &state->error_code,
> + sizeof(state->error_code)))
> + ret = -EFAULT;
> + }
This runs on the `-EDEADLK` path before the deadlock recovery check. If `copy_to_user` fails (e.g., the user pointer is invalid), `ret` is changed to `-EFAULT`, and the `ret == -EDEADLK` check below will be false. This means a bad error pointer will **prevent deadlock recovery**, turning a transient retry into a permanent failure. The `copy_to_user` should be skipped for `-EDEADLK`, or the EDEADLK check should be moved before the copy_to_user.
Also, on the `-EDEADLK` retry path, this writes potentially-stale error data to userspace (from a failed attempt that will be retried). The retry might succeed, but userspace has already received error data.
> + if (ret) {
> + drm_mode_atomic_add_error_msg(&state->error_code,
> + DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> + "property change not allowed in async flip");
> + }
In `drm_atomic_set_property`, the error message is added for connector and crtc async property changes. But looking at the code flow, the `drm_atomic_check_prop_changes` call is made only inside the `if (async_flip)` block. The error message should include which property or object triggered it, similar to how the plane case includes `obj->id` and `plane->name`.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 6/7] drm/i915/display: Error codes for async flip failures
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (4 preceding siblings ...)
2026-02-23 9:16 ` [PATCH v10 5/7] drm/atomic: Return user readable error in atomic_ioctl Arun R Murthy
@ 2026-02-23 9:16 ` Arun R Murthy
2026-02-24 0:27 ` Claude review: " Claude Code Review Bot
2026-02-23 9:16 ` [PATCH v10 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING Arun R Murthy
` (2 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-02-23 9:16 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: Imported from, dri-devel, intel-gfx, intel-xe, Arun R Murthy,
Suraj Kandpal
For failures in async flip atomic check/commit path return user readable
error codes in struct drm_atomic_state.
v8: Replaced DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP with INVALUD_API_USAGE
(Xaver)
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 33821cc2948b6ea1f96a8adceb459a08c255d9e3..b648dbe1f0b3c70eef093f32e3e35d9187961d94 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6055,9 +6055,10 @@ static int intel_async_flip_check_uapi(struct intel_atomic_state *state,
}
if (intel_crtc_needs_modeset(new_crtc_state)) {
- drm_dbg_kms(display->drm,
- "[CRTC:%d:%s] modeset required\n",
- crtc->base.base.id, crtc->base.name);
+ drm_mode_atomic_add_error_msg(&state->base.error_code,
+ DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+ "[CRTC:%d:%s] requires full modeset",
+ crtc->base.base.id, crtc->base.name);
return -EINVAL;
}
@@ -6124,9 +6125,10 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
}
if (intel_crtc_needs_modeset(new_crtc_state)) {
- drm_dbg_kms(display->drm,
- "[CRTC:%d:%s] modeset required\n",
- crtc->base.base.id, crtc->base.name);
+ drm_mode_atomic_add_error_msg(&state->base.error_code,
+ DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
+ "[CRTC:%d:%s] requires full modeset",
+ crtc->base.base.id, crtc->base.name);
return -EINVAL;
}
@@ -6164,11 +6166,12 @@ static int intel_async_flip_check_hw(struct intel_atomic_state *state, struct in
if (!intel_plane_can_async_flip(plane, new_plane_state->hw.fb->format,
new_plane_state->hw.fb->modifier)) {
- drm_dbg_kms(display->drm,
- "[PLANE:%d:%s] pixel format %p4cc / modifier 0x%llx does not support async flip\n",
- plane->base.base.id, plane->base.name,
- &new_plane_state->hw.fb->format->format,
- new_plane_state->hw.fb->modifier);
+ drm_mode_atomic_add_error_msg(&state->base.error_code,
+ DRM_MODE_ATOMIC_INVALID_API_USAGE,
+ "[PLANE:%d:%s] pixel format %p4cc / 0x%llx modifier does not support async flip",
+ plane->base.base.id, plane->base.name,
+ &new_plane_state->hw.fb->format->format,
+ new_plane_state->hw.fb->modifier);
return -EINVAL;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: drm/i915/display: Error codes for async flip failures
2026-02-23 9:16 ` [PATCH v10 6/7] drm/i915/display: Error codes for async flip failures Arun R Murthy
@ 2026-02-24 0:27 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:27 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
> + drm_mode_atomic_add_error_msg(&state->base.error_code,
> + DRM_MODE_ATOMIC_INVALID_API_USAGE,
> + "[PLANE:%d:%s] pixel format %p4cc / 0x%llx modifier does not support async flip",
> + plane->base.base.id, plane->base.name,
> + &new_plane_state->hw.fb->format->format,
> + new_plane_state->hw.fb->modifier);
The `%p4cc` format specifier is a kernel printk extension. While `kvasprintf` (used internally) does support it, the resulting string is copied to userspace. The kernel %p extensions produce kernel-internal formatting that may change between kernel versions. Passing kernel-formatted strings to userspace as a stable API is fragile. Consider using a plain hex value instead.
The conversion is also incomplete. Only 3 out of many error cases in `intel_async_flip_check_uapi` and `intel_async_flip_check_hw` have been converted. For example, the "not active", "joiner disallowed", "no old or new framebuffer", "stride cannot change", "modifier cannot change", etc. cases all still use `drm_dbg_kms`. If the feature is to be useful, it should cover all error paths, not just a few.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v10 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (5 preceding siblings ...)
2026-02-23 9:16 ` [PATCH v10 6/7] drm/i915/display: Error codes for async flip failures Arun R Murthy
@ 2026-02-23 9:16 ` Arun R Murthy
2026-02-24 0:27 ` Claude review: " Claude Code Review Bot
2026-02-23 10:07 ` [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Murthy, Arun R
2026-02-24 0:26 ` Claude review: " Claude Code Review Bot
8 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-02-23 9:16 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: Imported from, dri-devel, intel-gfx, intel-xe, Arun R Murthy,
Suraj Kandpal
The new capability informs users that atomic_ioctl() supports
failure reporting when an error occurs.
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
drivers/gpu/drm/drm_ioctl.c | 3 +++
include/uapi/drm/drm.h | 7 +++++++
2 files changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ff193155129e7e863888d8958458978566b144f8..59f2b5b53830fd3aadc6e18cf49f0660a99e9c96 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -304,6 +304,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) &&
dev->mode_config.async_page_flip;
break;
+ case DRM_CAP_ATOMIC_ERROR_REPORTING:
+ req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) && 1;
+ break;
default:
return -EINVAL;
}
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 27cc159c1d275c7a7fe057840ef792f30a582bb7..6082410bcabfb4aa37b85e5f03d3611e5aed4aa5 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -812,6 +812,13 @@ struct drm_gem_change_handle {
* commits.
*/
#define DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP 0x15
+/**
+ * DRM_CAP_ATOMIC_ERROR_REPORTING
+ *
+ * If set to 1, the driver supports reporting of failure codes on error in
+ * atomic ioctl().
+ */
+#define DRM_CAP_ATOMIC_ERROR_REPORTING 0x16
/* DRM_IOCTL_GET_CAP ioctl argument type */
struct drm_get_cap {
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Claude review: drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING
2026-02-23 9:16 ` [PATCH v10 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING Arun R Murthy
@ 2026-02-24 0:27 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:27 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
> + case DRM_CAP_ATOMIC_ERROR_REPORTING:
> + req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) && 1;
> + break;
The `&& 1` is redundant. `drm_core_check_feature` already returns a boolean-like value. This appears to be copy-paste from `DRM_CAP_ATOMIC_ASYNC_PAGE_FLIP` where `&& dev->mode_config.async_page_flip` checks an actual driver flag.
More importantly, this reports error reporting as supported for ALL atomic drivers. But only i915 has been modified to populate any error codes (and even then, only for a few error paths). Compositors checking this capability will expect meaningful error information from any driver, but non-i915 drivers will return a zero-filled error struct (which looks like `DRM_MODE_ATOMIC_INVALID_API_USAGE` due to the enum-starts-at-0 issue). This makes the capability misleading.
The cover letter still contains multiple "EDITME" placeholders, and the "Changes in v9" section is unfilled. While not a code issue, this suggests the series was not properly reviewed before posting.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v10 0/7] User readable error codes on atomic_ioctl failure
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (6 preceding siblings ...)
2026-02-23 9:16 ` [PATCH v10 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING Arun R Murthy
@ 2026-02-23 10:07 ` Murthy, Arun R
2026-02-24 0:26 ` Claude review: " Claude Code Review Bot
8 siblings, 0 replies; 18+ messages in thread
From: Murthy, Arun R @ 2026-02-23 10:07 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jani Nikula, Rodrigo Vivi, Joonas Lahtinen,
Tvrtko Ursulin, xaver.hugl, harry.wentland, uma.shankar,
louis.chauvet, naveen1.kumar, ramya.krishna.yella
Cc: dri-devel, intel-gfx, intel-xe, Suraj Kandpal
Any other comments/feedback on this?
Thanks and Regards,
Arun R Murthy
-------------------
On 23-02-2026 14:45, Arun R Murthy wrote:
> EDITME: Imported from f20260210-atomic-v9-5-525c88fd2402@intel.com
> Please review before sending.
>
> 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.
>
> User will have to check for the capability
> DRM_CAP_ATOMIC_ERROR_REPORTING before using this feature so as to ensure
> that the driver supports failure reporting.
>
> TODO: driver specific error codes are to be added and will be done in
> the follow-up patches.
>
> TODO: Once the series is merged the element 'reserved' used for sending
> the failure code in struct drm_mode_atomic is to changed to err_code.
>
> The IGT related changes are pushed for review @
> https://patchwork.freedesktop.org/series/153330/
>
> [RFC] changes for libdrm pushed for review @
> https://gitlab.freedesktop.org/mesa/libdrm/-/merge_requests/450
>
> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> To: suraj.kandpal@intel.com>
> To: Maxime Ripard <mripard@kernel.org>
> To: Thomas Zimmermann <tzimmermann@suse.de>
> To: David Airlie <airlied@gmail.com>
> To: Simona Vetter <simona@ffwll.ch>
> To: Jani Nikula <jani.nikula@linux.intel.com>
> To: Rodrigo Vivi <rodrigo.vivi@intel.com>
> To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> To: Tvrtko Ursulin <tursulin@ursulin.net>
> To: xaver.hugl@kde.org
> To: harry.wentland@amd.com
> To: uma.shankar@intel.com
> To: louis.chauvet@bootlin.com
> To: naveen1.kumar@intel.com
> To: ramya.krishna.yella@intel.com
> Cc: dri-devel@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
> Changes in v9:
> - EDITME: describe what is new in this series revision.
> - EDITME: use bulletpoints and terse descriptions.
> - Link to v8: https://lore.kernel.org/r/20260129-atomic-v8-0-4cb7b0faa051@intel.com
>
> ---
> Arun R Murthy (7):
> drm: Define user readable error codes for atomic ioctl
> drm/atomic: Add error_code element in atomic_state
> drm/atomic: Call complete_signaling only if prepare_signaling is done
> drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl
> drm/atomic: Return user readable error in atomic_ioctl
> drm/i915/display: Error codes for async flip failures
> drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING
>
> drivers/gpu/drm/drm_atomic.c | 31 ++++++++++
> drivers/gpu/drm/drm_atomic_uapi.c | 89 ++++++++++++++++++++--------
> drivers/gpu/drm/drm_ioctl.c | 3 +
> drivers/gpu/drm/i915/display/intel_display.c | 25 ++++----
> include/drm/drm_atomic.h | 10 ++++
> include/uapi/drm/drm.h | 7 +++
> include/uapi/drm/drm_mode.h | 37 ++++++++++++
> 7 files changed, 165 insertions(+), 37 deletions(-)
> ---
> base-commit: cec43d5c2696af219fc2ef71dd7e93db48c80f66
> change-id: 20250728-atomic-c9713fd357e4
>
> Best regards,
^ permalink raw reply [flat|nested] 18+ messages in thread* Claude review: User readable error codes on atomic_ioctl failure
2026-02-23 9:15 [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (7 preceding siblings ...)
2026-02-23 10:07 ` [PATCH v10 0/7] User readable error codes on atomic_ioctl failure Murthy, Arun R
@ 2026-02-24 0:26 ` Claude Code Review Bot
8 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-24 0:26 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: User readable error codes on atomic_ioctl failure
Author: Arun R Murthy <arun.r.murthy@intel.com>
Patches: 9
Reviewed: 2026-02-24T10:26:59.069405
---
This series adds user-readable error codes to the DRM atomic ioctl by repurposing the `reserved` field of `struct drm_mode_atomic` as a pointer to a new `struct drm_mode_atomic_err_code`. When the atomic ioctl fails, the kernel populates this struct with an error code enum and a human-readable error string, then copies it back to userspace. A new capability `DRM_CAP_ATOMIC_ERROR_REPORTING` is added so userspace can discover support.
The idea of providing richer error information from atomic commits has value, particularly for compositor developers debugging modeset failures. However, the implementation has several significant issues that need to be addressed before this can be merged.
The most critical bug is a **memory leak** in `drm_mode_atomic_add_error_msg`: `kvasprintf()` allocates a string that is never freed. This leaks memory on every error path. There is also a **NULL pointer dereference** if that allocation fails. Beyond that, the UAPI design has problems: the failure code enum starts at 0, making it impossible for userspace to distinguish "no error was reported" from `DRM_MODE_ATOMIC_INVALID_API_USAGE`. The `copy_to_user` on the `-EDEADLK` retry path can abort deadlock recovery if the user pointer is bad. Several fields in the new UAPI struct (`failure_objs_ptr`, `count_objs`, `reserved`) are never populated by any patch. The cover letter still contains "EDITME" placeholders, and the kernel-doc has multiple typos. The capability in patch 7 reports error reporting as supported for all atomic drivers, even though only i915 populates any error codes.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread