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

Mon Apr 25 20:12:41 UTC 2016

Em Seg, 2016-04-25 às 10:45 +0100, Tvrtko Ursulin escreveu:
> On 25/04/16 09:51, Daniel Vetter wrote:
> > 
> > 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.
> Okay, so these patches are more about defining on what full BAT is,
> not 
> trying to break full BAT.
> 
> > 
> > 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.
> My general idea was that the per-patch BAT (todays basic set) could
> get 
> away with running on one pipe, and then the overnight one (which we
> do 
> not have yet) would run the more extensive set.
> 
> So even if BAT runs everything on pipe A, it is still self-
> validating. 
> It is not that some tests would run on pipe A and some on all pipes, 
> that at least was the idea.
> 
> Motivation simply was that the current BAT run did 80 backlight on-
> off 
> cycles on my BDW RVP and just waiting for eDP to turn on 80 times
> takes 
> ~50 seconds in total. The test runtime was around eight minutes here.
> 
> When I turned off legacy fbdev that went to ~4 minutes. And then
> with 
> these two (and other two) patches I shaved off 1-2 minute more.

One of the things about our driver is that it tries to be very very
conservative regarding the eDP timings, leading to waits longer than
necessary for pretty much everyone. We tried to optimize this in the
past, but due to the fact that we have a single code base to support
every product released since gen 2, pretty much any sort of
optimization attempt tends to break someone's machine because maybe
they have a bad BIOS or a bad VBT or a bad panel or something else. Now
that this problem is impacting our ability to run tests, maybe it is
time to start thinking about these optimizations again?

If you want to investigate this, you could start by looking at the
patches stored here:

https://cgit.freedesktop.org/~pzanoni/linux/?h=edp-reverts

The first patch adds an option that you could try to play it, and the
other patches contain references to my previous optimization attempts
that ended up being reverted. You'll have to search the mailing list
for the actual regression reports. Feel free to adopt the patches and
change them.

Perhaps we could also think about some local changes that would only
optimize QA's systems, like a module option used by them? The problem
is that this would mean QA wouldn't be testing exactly what's upstream.
Still, it could be worth it, depending on how much time it saves.

> 
> Considering the time spent vs the typical system usage, which is
> fewer 
> display on-offs vs. much much more GEM operations, the current state 
> seemed like a sub-optimal balance to me. Especially in the context
> of 
> not allowing more GEM basic tests since the whole suite takes so
> long.
> 
> Also, for the kms_flip patches I posted I should probably have added 
> cloned test cases for the ones that I modified. So that we have for 
> example basic-plain-flip (one pipe) and plain-flip (all pipes as
> today).
> 
> Regards,
> 
> Tvrtko
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx