* Claude review: drm: Define user readable error codes for atomic ioctl
2026-02-10 9:03 ` [PATCH v9 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
@ 2026-02-11 6:31 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-02-11 6:31 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Issues:**
```c
+/**
+ * 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)
```
- **Typo**: "invallid" should be "invalid"
- **Typo**: "falg" should be "flag"
- **Style**: Missing space after "usage" and before the opening parenthesis
```c
+ * @DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET: Need full modeset on this crtc
```
- **Consistency**: "Need" should be lowercase to match kernel-doc style
```c
+/**
+ * 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.
```
- **Missing @**: First line should mention "struct to store" in the brief, the detailed description should be below the field descriptions
- Unconventional structure for kernel-doc
```c
+ * @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"
```c
+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];
+};
```
- **ABI Concern**: `failure_objs_ptr` is documented as a pointer but using it from kernel to userspace is complex. How is memory management handled? Who allocates? This needs clarification.
- **Padding**: Structure has implicit padding after `count_objs` before `failure_string`. Should be explicit or reordered.
- **String Safety**: Fixed-size string without explicit null-termination guarantee is risky.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [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 v11 0/7] User readable error codes on atomic_ioctl failure
@ 2026-03-31 9:03 Arun R Murthy
2026-03-31 9:03 ` [PATCH v11 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, Arun R Murthy, Suraj Kandpal
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
[DRAFT] KWin changes are pushed for review @
https://invent.kde.org/plasma/kwin/-/merge_requests/8991
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 | 56 +++++++++++++++++
7 files changed, 184 insertions(+), 37 deletions(-)
---
base-commit: 65b94118bc585aefc7095c0e9554c3bb12d98a69
change-id: 20250728-atomic-c9713fd357e4
Best regards,
--
Arun R Murthy <arun.r.murthy@intel.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v11 1/7] drm: Define user readable error codes for atomic ioctl
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
@ 2026-03-31 9:03 ` Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 2/7] drm/atomic: Add error_code element in atomic_state Arun R Murthy
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, 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
v10: Added more error codes for the enum
v11: Add default/unspecified error code
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 | 56 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index a4bdc4bd11bc142e9d3b172397e18a1909a21488..8bf5fd8533912dc7a188aad19cc3741dd2099592 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -48,6 +48,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 */
@@ -1346,6 +1347,61 @@ 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_UNSPECIFIED_ERROR: this is the default/unspecified error.
+ * @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_NEED_FULL_MODESET: Need full modeset on all connected crtc's
+ * @DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED: Property changed in async flip
+ * @DRM_MODE_ATOMIC_SCANOUT_BW: For a given resolution, refresh rate and the
+ * color depth cannot be accomodated. Resolution
+ * is to lower the refresh rate or color depth.
+ * @DRM_MODE_ATOMIC_CONNECTOR_BW: Refers to the limitation on the link rate on
+ * a given connector.
+ * @DRM_MODE_ATOMIC_PIPE_BW: Limitation on the pipe, either pipe not available
+ * or the pipe scaling factor limitation.
+ * @DRM_MODE_ATOMIC_MEMORY_DOMAIN: Any other memory/bandwidth related limitation
+ * other then the ones specified above.
+ * @DRM_MODE_ATOMIC_SPEC_VIOLOATION: Limitation of a particular feature on that
+ * hardware. To get to know the feature, the
+ * property/object causing this is being sent
+ * back to user @failure_objs_ptr in the
+ * struct drm_mode_atomic_err_code
+ */
+enum drm_mode_atomic_failure_codes {
+ DRM_MODE_ATOMIC_UNSPECIFIED_ERROR,
+ DRM_MODE_ATOMIC_INVALID_API_USAGE,
+ DRM_MODE_ATOMIC_NEED_FULL_MODESET,
+ DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
+ DRM_MODE_ATOMIC_SCANOUT_BW,
+ DRM_MODE_ATTOMIC_CONNECTOR_BW,
+ DRM_MODE_ATTOMIC_PIPE_BW,
+ DRM_MODE_ATOMIC_MEMORY_DOMAIN,
+ DRM_MODE_ATOMIC_SPEC_VIOLOATION,
+};
+
+/**
+ * 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
* [PATCH v11 2/7] drm/atomic: Add error_code element in atomic_state
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
2026-03-31 9:03 ` [PATCH v11 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
@ 2026-03-31 9:03 ` Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done Arun R Murthy
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, 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 41c57063f3b4d6d6e61a274d818844fa9bd582bf..647b31a432bf55852aeac47ce29f9bfb3fcb6830 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2118,6 +2118,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 f03cd199aee73fa8e15b2d9e16a53d134fc7de7d..6fd43b31e8482550cc93273437cb46b8d44c800a 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);
@@ -1372,5 +1379,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
* [PATCH v11 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
2026-03-31 9:03 ` [PATCH v11 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
2026-03-31 9:03 ` [PATCH v11 2/7] drm/atomic: Add error_code element in atomic_state Arun R Murthy
@ 2026-03-31 9:03 ` Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl Arun R Murthy
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, 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 5bd5bf6661df7f6cefae616970a99a4b04de7121..64e33aeb9ee2d553b1247cfb9a11e17d8ddf1e86 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1573,7 +1573,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: */
@@ -1723,7 +1723,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
* [PATCH v11 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (2 preceding siblings ...)
2026-03-31 9:03 ` [PATCH v11 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done Arun R Murthy
@ 2026-03-31 9:03 ` Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 5/7] drm/atomic: Return user readable error in atomic_ioctl Arun R Murthy
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, 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 64e33aeb9ee2d553b1247cfb9a11e17d8ddf1e86..a0227b4ca57187624d17cdda24c4da47916c3467 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1580,6 +1580,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)
@@ -1587,24 +1595,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;
@@ -1615,16 +1627,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
* [PATCH v11 5/7] drm/atomic: Return user readable error in atomic_ioctl
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (3 preceding siblings ...)
2026-03-31 9:03 ` [PATCH v11 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl Arun R Murthy
@ 2026-03-31 9:03 ` Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 6/7] drm/i915/display: Error codes for async flip failures Arun R Murthy
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, 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 a0227b4ca57187624d17cdda24c4da47916c3467..14d65d40efc8014fd97c1afa2cdc20ba6003d08c 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1200,6 +1200,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;
}
@@ -1222,6 +1227,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;
}
@@ -1260,9 +1270,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;
}
}
@@ -1572,6 +1583,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;
@@ -1580,6 +1592,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;
@@ -1588,11 +1608,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;
@@ -1600,21 +1625,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;
}
@@ -1625,8 +1647,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;
}
@@ -1729,6 +1752,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
* [PATCH v11 6/7] drm/i915/display: Error codes for async flip failures
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (4 preceding siblings ...)
2026-03-31 9:03 ` [PATCH v11 5/7] drm/atomic: Return user readable error in atomic_ioctl Arun R Murthy
@ 2026-03-31 9:03 ` Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING Arun R Murthy
2026-03-31 21:55 ` Claude review: User readable error codes on atomic_ioctl failure Claude Code Review Bot
7 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, 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 ee501009a251f18c1c14b6df5c267b7f761871ab..a4cf06f6c74e79eb6429bf8c710ac21cf792c3e3 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6053,9 +6053,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_NEED_FULL_MODESET,
+ "[CRTC:%d:%s] requires full modeset",
+ crtc->base.base.id, crtc->base.name);
return -EINVAL;
}
@@ -6122,9 +6123,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_NEED_FULL_MODESET,
+ "[CRTC:%d:%s] requires full modeset",
+ crtc->base.base.id, crtc->base.name);
return -EINVAL;
}
@@ -6162,11 +6164,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
* [PATCH v11 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (5 preceding siblings ...)
2026-03-31 9:03 ` [PATCH v11 6/7] drm/i915/display: Error codes for async flip failures Arun R Murthy
@ 2026-03-31 9:03 ` Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 21:55 ` Claude review: User readable error codes on atomic_ioctl failure Claude Code Review Bot
7 siblings, 1 reply; 18+ messages in thread
From: Arun R Murthy @ 2026-03-31 9:03 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, 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: User readable error codes on atomic_ioctl failure
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
` (6 preceding siblings ...)
2026-03-31 9:03 ` [PATCH v11 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
7 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 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: 8
Reviewed: 2026-04-01T07:55:26.976758
---
This v11 series adds user-readable error codes to `drm_atomic_ioctl()` by repurposing the `reserved` field in `struct drm_mode_atomic` as a pointer to a userspace `struct drm_mode_atomic_err_code`. The concept has value -- compositors currently get bare `-EINVAL` and must guess what went wrong. However, the implementation has **significant UAPI design issues, multiple bugs, typos in the ABI, and a memory leak** that need addressing before this can be merged.
**Key concerns:**
1. **Breaking the `reserved` field contract** -- The `reserved` field was previously validated to be zero, ensuring old kernels reject new userspace and vice versa. This patch silently removes that check, meaning old userspace that accidentally has garbage in `reserved` will now trigger `copy_to_user` to a random address instead of getting `-EINVAL`.
2. **Memory leak** in `drm_mode_atomic_add_error_msg()` -- `kvasprintf()` allocates memory that is never freed.
3. **Typos baked into UAPI** -- `DRM_MODE_ATTOMIC_*`, `VIOLOATION`, `invallid`, `falg` -- these become permanent ABI once merged.
4. **`error_code_ptr` used uninitialized** when `arg->reserved == 0` and an error occurs on a code path that was present before patch 4 moved allocation earlier.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: drm: Define user readable error codes for atomic ioctl
2026-03-31 9:03 ` [PATCH v11 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Typos in UAPI enum names (permanent ABI breakage):**
```c
DRM_MODE_ATTOMIC_CONNECTOR_BW,
DRM_MODE_ATTOMIC_PIPE_BW,
DRM_MODE_ATOMIC_SPEC_VIOLOATION,
```
These should be `DRM_MODE_ATOMIC_CONNECTOR_BW`, `DRM_MODE_ATOMIC_PIPE_BW`, and `DRM_MODE_ATOMIC_SPEC_VIOLATION`. Once merged, these misspellings are permanent UAPI.
**Typos in kernel-doc:**
```c
* @DRM_MODE_ATOMIC_INVALID_API_USAGE: invallid API usage(DRM_ATOMIC not
* enabled, invalid falg, page_flip event
```
"invallid" should be "invalid", "falg" should be "flag".
**Kernel-doc for `drm_mode_atomic_err_code`:**
```c
* @failure_code: error codes defined in enum drm_moide_atomic_failure_code
```
"drm_moide_atomic_failure_code" should be "drm_mode_atomic_failure_codes".
**Design concern with `failure_objs_ptr`:** This field is a `__u64` pointer to DRM objects, but there's no documentation on what the pointed-to data looks like. Is it an array of `__u32` object IDs? What allocates/frees this memory? None of the patches in this series actually populate it, so it's dead UAPI surface area.
**`__u64` for `failure_code`:** The enum only has 9 values. Using `__u32` would be more natural and leave the `__u64 reserved` for actual expansion. The current layout wastes space.
**`failure_string` as a fixed 128-byte char array inside a struct that gets `copy_to_user`'d:** This is 128 bytes copied on every failed atomic commit. Consider whether this is the right tradeoff vs. a userspace-provided buffer.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: drm/atomic: Add error_code element in atomic_state
2026-03-31 9:03 ` [PATCH v11 2/7] drm/atomic: Add error_code element in atomic_state Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Memory leak in `drm_mode_atomic_add_error_msg()`:**
```c
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);
```
`kvasprintf()` allocates memory via `kmalloc` but there is no `kfree(failure_string)` anywhere. This leaks on every error. Add `kfree(failure_string)` after `strscpy_pad`, or better yet, use `vsnprintf` directly into `err_code->failure_string` to avoid the allocation entirely.
**No NULL check on `kvasprintf` return:** `kvasprintf` can return NULL on allocation failure, which would then be passed to `drm_dbg_atomic` and `strscpy_pad`, causing a NULL dereference.
**`container_of` usage is fragile:** The function takes `struct drm_mode_atomic_err_code *err_code` and uses `container_of` to get back to `struct drm_atomic_state`. This only works if `err_code` is always `&state->error_code`. If someone ever passes a standalone struct, this will produce garbage. Consider just passing `struct drm_atomic_state *state` directly.
**Kernel-doc says "Returns: void"** -- kernel-doc convention doesn't require documenting void returns. Minor nit.
**Comment references wrong enum name:**
```c
/* @error_code: pointer to struct holding failure reason and string
*
* Error codes defined in enum drm_mode_atomic_failure_flags
*/
```
The enum is called `drm_mode_atomic_failure_codes`, not `drm_mode_atomic_failure_flags`. Also, this comment should use `/**` kernel-doc format since it's documenting a struct member.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: drm/atomic: Call complete_signaling only if prepare_signaling is done
2026-03-31 9:03 ` [PATCH v11 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
```c
- complete_signaling(dev, state, fence_state, num_fences, !ret);
+ if (num_fences)
+ complete_signaling(dev, state, fence_state, num_fences, !ret);
```
This is needed because patch 4 moves the state allocation earlier, so error paths now reach `out:` before `prepare_signaling` runs. The check is reasonable since `num_fences` is now initialized to 0.
However, `complete_signaling` also handles cleanup of `crtc_state->event` when `install_fds` is false (error path). Skipping it entirely when `num_fences == 0` could miss that event cleanup if we've partially set up state but never called `prepare_signaling`. Looking at the code, events are only set up inside `prepare_signaling`, so this should be safe. But a comment explaining why this is safe would help.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl
2026-03-31 9:03 ` [PATCH v11 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
This moves `drm_atomic_state_alloc()` and `drm_modeset_acquire_init()` before the early validation checks, converting early `return -EINVAL` to `goto out`. This means every trivially-invalid call now allocates and frees an atomic state, acquires/releases modeset locks, etc. That's unnecessary overhead for simple validation failures.
The original code was structured to fail fast cheaply. The justification is to have `state->error_code` available, but for these trivial "invalid flag" / "reserved field set" cases, the error is already clear from the errno. Consider only allocating early when the error reporting pointer (`arg->reserved`) is actually provided.
**The `arg->reserved` check is removed** from the validation:
```c
- if (arg->reserved) {
- drm_dbg_atomic(dev, "commit failed: reserved field set\n");
- return -EINVAL;
- }
```
This is the mechanism that ensures forward/backward compatibility. Removing it without a versioning scheme means old kernels that don't understand the new meaning of `reserved` will reject the call, which is fine, but old userspace that sets `reserved` to garbage will now have `copy_to_user` write to a random address. The capability check in patch 7 partially addresses this (new userspace should check the cap first), but the kernel should still validate the pointer somehow.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: drm/atomic: Return user readable error in atomic_ioctl
2026-03-31 9:03 ` [PATCH v11 5/7] drm/atomic: Return user readable error in atomic_ioctl Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**`error_code_ptr` used uninitialized:**
```c
if (!arg->reserved)
drm_dbg_atomic(dev, "memory not allocated...\n");
else
error_code_ptr = (struct drm_mode_atomic_err_code __user *)
(unsigned long)arg->reserved;
```
When `arg->reserved == 0`, `error_code_ptr` is uninitialized. Later:
```c
if (ret < 0 && arg->reserved) {
if (copy_to_user(error_code_ptr, ...))
```
The `arg->reserved` check guards the usage, so technically safe, but `error_code_ptr` is still declared without initialization. Set it to `NULL` at declaration.
**Debug message is backwards:** When `!arg->reserved` (i.e., userspace didn't provide a pointer), the code prints a debug message saying "memory not allocated for drm_atomic error reporting". This will fire on **every single atomic ioctl call** from any userspace that doesn't use this feature, which is essentially all current userspace. This is extremely noisy and should be removed.
**`memset` uses wrong sizeof:**
```c
memset(&state->error_code, 0, sizeof(*error_code_ptr));
```
This should be `sizeof(state->error_code)`. Using `sizeof(*error_code_ptr)` happens to be the same size but is semantically wrong and will break if `error_code_ptr`'s type ever diverges.
**`copy_to_user` on `-EDEADLK`:** The error reporting happens before the deadlock retry logic:
```c
out:
if (ret < 0 && arg->reserved) {
if (copy_to_user(error_code_ptr, &state->error_code, ...))
ret = -EFAULT;
}
...
if (ret == -EDEADLK) {
drm_atomic_state_clear(state);
ret = drm_modeset_backoff(&ctx);
if (!ret) goto retry;
}
```
On `-EDEADLK`, this will copy a stale/meaningless error report to userspace, then retry. If the retry succeeds, userspace gets `ret=0` but has stale error data written to their buffer. Move the `copy_to_user` after the deadlock retry handling.
**Overwriting `-EFAULT` with `-EDEADLK` handling:** If `copy_to_user` fails and sets `ret = -EFAULT`, the deadlock check `if (ret == -EDEADLK)` won't match, so at least it won't loop, but the original error is lost.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: drm/i915/display: Error codes for async flip failures
2026-03-31 9:03 ` [PATCH v11 6/7] drm/i915/display: Error codes for async flip failures Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**`DRM_MODE_ATOMIC_INVALID_API_USAGE` for unsupported pixel format/modifier:**
```c
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",
```
Using `INVALID_API_USAGE` for "this hardware doesn't support this format/modifier combo with async flip" seems wrong. This isn't invalid API usage; the API is being used correctly but the hardware can't do it. `DRM_MODE_ATOMIC_SPEC_VIOLOATION` (typo aside) seems more appropriate, or a new code.
**`%p4cc` format specifier in `kvasprintf`:** The `%p4cc` format is a kernel-specific `printk` extension for fourcc codes. `kvasprintf` should support it since it uses the kernel's `vsnprintf`, but this is worth verifying. It will produce something like `XR24 little-endian` in the error string -- is that useful for userspace?
**Only partial coverage:** Only a few error paths in `intel_async_flip_check_uapi` and `intel_async_flip_check_hw` are converted. Many other error returns in these functions still use `drm_dbg_kms` without setting an error code. This means userspace will get `DRM_MODE_ATOMIC_UNSPECIFIED_ERROR` (0) for those cases, which defeats the purpose.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
* Claude review: drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING
2026-03-31 9:03 ` [PATCH v11 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING Arun R Murthy
@ 2026-03-31 21:55 ` Claude Code Review Bot
0 siblings, 0 replies; 18+ messages in thread
From: Claude Code Review Bot @ 2026-03-31 21:55 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
```c
case DRM_CAP_ATOMIC_ERROR_REPORTING:
req->value = drm_core_check_feature(dev, DRIVER_ATOMIC) && 1;
break;
```
**`&& 1` is redundant.** `drm_core_check_feature()` already returns a boolean-like value. Just use:
```c
req->value = drm_core_check_feature(dev, DRIVER_ATOMIC);
```
**This cap is always true for all atomic drivers.** That means every atomic driver now implicitly "supports" error reporting, even though only the DRM core and i915 async flip paths actually set error codes. All other error paths will return `DRM_MODE_ATOMIC_UNSPECIFIED_ERROR` with an empty string. The capability should probably be opt-in per driver until meaningful coverage exists.
**Missing documentation:** The `DRM_CAP_ATOMIC_ERROR_REPORTING` constant should be documented alongside the description of how userspace should use the `reserved` field. There's no documentation in the UAPI header explaining the protocol: allocate `struct drm_mode_atomic_err_code`, put its address in `reserved`, call the ioctl, check the struct on failure.
---
**Summary of required fixes before merge:**
1. Fix UAPI typos (`ATTOMIC`, `VIOLOATION`, etc.) -- these are permanent
2. Fix the memory leak in `drm_mode_atomic_add_error_msg()` (missing `kfree`)
3. Add NULL check for `kvasprintf` return
4. Initialize `error_code_ptr = NULL`
5. Remove the spammy debug message when `!arg->reserved`
6. Move `copy_to_user` after the `-EDEADLK` retry logic
7. The `reserved` field repurposing needs more careful ABI design -- consider a flag bit to opt into error reporting rather than assuming non-zero `reserved` is always a valid pointer
8. Remove `&& 1` from the capability check
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2026-03-31 21:55 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-31 9:03 [PATCH v11 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
2026-03-31 9:03 ` [PATCH v11 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 2/7] drm/atomic: Add error_code element in atomic_state Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 3/7] drm/atomic: Call complete_signaling only if prepare_signaling is done Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 4/7] drm/atomic: Allocate atomic_state at the beginning of atomic_ioctl Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 5/7] drm/atomic: Return user readable error in atomic_ioctl Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 6/7] drm/i915/display: Error codes for async flip failures Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 9:03 ` [PATCH v11 7/7] drm: Introduce DRM_CAP_ATOMIC_ERROR_REPORTING Arun R Murthy
2026-03-31 21:55 ` Claude review: " Claude Code Review Bot
2026-03-31 21:55 ` Claude review: User readable error codes on atomic_ioctl failure Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-02-23 9:15 [PATCH v10 0/7] " 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-24 0:26 ` Claude review: " Claude Code Review Bot
2026-02-10 9:03 [PATCH v9 0/7] User readable error codes on atomic_ioctl failure Arun R Murthy
2026-02-10 9:03 ` [PATCH v9 1/7] drm: Define user readable error codes for atomic ioctl Arun R Murthy
2026-02-11 6:31 ` Claude review: " Claude Code Review Bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox