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@xxxxxxxxx>
> >
> > I would argue it is enough to test one pipe in the BAT set.
> >
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
