Re: [PATCH v11 1/7] drm: Define user readable error codes for atomic ioctl

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi,

I just watched the recording of your presentation at XDC2026 and had a 
thoughts on it. Comments are below

Am 31.03.26 um 11:03 schrieb Arun R Murthy:
> There can be multiple reasons for a failure in atomic_ioctl. Most often
> in these error conditions -EINVAL is returned. User/Compositor would
> have to blindly take a call on failure of this ioctl so as to use
> ALLOW_MODESET or retry. It would be good if user/compositor gets a
> readable error code on failure so they can take proper corrections in
> the next commit.
> The struct drm_mode_atomic is being passed by the user/compositor which
> holds the properties for modeset/flip. Reusing the same struct for
> returning the error code in case of failure, thereby creation of new
> uapi/interface for returning the error code is not required.
> The element 'reserved' in the struct drm_mode_atomic is used for
> returning the user readable error code. This points to the struct
> drm_mode_atomic_err_code. Failure reasons as a string can also be added
> on need basis by the variable failure_string in the same struct
> drm_mode_atomic_err_code.
>
> v3: Remove fixed error (Jani/Xaver)
> v5: Fix kernel-doc (Jani)
> v7: Rephrase the kernel doc description (Suraj)
> v8: Removed the below enum and suggest to use INVALID_API_USAGE (Xaver)
> 	DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE
> 	DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP
> v10: Added more error codes for the enum
> v11: Add default/unspecified error code
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>   include/uapi/drm/drm_mode.h | 56 +++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a4bdc4bd11bc142e9d3b172397e18a1909a21488..8bf5fd8533912dc7a188aad19cc3741dd2099592 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -48,6 +48,7 @@ extern "C" {
>   #define DRM_CONNECTOR_NAME_LEN	32
>   #define DRM_DISPLAY_MODE_LEN	32
>   #define DRM_PROP_NAME_LEN	32
> +#define DRM_MODE_ATOMIC_FAILURE_STRING_LEN	128
>   
>   #define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
>   #define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
> @@ -1346,6 +1347,61 @@ struct drm_mode_destroy_dumb {
>   		DRM_MODE_ATOMIC_NONBLOCK |\
>   		DRM_MODE_ATOMIC_ALLOW_MODESET)
>   
> +/**
> + * enum drm_mode_atomic_failure_codes -  error codes for failures in atomic_ioctl

I have serious doubts about these failure codes. The core issue to me is 
that when the kernel driver detects an impossible commit, it probably 
knows best how to fix it. Yet the failure codes often lack actionable 
meaning. Examples below

> + * @DRM_MODE_ATOMIC_UNSPECIFIED_ERROR: this is the default/unspecified error.

This one does not give a hint to what happens. Should userspace abort or 
fall back to TEST_ONLY?

> + * @DRM_MODE_ATOMIC_INVALID_API_USAGE: invallid API usage(DRM_ATOMIC not
> + *				       enabled, invalid falg, page_flip event
> + *				       with test-only, etc)

I've seen this being used in the i915 patch for a async flip.  Could 
mean anything there (format, driver specifics).

> + * @DRM_MODE_ATOMIC_NEED_FULL_MODESET: Need full modeset on all connected crtc's

This one is helpful because it gives user space an actionable hint.

> + * @DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED: Property changed in async flip
> + * @DRM_MODE_ATOMIC_SCANOUT_BW: For a given resolution, refresh rate and the
> + *                              color depth cannot be accomodated. Resolution
> + *                              is to lower the refresh rate or color depth.
> + * @DRM_MODE_ATOMIC_CONNECTOR_BW: Refers to the limitation on the link rate on
> + *                                a given connector.
> + * @DRM_MODE_ATOMIC_PIPE_BW: Limitation on the pipe, either pipe not available
> + *                           or the pipe scaling factor limitation.
> + * @DRM_MODE_ATOMIC_MEMORY_DOMAIN: Any other memory/bandwidth related limitation
> + *                                 other then the ones specified above.
> + * @DRM_MODE_ATOMIC_SPEC_VIOLOATION: Limitation of a particular feature on that
> + *                                   hardware. To get to know the feature, the
> + *                                   property/object causing this is being sent
> + *                                   back to user @failure_objs_ptr in the
> + *                                   struct drm_mode_atomic_err_code

There codes don't seem actionable to me.

What I'm proposing is a set if hints (instead of failure) that give user 
space a clear instruction on what to try next.

I assume you're familiar with the mode-status codes? [1] They do this to 
some extend.

[1] 
https://elixir.bootlin.com/linux/v7.2.8/source/include/drm/drm_modes.h#L91

> + */
> +enum drm_mode_atomic_failure_codes {
> +	DRM_MODE_ATOMIC_UNSPECIFIED_ERROR,
> +	DRM_MODE_ATOMIC_INVALID_API_USAGE,
> +	DRM_MODE_ATOMIC_NEED_FULL_MODESET,
> +	DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
> +	DRM_MODE_ATOMIC_SCANOUT_BW,
> +	DRM_MODE_ATTOMIC_CONNECTOR_BW,
> +	DRM_MODE_ATTOMIC_PIPE_BW,
> +	DRM_MODE_ATOMIC_MEMORY_DOMAIN,
> +	DRM_MODE_ATOMIC_SPEC_VIOLOATION,
> +};
> +
> +/**
> + * struct drm_mode_atomic_err_code - struct to store the error code
> + *
> + * pointer to this struct will be stored in reserved variable of
> + * struct drm_mode_atomic to report the failure cause to the user.
> + *
> + * @failure_code: error codes defined in enum drm_moide_atomic_failure_code
> + * @failure_objs_ptr: pointer to the drm_object that caused error
> + * @reserved: reserved for future use
> + * @count_objs: count of drm_objects if multiple drm_objects caused error
> + * @failure_string: user readable error message string
> + */
> +struct drm_mode_atomic_err_code {
> +	__u64 failure_code;
> +	__u64 failure_objs_ptr;
> +	__u64 reserved;
> +	__u32 count_objs;
> +	char failure_string[DRM_MODE_ATOMIC_FAILURE_STRING_LEN];

I'd don't think we should return error strings from the kernel. If we 
do, these strings will become uAPI. And I guarantee that someone will 
start parsing them for information. We'll be in a situation where we 
cannot ever change the strings.

For error logging, we can put messages directly in the kernel log or you 
can build them in user space from within libdrm.

Best regards
Thomas


> +};
> +
>   struct drm_mode_atomic {
>   	__u32 flags;
>   	__u32 count_objs;
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)