| Message ID | 20190212224341.26361-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] i915/gem_exec_big: Add a single shot test | expand |
On Tue, Feb 12, 2019 at 10:43:41PM +0000, Chris Wilson wrote: > CI complains that the exhaustive test of trying every size up to the > limit is too slow, so add a simple test that tries to submit one > extreme batch buffer and check all the relocations land. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105555 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/i915/gem_exec_big.c | 70 ++++++++++++++++++++++++++++++------ > tests/intel-ci/blacklist.txt | 1 + > 2 files changed, 60 insertions(+), 11 deletions(-) > > diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c > index a15672f66..6d7041cf4 100644 > --- a/tests/i915/gem_exec_big.c > +++ b/tests/i915/gem_exec_big.c > @@ -71,7 +71,7 @@ static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, c > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > gem_exec[0].alignment = 0; > gem_exec[0].offset = 0; > - gem_exec[0].flags = 0; > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > gem_exec[0].rsvd1 = 0; > gem_exec[0].rsvd2 = 0; > > @@ -154,12 +154,11 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > gem_exec[0].handle = handle; > gem_exec[0].relocation_count = nreloc; > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > memset(&execbuf, 0, sizeof(execbuf)); > execbuf.buffers_ptr = to_user_pointer(gem_exec); > execbuf.buffer_count = 1; > - execbuf.batch_start_offset = 0; > - execbuf.batch_len = 8; > execbuf.flags = flags; > > /* Avoid hitting slowpaths in the reloc processing which might yield a > @@ -197,16 +196,10 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > #undef reloc_ofs > } > > -igt_simple_main > +static void exhaustive(int fd) > { > uint32_t batch[2] = {MI_BATCH_BUFFER_END}; > uint64_t batch_size, max, ggtt_max, reloc_ofs; > - int fd; > - > - fd = drm_open_driver(DRIVER_INTEL); > - igt_require_gem(fd); > - > - use_64bit_relocs = intel_gen(intel_get_drm_devid(fd)) >= 8; > > max = 3 * gem_aperture_size(fd) / 4; > ggtt_max = 3 * gem_global_aperture_size(fd) / 4; > @@ -258,6 +251,61 @@ igt_simple_main > else > batch_size *= 2; > } > +} > + > +static void single(int i915) > +{ > + const uint32_t bbe = MI_BATCH_BUFFER_END; > + uint64_t batch_size, limit; > + uint32_t handle; > + void *ptr; > + > + batch_size = (intel_get_avail_ram_mb() - 4) << 20; /* internal slack */ > + limit = gem_aperture_size(i915) - (256 << 10); /* low pages reserved */ > + if (!gem_uses_full_ppgtt(i915)) > + limit = 3 * limit / 4; > + > + batch_size = min(batch_size, limit); > + batch_size = ALIGN(batch_size, 4096); > + igt_info("Submitting a %'"PRId64"MiB batch, %saperture size %'"PRId64"MiB\n", > + batch_size >> 20, > + gem_uses_full_ppgtt(i915) ? "" : "shared ", > + gem_aperture_size(i915) >> 20); > + intel_require_memory(1, batch_size, CHECK_RAM); > + > + handle = gem_create(i915, batch_size); > + gem_write(i915, handle, 0, &bbe, sizeof(bbe)); > + > + if (!FORCE_PREAD_PWRITE && gem_has_llc(i915)) > + ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ); > + else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915)) > + ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ); > + else > + ptr = NULL; > + > + execN(i915, handle, batch_size, 0, ptr); > + > + if (ptr) > + munmap(ptr, batch_size); > +} > + > +igt_main > +{ > + int i915 = -1; > + > + igt_fixture { > + i915 = drm_open_driver(DRIVER_INTEL); > + igt_require_gem(i915); > + > + use_64bit_relocs = intel_gen(intel_get_drm_devid(i915)) >= 8; > + } > + > + igt_subtest("single") > + single(i915); > + > + igt_subtest("exhaustive") > + exhaustive(i915); Do we still need this one? CI time isn't an endless resource (as much as we'd want to), neither is our ability to maintain everything. And if all we get is timeouts in CI I think there's better uses for that machine time. And we do use all the CI machine time, so anytime you take away 10 minutes, it's 10 minutes of not running some other testcase. -Daniel > > - close(fd); > + igt_fixture > + close(i915); > } > diff --git a/tests/intel-ci/blacklist.txt b/tests/intel-ci/blacklist.txt > index cef0da84a..0e6beeae4 100644 > --- a/tests/intel-ci/blacklist.txt > +++ b/tests/intel-ci/blacklist.txt > @@ -28,6 +28,7 @@ igt@gem_ctx_thrash(@.*)? > igt@gem_evict_alignment(@.*)? > igt@gem_evict_everything(@.*)? > igt@gem_exec_alignment@(?!.*single).* > +igt@gem_exec_big@(?!.*single).* > igt@gem_exec_capture@many-(?!4K-).* > igt@gem_exec_fence@(?!.*basic).* > igt@gem_exec_flush@(?!.*basic).* > -- > 2.20.1 > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev
Quoting Daniel Vetter (2019-02-13 10:11:27) > On Tue, Feb 12, 2019 at 10:43:41PM +0000, Chris Wilson wrote: > > CI complains that the exhaustive test of trying every size up to the > > limit is too slow, so add a simple test that tries to submit one > > extreme batch buffer and check all the relocations land. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105555 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > tests/i915/gem_exec_big.c | 70 ++++++++++++++++++++++++++++++------ > > tests/intel-ci/blacklist.txt | 1 + > > 2 files changed, 60 insertions(+), 11 deletions(-) > > > > diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c > > index a15672f66..6d7041cf4 100644 > > --- a/tests/i915/gem_exec_big.c > > +++ b/tests/i915/gem_exec_big.c > > @@ -71,7 +71,7 @@ static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, c > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > gem_exec[0].alignment = 0; > > gem_exec[0].offset = 0; > > - gem_exec[0].flags = 0; > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > gem_exec[0].rsvd1 = 0; > > gem_exec[0].rsvd2 = 0; > > > > @@ -154,12 +154,11 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > gem_exec[0].handle = handle; > > gem_exec[0].relocation_count = nreloc; > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > memset(&execbuf, 0, sizeof(execbuf)); > > execbuf.buffers_ptr = to_user_pointer(gem_exec); > > execbuf.buffer_count = 1; > > - execbuf.batch_start_offset = 0; > > - execbuf.batch_len = 8; > > execbuf.flags = flags; > > > > /* Avoid hitting slowpaths in the reloc processing which might yield a > > @@ -197,16 +196,10 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > #undef reloc_ofs > > } > > > > -igt_simple_main > > +static void exhaustive(int fd) > > { > > uint32_t batch[2] = {MI_BATCH_BUFFER_END}; > > uint64_t batch_size, max, ggtt_max, reloc_ofs; > > - int fd; > > - > > - fd = drm_open_driver(DRIVER_INTEL); > > - igt_require_gem(fd); > > - > > - use_64bit_relocs = intel_gen(intel_get_drm_devid(fd)) >= 8; > > > > max = 3 * gem_aperture_size(fd) / 4; > > ggtt_max = 3 * gem_global_aperture_size(fd) / 4; > > @@ -258,6 +251,61 @@ igt_simple_main > > else > > batch_size *= 2; > > } > > +} > > + > > +static void single(int i915) > > +{ > > + const uint32_t bbe = MI_BATCH_BUFFER_END; > > + uint64_t batch_size, limit; > > + uint32_t handle; > > + void *ptr; > > + > > + batch_size = (intel_get_avail_ram_mb() - 4) << 20; /* internal slack */ > > + limit = gem_aperture_size(i915) - (256 << 10); /* low pages reserved */ > > + if (!gem_uses_full_ppgtt(i915)) > > + limit = 3 * limit / 4; > > + > > + batch_size = min(batch_size, limit); > > + batch_size = ALIGN(batch_size, 4096); > > + igt_info("Submitting a %'"PRId64"MiB batch, %saperture size %'"PRId64"MiB\n", > > + batch_size >> 20, > > + gem_uses_full_ppgtt(i915) ? "" : "shared ", > > + gem_aperture_size(i915) >> 20); > > + intel_require_memory(1, batch_size, CHECK_RAM); > > + > > + handle = gem_create(i915, batch_size); > > + gem_write(i915, handle, 0, &bbe, sizeof(bbe)); > > + > > + if (!FORCE_PREAD_PWRITE && gem_has_llc(i915)) > > + ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ); > > + else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915)) > > + ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ); > > + else > > + ptr = NULL; > > + > > + execN(i915, handle, batch_size, 0, ptr); > > + > > + if (ptr) > > + munmap(ptr, batch_size); > > +} > > + > > +igt_main > > +{ > > + int i915 = -1; > > + > > + igt_fixture { > > + i915 = drm_open_driver(DRIVER_INTEL); > > + igt_require_gem(i915); > > + > > + use_64bit_relocs = intel_gen(intel_get_drm_devid(i915)) >= 8; > > + } > > + > > + igt_subtest("single") > > + single(i915); > > + > > + igt_subtest("exhaustive") > > + exhaustive(i915); > > Do we still need this one? CI time isn't an endless resource (as much as > we'd want to), neither is our ability to maintain everything. And if all > we get is timeouts in CI I think there's better uses for that machine > time. And we do use all the CI machine time, so anytime you take away 10 > minutes, it's 10 minutes of not running some other testcase. It's not for CI and not run in CI. CI is not the be all and end all of testing. We still have to manually find test cases for CI to run... -Chris
On Wed, Feb 13, 2019 at 11:15 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Daniel Vetter (2019-02-13 10:11:27) > > On Tue, Feb 12, 2019 at 10:43:41PM +0000, Chris Wilson wrote: > > > CI complains that the exhaustive test of trying every size up to the > > > limit is too slow, so add a simple test that tries to submit one > > > extreme batch buffer and check all the relocations land. > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105555 > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > tests/i915/gem_exec_big.c | 70 ++++++++++++++++++++++++++++++------ > > > tests/intel-ci/blacklist.txt | 1 + > > > 2 files changed, 60 insertions(+), 11 deletions(-) > > > > > > diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c > > > index a15672f66..6d7041cf4 100644 > > > --- a/tests/i915/gem_exec_big.c > > > +++ b/tests/i915/gem_exec_big.c > > > @@ -71,7 +71,7 @@ static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, c > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > gem_exec[0].alignment = 0; > > > gem_exec[0].offset = 0; > > > - gem_exec[0].flags = 0; > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > gem_exec[0].rsvd1 = 0; > > > gem_exec[0].rsvd2 = 0; > > > > > > @@ -154,12 +154,11 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > gem_exec[0].handle = handle; > > > gem_exec[0].relocation_count = nreloc; > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > > > memset(&execbuf, 0, sizeof(execbuf)); > > > execbuf.buffers_ptr = to_user_pointer(gem_exec); > > > execbuf.buffer_count = 1; > > > - execbuf.batch_start_offset = 0; > > > - execbuf.batch_len = 8; > > > execbuf.flags = flags; > > > > > > /* Avoid hitting slowpaths in the reloc processing which might yield a > > > @@ -197,16 +196,10 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > #undef reloc_ofs > > > } > > > > > > -igt_simple_main > > > +static void exhaustive(int fd) > > > { > > > uint32_t batch[2] = {MI_BATCH_BUFFER_END}; > > > uint64_t batch_size, max, ggtt_max, reloc_ofs; > > > - int fd; > > > - > > > - fd = drm_open_driver(DRIVER_INTEL); > > > - igt_require_gem(fd); > > > - > > > - use_64bit_relocs = intel_gen(intel_get_drm_devid(fd)) >= 8; > > > > > > max = 3 * gem_aperture_size(fd) / 4; > > > ggtt_max = 3 * gem_global_aperture_size(fd) / 4; > > > @@ -258,6 +251,61 @@ igt_simple_main > > > else > > > batch_size *= 2; > > > } > > > +} > > > + > > > +static void single(int i915) > > > +{ > > > + const uint32_t bbe = MI_BATCH_BUFFER_END; > > > + uint64_t batch_size, limit; > > > + uint32_t handle; > > > + void *ptr; > > > + > > > + batch_size = (intel_get_avail_ram_mb() - 4) << 20; /* internal slack */ > > > + limit = gem_aperture_size(i915) - (256 << 10); /* low pages reserved */ > > > + if (!gem_uses_full_ppgtt(i915)) > > > + limit = 3 * limit / 4; > > > + > > > + batch_size = min(batch_size, limit); > > > + batch_size = ALIGN(batch_size, 4096); > > > + igt_info("Submitting a %'"PRId64"MiB batch, %saperture size %'"PRId64"MiB\n", > > > + batch_size >> 20, > > > + gem_uses_full_ppgtt(i915) ? "" : "shared ", > > > + gem_aperture_size(i915) >> 20); > > > + intel_require_memory(1, batch_size, CHECK_RAM); > > > + > > > + handle = gem_create(i915, batch_size); > > > + gem_write(i915, handle, 0, &bbe, sizeof(bbe)); > > > + > > > + if (!FORCE_PREAD_PWRITE && gem_has_llc(i915)) > > > + ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ); > > > + else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915)) > > > + ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ); > > > + else > > > + ptr = NULL; > > > + > > > + execN(i915, handle, batch_size, 0, ptr); > > > + > > > + if (ptr) > > > + munmap(ptr, batch_size); > > > +} > > > + > > > +igt_main > > > +{ > > > + int i915 = -1; > > > + > > > + igt_fixture { > > > + i915 = drm_open_driver(DRIVER_INTEL); > > > + igt_require_gem(i915); > > > + > > > + use_64bit_relocs = intel_gen(intel_get_drm_devid(i915)) >= 8; > > > + } > > > + > > > + igt_subtest("single") > > > + single(i915); > > > + > > > + igt_subtest("exhaustive") > > > + exhaustive(i915); > > > > Do we still need this one? CI time isn't an endless resource (as much as > > we'd want to), neither is our ability to maintain everything. And if all > > we get is timeouts in CI I think there's better uses for that machine > > time. And we do use all the CI machine time, so anytime you take away 10 > > minutes, it's 10 minutes of not running some other testcase. > > It's not for CI and not run in CI. CI is not the be all and end all of > testing. We still have to manually find test cases for CI to run... It's run in drmtip runs afaict. That's time shared with a ton of other runs we do, so yeah, more time spent here means less time spent somewhere else. "Adding even more tests" when CI folks seem to say "already takes too long" just seems like the wrong direction. -Daniel
Quoting Daniel Vetter (2019-02-13 13:02:29) > On Wed, Feb 13, 2019 at 11:15 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Daniel Vetter (2019-02-13 10:11:27) > > > On Tue, Feb 12, 2019 at 10:43:41PM +0000, Chris Wilson wrote: > > > > CI complains that the exhaustive test of trying every size up to the > > > > limit is too slow, so add a simple test that tries to submit one > > > > extreme batch buffer and check all the relocations land. > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105555 > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > --- > > > > tests/i915/gem_exec_big.c | 70 ++++++++++++++++++++++++++++++------ > > > > tests/intel-ci/blacklist.txt | 1 + > > > > 2 files changed, 60 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c > > > > index a15672f66..6d7041cf4 100644 > > > > --- a/tests/i915/gem_exec_big.c > > > > +++ b/tests/i915/gem_exec_big.c > > > > @@ -71,7 +71,7 @@ static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, c > > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > > gem_exec[0].alignment = 0; > > > > gem_exec[0].offset = 0; > > > > - gem_exec[0].flags = 0; > > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > gem_exec[0].rsvd1 = 0; > > > > gem_exec[0].rsvd2 = 0; > > > > > > > > @@ -154,12 +154,11 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > > gem_exec[0].handle = handle; > > > > gem_exec[0].relocation_count = nreloc; > > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > > > > > memset(&execbuf, 0, sizeof(execbuf)); > > > > execbuf.buffers_ptr = to_user_pointer(gem_exec); > > > > execbuf.buffer_count = 1; > > > > - execbuf.batch_start_offset = 0; > > > > - execbuf.batch_len = 8; > > > > execbuf.flags = flags; > > > > > > > > /* Avoid hitting slowpaths in the reloc processing which might yield a > > > > @@ -197,16 +196,10 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > > #undef reloc_ofs > > > > } > > > > > > > > -igt_simple_main > > > > +static void exhaustive(int fd) > > > > { > > > > uint32_t batch[2] = {MI_BATCH_BUFFER_END}; > > > > uint64_t batch_size, max, ggtt_max, reloc_ofs; > > > > - int fd; > > > > - > > > > - fd = drm_open_driver(DRIVER_INTEL); > > > > - igt_require_gem(fd); > > > > - > > > > - use_64bit_relocs = intel_gen(intel_get_drm_devid(fd)) >= 8; > > > > > > > > max = 3 * gem_aperture_size(fd) / 4; > > > > ggtt_max = 3 * gem_global_aperture_size(fd) / 4; > > > > @@ -258,6 +251,61 @@ igt_simple_main > > > > else > > > > batch_size *= 2; > > > > } > > > > +} > > > > + > > > > +static void single(int i915) > > > > +{ > > > > + const uint32_t bbe = MI_BATCH_BUFFER_END; > > > > + uint64_t batch_size, limit; > > > > + uint32_t handle; > > > > + void *ptr; > > > > + > > > > + batch_size = (intel_get_avail_ram_mb() - 4) << 20; /* internal slack */ > > > > + limit = gem_aperture_size(i915) - (256 << 10); /* low pages reserved */ > > > > + if (!gem_uses_full_ppgtt(i915)) > > > > + limit = 3 * limit / 4; > > > > + > > > > + batch_size = min(batch_size, limit); > > > > + batch_size = ALIGN(batch_size, 4096); > > > > + igt_info("Submitting a %'"PRId64"MiB batch, %saperture size %'"PRId64"MiB\n", > > > > + batch_size >> 20, > > > > + gem_uses_full_ppgtt(i915) ? "" : "shared ", > > > > + gem_aperture_size(i915) >> 20); > > > > + intel_require_memory(1, batch_size, CHECK_RAM); > > > > + > > > > + handle = gem_create(i915, batch_size); > > > > + gem_write(i915, handle, 0, &bbe, sizeof(bbe)); > > > > + > > > > + if (!FORCE_PREAD_PWRITE && gem_has_llc(i915)) > > > > + ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ); > > > > + else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915)) > > > > + ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ); > > > > + else > > > > + ptr = NULL; > > > > + > > > > + execN(i915, handle, batch_size, 0, ptr); > > > > + > > > > + if (ptr) > > > > + munmap(ptr, batch_size); > > > > +} > > > > + > > > > +igt_main > > > > +{ > > > > + int i915 = -1; > > > > + > > > > + igt_fixture { > > > > + i915 = drm_open_driver(DRIVER_INTEL); > > > > + igt_require_gem(i915); > > > > + > > > > + use_64bit_relocs = intel_gen(intel_get_drm_devid(i915)) >= 8; > > > > + } > > > > + > > > > + igt_subtest("single") > > > > + single(i915); > > > > + > > > > + igt_subtest("exhaustive") > > > > + exhaustive(i915); > > > > > > Do we still need this one? CI time isn't an endless resource (as much as > > > we'd want to), neither is our ability to maintain everything. And if all > > > we get is timeouts in CI I think there's better uses for that machine > > > time. And we do use all the CI machine time, so anytime you take away 10 > > > minutes, it's 10 minutes of not running some other testcase. > > > > It's not for CI and not run in CI. CI is not the be all and end all of > > testing. We still have to manually find test cases for CI to run... > > It's run in drmtip runs afaict. That's time shared with a ton of other > runs we do, so yeah, more time spent here means less time spent > somewhere else. "Adding even more tests" when CI folks seem to say > "already takes too long" just seems like the wrong direction. Which is why I'm replacing it with a slimmer variant. > > > > CI complains that the exhaustive test of trying every size up to the > > > > limit is too slow, so add a simple test that tries to submit one > > > > extreme batch buffer and check all the relocations land. But doesn't mean the old test is worthless; far from it. -Chris
On Wed, Feb 13, 2019 at 2:05 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Daniel Vetter (2019-02-13 13:02:29) > > On Wed, Feb 13, 2019 at 11:15 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > Quoting Daniel Vetter (2019-02-13 10:11:27) > > > > On Tue, Feb 12, 2019 at 10:43:41PM +0000, Chris Wilson wrote: > > > > > CI complains that the exhaustive test of trying every size up to the > > > > > limit is too slow, so add a simple test that tries to submit one > > > > > extreme batch buffer and check all the relocations land. > > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105555 > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > --- > > > > > tests/i915/gem_exec_big.c | 70 ++++++++++++++++++++++++++++++------ > > > > > tests/intel-ci/blacklist.txt | 1 + > > > > > 2 files changed, 60 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c > > > > > index a15672f66..6d7041cf4 100644 > > > > > --- a/tests/i915/gem_exec_big.c > > > > > +++ b/tests/i915/gem_exec_big.c > > > > > @@ -71,7 +71,7 @@ static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, c > > > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > > > gem_exec[0].alignment = 0; > > > > > gem_exec[0].offset = 0; > > > > > - gem_exec[0].flags = 0; > > > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > > gem_exec[0].rsvd1 = 0; > > > > > gem_exec[0].rsvd2 = 0; > > > > > > > > > > @@ -154,12 +154,11 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > > > gem_exec[0].handle = handle; > > > > > gem_exec[0].relocation_count = nreloc; > > > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > > > > > > > memset(&execbuf, 0, sizeof(execbuf)); > > > > > execbuf.buffers_ptr = to_user_pointer(gem_exec); > > > > > execbuf.buffer_count = 1; > > > > > - execbuf.batch_start_offset = 0; > > > > > - execbuf.batch_len = 8; > > > > > execbuf.flags = flags; > > > > > > > > > > /* Avoid hitting slowpaths in the reloc processing which might yield a > > > > > @@ -197,16 +196,10 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > > > #undef reloc_ofs > > > > > } > > > > > > > > > > -igt_simple_main > > > > > +static void exhaustive(int fd) > > > > > { > > > > > uint32_t batch[2] = {MI_BATCH_BUFFER_END}; > > > > > uint64_t batch_size, max, ggtt_max, reloc_ofs; > > > > > - int fd; > > > > > - > > > > > - fd = drm_open_driver(DRIVER_INTEL); > > > > > - igt_require_gem(fd); > > > > > - > > > > > - use_64bit_relocs = intel_gen(intel_get_drm_devid(fd)) >= 8; > > > > > > > > > > max = 3 * gem_aperture_size(fd) / 4; > > > > > ggtt_max = 3 * gem_global_aperture_size(fd) / 4; > > > > > @@ -258,6 +251,61 @@ igt_simple_main > > > > > else > > > > > batch_size *= 2; > > > > > } > > > > > +} > > > > > + > > > > > +static void single(int i915) > > > > > +{ > > > > > + const uint32_t bbe = MI_BATCH_BUFFER_END; > > > > > + uint64_t batch_size, limit; > > > > > + uint32_t handle; > > > > > + void *ptr; > > > > > + > > > > > + batch_size = (intel_get_avail_ram_mb() - 4) << 20; /* internal slack */ > > > > > + limit = gem_aperture_size(i915) - (256 << 10); /* low pages reserved */ > > > > > + if (!gem_uses_full_ppgtt(i915)) > > > > > + limit = 3 * limit / 4; > > > > > + > > > > > + batch_size = min(batch_size, limit); > > > > > + batch_size = ALIGN(batch_size, 4096); > > > > > + igt_info("Submitting a %'"PRId64"MiB batch, %saperture size %'"PRId64"MiB\n", > > > > > + batch_size >> 20, > > > > > + gem_uses_full_ppgtt(i915) ? "" : "shared ", > > > > > + gem_aperture_size(i915) >> 20); > > > > > + intel_require_memory(1, batch_size, CHECK_RAM); > > > > > + > > > > > + handle = gem_create(i915, batch_size); > > > > > + gem_write(i915, handle, 0, &bbe, sizeof(bbe)); > > > > > + > > > > > + if (!FORCE_PREAD_PWRITE && gem_has_llc(i915)) > > > > > + ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ); > > > > > + else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915)) > > > > > + ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ); > > > > > + else > > > > > + ptr = NULL; > > > > > + > > > > > + execN(i915, handle, batch_size, 0, ptr); > > > > > + > > > > > + if (ptr) > > > > > + munmap(ptr, batch_size); > > > > > +} > > > > > + > > > > > +igt_main > > > > > +{ > > > > > + int i915 = -1; > > > > > + > > > > > + igt_fixture { > > > > > + i915 = drm_open_driver(DRIVER_INTEL); > > > > > + igt_require_gem(i915); > > > > > + > > > > > + use_64bit_relocs = intel_gen(intel_get_drm_devid(i915)) >= 8; > > > > > + } > > > > > + > > > > > + igt_subtest("single") > > > > > + single(i915); > > > > > + > > > > > + igt_subtest("exhaustive") > > > > > + exhaustive(i915); > > > > > > > > Do we still need this one? CI time isn't an endless resource (as much as > > > > we'd want to), neither is our ability to maintain everything. And if all > > > > we get is timeouts in CI I think there's better uses for that machine > > > > time. And we do use all the CI machine time, so anytime you take away 10 > > > > minutes, it's 10 minutes of not running some other testcase. > > > > > > It's not for CI and not run in CI. CI is not the be all and end all of > > > testing. We still have to manually find test cases for CI to run... > > > > It's run in drmtip runs afaict. That's time shared with a ton of other > > runs we do, so yeah, more time spent here means less time spent > > somewhere else. "Adding even more tests" when CI folks seem to say > > "already takes too long" just seems like the wrong direction. > > Which is why I'm replacing it with a slimmer variant. It's added, not replaced. > > > > > CI complains that the exhaustive test of trying every size up to the > > > > > limit is too slow, so add a simple test that tries to submit one > > > > > extreme batch buffer and check all the relocations land. > > But doesn't mean the old test is worthless; far from it. It's not about 0 value vs !0 value, it's about value/machine time. And yes it's kinda impossible to be sure that changing the test won't change coverage in just the worst possible way, but simply keeping all the tests isn't a solution either. -Daniel
Quoting Daniel Vetter (2019-02-13 13:08:32) > On Wed, Feb 13, 2019 at 2:05 PM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Daniel Vetter (2019-02-13 13:02:29) > > > On Wed, Feb 13, 2019 at 11:15 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Daniel Vetter (2019-02-13 10:11:27) > > > > > On Tue, Feb 12, 2019 at 10:43:41PM +0000, Chris Wilson wrote: > > > > > > CI complains that the exhaustive test of trying every size up to the > > > > > > limit is too slow, so add a simple test that tries to submit one > > > > > > extreme batch buffer and check all the relocations land. > > > > > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105555 > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > --- > > > > > > tests/i915/gem_exec_big.c | 70 ++++++++++++++++++++++++++++++------ > > > > > > tests/intel-ci/blacklist.txt | 1 + > > > > > > 2 files changed, 60 insertions(+), 11 deletions(-) > > > > > > > > > > > > diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c > > > > > > index a15672f66..6d7041cf4 100644 > > > > > > --- a/tests/i915/gem_exec_big.c > > > > > > +++ b/tests/i915/gem_exec_big.c > > > > > > @@ -71,7 +71,7 @@ static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, c > > > > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > > > > gem_exec[0].alignment = 0; > > > > > > gem_exec[0].offset = 0; > > > > > > - gem_exec[0].flags = 0; > > > > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > > > gem_exec[0].rsvd1 = 0; > > > > > > gem_exec[0].rsvd2 = 0; > > > > > > > > > > > > @@ -154,12 +154,11 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > > > > gem_exec[0].handle = handle; > > > > > > gem_exec[0].relocation_count = nreloc; > > > > > > gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); > > > > > > + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; > > > > > > > > > > > > memset(&execbuf, 0, sizeof(execbuf)); > > > > > > execbuf.buffers_ptr = to_user_pointer(gem_exec); > > > > > > execbuf.buffer_count = 1; > > > > > > - execbuf.batch_start_offset = 0; > > > > > > - execbuf.batch_len = 8; > > > > > > execbuf.flags = flags; > > > > > > > > > > > > /* Avoid hitting slowpaths in the reloc processing which might yield a > > > > > > @@ -197,16 +196,10 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, > > > > > > #undef reloc_ofs > > > > > > } > > > > > > > > > > > > -igt_simple_main > > > > > > +static void exhaustive(int fd) > > > > > > { > > > > > > uint32_t batch[2] = {MI_BATCH_BUFFER_END}; > > > > > > uint64_t batch_size, max, ggtt_max, reloc_ofs; > > > > > > - int fd; > > > > > > - > > > > > > - fd = drm_open_driver(DRIVER_INTEL); > > > > > > - igt_require_gem(fd); > > > > > > - > > > > > > - use_64bit_relocs = intel_gen(intel_get_drm_devid(fd)) >= 8; > > > > > > > > > > > > max = 3 * gem_aperture_size(fd) / 4; > > > > > > ggtt_max = 3 * gem_global_aperture_size(fd) / 4; > > > > > > @@ -258,6 +251,61 @@ igt_simple_main > > > > > > else > > > > > > batch_size *= 2; > > > > > > } > > > > > > +} > > > > > > + > > > > > > +static void single(int i915) > > > > > > +{ > > > > > > + const uint32_t bbe = MI_BATCH_BUFFER_END; > > > > > > + uint64_t batch_size, limit; > > > > > > + uint32_t handle; > > > > > > + void *ptr; > > > > > > + > > > > > > + batch_size = (intel_get_avail_ram_mb() - 4) << 20; /* internal slack */ > > > > > > + limit = gem_aperture_size(i915) - (256 << 10); /* low pages reserved */ > > > > > > + if (!gem_uses_full_ppgtt(i915)) > > > > > > + limit = 3 * limit / 4; > > > > > > + > > > > > > + batch_size = min(batch_size, limit); > > > > > > + batch_size = ALIGN(batch_size, 4096); > > > > > > + igt_info("Submitting a %'"PRId64"MiB batch, %saperture size %'"PRId64"MiB\n", > > > > > > + batch_size >> 20, > > > > > > + gem_uses_full_ppgtt(i915) ? "" : "shared ", > > > > > > + gem_aperture_size(i915) >> 20); > > > > > > + intel_require_memory(1, batch_size, CHECK_RAM); > > > > > > + > > > > > > + handle = gem_create(i915, batch_size); > > > > > > + gem_write(i915, handle, 0, &bbe, sizeof(bbe)); > > > > > > + > > > > > > + if (!FORCE_PREAD_PWRITE && gem_has_llc(i915)) > > > > > > + ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ); > > > > > > + else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915)) > > > > > > + ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ); > > > > > > + else > > > > > > + ptr = NULL; > > > > > > + > > > > > > + execN(i915, handle, batch_size, 0, ptr); > > > > > > + > > > > > > + if (ptr) > > > > > > + munmap(ptr, batch_size); > > > > > > +} > > > > > > + > > > > > > +igt_main > > > > > > +{ > > > > > > + int i915 = -1; > > > > > > + > > > > > > + igt_fixture { > > > > > > + i915 = drm_open_driver(DRIVER_INTEL); > > > > > > + igt_require_gem(i915); > > > > > > + > > > > > > + use_64bit_relocs = intel_gen(intel_get_drm_devid(i915)) >= 8; > > > > > > + } > > > > > > + > > > > > > + igt_subtest("single") > > > > > > + single(i915); > > > > > > + > > > > > > + igt_subtest("exhaustive") > > > > > > + exhaustive(i915); > > > > > > > > > > Do we still need this one? CI time isn't an endless resource (as much as > > > > > we'd want to), neither is our ability to maintain everything. And if all > > > > > we get is timeouts in CI I think there's better uses for that machine > > > > > time. And we do use all the CI machine time, so anytime you take away 10 > > > > > minutes, it's 10 minutes of not running some other testcase. > > > > > > > > It's not for CI and not run in CI. CI is not the be all and end all of > > > > testing. We still have to manually find test cases for CI to run... > > > > > > It's run in drmtip runs afaict. That's time shared with a ton of other > > > runs we do, so yeah, more time spent here means less time spent > > > somewhere else. "Adding even more tests" when CI folks seem to say > > > "already takes too long" just seems like the wrong direction. > > > > Which is why I'm replacing it with a slimmer variant. > > It's added, not replaced. It's replaced, just go look at the results if you are in doubt. -Chris
diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c index a15672f66..6d7041cf4 100644 --- a/tests/i915/gem_exec_big.c +++ b/tests/i915/gem_exec_big.c @@ -71,7 +71,7 @@ static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, c gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); gem_exec[0].alignment = 0; gem_exec[0].offset = 0; - gem_exec[0].flags = 0; + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; gem_exec[0].rsvd1 = 0; gem_exec[0].rsvd2 = 0; @@ -154,12 +154,11 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, gem_exec[0].handle = handle; gem_exec[0].relocation_count = nreloc; gem_exec[0].relocs_ptr = to_user_pointer(gem_reloc); + gem_exec[0].flags = EXEC_OBJECT_SUPPORTS_48B_ADDRESS; memset(&execbuf, 0, sizeof(execbuf)); execbuf.buffers_ptr = to_user_pointer(gem_exec); execbuf.buffer_count = 1; - execbuf.batch_start_offset = 0; - execbuf.batch_len = 8; execbuf.flags = flags; /* Avoid hitting slowpaths in the reloc processing which might yield a @@ -197,16 +196,10 @@ static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, #undef reloc_ofs } -igt_simple_main +static void exhaustive(int fd) { uint32_t batch[2] = {MI_BATCH_BUFFER_END}; uint64_t batch_size, max, ggtt_max, reloc_ofs; - int fd; - - fd = drm_open_driver(DRIVER_INTEL); - igt_require_gem(fd); - - use_64bit_relocs = intel_gen(intel_get_drm_devid(fd)) >= 8; max = 3 * gem_aperture_size(fd) / 4; ggtt_max = 3 * gem_global_aperture_size(fd) / 4; @@ -258,6 +251,61 @@ igt_simple_main else batch_size *= 2; } +} + +static void single(int i915) +{ + const uint32_t bbe = MI_BATCH_BUFFER_END; + uint64_t batch_size, limit; + uint32_t handle; + void *ptr; + + batch_size = (intel_get_avail_ram_mb() - 4) << 20; /* internal slack */ + limit = gem_aperture_size(i915) - (256 << 10); /* low pages reserved */ + if (!gem_uses_full_ppgtt(i915)) + limit = 3 * limit / 4; + + batch_size = min(batch_size, limit); + batch_size = ALIGN(batch_size, 4096); + igt_info("Submitting a %'"PRId64"MiB batch, %saperture size %'"PRId64"MiB\n", + batch_size >> 20, + gem_uses_full_ppgtt(i915) ? "" : "shared ", + gem_aperture_size(i915) >> 20); + intel_require_memory(1, batch_size, CHECK_RAM); + + handle = gem_create(i915, batch_size); + gem_write(i915, handle, 0, &bbe, sizeof(bbe)); + + if (!FORCE_PREAD_PWRITE && gem_has_llc(i915)) + ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ); + else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915)) + ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ); + else + ptr = NULL; + + execN(i915, handle, batch_size, 0, ptr); + + if (ptr) + munmap(ptr, batch_size); +} + +igt_main +{ + int i915 = -1; + + igt_fixture { + i915 = drm_open_driver(DRIVER_INTEL); + igt_require_gem(i915); + + use_64bit_relocs = intel_gen(intel_get_drm_devid(i915)) >= 8; + } + + igt_subtest("single") + single(i915); + + igt_subtest("exhaustive") + exhaustive(i915); - close(fd); + igt_fixture + close(i915); } diff --git a/tests/intel-ci/blacklist.txt b/tests/intel-ci/blacklist.txt index cef0da84a..0e6beeae4 100644 --- a/tests/intel-ci/blacklist.txt +++ b/tests/intel-ci/blacklist.txt @@ -28,6 +28,7 @@ igt@gem_ctx_thrash(@.*)? igt@gem_evict_alignment(@.*)? igt@gem_evict_everything(@.*)? igt@gem_exec_alignment@(?!.*single).* +igt@gem_exec_big@(?!.*single).* igt@gem_exec_capture@many-(?!4K-).* igt@gem_exec_fence@(?!.*basic).* igt@gem_exec_flush@(?!.*basic).*
CI complains that the exhaustive test of trying every size up to the limit is too slow, so add a simple test that tries to submit one extreme batch buffer and check all the relocations land. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105555 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- tests/i915/gem_exec_big.c | 70 ++++++++++++++++++++++++++++++------ tests/intel-ci/blacklist.txt | 1 + 2 files changed, 60 insertions(+), 11 deletions(-)