On Sat, 23 Aug 2025, "Murthy, Arun R" <arun.r.mur...@intel.com> wrote:
> 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.
See my reply [1] about fixed vs. non-fixed error messages.
I believe Xaver is also saying we don't want the fixed error messages,
and especially not in a uapi header.
BR,
Jani.
[1] https://lore.kernel.org/r/419591dda7158b3d56c40aac0df86ca499202...@intel.com
--
Jani Nikula, Intel
