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

[Tvrtko Ursulin]

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).

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