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

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

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

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