diff mbox series

[RFC,i-g-t,1/1] tests/gem_mmap_offset: Exercise mapping to userptr

Message ID 20200131131234.23058-2-janusz.krzysztofik@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series tests/gem_mmap_offset: Exercise mapping to userptr | expand

Commit Message

Janusz Krzysztofik Jan. 31, 2020, 1:12 p.m. UTC
Creating a mapping to a userptr backed GEM object may cause a currently
unavoidable lockdep splat inside the i915 driver.  Then, such operation
is expected to fail to prevent from that badness to happen.

Add a respective subtest for each mapping type.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_mmap_offset.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Chris Wilson Jan. 31, 2020, 2:37 p.m. UTC | #1
Quoting Janusz Krzysztofik (2020-01-31 13:12:34)
> Creating a mapping to a userptr backed GEM object may cause a currently
> unavoidable lockdep splat inside the i915 driver.  Then, such operation
> is expected to fail to prevent from that badness to happen.
> 
> Add a respective subtest for each mapping type.
> 
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/i915/gem_mmap_offset.c | 55 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/tests/i915/gem_mmap_offset.c b/tests/i915/gem_mmap_offset.c
> index 7c4088cdf..a5f28328b 100644
> --- a/tests/i915/gem_mmap_offset.c
> +++ b/tests/i915/gem_mmap_offset.c
> @@ -141,6 +141,36 @@ static void bad_extensions(int i915)
>         gem_close(i915, arg.handle);
>  }
>  
> +static bool has_userptr(int i915)
> +{
> +       uint32_t handle = 0;
> +       void *ptr;
> +
> +       igt_assert_eq(posix_memalign(&ptr, 4096, 4096), 0);
> +       if (__gem_userptr(i915, ptr, 4096, 0, 0, &handle) == 0)
> +               gem_close(i915, handle);
> +       free(ptr);
> +
> +       return handle;
> +}
> +
> +static void userptr(int i915, uint64_t flags)
> +{
> +       struct drm_i915_gem_mmap_offset arg = {
> +               .flags = flags,
> +       };
> +       void *ptr;
> +
> +       igt_assert_eq(posix_memalign(&ptr, 4096, 4096), 0);
> +
> +       gem_userptr(i915, ptr, 4096, 0, 0, &arg.handle);
> +
> +       igt_assert_eq(mmap_offset_ioctl(i915, &arg), -EINVAL);

Not quite. The only reason it doesn't work is because the implementation
ties itself into knots, not that it is meant to not work. :|
-Chris
Janusz Krzysztofik Jan. 31, 2020, 2:56 p.m. UTC | #2
On Friday, January 31, 2020 3:37:05 PM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2020-01-31 13:12:34)
> > Creating a mapping to a userptr backed GEM object may cause a currently
> > unavoidable lockdep splat inside the i915 driver.  Then, such operation
> > is expected to fail to prevent from that badness to happen.
> > 
> > Add a respective subtest for each mapping type.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_mmap_offset.c | 55 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> > 
> > diff --git a/tests/i915/gem_mmap_offset.c b/tests/i915/gem_mmap_offset.c
> > index 7c4088cdf..a5f28328b 100644
> > --- a/tests/i915/gem_mmap_offset.c
> > +++ b/tests/i915/gem_mmap_offset.c
> > @@ -141,6 +141,36 @@ static void bad_extensions(int i915)
> >         gem_close(i915, arg.handle);
> >  }
> >  
> > +static bool has_userptr(int i915)
> > +{
> > +       uint32_t handle = 0;
> > +       void *ptr;
> > +
> > +       igt_assert_eq(posix_memalign(&ptr, 4096, 4096), 0);
> > +       if (__gem_userptr(i915, ptr, 4096, 0, 0, &handle) == 0)
> > +               gem_close(i915, handle);
> > +       free(ptr);
> > +
> > +       return handle;
> > +}
> > +
> > +static void userptr(int i915, uint64_t flags)
> > +{
> > +       struct drm_i915_gem_mmap_offset arg = {
> > +               .flags = flags,
> > +       };
> > +       void *ptr;
> > +
> > +       igt_assert_eq(posix_memalign(&ptr, 4096, 4096), 0);
> > +
> > +       gem_userptr(i915, ptr, 4096, 0, 0, &arg.handle);
> > +
> > +       igt_assert_eq(mmap_offset_ioctl(i915, &arg), -EINVAL);
> 
> Not quite. The only reason it doesn't work is because the implementation
> ties itself into knots, not that it is meant to not work. :|

Are you suggesting the test should fail if the IOCTL fails?  That would be 
fair, but how are we going to have that accepted by CI then, and merged?

Skipping also doesn't seem a good option to me.

I can add some more exhaustive examination should the IOCTL succeed, but that 
won't help to make CI happy.

From your long experience, what approach should we take?

Thanks,
Janusz


> -Chris
>
diff mbox series

Patch

diff --git a/tests/i915/gem_mmap_offset.c b/tests/i915/gem_mmap_offset.c
index 7c4088cdf..a5f28328b 100644
--- a/tests/i915/gem_mmap_offset.c
+++ b/tests/i915/gem_mmap_offset.c
@@ -141,6 +141,36 @@  static void bad_extensions(int i915)
 	gem_close(i915, arg.handle);
 }
 
+static bool has_userptr(int i915)
+{
+	uint32_t handle = 0;
+	void *ptr;
+
+	igt_assert_eq(posix_memalign(&ptr, 4096, 4096), 0);
+	if (__gem_userptr(i915, ptr, 4096, 0, 0, &handle) == 0)
+		gem_close(i915, handle);
+	free(ptr);
+
+	return handle;
+}
+
+static void userptr(int i915, uint64_t flags)
+{
+	struct drm_i915_gem_mmap_offset arg = {
+		.flags = flags,
+	};
+	void *ptr;
+
+	igt_assert_eq(posix_memalign(&ptr, 4096, 4096), 0);
+
+	gem_userptr(i915, ptr, 4096, 0, 0, &arg.handle);
+
+	igt_assert_eq(mmap_offset_ioctl(i915, &arg), -EINVAL);
+
+	gem_close(i915, arg.handle);
+	free(ptr);
+}
+
 static void basic_uaf(int i915)
 {
 	const uint32_t obj_size = 4096;
@@ -461,6 +491,31 @@  igt_main
 	igt_subtest_f("bad-extensions")
 		bad_extensions(i915);
 
+	igt_subtest_group {
+		igt_fixture
+			igt_require(has_userptr(i915));
+
+		for_each_mmap_offset_type(t) {
+			igt_describe_f("Verify %s mapping to userptr backed GEM object will fail",
+				       t->name);
+			igt_subtest_f("userptr-%s-mapping", t->name) {
+				switch (t->type) {
+				case I915_MMAP_OFFSET_GTT:
+					gem_require_mappable_ggtt(i915);
+					break;
+				case I915_MMAP_OFFSET_WC:
+				case I915_MMAP_OFFSET_UC:
+					igt_require(gem_mmap_offset__has_wc(i915));
+					break;
+				defalut:
+					break;
+				}
+
+				userptr(i915, t->type);
+			}
+		}
+	}
+
 	igt_describe("Check buffer object mapping persists after gem_close");
 	igt_subtest_f("basic-uaf")
 		basic_uaf(i915);