From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 556316EEB6 for ; Fri, 26 Mar 2021 07:23:55 +0000 (UTC) Date: Fri, 26 Mar 2021 00:23:54 -0700 Message-ID: <87ft0i2x9x.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20210326065853.GA3927@zkempczy-mobl2> References: <20210326043303.67982-1-ashutosh.dixit@intel.com> <20210326065853.GA3927@zkempczy-mobl2> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [igt-dev] [PATCH i-g-t] i915: Skip two subsets when pread/pwrite are unavailable List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Zbigniew =?ISO-8859-2?Q?Kempczy=F1ski?= Cc: igt-dev@lists.freedesktop.org, Petri Latvala List-ID: On Thu, 25 Mar 2021 23:58:53 -0700, Zbigniew Kempczy=F1ski 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_dom= ain > > 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) =3D=3D 0 > > ioctl_wrappers-CRITICAL: Last errno: 12, Cannot allocate memory > > ioctl_wrappers-CRITICAL: error: -12 !=3D 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 > > --- > > 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 =3D 1ULL << 32; /* batch_len is __u32 as per the ABI */ > struct drm_i915_gem_exec_object2 exec =3D { > .handle =3D 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 =3D (gem_mappable_aperture_size(fd) * 7) / 8; > > uint64_t size2 =3D (gem_mappable_aperture_size(fd) * 3) / 8; > > > > + /* Prevent gem_set_domain -ENOMEM failures */ > > + gem_require_pread_pwrite(fd); > > handle1 =3D gem_create(fd, size1); > > fill_bo(handle1, BO_SIZE); > > > > -- > > 2.29.2 > > _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev