From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 80FD36E95B for ; Tue, 30 Mar 2021 19:31:47 +0000 (UTC) Date: Tue, 30 Mar 2021 12:31:45 -0700 Message-ID: <87eefwo2u6.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: References: <20210330035056.52019-1-ashutosh.dixit@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [igt-dev] [PATCH i-g-t] i915: Avoid set_domain -ENOMEM error with huge buffers List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Matthew Auld Cc: igt-dev@lists.freedesktop.org List-ID: On Tue, 30 Mar 2021 03:28:01 -0700, Matthew Auld wrote: > > On Tue, 30 Mar 2021 at 04:51, Ashutosh Dixit wrote: > > > > When pread/pwrite are unavailable, 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() > > > > Therefore avoid using the pread/pwrite replacement for huge buffers, mmap > > and write instead. This fixes failures seen in > > prime_mmap@test_aperture_limit and gem_exec_params@larger-than-life-batch > > when pread/pwrite are unavailable. > > > > Signed-off-by: Ashutosh Dixit > > --- > > tests/i915/gem_exec_params.c | 5 ++++- > > tests/prime_mmap.c | 33 ++++++++++++++++++++++----------- > > 2 files changed, 26 insertions(+), 12 deletions(-) > > > > diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c > > index 6840cf40ce..613bc26485 100644 > > --- a/tests/i915/gem_exec_params.c > > +++ b/tests/i915/gem_exec_params.c > > @@ -254,9 +254,12 @@ static uint32_t batch_create_size(int fd, uint64_t size) > > { > > const uint32_t bbe = MI_BATCH_BUFFER_END; > > uint32_t handle; > > + char *ptr; > > > > handle = gem_create(fd, size); > > - gem_write(fd, handle, 0, &bbe, sizeof(bbe)); > > + ptr = gem_mmap__device_coherent(fd, handle, 0, sizeof(bbe), PROT_WRITE); > > + memcpy(ptr, &bbe, sizeof(bbe)); > > + munmap(ptr, sizeof(bbe)); > > I thought mmap_offfset still just pins all the pages on fault, so why > don't we still hit -ENOMEM somewhere? Sorry I think this statement in the commit message is what has caused the confusion, it's just badly written: "gem_set_domain which pins all pages which have to be read/written". set_domain doesn't just pin all pages which have to read/written but actually pins the entire object. Does this explain the reason now? I would assume mmap_offset would only fault in the required pages. > I would have assumed we want gem_mmap__cpu/wc here, My intention is to gem_mmap__device_coherent as a shorthand for gem_mmap__wc (or gem_mmap_offset__wc). > which instead goes through the shmem/page-cache backend, and so only > needs to allocate the first few pages or so IIRC, similar to the tricks > in the shmem pwrite backend? Or I guess just move the igt_require() for > the memory requirements to earlier? Even if we did that I think we might still need to fix the issue with the set_domain pinning the entire object so that's what I'm trying to avoid here with this patch. Thanks. > Or maybe I am misunderstanding something? _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev