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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 03 Sep 2025, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
> On 02-09-2025 15:46, Jani Nikula wrote:
>> On Tue, 02 Sep 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      | 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.
> Yes, felt so, as adding a new function doesnt make much sense just for 
> filling the struct drm_mode_atomic_err_code.
>
> Will remove this function and fill the struct drm_mode_atomic_err_code 
> in place, thereby the below comment also should be handled.

Nobody suggested that! Keep the function.


>
> 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_ */

-- 
Jani Nikula, Intel