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

[Melkaveri, Arjun]

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 .
> 
> > > 
> > > >                                        igt_while_interruptible(f->flags & INTERRUPTIBLE)
> > > >                                                basic_range(fd, f->flags);
> > > >                                }
> > > > @@ -1264,6 +1265,7 @@ igt_main
> > > >        }
> > > >        igt_subtest_with_dynamic("basic-spin") {
> > > > +
> > > 
> > > !
> > > 
> > > >                __for_each_physical_engine(fd, e) {
> > > >                        igt_dynamic_f("%s", e->name)
> > > >                                active_spin(fd, e->flags);
> > > > @@ -1278,6 +1280,7 @@ igt_main
> > > >        }
> > > >        igt_subtest_with_dynamic("basic-many-active") {
> > > > +             gem_require_mappable_ggtt(fd);
> > > >                __for_each_physical_engine(fd, e) {
> > > >                        igt_dynamic_f("%s", e->name)
> > > >                                many_active(fd, e->flags);
> > > 
> > > Same for many_active - maybe tests should use ggtt size and not
> > > aperture, with some tweaks to make size/runtime sane?
> > 
> > It does not. This is somebody hacking over a major kernel bug.
> 
> Okay so same as above, don't probe aperture but address space size.
> 
> > 
> > > > 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))

alloc_size = AT_LEAST_GEN(intel_get_drm_devid(fd), 8) ? MAX_32b : gem_mappable_aperture_size();
alloc_size = alloc_size /2
Test would probabbly skip if estimated size is more than  RAM + swap)

-Arjun