> Subject: [PATCH v6 4/5] drm/atomic: Return user readable error in
> atomic_ioctl
>
> 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_uapi.c | 71
> ++++++++++++++++++++++++++++-----------
> 1 file changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index
> 3ef478e717bec917d1b8803c72bbcc8d6409d745..3d4e9709e8e289cf302413
> e171b1336812d65c1c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1036,6 +1036,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;
> }
>
> @@ -1058,6 +1063,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;
> }
>
> @@ -1096,9 +1106,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_ASYNC_NOT_SUPP_PLANE,
> +
> "[PLANE:%d:%s] does not support async flip",
> + obj->id,
> plane->name);
> break;
> }
> }
> @@ -1393,6 +1404,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;
> @@ -1401,6 +1413,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
> */
First letter in uppercase.
> + error_code_ptr = (struct drm_mode_atomic_err_code __user
> *)
> + (unsigned long)arg->reserved;
> +
> state = drm_atomic_state_alloc(dev);
> if (!state)
> return -ENOMEM;
> @@ -1409,31 +1429,35 @@ 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(struct
> +drm_mode_atomic_err_code));
Isn't is better to have it as
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_dbg_atomic(dev,
> - "commit failed: atomic cap not enabled\n");
> - return -EINVAL;
> + drm_mode_atomic_add_error_msg(&state->error_code,
> +
> DRM_MODE_ATOMIC_INVALID_API_USAGE,
> + "DRM_ATOMIC capability not
> enabled");
I'd rather have it say drm atomic capability 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;
> + 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");
> - return -EINVAL;
> + 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;
> }
>
> async_flip = true;
> @@ -1442,9 +1466,11 @@ 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;
> + 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;
> }
>
> retry:
> @@ -1543,6 +1569,13 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
> }
>
> out:
> + /* update the error code if any error to allow user handling it */
First letter in uppercase here
Regards,
Suraj Kandpal
> + if (ret < 0 && arg->reserved) {
> + if (copy_to_user(error_code_ptr, &state->error_code,
> + sizeof(state->error_code)))
> + return -EFAULT;
> + }
> +
> if (num_fences)
> complete_signaling(dev, state, fence_state, num_fences, !ret);
>
>
> --
> 2.25.1
