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

[Dave Gordon]

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

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