Re: [PATCH v5 4/5] drm/atomic: Return user readable error in atomic_ioctl

["Murthy, Arun R" <arun.r.murthy@intel.com>]

On 01-10-2025 20:17, Louis Chauvet wrote:
>
>
> Le 16/09/2025 à 09:18, Arun R Murthy a écrit :
>> Add user readable error codes for failure cases in drm_atomic_ioctl() so
>> that user can decode the error code and take corrective measurements.
>>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_uapi.c | 71 
>> ++++++++++++++++++++++++++++-----------
>>   1 file changed, 52 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
>> b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 
>> 3ef478e717bec917d1b8803c72bbcc8d6409d745..3d4e9709e8e289cf302413e171b1336812d65c1c 
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -1036,6 +1036,11 @@ int drm_atomic_set_property(struct 
>> drm_atomic_state *state,
>>               ret = drm_atomic_connector_get_property(connector, 
>> connector_state,
>>                                   prop, &old_val);
>>               ret = drm_atomic_check_prop_changes(ret, old_val, 
>> prop_value, prop);
>> +            if (ret) {
>> + drm_mode_atomic_add_error_msg(&state->error_code,
>> + DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
>> +                                  "property change not allowed in 
>> async flip");
>> +            }
>>               break;
>>           }
>>   @@ -1058,6 +1063,11 @@ int drm_atomic_set_property(struct 
>> drm_atomic_state *state,
>>               ret = drm_atomic_crtc_get_property(crtc, crtc_state,
>>                                  prop, &old_val);
>>               ret = drm_atomic_check_prop_changes(ret, old_val, 
>> prop_value, prop);
>> +            if (ret) {
>> + drm_mode_atomic_add_error_msg(&state->error_code,
>> + DRM_MODE_ATOMIC_ASYNC_PROP_CHANGED,
>> +                                  "property change not allowed in 
>> async flip");
>> +            }
>>               break;
>>           }
>>   @@ -1096,9 +1106,10 @@ int drm_atomic_set_property(struct 
>> drm_atomic_state *state,
>>                       ret = plane_funcs->atomic_async_check(plane, 
>> state, true);
>>                     if (ret) {
>> -                    drm_dbg_atomic(prop->dev,
>> -                               "[PLANE:%d:%s] does not support async 
>> flips\n",
>> -                               obj->id, plane->name);
>> + drm_mode_atomic_add_error_msg(&state->error_code,
>> + DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE,
>> +                                      "[PLANE:%d:%s] does not 
>> support async flip",
>> +                                      obj->id, plane->name);
>>                       break;
>
> Thanks for this series, it is a nice contribution!
>
> I am not sure about this change, drm_dbg will be in debug logs, but 
> drm_mode_atomic_add_error_msg do a drm_err, is it expected?
> This is also the case for other drm_mode_atomic_add_error_msg calls in 
> this patch and the next one.
>
Thanks for pointing out, this was a miss from my side.
Will get it changed to debug prints.

Thanks and Regards,
Arun R Murthy
-------------------

>>                   }
>>               }
>> @@ -1393,6 +1404,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>       struct drm_atomic_state *state;
>>       struct drm_modeset_acquire_ctx ctx;
>>       struct drm_out_fence_state *fence_state;
>> +    struct drm_mode_atomic_err_code __user *error_code_ptr;
>>       int ret = 0;
>>       unsigned int i, j, num_fences = 0;
>>       bool async_flip = false;
>> @@ -1401,6 +1413,14 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>       if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>>           return -EOPNOTSUPP;
>>   +    if (!arg->reserved)
>> +        drm_dbg_atomic(dev,
>> +                   "memory not allocated for drm_atomic error 
>> reporting\n");
>> +    else
>> +        /* update the error code if any error to allow user handling 
>> it */
>> +        error_code_ptr = (struct drm_mode_atomic_err_code __user *)
>> +                 (unsigned long)arg->reserved;
>> +
>>       state = drm_atomic_state_alloc(dev);
>>       if (!state)
>>           return -ENOMEM;
>> @@ -1409,31 +1429,35 @@ int drm_mode_atomic_ioctl(struct drm_device 
>> *dev,
>>       state->acquire_ctx = &ctx;
>>       state->allow_modeset = !!(arg->flags & 
>> DRM_MODE_ATOMIC_ALLOW_MODESET);
>>   +    memset(&state->error_code, 0, sizeof(struct 
>> drm_mode_atomic_err_code));
>> +
>>       /* disallow for userspace that has not enabled atomic cap (even
>>        * though this may be a bit overkill, since legacy userspace
>>        * wouldn't know how to call this ioctl)
>>        */
>>       if (!file_priv->atomic) {
>> -        drm_dbg_atomic(dev,
>> -                   "commit failed: atomic cap not enabled\n");
>> -        return -EINVAL;
>> + drm_mode_atomic_add_error_msg(&state->error_code,
>> +                          DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +                          "DRM_ATOMIC capability not enabled");
>> +        ret = -EINVAL;
>> +        goto out;
>>       }
>>         if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
>> -        drm_dbg_atomic(dev, "commit failed: invalid flag\n");
>> -        return -EINVAL;
>> -    }
>> -
>> -    if (arg->reserved) {
>> -        drm_dbg_atomic(dev, "commit failed: reserved field set\n");
>> -        return -EINVAL;
>> + drm_mode_atomic_add_error_msg(&state->error_code,
>> +                          DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +                          "invalid flag");
>> +        ret = -EINVAL;
>> +        goto out;
>>       }
>>         if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
>>           if (!dev->mode_config.async_page_flip) {
>> -            drm_dbg_atomic(dev,
>> -                       "commit failed: DRM_MODE_PAGE_FLIP_ASYNC not 
>> supported\n");
>> -            return -EINVAL;
>> + drm_mode_atomic_add_error_msg(&state->error_code,
>> + DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +                              "DRM_MODE_PAGE_FLIP_ASYNC not 
>> supported with ATOMIC ioctl");
>> +            ret = -EINVAL;
>> +            goto out;
>>           }
>>             async_flip = true;
>> @@ -1442,9 +1466,11 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>       /* can't test and expect an event at the same time. */
>>       if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) &&
>>               (arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
>> -        drm_dbg_atomic(dev,
>> -                   "commit failed: page-flip event requested with 
>> test-only commit\n");
>> -        return -EINVAL;
>> + drm_mode_atomic_add_error_msg(&state->error_code,
>> +                          DRM_MODE_ATOMIC_INVALID_API_USAGE,
>> +                          "page-flip event requested with test-only 
>> commit");
>> +        ret = -EINVAL;
>> +        goto out;
>>       }
>>     retry:
>> @@ -1543,6 +1569,13 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>>       }
>>     out:
>> +    /* update the error code if any error to allow user handling it */
>> +    if (ret < 0 && arg->reserved) {
>> +        if (copy_to_user(error_code_ptr, &state->error_code,
>> +                 sizeof(state->error_code)))
>> +            return -EFAULT;
>> +    }
>> +
>>       if (num_fences)
>>           complete_signaling(dev, state, fence_state, num_fences, !ret);
>>
>