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

[Daniel Vetter <daniel@ffwll.ch>]

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

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

> 
> > 
> > > 
> > > > -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@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > > 
> > > > 
> > > 
> > > -- 
> > > Best Regards,
> > > 
> > > Lisovskiy Stanislav
> > 
> > 
> -- 
> Best Regards,
> 
> Lisovskiy Stanislav

-- 
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