[RFC,i-g-t,1/2] extended.testlist: Remove default and render engine test duplicates

Commit Message

Tvrtko Ursulin June 16, 2017, 10:55 a.m. UTC

From: Tvrtko Ursulin <[e-mail]>

Where there is both default and render for the same test,
remove the former to save some execution time.

Signed-off-by: Tvrtko Ursulin <[e-mail]>
---
 tests/intel-ci/extended.testlist | 21 ---------------------
 1 file changed, 21 deletions(-)

Comments

Chris Wilson June 16, 2017, 4 p.m. UTC | #1

Quoting Tvrtko Ursulin (2017-06-16 11:55:29)
> From: Tvrtko Ursulin <[e-mail]>
> 
> Where there is both default and render for the same test,
> remove the former to save some execution time.

Fwiw, ack to using a subset of engines in each test. Though I'd prefer
it if we engineered a better subtest for each test that gave coverage
across all, the caveat being that we need clear detection of the error
(e.g. if we have a dodgy flush on one engine).
-Chris

Daniel Vetter June 22, 2017, 9:54 a.m. UTC | #2

On Fri, Jun 16, 2017 at 12:55 PM, Tvrtko Ursulin <[e-mail]> wrote:
> From: Tvrtko Ursulin <[e-mail]>
>
> Where there is both default and render for the same test,
> remove the former to save some execution time.

If they are redundant, why do we even have them? Can we just remove
the testcase itself? Accumulating unused tests of questionable use at
best in igt is serious pain, because it means we never can get to a
world where new testcases are auto-added to CI withou some manual
review. And that's the world of pain we live in now and I really want
to get out of it. That means reviewing and removing testcases, not
massaging courated testlists forever, I don't think we have the time
for that among all the other tasks.

</rant> :-)

Cheers, Daniel
>
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> ---
>  tests/intel-ci/extended.testlist | 21 ---------------------
>  1 file changed, 21 deletions(-)
>
> diff --git a/tests/intel-ci/extended.testlist b/tests/intel-ci/extended.testlist
> index 24ec295faf66..e0926ff09bcd 100644
> --- a/tests/intel-ci/extended.testlist
> +++ b/tests/intel-ci/extended.testlist
> @@ -76,7 +76,6 @@ igt@gem_ringfill@blt-bomb
>  igt@gem_ringfill@bsd-bomb
>  igt@gem_ringfill@bsd1-bomb
>  igt@gem_ringfill@bsd2-bomb
> -igt@gem_ringfill@default-bomb
>  igt@gem_ringfill@render-bomb
>  igt@gem_ringfill@vebox-bomb
>  igt@gem_userptr_blits@stress-mm
> @@ -1154,12 +1153,10 @@ igt@gem_bad_reloc@negative-reloc-bltcopy
>  igt@gem_bad_reloc@negative-reloc-bsd
>  igt@gem_bad_reloc@negative-reloc-bsd1
>  igt@gem_bad_reloc@negative-reloc-bsd2
> -igt@gem_bad_reloc@negative-reloc-default
>  igt@gem_bad_reloc@negative-reloc-lut-blt
>  igt@gem_bad_reloc@negative-reloc-lut-bsd
>  igt@gem_bad_reloc@negative-reloc-lut-bsd1
>  igt@gem_bad_reloc@negative-reloc-lut-bsd2
> -igt@gem_bad_reloc@negative-reloc-lut-default
>  igt@gem_bad_reloc@negative-reloc-lut-render
>  igt@gem_bad_reloc@negative-reloc-lut-vebox
>  igt@gem_bad_reloc@negative-reloc-render
> @@ -1203,7 +1200,6 @@ igt@gem_cs_prefetch@blt
>  igt@gem_cs_prefetch@bsd
>  igt@gem_cs_prefetch@bsd1
>  igt@gem_cs_prefetch@bsd2
> -igt@gem_cs_prefetch@default
>  igt@gem_cs_prefetch@render
>  igt@gem_cs_prefetch@vebox
>  igt@gem_cs_tlb@blt
> @@ -1220,7 +1216,6 @@ igt@gem_ctx_bad_exec@blt
>  igt@gem_ctx_bad_exec@bsd
>  igt@gem_ctx_bad_exec@bsd1
>  igt@gem_ctx_bad_exec@bsd2
> -igt@gem_ctx_bad_exec@default
>  igt@gem_ctx_bad_exec@render
>  igt@gem_ctx_bad_exec@vebox
>  igt@gem_ctx_exec@lrc-lite-restore
> @@ -1244,7 +1239,6 @@ igt@gem_ctx_switch@bsd1
>  igt@gem_ctx_switch@bsd1-interruptible
>  igt@gem_ctx_switch@bsd2
>  igt@gem_ctx_switch@bsd2-interruptible
> -igt@gem_ctx_switch@default-interruptible
>  igt@gem_ctx_switch@render
>  igt@gem_ctx_switch@render-interruptible
>  igt@gem_ctx_switch@vebox
> @@ -1322,7 +1316,6 @@ igt@gem_exec_nop@blt
>  igt@gem_exec_nop@bsd
>  igt@gem_exec_nop@bsd1
>  igt@gem_exec_nop@bsd2
> -igt@gem_exec_nop@default
>  igt@gem_exec_nop@parallel
>  igt@gem_exec_nop@render
>  igt@gem_exec_nop@series
> @@ -1340,9 +1333,6 @@ igt@gem_exec_parallel@bsd2
>  igt@gem_exec_parallel@bsd2-contexts
>  igt@gem_exec_parallel@bsd2-fds
>  igt@gem_exec_parallel@contexts
> -igt@gem_exec_parallel@default
> -igt@gem_exec_parallel@default-contexts
> -igt@gem_exec_parallel@default-fds
>  igt@gem_exec_parallel@fds
>  igt@gem_exec_parallel@render
>  igt@gem_exec_parallel@render-contexts
> @@ -1392,7 +1382,6 @@ igt@gem_exec_reloc@active-blt
>  igt@gem_exec_reloc@active-bsd
>  igt@gem_exec_reloc@active-bsd1
>  igt@gem_exec_reloc@active-bsd2
> -igt@gem_exec_reloc@active-default
>  igt@gem_exec_reloc@active-render
>  igt@gem_exec_reloc@active-vebox
>  igt@gem_exec_schedule@deep-blt
> @@ -1431,7 +1420,6 @@ igt@gem_exec_whisper@bsd1-normal
>  igt@gem_exec_whisper@bsd2-normal
>  igt@gem_exec_whisper@chain
>  igt@gem_exec_whisper@contexts
> -igt@gem_exec_whisper@default-normal
>  igt@gem_exec_whisper@fds
>  igt@gem_exec_whisper@forked
>  igt@gem_exec_whisper@interruptible
> @@ -1682,8 +1670,6 @@ igt@gem_ringfill@bsd2
>  igt@gem_ringfill@bsd2-child
>  igt@gem_ringfill@bsd2-interruptible
>  igt@gem_ringfill@bsd2-s3
> -igt@gem_ringfill@default-child
> -igt@gem_ringfill@default-s3
>  igt@gem_ringfill@render
>  igt@gem_ringfill@render-child
>  igt@gem_ringfill@render-interruptible
> @@ -1728,26 +1714,22 @@ igt@gem_storedw_loop@long-blt
>  igt@gem_storedw_loop@long-bsd
>  igt@gem_storedw_loop@long-bsd1
>  igt@gem_storedw_loop@long-bsd2
> -igt@gem_storedw_loop@long-default
>  igt@gem_storedw_loop@long-render
>  igt@gem_storedw_loop@long-vebox
>  igt@gem_storedw_loop@short-blt
>  igt@gem_storedw_loop@short-bsd
>  igt@gem_storedw_loop@short-bsd1
>  igt@gem_storedw_loop@short-bsd2
> -igt@gem_storedw_loop@short-default
>  igt@gem_storedw_loop@short-render
>  igt@gem_storedw_loop@short-vebox
>  igt@gem_sync@blt
>  igt@gem_sync@bsd
>  igt@gem_sync@bsd1
>  igt@gem_sync@bsd2
> -igt@gem_sync@default
>  igt@gem_sync@many-blt
>  igt@gem_sync@many-bsd
>  igt@gem_sync@many-bsd1
>  igt@gem_sync@many-bsd2
> -igt@gem_sync@many-default
>  igt@gem_sync@many-render
>  igt@gem_sync@many-vebox
>  igt@gem_sync@render
> @@ -1755,7 +1737,6 @@ igt@gem_sync@store-blt
>  igt@gem_sync@store-bsd
>  igt@gem_sync@store-bsd1
>  igt@gem_sync@store-bsd2
> -igt@gem_sync@store-default
>  igt@gem_sync@store-render
>  igt@gem_sync@store-vebox
>  igt@gem_sync@vebox
> @@ -1808,7 +1789,6 @@ igt@gem_wait@busy-blt
>  igt@gem_wait@busy-bsd
>  igt@gem_wait@busy-bsd1
>  igt@gem_wait@busy-bsd2
> -igt@gem_wait@busy-default
>  igt@gem_wait@busy-render
>  igt@gem_wait@busy-vebox
>  igt@gem_wait@invalid-buf
> @@ -1817,7 +1797,6 @@ igt@gem_wait@wait-blt
>  igt@gem_wait@wait-bsd
>  igt@gem_wait@wait-bsd1
>  igt@gem_wait@wait-bsd2
> -igt@gem_wait@wait-default
>  igt@gem_wait@wait-render
>  igt@gem_wait@wait-vebox
>  igt@gem_workarounds@reset
> --
> 2.9.4
>
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Tvrtko Ursulin June 22, 2017, 10:45 a.m. UTC | #3

On 22/06/2017 10:54, Daniel Vetter wrote:
> On Fri, Jun 16, 2017 at 12:55 PM, Tvrtko Ursulin <[e-mail]> wrote:
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> Where there is both default and render for the same test,
>> remove the former to save some execution time.
> 
> If they are redundant, why do we even have them? Can we just remove

Maybe we can remove the default entry from intel_execution_engines 
array? And just test that default == render explicitly in a few short tests.

> the testcase itself? Accumulating unused tests of questionable use at
> best in igt is serious pain, because it means we never can get to a
> world where new testcases are auto-added to CI withou some manual
> review. And that's the world of pain we live in now and I really want
> to get out of it. That means reviewing and removing testcases, not
> massaging courated testlists forever, I don't think we have the time
> for that among all the other tasks.
> 
> </rant> :-)

I am not a fan of the testlist neither, but thought to avoid letting 
perfect being the enemy of good, and at least do something until the 
final solution crystallizes.

Regards,

Tvrtko

Daniel Vetter June 23, 2017, 11:14 a.m. UTC | #4

On Thu, Jun 22, 2017 at 12:45 PM, Tvrtko Ursulin
<[e-mail]> wrote:
>
> On 22/06/2017 10:54, Daniel Vetter wrote:
>>
>> On Fri, Jun 16, 2017 at 12:55 PM, Tvrtko Ursulin <[e-mail]>
>> wrote:
>>>
>>> From: Tvrtko Ursulin <[e-mail]>
>>>
>>> Where there is both default and render for the same test,
>>> remove the former to save some execution time.
>>
>>
>> If they are redundant, why do we even have them? Can we just remove
>
>
> Maybe we can remove the default entry from intel_execution_engines array?
> And just test that default == render explicitly in a few short tests.

That has my ack, in case you have the patch ...

>> the testcase itself? Accumulating unused tests of questionable use at
>> best in igt is serious pain, because it means we never can get to a
>> world where new testcases are auto-added to CI withou some manual
>> review. And that's the world of pain we live in now and I really want
>> to get out of it. That means reviewing and removing testcases, not
>> massaging courated testlists forever, I don't think we have the time
>> for that among all the other tasks.
>>
>> </rant> :-)
>
>
> I am not a fan of the testlist neither, but thought to avoid letting perfect
> being the enemy of good, and at least do something until the final solution
> crystallizes.

Yeah not going to block this here, just wanted to chime in with what I
think we need to do in the longer-term. I really want CI to run all
igt, and run new igt tests by default, to make sure both tests are
high-quality (no noise), and the kernel doesn't randomly regress
because we didn't run the igts :-/
-Daniel

Chris Wilson June 23, 2017, 11:21 a.m. UTC | #5

Quoting Daniel Vetter (2017-06-23 12:14:49)
> On Thu, Jun 22, 2017 at 12:45 PM, Tvrtko Ursulin
> <[e-mail]> wrote:
> >
> > On 22/06/2017 10:54, Daniel Vetter wrote:
> >>
> >> On Fri, Jun 16, 2017 at 12:55 PM, Tvrtko Ursulin <[e-mail]>
> >> wrote:
> >>>
> >>> From: Tvrtko Ursulin <[e-mail]>
> >>>
> >>> Where there is both default and render for the same test,
> >>> remove the former to save some execution time.
> >>
> >>
> >> If they are redundant, why do we even have them? Can we just remove
> >
> >
> > Maybe we can remove the default entry from intel_execution_engines array?
> > And just test that default == render explicitly in a few short tests.
> 
> That has my ack, in case you have the patch ...

No. The ABI does allow for the switch in theory and it is a distinct
enum so it does need testing.

> >> the testcase itself? Accumulating unused tests of questionable use at
> >> best in igt is serious pain, because it means we never can get to a
> >> world where new testcases are auto-added to CI withou some manual
> >> review. And that's the world of pain we live in now and I really want
> >> to get out of it. That means reviewing and removing testcases, not
> >> massaging courated testlists forever, I don't think we have the time
> >> for that among all the other tasks.
> >>
> >> </rant> :-)

It's the curated lists that are the problem, not the tests. The fact
that we don't feel it is worthwhile to continuously test minor
variations to catch weird bugs is our problem.
-Chris

Daniel Vetter June 26, 2017, 4:05 p.m. UTC | #6

On Fri, Jun 23, 2017 at 12:21:48PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-06-23 12:14:49)
> > On Thu, Jun 22, 2017 at 12:45 PM, Tvrtko Ursulin
> > <[e-mail]> wrote:
> > >
> > > On 22/06/2017 10:54, Daniel Vetter wrote:
> > >>
> > >> On Fri, Jun 16, 2017 at 12:55 PM, Tvrtko Ursulin <[e-mail]>
> > >> wrote:
> > >>>
> > >>> From: Tvrtko Ursulin <[e-mail]>
> > >>>
> > >>> Where there is both default and render for the same test,
> > >>> remove the former to save some execution time.
> > >>
> > >>
> > >> If they are redundant, why do we even have them? Can we just remove
> > >
> > >
> > > Maybe we can remove the default entry from intel_execution_engines array?
> > > And just test that default == render explicitly in a few short tests.
> > 
> > That has my ack, in case you have the patch ...
> 
> No. The ABI does allow for the switch in theory and it is a distinct
> enum so it does need testing.

One testcase to make sure the default engine is the render engine is I
think ok. Testing all the combinatorics is over the top.

> > >> the testcase itself? Accumulating unused tests of questionable use at
> > >> best in igt is serious pain, because it means we never can get to a
> > >> world where new testcases are auto-added to CI withou some manual
> > >> review. And that's the world of pain we live in now and I really want
> > >> to get out of it. That means reviewing and removing testcases, not
> > >> massaging courated testlists forever, I don't think we have the time
> > >> for that among all the other tasks.
> > >>
> > >> </rant> :-)
> 
> It's the curated lists that are the problem, not the tests. The fact
> that we don't feel it is worthwhile to continuously test minor
> variations to catch weird bugs is our problem.

So it's an different topic, but I disagree. gem_* testcases are massively
over the top with how much machine time they blow through, to the point
where gem takes 90%+ of all machine time.

The result of that is that we simply don't run anything at all, so gem is
hurting kms_ and other test coverage. We really need to get this down to
something reasonable, and I think reasonable = completes within 1 work-day
on a fast machine. Stress-tests are ok to keep, and run on an ad-hoc
basis, but we can't have them in the default list.

Atm gem_concurrent_blt alone blows through 24 days of machine time, and
that's 24 days of not testing some other patch series. We have a too high
patch count to afford that, so if you insist on keeping all these gem
tests in the default set then we'll be able to merge about 1 patch series
per week. That just doesn't work.

Even excluding gem_concurrent_blt gem_* tests take a few times more than
everything else combined.

There's really imo just 2 options:
a) drastically reduce the time we spend running gem_* tests.
b) don't run gem_* tests, and don't merge gem_* patches.

Atm the result is that we can't even run the other igt tests, which means
everyone (not just the gem team) loses.
-Daniel

Patch

diff --git a/tests/intel-ci/extended.testlist b/tests/intel-ci/extended.testlist
index 24ec295faf66..e0926ff09bcd 100644
--- a/tests/intel-ci/extended.testlist
+++ b/tests/intel-ci/extended.testlist
@@ -76,7 +76,6 @@  igt@gem_ringfill@blt-bomb
 igt@gem_ringfill@bsd-bomb
 igt@gem_ringfill@bsd1-bomb
 igt@gem_ringfill@bsd2-bomb
-igt@gem_ringfill@default-bomb
 igt@gem_ringfill@render-bomb
 igt@gem_ringfill@vebox-bomb
 igt@gem_userptr_blits@stress-mm
@@ -1154,12 +1153,10 @@  igt@gem_bad_reloc@negative-reloc-bltcopy
 igt@gem_bad_reloc@negative-reloc-bsd
 igt@gem_bad_reloc@negative-reloc-bsd1
 igt@gem_bad_reloc@negative-reloc-bsd2
-igt@gem_bad_reloc@negative-reloc-default
 igt@gem_bad_reloc@negative-reloc-lut-blt
 igt@gem_bad_reloc@negative-reloc-lut-bsd
 igt@gem_bad_reloc@negative-reloc-lut-bsd1
 igt@gem_bad_reloc@negative-reloc-lut-bsd2
-igt@gem_bad_reloc@negative-reloc-lut-default
 igt@gem_bad_reloc@negative-reloc-lut-render
 igt@gem_bad_reloc@negative-reloc-lut-vebox
 igt@gem_bad_reloc@negative-reloc-render
@@ -1203,7 +1200,6 @@  igt@gem_cs_prefetch@blt
 igt@gem_cs_prefetch@bsd
 igt@gem_cs_prefetch@bsd1
 igt@gem_cs_prefetch@bsd2
-igt@gem_cs_prefetch@default
 igt@gem_cs_prefetch@render
 igt@gem_cs_prefetch@vebox
 igt@gem_cs_tlb@blt
@@ -1220,7 +1216,6 @@  igt@gem_ctx_bad_exec@blt
 igt@gem_ctx_bad_exec@bsd
 igt@gem_ctx_bad_exec@bsd1
 igt@gem_ctx_bad_exec@bsd2
-igt@gem_ctx_bad_exec@default
 igt@gem_ctx_bad_exec@render
 igt@gem_ctx_bad_exec@vebox
 igt@gem_ctx_exec@lrc-lite-restore
@@ -1244,7 +1239,6 @@  igt@gem_ctx_switch@bsd1
 igt@gem_ctx_switch@bsd1-interruptible
 igt@gem_ctx_switch@bsd2
 igt@gem_ctx_switch@bsd2-interruptible
-igt@gem_ctx_switch@default-interruptible
 igt@gem_ctx_switch@render
 igt@gem_ctx_switch@render-interruptible
 igt@gem_ctx_switch@vebox
@@ -1322,7 +1316,6 @@  igt@gem_exec_nop@blt
 igt@gem_exec_nop@bsd
 igt@gem_exec_nop@bsd1
 igt@gem_exec_nop@bsd2
-igt@gem_exec_nop@default
 igt@gem_exec_nop@parallel
 igt@gem_exec_nop@render
 igt@gem_exec_nop@series
