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

[Gore, Tim]

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

 Tim

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch