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

[Lisovskiy, Stanislav]

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=109225
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> 
> 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?highlight=drm_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.

> -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_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)
> > +		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