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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 04, 2019 at 07:33:31AM +0000, Lisovskiy, Stanislav wrote:
> 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@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.

Yeah I think the patch is fine, but the commit message isn't the clearest,
since it explains what exactly an interim version did, and then amends
that. Which is confusing.

You're explanation above would have made a much better commit message, but
oh well the patch is committed so can't fix that up anymore. But good to
heed for next time around, review should also make sure the commit message
captures everything discussed as well as possible.

Cheers, Daniel
> 
> > 
> > > > 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