On 22/01/20 08:26, Janusz Krzysztofik wrote:
> Working with a userptr GEM object backed by any type of mapping to
> another GEM object, not only GTT mapping currently examined bu the
> test, may cause a currently unavoidable lockdep splat inside the i915
> driver. Then, such operations are expected to fail in advance to
> prevent from that badness to happen.
>
> Extend the scope of the test by adding subtests which exercise other,
> non-GTT mapping types. Moreover, also succeed if a failure happens
> as soon as a userptr object is created on top of an invalid mapping.
>
> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> ---
> tests/i915/gem_userptr_blits.c | 87 +++++++++++++++++++++++-----------
> 1 file changed, 59 insertions(+), 28 deletions(-)
>
> diff --git a/tests/i915/gem_userptr_blits.c b/tests/i915/gem_userptr_blits.c
> index a8d3783f..69e5bd1f 100644
> --- a/tests/i915/gem_userptr_blits.c
> +++ b/tests/i915/gem_userptr_blits.c
> @@ -60,6 +60,7 @@
>
> #include "drm.h"
> #include "i915_drm.h"
> +#include "i915/gem_mman.h"
>
> #include "intel_bufmgr.h"
>
> @@ -577,11 +578,11 @@ static int test_invalid_null_pointer(int fd)
> return 0;
> }
>
> -static int test_invalid_gtt_mapping(int fd)
> +static int test_invalid_mapping(int fd, uint64_t flags)
> {
> - struct drm_i915_gem_mmap_gtt arg;
> + struct drm_i915_gem_mmap_offset arg;
> uint32_t handle;
> - char *gtt, *map;
> + char *offset, *map;
>
> /* Anonymous mapping to find a hole */
> map = mmap(NULL, sizeof(linear) + 2 * PAGE_SIZE,
> @@ -602,39 +603,46 @@ static int test_invalid_gtt_mapping(int fd)
> igt_assert_eq(copy(fd, handle, handle), 0);
> gem_close(fd, handle);
>
> - /* GTT mapping */
> + /* object mapping */
> memset(&arg, 0, sizeof(arg));
> arg.handle = create_bo(fd, 0);
> - do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &arg);
> - gtt = mmap(map + PAGE_SIZE, sizeof(linear),
> - PROT_READ | PROT_WRITE,
> - MAP_SHARED | MAP_FIXED,
> - fd, arg.offset);
> - igt_assert(gtt == map + PAGE_SIZE);
> + arg.flags = flags;
> + do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_OFFSET, &arg);
> + offset = mmap(map + PAGE_SIZE, sizeof(linear), PROT_READ | PROT_WRITE,
> + MAP_SHARED | MAP_FIXED, fd, arg.offset);
> + igt_assert(offset == map + PAGE_SIZE);
> gem_close(fd, arg.handle);
> - igt_assert(((unsigned long)gtt & (PAGE_SIZE - 1)) == 0);
> + igt_assert(((unsigned long)offset & (PAGE_SIZE - 1)) == 0);
> igt_assert((sizeof(linear) & (PAGE_SIZE - 1)) == 0);
>
> - gem_userptr(fd, gtt, sizeof(linear), 0, userptr_flags, &handle);
> - igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> - gem_close(fd, handle);
> + if (!__gem_userptr(fd, offset, sizeof(linear), 0, userptr_flags,
> + &handle)) {
Not sure I follow why you converted this to an if. Do we expect the
userptr IOCTL not to work? Could you add a small comment.
Antonio
> + igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> + gem_close(fd, handle);
> + }
>
> - gem_userptr(fd, gtt, PAGE_SIZE, 0, userptr_flags, &handle);
> - igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> - gem_close(fd, handle);
> + if (!__gem_userptr(fd, offset, PAGE_SIZE, 0, userptr_flags, &handle)) {
> + igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> + gem_close(fd, handle);
> + }
>
> - gem_userptr(fd, gtt + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, 0, userptr_flags, &handle);
> - igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> - gem_close(fd, handle);
> + if (!__gem_userptr(fd, offset + sizeof(linear) - PAGE_SIZE, PAGE_SIZE,
> + 0, userptr_flags, &handle)) {
> + igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> + gem_close(fd, handle);
> + }
>
> /* boundaries */
> - gem_userptr(fd, map, 2*PAGE_SIZE, 0, userptr_flags, &handle);
> - igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> - gem_close(fd, handle);
> + if (!__gem_userptr(fd, map, 2 * PAGE_SIZE, 0, userptr_flags, &handle)) {
> + igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> + gem_close(fd, handle);
> + }
>
> - gem_userptr(fd, map + sizeof(linear), 2*PAGE_SIZE, 0, userptr_flags, &handle);
> - igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> - gem_close(fd, handle);
> + if (!__gem_userptr(fd, map + sizeof(linear), 2 * PAGE_SIZE, 0,
> + userptr_flags, &handle)) {
> + igt_assert_eq(copy(fd, handle, handle), -EFAULT);
> + gem_close(fd, handle);
> + }
>
> munmap(map, sizeof(linear) + 2*PAGE_SIZE);
>
> @@ -2009,8 +2017,31 @@ igt_main_args("c:", NULL, help_str, opt_handler, NULL)
> igt_subtest("invalid-null-pointer")
> test_invalid_null_pointer(fd);
>
> - igt_subtest("invalid-gtt-mapping")
> - test_invalid_gtt_mapping(fd);
> + igt_describe("Verify userptr on top of GTT mapping to GEM object will fail");
> + igt_subtest("invalid-gtt-mapping") {
> + gem_require_mappable_ggtt(fd);
> + test_invalid_mapping(fd, I915_MMAP_OFFSET_GTT);
> + }
> + igt_subtest_group {
> + igt_fixture
> + igt_require(gem_has_mmap_offset(fd));
> +
> + igt_describe("Verify userptr on top of CPU mapping to GEM object will fail");
> + igt_subtest("invalid-wb-mapping")
> + test_invalid_mapping(fd, I915_MMAP_OFFSET_WB);
> +
> + igt_subtest_group {
> + igt_fixture
> + igt_require(gem_mmap_offset__has_wc(fd));
> +
> + igt_describe("Verify userptr on top of coherent mapping to GEM object will fail");
> + igt_subtest("invalid-wc-mapping")
> + test_invalid_mapping(fd, I915_MMAP_OFFSET_WC);
> + igt_describe("Verify userptr on top of uncached mapping to GEM object will fail");
> + igt_subtest("invalid-uc-mapping")
> + test_invalid_mapping(fd, I915_MMAP_OFFSET_UC);
> + }
> + }
>
> igt_subtest("forked-access")
> test_forked_access(fd);
>