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

[Lisovskiy, Stanislav]

On Thu, 2019-02-14 at 09:33 +0100, Daniel Vetter wrote:
> On Thu, Feb 14, 2019 at 08:31:52AM +0000, Lisovskiy, Stanislav wrote:
> > On Thu, 2019-02-14 at 09:25 +0100, Daniel Vetter wrote:
> > > On Thu, Feb 14, 2019 at 08:22:29AM +0000, Lisovskiy, Stanislav
> > > wrote:
> > > > On Wed, 2019-02-13 at 21:41 +0100, Daniel Vetter wrote:
> > > > > On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy
> > > > > via
> > > > > igt-dev wrote:
> > > > > > 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.
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1092
> > > > > > 25
> > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at int
> > > > > > el.c
> > > > > > om>
> > > > > 
> > > > > Kernel atomic spec is pretty clear that it's either EINVAL or
> > > > > ERANGE,
> > > > > and
> > > > > nothing else. In the docs we even limit to EINVAL (scroll
> > > > > down to
> > > > > atomic_check):
> > > > > 
> > > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlig
> > > > > ht=d
> > > > > rm_m
> > > > > ode_config_funcs#c.drm_mode_config_funcs
> > > > > 
> > > > > Would be great if you can submit a kernel patch to add the
> > > > > ERANGE.
> > > > > And
> > > > > change this one here to only accept ERANGE and EINVAL as
> > > > > "sprite
> > > > > doesn't
> > > > > work".
> > > > 
> > > > Unfortunately, in reality it is not like that. We have a bug
> > > > fdo#109225, where it returns ENOSPC. DRM does this when plane
> > > > size
> > > > exceeds the framebuffer size(check drm_atomic.c,
> > > > drm_atomic_plane_check). 
> > > > We hit this issue when we happen to have resolution lower than
> > > > expected
> > > > by IGT, so we actually create a framebuffer of smaller size
> > > > than it
> > > > sets for the plane and then we
> > > > hit ENOSPC instead EINVAL, which is treated now as OK by the
> > > > test
> > > > case.
> > > > So adding ERANGE will not help for this bug. 
> > > > Either I should add all three to IGT(ERANGE, ENOSPC, EINVAL) or
> > > > fix
> > > > it
> > > > in the kernel, so that it returns EINVAL instead of ENOSPC.
> > > 
> > > Hm yeah, pls do, and pls do update the kernel documentation. I'm
> > > also
> > > wondering whether we shouldn't wrap some checks around calls to
> > > atomic_check in the kernel for this.
> > > -Daniel
> > 
> > So do you mean I should fix the kernel or igt? :)
> 
> Oops :-) I meant to fix the igt logic to also accept ENOSPC and
> EINVAL,
> plus update the kernel's documentation so that docs reflect reality.
> -Daniel

Ok, I will fix the kernel docs also. However regarding this patch,
should I still simply just compare to 0 in a check rather than adding
all three, as conditions like "if ((ret == -EINVAL) || (ret == -ERANGE)
|| (ret == -ENOSPC))" would look a bit weird, I guess..

> 
> > 
> > > 
> > > > 
> > > > > -Daniel
> > > > > 
> > > > > > ---
> > > > > >  tests/kms_atomic_transition.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > > b/tests/kms_atomic_transition.c
> > > > > > index ec5d25de..58bf6280 100644
> > > > > > --- a/tests/kms_atomic_transition.c
> > > > > > +++ b/tests/kms_atomic_transition.c
> > > > > > @@ -282,7 +282,7 @@ 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);
> > > > > >  
> > > > > > -		if (ret == -EINVAL) {
> > > > > > +		if (ret != 0) {
> > > > > >  			if (cursor_width == sprite_width
> > > > > > &&
> > > > > >  			    cursor_height ==
> > > > > > sprite_height) {
> > > > > >  				igt_assert_f(alpha,
> > > > > > @@ -472,7 +472,7 @@ run_transition_test(igt_display_t
> > > > > > *display,
> > > > > > enum pipe pipe, igt_output_t *output
> > > > > >  			igt_pipe_request_out_fence(pipe_ob
> > > > > > j);
> > > > > >  
> > > > > >  		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)
> > > > > > +		if (ret == 0 || pipe_obj->n_planes < 3)
> > > > > >  			break;
> > > > > >  
> > > > > >  		ret = 0;
> > > > > > -- 
> > > > > > 2.17.1
> > > > > > 
> > > > > > _______________________________________________
> > > > > > igt-dev mailing list
> > > > > > igt-dev at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > > > 
> > > > > 
> > > > 
> > > > -- 
> > > > Best Regards,
> > > > 
> > > > Lisovskiy Stanislav
> > > 
> > > 
> > 
> > -- 
> > Best Regards,
> > 
> > Lisovskiy Stanislav
> 
> 
-- 
Best Regards,

Lisovskiy Stanislav