Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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
> > ---
> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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
> ---
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 4/3] tests/gem_ctx_param_basic: Expand ctx_param tests
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
---
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;
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);
}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
