| Message ID | 20200220170819.31579-1-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,v2] lib/i915: Restrict mmap types to GTT if no MMAP_OFFSET support | expand |
Please ignore this submission, the code is broken. Sorry for that. Janusz On Thursday, February 20, 2020 6:08:19 PM CET Janusz Krzysztofik wrote: > Commit b0da8bb705c0 ("lib/i915: for_each_mmap_offset_type()") > introduced a macro that makes it easy to repeat a test body within a > loop for each mmap-offset mapping type supported by v4 of i915 MMAP_GTT > API. However, when run on an older version of the driver, those > subtests are believed to be still repeated for each known mmap-offset > mapping type while effectively exercising GTT mapping type only. As > that may be confusing, fix it. > > It has been assumed that the modified macro is still suitable for use > inside gem_mmap_offset test itself. Would that not be case, > gem_mmap_offset could redefine the macro back to its initial form for > internal use. > > v2: Move extra condition to a separate function and call it via > for_each_if(), in case we need to fix it again in future (Chris) > > Suggested-by: Michał Winiarski <michal.winiarski@intel.com> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > lib/i915/gem_mman.c | 5 +++++ > lib/i915/gem_mman.h | 7 +++++-- > tests/i915/gem_ctx_sseu.c | 2 +- > tests/i915/gem_exec_params.c | 2 +- > tests/i915/gem_madvise.c | 18 ++++++++++++++---- > tests/i915/gem_mmap_offset.c | 10 +++++----- > tests/i915/i915_pm_rpm.c | 2 +- > 7 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c > index 08ae67696..6fa8d1e8b 100644 > --- a/lib/i915/gem_mman.c > +++ b/lib/i915/gem_mman.c > @@ -60,6 +60,11 @@ bool gem_has_mmap_offset(int fd) > return gtt_version >= 4; > } > > +bool gem_has_mmap_offset_type(int fd, const struct mmap_offset *t); > +{ > + return gem_has_mmap_offset(fd) || t->type == I915_MMAP_OFFSET_GTT; > +} > + > /** > * __gem_mmap__gtt: > * @fd: open i915 drm file descriptor > diff --git a/lib/i915/gem_mman.h b/lib/i915/gem_mman.h > index 4fc6a0186..2c4a7a00b 100644 > --- a/lib/i915/gem_mman.h > +++ b/lib/i915/gem_mman.h > @@ -101,10 +101,13 @@ extern const struct mmap_offset { > unsigned int domain; > } mmap_offset_types[]; > > -#define for_each_mmap_offset_type(__t) \ > +bool gem_has_mmap_offset_type(int fd, const struct mmap_offset *t); > + > +#define for_each_mmap_offset_type(fd, __t) \ > for (const struct mmap_offset *__t = mmap_offset_types; \ > (__t)->name; \ > - (__t)++) > + (__t)++) \ > + for_each_if(gem_has_mmap_offset_type((fd), (__t))) > > #endif /* GEM_MMAN_H */ > > diff --git a/tests/i915/gem_ctx_sseu.c b/tests/i915/gem_ctx_sseu.c > index d558c8baa..3bef11b51 100644 > --- a/tests/i915/gem_ctx_sseu.c > +++ b/tests/i915/gem_ctx_sseu.c > @@ -531,7 +531,7 @@ igt_main > test_invalid_sseu(fd); > > igt_subtest_with_dynamic("mmap-args") { > - for_each_mmap_offset_type(t) { > + for_each_mmap_offset_type(fd, t) { > igt_dynamic_f("%s", t->name) > test_mmapped_args(fd, t); > } > diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c > index e2912685b..cf7ea3065 100644 > --- a/tests/i915/gem_exec_params.c > +++ b/tests/i915/gem_exec_params.c > @@ -244,7 +244,7 @@ static void mmapped(int i915) > buf = gem_create(i915, 4096); > handle = batch_create(i915); > > - for_each_mmap_offset_type(t) { /* repetitive! */ > + for_each_mmap_offset_type(i915, t) { /* repetitive! */ > struct drm_i915_gem_execbuffer2 *execbuf; > struct drm_i915_gem_exec_object2 *exec; > > diff --git a/tests/i915/gem_madvise.c b/tests/i915/gem_madvise.c > index e8716a891..54c9befff 100644 > --- a/tests/i915/gem_madvise.c > +++ b/tests/i915/gem_madvise.c > @@ -62,12 +62,13 @@ dontneed_before_mmap(void) > char *ptr; > int fd; > > - for_each_mmap_offset_type(t) { > + fd = drm_open_driver(DRIVER_INTEL); > + > + for_each_mmap_offset_type(fd, t) { > sighandler_t old_sigsegv, old_sigbus; > > igt_debug("Mapping mode: %s\n", t->name); > > - fd = drm_open_driver(DRIVER_INTEL); > handle = gem_create(fd, OBJECT_SIZE); > gem_madvise(fd, handle, I915_MADV_DONTNEED); > > @@ -93,7 +94,11 @@ dontneed_before_mmap(void) > munmap(ptr, OBJECT_SIZE); > signal(SIGBUS, old_sigsegv); > signal(SIGSEGV, old_sigbus); > + > + fd = drm_open_driver(DRIVER_INTEL); > } > + > + close(fd); > } > > static void > @@ -103,12 +108,13 @@ dontneed_after_mmap(void) > char *ptr; > int fd; > > - for_each_mmap_offset_type(t) { > + fd = drm_open_driver(DRIVER_INTEL); > + > + for_each_mmap_offset_type(fd, t) { > sighandler_t old_sigsegv, old_sigbus; > > igt_debug("Mapping mode: %s\n", t->name); > > - fd = drm_open_driver(DRIVER_INTEL); > handle = gem_create(fd, OBJECT_SIZE); > > ptr = __gem_mmap_offset(fd, handle, 0, OBJECT_SIZE, > @@ -134,7 +140,11 @@ dontneed_after_mmap(void) > munmap(ptr, OBJECT_SIZE); > signal(SIGBUS, old_sigbus); > signal(SIGSEGV, old_sigsegv); > + > + fd = drm_open_driver(DRIVER_INTEL); > } > + > + close(fd); > } > > static void > diff --git a/tests/i915/gem_mmap_offset.c b/tests/i915/gem_mmap_offset.c > index f49d18e63..1ec963b25 100644 > --- a/tests/i915/gem_mmap_offset.c > +++ b/tests/i915/gem_mmap_offset.c > @@ -128,7 +128,7 @@ static void basic_uaf(int i915) > { > const uint32_t obj_size = 4096; > > - for_each_mmap_offset_type(t) { > + for_each_mmap_offset_type(i915, t) { > uint32_t handle = gem_create(i915, obj_size); > uint8_t *expected, *buf, *addr; > > @@ -176,7 +176,7 @@ static void basic_uaf(int i915) > > static void isolation(int i915) > { > - for_each_mmap_offset_type(t) { > + for_each_mmap_offset_type(i915, t) { > struct drm_i915_gem_mmap_offset mmap_arg = { > .flags = t->type > }; > @@ -245,7 +245,7 @@ static void pf_nonblock(int i915) > { > igt_spin_t *spin = igt_spin_new(i915); > > - for_each_mmap_offset_type(t) { > + for_each_mmap_offset_type(i915, t) { > uint32_t *ptr; > > ptr = __mmap_offset(i915, spin->handle, 0, 4096, > @@ -324,7 +324,7 @@ static void open_flood(int i915, int timeout) > handle = gem_create(i915, 4096); > dmabuf = prime_handle_to_fd(i915, handle); > > - for_each_mmap_offset_type(t) { > + for_each_mmap_offset_type(i915, t) { > struct drm_i915_gem_mmap_offset arg = { > .handle = handle, > .flags = t->type, > @@ -351,7 +351,7 @@ static void open_flood(int i915, int timeout) > tmp = gem_reopen_driver(i915); > handle = prime_fd_to_handle(i915, dmabuf); > > - for_each_mmap_offset_type(t) { > + for_each_mmap_offset_type(i915, t) { > struct drm_i915_gem_mmap_offset arg = { > .handle = handle, > .flags = t->type, > diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c > index 0c2821122..1bec80db7 100644 > --- a/tests/i915/i915_pm_rpm.c > +++ b/tests/i915/i915_pm_rpm.c > @@ -2006,7 +2006,7 @@ igt_main_args("", long_options, help_str, opt_handler, NULL) > > /* GEM */ > igt_subtest_with_dynamic("gem-mmap-type") { > - for_each_mmap_offset_type(t) { > + for_each_mmap_offset_type(drm_fd, t) { > igt_dynamic_f("%s", t->name) > gem_mmap_args(t); > } >
diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c index 08ae67696..6fa8d1e8b 100644 --- a/lib/i915/gem_mman.c +++ b/lib/i915/gem_mman.c @@ -60,6 +60,11 @@ bool gem_has_mmap_offset(int fd) return gtt_version >= 4; } +bool gem_has_mmap_offset_type(int fd, const struct mmap_offset *t); +{ + return gem_has_mmap_offset(fd) || t->type == I915_MMAP_OFFSET_GTT; +} + /** * __gem_mmap__gtt: * @fd: open i915 drm file descriptor diff --git a/lib/i915/gem_mman.h b/lib/i915/gem_mman.h index 4fc6a0186..2c4a7a00b 100644 --- a/lib/i915/gem_mman.h +++ b/lib/i915/gem_mman.h @@ -101,10 +101,13 @@ extern const struct mmap_offset { unsigned int domain; } mmap_offset_types[]; -#define for_each_mmap_offset_type(__t) \ +bool gem_has_mmap_offset_type(int fd, const struct mmap_offset *t); + +#define for_each_mmap_offset_type(fd, __t) \ for (const struct mmap_offset *__t = mmap_offset_types; \ (__t)->name; \ - (__t)++) + (__t)++) \ + for_each_if(gem_has_mmap_offset_type((fd), (__t))) #endif /* GEM_MMAN_H */ diff --git a/tests/i915/gem_ctx_sseu.c b/tests/i915/gem_ctx_sseu.c index d558c8baa..3bef11b51 100644 --- a/tests/i915/gem_ctx_sseu.c +++ b/tests/i915/gem_ctx_sseu.c @@ -531,7 +531,7 @@ igt_main test_invalid_sseu(fd); igt_subtest_with_dynamic("mmap-args") { - for_each_mmap_offset_type(t) { + for_each_mmap_offset_type(fd, t) { igt_dynamic_f("%s", t->name) test_mmapped_args(fd, t); } diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c index e2912685b..cf7ea3065 100644 --- a/tests/i915/gem_exec_params.c +++ b/tests/i915/gem_exec_params.c @@ -244,7 +244,7 @@ static void mmapped(int i915) buf = gem_create(i915, 4096); handle = batch_create(i915); - for_each_mmap_offset_type(t) { /* repetitive! */ + for_each_mmap_offset_type(i915, t) { /* repetitive! */ struct drm_i915_gem_execbuffer2 *execbuf; struct drm_i915_gem_exec_object2 *exec; diff --git a/tests/i915/gem_madvise.c b/tests/i915/gem_madvise.c index e8716a891..54c9befff 100644 --- a/tests/i915/gem_madvise.c +++ b/tests/i915/gem_madvise.c @@ -62,12 +62,13 @@ dontneed_before_mmap(void) char *ptr; int fd; - for_each_mmap_offset_type(t) { + fd = drm_open_driver(DRIVER_INTEL); + + for_each_mmap_offset_type(fd, t) { sighandler_t old_sigsegv, old_sigbus; igt_debug("Mapping mode: %s\n", t->name); - fd = drm_open_driver(DRIVER_INTEL); handle = gem_create(fd, OBJECT_SIZE); gem_madvise(fd, handle, I915_MADV_DONTNEED); @@ -93,7 +94,11 @@ dontneed_before_mmap(void) munmap(ptr, OBJECT_SIZE); signal(SIGBUS, old_sigsegv); signal(SIGSEGV, old_sigbus); + + fd = drm_open_driver(DRIVER_INTEL); } + + close(fd); } static void @@ -103,12 +108,13 @@ dontneed_after_mmap(void) char *ptr; int fd; - for_each_mmap_offset_type(t) { + fd = drm_open_driver(DRIVER_INTEL); + + for_each_mmap_offset_type(fd, t) { sighandler_t old_sigsegv, old_sigbus; igt_debug("Mapping mode: %s\n", t->name); - fd = drm_open_driver(DRIVER_INTEL); handle = gem_create(fd, OBJECT_SIZE); ptr = __gem_mmap_offset(fd, handle, 0, OBJECT_SIZE, @@ -134,7 +140,11 @@ dontneed_after_mmap(void) munmap(ptr, OBJECT_SIZE); signal(SIGBUS, old_sigbus); signal(SIGSEGV, old_sigsegv); + + fd = drm_open_driver(DRIVER_INTEL); } + + close(fd); } static void diff --git a/tests/i915/gem_mmap_offset.c b/tests/i915/gem_mmap_offset.c index f49d18e63..1ec963b25 100644 --- a/tests/i915/gem_mmap_offset.c +++ b/tests/i915/gem_mmap_offset.c @@ -128,7 +128,7 @@ static void basic_uaf(int i915) { const uint32_t obj_size = 4096; - for_each_mmap_offset_type(t) { + for_each_mmap_offset_type(i915, t) { uint32_t handle = gem_create(i915, obj_size); uint8_t *expected, *buf, *addr; @@ -176,7 +176,7 @@ static void basic_uaf(int i915) static void isolation(int i915) { - for_each_mmap_offset_type(t) { + for_each_mmap_offset_type(i915, t) { struct drm_i915_gem_mmap_offset mmap_arg = { .flags = t->type }; @@ -245,7 +245,7 @@ static void pf_nonblock(int i915) { igt_spin_t *spin = igt_spin_new(i915); - for_each_mmap_offset_type(t) { + for_each_mmap_offset_type(i915, t) { uint32_t *ptr; ptr = __mmap_offset(i915, spin->handle, 0, 4096, @@ -324,7 +324,7 @@ static void open_flood(int i915, int timeout) handle = gem_create(i915, 4096); dmabuf = prime_handle_to_fd(i915, handle); - for_each_mmap_offset_type(t) { + for_each_mmap_offset_type(i915, t) { struct drm_i915_gem_mmap_offset arg = { .handle = handle, .flags = t->type, @@ -351,7 +351,7 @@ static void open_flood(int i915, int timeout) tmp = gem_reopen_driver(i915); handle = prime_fd_to_handle(i915, dmabuf); - for_each_mmap_offset_type(t) { + for_each_mmap_offset_type(i915, t) { struct drm_i915_gem_mmap_offset arg = { .handle = handle, .flags = t->type, diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c index 0c2821122..1bec80db7 100644 --- a/tests/i915/i915_pm_rpm.c +++ b/tests/i915/i915_pm_rpm.c @@ -2006,7 +2006,7 @@ igt_main_args("", long_options, help_str, opt_handler, NULL) /* GEM */ igt_subtest_with_dynamic("gem-mmap-type") { - for_each_mmap_offset_type(t) { + for_each_mmap_offset_type(drm_fd, t) { igt_dynamic_f("%s", t->name) gem_mmap_args(t); }
Commit b0da8bb705c0 ("lib/i915: for_each_mmap_offset_type()") introduced a macro that makes it easy to repeat a test body within a loop for each mmap-offset mapping type supported by v4 of i915 MMAP_GTT API. However, when run on an older version of the driver, those subtests are believed to be still repeated for each known mmap-offset mapping type while effectively exercising GTT mapping type only. As that may be confusing, fix it. It has been assumed that the modified macro is still suitable for use inside gem_mmap_offset test itself. Would that not be case, gem_mmap_offset could redefine the macro back to its initial form for internal use. v2: Move extra condition to a separate function and call it via for_each_if(), in case we need to fix it again in future (Chris) Suggested-by: Michał Winiarski <michal.winiarski@intel.com> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- lib/i915/gem_mman.c | 5 +++++ lib/i915/gem_mman.h | 7 +++++-- tests/i915/gem_ctx_sseu.c | 2 +- tests/i915/gem_exec_params.c | 2 +- tests/i915/gem_madvise.c | 18 ++++++++++++++---- tests/i915/gem_mmap_offset.c | 10 +++++----- tests/i915/i915_pm_rpm.c | 2 +- 7 files changed, 32 insertions(+), 14 deletions(-)