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

[Matthew Auld]

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.

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