On 02-09-2025 15:46, Jani Nikula wrote:
Yes, felt so, as adding a new function doesnt make much sense just for filling the struct drm_mode_atomic_err_code.On Tue, 02 Sep 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 | 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.
Will remove this function and fill the struct drm_mode_atomic_err_code in place, thereby the below comment also should be handled.
Thanks and Regards, Arun R Murthy -------------------
+ 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.
Will take it out of this series.
} } @@ -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.
Will take it out of this series.
}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.
Noted!
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_ */
