[PATCH i-g-t 1/2] tests/gem_create: Explicitly check for -EINVAL on create-invalid-size

[PATCH i-g-t 1/2] tests/gem_create: Explicitly check for -EINVAL on create-invalid-size

[Joonas Lahtinen]

Merge the remaining testing coverage delta from gem_bad_length and
check that creating an object of zero size fails with -EINVAL.

Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_create.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/gem_create.c b/tests/gem_create.c
index de7b8209..655f7a4e 100644
--- a/tests/gem_create.c
+++ b/tests/gem_create.c
@@ -70,17 +70,18 @@ struct local_i915_gem_create_v2 {
 	uint32_t pad;
 #define I915_CREATE_PLACEMENT_STOLEN (1<<0)
 	uint32_t flags;
-} create;
+};
 
 #define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
 
 static void invalid_flag_test(int fd)
 {
+	struct local_i915_gem_create_v2 create;
 	int ret;
 
 	gem_require_stolen_support(fd);
 
-	create.handle = 0;
+	CLEAR(create);
 	create.size = PAGE_SIZE;
 	create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
 	ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
@@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
 
 static void invalid_size_test(int fd)
 {
-	int handle;
+	struct i915_gem_create create;
+
+	CLEAR(create);
+	create.size = 0;
+	igt_assert_eq(igt_ioctl(fd, IOCTL_I915_GEM_CREATE, &create), -1);
 
-	handle = __gem_create(fd, 0);
-	igt_assert(!handle);
+	igt_assert_eq(errno, EINVAL);
 }
 
 /*
-- 
2.13.6

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

[PATCH i-g-t 2/2] tests: Drop gem_bad_length as redundant

[Joonas Lahtinen]

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Most of the gem_bad_length code is compiled out because creating
a zero-length object is not allowed anymore by i915 and thus it is
not possible to execute it. The remaining part checks that creation of
a zero-length object does indeed fail, which is also checked by
gem_create/create-invalid-size.
The only difference between the 2 tests is that gem_bad_length checks
specifically for EINVAL while create-invalid-size only looks for
failure, so after adding the EINVAL check to create-invalid-size it is
safe to drop gem_bad_length.

v2 (Joonas):
- Clarify header line, split the patch, fix Meson build and add Cc:

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/Makefile.sources |   1 -
 tests/gem_bad_length.c | 133 -------------------------------------------------
 tests/meson.build      |   1 -
 3 files changed, 135 deletions(-)
 delete mode 100644 tests/gem_bad_length.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index c4d320eb..142ff2bb 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -40,7 +40,6 @@ TESTS_progs = \
 	drv_module_reload \
 	drv_selftest \
 	drv_suspend \
-	gem_bad_length \
 	gem_bad_reloc \
 	gem_basic \
 	gem_busy \
diff --git a/tests/gem_bad_length.c b/tests/gem_bad_length.c
deleted file mode 100644
index cca0145e..00000000
--- a/tests/gem_bad_length.c
+++ /dev/null
@@ -1,133 +0,0 @@
-/*
- * Copyright © 2011 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:
- *    Chris Wilson <chris@chris-wilson.co.uk>
- *
- */
-
-#include "igt.h"
-#include <unistd.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/ioctl.h>
-#include "drm.h"
-
-IGT_TEST_DESCRIPTION("Test minimal bo_create and batchbuffer exec.");
-
-/*
- * Testcase: Minimal bo_create and batchbuffer exec
- *
- * Originally this caught an kernel oops due to the unchecked assumption that
- * objects have size > 0.
- */
-
-static uint32_t do_gem_create(int fd, int size, int *retval)
-{
-	struct drm_i915_gem_create create;
-	int ret;
-
-	create.handle = 0;
-	create.size = (size + 4095) & -4096;
-	ret = drmIoctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
-	igt_assert(retval || ret == 0);
-	if (retval)
-		*retval = errno;
-
-	return create.handle;
-}
-
-#if 0
-static int gem_exec(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
-{
-	return drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, execbuf);
-}
-#endif
-
-static void create0(int fd)
-{
-	int retval = 0;
-	igt_info("trying to create a zero-length gem object\n");
-	do_gem_create(fd, 0, &retval);
-	igt_assert(retval == EINVAL);
-}
-
-#if 0
-static void exec0(int fd)
-{
-	struct drm_i915_gem_execbuffer2 execbuf;
-	struct drm_i915_gem_exec_object2 exec[1];
-	uint32_t buf[2] = { MI_BATCH_BUFFER_END, 0 };
-
-	/* Just try executing with a zero-length bo.
-	 * We expect the kernel to either accept the nop batch, or reject it
-	 * for the zero-length buffer, but never crash.
-	 */
-
-	exec[0].handle = gem_create(fd, 4096);
-	gem_write(fd, exec[0].handle, 0, buf, sizeof(buf));
-	exec[0].relocation_count = 0;
-	exec[0].relocs_ptr = 0;
-	exec[0].alignment = 0;
-	exec[0].offset = 0;
-	exec[0].flags = 0;
-	exec[0].rsvd1 = 0;
-	exec[0].rsvd2 = 0;
-
-	execbuf.buffers_ptr = (uintptr_t)exec;
-	execbuf.buffer_count = 1;
-	execbuf.batch_start_offset = 0;
-	execbuf.batch_len = sizeof(buf);
-	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_info("trying to run an empty batchbuffer\n");
-	gem_exec(fd, &execbuf);
-
-	gem_close(fd, exec[0].handle);
-}
-#endif
-
-igt_simple_main
-{
-	int fd;
-
-	igt_skip_on_simulation();
-
-	fd = drm_open_driver(DRIVER_INTEL);
-
-	create0(fd);
-
-	//exec0(fd);
-
-	close(fd);
-}
diff --git a/tests/meson.build b/tests/meson.build
index 6cb3584a..85666f6d 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -20,7 +20,6 @@ test_progs = [
 	'drv_module_reload',
 	'drv_selftest',
 	'drv_suspend',
-	'gem_bad_length',
 	'gem_bad_reloc',
 	'gem_basic',
 	'gem_busy',
-- 
2.13.6

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

Re: [PATCH i-g-t 1/2] tests/gem_create: Explicitly check for -EINVAL on create-invalid-size

[Chris Wilson]

Quoting Joonas Lahtinen (2017-10-03 12:47:33)
> Merge the remaining testing coverage delta from gem_bad_length and
> check that creating an object of zero size fails with -EINVAL.
> 
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/gem_create.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/gem_create.c b/tests/gem_create.c
> index de7b8209..655f7a4e 100644
> --- a/tests/gem_create.c
> +++ b/tests/gem_create.c
> @@ -70,17 +70,18 @@ struct local_i915_gem_create_v2 {
>         uint32_t pad;
>  #define I915_CREATE_PLACEMENT_STOLEN (1<<0)
>         uint32_t flags;
> -} create;
> +};
>  
>  #define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
>  
>  static void invalid_flag_test(int fd)
>  {
> +       struct local_i915_gem_create_v2 create;
>         int ret;
>  
>         gem_require_stolen_support(fd);
>  
> -       create.handle = 0;
> +       CLEAR(create);
>         create.size = PAGE_SIZE;
>         create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
>         ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)

First chunk, 
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Second chunk, I think we should fix __gem_create() to follow the
convention of returning the error code.
-Chris

>  static void invalid_size_test(int fd)
>  {
> -       int handle;
> +       struct i915_gem_create create;
> +
> +       CLEAR(create);
> +       create.size = 0;
> +       igt_assert_eq(igt_ioctl(fd, IOCTL_I915_GEM_CREATE, &create), -1);
>  
> -       handle = __gem_create(fd, 0);
> -       igt_assert(!handle);
> +       igt_assert_eq(errno, EINVAL);
>  }
>  
>  /*
> -- 
> 2.13.6
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/2] tests/gem_create: Explicitly check for -EINVAL on create-invalid-size

[Patchwork]

== Series Details ==

Series: series starting with [1/2] tests/gem_create: Explicitly check for -EINVAL on create-invalid-size
URL   : https://patchwork.freedesktop.org/series/31316/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
08a2f887b26c89cfb1e1b5715a23d561b75e0dab lib: Reduce dependency on glib

make  all-recursive
Making all in lib
make  all-recursive
Making all in .
Making all in tests
make[4]: Nothing to be done for 'all'.
Making all in man
make[2]: Nothing to be done for 'all'.
Making all in tools
Making all in null_state_gen
make[3]: Nothing to be done for 'all'.
Making all in registers
make[3]: Nothing to be done for 'all'.
make[3]: Nothing to be done for 'all-am'.
Making all in scripts
make[2]: Nothing to be done for 'all'.
Making all in benchmarks
Making all in wsim
make[3]: Nothing to be done for 'all'.
Making all in ezbench.d
make[3]: Nothing to be done for 'all'.
make[3]: Nothing to be done for 'all-am'.
Making all in tests
Making all in intel-ci
make[3]: Nothing to be done for 'all'.
  CCLD     gem_bad_batch
  CCLD     gem_hang
  CCLD     gem_bad_blit
  CCLD     gem_bad_address
  CCLD     gem_non_secure_batch
  CCLD     gem_stress
  CCLD     core_auth
  CCLD     core_get_client_auth
  CCLD     core_getclient
  CCLD     core_getstats
  CCLD     core_getversion
  CCLD     core_prop_blob
  CCLD     core_setmaster_vs_auth
  CCLD     debugfs_test
  CCLD     drm_import_export
  CCLD     drm_mm
  CCLD     drm_read
  CCLD     drm_vma_limiter
  CCLD     drm_vma_limiter_cached
  CCLD     drm_vma_limiter_cpu
  CCLD     drm_vma_limiter_gtt
  CCLD     drv_getparams_basic
  CCLD     drv_hangman
  CCLD     drv_missed_irq
  CCLD     drv_module_reload
  CCLD     drv_selftest
  CCLD     drv_suspend
  CCLD     gem_bad_reloc
  CCLD     gem_basic
  CCLD     gem_busy
  CCLD     gem_caching
  CCLD     gem_close
  CCLD     gem_close_race
  CCLD     gem_concurrent_blit
  CCLD     gem_cpu_reloc
  CC       gem_create.o
gem_create.c: In function ‘invalid_size_test’:
gem_create.c:99:25: error: storage size of ‘create’ isn’t known
  struct i915_gem_create create;
                         ^~~~~~
In file included from ./../lib/intel_batchbuffer.h:8:0,
                 from gem_create.c:52:
gem_create.c:103:30: error: ‘IOCTL_I915_GEM_CREATE’ undeclared (first use in this function)
  igt_assert_eq(igt_ioctl(fd, IOCTL_I915_GEM_CREATE, &create), -1);
                              ^
./../lib/igt_core.h:396:15: note: in definition of macro ‘igt_assert_cmpint’
   int __n1 = (n1), __n2 = (n2); \
               ^~
gem_create.c:103:2: note: in expansion of macro ‘igt_assert_eq’
  igt_assert_eq(igt_ioctl(fd, IOCTL_I915_GEM_CREATE, &create), -1);
  ^~~~~~~~~~~~~
gem_create.c:103:30: note: each undeclared identifier is reported only once for each function it appears in
  igt_assert_eq(igt_ioctl(fd, IOCTL_I915_GEM_CREATE, &create), -1);
                              ^
./../lib/igt_core.h:396:15: note: in definition of macro ‘igt_assert_cmpint’
   int __n1 = (n1), __n2 = (n2); \
               ^~
gem_create.c:103:2: note: in expansion of macro ‘igt_assert_eq’
  igt_assert_eq(igt_ioctl(fd, IOCTL_I915_GEM_CREATE, &create), -1);
  ^~~~~~~~~~~~~
gem_create.c:99:25: warning: unused variable ‘create’ [-Wunused-variable]
  struct i915_gem_create create;
                         ^~~~~~
Makefile:3947: recipe for target 'gem_create.o' failed
make[3]: *** [gem_create.o] Error 1
Makefile:4432: recipe for target 'all-recursive' failed
make[2]: *** [all-recursive] Error 1
Makefile:533: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
Makefile:465: recipe for target 'all' failed
make: *** [all] Error 2

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

Re: [PATCH i-g-t 1/2] tests/gem_create: Explicitly check for -EINVAL on create-invalid-size

[Joonas Lahtinen]

On Tue, 2017-10-03 at 12:52 +0100, Chris Wilson wrote:
> Quoting Joonas Lahtinen (2017-10-03 12:47:33)
> > Merge the remaining testing coverage delta from gem_bad_length and
> > check that creating an object of zero size fails with -EINVAL.
> > 
> > Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/gem_create.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tests/gem_create.c b/tests/gem_create.c
> > index de7b8209..655f7a4e 100644
> > --- a/tests/gem_create.c
> > +++ b/tests/gem_create.c
> > @@ -70,17 +70,18 @@ struct local_i915_gem_create_v2 {
> >         uint32_t pad;
> >  #define I915_CREATE_PLACEMENT_STOLEN (1<<0)
> >         uint32_t flags;
> > -} create;
> > +};
> >  
> >  #define LOCAL_IOCTL_I915_GEM_CREATE       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
> >  
> >  static void invalid_flag_test(int fd)
> >  {
> > +       struct local_i915_gem_create_v2 create;
> >         int ret;
> >  
> >         gem_require_stolen_support(fd);
> >  
> > -       create.handle = 0;
> > +       CLEAR(create);
> >         create.size = PAGE_SIZE;
> >         create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
> >         ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> > @@ -95,10 +96,13 @@ static void invalid_flag_test(int fd)
> 
> First chunk, 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Second chunk, I think we should fix __gem_create() to follow the
> convention of returning the error code.

That's indeed better, I reviewed it. Please merge it first, I'll rebase
these two patches.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 2/2] tests: Drop gem_bad_length as redundant

[Chris Wilson]

Quoting Joonas Lahtinen (2017-10-03 12:47:34)
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Most of the gem_bad_length code is compiled out because creating
> a zero-length object is not allowed anymore by i915 and thus it is
> not possible to execute it. The remaining part checks that creation of
> a zero-length object does indeed fail, which is also checked by
> gem_create/create-invalid-size.
> The only difference between the 2 tests is that gem_bad_length checks
> specifically for EINVAL while create-invalid-size only looks for
> failure, so after adding the EINVAL check to create-invalid-size it is
> safe to drop gem_bad_length.
> 
> v2 (Joonas):
> - Clarify header line, split the patch, fix Meson build and add Cc:
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx