[PATCH] drm: add debug logs for drm_mode_atomic_ioctl errors

[PATCH] drm: add debug logs for drm_mode_atomic_ioctl errors

[Simon Ser]

Be nice to user-space and log what happened when returning EINVAL in
drm_mode_atomic_ioctl.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@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");
 		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;
 
-	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC)
+	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
+		DRM_DEBUG_ATOMIC("atomic commit failed: invalid flag DRM_MODE_PAGE_FLIP_ASYNC\n");
 		return -EINVAL;
+	}
 
 	/* 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))
+			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
+		DRM_DEBUG_ATOMIC("atomic commit failed: page-flip event requested with test-only commit\n");
 		return -EINVAL;
+	}
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
-- 
2.29.2


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: add debug logs for drm_mode_atomic_ioctl errors

[Ville Syrjälä]

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@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@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.

>  		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...

>  
> -	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC)
> +	if (arg->flags & DRM_MODE_PAGE_FLIP_ASYNC) {
> +		DRM_DEBUG_ATOMIC("atomic commit failed: invalid flag DRM_MODE_PAGE_FLIP_ASYNC\n");
>  		return -EINVAL;
> +	}
>  
>  	/* 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))
> +			(arg->flags & DRM_MODE_PAGE_FLIP_EVENT)) {
> +		DRM_DEBUG_ATOMIC("atomic commit failed: page-flip event requested with test-only commit\n");
>  		return -EINVAL;
> +	}
>  
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state)
> -- 
> 2.29.2
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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@emersion.fr>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@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?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: add debug logs for drm_mode_atomic_ioctl errors

[Simon Ser]

On Tuesday, November 10, 2020 6:39 PM, Sam Ravnborg <sam@ravnborg.org> wrote:

> You could consider migrating to drm_dbg_atomic() and friends while
> touching this - so we get one more core file migrated.
> You have a drm_device so the code is fine with the new drm_ based
> logging.

That's a good point! Sent a v2.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel