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

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

On Fri, 22 Aug 2025, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> 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 any. 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 can save by creating a new
> uapi/interface for returning the error code.
> 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 have been initialized in
> DRM_MODE_ATOMIC_FAILURE_REASON.

At the moment, I'm not (yet) convinced any of this will work, even as a
concept.

Even so, I'll comment on some of the choices in the series.

> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/uapi/drm/drm_mode.h | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index a122bea2559387576150236e3a88f99c24ad3138..f0986a3fe9a7d61e57e9a9a5ec01a604343f6930 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -45,6 +45,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	64
>  
>  #define DRM_MODE_TYPE_BUILTIN	(1<<0) /* deprecated */
>  #define DRM_MODE_TYPE_CLOCK_C	((1<<1) | DRM_MODE_TYPE_BUILTIN) /* deprecated */
> @@ -1157,6 +1158,47 @@ struct drm_mode_destroy_dumb {
>  		DRM_MODE_ATOMIC_NONBLOCK |\
>  		DRM_MODE_ATOMIC_ALLOW_MODESET)
>  
> +#define DRM_MODE_ATOMIC_FAILURE_REASON \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_CAP_NOT_ENABLED, "DRM_ATOMIC capability not enabled") \

Please don't add macros that expect other macros in the context. They
become very confusing. Just pass in the other macro to use. See MACRO__
in include/drm/intel/pciids.h for an example.

> +	FAILURE_REASON(DRM_MODE_ATOMIC_INVALID_FLAG, "invalid flag") \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_PAGE_FLIP_ASYNC, "Legacy DRM_MODE_PAGE_FLIP_ASYNC not to be used in atomic ioctl") \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_FLIP_EVENT_WITH_CHECKONLY, "requesting page-flip event with TEST_ONLY") \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET, "Need full modeset on this crtc") \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_NEED_FULL_MODESET, "Need full modeset on all the connected crtc's") \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_ASYNC_NOT_SUP_PLANE, "Async flip not supported on this plane") \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED, "Modifier not supported on this plane with async flip") \
> +	FAILURE_REASON(DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED, "No property change allowed when async flip is enabled")
> +
> +#define FAILURE_REASON(flag, reason) flag,
> +typedef enum {
> +	DRM_MODE_ATOMIC_FAILURE_REASON
> +} drm_mode_atomic_failure_flag;
> +#undef FAILURE_REASON

This is a uapi header. Do we really want all of that macro trickery here
in the first place? What's wrong with just a plain enum? (Or macros.)

> +
> +#define FAILURE_REASON(flag, reason) #reason,
> +extern const char *drm_mode_atomic_failure_string[];
> +#undef FAILURE_REASON

Data is not an interface. Don't expose arrays like this. Who is even
going to know what its size is? Again, uapi header, where does it point,
what address space is it?

> +
> +/**
> + * 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_flags: error codes defined in drm_atomic_failure.failure_flag

Flags? As in a mask of multiple things? Is 64 going to be enough for
everyone, all conditions in all drivers?

OTOH, what's the promise wrt multiple reasons? Practically all drivers
bail out at the sight of first issue, and it's usually pretty much
meaningless to continue to figure out *other* reasons after that.

And this brings us to one of my main concerns on making this fly:

You bail out on the first thing, but you can't expect all drivers to
check stuff in the same order. It's not practical. So different drivers
will likely return different reasons for virtually the same
condition. So how is userspace going to handle that?

> + * @failure_string_ptr: pointer to user readable error message drm_mode_failure.failure_string

Is the string part of uapi, and can never change? Doesn't sound
great. What's wrong with just a plain enum?

> + * @failure_obj_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
> + */
> +struct drm_mode_atomic_err_code {
> +	__u64 failure_flags;
> +	__u64 failure_objs_ptr;
> +	__u64 reserved;
> +	__u32 count_objs;
> +	char failure_string[DRM_MODE_ATOMIC_FAILURE_STRING_LEN];
> +};
> +
>  struct drm_mode_atomic {
>  	__u32 flags;
>  	__u32 count_objs;

-- 
Jani Nikula, Intel