[PATCH i-g-t 0/3] test/kms_async_flip: fixes to get execution

[PATCH i-g-t 0/3] test/kms_async_flip: fixes to get execution

[Melissa Wen]

Hi,

When I was reviewing "tests/kms_async_flip: Speed up the CRC test on
Intel hw" (https://patchwork.freedesktop.org/patch/572473/), I noticied
kms_async_flip aborts in the first test case with non-intel device since
commit 7fa7602ccf ("tests/kms_async_flips: Test all modifiers").
To get the execution back, the first patch changes the name of a
non-intel modifier from "?" to "unknown".

The same commit also claims to allow failure with some modifiers, but
the path for handling fb with modifiers != LINEAR and != intel doesn't
cover other vendors requirements and isn't consistent yet. Ofc,
increasing coverage with other vendor modifiers would be nice, but before
changing from run_test() to run_test_with_modifiers() the test case was
working as expected. Moreover, it seems we need a good rework of the
_with_modifiers path to make it stable again. This regression is the
reason I choose to bring back the original approach for non-intel
devices in the third path of this series. Follow-up patches can be sent
to increase test coverage with modifiers for non-intel device.

Let me know your thoughts.

Thanks,

Melissa

Melissa Wen (3):
  test/kms_async_flips: fix test abortion due to unreadable mod name
  lib/igt_fb: fix documentation of igt_create_color_fb return
  tests/kms_async_flips: test with modifiers only on intel devices

 lib/igt_fb.c            | 5 ++---
 tests/kms_async_flips.c | 5 ++++-
 2 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH i-g-t 1/3] test/kms_async_flips: fix test abortion due to unreadable mod name

[Melissa Wen]

When looking for names of non-intel modifiers, igt_fb_modifier_name()
returns "?" which causes run_test_with_modifiers abortion and therefore
a regression to `async-flip-with-page-flip-events` test case, as below:

(kms_async_flips:9663) igt_core-CRITICAL: Invalid dynamic subtest name "pipe-A-eDP-1-?".
kms_async_flips: ../lib/igt_core.c:2296: igt_exit: Assertion `!test_with_subtests || skipped_one || succeeded_one || failed_one' failed.
Received signal SIGABRT.
Stack trace:
 #0 [fatal_sig_handler+0x17b]
 #1 [__sigaction+0x40]
 #2 [pthread_key_delete+0x14c]
 #3 [gsignal+0x12]
 #4 [abort+0xd3]
 #5 [<unknown>+0xd3]
 #6 [__assert_fail+0x42]
 #7 [igt_exit+0x1ca]
 #8 [__igt_run_dynamic_subtest+0x122]
 #9 [run_test_with_modifiers.constprop.0+0x1c7]
 #10 [__igt_unique____real_main635+0x56b]
 #11 [main+0x27]
 #12 [__libc_init_first+0x8a]
 #13 [__libc_start_main+0x85]
 #14 [_start+0x21]
Aborted

Replacing "?" with "unknown" solves the issue and async flip tests run
again.

Fixes: 7fa7602ccf ("tests/kms_async_flips: Test all modifiers")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 lib/igt_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 683eb176b..2cf94013e 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -4951,6 +4951,6 @@ const char *igt_fb_modifier_name(uint64_t modifier)
 	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
 		return "4-rc-ccs-cc";
 	default:
-		return "?";
+		return "unknown";
 	}
 }
-- 
2.43.0

[PATCH i-g-t 2/3] lib/igt_fb: fix documentation of igt_create_color_fb return

[Melissa Wen]

igt_create_fb fails without returning a negative error code (unsigned
int), so does igt_create_color_fb. Remove the `negative error code on
failure` from function docs to avoid misleading usage.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 lib/igt_fb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 2cf94013e..2446edd2b 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -2174,8 +2174,7 @@ unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
  * with the given color, which is useful for some simple pipe crc based tests.
  *
  * Returns:
- * The kms id of the created framebuffer on success or a negative error code on
- * failure.
+ * The kms id of the created framebuffer on success.
  */
 unsigned int igt_create_color_fb(int fd, int width, int height,
 				 uint32_t format, uint64_t modifier,
-- 
2.43.0

[PATCH i-g-t 3/3] tests/kms_async_flips: test with modifiers only on intel devices

[Melissa Wen]

Although increasing the coverage of tests with modifiers, the change
from run_test() to run_test_with_modifiers() is now a regression on
non-intel devices. run_test_with_modifiers() only ensures conditions for
modifiers != LINEAR on intel devices, and the original run_test() is
more consistent across vendors.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 tests/kms_async_flips.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
index 6fddad093..55d59196e 100644
--- a/tests/kms_async_flips.c
+++ b/tests/kms_async_flips.c
@@ -654,7 +654,10 @@ igt_main
 		igt_describe("Wait for page flip events in between successive asynchronous flips");
 		igt_subtest_with_dynamic("async-flip-with-page-flip-events") {
 			data.alternate_sync_async = false;
-			run_test_with_modifiers(&data, test_async_flip);
+			if (is_intel_device(data.drm_fd))
+				run_test_with_modifiers(&data, test_async_flip);
+			else
+				run_test(&data, test_async_flip);
 		}
 
 		igt_describe("Alternate between sync and async flips");
-- 
2.43.0

Re: [PATCH i-g-t 1/3] test/kms_async_flips: fix test abortion due to unreadable mod name

[Kamil Konieczny]

Hi Melissa,
On 2023-12-26 at 17:57:32 -0100, Melissa Wen wrote:
> When looking for names of non-intel modifiers, igt_fb_modifier_name()
> returns "?" which causes run_test_with_modifiers abortion and therefore
> a regression to `async-flip-with-page-flip-events` test case, as below:
> 
> (kms_async_flips:9663) igt_core-CRITICAL: Invalid dynamic subtest name "pipe-A-eDP-1-?".
> kms_async_flips: ../lib/igt_core.c:2296: igt_exit: Assertion `!test_with_subtests || skipped_one || succeeded_one || failed_one' failed.
> Received signal SIGABRT.
> Stack trace:
>  #0 [fatal_sig_handler+0x17b]
>  #1 [__sigaction+0x40]
>  #2 [pthread_key_delete+0x14c]
>  #3 [gsignal+0x12]
>  #4 [abort+0xd3]
>  #5 [<unknown>+0xd3]
>  #6 [__assert_fail+0x42]
>  #7 [igt_exit+0x1ca]
>  #8 [__igt_run_dynamic_subtest+0x122]
>  #9 [run_test_with_modifiers.constprop.0+0x1c7]
>  #10 [__igt_unique____real_main635+0x56b]
>  #11 [main+0x27]
>  #12 [__libc_init_first+0x8a]
>  #13 [__libc_start_main+0x85]
>  #14 [_start+0x21]
> Aborted
> 
> Replacing "?" with "unknown" solves the issue and async flip tests run
> again.
> 
> Fixes: 7fa7602ccf ("tests/kms_async_flips: Test all modifiers")
> Signed-off-by: Melissa Wen <mwen@igalia.com>

LGTM,
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  lib/igt_fb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 683eb176b..2cf94013e 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -4951,6 +4951,6 @@ const char *igt_fb_modifier_name(uint64_t modifier)
>  	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
>  		return "4-rc-ccs-cc";
>  	default:
> -		return "?";
> +		return "unknown";
>  	}
>  }
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 2/3] lib/igt_fb: fix documentation of igt_create_color_fb return

[Kamil Konieczny]

Hi Melissa,
On 2023-12-26 at 17:57:33 -0100, Melissa Wen wrote:
> igt_create_fb fails without returning a negative error code (unsigned
> int), so does igt_create_color_fb. Remove the `negative error code on
> failure` from function docs to avoid misleading usage.

Why not just make it correct?

int igt_create_color_fb(int fd, int width, int height,

and check for errors and return -errno or what is received.

Regards,
Kamil

> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>  lib/igt_fb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> index 2cf94013e..2446edd2b 100644
> --- a/lib/igt_fb.c
> +++ b/lib/igt_fb.c
> @@ -2174,8 +2174,7 @@ unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
>   * with the given color, which is useful for some simple pipe crc based tests.
>   *
>   * Returns:
> - * The kms id of the created framebuffer on success or a negative error code on
> - * failure.
> + * The kms id of the created framebuffer on success.
>   */
>  unsigned int igt_create_color_fb(int fd, int width, int height,
>  				 uint32_t format, uint64_t modifier,
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 3/3] tests/kms_async_flips: test with modifiers only on intel devices

[Kamil Konieczny]

Hi Melissa,
On 2023-12-26 at 17:57:34 -0100, Melissa Wen wrote:
> Although increasing the coverage of tests with modifiers, the change
> from run_test() to run_test_with_modifiers() is now a regression on
> non-intel devices. run_test_with_modifiers() only ensures conditions for
> modifiers != LINEAR on intel devices, and the original run_test() is
> more consistent across vendors.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>

LGTM,
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

> ---
>  tests/kms_async_flips.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> index 6fddad093..55d59196e 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -654,7 +654,10 @@ igt_main
>  		igt_describe("Wait for page flip events in between successive asynchronous flips");
>  		igt_subtest_with_dynamic("async-flip-with-page-flip-events") {
>  			data.alternate_sync_async = false;
> -			run_test_with_modifiers(&data, test_async_flip);
> +			if (is_intel_device(data.drm_fd))
> +				run_test_with_modifiers(&data, test_async_flip);
> +			else
> +				run_test(&data, test_async_flip);
>  		}
>  
>  		igt_describe("Alternate between sync and async flips");
> -- 
> 2.43.0
> 

Re: [PATCH i-g-t 2/3] lib/igt_fb: fix documentation of igt_create_color_fb return

[Melissa Wen]

On 01/04, Kamil Konieczny wrote:
> Hi Melissa,
> On 2023-12-26 at 17:57:33 -0100, Melissa Wen wrote:
> > igt_create_fb fails without returning a negative error code (unsigned
> > int), so does igt_create_color_fb. Remove the `negative error code on
> > failure` from function docs to avoid misleading usage.
> 
> Why not just make it correct?
> 
> int igt_create_color_fb(int fd, int width, int height,
> 
> and check for errors and return -errno or what is received.

The return value of igt_create_color_fb() derives from
igt_create_fb_with_bo_size() and igt_create_fb(). Therefore, changing it
requires modification in those three functions and, at least, all
occurences of igt_create_fb_with_bo_size and igt_create_color_fb (many).

When checking the usage of these functions, the unsigned int return for
both seems to me the current understanding because inside
igt_create_fb_with_bo_size() we have a do_or_die() call that asserts
negative errors.

I've considered changing it, but I don't have a different usage for the
function so far to justify the whole change, that's why I chose not
modifying the behavior of those functions. But let me know if I'm
missing anything that would make the change appropriate and sure I can
go for it.

Anyway, thanks for raising this point.

Best Regards,

Melissa
> 
> Regards,
> Kamil
> 
> > 
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >  lib/igt_fb.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index 2cf94013e..2446edd2b 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -2174,8 +2174,7 @@ unsigned int igt_create_fb(int fd, int width, int height, uint32_t format,
> >   * with the given color, which is useful for some simple pipe crc based tests.
> >   *
> >   * Returns:
> > - * The kms id of the created framebuffer on success or a negative error code on
> > - * failure.
> > + * The kms id of the created framebuffer on success.
> >   */
> >  unsigned int igt_create_color_fb(int fd, int width, int height,
> >  				 uint32_t format, uint64_t modifier,
> > -- 
> > 2.43.0
> >