* [PATCH] tests: Add gem_exec_params
@ 2014-04-23 18:32 Daniel Vetter
2014-04-24 6:43 ` Zhao Yakui
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2014-04-23 18:32 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
This fills all the gaps we've had in our execbuf testing. Overflow
testing of the various arrays is already done by gem_reloc_overflow.
Also add kms_flip_tiling to .gitignore.
This will cause a bunch of failures since current kernels don't catch
all fallout.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
tests/.gitignore | 2 +
tests/Makefile.sources | 1 +
tests/gem_exec_params.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 215 insertions(+)
create mode 100644 tests/gem_exec_params.c
diff --git a/tests/.gitignore b/tests/.gitignore
index 146bab06b565..4c50bae93aa3 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -35,6 +35,7 @@ gem_exec_blt
gem_exec_faulting_reloc
gem_exec_lut_handle
gem_exec_nop
+gem_exec_params
gem_exec_parse
gem_fd_exhaustion
gem_fenced_exec_thrash
@@ -113,6 +114,7 @@ kms_addfb
kms_cursor_crc
kms_fbc_crc
kms_flip
+kms_flip_tiling
kms_pipe_crc_basic
kms_plane
kms_render
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index c957ace2ace0..9b2d7cff1113 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -29,6 +29,7 @@ TESTS_progs_M = \
gem_exec_bad_domains \
gem_exec_faulting_reloc \
gem_exec_nop \
+ gem_exec_params \
gem_exec_parse \
gem_fenced_exec_thrash \
gem_fence_thrash \
diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
new file mode 100644
index 000000000000..b1d996c530f5
--- /dev/null
+++ b/tests/gem_exec_params.c
@@ -0,0 +1,212 @@
+/*
+ * Copyright (c) 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Daniel Vetter
+ *
+ */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include "drm.h"
+
+#include "ioctl_wrappers.h"
+#include "drmtest.h"
+#include "intel_io.h"
+#include "intel_chipset.h"
+#include "igt_aux.h"
+
+#define LOCAL_I915_EXEC_VEBOX (4<<0)
+
+struct drm_i915_gem_execbuffer2 execbuf;
+struct drm_i915_gem_exec_object2 gem_exec[1];
+uint32_t batch[2] = {MI_BATCH_BUFFER_END};
+uint32_t handle, devid;
+int fd;
+
+igt_main
+{
+ igt_fixture {
+ fd = drm_open_any();
+
+ devid = intel_get_drm_devid(fd);
+
+ handle = gem_create(fd, 4096);
+ gem_write(fd, handle, 0, batch, sizeof(batch));
+
+ gem_exec[0].handle = handle;
+ gem_exec[0].relocation_count = 0;
+ gem_exec[0].relocs_ptr = 0;
+ gem_exec[0].alignment = 0;
+ gem_exec[0].offset = 0;
+ gem_exec[0].flags = 0;
+ gem_exec[0].rsvd1 = 0;
+ gem_exec[0].rsvd2 = 0;
+
+ execbuf.buffers_ptr = (uintptr_t)gem_exec;
+ execbuf.buffer_count = 1;
+ execbuf.batch_start_offset = 0;
+ execbuf.batch_len = 8;
+ execbuf.cliprects_ptr = 0;
+ execbuf.num_cliprects = 0;
+ execbuf.DR1 = 0;
+ execbuf.DR4 = 0;
+ execbuf.flags = 0;
+ i915_execbuffer2_set_context_id(execbuf, 0);
+ execbuf.rsvd2 = 0;
+ }
+
+ igt_subtest("control") {
+ igt_assert(drmIoctl(fd,
+ DRM_IOCTL_I915_GEM_EXECBUFFER2,
+ &execbuf) == 0);
+ execbuf.flags = I915_EXEC_RENDER;
+ igt_assert(drmIoctl(fd,
+ DRM_IOCTL_I915_GEM_EXECBUFFER2,
+ &execbuf) == 0);
+ }
+
+#define RUN_FAIL(expected_errno) do { \
+ igt_assert(drmIoctl(fd, \
+ DRM_IOCTL_I915_GEM_EXECBUFFER2, \
+ &execbuf) == -1); \
+ igt_assert_cmpint(errno, ==, expected_errno); \
+ } while(0)
+
+ igt_subtest("no-bsd") {
+ igt_require(!gem_has_bsd(fd));
+ execbuf.flags = I915_EXEC_BSD;
+ RUN_FAIL(EINVAL);
+ }
+ igt_subtest("no-blt") {
+ igt_require(!gem_has_blt(fd));
+ execbuf.flags = I915_EXEC_BLT;
+ RUN_FAIL(EINVAL);
+ }
+ igt_subtest("no-vebox") {
+ igt_require(!gem_has_vebox(fd));
+ execbuf.flags = LOCAL_I915_EXEC_VEBOX;
+ RUN_FAIL(EINVAL);
+ }
+ igt_subtest("invalid-ring") {
+ execbuf.flags = LOCAL_I915_EXEC_VEBOX+1;
+ RUN_FAIL(EINVAL);
+ }
+
+ igt_subtest("rel-constants-invalid-ring") {
+ igt_require(gem_has_bsd(fd));
+ execbuf.flags = I915_EXEC_BSD | I915_EXEC_CONSTANTS_ABSOLUTE;
+ RUN_FAIL(EINVAL);
+ }
+
+ igt_subtest("rel-constants-invalid-rel-gen5") {
+ igt_require(intel_gen(devid) > 5);
+ execbuf.flags = I915_EXEC_RENDER | I915_EXEC_CONSTANTS_REL_SURFACE;
+ RUN_FAIL(EINVAL);
+ }
+
+ igt_subtest("rel-constants-invalid") {
+ execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1);
+ RUN_FAIL(EINVAL);
+ }
+
+ igt_subtest("sol-reset-invalid") {
+ igt_require(gem_has_bsd(fd));
+ execbuf.flags = I915_EXEC_BSD | I915_EXEC_GEN7_SOL_RESET;
+ RUN_FAIL(EINVAL);
+ }
+
+ igt_subtest("sol-reset-not-gen7") {
+ igt_require(intel_gen(devid) != 7);
+ execbuf.flags = I915_EXEC_RENDER | I915_EXEC_GEN7_SOL_RESET;
+ RUN_FAIL(EINVAL);
+ }
+
+ igt_subtest("secure-non-root") {
+ igt_fork(child, 1) {
+ igt_drop_root();
+
+ execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
+ RUN_FAIL(EPERM);
+ }
+
+ igt_waitchildren();
+ }
+
+ igt_subtest("secure-non-master") {
+ do_or_die(drmDropMaster(fd));
+ execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
+ RUN_FAIL(EPERM);
+ do_or_die(drmSetMaster(fd));
+ igt_assert(drmIoctl(fd,
+ DRM_IOCTL_I915_GEM_EXECBUFFER2,
+ &execbuf) == 0);
+ }
+
+ /* 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);
+ RUN_FAIL(EINVAL);
+ }
+
+ /* rsvd1 aka context id is already exercised by gem_ctx_bad_exec */
+
+ igt_subtest("cliprects-invalid") {
+ igt_require(intel_gen(devid) >= 5);
+ execbuf.flags = 0;
+ execbuf.num_cliprects = 1;
+ RUN_FAIL(EINVAL);
+ execbuf.num_cliprects = 0;
+ }
+
+#define DIRT(name) \
+ igt_subtest(#name "-dirt") { \
+ execbuf.flags = 0; \
+ execbuf.name = 1; \
+ RUN_FAIL(EINVAL); \
+ execbuf.name = 0; \
+ }
+
+ DIRT(rsvd2);
+ DIRT(cliprects_ptr);
+ DIRT(DR1);
+ DIRT(DR4);
+#undef DIRT
+
+#undef RUN_FAIL
+
+ igt_fixture {
+ gem_close(fd, handle);
+
+ close(fd);
+ }
+}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tests: Add gem_exec_params
2014-04-23 18:32 [PATCH] tests: Add gem_exec_params Daniel Vetter
@ 2014-04-24 6:43 ` Zhao Yakui
2014-04-24 7:18 ` Daniel Vetter
0 siblings, 1 reply; 7+ messages in thread
From: Zhao Yakui @ 2014-04-24 6:43 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Wed, 2014-04-23 at 12:32 -0600, Daniel Vetter wrote:
> This fills all the gaps we've had in our execbuf testing. Overflow
> testing of the various arrays is already done by gem_reloc_overflow.
>
> Also add kms_flip_tiling to .gitignore.
>
> This will cause a bunch of failures since current kernels don't catch
> all fallout.
>
Very good patch. Except some small concerns, it is OK to me.
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> tests/.gitignore | 2 +
> tests/Makefile.sources | 1 +
> tests/gem_exec_params.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 215 insertions(+)
> create mode 100644 tests/gem_exec_params.c
>
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 146bab06b565..4c50bae93aa3 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -35,6 +35,7 @@ gem_exec_blt
> gem_exec_faulting_reloc
> gem_exec_lut_handle
> gem_exec_nop
> +gem_exec_params
> gem_exec_parse
> gem_fd_exhaustion
> gem_fenced_exec_thrash
> @@ -113,6 +114,7 @@ kms_addfb
> kms_cursor_crc
> kms_fbc_crc
> kms_flip
> +kms_flip_tiling
> kms_pipe_crc_basic
> kms_plane
> kms_render
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index c957ace2ace0..9b2d7cff1113 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -29,6 +29,7 @@ TESTS_progs_M = \
> gem_exec_bad_domains \
> gem_exec_faulting_reloc \
> gem_exec_nop \
> + gem_exec_params \
> gem_exec_parse \
> gem_fenced_exec_thrash \
> gem_fence_thrash \
> diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
> new file mode 100644
> index 000000000000..b1d996c530f5
> --- /dev/null
> +++ b/tests/gem_exec_params.c
> @@ -0,0 +1,212 @@
> +/*
> + * Copyright (c) 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Daniel Vetter
> + *
> + */
> +
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include "drm.h"
> +
> +#include "ioctl_wrappers.h"
> +#include "drmtest.h"
> +#include "intel_io.h"
> +#include "intel_chipset.h"
> +#include "igt_aux.h"
> +
> +#define LOCAL_I915_EXEC_VEBOX (4<<0)
> +
> +struct drm_i915_gem_execbuffer2 execbuf;
> +struct drm_i915_gem_exec_object2 gem_exec[1];
> +uint32_t batch[2] = {MI_BATCH_BUFFER_END};
> +uint32_t handle, devid;
> +int fd;
> +
> +igt_main
> +{
> + igt_fixture {
> + fd = drm_open_any();
> +
> + devid = intel_get_drm_devid(fd);
> +
> + handle = gem_create(fd, 4096);
> + gem_write(fd, handle, 0, batch, sizeof(batch));
> +
> + gem_exec[0].handle = handle;
> + gem_exec[0].relocation_count = 0;
> + gem_exec[0].relocs_ptr = 0;
> + gem_exec[0].alignment = 0;
> + gem_exec[0].offset = 0;
> + gem_exec[0].flags = 0;
> + gem_exec[0].rsvd1 = 0;
> + gem_exec[0].rsvd2 = 0;
> +
> + execbuf.buffers_ptr = (uintptr_t)gem_exec;
> + execbuf.buffer_count = 1;
> + execbuf.batch_start_offset = 0;
> + execbuf.batch_len = 8;
Can we use the sizeof(batch) instead of 8?
> + execbuf.cliprects_ptr = 0;
> + execbuf.num_cliprects = 0;
> + execbuf.DR1 = 0;
> + execbuf.DR4 = 0;
> + execbuf.flags = 0;
> + i915_execbuffer2_set_context_id(execbuf, 0);
> + execbuf.rsvd2 = 0;
> + }
> +
> + igt_subtest("control") {
> + igt_assert(drmIoctl(fd,
> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
> + &execbuf) == 0);
> + execbuf.flags = I915_EXEC_RENDER;
> + igt_assert(drmIoctl(fd,
> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
> + &execbuf) == 0);
> + }
> +
> +#define RUN_FAIL(expected_errno) do { \
> + igt_assert(drmIoctl(fd, \
> + DRM_IOCTL_I915_GEM_EXECBUFFER2, \
> + &execbuf) == -1); \
> + igt_assert_cmpint(errno, ==, expected_errno); \
> + } while(0)
> +
> + igt_subtest("no-bsd") {
> + igt_require(!gem_has_bsd(fd));
> + execbuf.flags = I915_EXEC_BSD;
> + RUN_FAIL(EINVAL);
> + }
> + igt_subtest("no-blt") {
> + igt_require(!gem_has_blt(fd));
> + execbuf.flags = I915_EXEC_BLT;
> + RUN_FAIL(EINVAL);
> + }
> + igt_subtest("no-vebox") {
> + igt_require(!gem_has_vebox(fd));
> + execbuf.flags = LOCAL_I915_EXEC_VEBOX;
> + RUN_FAIL(EINVAL);
> + }
> + igt_subtest("invalid-ring") {
> + execbuf.flags = LOCAL_I915_EXEC_VEBOX+1;
> + RUN_FAIL(EINVAL);
> + }
> +
> + igt_subtest("rel-constants-invalid-ring") {
> + igt_require(gem_has_bsd(fd));
> + execbuf.flags = I915_EXEC_BSD | I915_EXEC_CONSTANTS_ABSOLUTE;
> + RUN_FAIL(EINVAL);
> + }
> +
> + igt_subtest("rel-constants-invalid-rel-gen5") {
> + igt_require(intel_gen(devid) > 5);
> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_CONSTANTS_REL_SURFACE;
> + RUN_FAIL(EINVAL);
> + }
> +
> + igt_subtest("rel-constants-invalid") {
> + execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1);
> + RUN_FAIL(EINVAL);
It seems that the exec.flags is the same as "I915_EXEC_BSD |
I915_EXEC_CONSTANTS_REL_SURFACE). And then it is similar to subtest of
rel-constants-invalid-ring. Not sure whether you are hoping to set the
flag as "I915_EXEC_RENDER | I915_EXEC_CONSTANTS_MASK"?
> + }
> +
> + igt_subtest("sol-reset-invalid") {
> + igt_require(gem_has_bsd(fd));
> + execbuf.flags = I915_EXEC_BSD | I915_EXEC_GEN7_SOL_RESET;
> + RUN_FAIL(EINVAL);
> + }
> +
> + igt_subtest("sol-reset-not-gen7") {
> + igt_require(intel_gen(devid) != 7);
> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_GEN7_SOL_RESET;
> + RUN_FAIL(EINVAL);
> + }
> +
> + igt_subtest("secure-non-root") {
> + igt_fork(child, 1) {
> + igt_drop_root();
> +
> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
> + RUN_FAIL(EPERM);
> + }
> +
> + igt_waitchildren();
> + }
> +
> + igt_subtest("secure-non-master") {
> + do_or_die(drmDropMaster(fd));
> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
> + RUN_FAIL(EPERM);
> + do_or_die(drmSetMaster(fd));
> + igt_assert(drmIoctl(fd,
> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
> + &execbuf) == 0);
> + }
> +
> + /* 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);
> + RUN_FAIL(EINVAL);
> + }
> +
Can we use (random() & __I915_EXEC_UNKNOWN_FLAGS) for the invalid-flag
subtest?
> + /* rsvd1 aka context id is already exercised by gem_ctx_bad_exec */
> +
> + igt_subtest("cliprects-invalid") {
> + igt_require(intel_gen(devid) >= 5);
> + execbuf.flags = 0;
> + execbuf.num_cliprects = 1;
> + RUN_FAIL(EINVAL);
> + execbuf.num_cliprects = 0;
> + }
> +
> +#define DIRT(name) \
> + igt_subtest(#name "-dirt") { \
> + execbuf.flags = 0; \
> + execbuf.name = 1; \
> + RUN_FAIL(EINVAL); \
> + execbuf.name = 0; \
> + }
> +
> + DIRT(rsvd2);
> + DIRT(cliprects_ptr);
> + DIRT(DR1);
> + DIRT(DR4);
> +#undef DIRT
> +
> +#undef RUN_FAIL
> +
> + igt_fixture {
> + gem_close(fd, handle);
> +
> + close(fd);
> + }
> +}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tests: Add gem_exec_params
2014-04-24 6:43 ` Zhao Yakui
@ 2014-04-24 7:18 ` Daniel Vetter
2014-04-24 7:55 ` Ville Syrjälä
2014-04-24 8:27 ` Zhao Yakui
0 siblings, 2 replies; 7+ messages in thread
From: Daniel Vetter @ 2014-04-24 7:18 UTC (permalink / raw)
To: Zhao Yakui; +Cc: Intel Graphics Development
On Thu, Apr 24, 2014 at 8:43 AM, Zhao Yakui <yakui.zhao@intel.com> wrote:
> On Wed, 2014-04-23 at 12:32 -0600, Daniel Vetter wrote:
>> This fills all the gaps we've had in our execbuf testing. Overflow
>> testing of the various arrays is already done by gem_reloc_overflow.
>>
>> Also add kms_flip_tiling to .gitignore.
>>
>> This will cause a bunch of failures since current kernels don't catch
>> all fallout.
>>
>
> Very good patch. Except some small concerns, it is OK to me.
Thanks for your comments, replies below.
-Daniel
>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>> tests/.gitignore | 2 +
>> tests/Makefile.sources | 1 +
>> tests/gem_exec_params.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 215 insertions(+)
>> create mode 100644 tests/gem_exec_params.c
>>
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index 146bab06b565..4c50bae93aa3 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -35,6 +35,7 @@ gem_exec_blt
>> gem_exec_faulting_reloc
>> gem_exec_lut_handle
>> gem_exec_nop
>> +gem_exec_params
>> gem_exec_parse
>> gem_fd_exhaustion
>> gem_fenced_exec_thrash
>> @@ -113,6 +114,7 @@ kms_addfb
>> kms_cursor_crc
>> kms_fbc_crc
>> kms_flip
>> +kms_flip_tiling
>> kms_pipe_crc_basic
>> kms_plane
>> kms_render
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index c957ace2ace0..9b2d7cff1113 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -29,6 +29,7 @@ TESTS_progs_M = \
>> gem_exec_bad_domains \
>> gem_exec_faulting_reloc \
>> gem_exec_nop \
>> + gem_exec_params \
>> gem_exec_parse \
>> gem_fenced_exec_thrash \
>> gem_fence_thrash \
>> diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
>> new file mode 100644
>> index 000000000000..b1d996c530f5
>> --- /dev/null
>> +++ b/tests/gem_exec_params.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + * Copyright (c) 2014 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the next
>> + * paragraph) shall be included in all copies or substantial portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + * Daniel Vetter
>> + *
>> + */
>> +
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <fcntl.h>
>> +#include <inttypes.h>
>> +#include <errno.h>
>> +#include <sys/stat.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/time.h>
>> +#include "drm.h"
>> +
>> +#include "ioctl_wrappers.h"
>> +#include "drmtest.h"
>> +#include "intel_io.h"
>> +#include "intel_chipset.h"
>> +#include "igt_aux.h"
>> +
>> +#define LOCAL_I915_EXEC_VEBOX (4<<0)
>> +
>> +struct drm_i915_gem_execbuffer2 execbuf;
>> +struct drm_i915_gem_exec_object2 gem_exec[1];
>> +uint32_t batch[2] = {MI_BATCH_BUFFER_END};
>> +uint32_t handle, devid;
>> +int fd;
>> +
>> +igt_main
>> +{
>> + igt_fixture {
>> + fd = drm_open_any();
>> +
>> + devid = intel_get_drm_devid(fd);
>> +
>> + handle = gem_create(fd, 4096);
>> + gem_write(fd, handle, 0, batch, sizeof(batch));
>> +
>> + gem_exec[0].handle = handle;
>> + gem_exec[0].relocation_count = 0;
>> + gem_exec[0].relocs_ptr = 0;
>> + gem_exec[0].alignment = 0;
>> + gem_exec[0].offset = 0;
>> + gem_exec[0].flags = 0;
>> + gem_exec[0].rsvd1 = 0;
>> + gem_exec[0].rsvd2 = 0;
>> +
>> + execbuf.buffers_ptr = (uintptr_t)gem_exec;
>> + execbuf.buffer_count = 1;
>> + execbuf.batch_start_offset = 0;
>> + execbuf.batch_len = 8;
>
> Can we use the sizeof(batch) instead of 8?
We use noop batches like this all over the place and it's kinda all
hard-coded magic numbers. Constructing execbufs manually is one of
those areas in igt which are rather painful, but thus far I just
didn't come up with a nice approach to it.
Hence I think leaving all the brittle magic in there is ok ;-)
>> + execbuf.cliprects_ptr = 0;
>> + execbuf.num_cliprects = 0;
>> + execbuf.DR1 = 0;
>> + execbuf.DR4 = 0;
>> + execbuf.flags = 0;
>> + i915_execbuffer2_set_context_id(execbuf, 0);
>> + execbuf.rsvd2 = 0;
>> + }
>> +
>> + igt_subtest("control") {
>> + igt_assert(drmIoctl(fd,
>> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
>> + &execbuf) == 0);
>> + execbuf.flags = I915_EXEC_RENDER;
>> + igt_assert(drmIoctl(fd,
>> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
>> + &execbuf) == 0);
>> + }
>> +
>> +#define RUN_FAIL(expected_errno) do { \
>> + igt_assert(drmIoctl(fd, \
>> + DRM_IOCTL_I915_GEM_EXECBUFFER2, \
>> + &execbuf) == -1); \
>> + igt_assert_cmpint(errno, ==, expected_errno); \
>> + } while(0)
>> +
>> + igt_subtest("no-bsd") {
>> + igt_require(!gem_has_bsd(fd));
>> + execbuf.flags = I915_EXEC_BSD;
>> + RUN_FAIL(EINVAL);
>> + }
>> + igt_subtest("no-blt") {
>> + igt_require(!gem_has_blt(fd));
>> + execbuf.flags = I915_EXEC_BLT;
>> + RUN_FAIL(EINVAL);
>> + }
>> + igt_subtest("no-vebox") {
>> + igt_require(!gem_has_vebox(fd));
>> + execbuf.flags = LOCAL_I915_EXEC_VEBOX;
>> + RUN_FAIL(EINVAL);
>> + }
>> + igt_subtest("invalid-ring") {
>> + execbuf.flags = LOCAL_I915_EXEC_VEBOX+1;
>> + RUN_FAIL(EINVAL);
>> + }
>> +
>> + igt_subtest("rel-constants-invalid-ring") {
>> + igt_require(gem_has_bsd(fd));
>> + execbuf.flags = I915_EXEC_BSD | I915_EXEC_CONSTANTS_ABSOLUTE;
>> + RUN_FAIL(EINVAL);
>> + }
>> +
>> + igt_subtest("rel-constants-invalid-rel-gen5") {
>> + igt_require(intel_gen(devid) > 5);
>> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_CONSTANTS_REL_SURFACE;
>> + RUN_FAIL(EINVAL);
>> + }
>> +
>> + igt_subtest("rel-constants-invalid") {
>> + execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1);
>> + RUN_FAIL(EINVAL);
>
> It seems that the exec.flags is the same as "I915_EXEC_BSD |
> I915_EXEC_CONSTANTS_REL_SURFACE). And then it is similar to subtest of
> rel-constants-invalid-ring. Not sure whether you are hoping to set the
> flag as "I915_EXEC_RENDER | I915_EXEC_CONSTANTS_MASK"?
They're three completely different checks:
1. checks for invalid flags on rings other than RENDER
2. checks for a specific invalid flag which doesn't exist on gen5+ any more
3. checks for a completely invalid flag (notice the + 1) on any platform
All three cases hit different conditions in the execbuf validataion code.
>> + }
>> +
>> + igt_subtest("sol-reset-invalid") {
>> + igt_require(gem_has_bsd(fd));
>> + execbuf.flags = I915_EXEC_BSD | I915_EXEC_GEN7_SOL_RESET;
>> + RUN_FAIL(EINVAL);
>> + }
>> +
>> + igt_subtest("sol-reset-not-gen7") {
>> + igt_require(intel_gen(devid) != 7);
>> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_GEN7_SOL_RESET;
>> + RUN_FAIL(EINVAL);
>> + }
>> +
>> + igt_subtest("secure-non-root") {
>> + igt_fork(child, 1) {
>> + igt_drop_root();
>> +
>> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
>> + RUN_FAIL(EPERM);
>> + }
>> +
>> + igt_waitchildren();
>> + }
>> +
>> + igt_subtest("secure-non-master") {
>> + do_or_die(drmDropMaster(fd));
>> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
>> + RUN_FAIL(EPERM);
>> + do_or_die(drmSetMaster(fd));
>> + igt_assert(drmIoctl(fd,
>> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
>> + &execbuf) == 0);
>> + }
>> +
>> + /* 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);
>> + RUN_FAIL(EINVAL);
>> + }
>> +
>
> Can we use (random() & __I915_EXEC_UNKNOWN_FLAGS) for the invalid-flag
> subtest?
I've picked the very next flag since that will be the one we use for
the next execbuf feature. If people fail to update this testcase then
it will fail and I have a good excuse to scold them for not writing
and running igt testcase ;-)
So random() is imo less useful here.
>> + /* rsvd1 aka context id is already exercised by gem_ctx_bad_exec */
>> +
>> + igt_subtest("cliprects-invalid") {
>> + igt_require(intel_gen(devid) >= 5);
>> + execbuf.flags = 0;
>> + execbuf.num_cliprects = 1;
>> + RUN_FAIL(EINVAL);
>> + execbuf.num_cliprects = 0;
>> + }
>> +
>> +#define DIRT(name) \
>> + igt_subtest(#name "-dirt") { \
>> + execbuf.flags = 0; \
>> + execbuf.name = 1; \
>> + RUN_FAIL(EINVAL); \
>> + execbuf.name = 0; \
>> + }
>> +
>> + DIRT(rsvd2);
>> + DIRT(cliprects_ptr);
>> + DIRT(DR1);
>> + DIRT(DR4);
>> +#undef DIRT
>> +
>> +#undef RUN_FAIL
>> +
>> + igt_fixture {
>> + gem_close(fd, handle);
>> +
>> + close(fd);
>> + }
>> +}
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tests: Add gem_exec_params
2014-04-24 7:18 ` Daniel Vetter
@ 2014-04-24 7:55 ` Ville Syrjälä
2014-04-24 8:46 ` [PATCH] tests/gem_exec_params: Fix rel-constants-invalid subtest Daniel Vetter
2014-04-24 10:05 ` [PATCH] tests: Add gem_exec_params Daniel Vetter
2014-04-24 8:27 ` Zhao Yakui
1 sibling, 2 replies; 7+ messages in thread
From: Ville Syrjälä @ 2014-04-24 7:55 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Thu, Apr 24, 2014 at 09:18:24AM +0200, Daniel Vetter wrote:
> On Thu, Apr 24, 2014 at 8:43 AM, Zhao Yakui <yakui.zhao@intel.com> wrote:
> > On Wed, 2014-04-23 at 12:32 -0600, Daniel Vetter wrote:
> >> + igt_subtest("rel-constants-invalid") {
> >> + execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1);
> >> + RUN_FAIL(EINVAL);
> >
> > It seems that the exec.flags is the same as "I915_EXEC_BSD |
> > I915_EXEC_CONSTANTS_REL_SURFACE). And then it is similar to subtest of
> > rel-constants-invalid-ring. Not sure whether you are hoping to set the
> > flag as "I915_EXEC_RENDER | I915_EXEC_CONSTANTS_MASK"?
>
> They're three completely different checks:
> 1. checks for invalid flags on rings other than RENDER
> 2. checks for a specific invalid flag which doesn't exist on gen5+ any more
> 3. checks for a completely invalid flag (notice the + 1) on any platform
I think the point was that I915_EXEC_RENDER+1 == I915_EXEC_BSD. Hence
the +1 is entirely bogus. So you want either
I915_EXEC_CONSTANTS_REL_SURFACE+(1<<6) or just
I915_EXEC_CONSTANTS_MASK.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tests: Add gem_exec_params
2014-04-24 7:18 ` Daniel Vetter
2014-04-24 7:55 ` Ville Syrjälä
@ 2014-04-24 8:27 ` Zhao Yakui
1 sibling, 0 replies; 7+ messages in thread
From: Zhao Yakui @ 2014-04-24 8:27 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Thu, 2014-04-24 at 01:18 -0600, Daniel Vetter wrote:
> On Thu, Apr 24, 2014 at 8:43 AM, Zhao Yakui <yakui.zhao@intel.com> wrote:
> > On Wed, 2014-04-23 at 12:32 -0600, Daniel Vetter wrote:
> >> This fills all the gaps we've had in our execbuf testing. Overflow
> >> testing of the various arrays is already done by gem_reloc_overflow.
> >>
> >> Also add kms_flip_tiling to .gitignore.
> >>
> >> This will cause a bunch of failures since current kernels don't catch
> >> all fallout.
> >>
> >
> > Very good patch. Except some small concerns, it is OK to me.
>
> Thanks for your comments, replies below.
> -Daniel
>
> >
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> ---
> >> tests/.gitignore | 2 +
> >> tests/Makefile.sources | 1 +
> >> tests/gem_exec_params.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 215 insertions(+)
> >> create mode 100644 tests/gem_exec_params.c
> >>
> >> diff --git a/tests/.gitignore b/tests/.gitignore
> >> index 146bab06b565..4c50bae93aa3 100644
> >> --- a/tests/.gitignore
> >> +++ b/tests/.gitignore
> >> @@ -35,6 +35,7 @@ gem_exec_blt
> >> gem_exec_faulting_reloc
> >> gem_exec_lut_handle
> >> gem_exec_nop
> >> +gem_exec_params
> >> gem_exec_parse
> >> gem_fd_exhaustion
> >> gem_fenced_exec_thrash
> >> @@ -113,6 +114,7 @@ kms_addfb
> >> kms_cursor_crc
> >> kms_fbc_crc
> >> kms_flip
> >> +kms_flip_tiling
> >> kms_pipe_crc_basic
> >> kms_plane
> >> kms_render
> >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> >> index c957ace2ace0..9b2d7cff1113 100644
> >> --- a/tests/Makefile.sources
> >> +++ b/tests/Makefile.sources
> >> @@ -29,6 +29,7 @@ TESTS_progs_M = \
> >> gem_exec_bad_domains \
> >> gem_exec_faulting_reloc \
> >> gem_exec_nop \
> >> + gem_exec_params \
> >> gem_exec_parse \
> >> gem_fenced_exec_thrash \
> >> gem_fence_thrash \
> >> diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
> >> new file mode 100644
> >> index 000000000000..b1d996c530f5
> >> --- /dev/null
> >> +++ b/tests/gem_exec_params.c
> >> @@ -0,0 +1,212 @@
> >> +/*
> >> + * Copyright (c) 2014 Intel Corporation
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the "Software"),
> >> + * to deal in the Software without restriction, including without limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including the next
> >> + * paragraph) shall be included in all copies or substantial portions of the
> >> + * Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >> + * IN THE SOFTWARE.
> >> + *
> >> + * Authors:
> >> + * Daniel Vetter
> >> + *
> >> + */
> >> +
> >> +#include <unistd.h>
> >> +#include <stdlib.h>
> >> +#include <stdint.h>
> >> +#include <stdio.h>
> >> +#include <string.h>
> >> +#include <fcntl.h>
> >> +#include <inttypes.h>
> >> +#include <errno.h>
> >> +#include <sys/stat.h>
> >> +#include <sys/ioctl.h>
> >> +#include <sys/time.h>
> >> +#include "drm.h"
> >> +
> >> +#include "ioctl_wrappers.h"
> >> +#include "drmtest.h"
> >> +#include "intel_io.h"
> >> +#include "intel_chipset.h"
> >> +#include "igt_aux.h"
> >> +
> >> +#define LOCAL_I915_EXEC_VEBOX (4<<0)
> >> +
> >> +struct drm_i915_gem_execbuffer2 execbuf;
> >> +struct drm_i915_gem_exec_object2 gem_exec[1];
> >> +uint32_t batch[2] = {MI_BATCH_BUFFER_END};
> >> +uint32_t handle, devid;
> >> +int fd;
> >> +
> >> +igt_main
> >> +{
> >> + igt_fixture {
> >> + fd = drm_open_any();
> >> +
> >> + devid = intel_get_drm_devid(fd);
> >> +
> >> + handle = gem_create(fd, 4096);
> >> + gem_write(fd, handle, 0, batch, sizeof(batch));
> >> +
> >> + gem_exec[0].handle = handle;
> >> + gem_exec[0].relocation_count = 0;
> >> + gem_exec[0].relocs_ptr = 0;
> >> + gem_exec[0].alignment = 0;
> >> + gem_exec[0].offset = 0;
> >> + gem_exec[0].flags = 0;
> >> + gem_exec[0].rsvd1 = 0;
> >> + gem_exec[0].rsvd2 = 0;
> >> +
> >> + execbuf.buffers_ptr = (uintptr_t)gem_exec;
> >> + execbuf.buffer_count = 1;
> >> + execbuf.batch_start_offset = 0;
> >> + execbuf.batch_len = 8;
> >
> > Can we use the sizeof(batch) instead of 8?
>
> We use noop batches like this all over the place and it's kinda all
> hard-coded magic numbers. Constructing execbufs manually is one of
> those areas in igt which are rather painful, but thus far I just
> didn't come up with a nice approach to it.
>
> Hence I think leaving all the brittle magic in there is ok ;-)
Understand. The magic number is no problem.
>
> >> + execbuf.cliprects_ptr = 0;
> >> + execbuf.num_cliprects = 0;
> >> + execbuf.DR1 = 0;
> >> + execbuf.DR4 = 0;
> >> + execbuf.flags = 0;
> >> + i915_execbuffer2_set_context_id(execbuf, 0);
> >> + execbuf.rsvd2 = 0;
> >> + }
> >> +
> >> + igt_subtest("control") {
> >> + igt_assert(drmIoctl(fd,
> >> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
> >> + &execbuf) == 0);
> >> + execbuf.flags = I915_EXEC_RENDER;
> >> + igt_assert(drmIoctl(fd,
> >> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
> >> + &execbuf) == 0);
> >> + }
> >> +
> >> +#define RUN_FAIL(expected_errno) do { \
> >> + igt_assert(drmIoctl(fd, \
> >> + DRM_IOCTL_I915_GEM_EXECBUFFER2, \
> >> + &execbuf) == -1); \
> >> + igt_assert_cmpint(errno, ==, expected_errno); \
> >> + } while(0)
> >> +
> >> + igt_subtest("no-bsd") {
> >> + igt_require(!gem_has_bsd(fd));
> >> + execbuf.flags = I915_EXEC_BSD;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> + igt_subtest("no-blt") {
> >> + igt_require(!gem_has_blt(fd));
> >> + execbuf.flags = I915_EXEC_BLT;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> + igt_subtest("no-vebox") {
> >> + igt_require(!gem_has_vebox(fd));
> >> + execbuf.flags = LOCAL_I915_EXEC_VEBOX;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> + igt_subtest("invalid-ring") {
> >> + execbuf.flags = LOCAL_I915_EXEC_VEBOX+1;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> +
> >> + igt_subtest("rel-constants-invalid-ring") {
> >> + igt_require(gem_has_bsd(fd));
> >> + execbuf.flags = I915_EXEC_BSD | I915_EXEC_CONSTANTS_ABSOLUTE;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> +
> >> + igt_subtest("rel-constants-invalid-rel-gen5") {
> >> + igt_require(intel_gen(devid) > 5);
> >> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_CONSTANTS_REL_SURFACE;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> +
> >> + igt_subtest("rel-constants-invalid") {
> >> + execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1);
> >> + RUN_FAIL(EINVAL);
> >
> > It seems that the exec.flags is the same as "I915_EXEC_BSD |
> > I915_EXEC_CONSTANTS_REL_SURFACE). And then it is similar to subtest of
> > rel-constants-invalid-ring. Not sure whether you are hoping to set the
> > flag as "I915_EXEC_RENDER | I915_EXEC_CONSTANTS_MASK"?
>
> They're three completely different checks:
> 1. checks for invalid flags on rings other than RENDER
> 2. checks for a specific invalid flag which doesn't exist on gen5+ any more
> 3. checks for a completely invalid flag (notice the + 1) on any platform
>
> All three cases hit different conditions in the execbuf validataion code.
I understand that they are completely different.
But as Ville mentioned, the definition of "
I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1)" is bogus.
This is my point.
Maybe you are hoping the :
I915_EXEC_CONSTANTS_REL_SURFACE+(1<<6) or just
I915_EXEC_CONSTANTS_MASK
>
> >> + }
> >> +
> >> + igt_subtest("sol-reset-invalid") {
> >> + igt_require(gem_has_bsd(fd));
> >> + execbuf.flags = I915_EXEC_BSD | I915_EXEC_GEN7_SOL_RESET;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> +
> >> + igt_subtest("sol-reset-not-gen7") {
> >> + igt_require(intel_gen(devid) != 7);
> >> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_GEN7_SOL_RESET;
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> +
> >> + igt_subtest("secure-non-root") {
> >> + igt_fork(child, 1) {
> >> + igt_drop_root();
> >> +
> >> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
> >> + RUN_FAIL(EPERM);
> >> + }
> >> +
> >> + igt_waitchildren();
> >> + }
> >> +
> >> + igt_subtest("secure-non-master") {
> >> + do_or_die(drmDropMaster(fd));
> >> + execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
> >> + RUN_FAIL(EPERM);
> >> + do_or_die(drmSetMaster(fd));
> >> + igt_assert(drmIoctl(fd,
> >> + DRM_IOCTL_I915_GEM_EXECBUFFER2,
> >> + &execbuf) == 0);
> >> + }
> >> +
> >> + /* 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);
> >> + RUN_FAIL(EINVAL);
> >> + }
> >> +
> >
> > Can we use (random() & __I915_EXEC_UNKNOWN_FLAGS) for the invalid-flag
> > subtest?
>
> I've picked the very next flag since that will be the one we use for
> the next execbuf feature. If people fail to update this testcase then
> it will fail and I have a good excuse to scold them for not writing
> and running igt testcase ;-)
>
> So random() is imo less useful here.
OK. Understand it. Sorry for my incorrect understanding.
>
> >> + /* rsvd1 aka context id is already exercised by gem_ctx_bad_exec */
> >> +
> >> + igt_subtest("cliprects-invalid") {
> >> + igt_require(intel_gen(devid) >= 5);
> >> + execbuf.flags = 0;
> >> + execbuf.num_cliprects = 1;
> >> + RUN_FAIL(EINVAL);
> >> + execbuf.num_cliprects = 0;
> >> + }
> >> +
> >> +#define DIRT(name) \
> >> + igt_subtest(#name "-dirt") { \
> >> + execbuf.flags = 0; \
> >> + execbuf.name = 1; \
> >> + RUN_FAIL(EINVAL); \
> >> + execbuf.name = 0; \
> >> + }
> >> +
> >> + DIRT(rsvd2);
> >> + DIRT(cliprects_ptr);
> >> + DIRT(DR1);
> >> + DIRT(DR4);
> >> +#undef DIRT
> >> +
> >> +#undef RUN_FAIL
> >> +
> >> + igt_fixture {
> >> + gem_close(fd, handle);
> >> +
> >> + close(fd);
> >> + }
> >> +}
> >
> >
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] tests/gem_exec_params: Fix rel-constants-invalid subtest
2014-04-24 7:55 ` Ville Syrjälä
@ 2014-04-24 8:46 ` Daniel Vetter
2014-04-24 10:05 ` [PATCH] tests: Add gem_exec_params Daniel Vetter
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2014-04-24 8:46 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Reported by Ville and Zhao Yakui.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Zhao Yakui <yakui.zhao@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
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 b1d996c530f5..306039c244e3 100644
--- a/tests/gem_exec_params.c
+++ b/tests/gem_exec_params.c
@@ -134,7 +134,7 @@ igt_main
}
igt_subtest("rel-constants-invalid") {
- execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1);
+ execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+(1<<6));
RUN_FAIL(EINVAL);
}
--
1.8.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tests: Add gem_exec_params
2014-04-24 7:55 ` Ville Syrjälä
2014-04-24 8:46 ` [PATCH] tests/gem_exec_params: Fix rel-constants-invalid subtest Daniel Vetter
@ 2014-04-24 10:05 ` Daniel Vetter
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2014-04-24 10:05 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Daniel Vetter, Intel Graphics Development
On Thu, Apr 24, 2014 at 10:55:47AM +0300, Ville Syrjälä wrote:
> On Thu, Apr 24, 2014 at 09:18:24AM +0200, Daniel Vetter wrote:
> > On Thu, Apr 24, 2014 at 8:43 AM, Zhao Yakui <yakui.zhao@intel.com> wrote:
> > > On Wed, 2014-04-23 at 12:32 -0600, Daniel Vetter wrote:
> > >> + igt_subtest("rel-constants-invalid") {
> > >> + execbuf.flags = I915_EXEC_RENDER | (I915_EXEC_CONSTANTS_REL_SURFACE+1);
> > >> + RUN_FAIL(EINVAL);
> > >
> > > It seems that the exec.flags is the same as "I915_EXEC_BSD |
> > > I915_EXEC_CONSTANTS_REL_SURFACE). And then it is similar to subtest of
> > > rel-constants-invalid-ring. Not sure whether you are hoping to set the
> > > flag as "I915_EXEC_RENDER | I915_EXEC_CONSTANTS_MASK"?
> >
> > They're three completely different checks:
> > 1. checks for invalid flags on rings other than RENDER
> > 2. checks for a specific invalid flag which doesn't exist on gen5+ any more
> > 3. checks for a completely invalid flag (notice the + 1) on any platform
>
> I think the point was that I915_EXEC_RENDER+1 == I915_EXEC_BSD. Hence
> the +1 is entirely bogus. So you want either
> I915_EXEC_CONSTANTS_REL_SURFACE+(1<<6) or just
> I915_EXEC_CONSTANTS_MASK.
Yeah, pardon for being blind ;-) Will fix up the test and push a fixup.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-04-24 10:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-23 18:32 [PATCH] tests: Add gem_exec_params Daniel Vetter
2014-04-24 6:43 ` Zhao Yakui
2014-04-24 7:18 ` Daniel Vetter
2014-04-24 7:55 ` Ville Syrjälä
2014-04-24 8:46 ` [PATCH] tests/gem_exec_params: Fix rel-constants-invalid subtest Daniel Vetter
2014-04-24 10:05 ` [PATCH] tests: Add gem_exec_params Daniel Vetter
2014-04-24 8:27 ` Zhao Yakui
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.