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

[Tvrtko Ursulin]

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.

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