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

[Lisovskiy, Stanislav]

On Wed, 2019-04-03 at 16:23 +0200, Daniel Vetter wrote:
> 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 at 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.

Hi Daniel,

As the commit message says, previsouly we were expecting only
EINVAL(which indicates for
us that we had reached max size of a plane) or 0.

Problem was that in some circumstances, if we get something else
test case did not catch that and proceeded as if everything is
great, failing afterwards, when trying already to commit or other
place.
The thing which this patch does is as said "Added assertion if error is
not EINVAL as expected, other errors except EINVAL are considered now a
failures". Otherwise if we got EINVAL, we proceed normally.

There was actually no purpose to "fix" something by that, but rather
make it fail in a proper place and proper way, otherwise error just
went further. 

My initial idea actually was to catch all kind of problems here, so
that we always get some kind of working plane configuration here.
However it was not accepted and currently only EINVAL is treated as
"normal" error.

If you have any proposals, how this can be improved sure I can do this.

> 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at 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 at intel.com>
> 
> 
> 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
>