Hi
Am 05.10.26 um 13:39 schrieb Xaver Hugl:
Am Mo., 5. Okt. 2026 um 11:08 Uhr schrieb Thomas Zimmermann
<[email protected]>:
I have serious doubts about these failure codes. The core issue to me is
that when the kernel driver detects an impossible commit, it probably
knows best how to fix it.
There's lots of different ways you could fix any specific error, with
different tradeoffs and it all depends a lot on what the compositor is
actually trying to do. The kernel can't make those kinds of policy
decisions.
That's not a policy decision. User space is free to do what ever it
wants. What I have in mind is a hint from the kernel what to do next.
+ * @DRM_MODE_ATOMIC_UNSPECIFIED_ERROR: this is the default/unspecified error.
This one does not give a hint to what happens. Should userspace abort or
fall back to TEST_ONLY?
That's up to userspace. It's the same as ret=-EINVAL today.
IMHO most of these errors are fancy variants of EINVAL; except for
NEED_FULL_MODESET.
+ * @DRM_MODE_ATOMIC_INVALID_API_USAGE: invallid API usage(DRM_ATOMIC not
+ * enabled, invalid falg, page_flip event
+ * with test-only, etc)
I've seen this being used in the i915 patch for a async flip. Could
mean anything there (format, driver specifics).
Its purpose is to catch anything that the compositor is supposed to
know it can't do - using unsupported formats, invalid combinations of
properties (crtc active=1 without a connector), that sort of thing. It
should never be anything driver specific though.
I don't see how this could be useful except for the prototype that come
with this series. It's so broad in meaning that it's almost meaningless.
And error codes don't have to be driver specific to be meaningful. I
already brought up DRM's mode-status code as example. They are not
specific to any mode, display or hardware. Yet they clearly state what
went wrong with the mode.
+ * @DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED: Property changed in async flip
+ * @DRM_MODE_ATOMIC_SCANOUT_BW: For a given resolution, refresh rate and the
+ * color depth cannot be accomodated. Resolution
+ * is to lower the refresh rate or color depth.
+ * @DRM_MODE_ATOMIC_CONNECTOR_BW: Refers to the limitation on the link rate on
+ * a given connector.
+ * @DRM_MODE_ATOMIC_PIPE_BW: Limitation on the pipe, either pipe not available
+ * or the pipe scaling factor limitation.
+ * @DRM_MODE_ATOMIC_MEMORY_DOMAIN: Any other memory/bandwidth related
limitation
+ * other then the ones specified above.
+ * @DRM_MODE_ATOMIC_SPEC_VIOLOATION: Limitation of a particular feature on that
+ * hardware. To get to know the feature, the
+ * property/object causing this is being sent
+ * back to user @failure_objs_ptr in the
+ * struct drm_mode_atomic_err_code
There codes don't seem actionable to me.
Scanout and connector bandwidth are very directly actionable. I agree
Fair point.
about the others though, adding these error codes should wait until
there's a compositor actually using them for something.
+ char failure_string[DRM_MODE_ATOMIC_FAILURE_STRING_LEN];
I'd don't think we should return error strings from the kernel. If we
do, these strings will become uAPI. And I guarantee that someone will
start parsing them for information. We'll be in a situation where we
cannot ever change the strings.
For error logging, we can put messages directly in the kernel log
The error string is half the reason we need the API: When things fail
on an end user system, we need to be able to find out what happened
after the fact, without the user needing to have drm debug logging
enabled and without them having to trigger the bug again (which can be
very difficult to do).
You could to this with detailed and precise error codes plus the object
on which it failed.
If the error cannot be expressed in abstract terms then users will begin
parsing the error string, which is a no-go IMHO.
I think making the error string something more abstract like the file
+ line number of the failure would also be okay, if that solves the
concerns about uAPI. We just need some way to figure out what happened
after something goes wrong, which would usually involve diving through
kernel sources anyways.
Filename + line is even worse. None of this is even close to stable.
I'll nak this as much as possible.
I also don't buy the argument about diving through kernel sources. Not
doing that should be the goal here because it is what we currently do.
I don't want to leave it at this for this series, so here's my suggestion.
Pick 5 common mode-setting failures and go through all of the kernel
drivers and report them as detailed as possible. This will give a good
understanding of the characteristics of the errors involved and what
could be done in the kernel. The simple i915 patch doesn't seem to grasp
the effects of this change.
You may also want to play with an in-kernel interface that does the
actual testing. Something like this.
int drm_check_crtc_max_pixel_bandwidth(...)
{
// do the test and encode the error if any
}
if (!drm_check_crtc_max_pixel_(crtc,
SOME_EXTRA_HINT_WHAT_WE_RE_DOING, (value < max_value))
return -EINVAL.
do the same for other DRM objects and try to find a good API. Because if
we're not careful, we'll end up with different kernel drivers
implementing the failure reporting in different ways. It wouldn't be the
first time.
Best regards
Thomas
or you can build them in user space from within libdrm.
libdrm doesn't have any of the information returned in the error string.
- Xaver
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)
