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

[Dixit, Ashutosh]

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.

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