<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 25-08-2025 15:14, Jani Nikula wrote: </div> <blockquote type="cite" cite="mid:419591dda7158b3d56c40aac0df86ca499202676@intel.com"> <pre wrap="" class="moz-quote-pre">On Sat, 23 Aug 2025, "Murthy, Arun R" <a class="moz-txt-link-rfc2396E" href="mailto:arun.r.murthy@intel.com"><arun.r.murthy@intel.com></a> wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">On 22-08-2025 16:07, Jani Nikula wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">On Fri, 22 Aug 2025, Arun R Murthy <a class="moz-txt-link-rfc2396E" href="mailto:arun.r.murthy@intel.com"><arun.r.murthy@intel.com></a> wrote: </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre">At the moment, I'm not (yet) convinced any of this will work, even as a concept. </pre> </blockquote> <pre wrap="" class="moz-quote-pre">km_flip test in IGT has been run and is working fine with these patch series. Also the existing kms_atomic test with atomic_invalid_test has been modified to test this error code(<a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/series/153330/">https://patchwork.freedesktop.org/series/153330/</a>) Am I missing anything over here? </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Having a few tests in IGT does not mean a non-trivial userspace (compositor) can use the information in a useful way. </pre> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Even so, I'll comment on some of the choices in the series. </pre> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">Signed-off-by: Arun R Murthy <a class="moz-txt-link-rfc2396E" href="mailto:arun.r.murthy@intel.com"><arun.r.murthy@intel.com></a> --- 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") \ </pre> </blockquote> <pre wrap="" class="moz-quote-pre">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. </pre> </blockquote> <pre wrap="" class="moz-quote-pre">Here we are mapping an error_code defined as enum with a corresponding string using the X macros. Anyway will have a look at the macro in include/drm/intel/pciids.h </pre> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ 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 </pre> </blockquote> <pre wrap="" class="moz-quote-pre">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.) </pre> </blockquote> <pre wrap="" class="moz-quote-pre">This is defined as enum. We have two separate list one for enum and other for error message(string) but upon user adding a new element/error_code in the enum, there can be a possibility of missing adding the error string. So have linked enum with the corresponding error message/string so both are at a single place and upon adding new entry will be easy for the user. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> I think the overall point is this: a) If the messages are fixed for each enum, there is no point in passing that message across the ioctl. Userspace can map the enums and messages directly. b) If the messages are not fixed, I see no point in having the above fixed messages at all. c) If the messages are not fixed, but you want to have default messages, the messages do not belong in the uapi header at all. Hide them in the kernel. </pre> </blockquote> My intention was option C, wherein have a default message and the driver is free to overwrite the default message string if required. I agree in that case doesn't make sense having it on uapi, will move this inside the driver. <blockquote type="cite" cite="mid:419591dda7158b3d56c40aac0df86ca499202676@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ +#define FAILURE_REASON(flag, reason) #reason, +extern const char *drm_mode_atomic_failure_string[]; +#undef FAILURE_REASON </pre> </blockquote> <pre wrap="" class="moz-quote-pre">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? </pre> </blockquote> <pre wrap="" class="moz-quote-pre">Sorry missed adding documentation for this. Will add in my next series. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Documentation does not fix the issue that data is not an interface. </pre> </blockquote> As pointed out above will move to option C, and will get rid of this failure messages in the uapi. <blockquote type="cite" cite="mid:419591dda7158b3d56c40aac0df86ca499202676@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">+ +/** + * 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 </pre> </blockquote> <pre wrap="" class="moz-quote-pre">Flags? As in a mask of multiple things? Is 64 going to be enough for everyone, all conditions in all drivers? </pre> </blockquote> <pre wrap="" class="moz-quote-pre">Should be more that sufficient, this is an enum. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> Then don't call it "flags". </pre> </blockquote> Done! <blockquote type="cite" cite="mid:419591dda7158b3d56c40aac0df86ca499202676@intel.com"> <pre wrap="" class="moz-quote-pre"> </pre> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="" class="moz-quote-pre">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? </pre> </blockquote> <pre wrap="" class="moz-quote-pre">Most of the error check is in the drm-core and these are common across the drivers. Upon drivers sending driver specific error code, the compositor may not be able to handle the error code. But in cases required compositor changes can be added so as to handle them. In general the generic error conditions that are met by compositor and for which compositor can take corrective measurements are listed down first. </pre> </blockquote> <pre wrap="" class="moz-quote-pre"> I think we're going to need a handful of error codes for starters, along with compositor changes that make meaningful and benefitial usage of the error codes. </pre> </blockquote> Sure will narrow down the list in my next version. Thanks and Regards, Arun R Murthy ------------------- </body> </html>