On Fri, 22 Aug 2025, Maarten Lankhorst <maarten.lankho...@linux.intel.com> wrote: > Hey, > > I'm not entirely sold on the design, it's way more complicated than it > should be, it should be trivial to add any amount of error messages.
If we add error messages, how do we ensure the error messages themselves never become part of UAPI? It doesn't take much for someone in userspace to start doing strcmp on them. > Replace return -EINVAL; with return drm_atomic_error_einval(state, class, > "string"); Yeah, much cleaner. And if we go this route, then maybe make it printf style to avoid callers having to clumsily fill a buffer first. > Where class may be an enum, but in a way more generic way than > currently specified, for example "invalid use of api", "requires > modeset", "invalid arguments", "driver limitations", "async flip not > possible". That's also easier for userspace to deal with than a plethora of flags. I guess the goals were more ambitious wrt precision/granularity, but is that achievable? BR, Jani. > The drm_atomic_error_einval() would set class and str as appropriate, > and then return -EINVAL. > > That's probably all we should need here. > > Kind regards, > ~Maarten > > Den 2025-08-22 kl. 09:00, skrev Arun R Murthy: >> For failures in async flip atomic check/commit path return user readable >> error codes in struct drm_atomic_state. >> >> Signed-off-by: Arun R Murthy <arun.r.mur...@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >> b/drivers/gpu/drm/i915/display/intel_display.c >> index >> c1a3a95c65f0b66c24ddd64f47dfdc67bbde86c9..5e23f4fc747bd01fa05eba63661bf7279b083317 >> 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -5950,6 +5950,7 @@ static int intel_async_flip_check_uapi(struct >> intel_atomic_state *state, >> drm_dbg_kms(display->drm, >> "[CRTC:%d:%s] modeset required\n", >> crtc->base.base.id, crtc->base.name); >> + state->base.error_code->failure_flags = >> DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET; >> return -EINVAL; >> } >> >> @@ -6019,6 +6020,7 @@ static int intel_async_flip_check_hw(struct >> intel_atomic_state *state, struct in >> drm_dbg_kms(display->drm, >> "[CRTC:%d:%s] modeset required\n", >> crtc->base.base.id, crtc->base.name); >> + state->base.error_code->failure_flags = >> DRM_MODE_ATOMIC_CRTC_NEED_FULL_MODESET; >> return -EINVAL; >> } >> >> @@ -6061,6 +6063,8 @@ static int intel_async_flip_check_hw(struct >> intel_atomic_state *state, struct in >> plane->base.base.id, plane->base.name, >> &new_plane_state->hw.fb->format->format, >> new_plane_state->hw.fb->modifier); >> + state->base.error_code->failure_flags = >> + DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPPORTED; >> return -EINVAL; >> } >> >> > -- Jani Nikula, Intel
