| Message ID | 20150521094453.GA13768@boom (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 21, 2015 at 12:44:53PM +0300, David Weinehall wrote: > tests/gem_ctx_param_basic: Expand ctx_param tests > > Expand the context parameter tests to cover the > no-zeromap parameter. > > Signed-off-by: David Weinehall <david.weinehall@intel.com> > --- > gem_ctx_param_basic.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/tests/gem_ctx_param_basic.c b/tests/gem_ctx_param_basic.c > index b44b37cf0538..ba9366d1a679 100644 > --- a/tests/gem_ctx_param_basic.c > +++ b/tests/gem_ctx_param_basic.c > @@ -98,7 +98,7 @@ igt_main > ctx_param.size = 0; > } > > - ctx_param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD + 1; > + ctx_param.param = I915_CONTEXT_PARAM_NO_ZEROMAP + 1; Please respin this one with a LOCAL_ define for NO_ZEROMAP. We generally don't want to have a hard coupling between the headers in libdrm and igt, would mean a libdrm release roughly every week ;-) > > igt_subtest("invalid-param-get") { > ctx_param.context = ctx; > @@ -132,6 +132,28 @@ igt_main > TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); > } > > + ctx_param.param = LOCAL_CONTEXT_PARAM_NO_ZEROMAP; > + > + igt_subtest("non-root-set-no-zeromap") { > + igt_fork(child, 1) { > + igt_drop_root(); > + > + ctx_param.context = ctx; > + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM); > + ctx_param.value--; > + TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EPERM); > + } > + > + igt_waitchildren(); > + } > + > + igt_subtest("root-set-no-zeromap") { > + ctx_param.context = ctx; > + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM); > + ctx_param.value--; > + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); > + } A simple functional test here which does: a) an execbuf with just 1 batch. With full ppgtt you should get that one at offset 0. If not, skip the testcase. b) set the NO_ZEROMAP property. c) re-run the same batch, assert that now the buffer is relocated to something non-0. Just to make sure we have a bare minimal testcase to make sure we don't break this. Thanks, Daniel > + > igt_fixture > close(fd); > } > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: > On Thu, May 21, 2015 at 12:44:53PM +0300, David Weinehall wrote: > > tests/gem_ctx_param_basic: Expand ctx_param tests > > > > Expand the context parameter tests to cover the > > no-zeromap parameter. > > > > Signed-off-by: David Weinehall <david.weinehall@intel.com> > > --- > > gem_ctx_param_basic.c | 24 +++++++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > diff --git a/tests/gem_ctx_param_basic.c b/tests/gem_ctx_param_basic.c > > index b44b37cf0538..ba9366d1a679 100644 > > --- a/tests/gem_ctx_param_basic.c > > +++ b/tests/gem_ctx_param_basic.c > > @@ -98,7 +98,7 @@ igt_main > > ctx_param.size = 0; > > } > > > > - ctx_param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD + 1; > > + ctx_param.param = I915_CONTEXT_PARAM_NO_ZEROMAP + 1; > > Please respin this one with a LOCAL_ define for NO_ZEROMAP. We generally > don't want to have a hard coupling between the headers in libdrm and igt, > would mean a libdrm release roughly every week ;-) Oh, sorry, that was a typo, will fix. > > > > igt_subtest("invalid-param-get") { > > ctx_param.context = ctx; > > @@ -132,6 +132,28 @@ igt_main > > TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); > > } > > > > + ctx_param.param = LOCAL_CONTEXT_PARAM_NO_ZEROMAP; > > + > > + igt_subtest("non-root-set-no-zeromap") { > > + igt_fork(child, 1) { > > + igt_drop_root(); > > + > > + ctx_param.context = ctx; > > + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM); > > + ctx_param.value--; > > + TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EPERM); > > + } > > + > > + igt_waitchildren(); > > + } > > + > > + igt_subtest("root-set-no-zeromap") { > > + ctx_param.context = ctx; > > + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM); > > + ctx_param.value--; > > + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); > > + } > > A simple functional test here which does: > a) an execbuf with just 1 batch. With full ppgtt you should get that one > at offset 0. If not, skip the testcase. > b) set the NO_ZEROMAP property. > c) re-run the same batch, assert that now the buffer is relocated to > something non-0. > > Just to make sure we have a bare minimal testcase to make sure we don't > break this. OK, will add that -- thanks for the input. Kind regards, David
On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: > A simple functional test here which does: > a) an execbuf with just 1 batch. With full ppgtt you should get that one > at offset 0. If not, skip the testcase. > b) set the NO_ZEROMAP property. > c) re-run the same batch, assert that now the buffer is relocated to > something non-0. > > Just to make sure we have a bare minimal testcase to make sure we don't > break this. Maybe this should be added to another test rather than here? This test is described as a: "Basic test for context set/get param input validation." Somehow I feel that testing whether the *functionality* is correct does not belong in this test, but rather in some test case that's already related to execbufs, or even a dedicated test case. But that might be over-engineering. Opinions? Kind regards, David
On Thu, May 28, 2015 at 05:53:17PM +0300, David Weinehall wrote: > On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: > > A simple functional test here which does: > > a) an execbuf with just 1 batch. With full ppgtt you should get that one > > at offset 0. If not, skip the testcase. > > b) set the NO_ZEROMAP property. > > c) re-run the same batch, assert that now the buffer is relocated to > > something non-0. > > > > Just to make sure we have a bare minimal testcase to make sure we don't > > break this. > > Maybe this should be added to another test rather than here? This test > is described as a: > > "Basic test for context set/get param input validation." > > Somehow I feel that testing whether the *functionality* is correct > does not belong in this test, but rather in some test case that's > already related to execbufs, or even a dedicated test case. > > But that might be over-engineering. Opinions? Yeah separate testcase would fit better, agreed. -Daniel
On Fri, May 29, 2015 at 09:52:52AM +0200, Daniel Vetter wrote: > On Thu, May 28, 2015 at 05:53:17PM +0300, David Weinehall wrote: > > On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: > > > A simple functional test here which does: > > > a) an execbuf with just 1 batch. With full ppgtt you should get that one > > > at offset 0. If not, skip the testcase. > > > b) set the NO_ZEROMAP property. > > > c) re-run the same batch, assert that now the buffer is relocated to > > > something non-0. > > > > > > Just to make sure we have a bare minimal testcase to make sure we don't > > > break this. > > > > Maybe this should be added to another test rather than here? This test > > is described as a: > > > > "Basic test for context set/get param input validation." > > > > Somehow I feel that testing whether the *functionality* is correct > > does not belong in this test, but rather in some test case that's > > already related to execbufs, or even a dedicated test case. > > > > But that might be over-engineering. Opinions? > > Yeah separate testcase would fit better, agreed. Update version of this patch is still missing. I'll need to revert the kernel side if this one doesn't show up soonish. Also you're breaking the invalid-flags testcase (did you bother to run them all and check for regressions?) which Jesse spotted, and with the new basic set this will be a P1 "I'm going to block everything" bug. -Daniel
On Thu, Aug 06, 2015 at 11:30:00PM +0200, Daniel Vetter wrote: > On Fri, May 29, 2015 at 09:52:52AM +0200, Daniel Vetter wrote: > > On Thu, May 28, 2015 at 05:53:17PM +0300, David Weinehall wrote: > > > On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: > > > > A simple functional test here which does: > > > > a) an execbuf with just 1 batch. With full ppgtt you should get that one > > > > at offset 0. If not, skip the testcase. > > > > b) set the NO_ZEROMAP property. > > > > c) re-run the same batch, assert that now the buffer is relocated to > > > > something non-0. > > > > > > > > Just to make sure we have a bare minimal testcase to make sure we don't > > > > break this. > > > > > > Maybe this should be added to another test rather than here? This test > > > is described as a: > > > > > > "Basic test for context set/get param input validation." > > > > > > Somehow I feel that testing whether the *functionality* is correct > > > does not belong in this test, but rather in some test case that's > > > already related to execbufs, or even a dedicated test case. > > > > > > But that might be over-engineering. Opinions? > > > > Yeah separate testcase would fit better, agreed. > > Update version of this patch is still missing. I'll need to revert the > kernel side if this one doesn't show up soonish. > > Also you're breaking the invalid-flags testcase (did you bother to run > them all and check for regressions?) which Jesse spotted, and with the new > basic set this will be a P1 "I'm going to block everything" bug. Forgot to add Jesse. -Daniel
On 08/06/2015 02:30 PM, Daniel Vetter wrote: > On Fri, May 29, 2015 at 09:52:52AM +0200, Daniel Vetter wrote: >> On Thu, May 28, 2015 at 05:53:17PM +0300, David Weinehall wrote: >>> On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: >>>> A simple functional test here which does: >>>> a) an execbuf with just 1 batch. With full ppgtt you should get that one >>>> at offset 0. If not, skip the testcase. >>>> b) set the NO_ZEROMAP property. >>>> c) re-run the same batch, assert that now the buffer is relocated to >>>> something non-0. >>>> >>>> Just to make sure we have a bare minimal testcase to make sure we don't >>>> break this. >>> >>> Maybe this should be added to another test rather than here? This test >>> is described as a: >>> >>> "Basic test for context set/get param input validation." >>> >>> Somehow I feel that testing whether the *functionality* is correct >>> does not belong in this test, but rather in some test case that's >>> already related to execbufs, or even a dedicated test case. >>> >>> But that might be over-engineering. Opinions? >> >> Yeah separate testcase would fit better, agreed. > > Update version of this patch is still missing. I'll need to revert the > kernel side if this one doesn't show up soonish. > > Also you're breaking the invalid-flags testcase (did you bother to run > them all and check for regressions?) which Jesse spotted, and with the new > basic set this will be a P1 "I'm going to block everything" bug. We really need man pages for new ioctls as well in libdrm. Jesse
On Thu, Aug 06, 2015 at 02:33:31PM -0700, Jesse Barnes wrote: > On 08/06/2015 02:30 PM, Daniel Vetter wrote: > > On Fri, May 29, 2015 at 09:52:52AM +0200, Daniel Vetter wrote: > >> On Thu, May 28, 2015 at 05:53:17PM +0300, David Weinehall wrote: > >>> On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: > >>>> A simple functional test here which does: > >>>> a) an execbuf with just 1 batch. With full ppgtt you should get that one > >>>> at offset 0. If not, skip the testcase. > >>>> b) set the NO_ZEROMAP property. > >>>> c) re-run the same batch, assert that now the buffer is relocated to > >>>> something non-0. > >>>> > >>>> Just to make sure we have a bare minimal testcase to make sure we don't > >>>> break this. > >>> > >>> Maybe this should be added to another test rather than here? This test > >>> is described as a: > >>> > >>> "Basic test for context set/get param input validation." > >>> > >>> Somehow I feel that testing whether the *functionality* is correct > >>> does not belong in this test, but rather in some test case that's > >>> already related to execbufs, or even a dedicated test case. > >>> > >>> But that might be over-engineering. Opinions? > >> > >> Yeah separate testcase would fit better, agreed. > > > > Update version of this patch is still missing. I'll need to revert the > > kernel side if this one doesn't show up soonish. > > > > Also you're breaking the invalid-flags testcase (did you bother to run > > them all and check for regressions?) which Jesse spotted, and with the new > > basic set this will be a P1 "I'm going to block everything" bug. > > We really need man pages for new ioctls as well in libdrm. Hmmm, this isn't a new ioctl, just a context parameter that can be set/queried using a pre-existing ioctl, but I can modify the existing manual page (if there is one?) to include information about the new parameter. Kind regards, David
On Thu, Aug 06, 2015 at 11:30:00PM +0200, Daniel Vetter wrote: [snip] > Update version of this patch is still missing. I'll need to revert the > kernel side if this one doesn't show up soonish. > > Also you're breaking the invalid-flags testcase (did you bother to run > them all and check for regressions?) which Jesse spotted, and with the new > basic set this will be a P1 "I'm going to block everything" bug. The patch I sent this Friday (the one I'd forgotten to send earlier) should work for the invalid-flags case, yes. Kind regards, David
On 08/10/2015 07:15 AM, David Weinehall wrote: > On Thu, Aug 06, 2015 at 02:33:31PM -0700, Jesse Barnes wrote: >> On 08/06/2015 02:30 PM, Daniel Vetter wrote: >>> On Fri, May 29, 2015 at 09:52:52AM +0200, Daniel Vetter wrote: >>>> On Thu, May 28, 2015 at 05:53:17PM +0300, David Weinehall wrote: >>>>> On Wed, May 27, 2015 at 01:32:10PM +0200, Daniel Vetter wrote: >>>>>> A simple functional test here which does: >>>>>> a) an execbuf with just 1 batch. With full ppgtt you should get that one >>>>>> at offset 0. If not, skip the testcase. >>>>>> b) set the NO_ZEROMAP property. >>>>>> c) re-run the same batch, assert that now the buffer is relocated to >>>>>> something non-0. >>>>>> >>>>>> Just to make sure we have a bare minimal testcase to make sure we don't >>>>>> break this. >>>>> >>>>> Maybe this should be added to another test rather than here? This test >>>>> is described as a: >>>>> >>>>> "Basic test for context set/get param input validation." >>>>> >>>>> Somehow I feel that testing whether the *functionality* is correct >>>>> does not belong in this test, but rather in some test case that's >>>>> already related to execbufs, or even a dedicated test case. >>>>> >>>>> But that might be over-engineering. Opinions? >>>> >>>> Yeah separate testcase would fit better, agreed. >>> >>> Update version of this patch is still missing. I'll need to revert the >>> kernel side if this one doesn't show up soonish. >>> >>> Also you're breaking the invalid-flags testcase (did you bother to run >>> them all and check for regressions?) which Jesse spotted, and with the new >>> basic set this will be a P1 "I'm going to block everything" bug. >> >> We really need man pages for new ioctls as well in libdrm. > > Hmmm, this isn't a new ioctl, just a context parameter that can be > set/queried using a pre-existing ioctl, but I can modify the existing > manual page (if there is one?) to include information about the new > parameter. Yeah we don't have one for this ioctl unfortunately. There are examples of other ioctl man pages in the libdrm repo though; you could copy one of those and do one for the context get/set ioctl. If you don't have any time, can you just file a JIRA instead? We'll get to it eventually... :) Thanks, Jesse
diff --git a/tests/gem_ctx_param_basic.c b/tests/gem_ctx_param_basic.c index b44b37cf0538..ba9366d1a679 100644 --- a/tests/gem_ctx_param_basic.c +++ b/tests/gem_ctx_param_basic.c @@ -98,7 +98,7 @@ igt_main ctx_param.size = 0; } - ctx_param.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD + 1; + ctx_param.param = I915_CONTEXT_PARAM_NO_ZEROMAP + 1; igt_subtest("invalid-param-get") { ctx_param.context = ctx; @@ -132,6 +132,28 @@ igt_main TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); } + ctx_param.param = LOCAL_CONTEXT_PARAM_NO_ZEROMAP; + + igt_subtest("non-root-set-no-zeromap") { + igt_fork(child, 1) { + igt_drop_root(); + + ctx_param.context = ctx; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM); + ctx_param.value--; + TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EPERM); + } + + igt_waitchildren(); + } + + igt_subtest("root-set-no-zeromap") { + ctx_param.context = ctx; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM); + ctx_param.value--; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); + } + igt_fixture close(fd); }
tests/gem_ctx_param_basic: Expand ctx_param tests Expand the context parameter tests to cover the no-zeromap parameter. Signed-off-by: David Weinehall <david.weinehall@intel.com> --- gem_ctx_param_basic.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)