| Message ID | 2sJwtJZS8GpTVmDedCE6b5WNkmnmUARXGt0mugjU2BA@cp3-web-033.plabs.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: add debug logs for drm_mode_atomic_ioctl errors | expand |
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
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?
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.
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)
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(-)