[PATCH] drm: add debug logs for drm_mode_atomic_ioctl errors

[Simon Ser]

On Tuesday, November 10, 2020 5:04 PM, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Tue, Nov 10, 2020 at 03:58:01PM +0000, Simon Ser wrote:
> > Be nice to user-space and log what happened when returning EINVAL in
> > drm_mode_atomic_ioctl.
> >
> > Signed-off-by: Simon Ser <contact at emersion.fr>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Maxime Ripard <mripard at kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 25c269bc4681..68d767420082 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -1303,22 +1303,30 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >  	 * though this may be a bit overkill, since legacy userspace
> >  	 * wouldn't know how to call this ioctl)
> >  	 */
> > -	if (!file_priv->atomic)
> > +	if (!file_priv->atomic) {
> > +		DRM_DEBUG_ATOMIC("atomic commit failed: atomic cap not enabled\n");
>
> The "atomic commit failed:" bit seems a bit redundant.

I guess the "atomic" part can be dropped indeed. However I'd really like to
keep the word "failed" here, because it makes grepping large DRM logs much
easier (and is already used for other failures, e.g. driver failures).

> >  		return -EINVAL;
> > +	}
> >
> > -	if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS)
> > +	if (arg->flags & ~DRM_MODE_ATOMIC_FLAGS) {
> > +		DRM_DEBUG_ATOMIC("atomic commit failed: invalid flag\n");
> >  		return -EINVAL;
> > +	}
> >
> >  	if (arg->reserved)
> >  		return -EINVAL;
>
> You don't want one for this? I wonder why this "reserved" field
> even exists...

Yeah, I wasn't sure either so preferred not to touch it. I guess it's
scratch space which can be used to extend the ioctl in the future?