On 13/01/15 22:20, Daniel Vetter wrote: > 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. Hardcoding the flag /doesn't/ necessarily detect that you've forgotten to update the testcases. For example, suppose you hardcoded it as bit 17, which is the lowest invalid bit in your kernel. I've added a new kernel feature which makes bit 17 a valid flag, but only on GEN7, on the BSD ring, and in a batch containing an odd number of DWords. Your test will find that bit 17 *is* still an invalid flag, unless it happens to use exactly the combination of options that make it valid. *Proving* that setting a bit that should cause a request to be rejected /always/ does so is therefore a combinatorially infeasible problem. .Dave. > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
