[igt-dev] [PATCH i-g-t] gem_exec_reloc & gem_shrink: Added gem_require_mappable_ggtt to check mappable aperture

[Chris Wilson]

Quoting Melkaveri, Arjun (2020-12-02 11:05:23)
> On Mon, Nov 30, 2020 at 12:09:10PM +0000, Tvrtko Ursulin wrote:
> > 
> > On 30/11/2020 10:12, Chris Wilson wrote:
> > > Quoting Tvrtko Ursulin (2020-11-30 10:04:17)
> > > > On 30/11/2020 02:03, Arjun Melkaveri wrote:
> > > > > Added gem_require_mappable_ggtt to check mappable aperture.
> > > > > This is to avoid any test crash that might happen
> > > > > if mappable aperture is not avilable.
> > > > > 
> > > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > > > Signed-off-by: Arjun Melkaveri <arjun.melkaveri at intel.com>
> > > > > ---
> > > > >    tests/i915/gem_exec_reloc.c | 3 +++
> > > > >    tests/i915/gem_shrink.c     | 2 ++
> > > > >    2 files changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> > > > > index 8dcb24a6..596b1157 100644
> > > > > --- a/tests/i915/gem_exec_reloc.c
> > > > > +++ b/tests/i915/gem_exec_reloc.c
> > > > > @@ -1217,6 +1217,7 @@ igt_main
> > > > >                        if (!(f->flags & NORELOC)) {
> > > > >                                igt_subtest_f("%srange%s",
> > > > >                                              f->basic ? "basic-" : "", f->name) {
> > > > > +                                     gem_require_mappable_ggtt(fd);
> > > > 
> > > >   From the code and commit message it is not immediately obvious to me
> > > > why basic_range needs the aperture, Chris? Answer to that will drive the
> > > > solution.
> > > 
> > > It does not.
> > 
> > Okay, so the plan for this one should be to stop probing aperture size and
> > instead probe the size of the address space in use. This will be ggtt on old
> > gens and ppgtt on new ones. And to keep object count in check with regards
> > to available backing store, if required (not sure right now).
> Let me know if this is okay 
> #define MAX_47b ((1ull << 47)) // address range's most signifcant bit. 
> const uint64_t max = (AT_LEAST_GEN(devid, 8) ? MAX_47b : gem_aperture_size(i915) / 2); 
> instead of checking gem_require_mappable_ggtt in subtest .

Where are you getting the impression that basic_range() uses the
mappable aperture?

> > > > > diff --git a/tests/i915/gem_shrink.c b/tests/i915/gem_shrink.c
> > > > > index dba62c8f..094975af 100644
> > > > > --- a/tests/i915/gem_shrink.c
> > > > > +++ b/tests/i915/gem_shrink.c
> > > > > @@ -432,6 +432,8 @@ igt_main
> > > > >                fd = drm_open_driver(DRIVER_INTEL);
> > > > >                igt_require_gem(fd);
> > > > > +             gem_require_mappable_ggtt(fd);
> > > > > +
> > > > >                /*
> > > > >                 * Spawn enough processes to use all memory, but each only
> > > > >                 * uses half the available mappable aperture ~128MiB.
> > > > > 
> > > > 
> > > > And for this one the same I think. Apart from the subtests using
> > > > mmap-gtt other ones could probably be made work by calculating the
> > > > working set in a different way. Possibly just use the ggtt size and skip
> > > > tests which need aperture if no aperture. Chris would that be acceptable
> > > > or there is a special reason to have N clients with each using an
> > > > aperture sized amount of memory, to total RAM size?
> > > 
> > > shrink is system memory limits.
> > 
> > Yes, I was ignoring that aspect for now and focusing on the aperture size
> > misuse. But yes, making gem_shrink falsely pass on dg1 is not what we really
> > want. Probably make it explicitly use system memory objects once that API is
> > available.
> > 
> > Regards,
> > 
> > Tvrtko
> Any suggestion for this ?  can we use gem_require_mappable_ggtt as all
> tests use size from gem_mappable_aperture_size. or can i modify code 
> something like this 
> #define MAX_32b ((1ull << 32))

More conservative; use the 128MiB that I mention in the comments.
The goal is to have many processes pushing the system to the edge so
that there is plenty of contention on the shrinker. Being specifically a
shrinker test, they should remain smem objects.
-Chris