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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 05.10.26 um 13:39 schrieb Xaver Hugl:
> Am Mo., 5. Okt. 2026 um 11:08 Uhr schrieb Thomas Zimmermann
> <tzimmermann@suse.de>:
>> 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)