Re: [PATCH v3 3/4] drm/atomic: Return user readable error in atomic_ioctl

["Murthy, Arun R" <arun.r.murthy@intel.com>]

On 22-08-2025 16:20, Jani Nikula wrote:
> On Fri, 22 Aug 2025, Arun R Murthy <arun.r.murthy@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.murthy@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.
Yes its a 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.
Will get it corrected in the next version!
>
>>   	}
>>   
>>   	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) {