> Subject: [PATCH v9 5/7] drm/atomic: Return user readable error in atomic_ioctl > > Add user readable error codes for failure cases in drm_atomic_ioctl() so that > user can decode the error code and take corrective measurements. > > v8: Replaced DRM_MODE_ATOMIC_ASYNC_NOT_SUPP_PLANE, > DRM_MODE_ATOMIC_ASYNC_MODIFIER_NOT_SUPP with > INVALID_API_USAGE > (Xaver) > v9: Move free atomic_state on error to patch 3 (Suraj) > > Signed-off-by: Arun R Murthy <[email protected]> > --- > drivers/gpu/drm/drm_atomic_uapi.c | 58 +++++++++++++++++++++++++++++-- > -------- > 1 file changed, 44 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > b/drivers/gpu/drm/drm_atomic_uapi.c > index > bcd12b6eac4f497d2edb8581d9fb0fd54cbef827..f0c3f080f5d66c733dfbfa23f38a > 22132193adec 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -1196,6 +1196,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; > } > > @@ -1218,6 +1223,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; > } > > @@ -1256,9 +1266,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_INVALID_API_USAGE, > + > "[PLANE:%d:%s] does not support async flip", > + obj->id, > plane->name); > break; > } > } > @@ -1568,6 +1579,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; > @@ -1576,6 +1588,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; > @@ -1584,11 +1604,16 @@ 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(*error_code_ptr)); > + > /* 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_mode_atomic_add_error_msg(&state->error_code, > + > DRM_MODE_ATOMIC_INVALID_API_USAGE, > + "drm atomic capability not > enabled"); > drm_dbg_atomic(dev, > "commit failed: atomic cap not enabled\n"); > ret = -EINVAL; > @@ -1596,21 +1621,18 @@ int drm_mode_atomic_ioctl(struct drm_device > *dev, > } > > if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) { > - drm_dbg_atomic(dev, "commit failed: invalid flag\n"); > - ret = -EINVAL; > - goto out; > - } > - > - if (arg->reserved) { > - drm_dbg_atomic(dev, "commit failed: reserved field set\n"); > + 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"); > + 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; > } > @@ -1621,8 +1643,9 @@ 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"); > + 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; > } > @@ -1725,6 +1748,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)))
Corner case what if you end up here right after allocating a state and come here because of one of the conditions Which were previously checked before state allocation. And then this copy to user fails. Then we return without freeing the state, fences etc. Should this be done in a cleaner way. Regards, Suraj Kandpal > + return -EFAULT; > + } > + > if (num_fences) > complete_signaling(dev, state, fence_state, num_fences, !ret); > > > -- > 2.25.1
