[Intel-gfx] [PATCH i-g-t 1/2] kms_pipe_crc_basic: Only test one pipe in the basic set

[Daniel Vetter]

On Fri, Apr 22, 2016 at 04:11:11PM +0100, Chris Wilson wrote:
> On Fri, Apr 22, 2016 at 03:41:55PM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > 
> > I would argue it is enough to test one pipe in the BAT set.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >  tests/kms_pipe_crc_basic.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> > index 4de53bc77a3a..291775934758 100644
> > --- a/tests/kms_pipe_crc_basic.c
> > +++ b/tests/kms_pipe_crc_basic.c
> > @@ -180,6 +180,9 @@ static void test_read_crc(data_t *data, int pipe, unsigned flags)
> >  
> >  data_t data = {0, };
> >  
> > +#define test_prefix(i) ((i) == 0 ? "basic-" : "")
> > +#define pipe_name(i) ((i) + 'A')
> > +
> >  igt_main
> >  {
> >  	igt_skip_on_simulation();
> > @@ -196,39 +199,39 @@ igt_main
> >  		igt_display_init(&data.display, data.drm_fd);
> >  	}
> >  
> > -	igt_subtest("bad-pipe")
> > +	igt_subtest("basic-bad-pipe")
> >  		test_bad_command(&data, "pipe D none");
> >  
> > -	igt_subtest("bad-source")
> > +	igt_subtest("basic-bad-source")
> >  		test_bad_command(&data, "pipe A foo");
> >  
> > -	igt_subtest("bad-nb-words-1")
> > +	igt_subtest("basic-bad-nb-words-1")
> >  		test_bad_command(&data, "pipe foo");
> >  
> > -	igt_subtest("bad-nb-words-3")
> > +	igt_subtest("basic-bad-nb-words-3")
> >  		test_bad_command(&data, "pipe A none option");
> >  
> >  	for (int i = 0; i < 3; i++) {
> > -		igt_subtest_f("read-crc-pipe-%c", 'A'+i)
> > +		igt_subtest_f("%sread-crc-pipe-%c", test_prefix(i), pipe_name(i))
> 
> So the CRC is the backchannel through which we measure the output on the
> screen matches expectations. Do any of the following belong in the basic
> test set? Having demonstrated that the CRC is functional, all the rest
> are components of other tests - and if a bug if found in any of the
> other basic tests, one can run the entire kms_pipe_crc to sanity check
> the test suite itself.

The reason I wanted to include kms_pipe_crc_basic was not to validate the
kernel driver (it's a side-effect), but to make sure the CRC support is
really stable. Otherwise we can't rely on a full BAT run at all.

And yes every single subtest (including testing on all the pipes) included
in there is for a bug in the CRC code that actually happened.

I know that we're not there yet with CI, and we don't yet run all the nice
kms_*crc tests we have. But imo this is crucial prep work for that, and
we really shouldn't reduce test coverage in this area. Same reason we have
1 edp sink crc testcase, essentially it's to make sure that we have a
reasonable basis to run the full test set.

Imo BAT should guarantee two things:
- no insta-fireworks
- all the validation stuff actually works and running more tests will give
  you valid results

>From the above reasons I'm against this patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch