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.
> +
> 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.
> }
>
> 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) {
--
Jani Nikula, Intel
