Re: [igt-dev] [RFT v4 4/6] igt/i915: Require GTT mapping to be available when needed.

[Antonio Argenziano <antonio.argenziano@intel.com>]

On 25/03/19 16:36, Chris Wilson wrote:
> Quoting Antonio Argenziano (2019-03-25 23:20:41)
>> diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
>> index 47f6b92b..2e4c0335 100644
>> --- a/lib/igt_dummyload.c
>> +++ b/lib/igt_dummyload.c
>> @@ -117,9 +117,11 @@ emit_recursive_batch(igt_spin_t *spin,
>>          obj[BATCH].handle = gem_create(fd, BATCH_SIZE);
>>          batch = __gem_mmap__wc(fd, obj[BATCH].handle,
>>                                 0, BATCH_SIZE, PROT_WRITE);
>> -       if (!batch)
>> +       if (!batch) {
>> +               gem_require_mmap_gtt(fd);
>>                  batch = gem_mmap__gtt(fd, obj[BATCH].handle,
>>                                          BATCH_SIZE, PROT_WRITE);
> 
> batch = __gem_mmap__gtt();
> igt_require(batch) ?
> 
> Possibly not as informative as gem_require_mmap_gtt() So ~o~
> 
>> diff --git a/tests/i915/gem_concurrent_all.c b/tests/i915/gem_concurrent_all.c
>> index f719b0a1..e4fc1426 100644
>> --- a/tests/i915/gem_concurrent_all.c
>> +++ b/tests/i915/gem_concurrent_all.c
>> @@ -1422,6 +1422,7 @@ static void cpu_require(void)
>>   
>>   static void gtt_require(void)
>>   {
>> +       gem_require_mmap_gtt(fd);
>>   }
> 
> Needs the combinatorial explosion to exercise accessing via mmap-offset.
> If you only update one stress test, update this one.

Don't we get the mmap-offset coverage by default in the wc test cases or 
do you mean to use the mmap_offset IOCTL to mmap GTT?

> 
>>   static void bcs_require(void)
>> diff --git a/tests/i915/gem_ctx_sseu.c b/tests/i915/gem_ctx_sseu.c
>> index 3afa5c15..bf1b50d5 100644
>> --- a/tests/i915/gem_ctx_sseu.c
>> +++ b/tests/i915/gem_ctx_sseu.c
>> @@ -528,8 +528,10 @@ igt_main
>>                  igt_subtest("invalid-sseu")
>>                          test_invalid_sseu(fd);
>>   
>> -               igt_subtest("ggtt-args")
>> +               igt_subtest("ggtt-args") {
>> +                       gem_require_mmap_gtt(fd);
>>                          test_ggtt_args(fd);
>> +               }
>>   
>>                  igt_subtest("engines")
>>                          test_engines(fd);
> 
>> diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
>> index 837f60a6..bb4eec31 100644
>> --- a/tests/i915/gem_exec_reloc.c
>> +++ b/tests/i915/gem_exec_reloc.c
>> @@ -115,6 +115,9 @@ static void from_mmap(int fd, uint64_t size, enum mode mode)
>>           */
>>          intel_require_memory(2, size, CHECK_RAM);
>>   
>> +       if ((mode & ~RO) == GTT)
>> +               gem_require_mmap_gtt(fd);
>> +
> 
> We need to stick arguments into mmap-offset buffers. Same is true for
> several of the "do we survive userspace being nasty with __user pointers"
> tests.
> 
>> diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
>> index a9383000..15c8440f 100644
>> --- a/tests/i915/gem_exec_schedule.c
>> +++ b/tests/i915/gem_exec_schedule.c
>> @@ -1236,6 +1236,7 @@ igt_main
>>                          igt_subtest_f("independent-%s", e->name) {
>>                                  igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>>                                  igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
>> +                               gem_require_mmap_gtt(fd);
>>                                  independent(fd, e->exec_id | e->flags);
>>                          }
>>                  }
>> @@ -1328,8 +1329,10 @@ igt_main
>>                                  igt_subtest_f("wide-%s", e->name)
>>                                          wide(fd, e->exec_id | e->flags);
>>   
>> -                               igt_subtest_f("reorder-wide-%s", e->name)
>> +                               igt_subtest_f("reorder-wide-%s", e->name) {
>> +                                       gem_require_mmap_gtt(fd);
>>                                          reorder_wide(fd, e->exec_id | e->flags);
>> +                               }
>>   
>>                                  igt_subtest_f("smoketest-%s", e->name)
>>                                          smoketest(fd, e->exec_id | e->flags, 5);
> 
> Hmm. I would rather the basic scheduling tests remained functioning.

Didn't we have a convincing argument against using wc + sync?

Antonio

> 
>> diff --git a/tests/i915/gem_largeobject.c b/tests/i915/gem_largeobject.c
>> index 518396fa..a2d47edc 100644
>> --- a/tests/i915/gem_largeobject.c
>> +++ b/tests/i915/gem_largeobject.c
>> @@ -84,6 +84,8 @@ igt_simple_main
>>   
>>          fd = drm_open_driver(DRIVER_INTEL);
>>   
>> +       gem_require_mmap_gtt(fd);
> 
> Ok, not mmap-offset specific in future, but one of those fun challenges.
> 
>> diff --git a/tests/i915/gem_madvise.c b/tests/i915/gem_madvise.c
>> index 729a4d33..f4226a84 100644
>> --- a/tests/i915/gem_madvise.c
>> +++ b/tests/i915/gem_madvise.c
>> @@ -61,6 +61,8 @@ dontneed_before_mmap(void)
>>          uint32_t handle;
>>          char *ptr;
>>   
>> +       gem_require_mmap_gtt(fd);
> 
> Needs mmap-offset integration.
> 
>> diff --git a/tests/i915/gem_mmap_offset_exhaustion.c b/tests/i915/gem_mmap_offset_exhaustion.c
>> index 8c8e3fa2..86464231 100644
>> --- a/tests/i915/gem_mmap_offset_exhaustion.c
>> +++ b/tests/i915/gem_mmap_offset_exhaustion.c
>> @@ -82,6 +82,8 @@ igt_simple_main
>>   
>>          fd = drm_open_driver(DRIVER_INTEL);
>>   
>> +       gem_require_mmap_gtt(fd);
> 
> Some might say this became a more important test. Not sure if the test
> itself is the best strategy (vs the selftest approach), but it
> definitely shouldn't just be skipped for mmap-offset.
> 
>> diff --git a/tests/i915/gem_render_copy.c b/tests/i915/gem_render_copy.c
>> index 8d62a0f4..5757c547 100644
>> --- a/tests/i915/gem_render_copy.c
>> +++ b/tests/i915/gem_render_copy.c
>> @@ -572,6 +572,8 @@ static void test(data_t *data, uint32_t tiling, uint64_t ccs_modifier)
>>          int opt_dump_aub = igt_aub_dump_enabled();
>>          int num_src = ARRAY_SIZE(src);
>>   
>> +       gem_require_mmap_gtt(data->drm_fd);
> 
> We should make sure gem_render_copy remains available for debugging the
> rendercopy routines, I doubt they are going away.
> 
>> diff --git a/tests/i915/gem_userptr_blits.c b/tests/i915/gem_userptr_blits.c
>> index 8f8ddf43..9962e539 100644
>> --- a/tests/i915/gem_userptr_blits.c
>> +++ b/tests/i915/gem_userptr_blits.c
>> @@ -554,6 +554,8 @@ static int test_invalid_gtt_mapping(int fd)
>>          uint32_t handle;
>>          char *gtt, *map;
>>   
>> +       gem_require_mmap_gtt(fd);
> 
> userptr-blits needs mmap-offset enhancements. Principally, if we can
> mmap(MAP_FIXED) over top of an active userptr, we need to resolve that
> amicably.
> -Chris
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev