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

Commit Message

Tvrtko Ursulin April 22, 2016, 2:41 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

I would argue it is enough to test one pipe in the BAT set.

Signed-off-by: Tvrtko Ursulin <[e-mail]>
---
 tests/kms_pipe_crc_basic.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Chris Wilson April 22, 2016, 3:11 p.m. UTC | #1

On Fri, Apr 22, 2016 at 03:41:55PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <[e-mail]>
> 
> I would argue it is enough to test one pipe in the BAT set.
> 
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> ---
>  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.

As far as I can see this is a test that tests the BAT and not the
proposed changes.
-Chris

Jani Nikula April 25, 2016, 7 a.m. UTC | #2

On Fri, 22 Apr 2016, Tvrtko Ursulin <[e-mail]> wrote:
> From: Tvrtko Ursulin <[e-mail]>
>
> I would argue it is enough to test one pipe in the BAT set.

FWIW BYT/CHV DSI may be fixed to port C which is fixed to pipe B,
skipping the test for pipe A.

BR,
Jani.


>
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> ---
>  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))
>  			test_read_crc(&data, i, 0);
>  
> -		igt_subtest_f("read-crc-pipe-%c-frame-sequence", 'A'+i)
> +		igt_subtest_f("%sread-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
>  			test_read_crc(&data, i, TEST_SEQUENCE);
>  
> -		igt_subtest_f("nonblocking-crc-pipe-%c", 'A'+i)
> +		igt_subtest_f("%snonblocking-crc-pipe-%c", test_prefix(i), pipe_name(i))
>  			test_read_crc(&data, i, TEST_NONBLOCK);
>  
> -		igt_subtest_f("nonblocking-crc-pipe-%c-frame-sequence", 'A'+i)
> +		igt_subtest_f("%snonblocking-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
>  			test_read_crc(&data, i, TEST_SEQUENCE | TEST_NONBLOCK);
>  
> -		igt_subtest_f("suspend-read-crc-pipe-%c", 'A'+i) {
> +		igt_subtest_f("%ssuspend-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
>  			igt_skip_on(i >= data.display.n_pipes);
>  			igt_system_suspend_autoresume();
>  
>  			test_read_crc(&data, i, 0);
>  		}
>  
> -		igt_subtest_f("hang-read-crc-pipe-%c", 'A'+i) {
> +		igt_subtest_f("%shang-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
>  			igt_hang_ring_t hang =
>  				igt_hang_ring(data.drm_fd, I915_EXEC_RENDER);
>  			test_read_crc(&data, i, 0);

Daniel Vetter April 25, 2016, 8:51 a.m. UTC | #3

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 <[e-mail]>
> > 
> > I would argue it is enough to test one pipe in the BAT set.
> > 
> > Signed-off-by: Tvrtko Ursulin <[e-mail]>
> > ---
> >  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

Tvrtko Ursulin April 25, 2016, 9:45 a.m. UTC | #4

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 <[e-mail]>
>>>
>>> I would argue it is enough to test one pipe in the BAT set.
>>>
>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>> ---
>>>   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

Zanoni, Paulo R April 25, 2016, 8:12 p.m. UTC | #5

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 <[e-mail]>

> > > > 

> > > > I would argue it is enough to test one pipe in the BAT set.

> > > > 

> > > > Signed-off-by: Tvrtko Ursulin <[e-mail]>

> > > > ---

> > > >   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

> [e-mail]

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter April 26, 2016, 1:54 p.m. UTC | #6

On Mon, Apr 25, 2016 at 08:12:41PM +0000, Zanoni, Paulo R wrote:
> 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.

Broken record, but I don't like code disabled by default. What we could
try is to get this merged & enabled, but only for recent-ish vbt version.
That way the support nightmare should be manageable, and hopefully we can
figure out for recent firmware how it's supposed to work.

CI could then still use the option to enable fast timings everywhere (or
at least on those machines where it works).

Thanks, Daniel

Patch

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))
 			test_read_crc(&data, i, 0);
 
-		igt_subtest_f("read-crc-pipe-%c-frame-sequence", 'A'+i)
+		igt_subtest_f("%sread-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
 			test_read_crc(&data, i, TEST_SEQUENCE);
 
-		igt_subtest_f("nonblocking-crc-pipe-%c", 'A'+i)
+		igt_subtest_f("%snonblocking-crc-pipe-%c", test_prefix(i), pipe_name(i))
 			test_read_crc(&data, i, TEST_NONBLOCK);
 
-		igt_subtest_f("nonblocking-crc-pipe-%c-frame-sequence", 'A'+i)
+		igt_subtest_f("%snonblocking-crc-pipe-%c-frame-sequence", test_prefix(i), pipe_name(i))
 			test_read_crc(&data, i, TEST_SEQUENCE | TEST_NONBLOCK);
 
-		igt_subtest_f("suspend-read-crc-pipe-%c", 'A'+i) {
+		igt_subtest_f("%ssuspend-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
 			igt_skip_on(i >= data.display.n_pipes);
 			igt_system_suspend_autoresume();
 
 			test_read_crc(&data, i, 0);
 		}
 
-		igt_subtest_f("hang-read-crc-pipe-%c", 'A'+i) {
+		igt_subtest_f("%shang-read-crc-pipe-%c", test_prefix(i), pipe_name(i)) {
 			igt_hang_ring_t hang =
 				igt_hang_ring(data.drm_fd, I915_EXEC_RENDER);
 			test_read_crc(&data, i, 0);