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

[Lisovskiy, Stanislav]

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

> 
> > 
> > > -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
> 
> 
-- 
Best Regards,

Lisovskiy Stanislav