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

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

On 29-10-2025 13:45, Kandpal, Suraj wrote:
>> 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 <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..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.

Done!

>> +		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)

Done!

>> +
>>   	/* 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
Done!
>> +		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

Done!

Thanks and Regards,
Arun R Murthy
-------------------

>
> 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