RE: [PATCH v9 5/7] drm/atomic: Return user readable error in atomic_ioctl

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

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> Sent: Friday, February 13, 2026 2:54 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> <jani.nikula@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Joonas
> Lahtinen <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> <tursulin@ursulin.net>; xaver.hugl@kde.org; harry.wentland@amd.com;
> Shankar, Uma <uma.shankar@intel.com>; louis.chauvet@bootlin.com; Kumar,
> Naveen1 <naveen1.kumar@intel.com>; Yella, Ramya Krishna
> <ramya.krishna.yella@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org; Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: RE: [PATCH v9 5/7] drm/atomic: Return user readable error in
> atomic_ioctl
> 
> > 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 <arun.r.murthy@intel.com>
> > ---
> >  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..f0c3f080f5d66c733dfbfa23f38
> a
> > 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.
> 
Can as well change this return -EFAULT to just ret = -EFAULT and allow the code flow to continue to clear the atomic state and drop the locks acquired.

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