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

Sun, 24 Aug 2025 22:24:48 -0700 

On 22-08-2025 16:20, Jani Nikula wrote:

On Fri, 22 Aug 2025, Arun R Murthy <arun.r.mur...@intel.com> wrote:

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

Signed-off-by: Arun R Murthy <arun.r.mur...@intel.com>
---
  drivers/gpu/drm/drm_atomic.c      |  6 ++++
  drivers/gpu/drm/drm_atomic_uapi.c | 60 ++++++++++++++++++++++++++++++++-------
  2 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 
cd15cf52f0c9144711da5879da57884674aea9e4..5f25e6d3cf6cf246f83a8c39450b410e97fe45bb
 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1513,6 +1513,9 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
                        if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
                                drm_dbg_atomic(dev, "[CRTC:%d:%s] requires full 
modeset\n",
                                               crtc->base.id, crtc->name);
+                               state->error_code->failure_flags =
+                                       DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET;

It says flags, implying multiple, but you're just adding one there
anyway. Just like it was a regular enum.

Yes its a enum!



+
                                return -EINVAL;
                        }
                }
@@ -1537,8 +1540,11 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
                drm_dbg_atomic(dev,
                               "driver added CRTC to commit: requested 0x%x, 
affected 0x%0x\n",
                               requested_crtc, affected_crtc);
+               state->error_code->failure_flags = 
DRM_MODE_ATOMIC_NEED_FULL_MODESET;
                WARN(!state->allow_modeset, "adding CRTC not allowed without 
modesets: requested 0x%x, affected 0x%0x\n",
                     requested_crtc, affected_crtc);
+
+               return -EINVAL;

This changes behaviour.

Will get it corrected in the next version!



        }
return 0; 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 
ecc73d52bfae41a7ef455a7e13649ec56c690b90..94eaf9c98eb4ac2455799f1416010d366e1b5bbc
 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1058,6 +1058,9 @@ int drm_atomic_set_property(struct drm_atomic_state 
*state,
                        ret = drm_atomic_crtc_get_property(crtc, crtc_state,
                                                           prop, &old_val);
                        ret = drm_atomic_check_prop_changes(ret, old_val, 
prop_value, prop);
+                       if (ret)
+                               state->error_code->failure_flags =
+                                       DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED;
                        break;
                }
@@ -1089,6 +1092,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state, /* ask the driver if this non-primary plane is supported */ 
                        if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
+                               state->error_code->failure_flags =
+                                       DRM_MODE_ATOMIC_ASYNC_NOT_SUP_PLANE;
                                ret = -EINVAL;
if (plane_funcs && plane_funcs->atomic_async_check) 
@@ -1380,6 +1385,13 @@ set_async_flip(struct drm_atomic_state *state)
        }
  }
+#define FAILURE_REASON(flag, reason) #reason, 
+const char *drm_mode_atomic_failure_string[] = {
+       DRM_MODE_ATOMIC_FAILURE_REASON
+};
+
+#undef FAILURE_REASON
+
  int drm_mode_atomic_ioctl(struct drm_device *dev,
                          void *data, struct drm_file *file_priv)
  {
@@ -1389,9 +1401,11 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
        uint32_t __user *props_ptr = (uint32_t __user *)(unsigned 
long)(arg->props_ptr);
        uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned 
long)(arg->prop_values_ptr);
        unsigned int copied_objs, copied_props;
-       struct drm_atomic_state *state;
+       struct drm_atomic_state *state = NULL;
        struct drm_modeset_acquire_ctx ctx;
        struct drm_out_fence_state *fence_state;
+       struct drm_mode_atomic_err_code error_code;
+       struct drm_mode_atomic_err_code __user *error_code_ptr;
        int ret = 0;
        unsigned int i, j, num_fences;
        bool async_flip = false;
@@ -1400,6 +1414,11 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
        if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
                return -EOPNOTSUPP;
+ if (!arg->reserved) 
+               drm_err(dev, "memory not allocated for drm_atomic error 
reporting\n");

This right here makes me suspect you never really tried this with your
regular desktop environment.


+
+       memset(&error_code, 0, sizeof(struct drm_mode_atomic_err_code));
+
        /* disallow for userspace that has not enabled atomic cap (even
         * though this may be a bit overkill, since legacy userspace
         * wouldn't know how to call this ioctl)
@@ -1407,24 +1426,25 @@ 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;
+               error_code.failure_flags = DRM_MODE_ATOMIC_CAP_NOT_ENABLED;
+               ret = -EINVAL;
+               goto out;
        }
if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) { 
                drm_dbg_atomic(dev, "commit failed: invalid flag\n");
-               return -EINVAL;
-       }
-
-       if (arg->reserved) {
-               drm_dbg_atomic(dev, "commit failed: reserved field set\n");
-               return -EINVAL;
+               error_code.failure_flags = DRM_MODE_ATOMIC_INVALID_FLAG;
+               ret = -EINVAL;
+               goto out;
        }
if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) { 
                if (!dev->mode_config.async_page_flip) {
                        drm_dbg_atomic(dev,
                                       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not 
supported\n");
-                       return -EINVAL;
+                       error_code.failure_flags = 
DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC;
+                       ret = -EINVAL;
+                       goto out;
                }
async_flip = true; 
@@ -1435,7 +1455,9 @@ 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;
+               error_code.failure_flags = 
DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY;
+               ret = -EINVAL;
+               goto out;
        }
state = drm_atomic_state_alloc(dev); 
@@ -1446,6 +1468,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
        state->acquire_ctx = &ctx;
        state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+ state->error_code = &error_code; 
+
  retry:
        copied_objs = 0;
        copied_props = 0;
@@ -1542,6 +1566,22 @@ 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) {
+               error_code_ptr = (struct drm_mode_atomic_err_code __user *)
+                                (unsigned long)arg->reserved;
+
+               strscpy_pad(error_code.failure_string,
+                           
drm_mode_atomic_failure_string[error_code.failure_flags],
+                           sizeof(error_code.failure_string));
+
+               if (copy_to_user(error_code_ptr, &error_code, 
sizeof(error_code)))
+                       return -EFAULT;
+       }
+
+       if (!state)
+               return ret;
+
        complete_signaling(dev, state, fence_state, num_fences, !ret);
if (ret == -EDEADLK) {

Reply via email to