@@ -1340,9 +1333,6 @@  igt@gem_exec_parallel@bsd2
 igt@gem_exec_parallel@bsd2-contexts
 igt@gem_exec_parallel@bsd2-fds
 igt@gem_exec_parallel@contexts
-igt@gem_exec_parallel@default
-igt@gem_exec_parallel@default-contexts
-igt@gem_exec_parallel@default-fds
 igt@gem_exec_parallel@fds
 igt@gem_exec_parallel@render
 igt@gem_exec_parallel@render-contexts
@@ -1392,7 +1382,6 @@  igt@gem_exec_reloc@active-blt
 igt@gem_exec_reloc@active-bsd
 igt@gem_exec_reloc@active-bsd1
 igt@gem_exec_reloc@active-bsd2
-igt@gem_exec_reloc@active-default
 igt@gem_exec_reloc@active-render
 igt@gem_exec_reloc@active-vebox
 igt@gem_exec_schedule@deep-blt
@@ -1431,7 +1420,6 @@  igt@gem_exec_whisper@bsd1-normal
 igt@gem_exec_whisper@bsd2-normal
 igt@gem_exec_whisper@chain
 igt@gem_exec_whisper@contexts
-igt@gem_exec_whisper@default-normal
 igt@gem_exec_whisper@fds
 igt@gem_exec_whisper@forked
 igt@gem_exec_whisper@interruptible
@@ -1682,8 +1670,6 @@  igt@gem_ringfill@bsd2
 igt@gem_ringfill@bsd2-child
 igt@gem_ringfill@bsd2-interruptible
 igt@gem_ringfill@bsd2-s3
-igt@gem_ringfill@default-child
-igt@gem_ringfill@default-s3
 igt@gem_ringfill@render
 igt@gem_ringfill@render-child
 igt@gem_ringfill@render-interruptible
@@ -1728,26 +1714,22 @@  igt@gem_storedw_loop@long-blt
 igt@gem_storedw_loop@long-bsd
 igt@gem_storedw_loop@long-bsd1
 igt@gem_storedw_loop@long-bsd2
-igt@gem_storedw_loop@long-default
 igt@gem_storedw_loop@long-render
 igt@gem_storedw_loop@long-vebox
 igt@gem_storedw_loop@short-blt
 igt@gem_storedw_loop@short-bsd
 igt@gem_storedw_loop@short-bsd1
 igt@gem_storedw_loop@short-bsd2
-igt@gem_storedw_loop@short-default
 igt@gem_storedw_loop@short-render
 igt@gem_storedw_loop@short-vebox
 igt@gem_sync@blt
 igt@gem_sync@bsd
 igt@gem_sync@bsd1
 igt@gem_sync@bsd2
-igt@gem_sync@default
 igt@gem_sync@many-blt
 igt@gem_sync@many-bsd
 igt@gem_sync@many-bsd1
 igt@gem_sync@many-bsd2
-igt@gem_sync@many-default
 igt@gem_sync@many-render
 igt@gem_sync@many-vebox
 igt@gem_sync@render
@@ -1755,7 +1737,6 @@  igt@gem_sync@store-blt
 igt@gem_sync@store-bsd
 igt@gem_sync@store-bsd1
 igt@gem_sync@store-bsd2
-igt@gem_sync@store-default
 igt@gem_sync@store-render
 igt@gem_sync@store-vebox
 igt@gem_sync@vebox
@@ -1808,7 +1789,6 @@  igt@gem_wait@busy-blt
 igt@gem_wait@busy-bsd
 igt@gem_wait@busy-bsd1
 igt@gem_wait@busy-bsd2
-igt@gem_wait@busy-default
 igt@gem_wait@busy-render
 igt@gem_wait@busy-vebox
 igt@gem_wait@invalid-buf
@@ -1817,7 +1797,6 @@  igt@gem_wait@wait-blt
 igt@gem_wait@wait-bsd
 igt@gem_wait@wait-bsd1
 igt@gem_wait@wait-bsd2
-igt@gem_wait@wait-default
 igt@gem_wait@wait-render
 igt@gem_wait@wait-vebox
 igt@gem_workarounds@reset