Re: [PATCH v5 4/5] drm/atomic: Return user readable error in atomic_ioctl

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 16/09/2025 à 09:18, Arun R Murthy a écrit :
> 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.murthy@intel.com>
> ---
>   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..3d4e9709e8e289cf302413e171b1336812d65c1c 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;

Thanks for this series, it is a nice contribution!

I am not sure about this change, drm_dbg will be in debug logs, but 
drm_mode_atomic_add_error_msg do a drm_err, is it expected?
This is also the case for other drm_mode_atomic_add_error_msg calls in 
this patch and the next one.

>   				}
>   			}
> @@ -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 */
> +		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));
> +
>   	/* 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");
> +		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 */
> +	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);
>   
> 

-- 
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com