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@xxxxxxxxx> 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@xxxxxxxx> > >> --- > >> 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); > >> + } > >> +} > > > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx