Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

Daniel Vetter
2019-04-03T14:23:15Z

On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote:
> On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote:
> > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com>
> >
> > There is no guarantee that error return value will be
> > always EINVAL, made a check more general as it can be
> > ERANGE, ENOSPC, EINVAL and probably others, which all
> > mean the same in context of this test case: i.e this sprite
> > size is not valid.
> >
> > v2: Added macro to make check look a bit nicer.
> > v3: Removed redundant debug line.
> > v4: Added assertion if error is not EINVAL as expected,
> > other errors except EINVAL are considered now a failures.

Uh, commit message says to make the error message more general, and then
proceeds to make it not more general. This kind of inconsistencies should
be caught in review.

> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

And I also don't understand how this patch fixed anything really, we still
just check for EINVAL.
-Daniel

> > ---
> > tests/kms_atomic_transition.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 12bafe87..e1a2d4a0 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t
> > *display, enum pipe pipe,
> > LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > }
> >
> > +#define is_atomic_check_failure_errno(errno) \
> > + (errno != -EINVAL && errno != 0)
> > +
> > +#define is_atomic_check_plane_size_errno(errno) \
> > + (errno == -EINVAL)
> > +
> > static void setup_parms(igt_display_t *display, enum pipe pipe,
> > const drmModeModeInfo *mode,
> > struct igt_fb *primary_fb,
> > @@ -251,8 +257,9 @@ retry:
> >
> > wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > parms, false);
> > ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > + igt_assert(!is_atomic_check_failure_errno(ret));
> >
> > - if (ret == -EINVAL) {
> > + if (is_atomic_check_plane_size_errno(ret)) {
> > if (cursor_width == sprite_width &&
> > cursor_height == sprite_height) {
> > igt_assert_f(alpha,
> > @@ -442,7 +449,9 @@ run_transition_test(igt_display_t *display, enum
> > pipe pipe, igt_output_t *output
> > igt_pipe_request_out_fence(pipe_obj);
> >
> > ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > - if (ret != -EINVAL || pipe_obj->n_planes < 3)
> > + igt_assert(!is_atomic_check_failure_errno(ret));
> > +
> > + if (!is_atomic_check_plane_size_errno(ret) || pipe_obj-
> > >n_planes < 3)
> > break;
> >
> > ret = 0;
>
> LGTM
>
> Reviewed-by: Stuart Summers <stuart.summers@intel.com>

> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

--
Daniel Vetter
Software Engineer, Intel Corporation