| Message ID | 20170531170037.35344-2-antonio.argenziano@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
If this ends up having further revisions, please split it into two (three?) separate changes: 1) Introduction of tests_hw directory 2) Moving (and changing) gem_bad_address to tests_hw [3) Changing gem_bad_address] First target of bikeshedding is the directory name. I kind of like hw_tests more, it flows nicely with BUILD_HW_TESTS and pals, but I'm not too fuzzed about that. More comments inline. On Wed, May 31, 2017 at 10:00:37AM -0700, Antonio Argenziano wrote: > When writing to an invalid memory location, the HW should be clever > enough to silently discard the write without disrupting execution. > gem_bad_address aim at just that. The test has been updated to move away > from the libDrm wrappers and use the IOCTL wrappers instead. Also the > invalid address has been updated to be just outside of the GTT space. > > Since the test is considered to be HW focused, meaning that it doesn't > really exercise the deriver but rather an HW feature, a new folder has > been created to host such tests. The commit moves the test in the newly > created folder for HW focused tests. > > Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> > --- > Makefile.am | 4 ++ > configure.ac | 11 ++++++ > tests_hw/Makefile.am | 36 ++++++++++++++++++ > tests_hw/Makefile.sources | 8 ++++ > {tests => tests_hw}/gem_bad_address.c | 69 ++++++++++++++++++++--------------- > 5 files changed, 98 insertions(+), 30 deletions(-) > create mode 100644 tests_hw/Makefile.am > create mode 100644 tests_hw/Makefile.sources > rename {tests => tests_hw}/gem_bad_address.c (50%) > > diff --git a/Makefile.am b/Makefile.am > index 60168628..dca76bf0 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -23,6 +23,10 @@ ACLOCAL_AMFLAGS = ${ACLOCAL_FLAGS} -I m4 > > SUBDIRS = lib man tools scripts benchmarks > > +if BUILD_HW_TESTS > +SUBDIRS += tests_hw > +endif > + > if BUILD_TESTS > SUBDIRS += tests > endif > diff --git a/configure.ac b/configure.ac > index 5342e33c..85885f25 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -333,6 +333,15 @@ fi > AM_CONDITIONAL(BUILD_TESTS, [test "x$BUILD_TESTS" = xyes]) > AC_DEFINE_UNQUOTED(TARGET_CPU_PLATFORM, ["$host_cpu"], [Target platform]) > > +AC_ARG_ENABLE(tests_hw, > + AS_HELP_STRING([--disable-hw-tests], > + [Disable HW tests build (default: enabled)]), > + [BUILD_HW_TESTS=$enableval], [BUILD_HW_TESTS="yes"]) > +if test "x$BUILD_TESTS" = xyes; then > + AC_DEFINE(BUILD_HW_TESTS, 1, [Build hw tests]) > +fi > +AM_CONDITIONAL(BUILD_HW_TESTS, [test "x$BUILD_HW_TESTS" = xyes]) > + > files="broadwell cherryview haswell ivybridge sandybridge valleyview skylake" > for file in $files; do > REGISTER_FILES="$REGISTER_FILES $file `cat $srcdir/tools/registers/$file`" > @@ -353,6 +362,7 @@ AC_CONFIG_FILES([ > man/Makefile > scripts/Makefile > tests/Makefile > + tests_hw/Makefile > tools/Makefile > tools/null_state_gen/Makefile > tools/registers/Makefile > @@ -376,6 +386,7 @@ echo "Intel GPU tools" > echo "" > echo " • Tests:" > echo " Build tests : ${BUILD_TESTS}" > +echo " Build hw tests : ${BUILD_HW_TESTS}" > echo " Compile prime tests: ${NOUVEAU}" > echo " Print stack traces : ${with_libunwind}" > echo " Debug flags : ${DEBUG_CFLAGS}" > diff --git a/tests_hw/Makefile.am b/tests_hw/Makefile.am > new file mode 100644 > index 00000000..36ed9eb9 > --- /dev/null > +++ b/tests_hw/Makefile.am > @@ -0,0 +1,36 @@ > +include Makefile.sources > + > +if BUILD_HW_TESTS > + > +hw-test-list.txt: Makefile.sources > + @echo TESTLIST > $@ > + @echo ${hw_tests_all} >> $@ > + @echo TESTLIST >> $@ > + > +all-local: .gitignore > +.gitignore: Makefile.sources > + @echo "$(HW_TESTS_PROGS) hw-test-list.txt .gitignore" | sed 's/\s\+/\n/g' | sort > $@ > + > +pkglibexec_PROGRAMS = \ > + $(HW_TESTS_PROGS) \ > + $(NULL) > + > +pkgdata_DATA = hw-test-list.txt > + > +CLEANFILES = hw-test-list.txt .gitignore > + > +AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\ > + -I$(srcdir)/.. \ > + -I$(srcdir)/../lib \ > + -include "$(srcdir)/../lib/check-ndebug.h" \ > + -DIGT_SRCDIR=\""$(abs_srcdir)"\" \ > + -DIGT_DATADIR=\""$(pkgdatadir)"\" \ > + $(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \ > + $(NULL) > + > +LDADD = ../lib/libintel_tools.la $(GLIB_LIBS) $(XMLRPC_LIBS) $(DLOPEN_LIBS) > + > +AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS) > +AM_LDFLAGS = -Wl,--as-needed > + > +endif > diff --git a/tests_hw/Makefile.sources b/tests_hw/Makefile.sources > new file mode 100644 > index 00000000..25d181c0 > --- /dev/null > +++ b/tests_hw/Makefile.sources > @@ -0,0 +1,8 @@ > +HW_TESTS_PROGS = \ > + gem_bad_address \ > + $(NULL) > + > +#This target contains all testcases > +hw_tests_all = \ > + $(HW_TESTS_PROGS) \ > + $(NULL) > diff --git a/tests/gem_bad_address.c b/tests_hw/gem_bad_address.c > similarity index 50% > rename from tests/gem_bad_address.c > rename to tests_hw/gem_bad_address.c > index a970dfa4..c254f894 100644 > --- a/tests/gem_bad_address.c > +++ b/tests_hw/gem_bad_address.c > @@ -23,37 +23,53 @@ > * Authors: > * Eric Anholt <eric@anholt.net> > * Jesse Barnes <jbarnes@virtuousgeek.org> (based on gem_bad_blit.c) > + * Antonio Argenziano <antonio.argenziano@intel.com> > * > */ > > #include "igt.h" > -#include <stdlib.h> > -#include <stdio.h> > -#include <string.h> > -#include <fcntl.h> > -#include <inttypes.h> > -#include <errno.h> > -#include <sys/stat.h> > -#include <sys/time.h> > -#include "drm.h" > -#include "intel_bufmgr.h" > > -static drm_intel_bufmgr *bufmgr; > -struct intel_batchbuffer *batch; > - > -#define BAD_GTT_DEST ((512*1024*1024)) /* past end of aperture */ > +/* > + * This test aims at verifying that writing to an invalid location in memory, > + * doesn't cause hangs. The store command should be ignored completely by the > + * HW and the whole process should be transparent to the user. Therefore, > + * the test doesn't perform any validation check but expects the wrapping > + * execution environment to check no hangs have occurred. The situation today is that there is no execution environment to even run this test when it's in tests_hw. Should probably mark the lack of such an environment as a TODO here. When this test fails, what actually happens? Hard system hang? Recoverable gpu hang? Unrecoverable gpu hang? > + * > + * The test needs to send a privileged batch to be able to write to the GTT. > + */ > > static void > -bad_store(void) > +bad_store(uint32_t fd, uint32_t engine) > { > - BEGIN_BATCH(4, 0); > - OUT_BATCH(MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL | 1 << 21); > - OUT_BATCH(0); > - OUT_BATCH(BAD_GTT_DEST); > - OUT_BATCH(0xdeadbeef); > - ADVANCE_BATCH(); > + struct drm_i915_gem_exec_object2 obj; > + struct drm_i915_gem_execbuffer2 execbuf; > + > + uint32_t batch[16]; > + int i = 0; > + > + memset(&obj, 0, sizeof(obj)); > + memset(&execbuf, 0, sizeof(execbuf)); > + > + execbuf.buffers_ptr = to_user_pointer(&obj); > + execbuf.buffer_count = 1; > + execbuf.flags = engine; > + execbuf.flags |= I915_EXEC_SECURE; > > - intel_batchbuffer_flush(batch); > + obj.handle = gem_create(fd, 4096); > + > + batch[i++] = MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL; > + batch[i++] = 0x0; //Low part of the GTT address = 4GByte > + batch[i++] = 0x1; //High part of the GTT address > GTT size > + batch[i++] = 0xdeadbeef; > + > + batch[i++] = MI_BATCH_BUFFER_END; > + batch[i++] = 0x0; > + > + gem_write(fd, obj.handle, 0, batch, sizeof(batch)); > + gem_execbuf(fd, &execbuf); > + > + gem_close(fd, obj.handle); > } Use tabs for indentation here.
diff --git a/Makefile.am b/Makefile.am index 60168628..dca76bf0 100644 --- a/Makefile.am +++ b/Makefile.am @@ -23,6 +23,10 @@ ACLOCAL_AMFLAGS = ${ACLOCAL_FLAGS} -I m4 SUBDIRS = lib man tools scripts benchmarks +if BUILD_HW_TESTS +SUBDIRS += tests_hw +endif + if BUILD_TESTS SUBDIRS += tests endif diff --git a/configure.ac b/configure.ac index 5342e33c..85885f25 100644 --- a/configure.ac +++ b/configure.ac @@ -333,6 +333,15 @@ fi AM_CONDITIONAL(BUILD_TESTS, [test "x$BUILD_TESTS" = xyes]) AC_DEFINE_UNQUOTED(TARGET_CPU_PLATFORM, ["$host_cpu"], [Target platform]) +AC_ARG_ENABLE(tests_hw, + AS_HELP_STRING([--disable-hw-tests], + [Disable HW tests build (default: enabled)]), + [BUILD_HW_TESTS=$enableval], [BUILD_HW_TESTS="yes"]) +if test "x$BUILD_TESTS" = xyes; then + AC_DEFINE(BUILD_HW_TESTS, 1, [Build hw tests]) +fi +AM_CONDITIONAL(BUILD_HW_TESTS, [test "x$BUILD_HW_TESTS" = xyes]) + files="broadwell cherryview haswell ivybridge sandybridge valleyview skylake" for file in $files; do REGISTER_FILES="$REGISTER_FILES $file `cat $srcdir/tools/registers/$file`" @@ -353,6 +362,7 @@ AC_CONFIG_FILES([ man/Makefile scripts/Makefile tests/Makefile + tests_hw/Makefile tools/Makefile tools/null_state_gen/Makefile tools/registers/Makefile @@ -376,6 +386,7 @@ echo "Intel GPU tools" echo "" echo " • Tests:" echo " Build tests : ${BUILD_TESTS}" +echo " Build hw tests : ${BUILD_HW_TESTS}" echo " Compile prime tests: ${NOUVEAU}" echo " Print stack traces : ${with_libunwind}" echo " Debug flags : ${DEBUG_CFLAGS}" diff --git a/tests_hw/Makefile.am b/tests_hw/Makefile.am new file mode 100644 index 00000000..36ed9eb9 --- /dev/null +++ b/tests_hw/Makefile.am @@ -0,0 +1,36 @@ +include Makefile.sources + +if BUILD_HW_TESTS + +hw-test-list.txt: Makefile.sources + @echo TESTLIST > $@ + @echo ${hw_tests_all} >> $@ + @echo TESTLIST >> $@ + +all-local: .gitignore +.gitignore: Makefile.sources + @echo "$(HW_TESTS_PROGS) hw-test-list.txt .gitignore" | sed 's/\s\+/\n/g' | sort > $@ + +pkglibexec_PROGRAMS = \ + $(HW_TESTS_PROGS) \ + $(NULL) + +pkgdata_DATA = hw-test-list.txt + +CLEANFILES = hw-test-list.txt .gitignore + +AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\ + -I$(srcdir)/.. \ + -I$(srcdir)/../lib \ + -include "$(srcdir)/../lib/check-ndebug.h" \ + -DIGT_SRCDIR=\""$(abs_srcdir)"\" \ + -DIGT_DATADIR=\""$(pkgdatadir)"\" \ + $(LIBUNWIND_CFLAGS) $(WERROR_CFLAGS) \ + $(NULL) + +LDADD = ../lib/libintel_tools.la $(GLIB_LIBS) $(XMLRPC_LIBS) $(DLOPEN_LIBS) + +AM_CFLAGS += $(CAIRO_CFLAGS) $(LIBUDEV_CFLAGS) $(GLIB_CFLAGS) +AM_LDFLAGS = -Wl,--as-needed + +endif diff --git a/tests_hw/Makefile.sources b/tests_hw/Makefile.sources new file mode 100644 index 00000000..25d181c0 --- /dev/null +++ b/tests_hw/Makefile.sources @@ -0,0 +1,8 @@ +HW_TESTS_PROGS = \ + gem_bad_address \ + $(NULL) + +#This target contains all testcases +hw_tests_all = \ + $(HW_TESTS_PROGS) \ + $(NULL) diff --git a/tests/gem_bad_address.c b/tests_hw/gem_bad_address.c similarity index 50% rename from tests/gem_bad_address.c rename to tests_hw/gem_bad_address.c index a970dfa4..c254f894 100644 --- a/tests/gem_bad_address.c +++ b/tests_hw/gem_bad_address.c @@ -23,37 +23,53 @@ * Authors: * Eric Anholt <eric@anholt.net> * Jesse Barnes <jbarnes@virtuousgeek.org> (based on gem_bad_blit.c) + * Antonio Argenziano <antonio.argenziano@intel.com> * */ #include "igt.h" -#include <stdlib.h> -#include <stdio.h> -#include <string.h> -#include <fcntl.h> -#include <inttypes.h> -#include <errno.h> -#include <sys/stat.h> -#include <sys/time.h> -#include "drm.h" -#include "intel_bufmgr.h" -static drm_intel_bufmgr *bufmgr; -struct intel_batchbuffer *batch; - -#define BAD_GTT_DEST ((512*1024*1024)) /* past end of aperture */ +/* + * This test aims at verifying that writing to an invalid location in memory, + * doesn't cause hangs. The store command should be ignored completely by the + * HW and the whole process should be transparent to the user. Therefore, + * the test doesn't perform any validation check but expects the wrapping + * execution environment to check no hangs have occurred. + * + * The test needs to send a privileged batch to be able to write to the GTT. + */ static void -bad_store(void) +bad_store(uint32_t fd, uint32_t engine) { - BEGIN_BATCH(4, 0); - OUT_BATCH(MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL | 1 << 21); - OUT_BATCH(0); - OUT_BATCH(BAD_GTT_DEST); - OUT_BATCH(0xdeadbeef); - ADVANCE_BATCH(); + struct drm_i915_gem_exec_object2 obj; + struct drm_i915_gem_execbuffer2 execbuf; + + uint32_t batch[16]; + int i = 0; + + memset(&obj, 0, sizeof(obj)); + memset(&execbuf, 0, sizeof(execbuf)); + + execbuf.buffers_ptr = to_user_pointer(&obj); + execbuf.buffer_count = 1; + execbuf.flags = engine; + execbuf.flags |= I915_EXEC_SECURE; - intel_batchbuffer_flush(batch); + obj.handle = gem_create(fd, 4096); + + batch[i++] = MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL; + batch[i++] = 0x0; //Low part of the GTT address = 4GByte + batch[i++] = 0x1; //High part of the GTT address > GTT size + batch[i++] = 0xdeadbeef; + + batch[i++] = MI_BATCH_BUFFER_END; + batch[i++] = 0x0; + + gem_write(fd, obj.handle, 0, batch, sizeof(batch)); + gem_execbuf(fd, &execbuf); + + gem_close(fd, obj.handle); } igt_simple_main @@ -62,14 +78,7 @@ igt_simple_main fd = drm_open_driver(DRIVER_INTEL); - bufmgr = drm_intel_bufmgr_gem_init(fd, 4096); - drm_intel_bufmgr_gem_enable_reuse(bufmgr); - batch = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd)); - - bad_store(); - - intel_batchbuffer_free(batch); - drm_intel_bufmgr_destroy(bufmgr); + bad_store(fd, I915_EXEC_BLT); close(fd); }
When writing to an invalid memory location, the HW should be clever enough to silently discard the write without disrupting execution. gem_bad_address aim at just that. The test has been updated to move away from the libDrm wrappers and use the IOCTL wrappers instead. Also the invalid address has been updated to be just outside of the GTT space. Since the test is considered to be HW focused, meaning that it doesn't really exercise the deriver but rather an HW feature, a new folder has been created to host such tests. The commit moves the test in the newly created folder for HW focused tests. Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> --- Makefile.am | 4 ++ configure.ac | 11 ++++++ tests_hw/Makefile.am | 36 ++++++++++++++++++ tests_hw/Makefile.sources | 8 ++++ {tests => tests_hw}/gem_bad_address.c | 69 ++++++++++++++++++++--------------- 5 files changed, 98 insertions(+), 30 deletions(-) create mode 100644 tests_hw/Makefile.am create mode 100644 tests_hw/Makefile.sources rename {tests => tests_hw}/gem_bad_address.c (50%)