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

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

On 22-08-2025 21:44, Xaver Hugl wrote:
>> +#define DRM_MODE_ATOMIC_FAILURE_REASON \
>> +       FAILURE_REASON(DRM_MODE_ATOMIC_CAP_NOT_ENABLED, "DRM_ATOMIC capability not enabled") \
>> +       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")
> As mentioned before, some of these errors are a bit too specific. We
> don't need to have an enum value for every way the API can be used
> wrongly - CAP_NOT_ENABLED, INVALID_FLAG, PAGE_FLIP_ASYNC and
> MODIFIER_NOT_SUPPORTED should all just be one enum value for "invalid
> API usage".
> In general, there should only be enum values that the compositor
> implementation can actually use on end-user systems. For further
> information when debugging a broken compositor implementation, other
> tools can be used instead, like drm debug logging or the returned
> string.
I have considered your comment in the last series and have removed 
driver specific errors.
Anyway will have a look again on this and will get back.
>> +#define FAILURE_REASON(flag, reason) flag,
>> +typedef enum {
>> +       DRM_MODE_ATOMIC_FAILURE_REASON
>> +} drm_mode_atomic_failure_flag;
>> +#undef FAILURE_REASON
>> +
>> +#define FAILURE_REASON(flag, reason) #reason,
>> +extern const char *drm_mode_atomic_failure_string[];
>> +#undef FAILURE_REASON
> The intention for the string wasn't for the enum values to be paired
> with a description of the enum - that belongs into documentation, not
> uAPI.
>
> The idea behind it was that drivers could add driver-specific
> information in the string for compositors to log (only in commits
> where failure isn't normally expected), so we have an easier time
> debugging issues a user system experienced by looking at the
> compositor logs. Sending the enum value again in string form isn't
> useful.

We are not sending enum value in string. Its just a single place where 
we have both enum and string. Upon user adding new error codes if both 
enum and string are at a single place it would be easy for the user. 
Hence adding both in a single place using X macros.

Its not mandatory to have a string for every enum, the string can be 
left empty if not required, or later in the driver user can overwrite 
the string as well.

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

> - Xaver