Re: [RFC i-g-t 1/1] tests/gem_bad_address: Fix and update gem_bad_address

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxx>
>   *    Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> (based on gem_bad_blit.c)
> + *    Antonio Argenziano <antonio.argenziano@xxxxxxxxx>
>   *
>   */
>  
>  #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.



-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux