[igt-dev] [PATCH i-g-t] i915: Avoid set_domain -ENOMEM error with huge buffers

[Dixit, Ashutosh]

On Wed, 31 Mar 2021 02:02:45 -0700, Matthew Auld wrote:
> On Tue, 30 Mar 2021 at 20:31, Dixit, Ashutosh <ashutosh.dixit at intel.com> wrote:
> >
> > On Tue, 30 Mar 2021 03:28:01 -0700, Matthew Auld wrote:
> > >
> > > On Tue, 30 Mar 2021 at 04:51, Ashutosh Dixit <ashutosh.dixit at intel.com> 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 at test_aperture_limit and gem_exec_params at larger-than-life-batch
> > > > when pread/pwrite are unavailable.
> > > >
> > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > > > ---
> > > >  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.
>
> mmap_offset still calls pin_pages()/get_pages() somewhere in the fault
> handler, which is for the entire object. In i915 all we currently have
> is pin-all-the-pages when we need to touch the pages, but if it's a
> shmem object then it's possible to use the page-cache underneath to
> populate individual pages in the shmem file, like in the shmem_pwrite
> backend, and gem_mmap__cpu/wc which uses shmem_fault IIRC.

Thanks, I was not aware of this difference between mmap and mmap_offset but
glancing through the code this indeed seems to be the case. I have changed
to gem_mmap__wc in v3.

> >
> > > 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?