On Tue, Jan 13, 2015 at 09:48:51AM +0000, Gore, Tim wrote: > > > > -----Original Message----- > > From: daniel.vetter@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] On Behalf Of > > Daniel Vetter > > Sent: Monday, January 12, 2015 11:26 PM > > To: Gore, Tim > > Cc: Gordon, David S; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Wood, Thomas > > Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx > > > > Cc: Wood, Thomas > > > > Subject: Re: [PATCH i-g-t] tests/gem_exec_params: change > > > > flags used in invalid-flags test > > > > > > > > On 12/01/15 14:09, tim.gore@xxxxxxxxx wrote: > > > > > From: Tim Gore <tim.gore@xxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxx> > > > > > --- > > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
