Message ID | 20161019192617.18982-1-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: > The inter-engine synchronisation (with and without semaphores) is > equally exercised by gem_sync, so leave gem_storedw_loop out of the > "quick" set. How equally is "equally"? Is the test actually redundant, should it be removed altogether? -- Petri Latvala > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > tests/gem_storedw_loop.c | 6 +++--- > tests/intel-ci/fast-feedback.testlist | 7 ------- > 2 files changed, 3 insertions(+), 10 deletions(-) > > diff --git a/tests/gem_storedw_loop.c b/tests/gem_storedw_loop.c > index 317b8c6..4f05b21 100644 > --- a/tests/gem_storedw_loop.c > +++ b/tests/gem_storedw_loop.c > @@ -185,14 +185,14 @@ igt_main > } > > for (e = intel_execution_engines; e->name; e++) { > - igt_subtest_f("basic-%s", e->name) { > + igt_subtest_f("short-%s", e->name) { > check_test_requirements(fd, e->exec_id); > - store_test(fd, e->exec_id | e->flags, 16*1024); > + store_test(fd, e->exec_id | e->flags, 4*1024); > } > > igt_subtest_f("long-%s", e->name) { > check_test_requirements(fd, e->exec_id); > - store_test(fd, e->exec_id | e->flags, 1024*1024); > + store_test(fd, e->exec_id | e->flags, 4*1024*1024); > } > } > > diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist > index 853b911..b8fce59 100644 > --- a/tests/intel-ci/fast-feedback.testlist > +++ b/tests/intel-ci/fast-feedback.testlist > @@ -111,13 +111,6 @@ igt@gem_ringfill@basic-default > igt@gem_ringfill@basic-default-forked > igt@gem_ringfill@basic-default-hang > igt@gem_ringfill@basic-default-interruptible > -igt@gem_storedw_loop@basic-blt > -igt@gem_storedw_loop@basic-bsd > -igt@gem_storedw_loop@basic-bsd1 > -igt@gem_storedw_loop@basic-bsd2 > -igt@gem_storedw_loop@basic-default > -igt@gem_storedw_loop@basic-render > -igt@gem_storedw_loop@basic-vebox > igt@gem_sync@basic-all > igt@gem_sync@basic-each > igt@gem_sync@basic-many-each > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: > On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: > > The inter-engine synchronisation (with and without semaphores) is > > equally exercised by gem_sync, so leave gem_storedw_loop out of the > > "quick" set. > > > How equally is "equally"? Is the test actually redundant, should it be > removed altogether? The stress patterns exhibited by the test are identical to others in BAT. The accuracy tests are covered by others in BAT. The actual flow (edge coverage) will be subtly different and therefore the test is still unique and may catch future bugs not caught by others. But as far as BAT goes the likelihood of this catching something not caught by others within BAT is very very small. -Chris
On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: > On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: > > On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: > > > The inter-engine synchronisation (with and without semaphores) is > > > equally exercised by gem_sync, so leave gem_storedw_loop out of the > > > "quick" set. > > > > > > How equally is "equally"? Is the test actually redundant, should it be > > removed altogether? > > The stress patterns exhibited by the test are identical to others in > BAT. The accuracy tests are covered by others in BAT. The actual flow > (edge coverage) will be subtly different and therefore the test is still > unique and may catch future bugs not caught by others. But as far as BAT > goes the likelihood of this catching something not caught by others > within BAT is very very small. But given that we have 50k gem tests in full igt, does it really make sense to keep it? Imo there's not much point in keeping around every minute combinatorial variation if it means we can never run the full set of testcases. Some serious trimming of the herd is probably called for. Joonas/Tvrtko/Mika and other gem folks: What's your stance here? -Daniel
On Thu, Oct 20, 2016 at 11:16:16AM +0200, Daniel Vetter wrote: > On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: > > On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: > > > On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: > > > > The inter-engine synchronisation (with and without semaphores) is > > > > equally exercised by gem_sync, so leave gem_storedw_loop out of the > > > > "quick" set. > > > > > > > > > How equally is "equally"? Is the test actually redundant, should it be > > > removed altogether? > > > > The stress patterns exhibited by the test are identical to others in > > BAT. The accuracy tests are covered by others in BAT. The actual flow > > (edge coverage) will be subtly different and therefore the test is still > > unique and may catch future bugs not caught by others. But as far as BAT > > goes the likelihood of this catching something not caught by others > > within BAT is very very small. > > But given that we have 50k gem tests in full igt, does it really make > sense to keep it? Imo there's not much point in keeping around every > minute combinatorial variation if it means we can never run the full set > of testcases. Some serious trimming of the herd is probably called for. 50k? I've 500k in gem_concurrent_all alone that I wish to be run... Wishlist: fuzz testing of both the driver and gpu simultaneously. -Chris
Daniel Vetter <daniel@ffwll.ch> writes: > On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: >> On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: >> > On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: >> > > The inter-engine synchronisation (with and without semaphores) is >> > > equally exercised by gem_sync, so leave gem_storedw_loop out of the >> > > "quick" set. >> > >> > >> > How equally is "equally"? Is the test actually redundant, should it be >> > removed altogether? >> >> The stress patterns exhibited by the test are identical to others in >> BAT. The accuracy tests are covered by others in BAT. The actual flow >> (edge coverage) will be subtly different and therefore the test is still >> unique and may catch future bugs not caught by others. But as far as BAT >> goes the likelihood of this catching something not caught by others >> within BAT is very very small. > > But given that we have 50k gem tests in full igt, does it really make > sense to keep it? Imo there's not much point in keeping around every > minute combinatorial variation if it means we can never run the full set > of testcases. Some serious trimming of the herd is probably called for. > > Joonas/Tvrtko/Mika and other gem folks: What's your stance here? No strong stances. But I really dont see the problem here from gem dev point of view. Only the maintenance burden of keeping latent/inactive testcases? Having more than we can possible run is a positive problem. We can pick more berries to basket instead of planting bushes. I throw this back by asking what is 'full igt'? -Mika > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 20, 2016 at 2:28 PM, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote: > Daniel Vetter <daniel@ffwll.ch> writes: >> On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: >>> On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: >>> > On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: >>> > > The inter-engine synchronisation (with and without semaphores) is >>> > > equally exercised by gem_sync, so leave gem_storedw_loop out of the >>> > > "quick" set. >>> > >>> > >>> > How equally is "equally"? Is the test actually redundant, should it be >>> > removed altogether? >>> >>> The stress patterns exhibited by the test are identical to others in >>> BAT. The accuracy tests are covered by others in BAT. The actual flow >>> (edge coverage) will be subtly different and therefore the test is still >>> unique and may catch future bugs not caught by others. But as far as BAT >>> goes the likelihood of this catching something not caught by others >>> within BAT is very very small. >> >> But given that we have 50k gem tests in full igt, does it really make >> sense to keep it? Imo there's not much point in keeping around every >> minute combinatorial variation if it means we can never run the full set >> of testcases. Some serious trimming of the herd is probably called for. >> >> Joonas/Tvrtko/Mika and other gem folks: What's your stance here? > > No strong stances. But I really dont see the problem here from > gem dev point of view. Only the maintenance burden of keeping > latent/inactive testcases? > > Having more than we can possible run is a positive problem. > We can pick more berries to basket instead of planting bushes. > > I throw this back by asking what is 'full igt'? Atm we run about 0.1% of igt in CI. I think that's a problem, because it means lots of testcases get written, but not used. The other issue is that by not running testcases on a big set of machines we don't catch the small bugs and races in the tests itself (and there's plenty of those too). Which means they are of less quality, and hence also not that useful for QA (as in quality reporting). Which further reduces the value we can gain from having these testcases. The last one is a chicken-egg problem: Because no one runs this stuff, it doesn't get better, and that's the reason it doesn't get run in CI, which means no one runs it. Imo the only way to get out of that cycle is by applying a _lot_ of survival pressure on testcases, to weed out the stuff which is not providing a benefit. So yes I do believe we have too many testcases, and it's costing us, because it's essentially dead code. So phrased another way: Why do we want to keep dead code in igt, but are happy to rip out disabled-by-default features in the kernel? Why is igt special and it's ok to treat it as a dumping ground and not clean out deadwood? I know that the shoddy shape of CI is part of the problem here, but like I said above, igt is imo also part of the problem, and there's a reinforcing cycle. -Daniel
On 20/10/2016 10:16, Daniel Vetter wrote: > On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: >> On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: >>> On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: >>>> The inter-engine synchronisation (with and without semaphores) is >>>> equally exercised by gem_sync, so leave gem_storedw_loop out of the >>>> "quick" set. >>> >>> How equally is "equally"? Is the test actually redundant, should it be >>> removed altogether? >> The stress patterns exhibited by the test are identical to others in >> BAT. The accuracy tests are covered by others in BAT. The actual flow >> (edge coverage) will be subtly different and therefore the test is still >> unique and may catch future bugs not caught by others. But as far as BAT >> goes the likelihood of this catching something not caught by others >> within BAT is very very small. > But given that we have 50k gem tests in full igt, does it really make > sense to keep it? Imo there's not much point in keeping around every > minute combinatorial variation if it means we can never run the full set > of testcases. Some serious trimming of the herd is probably called for. > > Joonas/Tvrtko/Mika and other gem folks: What's your stance here? I am very fond of gem_storedw_loop, it was indispensable during platform bringup in Fulsim in a not so distant past. Even if not so, it is a very short and simple test ("Basic CS check using MI_STORE_DATA_IMM"), while gem_sync is much more complex and deals with a different issues. Without going into wider discussion, I vote to keep this particular test. Regards, Tvrtko
On Thu, Oct 20, 2016 at 02:55:42PM +0100, Tvrtko Ursulin wrote: > > On 20/10/2016 10:16, Daniel Vetter wrote: > >On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: > >>On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: > >>>On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: > >>>>The inter-engine synchronisation (with and without semaphores) is > >>>>equally exercised by gem_sync, so leave gem_storedw_loop out of the > >>>>"quick" set. > >>> > >>>How equally is "equally"? Is the test actually redundant, should it be > >>>removed altogether? > >>The stress patterns exhibited by the test are identical to others in > >>BAT. The accuracy tests are covered by others in BAT. The actual flow > >>(edge coverage) will be subtly different and therefore the test is still > >>unique and may catch future bugs not caught by others. But as far as BAT > >>goes the likelihood of this catching something not caught by others > >>within BAT is very very small. > >But given that we have 50k gem tests in full igt, does it really make > >sense to keep it? Imo there's not much point in keeping around every > >minute combinatorial variation if it means we can never run the full set > >of testcases. Some serious trimming of the herd is probably called for. > > > >Joonas/Tvrtko/Mika and other gem folks: What's your stance here? > > I am very fond of gem_storedw_loop, it was indispensable during > platform bringup in Fulsim in a not so distant past. > > Even if not so, it is a very short and simple test ("Basic CS check > using MI_STORE_DATA_IMM"), while gem_sync is much more complex and > deals with a different issues. See gem_exec_store for an even shorter sanity test of CS (also in BAT). We have overlap between this, gem_exec_basic, gem_exec_store, gem_exec_nop, gem_ringfill and gem_sync. > Without going into wider discussion, I vote to keep this particular test. In BAT? -Chris
On 20/10/2016 15:02, Chris Wilson wrote: > On Thu, Oct 20, 2016 at 02:55:42PM +0100, Tvrtko Ursulin wrote: >> On 20/10/2016 10:16, Daniel Vetter wrote: >>> On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: >>>> On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: >>>>> On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: >>>>>> The inter-engine synchronisation (with and without semaphores) is >>>>>> equally exercised by gem_sync, so leave gem_storedw_loop out of the >>>>>> "quick" set. >>>>> How equally is "equally"? Is the test actually redundant, should it be >>>>> removed altogether? >>>> The stress patterns exhibited by the test are identical to others in >>>> BAT. The accuracy tests are covered by others in BAT. The actual flow >>>> (edge coverage) will be subtly different and therefore the test is still >>>> unique and may catch future bugs not caught by others. But as far as BAT >>>> goes the likelihood of this catching something not caught by others >>>> within BAT is very very small. >>> But given that we have 50k gem tests in full igt, does it really make >>> sense to keep it? Imo there's not much point in keeping around every >>> minute combinatorial variation if it means we can never run the full set >>> of testcases. Some serious trimming of the herd is probably called for. >>> >>> Joonas/Tvrtko/Mika and other gem folks: What's your stance here? >> I am very fond of gem_storedw_loop, it was indispensable during >> platform bringup in Fulsim in a not so distant past. >> >> Even if not so, it is a very short and simple test ("Basic CS check >> using MI_STORE_DATA_IMM"), while gem_sync is much more complex and >> deals with a different issues. > See gem_exec_store for an even shorter sanity test of CS (also in BAT). > We have overlap between this, gem_exec_basic, gem_exec_store, > gem_exec_nop, gem_ringfill and gem_sync. > >> Without going into wider discussion, I vote to keep this particular test. > In BAT? No sorry I read some later bits of the thread and came back to reply here - I was referring just to the igt codebase. Regards, Tvrtko > -Chris >
On Thu, Oct 20, 2016 at 03:18:00PM +0100, Tvrtko Ursulin wrote: > > On 20/10/2016 15:02, Chris Wilson wrote: > > On Thu, Oct 20, 2016 at 02:55:42PM +0100, Tvrtko Ursulin wrote: > > > On 20/10/2016 10:16, Daniel Vetter wrote: > > > > On Thu, Oct 20, 2016 at 09:54:33AM +0100, Chris Wilson wrote: > > > > > On Thu, Oct 20, 2016 at 11:45:47AM +0300, Petri Latvala wrote: > > > > > > On Wed, Oct 19, 2016 at 08:26:17PM +0100, Chris Wilson wrote: > > > > > > > The inter-engine synchronisation (with and without semaphores) is > > > > > > > equally exercised by gem_sync, so leave gem_storedw_loop out of the > > > > > > > "quick" set. > > > > > > How equally is "equally"? Is the test actually redundant, should it be > > > > > > removed altogether? > > > > > The stress patterns exhibited by the test are identical to others in > > > > > BAT. The accuracy tests are covered by others in BAT. The actual flow > > > > > (edge coverage) will be subtly different and therefore the test is still > > > > > unique and may catch future bugs not caught by others. But as far as BAT > > > > > goes the likelihood of this catching something not caught by others > > > > > within BAT is very very small. > > > > But given that we have 50k gem tests in full igt, does it really make > > > > sense to keep it? Imo there's not much point in keeping around every > > > > minute combinatorial variation if it means we can never run the full set > > > > of testcases. Some serious trimming of the herd is probably called for. > > > > > > > > Joonas/Tvrtko/Mika and other gem folks: What's your stance here? > > > I am very fond of gem_storedw_loop, it was indispensable during > > > platform bringup in Fulsim in a not so distant past. > > > > > > Even if not so, it is a very short and simple test ("Basic CS check > > > using MI_STORE_DATA_IMM"), while gem_sync is much more complex and > > > deals with a different issues. > > See gem_exec_store for an even shorter sanity test of CS (also in BAT). > > We have overlap between this, gem_exec_basic, gem_exec_store, > > gem_exec_nop, gem_ringfill and gem_sync. > > > > > Without going into wider discussion, I vote to keep this particular test. > > In BAT? > > No sorry I read some later bits of the thread and came back to reply here - > I was referring just to the igt codebase. Am I understanding correctly that gem folks don't object to gem_storedw_loop being removed from BAT? -- Petri Latvala
On Tue, Oct 25, 2016 at 01:07:25PM +0300, Petri Latvala wrote: > Am I understanding correctly that gem folks don't object to > gem_storedw_loop being removed from BAT? Interpreting silence as a yes. Acked-by: Petri Latvala <petri.latvala@intel.com> Please push this. -- Petri Latvala
diff --git a/tests/gem_storedw_loop.c b/tests/gem_storedw_loop.c index 317b8c6..4f05b21 100644 --- a/tests/gem_storedw_loop.c +++ b/tests/gem_storedw_loop.c @@ -185,14 +185,14 @@ igt_main } for (e = intel_execution_engines; e->name; e++) { - igt_subtest_f("basic-%s", e->name) { + igt_subtest_f("short-%s", e->name) { check_test_requirements(fd, e->exec_id); - store_test(fd, e->exec_id | e->flags, 16*1024); + store_test(fd, e->exec_id | e->flags, 4*1024); } igt_subtest_f("long-%s", e->name) { check_test_requirements(fd, e->exec_id); - store_test(fd, e->exec_id | e->flags, 1024*1024); + store_test(fd, e->exec_id | e->flags, 4*1024*1024); } } diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist index 853b911..b8fce59 100644 --- a/tests/intel-ci/fast-feedback.testlist +++ b/tests/intel-ci/fast-feedback.testlist @@ -111,13 +111,6 @@ igt@gem_ringfill@basic-default igt@gem_ringfill@basic-default-forked igt@gem_ringfill@basic-default-hang igt@gem_ringfill@basic-default-interruptible -igt@gem_storedw_loop@basic-blt -igt@gem_storedw_loop@basic-bsd -igt@gem_storedw_loop@basic-bsd1 -igt@gem_storedw_loop@basic-bsd2 -igt@gem_storedw_loop@basic-default -igt@gem_storedw_loop@basic-render -igt@gem_storedw_loop@basic-vebox igt@gem_sync@basic-all igt@gem_sync@basic-each igt@gem_sync@basic-many-each
The inter-engine synchronisation (with and without semaphores) is equally exercised by gem_sync, so leave gem_storedw_loop out of the "quick" set. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- tests/gem_storedw_loop.c | 6 +++--- tests/intel-ci/fast-feedback.testlist | 7 ------- 2 files changed, 3 insertions(+), 10 deletions(-)