On Tue, 02 Sep 2025, Arun R Murthy <[email protected]> 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 <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic.c | 23 +++++++----
> drivers/gpu/drm/drm_atomic_uapi.c | 83
> ++++++++++++++++++++++++++++++---------
> include/drm/drm_atomic.h | 19 +++++++++
> 3 files changed, 100 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index
> cd15cf52f0c9144711da5879da57884674aea9e4..4f6c7e659b362f0887ffcc85dade1122fd30df3d
> 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1463,6 +1463,7 @@ int drm_atomic_check_only(struct drm_atomic_state
> *state)
> unsigned int requested_crtc = 0;
> unsigned int affected_crtc = 0;
> int i, ret = 0;
> + char *err_string;
>
> drm_dbg_atomic(dev, "checking %p\n", state);
>
> @@ -1511,8 +1512,13 @@ int drm_atomic_check_only(struct drm_atomic_state
> *state)
> if (!state->allow_modeset) {
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> 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);
> + err_string = "requires full modeset";
This points a non-const pointer to const data.
Anyway none of this should require or start a precendent of using local
variables all over the place for the error strings. It's ugly. Ditto
below for all the places.
If all the places calling drm_mode_atomic_add_error_msg() do debug
logging, maybe that's where the debug logging should be?
> + drm_dbg_atomic(dev, "[CRTC:%d:%s] %s\n",
> + crtc->base.id, crtc->name,
> err_string);
> + drm_mode_atomic_add_error_msg(state->error_code,
> +
> DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET,
> + err_string);
I guess you didn't want to follow what Maarten suggested.
> +
> return -EINVAL;
Adding the return is a functional change, and I mentioned it
earlier. Ignoring review is one of the certain ways to stop receiving
review in the future.
> }
> }
> @@ -1534,11 +1540,14 @@ int drm_atomic_check_only(struct drm_atomic_state
> *state)
> * so compositors know what's going on.
> */
> if (affected_crtc != requested_crtc) {
> - drm_dbg_atomic(dev,
> - "driver added CRTC to commit: requested 0x%x,
> affected 0x%0x\n",
> - requested_crtc, affected_crtc);
> - WARN(!state->allow_modeset, "adding CRTC not allowed without
> modesets: requested 0x%x, affected 0x%0x\n",
> - requested_crtc, affected_crtc);
> + err_string = "adding CRTC not allowed without modesets";
> + drm_dbg_atomic(dev, "%s: requested 0x%x, affected 0x%0x\n",
> + err_string, requested_crtc, affected_crtc);
> + drm_mode_atomic_add_error_msg(state->error_code,
> + DRM_MODE_ATOMIC_NEED_FULL_MODESET,
> + err_string);
> +
> + return -EINVAL;
Adding the return is a functional change, and I mentioned it earlier.
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index
> 85dbdaa4a2e25878c953b9b41539c8566d55c6d9..60f1b8baebce0db1ce984c8cda56039261b519e8
> 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1017,6 +1017,7 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> struct drm_mode_object *ref;
> u64 old_val;
> int ret;
> + char *err_string;
>
> if (!drm_property_change_valid_get(prop, prop_value, &ref))
> return -EINVAL;
> @@ -1058,6 +1059,12 @@ 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) {
> + err_string = "property change not allowed in
> async flip";
> + drm_mode_atomic_add_error_msg(state->error_code,
> +
> DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> + err_string);
> + }
> break;
> }
>
> @@ -1096,9 +1103,14 @@ int drm_atomic_set_property(struct drm_atomic_state
> *state,
> ret =
> plane_funcs->atomic_async_check(plane, state, true);
>
> if (ret) {
> + err_string = "plane does not support
> async flip";
> +
> drm_mode_atomic_add_error_msg(state->error_code,
> +
> DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
> +
> err_string);
> drm_dbg_atomic(prop->dev,
> - "[PLANE:%d:%s] does not
> support async flips\n",
> - obj->id, plane->name);
> + "[PLANE:%d:%s] %s\n",
> + obj->id, plane->name,
> + err_string);
> break;
> }
> }
> @@ -1390,42 +1402,63 @@ 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;
Unrelated change.
> 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;
> + char *err_string;
>
> /* disallow for drivers not supporting atomic: */
> 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");
> + 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;
> +
> + 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)
> */
> if (!file_priv->atomic) {
> + err_string = "DRM_ATOMIC capability not enabled";
Yeah, just repeating, don't start using this err_string local variable
idea.
> drm_dbg_atomic(dev,
> - "commit failed: atomic cap not enabled\n");
> - return -EINVAL;
> + "commit failed: %s\n", err_string);
> + drm_mode_atomic_add_error_msg(&error_code,
> + DRM_MODE_ATOMIC_INVALID_API_USAGE,
> + err_string);
> + 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;
> + err_string = "invalid flag";
> + drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
> + drm_mode_atomic_add_error_msg(&error_code,
> + DRM_MODE_ATOMIC_INVALID_API_USAGE,
> + err_string);
> + 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;
> + err_string = "DRM_MODE_PAGE_FLIP_ASYNC not supported
> with ATOMIC ioctl";
> + drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
> + drm_mode_atomic_add_error_msg(&error_code,
> +
> DRM_MODE_ATOMIC_INVALID_API_USAGE,
> + err_string);
> + ret = -EINVAL;
> + goto out;
> }
>
> async_flip = true;
> @@ -1434,9 +1467,13 @@ 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");
> - return -EINVAL;
> + err_string = "page-flip event requested with test-only commit";
> + drm_dbg_atomic(dev, "commit failed: %s\n", err_string);
> + drm_mode_atomic_add_error_msg(&error_code,
> + DRM_MODE_ATOMIC_INVALID_API_USAGE,
> + err_string);
> + ret = -EINVAL;
> + goto out;
> }
>
> state = drm_atomic_state_alloc(dev);
> @@ -1447,6 +1484,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;
> @@ -1543,6 +1582,14 @@ 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, &error_code,
> sizeof(error_code)))
> + return -EFAULT;
> +
> + if (!state)
> + return ret;
> +
> complete_signaling(dev, state, fence_state, num_fences, !ret);
>
> if (ret == -EDEADLK) {
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index
> d380001b24b4223baa54dae6c3c43e19dfb1958d..8956b175795128422eefc2bc047b320b80aedc3f
> 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1255,4 +1255,23 @@ struct drm_bridge_state *
> drm_atomic_get_new_bridge_state(const struct drm_atomic_state *state,
> struct drm_bridge *bridge);
>
> +/**
> + * 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
> + */
> +static inline void drm_mode_atomic_add_error_msg(struct
> drm_mode_atomic_err_code *err_code,
> + __u64 failure_code,
> + char *failure_string)
So this should be "const char *format, ..." with printf format, with
printf format annotation, to help callers pass in other stuff than just
fixed strings.
This should be a proper function instead of static inline.
> +{
> + err_code->failure_code = failure_code;
> + strscpy_pad(err_code->failure_string, failure_string,
> + strlen(err_code->failure_string));
If the size is fixed, why do we just silently truncate the string? If
the message is for the user, it'll be ugly to emit truncated strings.
> +}
> +
> #endif /* DRM_ATOMIC_H_ */
--
Jani Nikula, Intel
