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

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

[tim.gore]

From: Tim Gore <tim.gore@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@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);
 	}
 
-- 
2.2.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dave Gordon]

On 12/01/15 14:09, tim.gore@intel.com wrote:
> From: Tim Gore <tim.gore@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@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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Gore, Tim]

> -----Original Message-----
> From: Gordon, David S
> Sent: Monday, January 12, 2015 4:04 PM
> To: Gore, Tim; intel-gfx@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@intel.com wrote:
> > From: Tim Gore <tim.gore@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@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.
 Tim
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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@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@intel.com wrote:
> > > From: Tim Gore <tim.gore@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@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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Gore, Tim]

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Monday, January 12, 2015 11:26 PM
> To: Gore, Tim
> Cc: Gordon, David S; intel-gfx@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@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@intel.com wrote:
> > > > From: Tim Gore <tim.gore@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> > Daniel Vetter
> > Sent: Monday, January 12, 2015 11:26 PM
> > To: Gore, Tim
> > Cc: Gordon, David S; intel-gfx@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@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@intel.com wrote:
> > > > > From: Tim Gore <tim.gore@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Gore, Tim]

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Tuesday, January 13, 2015 10:20 PM
> To: Gore, Tim
> Cc: Daniel Vetter; Gordon, David S; intel-gfx@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 Tue, Jan 13, 2015 at 09:48:51AM +0000, Gore, Tim wrote:
> >
> >
> > > -----Original Message-----
> > > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On
> > > Behalf Of Daniel Vetter
> > > Sent: Monday, January 12, 2015 11:26 PM
> > > To: Gore, Tim
> > > Cc: Gordon, David S; intel-gfx@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@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@intel.com wrote:
> > > > > > From: Tim Gore <tim.gore@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@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
> --

I appreciate that you want people to write test cases, but this only catches features
that involve adding a new flag, for this ioctl. This is an important subset but
cant really substitute for a more comprehensive mechanism.  It is of course possible
to carry an android / intel specific patch, but lots of people here work straight off the
freedesktop repository to ensure they stay up to date. My preference would be for
the version of igt in the android tree to diverge as little as possible.

  Tim

> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
>>> Daniel Vetter
>>> Sent: Monday, January 12, 2015 11:26 PM
>>> To: Gore, Tim
>>> Cc: Gordon, David S; intel-gfx@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@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@intel.com wrote:
>>>>>> From: Tim Gore <tim.gore@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@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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx