[PATCH v2 3/4] drm/atomic: Return user readable error in atomic_ioctl

[Harry Wentland]

On 2025-07-30 09:55, Murthy, Arun R wrote:
> On 30-07-2025 18:44, Harry Wentland wrote:
>>
>> On 2025-07-30 06:16, Arun R Murthy wrote:
>>> Add user readable error codes for failure cases in drm_atomic_ioctl() so
>>> that user can decode the error code and take corrective measurements.
>>>
>> Thanks for doing this work.
>>
>>
>>> @@ -1541,6 +1548,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>>           ret = drm_atomic_commit(state);
>>>       }
>>>   +    /* update the error code if any error to allow user handling it */
>>> +    arg->reserved = state->error_code;
>> Once we do this we paint ourselves into a corner.
>>
>> It's nice that we have this reserved field since it allows us to
>> extend the atomic ioctl without the need to define a new one.
>>
>> When we discussed this at the Display Next Hackfest [1] we came
>> to the conclusion that we would want:
>>
>> - an enum to show what is the issue
>> - a string that can be logged to display verbose information
>>    about the failure
>> - an optional array of KMS object IDs participating in the failure
>>
>> We could define these in a new struct
>>
>> struct drm_mode_atomic2 {
>>     __u64 failure_reason;
>>     __u64 failure_string;
>>     __u32 drm_object_ids[MAX_FAILURE_OBJECT_IDS]
>>     __u64 reserved;
>> }
>>
>> that we link to via the drm_mode_atomic->reserved field.
>>
>> Your approach of flags, as opposed to an enum, would allow reporting
>> of multiple failures. Do we think drivers would ever make use of it?
>> Normally check short-circuits when a failure/limitation is hit.
> Thanks for the feedback. As pointed it would be good to have a struct pointed by the reserved variable, so that we can further extend the scope.
> W.r.t the enum, I feel its better to have bit notification as we can convey multiple errors if any.
> 
> Understand that the driver at present will return on the first error but upon adding this user readable error code can extend the driver to check add the properties provided by user and report error at once, so that multiple failure iterations can be overridden.
> 

I don't have a strong preference either way here. I'm happy with a
set of flags so we can report multiple errors at once.

> Having obj_id would be a good one, but when the flow goes to the vendor specific driver, they may not have the link to the obj_id where a failure is occurred. But still good to have so that at the early stage in set_prop, sanity check failures can be reported with obj_id so that it would be easy for the user to correct them in the next atomic ioctl.
> 

Right. This is entirely optional for scenarios where a driver (or
DRM core) can provided this additional info. It might make sense for
MST bandwidth failures where we could provide the MST connectors that
are involved, i.e., ones on the same physical connector.

It could possibly also be useful for movable HW blocks, which is not
something we can express right now. Things like a LUT that can be
assigned to any pipe. When userspace tries enabling the property that
uses the LUT but all are in use we might be able to signal what is
conflicting.

But again, entirely optional for scenarios where a driver thinks it
can provide better information. It also depends on how userspace will
implement this. I'm sure compositors don't want to overcomplicate their
failure handling.

Harry


> Will take care of these feedback in my next revision of patch set.
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
>