[Intel-gfx] [RFC PATCH i-g-t] tests/gem_userptr_blits: Enhance invalid mapping exercise

[Janusz Krzysztofik]

Hi Antonio,

On Tuesday, January 28, 2020 1:59:00 AM CET Antonio Argenziano wrote:
> 
> 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.

Please have a look at Chris' response to CI report [1] for more information on 
what is expected.

Thanks,
Janusz

[1] https://lists.freedesktop.org/archives/igt-dev/2020-January/019515.html

> 
> 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);
> > 
>