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

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Thu, 2019-04-04 at 10:10 +0200, Daniel Vetter wrote:
> 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

Well, yeah v4 revision says something different from initial commit
message :) I just thought that I need to keep it for history.

To me it is still at least arguable, if we really need to fail in
setup_parms. Anyway, if the function purpose was to find a maximum size
for sprite plane, should it really matter what was the 
error( ENOSPC, ERANGE, EINVAL) then if we are still able to find max
plane size which works?

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