[Intel-gfx] [PATCH i-g-t] tests/gem_exec_params: change flags used in invalid-flags test

[Daniel Vetter]

On Tue, Jan 13, 2015 at 09:48:51AM +0000, Gore, Tim wrote:
>
>
> > -----Original Message-----
> > From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> > Daniel Vetter
> > Sent: Monday, January 12, 2015 11:26 PM
> > To: Gore, Tim
> > Cc: Gordon, David S; intel-gfx at lists.freedesktop.org; Wood, Thomas
> > Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/gem_exec_params: change flags
> > used in invalid-flags test
> >
> > On Mon, Jan 12, 2015 at 04:14:03PM +0000, Gore, Tim wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gordon, David S
> > > > Sent: Monday, January 12, 2015 4:04 PM
> > > > To: Gore, Tim; intel-gfx at lists.freedesktop.org
> > > > Cc: Wood, Thomas
> > > > Subject: Re: [Intel-gfx] [PATCH i-g-t] tests/gem_exec_params: change
> > > > flags used in invalid-flags test
> > > >
> > > > On 12/01/15 14:09, tim.gore at intel.com wrote:
> > > > > From: Tim Gore <tim.gore at intel.com>
> > > > >
> > > > > The invalid-flags test in gem_exec_params uses
> > > > > (I915_EXEC_HANDLE_LUT << 1) as an invalid flag, but this is no
> > > > > longer invalid for recent android versions, and may not be invalid
> > > > > in Linux in the future. So I have changed this test to use
> > (__I915_EXEC_UNKNOWN_FLAGS) instead.
> > > > > __I915_EXEC_UNKNOWN_FLAGS is defined in i915_drm.h as a mask of
> > > > > all the undefined flags.
> > > > >
> > > > > Signed-off-by: Tim Gore <tim.gore at intel.com>
> > > > > ---
> > > > >  tests/gem_exec_params.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
> > > > > index
> > > > > f63eda9..2a1c544 100644
> > > > > --- a/tests/gem_exec_params.c
> > > > > +++ b/tests/gem_exec_params.c
> > > > > @@ -179,7 +179,7 @@ igt_main
> > > > >   /* HANDLE_LUT and NO_RELOC are already exercised by
> > > > > gem_exec_lut_handle */
> > > > >
> > > > >   igt_subtest("invalid-flag") {
> > > > > - execbuf.flags = I915_EXEC_RENDER |
> > > > (I915_EXEC_HANDLE_LUT << 1);
> > > > > + execbuf.flags = I915_EXEC_RENDER |
> > > > (__I915_EXEC_UNKNOWN_FLAGS);
> > > > >   RUN_FAIL(EINVAL);
> > > > >   }
> > > > >
> > > >
> > > > Should we perhaps have a test that iterates over each bit in this
> > > > mask one at a time (to check that EACH of them is correctly detected
> > > > and
> > > > rejected) as well as this one with ALL the unknown flag bits set?
> > > >
> > > > .Dave.
> > >
> > > Yes, I can do that if people like the idea.
> >
> > Well the testcase should still fail if the kernel is accepting any flags - the idea
> > is very much that every time you add a flag the test fails and will remind you
> > to add the new testcases for the new flag. So any patch which makes LUT <<
> > 1 no longer fail the tests if it's not rejected is nacked by me.
> >
> > Imo you should just carry an igt patch in the android version somewhere to
> > adapt the testcase to your abi changes.
> > -Daniel
>
> No, the patch uses __I915_EXEC_UNKNOWN_FLAGS, which is set in i915_drm.h, and hopefully
> Is maintained to be the set of all invalid flags. In the upstream version this is set to
> -(I915_EXEC_HANDLE_LUT<<1). In the android version it is set to
> -(I915_EXEC_ENABLE_WATCHDOG<<1)
>
> So Using this macro should give you the right test in each case, rather than having a special
> Android test case that is separately maintained from the actual definition of the flags.

Yeah I mixed things up a bit. But my point is that hardcoding the invalid
flags forces you to update the testcase since when you add a new flag it
fails. With your patch it gets magically fixed with a recompile, which
makes it a lot easier for people to forget writing the new testcases.

It has the downside that the tests are specific to a given kernel
branch/release, but that's why we started tagging them roughly in lockstep
with the otc qa release testing. Definitely not a perfect approach, so
ideas highly welcome.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch