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

[Daniel Vetter <daniel@ffwll.ch>]

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
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev