[PATCH] drm: selftest: convert drm_plane_helper selftest to KUnit

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch

[PATCH] drm: selftest: convert drm_plane_helper selftest to KUnit

Anderson Fraga <kaspa@onionmail.org>

Details

Message ID: <20211120133847.238264-1-kaspa@onionmail.org>
DKIM signature: missing

Download raw message

2 years ago

Patch: +76 -78

test-drm_plane_helper.c is a unit test for plane_helper.c to check the
support of primary plane implementation on top of the normal CRTC
configuration interface. It also checks plane updates in the switch
process to the atomic helper infrastructure.

Convert test-drm_plane_helper.c to KUnit is a welcomed advance as relies
on a lightweight testing framework unit.

Co-developed-by: Djakson C. G. Filho <djakson.filho@gmail.com>
Signed-off-by: Djakson C. G. Filho <djakson.filho@gmail.com>
Signed-off-by: Anderson Fraga <kaspa@onionmail.org>
---
 drivers/gpu/drm/Kconfig                       |  13 ++
 drivers/gpu/drm/Makefile                      |   2 +-
 drivers/gpu/drm/selftests/Makefile            |   5 +-
 .../gpu/drm/selftests/test-drm_plane_helper.c | 134 ++++++++----------
 4 files changed, 76 insertions(+), 78 deletions(-)
 mode change 100644 => 100755 drivers/gpu/drm/Kconfig
 mode change 100644 => 100755 drivers/gpu/drm/Makefile
 mode change 100644 => 100755 drivers/gpu/drm/selftests/test-drm_plane_helper.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
old mode 100644
new mode 100755
index cea777ae7fb9..e0c8fc6d95c9
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -79,6 +79,19 @@ config DRM_DEBUG_SELFTEST
 
 	  If in doubt, say "N".
 
+config DRM_PLANE_HELPER_KUNIT_TEST
+	tristate "KUnit test for DRM plane helper"
+    depends on DRM && KUNIT
+	select DRM_KMS_HELPER
+	default KUNIT_ALL_TESTS
+	help
+	  This option provides KUnit modules that can be used to run
+	  various selftests on parts of the DRM plane helper API. This option is not
+	  useful for distributions or general kernels, but only for kernel
+	  developers working on DRM and associated drivers.
+
+	  If in doubt, say "N".
+
 config DRM_KMS_HELPER
 	tristate
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
old mode 100644
new mode 100755
index ad1112154898..e632c43c9b7d
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -58,7 +58,7 @@ drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
 drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
 
 obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
-obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-y += selftests/
 
 obj-$(CONFIG_DRM)	+= drm.o
 obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
index 484c35c7edd0..bb92454a8532 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_cmdline_parser.o \
 				    test-drm_format.o test-drm_framebuffer.o test-drm_rect.o \
-				    test-drm_plane_helper.o test-drm_dp_mst_helper.o \
-				    test-drm_damage_helper.o 
+				    test-drm_dp_mst_helper.o test-drm_damage_helper.o 
+
+obj-$(CONFIG_DRM_PLANE_HELPER_KUNIT_TEST) += test-drm_plane_helper.o 
diff --git a/drivers/gpu/drm/selftests/test-drm_plane_helper.c b/drivers/gpu/drm/selftests/test-drm_plane_helper.c
old mode 100644
new mode 100755
index a08e8c3e9cd2..522b76e19205
--- a/drivers/gpu/drm/selftests/test-drm_plane_helper.c
+++ b/drivers/gpu/drm/selftests/test-drm_plane_helper.c
@@ -3,28 +3,14 @@
  * Test cases for the drm_plane_helper functions
  */
 
-#define pr_fmt(fmt) "drm_plane_helper: " fmt
-
-#include <linux/module.h>
+#include <kunit/test.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_modes.h>
 
-#include "test-drm_modeset_common.h"
-
-#include "drm_selftest.h"
-struct drm_selftest tests[] = {
-       {
-               .enabled = true,
-               .name = "drm_igt_check_plane_state",
-               .func = igt_check_plane_state,
-       },
-};
-#include "drm_selftest.c"
-
 static void set_src(struct drm_plane_state *plane_state,
-		    unsigned src_x, unsigned src_y,
-		    unsigned src_w, unsigned src_h)
+		    unsigned int src_x, unsigned int src_y,
+		    unsigned int src_w, unsigned int src_h)
 {
 	plane_state->src_x = src_x;
 	plane_state->src_y = src_y;
@@ -33,8 +19,8 @@ static void set_src(struct drm_plane_state *plane_state,
 }
 
 static bool check_src_eq(struct drm_plane_state *plane_state,
-			 unsigned src_x, unsigned src_y,
-			 unsigned src_w, unsigned src_h)
+			 unsigned int src_x, unsigned int src_y,
+			 unsigned int src_w, unsigned int src_h)
 {
 	if (plane_state->src.x1 < 0) {
 		pr_err("src x coordinate %x should never be below 0.\n", plane_state->src.x1);
@@ -60,7 +46,7 @@ static bool check_src_eq(struct drm_plane_state *plane_state,
 
 static void set_crtc(struct drm_plane_state *plane_state,
 		     int crtc_x, int crtc_y,
-		     unsigned crtc_w, unsigned crtc_h)
+		     unsigned int crtc_w, unsigned int crtc_h)
 {
 	plane_state->crtc_x = crtc_x;
 	plane_state->crtc_y = crtc_y;
@@ -70,7 +56,7 @@ static void set_crtc(struct drm_plane_state *plane_state,
 
 static bool check_crtc_eq(struct drm_plane_state *plane_state,
 			  int crtc_x, int crtc_y,
-			  unsigned crtc_w, unsigned crtc_h)
+			  unsigned int crtc_w, unsigned int crtc_h)
 {
 	if (plane_state->dst.x1 != crtc_x ||
 	    plane_state->dst.y1 != crtc_y ||
@@ -84,7 +70,7 @@ static bool check_crtc_eq(struct drm_plane_state *plane_state,
 	return true;
 }
 
-int igt_check_plane_state(void *ignored)
+static void igt_check_plane_state(struct kunit *test)
 {
 	int ret;
 
@@ -115,10 +101,10 @@ int igt_check_plane_state(void *ignored)
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  false, false);
-	FAIL(ret < 0, "Simple clipping check should pass\n");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
 
 	/* Rotated clipping + reflection, no scaling. */
 	plane_state.rotation = DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X;
@@ -126,10 +112,10 @@ int igt_check_plane_state(void *ignored)
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  false, false);
-	FAIL(ret < 0, "Rotated clipping check should pass\n");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
 	plane_state.rotation = DRM_MODE_ROTATE_0;
 
 	/* Check whether positioning works correctly. */
@@ -139,16 +125,16 @@ int igt_check_plane_state(void *ignored)
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  false, false);
-	FAIL(!ret, "Should not be able to position on the crtc with can_position=false\n");
+	KUNIT_EXPECT_TRUE(test, ret);
 
 	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  true, false);
-	FAIL(ret < 0, "Simple positioning should work\n");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1023, 767));
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1023, 767));
 
 	/* Simple scaling tests. */
 	set_src(&plane_state, 0, 0, 512 << 16, 384 << 16);
@@ -157,28 +143,29 @@ int igt_check_plane_state(void *ignored)
 						  0x8001,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  false, false);
-	FAIL(!ret, "Upscaling out of range should fail.\n");
+	KUNIT_EXPECT_TRUE(test, ret);
 	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
 						  0x8000,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  false, false);
-	FAIL(ret < 0, "Upscaling exactly 2x should work\n");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
 
 	set_src(&plane_state, 0, 0, 2048 << 16, 1536 << 16);
 	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  0x1ffff, false, false);
-	FAIL(!ret, "Downscaling out of range should fail.\n");
+
+	KUNIT_EXPECT_TRUE(test, ret);
 	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  0x20000, false, false);
-	FAIL(ret < 0, "Should succeed with exact scaling limit\n");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
 
 	/* Testing rounding errors. */
 	set_src(&plane_state, 0, 0, 0x40001, 0x40001);
@@ -187,10 +174,11 @@ int igt_check_plane_state(void *ignored)
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  0x10001,
 						  true, false);
-	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
+
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 1022, 766, 2, 2));
 
 	set_src(&plane_state, 0x20001, 0x20001, 0x4040001, 0x3040001);
 	set_crtc(&plane_state, -2, -2, 1028, 772);
@@ -198,10 +186,10 @@ int igt_check_plane_state(void *ignored)
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  0x10001,
 						  false, false);
-	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
 
 	set_src(&plane_state, 0, 0, 0x3ffff, 0x3ffff);
 	set_crtc(&plane_state, 1022, 766, 4, 4);
@@ -209,11 +197,12 @@ int igt_check_plane_state(void *ignored)
 						  0xffff,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  true, false);
-	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
-	FAIL_ON(!plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+
 	/* Should not be rounded to 0x20001, which would be upscaling. */
-	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 1022, 766, 2, 2));
 
 	set_src(&plane_state, 0x1ffff, 0x1ffff, 0x403ffff, 0x303ffff);
 	set_crtc(&plane_state, -2, -2, 1028, 772);
@@ -221,29 +210,24 @@ int igt_check_plane_state(void *ignored)
 						  0xffff,
 						  DRM_PLANE_HELPER_NO_SCALING,
 						  false, false);
-	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
-	FAIL_ON(!plane_state.visible);
-	FAIL_ON(!check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
-	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
 
-	return 0;
+	KUNIT_EXPECT_TRUE(test, ret >= 0);
+	KUNIT_EXPECT_TRUE(test, plane_state.visible);
+	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
+	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
 }
 
-static int __init test_drm_plane_helper_init(void)
-{
-       int err;
-
-       err = run_selftests(tests, ARRAY_SIZE(tests), NULL);
-
-       return err > 0 ? 0 : err;
-}
+static struct kunit_case drm_plane_helper_test[] = {
+	KUNIT_CASE(igt_check_plane_state),
+	{}
+};
 
-static void __exit test_drm_plane_helper_exit(void)
-{
-}
+static struct kunit_suite drm_plane_helper_test_suite = {
+	.name = "drm_plane_helper_test",
+	.test_cases = drm_plane_helper_test,
+};
 
-module_init(test_drm_plane_helper_init);
-module_exit(test_drm_plane_helper_exit);
+kunit_test_suite(drm_plane_helper_test_suite);
 
 MODULE_AUTHOR("Intel");
 MODULE_LICENSE("GPL");
-- 
2.30.2

Nícolas F. R. A. Prado <n@nfraprado.net>

Details

Message ID: <20211124233835.g7ruxukjruzt5v2y@notapiano>
In-Reply-To: <20211120133847.238264-1-kaspa@onionmail.org> (view parent)
DKIM signature: pass

Download raw message

2 years ago

Hi Anderson,

thank you for the patch.

See comments below.

On Sat, Nov 20, 2021 at 10:38:47PM +0900, Anderson Fraga wrote:
> test-drm_plane_helper.c is a unit test for plane_helper.c to check the
> support of primary plane implementation on top of the normal CRTC
> configuration interface. It also checks plane updates in the switch
> process to the atomic helper infrastructure.

I think you don't need to explain what this test is about, since the change here
is only about the framework used.

> 
> Convert test-drm_plane_helper.c to KUnit is a welcomed advance as relies
> on a lightweight testing framework unit.

So maybe you could expand more on this. What are the reasons to use KUnit?
Tips: to use a unified testing framework in the kernel, and make it easier to
integrate in CI systems (maybe the KUnit documentation could give more reasons
as well).

You could also mention how you tested this patch. Don't need to be very long,
maybe just mention the command used.

> 
> Co-developed-by: Djakson C. G. Filho <djakson.filho@gmail.com>
> Signed-off-by: Djakson C. G. Filho <djakson.filho@gmail.com>
> Signed-off-by: Anderson Fraga <kaspa@onionmail.org>
> ---
>  drivers/gpu/drm/Kconfig                       |  13 ++
>  drivers/gpu/drm/Makefile                      |   2 +-
>  drivers/gpu/drm/selftests/Makefile            |   5 +-
>  .../gpu/drm/selftests/test-drm_plane_helper.c | 134 ++++++++----------
>  4 files changed, 76 insertions(+), 78 deletions(-)
>  mode change 100644 => 100755 drivers/gpu/drm/Kconfig
>  mode change 100644 => 100755 drivers/gpu/drm/Makefile
>  mode change 100644 => 100755 drivers/gpu/drm/selftests/test-drm_plane_helper.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> old mode 100644
> new mode 100755
> index cea777ae7fb9..e0c8fc6d95c9
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -79,6 +79,19 @@ config DRM_DEBUG_SELFTEST
>  
>  	  If in doubt, say "N".
>  
> +config DRM_PLANE_HELPER_KUNIT_TEST
> +	tristate "KUnit test for DRM plane helper"
> +    depends on DRM && KUNIT
> +	select DRM_KMS_HELPER
> +	default KUNIT_ALL_TESTS
> +	help
> +	  This option provides KUnit modules that can be used to run
> +	  various selftests on parts of the DRM plane helper API. This option is not
> +	  useful for distributions or general kernels, but only for kernel
> +	  developers working on DRM and associated drivers.
> +
> +	  If in doubt, say "N".
> +
>  config DRM_KMS_HELPER
>  	tristate
>  	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> old mode 100644
> new mode 100755
> index ad1112154898..e632c43c9b7d
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -58,7 +58,7 @@ drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
>  drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
>  
>  obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-y += selftests/
>  
>  obj-$(CONFIG_DRM)	+= drm.o
>  obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
> diff --git a/drivers/gpu/drm/selftests/Makefile b/drivers/gpu/drm/selftests/Makefile
> index 484c35c7edd0..bb92454a8532 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_cmdline_parser.o \
>  				    test-drm_format.o test-drm_framebuffer.o test-drm_rect.o \
> -				    test-drm_plane_helper.o test-drm_dp_mst_helper.o \
> -				    test-drm_damage_helper.o 
> +				    test-drm_dp_mst_helper.o test-drm_damage_helper.o 
> +
> +obj-$(CONFIG_DRM_PLANE_HELPER_KUNIT_TEST) += test-drm_plane_helper.o 
> diff --git a/drivers/gpu/drm/selftests/test-drm_plane_helper.c b/drivers/gpu/drm/selftests/test-drm_plane_helper.c
> old mode 100644
> new mode 100755
> index a08e8c3e9cd2..522b76e19205
> --- a/drivers/gpu/drm/selftests/test-drm_plane_helper.c
> +++ b/drivers/gpu/drm/selftests/test-drm_plane_helper.c
> @@ -3,28 +3,14 @@
>   * Test cases for the drm_plane_helper functions
>   */
>  
> -#define pr_fmt(fmt) "drm_plane_helper: " fmt
> -
> -#include <linux/module.h>
> +#include <kunit/test.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_modes.h>
>  
> -#include "test-drm_modeset_common.h"
> -
> -#include "drm_selftest.h"
> -struct drm_selftest tests[] = {
> -       {
> -               .enabled = true,
> -               .name = "drm_igt_check_plane_state",
> -               .func = igt_check_plane_state,
> -       },
> -};
> -#include "drm_selftest.c"
> -
>  static void set_src(struct drm_plane_state *plane_state,
> -		    unsigned src_x, unsigned src_y,
> -		    unsigned src_w, unsigned src_h)
> +		    unsigned int src_x, unsigned int src_y,
> +		    unsigned int src_w, unsigned int src_h)

It is indeed an improvement to add the missing type to the variables ('int'),
but you should do this as a separate commit. Either before or after this one. So
you'd have a series with two commits, one that converts to KUnit and another
that adds these missing 'int'.

It's always better to have each commit do a single thing, since it makes review
easier.

>  {
>  	plane_state->src_x = src_x;
>  	plane_state->src_y = src_y;
> @@ -33,8 +19,8 @@ static void set_src(struct drm_plane_state *plane_state,
>  }
>  
>  static bool check_src_eq(struct drm_plane_state *plane_state,
> -			 unsigned src_x, unsigned src_y,
> -			 unsigned src_w, unsigned src_h)
> +			 unsigned int src_x, unsigned int src_y,
> +			 unsigned int src_w, unsigned int src_h)
>  {
>  	if (plane_state->src.x1 < 0) {
>  		pr_err("src x coordinate %x should never be below 0.\n", plane_state->src.x1);
> @@ -60,7 +46,7 @@ static bool check_src_eq(struct drm_plane_state *plane_state,
>  
>  static void set_crtc(struct drm_plane_state *plane_state,
>  		     int crtc_x, int crtc_y,
> -		     unsigned crtc_w, unsigned crtc_h)
> +		     unsigned int crtc_w, unsigned int crtc_h)
>  {
>  	plane_state->crtc_x = crtc_x;
>  	plane_state->crtc_y = crtc_y;
> @@ -70,7 +56,7 @@ static void set_crtc(struct drm_plane_state *plane_state,
>  
>  static bool check_crtc_eq(struct drm_plane_state *plane_state,
>  			  int crtc_x, int crtc_y,
> -			  unsigned crtc_w, unsigned crtc_h)
> +			  unsigned int crtc_w, unsigned int crtc_h)
>  {
>  	if (plane_state->dst.x1 != crtc_x ||
>  	    plane_state->dst.y1 != crtc_y ||
> @@ -84,7 +70,7 @@ static bool check_crtc_eq(struct drm_plane_state *plane_state,
>  	return true;
>  }
>  
> -int igt_check_plane_state(void *ignored)
> +static void igt_check_plane_state(struct kunit *test)
>  {
>  	int ret;
>  
> @@ -115,10 +101,10 @@ int igt_check_plane_state(void *ignored)
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  false, false);
> -	FAIL(ret < 0, "Simple clipping check should pass\n");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);

You should use the _MSG variant to keep the message:

	KUNIT_EXPECT_TRUE_MSG(test, ret >= 0, "Simple clipping check should pass\n");

Same for other messages.

> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 1024 << 16, 768 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>  
>  	/* Rotated clipping + reflection, no scaling. */
>  	plane_state.rotation = DRM_MODE_ROTATE_90 | DRM_MODE_REFLECT_X;
> @@ -126,10 +112,10 @@ int igt_check_plane_state(void *ignored)
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  false, false);
> -	FAIL(ret < 0, "Rotated clipping check should pass\n");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 768 << 16, 1024 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>  	plane_state.rotation = DRM_MODE_ROTATE_0;
>  
>  	/* Check whether positioning works correctly. */
> @@ -139,16 +125,16 @@ int igt_check_plane_state(void *ignored)
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  false, false);
> -	FAIL(!ret, "Should not be able to position on the crtc with can_position=false\n");
> +	KUNIT_EXPECT_TRUE(test, ret);
>  
>  	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  true, false);
> -	FAIL(ret < 0, "Simple positioning should work\n");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1023, 767));
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 1023 << 16, 767 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1023, 767));
>  
>  	/* Simple scaling tests. */
>  	set_src(&plane_state, 0, 0, 512 << 16, 384 << 16);
> @@ -157,28 +143,29 @@ int igt_check_plane_state(void *ignored)
>  						  0x8001,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  false, false);
> -	FAIL(!ret, "Upscaling out of range should fail.\n");
> +	KUNIT_EXPECT_TRUE(test, ret);
>  	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>  						  0x8000,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  false, false);
> -	FAIL(ret < 0, "Upscaling exactly 2x should work\n");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 512 << 16, 384 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>  
>  	set_src(&plane_state, 0, 0, 2048 << 16, 1536 << 16);
>  	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  0x1ffff, false, false);
> -	FAIL(!ret, "Downscaling out of range should fail.\n");
> +
> +	KUNIT_EXPECT_TRUE(test, ret);
>  	ret = drm_atomic_helper_check_plane_state(&plane_state, &crtc_state,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  0x20000, false, false);
> -	FAIL(ret < 0, "Should succeed with exact scaling limit\n");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 2048 << 16, 1536 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>  
>  	/* Testing rounding errors. */
>  	set_src(&plane_state, 0, 0, 0x40001, 0x40001);
> @@ -187,10 +174,11 @@ int igt_check_plane_state(void *ignored)
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  0x10001,
>  						  true, false);
> -	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
> +

No need to add extra blank lines.

> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 1022, 766, 2, 2));
>  
>  	set_src(&plane_state, 0x20001, 0x20001, 0x4040001, 0x3040001);
>  	set_crtc(&plane_state, -2, -2, 1028, 772);
> @@ -198,10 +186,10 @@ int igt_check_plane_state(void *ignored)
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  0x10001,
>  						  false, false);
> -	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0x40002, 0x40002, 1024 << 16, 768 << 16));

It's usually better to keep lines wrapped at 80 characters and to align them
like so:

	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0x40002, 0x40002,
					     1024 << 16, 768 << 16));

Running checkpatch.pl on the commit/file should point this out. But do mind that
you should use common-sense with checkpatch. It gives some false-positives, so
don't believe everything it says. If you have doubts, reach out :).

Also the 80 characters limit is not written in stone. Sometimes it makes sense
to keep the line longer if it's clearer to read. There is no "right answer", so
use your own judgement (in the end it's the maintainer that will decide how to
it is merged).

Thanks,
Nícolas

> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>  
>  	set_src(&plane_state, 0, 0, 0x3ffff, 0x3ffff);
>  	set_crtc(&plane_state, 1022, 766, 4, 4);
> @@ -209,11 +197,12 @@ int igt_check_plane_state(void *ignored)
>  						  0xffff,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  true, false);
> -	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> -	FAIL_ON(!plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +
>  	/* Should not be rounded to 0x20001, which would be upscaling. */
> -	FAIL_ON(!check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 1022, 766, 2, 2));
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0, 0, 2 << 16, 2 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 1022, 766, 2, 2));
>  
>  	set_src(&plane_state, 0x1ffff, 0x1ffff, 0x403ffff, 0x303ffff);
>  	set_crtc(&plane_state, -2, -2, 1028, 772);
> @@ -221,29 +210,24 @@ int igt_check_plane_state(void *ignored)
>  						  0xffff,
>  						  DRM_PLANE_HELPER_NO_SCALING,
>  						  false, false);
> -	FAIL(ret < 0, "Should succeed by clipping to exact multiple");
> -	FAIL_ON(!plane_state.visible);
> -	FAIL_ON(!check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
> -	FAIL_ON(!check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>  
> -	return 0;
> +	KUNIT_EXPECT_TRUE(test, ret >= 0);
> +	KUNIT_EXPECT_TRUE(test, plane_state.visible);
> +	KUNIT_EXPECT_TRUE(test, check_src_eq(&plane_state, 0x3fffe, 0x3fffe, 1024 << 16, 768 << 16));
> +	KUNIT_EXPECT_TRUE(test, check_crtc_eq(&plane_state, 0, 0, 1024, 768));
>  }
>  
> -static int __init test_drm_plane_helper_init(void)
> -{
> -       int err;
> -
> -       err = run_selftests(tests, ARRAY_SIZE(tests), NULL);
> -
> -       return err > 0 ? 0 : err;
> -}
> +static struct kunit_case drm_plane_helper_test[] = {
> +	KUNIT_CASE(igt_check_plane_state),
> +	{}
> +};
>  
> -static void __exit test_drm_plane_helper_exit(void)
> -{
> -}
> +static struct kunit_suite drm_plane_helper_test_suite = {
> +	.name = "drm_plane_helper_test",
> +	.test_cases = drm_plane_helper_test,
> +};
>  
> -module_init(test_drm_plane_helper_init);
> -module_exit(test_drm_plane_helper_exit);
> +kunit_test_suite(drm_plane_helper_test_suite);
>  
>  MODULE_AUTHOR("Intel");
>  MODULE_LICENSE("GPL");
> -- 
> 2.30.2
>

~lkcamp/patches

[PATCH] drm: selftest: convert drm_plane_helper selftest to KUnit