[igt-dev] [PATCH i-g-t] i915: Skip two subsets when pread/pwrite are unavailable

[Dixit, Ashutosh]

On Thu, 25 Mar 2021 23:58:53 -0700, Zbigniew Kempczyński wrote:
>
> On Thu, Mar 25, 2021 at 09:33:03PM -0700, Ashutosh Dixit wrote:
> > The pread/pwrite replacement implemented in
> > ad5eb02eb3f1 ("lib/ioctl_wrappers: Keep IGT working without pread/pwrite
> > ioctls") uses gem_set_domain which pins all pages which have to be
> > read/written. When the read/write size is large this causes gem_set_domain
> > to return -ENOMEM with a trace such as:
> >
> > ioctl_wrappers-CRITICAL: Test assertion failure function gem_set_domain, file ../lib/ioctl_wrappers.c:563:
> > ioctl_wrappers-CRITICAL: Failed assertion: __gem_set_domain(fd, handle, read, write) == 0
> > ioctl_wrappers-CRITICAL: Last errno: 12, Cannot allocate memory
> > ioctl_wrappers-CRITICAL: error: -12 != 0
> > igt_core-INFO: Stack trace:
> > igt_core-INFO:   #0 ../lib/igt_core.c:1746 __igt_fail_assert()
> > igt_core-INFO:   #1 [gem_set_domain+0x44]
> > igt_core-INFO:   #2 ../lib/ioctl_wrappers.c:367 gem_write()
> > igt_core-INFO:   #3 ../tests/prime_mmap.c:67 test_aperture_limit()
> > igt_core-INFO:   #4 ../tests/prime_mmap.c:578 __real_main530()
> > igt_core-INFO:   #5 ../tests/prime_mmap.c:530 main()
> >
> > Skip these tests when pread/pwrite are unavailable because they cannot pass
> > with the pread/pwrite replacement.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> >  tests/i915/gem_exec_params.c | 2 ++
> >  tests/prime_mmap.c           | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c
> > index 6840cf40ce..a7bcd39d32 100644
> > --- a/tests/i915/gem_exec_params.c
> > +++ b/tests/i915/gem_exec_params.c
> > @@ -314,6 +314,8 @@ static void test_larger_than_life_batch(int fd)
> >	 */
> >	igt_require(size < gem_aperture_size(fd));
> >	intel_require_memory(2, size, CHECK_RAM); /* batch + shadow */
> > +	/* Prevent gem_set_domain -ENOMEM failures */
> > +	gem_require_pread_pwrite(fd);
>
> I have doubts regarding that test.
>
> What is real cause we got -ENOMEM in that subtest?

Hi Zbyszek, I explained this in the commit message:

gem_set_domain pins all pages which have to be read/written. When the
read/write size is large this causes gem_set_domain to return -ENOMEM.

> And on which machines?

Not sure about which machines but I saw some CI failures.

> I've enforced on my SKL (gen9) to use mmap/set_domain and I see success
> on that test.

OK good to know that it only fails on some machines, not always.

>
> Why require:
>
>	igt_require(size < gem_aperture_size(fd));
>
> comes after:
>
>	uint64_t size = 1ULL << 32; /* batch_len is __u32 as per the ABI */
>	struct drm_i915_gem_exec_object2 exec = {
>		.handle = batch_create_size(fd, size),
>	};
>
> I think require before mmap/set_domain would catch this issue.

Thanks, you are right, there is a bug in the patch,
gem_require_pread_pwrite() should be before batch_create_size() which
contains the gem_write(). Actually all these igt_require's should be before
batch_create_size() like you say. May not even need the
gem_require_pread_pwrite().

The failure was same for the prime_mmap below.

I will try and repost the patch. Thanks.

>
> --
> Zbigniew
>
> >
> >	__for_each_physical_engine(fd, e) {
> >		/* Keep the batch_len implicit [0] */
> > diff --git a/tests/prime_mmap.c b/tests/prime_mmap.c
> > index cdf2d51497..af313917da 100644
> > --- a/tests/prime_mmap.c
> > +++ b/tests/prime_mmap.c
> > @@ -451,6 +451,8 @@ test_aperture_limit(void)
> >	uint64_t size1 = (gem_mappable_aperture_size(fd) * 7) / 8;
> >	uint64_t size2 = (gem_mappable_aperture_size(fd) * 3) / 8;
> >
> > +	/* Prevent gem_set_domain -ENOMEM failures */
> > +	gem_require_pread_pwrite(fd);
> >	handle1 = gem_create(fd, size1);
> >	fill_bo(handle1, BO_SIZE);
> >
> > --
> > 2.29.2
> >