[PATCH v4 0/3] KUnit tests for drm_format_helper

[PATCH v4 0/3] KUnit tests for drm_format_helper

[José Expósito]

Hello everyone,

Following the style used in the selftest to KUnit series [1] and the AMD
series [2], the tests were moved to the "tests" folder.
In addition, to be consistent naming functions, I renamed the
kunit_suite and the test cases to use underscores as suggested in [3].

It is not clear yet whether we want to have one or multiple Kconfig
symbols and select which test should be built. However, refactoring from
one approach to the other is quite simple, so I think we should be fine
choosing the simpler option now and refactoring if required.

Thanks a lot,
José Expósito

[1] https://lore.kernel.org/dri-devel/20220615135824.15522-1-maira.canal@usp.br/T/
[2] https://lore.kernel.org/dri-devel/20220608010709.272962-1-maira.canal@usp.br/
[3] https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html

RFC -> v1: https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/

 - Add .kunitconfig (Maxime Ripard)
 - Fix memory leak (Daniel Latypov)
 - Make config option generic (Javier Martinez Canillas):
   DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
 - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)

v1 -> v2: https://lore.kernel.org/dri-devel/20220606095516.938934-1-jose.exposito89@gmail.com/T/

 Thomas Zimmermann:
 - Add DRM_RECT_INIT() macro
 - Move tests to drivers/gpu/drm/kunit
 - Improve test documentation

v2 -> v3: https://lore.kernel.org/dri-devel/20220612161248.271590-1-jose.exposito89@gmail.com/T/

 - Use designated initializer in DRM_RECT_INIT (Jani Nikula)
 - Simplify the "conversion_buf_size" helper

v3 -> v4: https://lore.kernel.org/dri-devel/20220616183852.GA12343@elementary/T/

 - Move the source to the "tests" folder
 - Use "_" in kunit_suite and cases:
   https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html
 - Reviewed-by and Acked-by tags

José Expósito (3):
  drm/rect: Add DRM_RECT_INIT() macro
  drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()
  drm/doc: Add KUnit documentation

 Documentation/gpu/drm-internals.rst           |  32 ++++
 drivers/gpu/drm/Kconfig                       |  16 ++
 drivers/gpu/drm/Makefile                      |   1 +
 drivers/gpu/drm/tests/.kunitconfig            |   3 +
 drivers/gpu/drm/tests/Makefile                |   3 +
 .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
 include/drm/drm_rect.h                        |  16 ++
 7 files changed, 232 insertions(+)
 create mode 100644 drivers/gpu/drm/tests/.kunitconfig
 create mode 100644 drivers/gpu/drm/tests/Makefile
 create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c

-- 
2.25.1

[PATCH v4 0/3] KUnit tests for drm_format_helper

[José Expósito]

Hello everyone,

Following the style used in the selftest to KUnit series [1] and the AMD
series [2], the tests were moved to the "tests" folder.
In addition, to be consistent naming functions, I renamed the
kunit_suite and the test cases to use underscores as suggested in [3].

It is not clear yet whether we want to have one or multiple Kconfig
symbols and select which test should be built. However, refactoring from
one approach to the other is quite simple, so I think we should be fine
choosing the simpler option now and refactoring if required.

Thanks a lot,
José Expósito

[1] https://lore.kernel.org/dri-devel/20220615135824.15522-1-maira.canal@usp.br/T/
[2] https://lore.kernel.org/dri-devel/20220608010709.272962-1-maira.canal@usp.br/
[3] https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html

RFC -> v1: https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/

 - Add .kunitconfig (Maxime Ripard)
 - Fix memory leak (Daniel Latypov)
 - Make config option generic (Javier Martinez Canillas):
   DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
 - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)

v1 -> v2: https://lore.kernel.org/dri-devel/20220606095516.938934-1-jose.exposito89@gmail.com/T/

 Thomas Zimmermann:
 - Add DRM_RECT_INIT() macro
 - Move tests to drivers/gpu/drm/kunit
 - Improve test documentation

v2 -> v3: https://lore.kernel.org/dri-devel/20220612161248.271590-1-jose.exposito89@gmail.com/T/

 - Use designated initializer in DRM_RECT_INIT (Jani Nikula)
 - Simplify the "conversion_buf_size" helper

v3 -> v4: https://lore.kernel.org/dri-devel/20220616183852.GA12343@elementary/T/

 - Move the source to the "tests" folder
 - Use "_" in kunit_suite and cases:
   https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html
 - Reviewed-by and Acked-by tags

José Expósito (3):
  drm/rect: Add DRM_RECT_INIT() macro
  drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()
  drm/doc: Add KUnit documentation

 Documentation/gpu/drm-internals.rst           |  32 ++++
 drivers/gpu/drm/Kconfig                       |  16 ++
 drivers/gpu/drm/Makefile                      |   1 +
 drivers/gpu/drm/tests/.kunitconfig            |   3 +
 drivers/gpu/drm/tests/Makefile                |   3 +
 .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
 include/drm/drm_rect.h                        |  16 ++
 7 files changed, 232 insertions(+)
 create mode 100644 drivers/gpu/drm/tests/.kunitconfig
 create mode 100644 drivers/gpu/drm/tests/Makefile
 create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c

-- 
2.25.1

[PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[José Expósito]

Add a helper macro to initialize a rectangle from x, y, width and
height information.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 include/drm/drm_rect.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 6f6e19bd4dac..e8d94fca2703 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -47,6 +47,22 @@ struct drm_rect {
 	int x1, y1, x2, y2;
 };
 
+/**
+ * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
+ * @x: x coordinate
+ * @y: y coordinate
+ * @w: width
+ * @h: height
+ *
+ * RETURNS:
+ * A new rectangle of the specified size.
+ */
+#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
+		.x1 = (x), \
+		.y1 = (y), \
+		.x2 = (x) + (w), \
+		.y2 = (y) + (h) })
+
 /**
  * DRM_RECT_FMT - printf string for &struct drm_rect
  */
-- 
2.25.1

[PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[José Expósito]

Add a helper macro to initialize a rectangle from x, y, width and
height information.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 include/drm/drm_rect.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 6f6e19bd4dac..e8d94fca2703 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -47,6 +47,22 @@ struct drm_rect {
 	int x1, y1, x2, y2;
 };
 
+/**
+ * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
+ * @x: x coordinate
+ * @y: y coordinate
+ * @w: width
+ * @h: height
+ *
+ * RETURNS:
+ * A new rectangle of the specified size.
+ */
+#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
+		.x1 = (x), \
+		.y1 = (y), \
+		.x2 = (x) + (w), \
+		.y2 = (y) + (h) })
+
 /**
  * DRM_RECT_FMT - printf string for &struct drm_rect
  */
-- 
2.25.1

[PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Test the conversion from XRGB8888 to RGB332.

What is tested?

 - Different values for the X in XRGB8888 to make sure it is ignored
 - Different clip values: Single pixel and full and partial buffer
 - Well known colors: White, black, red, green, blue, magenta, yellow
   and cyan
 - Other colors: Randomly picked
 - Destination pitch

How to run the tests?

 $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
         --kconfig_add CONFIG_VIRTIO_UML=y \
         --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y

Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/Kconfig                       |  16 ++
 drivers/gpu/drm/Makefile                      |   1 +
 drivers/gpu/drm/tests/.kunitconfig            |   3 +
 drivers/gpu/drm/tests/Makefile                |   3 +
 .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
 5 files changed, 184 insertions(+)
 create mode 100644 drivers/gpu/drm/tests/.kunitconfig
 create mode 100644 drivers/gpu/drm/tests/Makefile
 create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 22e7fa48d693..6c2256e8474b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
 
 	  If in doubt, say "N".
 
+config DRM_KUNIT_TEST
+	tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
+	depends on DRM && KUNIT=y
+	select DRM_KMS_HELPER
+	default KUNIT_ALL_TESTS
+	help
+	  This builds unit tests for DRM. This option is not useful for
+	  distributions or general kernels, but only for kernel
+	  developers working on DRM and associated drivers.
+
+	  For more information on KUnit and unit tests in general,
+	  please refer to the KUnit documentation in
+	  Documentation/dev-tools/kunit/.
+
+	  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
index 13ef240b3d2b..db8ffcf4e048 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 #
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
 
 obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
diff --git a/drivers/gpu/drm/tests/.kunitconfig b/drivers/gpu/drm/tests/.kunitconfig
new file mode 100644
index 000000000000..6ec04b4c979d
--- /dev/null
+++ b/drivers/gpu/drm/tests/.kunitconfig
@@ -0,0 +1,3 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_KUNIT_TEST=y
diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
new file mode 100644
index 000000000000..2c8273796d9d
--- /dev/null
+++ b/drivers/gpu/drm/tests/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
new file mode 100644
index 000000000000..98583bf56044
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <kunit/test.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_file.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_mode.h>
+#include <drm/drm_print.h>
+#include <drm/drm_rect.h>
+
+#include "../drm_crtc_internal.h"
+
+#define TEST_BUF_SIZE 50
+
+struct xrgb8888_to_rgb332_case {
+	const char *name;
+	unsigned int pitch;
+	unsigned int dst_pitch;
+	struct drm_rect clip;
+	const u32 xrgb8888[TEST_BUF_SIZE];
+	const u8 expected[4 * TEST_BUF_SIZE];
+};
+
+static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
+	{
+		.name = "single_pixel_source_buffer",
+		.pitch = 1 * 4,
+		.dst_pitch = 0,
+		.clip = DRM_RECT_INIT(0, 0, 1, 1),
+		.xrgb8888 = { 0x01FF0000 },
+		.expected = { 0xE0 },
+	},
+	{
+		.name = "single_pixel_clip_rectangle",
+		.pitch = 2 * 4,
+		.dst_pitch = 0,
+		.clip = DRM_RECT_INIT(1, 1, 1, 1),
+		.xrgb8888 = {
+			0x00000000, 0x00000000,
+			0x00000000, 0x10FF0000,
+		},
+		.expected = { 0xE0 },
+	},
+	{
+		/* Well known colors: White, black, red, green, blue, magenta,
+		 * yellow and cyan. Different values for the X in XRGB8888 to
+		 * make sure it is ignored. Partial clip area.
+		 */
+		.name = "well_known_colors",
+		.pitch = 4 * 4,
+		.dst_pitch = 0,
+		.clip = DRM_RECT_INIT(1, 1, 2, 4),
+		.xrgb8888 = {
+			0x00000000, 0x00000000, 0x00000000, 0x00000000,
+			0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000,
+			0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000,
+			0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
+			0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
+		},
+		.expected = {
+			0xFF, 0x00,
+			0xE0, 0x1C,
+			0x03, 0xE3,
+			0xFC, 0x1F,
+		},
+	},
+	{
+		/* Randomly picked colors. Full buffer within the clip area. */
+		.name = "destination_pitch",
+		.pitch = 3 * 4,
+		.dst_pitch = 5,
+		.clip = DRM_RECT_INIT(0, 0, 3, 3),
+		.xrgb8888 = {
+			0xA10E449C, 0xB1114D05, 0xC1A80303,
+			0xD16C7073, 0xA20E449C, 0xB2114D05,
+			0xC2A80303, 0xD26C7073, 0xA30E449C,
+		},
+		.expected = {
+			0x0A, 0x08, 0xA0, 0x00, 0x00,
+			0x6D, 0x0A, 0x08, 0x00, 0x00,
+			0xA0, 0x6D, 0x0A, 0x00, 0x00,
+		},
+	},
+};
+
+/*
+ * conversion_buf_size - Return the destination buffer size required to convert
+ * between formats.
+ * @dst_format: destination buffer pixel format (DRM_FORMAT_*)
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @clip: Clip rectangle area to convert
+ *
+ * Returns:
+ * The size of the destination buffer or negative value on error.
+ */
+static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
+				  const struct drm_rect *clip)
+{
+	const struct drm_format_info *dst_fi = drm_format_info(dst_format);
+
+	if (!dst_fi)
+		return -EINVAL;
+
+	if (!dst_pitch)
+		dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
+
+	return dst_pitch * drm_rect_height(clip);
+}
+
+static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
+					 char *desc)
+{
+	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
+		  xrgb8888_to_rgb332_case_desc);
+
+static void xrgb8888_to_rgb332_test(struct kunit *test)
+{
+	const struct xrgb8888_to_rgb332_case *params = test->param_value;
+	size_t dst_size;
+	__u8 *dst = NULL;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
+				       &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
+
+	drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
+				  &fb, &params->clip);
+	KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
+}
+
+static struct kunit_case drm_format_helper_test_cases[] = {
+	KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
+			 xrgb8888_to_rgb332_gen_params),
+	{}
+};
+
+static struct kunit_suite drm_format_helper_test_suite = {
+	.name = "drm_format_helper_test",
+	.test_cases = drm_format_helper_test_cases,
+};
+
+kunit_test_suite(drm_format_helper_test_suite);
+
+MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
-- 
2.25.1

[PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Test the conversion from XRGB8888 to RGB332.

What is tested?

 - Different values for the X in XRGB8888 to make sure it is ignored
 - Different clip values: Single pixel and full and partial buffer
 - Well known colors: White, black, red, green, blue, magenta, yellow
   and cyan
 - Other colors: Randomly picked
 - Destination pitch

How to run the tests?

 $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
         --kconfig_add CONFIG_VIRTIO_UML=y \
         --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y

Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/Kconfig                       |  16 ++
 drivers/gpu/drm/Makefile                      |   1 +
 drivers/gpu/drm/tests/.kunitconfig            |   3 +
 drivers/gpu/drm/tests/Makefile                |   3 +
 .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
 5 files changed, 184 insertions(+)
 create mode 100644 drivers/gpu/drm/tests/.kunitconfig
 create mode 100644 drivers/gpu/drm/tests/Makefile
 create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 22e7fa48d693..6c2256e8474b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
 
 	  If in doubt, say "N".
 
+config DRM_KUNIT_TEST
+	tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
+	depends on DRM && KUNIT=y
+	select DRM_KMS_HELPER
+	default KUNIT_ALL_TESTS
+	help
+	  This builds unit tests for DRM. This option is not useful for
+	  distributions or general kernels, but only for kernel
+	  developers working on DRM and associated drivers.
+
+	  For more information on KUnit and unit tests in general,
+	  please refer to the KUnit documentation in
+	  Documentation/dev-tools/kunit/.
+
+	  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
index 13ef240b3d2b..db8ffcf4e048 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 #
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
 
 obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
diff --git a/drivers/gpu/drm/tests/.kunitconfig b/drivers/gpu/drm/tests/.kunitconfig
new file mode 100644
index 000000000000..6ec04b4c979d
--- /dev/null
+++ b/drivers/gpu/drm/tests/.kunitconfig
@@ -0,0 +1,3 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_KUNIT_TEST=y
diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
new file mode 100644
index 000000000000..2c8273796d9d
--- /dev/null
+++ b/drivers/gpu/drm/tests/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
new file mode 100644
index 000000000000..98583bf56044
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <kunit/test.h>
+
+#include <drm/drm_device.h>
+#include <drm/drm_file.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_mode.h>
+#include <drm/drm_print.h>
+#include <drm/drm_rect.h>
+
+#include "../drm_crtc_internal.h"
+
+#define TEST_BUF_SIZE 50
+
+struct xrgb8888_to_rgb332_case {
+	const char *name;
+	unsigned int pitch;
+	unsigned int dst_pitch;
+	struct drm_rect clip;
+	const u32 xrgb8888[TEST_BUF_SIZE];
+	const u8 expected[4 * TEST_BUF_SIZE];
+};
+
+static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
+	{
+		.name = "single_pixel_source_buffer",
+		.pitch = 1 * 4,
+		.dst_pitch = 0,
+		.clip = DRM_RECT_INIT(0, 0, 1, 1),
+		.xrgb8888 = { 0x01FF0000 },
+		.expected = { 0xE0 },
+	},
+	{
+		.name = "single_pixel_clip_rectangle",
+		.pitch = 2 * 4,
+		.dst_pitch = 0,
+		.clip = DRM_RECT_INIT(1, 1, 1, 1),
+		.xrgb8888 = {
+			0x00000000, 0x00000000,
+			0x00000000, 0x10FF0000,
+		},
+		.expected = { 0xE0 },
+	},
+	{
+		/* Well known colors: White, black, red, green, blue, magenta,
+		 * yellow and cyan. Different values for the X in XRGB8888 to
+		 * make sure it is ignored. Partial clip area.
+		 */
+		.name = "well_known_colors",
+		.pitch = 4 * 4,
+		.dst_pitch = 0,
+		.clip = DRM_RECT_INIT(1, 1, 2, 4),
+		.xrgb8888 = {
+			0x00000000, 0x00000000, 0x00000000, 0x00000000,
+			0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000,
+			0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000,
+			0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
+			0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
+		},
+		.expected = {
+			0xFF, 0x00,
+			0xE0, 0x1C,
+			0x03, 0xE3,
+			0xFC, 0x1F,
+		},
+	},
+	{
+		/* Randomly picked colors. Full buffer within the clip area. */
+		.name = "destination_pitch",
+		.pitch = 3 * 4,
+		.dst_pitch = 5,
+		.clip = DRM_RECT_INIT(0, 0, 3, 3),
+		.xrgb8888 = {
+			0xA10E449C, 0xB1114D05, 0xC1A80303,
+			0xD16C7073, 0xA20E449C, 0xB2114D05,
+			0xC2A80303, 0xD26C7073, 0xA30E449C,
+		},
+		.expected = {
+			0x0A, 0x08, 0xA0, 0x00, 0x00,
+			0x6D, 0x0A, 0x08, 0x00, 0x00,
+			0xA0, 0x6D, 0x0A, 0x00, 0x00,
+		},
+	},
+};
+
+/*
+ * conversion_buf_size - Return the destination buffer size required to convert
+ * between formats.
+ * @dst_format: destination buffer pixel format (DRM_FORMAT_*)
+ * @dst_pitch: Number of bytes between two consecutive scanlines within dst
+ * @clip: Clip rectangle area to convert
+ *
+ * Returns:
+ * The size of the destination buffer or negative value on error.
+ */
+static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
+				  const struct drm_rect *clip)
+{
+	const struct drm_format_info *dst_fi = drm_format_info(dst_format);
+
+	if (!dst_fi)
+		return -EINVAL;
+
+	if (!dst_pitch)
+		dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
+
+	return dst_pitch * drm_rect_height(clip);
+}
+
+static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
+					 char *desc)
+{
+	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
+		  xrgb8888_to_rgb332_case_desc);
+
+static void xrgb8888_to_rgb332_test(struct kunit *test)
+{
+	const struct xrgb8888_to_rgb332_case *params = test->param_value;
+	size_t dst_size;
+	__u8 *dst = NULL;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
+				       &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
+
+	drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
+				  &fb, &params->clip);
+	KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
+}
+
+static struct kunit_case drm_format_helper_test_cases[] = {
+	KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
+			 xrgb8888_to_rgb332_gen_params),
+	{}
+};
+
+static struct kunit_suite drm_format_helper_test_suite = {
+	.name = "drm_format_helper_test",
+	.test_cases = drm_format_helper_test_cases,
+};
+
+kunit_test_suite(drm_format_helper_test_suite);
+
+MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
-- 
2.25.1

[PATCH v4 3/3] drm/doc: Add KUnit documentation

[José Expósito]

Explain how to run the KUnit tests present in the DRM subsystem and
clarify why the UML-only options were not added to the configuration
file present in drivers/gpu/drm/.kunitconfig [1] [2].

[1] https://lore.kernel.org/dri-devel/CABVgOSn8i=LO5p7830h2XU1Jgg0KrN0qTnxkOMhf1oTgxjaKKw@mail.gmail.com/
[2] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=T7ydk-dYcEw@mail.gmail.com/

Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 Documentation/gpu/drm-internals.rst | 32 +++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index 38afed24a75c..5fd20a306718 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -207,6 +207,38 @@ Utilities
    :internal:
 
 
+Unit testing
+============
+
+KUnit
+-----
+
+KUnit (Kernel unit testing framework) provides a common framework for unit tests
+within the Linux kernel.
+
+This section covers the specifics for the DRM subsystem. For general information
+about KUnit, please refer to Documentation/dev-tools/kunit/start.rst.
+
+How to run the tests?
+~~~~~~~~~~~~~~~~~~~~~
+
+In order to facilitate running the test suite, a configuration file is present
+in ``drivers/gpu/drm/tests/.kunitconfig``. It can be used by ``kunit.py`` as
+follows:
+
+.. code-block:: bash
+
+	$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
+		--kconfig_add CONFIG_VIRTIO_UML=y \
+		--kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
+
+.. note::
+	The configuration included in ``.kunitconfig`` should be as generic as
+	possible.
+	``CONFIG_VIRTIO_UML`` and ``CONFIG_UML_PCI_OVER_VIRTIO`` are not
+	included in it because they are only required for User Mode Linux.
+
+
 Legacy Support Code
 ===================
 
-- 
2.25.1

[PATCH v4 3/3] drm/doc: Add KUnit documentation

[José Expósito]

Explain how to run the KUnit tests present in the DRM subsystem and
clarify why the UML-only options were not added to the configuration
file present in drivers/gpu/drm/.kunitconfig [1] [2].

[1] https://lore.kernel.org/dri-devel/CABVgOSn8i=LO5p7830h2XU1Jgg0KrN0qTnxkOMhf1oTgxjaKKw@mail.gmail.com/
[2] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=T7ydk-dYcEw@mail.gmail.com/

Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 Documentation/gpu/drm-internals.rst | 32 +++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index 38afed24a75c..5fd20a306718 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -207,6 +207,38 @@ Utilities
    :internal:
 
 
+Unit testing
+============
+
+KUnit
+-----
+
+KUnit (Kernel unit testing framework) provides a common framework for unit tests
+within the Linux kernel.
+
+This section covers the specifics for the DRM subsystem. For general information
+about KUnit, please refer to Documentation/dev-tools/kunit/start.rst.
+
+How to run the tests?
+~~~~~~~~~~~~~~~~~~~~~
+
+In order to facilitate running the test suite, a configuration file is present
+in ``drivers/gpu/drm/tests/.kunitconfig``. It can be used by ``kunit.py`` as
+follows:
+
+.. code-block:: bash
+
+	$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
+		--kconfig_add CONFIG_VIRTIO_UML=y \
+		--kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
+
+.. note::
+	The configuration included in ``.kunitconfig`` should be as generic as
+	possible.
+	``CONFIG_VIRTIO_UML`` and ``CONFIG_UML_PCI_OVER_VIRTIO`` are not
+	included in it because they are only required for User Mode Linux.
+
+
 Legacy Support Code
 ===================
 
-- 
2.25.1

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

On Tue, Jun 21, 2022 at 12:06 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Add a helper macro to initialize a rectangle from x, y, width and
> height information.
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

This looks good to me, though I have one minor concern about the macro
name. (But if it's okay with the DRM folks, which it seems to be, I
won't object.)

Either way,
Reviewed-by: David Gow <davidgow@google.com>

>  include/drm/drm_rect.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index 6f6e19bd4dac..e8d94fca2703 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -47,6 +47,22 @@ struct drm_rect {
>         int x1, y1, x2, y2;
>  };
>
> +/**
> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
> + * @x: x coordinate
> + * @y: y coordinate
> + * @w: width
> + * @h: height
> + *
> + * RETURNS:
> + * A new rectangle of the specified size.
> + */
> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
> +               .x1 = (x), \
> +               .y1 = (y), \
> +               .x2 = (x) + (w), \
> +               .y2 = (y) + (h) })
> +

My only slight concern here is that it might be a little bit confusing
that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
actual struct drm_rect is x1/y1/x2/y2. If the macro were called
something like DRM_RECT_INIT_FROM_XYWH() or similar.


>  /**
>   * DRM_RECT_FMT - printf string for &struct drm_rect
>   */
> --
> 2.25.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1708 bytes --]

On Tue, Jun 21, 2022 at 12:06 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Add a helper macro to initialize a rectangle from x, y, width and
> height information.
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

This looks good to me, though I have one minor concern about the macro
name. (But if it's okay with the DRM folks, which it seems to be, I
won't object.)

Either way,
Reviewed-by: David Gow <davidgow@google.com>

>  include/drm/drm_rect.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
> index 6f6e19bd4dac..e8d94fca2703 100644
> --- a/include/drm/drm_rect.h
> +++ b/include/drm/drm_rect.h
> @@ -47,6 +47,22 @@ struct drm_rect {
>         int x1, y1, x2, y2;
>  };
>
> +/**
> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
> + * @x: x coordinate
> + * @y: y coordinate
> + * @w: width
> + * @h: height
> + *
> + * RETURNS:
> + * A new rectangle of the specified size.
> + */
> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
> +               .x1 = (x), \
> +               .y1 = (y), \
> +               .x2 = (x) + (w), \
> +               .y2 = (y) + (h) })
> +

My only slight concern here is that it might be a little bit confusing
that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
actual struct drm_rect is x1/y1/x2/y2. If the macro were called
something like DRM_RECT_INIT_FROM_XYWH() or similar.


>  /**
>   * DRM_RECT_FMT - printf string for &struct drm_rect
>   */
> --
> 2.25.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 11090 bytes --]

On Tue, Jun 21, 2022 at 12:06 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Test the conversion from XRGB8888 to RGB332.
>
> What is tested?
>
>  - Different values for the X in XRGB8888 to make sure it is ignored
>  - Different clip values: Single pixel and full and partial buffer
>  - Well known colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
>
> How to run the tests?
>
>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
>          --kconfig_add CONFIG_VIRTIO_UML=y \
>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

These tests all pass properly on my system, and look good to me from a
KUnit point of view. Thanks very much.

A couple of small notes below, which you can take or leave as you
wish: they mostly focus on potential future tests.

Regardless,
Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  drivers/gpu/drm/Kconfig                       |  16 ++
>  drivers/gpu/drm/Makefile                      |   1 +
>  drivers/gpu/drm/tests/.kunitconfig            |   3 +
>  drivers/gpu/drm/tests/Makefile                |   3 +
>  .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
>  5 files changed, 184 insertions(+)
>  create mode 100644 drivers/gpu/drm/tests/.kunitconfig
>  create mode 100644 drivers/gpu/drm/tests/Makefile
>  create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 22e7fa48d693..6c2256e8474b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
>
>           If in doubt, say "N".
>
> +config DRM_KUNIT_TEST
> +       tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> +       depends on DRM && KUNIT=y
> +       select DRM_KMS_HELPER
> +       default KUNIT_ALL_TESTS
> +       help
> +         This builds unit tests for DRM. This option is not useful for
> +         distributions or general kernels, but only for kernel
> +         developers working on DRM and associated drivers.
> +
> +         For more information on KUnit and unit tests in general,
> +         please refer to the KUnit documentation in
> +         Documentation/dev-tools/kunit/.
> +
> +         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
> index 13ef240b3d2b..db8ffcf4e048 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>  #
>
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
>
>  obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
>  obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> diff --git a/drivers/gpu/drm/tests/.kunitconfig b/drivers/gpu/drm/tests/.kunitconfig
> new file mode 100644
> index 000000000000..6ec04b4c979d
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/.kunitconfig
> @@ -0,0 +1,3 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_KUNIT_TEST=y
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> new file mode 100644
> index 000000000000..2c8273796d9d
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> new file mode 100644
> index 000000000000..98583bf56044
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_file.h>
> +#include <drm/drm_format_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_mode.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_rect.h>
> +
> +#include "../drm_crtc_internal.h"
> +
> +#define TEST_BUF_SIZE 50
> +
> +struct xrgb8888_to_rgb332_case {
> +       const char *name;
> +       unsigned int pitch;
> +       unsigned int dst_pitch;
> +       struct drm_rect clip;
> +       const u32 xrgb8888[TEST_BUF_SIZE];
> +       const u8 expected[4 * TEST_BUF_SIZE];

Why is this 4*TEST_BUF_SIZE if there are the same number of pixels
(which in rgb332 are 8-bit, not 32-bit) as in xrgb8888. I see there's
a pitch test, which does need some extra memory, but not a full 4
times (less than double, by the looks of things). Having this be 4 *
implies (to me) that the aim is to have the same total memory
available between xrgb8888 and expected, which doesn't seem to need to
be the case. Maybe make this 2 * or similar?

Relatedly, if instead of naming this 'expected', it were named rgb332,
it'd be possible to extend this struct to add other formats expected
values, and test several formats with the same list of test inputs.
(dst_pitch would probably need to become dst_pitch_rgb332 eventually,
too). This is all something which could wait for a later patch, but is
food for thought. I'd love to see an xrgb8888_to_rgb565 test at some
point, too.


> +};
> +
> +static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
> +       {
> +               .name = "single_pixel_source_buffer",
> +               .pitch = 1 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(0, 0, 1, 1),
> +               .xrgb8888 = { 0x01FF0000 },
> +               .expected = { 0xE0 },
> +       },
> +       {
> +               .name = "single_pixel_clip_rectangle",
> +               .pitch = 2 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(1, 1, 1, 1),
> +               .xrgb8888 = {
> +                       0x00000000, 0x00000000,
> +                       0x00000000, 0x10FF0000,
> +               },
> +               .expected = { 0xE0 },
> +       },
> +       {
> +               /* Well known colors: White, black, red, green, blue, magenta,
> +                * yellow and cyan. Different values for the X in XRGB8888 to
> +                * make sure it is ignored. Partial clip area.
> +                */
> +               .name = "well_known_colors",
> +               .pitch = 4 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(1, 1, 2, 4),
> +               .xrgb8888 = {
> +                       0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +                       0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000,
> +                       0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000,
> +                       0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
> +                       0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
> +               },
> +               .expected = {
> +                       0xFF, 0x00,
> +                       0xE0, 0x1C,
> +                       0x03, 0xE3,
> +                       0xFC, 0x1F,
> +               },
> +       },
> +       {
> +               /* Randomly picked colors. Full buffer within the clip area. */
> +               .name = "destination_pitch",
> +               .pitch = 3 * 4,
> +               .dst_pitch = 5,
> +               .clip = DRM_RECT_INIT(0, 0, 3, 3),
> +               .xrgb8888 = {
> +                       0xA10E449C, 0xB1114D05, 0xC1A80303,
> +                       0xD16C7073, 0xA20E449C, 0xB2114D05,
> +                       0xC2A80303, 0xD26C7073, 0xA30E449C,
> +               },
> +               .expected = {
> +                       0x0A, 0x08, 0xA0, 0x00, 0x00,
> +                       0x6D, 0x0A, 0x08, 0x00, 0x00,
> +                       0xA0, 0x6D, 0x0A, 0x00, 0x00,
> +               },
> +       },
> +};
> +
> +/*
> + * conversion_buf_size - Return the destination buffer size required to convert
> + * between formats.
> + * @dst_format: destination buffer pixel format (DRM_FORMAT_*)
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @clip: Clip rectangle area to convert
> + *
> + * Returns:
> + * The size of the destination buffer or negative value on error.
> + */
> +static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
> +                                 const struct drm_rect *clip)
> +{
> +       const struct drm_format_info *dst_fi = drm_format_info(dst_format);
> +
> +       if (!dst_fi)
> +               return -EINVAL;
> +
> +       if (!dst_pitch)
> +               dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
> +
> +       return dst_pitch * drm_rect_height(clip);
> +}
> +
> +static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
> +                                        char *desc)
> +{
> +       strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
> +}
> +
> +KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
> +                 xrgb8888_to_rgb332_case_desc);
> +
> +static void xrgb8888_to_rgb332_test(struct kunit *test)
> +{
> +       const struct xrgb8888_to_rgb332_case *params = test->param_value;
> +       size_t dst_size;
> +       __u8 *dst = NULL;
> +
> +       struct drm_framebuffer fb = {
> +               .format = drm_format_info(DRM_FORMAT_XRGB8888),
> +               .pitches = { params->pitch, 0, 0 },
> +       };
> +
> +       dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
> +                                      &params->clip);
> +       KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> +       dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> +
> +       drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
> +                                 &fb, &params->clip);
> +       KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> +}
> +
> +static struct kunit_case drm_format_helper_test_cases[] = {
> +       KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
> +                        xrgb8888_to_rgb332_gen_params),
> +       {}
> +};
> +
> +static struct kunit_suite drm_format_helper_test_suite = {
> +       .name = "drm_format_helper_test",
> +       .test_cases = drm_format_helper_test_cases,
> +};
> +
> +kunit_test_suite(drm_format_helper_test_suite);
> +
> +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
> --
> 2.25.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 11090 bytes --]

On Tue, Jun 21, 2022 at 12:06 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Test the conversion from XRGB8888 to RGB332.
>
> What is tested?
>
>  - Different values for the X in XRGB8888 to make sure it is ignored
>  - Different clip values: Single pixel and full and partial buffer
>  - Well known colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
>
> How to run the tests?
>
>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
>          --kconfig_add CONFIG_VIRTIO_UML=y \
>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

These tests all pass properly on my system, and look good to me from a
KUnit point of view. Thanks very much.

A couple of small notes below, which you can take or leave as you
wish: they mostly focus on potential future tests.

Regardless,
Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  drivers/gpu/drm/Kconfig                       |  16 ++
>  drivers/gpu/drm/Makefile                      |   1 +
>  drivers/gpu/drm/tests/.kunitconfig            |   3 +
>  drivers/gpu/drm/tests/Makefile                |   3 +
>  .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
>  5 files changed, 184 insertions(+)
>  create mode 100644 drivers/gpu/drm/tests/.kunitconfig
>  create mode 100644 drivers/gpu/drm/tests/Makefile
>  create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 22e7fa48d693..6c2256e8474b 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
>
>           If in doubt, say "N".
>
> +config DRM_KUNIT_TEST
> +       tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> +       depends on DRM && KUNIT=y
> +       select DRM_KMS_HELPER
> +       default KUNIT_ALL_TESTS
> +       help
> +         This builds unit tests for DRM. This option is not useful for
> +         distributions or general kernels, but only for kernel
> +         developers working on DRM and associated drivers.
> +
> +         For more information on KUnit and unit tests in general,
> +         please refer to the KUnit documentation in
> +         Documentation/dev-tools/kunit/.
> +
> +         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
> index 13ef240b3d2b..db8ffcf4e048 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>  #
>
>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
>
>  obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
>  obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> diff --git a/drivers/gpu/drm/tests/.kunitconfig b/drivers/gpu/drm/tests/.kunitconfig
> new file mode 100644
> index 000000000000..6ec04b4c979d
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/.kunitconfig
> @@ -0,0 +1,3 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_KUNIT_TEST=y
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> new file mode 100644
> index 000000000000..2c8273796d9d
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
> diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> new file mode 100644
> index 000000000000..98583bf56044
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_device.h>
> +#include <drm/drm_file.h>
> +#include <drm/drm_format_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_mode.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_rect.h>
> +
> +#include "../drm_crtc_internal.h"
> +
> +#define TEST_BUF_SIZE 50
> +
> +struct xrgb8888_to_rgb332_case {
> +       const char *name;
> +       unsigned int pitch;
> +       unsigned int dst_pitch;
> +       struct drm_rect clip;
> +       const u32 xrgb8888[TEST_BUF_SIZE];
> +       const u8 expected[4 * TEST_BUF_SIZE];

Why is this 4*TEST_BUF_SIZE if there are the same number of pixels
(which in rgb332 are 8-bit, not 32-bit) as in xrgb8888. I see there's
a pitch test, which does need some extra memory, but not a full 4
times (less than double, by the looks of things). Having this be 4 *
implies (to me) that the aim is to have the same total memory
available between xrgb8888 and expected, which doesn't seem to need to
be the case. Maybe make this 2 * or similar?

Relatedly, if instead of naming this 'expected', it were named rgb332,
it'd be possible to extend this struct to add other formats expected
values, and test several formats with the same list of test inputs.
(dst_pitch would probably need to become dst_pitch_rgb332 eventually,
too). This is all something which could wait for a later patch, but is
food for thought. I'd love to see an xrgb8888_to_rgb565 test at some
point, too.


> +};
> +
> +static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
> +       {
> +               .name = "single_pixel_source_buffer",
> +               .pitch = 1 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(0, 0, 1, 1),
> +               .xrgb8888 = { 0x01FF0000 },
> +               .expected = { 0xE0 },
> +       },
> +       {
> +               .name = "single_pixel_clip_rectangle",
> +               .pitch = 2 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(1, 1, 1, 1),
> +               .xrgb8888 = {
> +                       0x00000000, 0x00000000,
> +                       0x00000000, 0x10FF0000,
> +               },
> +               .expected = { 0xE0 },
> +       },
> +       {
> +               /* Well known colors: White, black, red, green, blue, magenta,
> +                * yellow and cyan. Different values for the X in XRGB8888 to
> +                * make sure it is ignored. Partial clip area.
> +                */
> +               .name = "well_known_colors",
> +               .pitch = 4 * 4,
> +               .dst_pitch = 0,
> +               .clip = DRM_RECT_INIT(1, 1, 2, 4),
> +               .xrgb8888 = {
> +                       0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +                       0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000,
> +                       0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000,
> +                       0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
> +                       0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
> +               },
> +               .expected = {
> +                       0xFF, 0x00,
> +                       0xE0, 0x1C,
> +                       0x03, 0xE3,
> +                       0xFC, 0x1F,
> +               },
> +       },
> +       {
> +               /* Randomly picked colors. Full buffer within the clip area. */
> +               .name = "destination_pitch",
> +               .pitch = 3 * 4,
> +               .dst_pitch = 5,
> +               .clip = DRM_RECT_INIT(0, 0, 3, 3),
> +               .xrgb8888 = {
> +                       0xA10E449C, 0xB1114D05, 0xC1A80303,
> +                       0xD16C7073, 0xA20E449C, 0xB2114D05,
> +                       0xC2A80303, 0xD26C7073, 0xA30E449C,
> +               },
> +               .expected = {
> +                       0x0A, 0x08, 0xA0, 0x00, 0x00,
> +                       0x6D, 0x0A, 0x08, 0x00, 0x00,
> +                       0xA0, 0x6D, 0x0A, 0x00, 0x00,
> +               },
> +       },
> +};
> +
> +/*
> + * conversion_buf_size - Return the destination buffer size required to convert
> + * between formats.
> + * @dst_format: destination buffer pixel format (DRM_FORMAT_*)
> + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> + * @clip: Clip rectangle area to convert
> + *
> + * Returns:
> + * The size of the destination buffer or negative value on error.
> + */
> +static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
> +                                 const struct drm_rect *clip)
> +{
> +       const struct drm_format_info *dst_fi = drm_format_info(dst_format);
> +
> +       if (!dst_fi)
> +               return -EINVAL;
> +
> +       if (!dst_pitch)
> +               dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
> +
> +       return dst_pitch * drm_rect_height(clip);
> +}
> +
> +static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
> +                                        char *desc)
> +{
> +       strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
> +}
> +
> +KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
> +                 xrgb8888_to_rgb332_case_desc);
> +
> +static void xrgb8888_to_rgb332_test(struct kunit *test)
> +{
> +       const struct xrgb8888_to_rgb332_case *params = test->param_value;
> +       size_t dst_size;
> +       __u8 *dst = NULL;
> +
> +       struct drm_framebuffer fb = {
> +               .format = drm_format_info(DRM_FORMAT_XRGB8888),
> +               .pitches = { params->pitch, 0, 0 },
> +       };
> +
> +       dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
> +                                      &params->clip);
> +       KUNIT_ASSERT_GT(test, dst_size, 0);
> +
> +       dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> +
> +       drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
> +                                 &fb, &params->clip);
> +       KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> +}
> +
> +static struct kunit_case drm_format_helper_test_cases[] = {
> +       KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
> +                        xrgb8888_to_rgb332_gen_params),
> +       {}
> +};
> +
> +static struct kunit_suite drm_format_helper_test_suite = {
> +       .name = "drm_format_helper_test",
> +       .test_cases = drm_format_helper_test_cases,
> +};
> +
> +kunit_test_suite(drm_format_helper_test_suite);
> +
> +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
> --
> 2.25.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 3/3] drm/doc: Add KUnit documentation

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Tue, Jun 21, 2022 at 12:06 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Explain how to run the KUnit tests present in the DRM subsystem and
> clarify why the UML-only options were not added to the configuration
> file present in drivers/gpu/drm/.kunitconfig [1] [2].
>
> [1] https://lore.kernel.org/dri-devel/CABVgOSn8i=LO5p7830h2XU1Jgg0KrN0qTnxkOMhf1oTgxjaKKw@mail.gmail.com/
> [2] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=T7ydk-dYcEw@mail.gmail.com/
>
> Reviewed-by: Maxime Ripard <maxime@cerno.tech>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

This looks good (and doesn't seem to introduce any 'make htmldocs'
build issues on my machine).

You could also mention that using --arch=x86_64 (or similar) instead
of the UML options is another, equally viable option for running the
tests. That'd make it more obvious how to run on different
architectures: UML, while a good default, is quite different to other
architectures in not having any PCI support out-of-the-box.

(Maybe we should make the --arch=um default config include these
options? Or have um-pci as another architecture. We did decide not to
bother with SMP and x86, though...)

Regardless, this is
Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 3/3] drm/doc: Add KUnit documentation

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Tue, Jun 21, 2022 at 12:06 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> Explain how to run the KUnit tests present in the DRM subsystem and
> clarify why the UML-only options were not added to the configuration
> file present in drivers/gpu/drm/.kunitconfig [1] [2].
>
> [1] https://lore.kernel.org/dri-devel/CABVgOSn8i=LO5p7830h2XU1Jgg0KrN0qTnxkOMhf1oTgxjaKKw@mail.gmail.com/
> [2] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=T7ydk-dYcEw@mail.gmail.com/
>
> Reviewed-by: Maxime Ripard <maxime@cerno.tech>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

This looks good (and doesn't seem to introduce any 'make htmldocs'
build issues on my machine).

You could also mention that using --arch=x86_64 (or similar) instead
of the UML options is another, equally viable option for running the
tests. That'd make it more obvious how to run on different
architectures: UML, while a good default, is quite different to other
architectures in not having any PCI support out-of-the-box.

(Maybe we should make the --arch=um default config include these
options? Or have um-pci as another architecture. We did decide not to
bother with SMP and x86, though...)

Regardless, this is
Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2223 bytes --]

Hi

Am 21.06.22 um 11:38 schrieb David Gow:
> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
> <jose.exposito89@gmail.com> wrote:
>>
>> Add a helper macro to initialize a rectangle from x, y, width and
>> height information.
>>
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>> ---
> 
> This looks good to me, though I have one minor concern about the macro
> name. (But if it's okay with the DRM folks, which it seems to be, I
> won't object.)
> 
> Either way,
> Reviewed-by: David Gow <davidgow@google.com>
> 
>>   include/drm/drm_rect.h | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
>> index 6f6e19bd4dac..e8d94fca2703 100644
>> --- a/include/drm/drm_rect.h
>> +++ b/include/drm/drm_rect.h
>> @@ -47,6 +47,22 @@ struct drm_rect {
>>          int x1, y1, x2, y2;
>>   };
>>
>> +/**
>> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
>> + * @x: x coordinate
>> + * @y: y coordinate
>> + * @w: width
>> + * @h: height
>> + *
>> + * RETURNS:
>> + * A new rectangle of the specified size.
>> + */
>> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
>> +               .x1 = (x), \
>> +               .y1 = (y), \
>> +               .x2 = (x) + (w), \
>> +               .y2 = (y) + (h) })
>> +
> 
> My only slight concern here is that it might be a little bit confusing
> that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
> actual struct drm_rect is x1/y1/x2/y2. If the macro were called
> something like DRM_RECT_INIT_FROM_XYWH() or similar.

The existing drm_rect_init() function uses xywh arguments. So the 
current name is consistent with existing practice. I don't think we 
refer to x2,y2 much, if ever.

Best regards
Thomas

> 
> 
>>   /**
>>    * DRM_RECT_FMT - printf string for &struct drm_rect
>>    */
>> --
>> 2.25.1
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2223 bytes --]

Hi

Am 21.06.22 um 11:38 schrieb David Gow:
> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
> <jose.exposito89@gmail.com> wrote:
>>
>> Add a helper macro to initialize a rectangle from x, y, width and
>> height information.
>>
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>> ---
> 
> This looks good to me, though I have one minor concern about the macro
> name. (But if it's okay with the DRM folks, which it seems to be, I
> won't object.)
> 
> Either way,
> Reviewed-by: David Gow <davidgow@google.com>
> 
>>   include/drm/drm_rect.h | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
>> index 6f6e19bd4dac..e8d94fca2703 100644
>> --- a/include/drm/drm_rect.h
>> +++ b/include/drm/drm_rect.h
>> @@ -47,6 +47,22 @@ struct drm_rect {
>>          int x1, y1, x2, y2;
>>   };
>>
>> +/**
>> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
>> + * @x: x coordinate
>> + * @y: y coordinate
>> + * @w: width
>> + * @h: height
>> + *
>> + * RETURNS:
>> + * A new rectangle of the specified size.
>> + */
>> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
>> +               .x1 = (x), \
>> +               .y1 = (y), \
>> +               .x2 = (x) + (w), \
>> +               .y2 = (y) + (h) })
>> +
> 
> My only slight concern here is that it might be a little bit confusing
> that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
> actual struct drm_rect is x1/y1/x2/y2. If the macro were called
> something like DRM_RECT_INIT_FROM_XYWH() or similar.

The existing drm_rect_init() function uses xywh arguments. So the 
current name is consistent with existing practice. I don't think we 
refer to x2,y2 much, if ever.

Best regards
Thomas

> 
> 
>>   /**
>>    * DRM_RECT_FMT - printf string for &struct drm_rect
>>    */
>> --
>> 2.25.1
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[Jani Nikula]

On Tue, 21 Jun 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 21.06.22 um 11:38 schrieb David Gow:
>> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
>> <jose.exposito89@gmail.com> wrote:
>>>
>>> Add a helper macro to initialize a rectangle from x, y, width and
>>> height information.
>>>
>>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>>> ---
>> 
>> This looks good to me, though I have one minor concern about the macro
>> name. (But if it's okay with the DRM folks, which it seems to be, I
>> won't object.)
>> 
>> Either way,
>> Reviewed-by: David Gow <davidgow@google.com>
>> 
>>>   include/drm/drm_rect.h | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
>>> index 6f6e19bd4dac..e8d94fca2703 100644
>>> --- a/include/drm/drm_rect.h
>>> +++ b/include/drm/drm_rect.h
>>> @@ -47,6 +47,22 @@ struct drm_rect {
>>>          int x1, y1, x2, y2;
>>>   };
>>>
>>> +/**
>>> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
>>> + * @x: x coordinate
>>> + * @y: y coordinate
>>> + * @w: width
>>> + * @h: height
>>> + *
>>> + * RETURNS:
>>> + * A new rectangle of the specified size.
>>> + */
>>> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
>>> +               .x1 = (x), \
>>> +               .y1 = (y), \
>>> +               .x2 = (x) + (w), \
>>> +               .y2 = (y) + (h) })
>>> +
>> 
>> My only slight concern here is that it might be a little bit confusing
>> that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
>> actual struct drm_rect is x1/y1/x2/y2. If the macro were called
>> something like DRM_RECT_INIT_FROM_XYWH() or similar.
>
> The existing drm_rect_init() function uses xywh arguments. So the 
> current name is consistent with existing practice. I don't think we 
> refer to x2,y2 much, if ever.

Agreed, and if we initialized with x1,y1,x2,y2 we wouldn't need the
function/macro in the first place.

BR,
Jani.

>
> Best regards
> Thomas
>
>> 
>> 
>>>   /**
>>>    * DRM_RECT_FMT - printf string for &struct drm_rect
>>>    */
>>> --
>>> 2.25.1
>>>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[Jani Nikula]

On Tue, 21 Jun 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 21.06.22 um 11:38 schrieb David Gow:
>> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
>> <jose.exposito89@gmail.com> wrote:
>>>
>>> Add a helper macro to initialize a rectangle from x, y, width and
>>> height information.
>>>
>>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>>> ---
>> 
>> This looks good to me, though I have one minor concern about the macro
>> name. (But if it's okay with the DRM folks, which it seems to be, I
>> won't object.)
>> 
>> Either way,
>> Reviewed-by: David Gow <davidgow@google.com>
>> 
>>>   include/drm/drm_rect.h | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
>>> index 6f6e19bd4dac..e8d94fca2703 100644
>>> --- a/include/drm/drm_rect.h
>>> +++ b/include/drm/drm_rect.h
>>> @@ -47,6 +47,22 @@ struct drm_rect {
>>>          int x1, y1, x2, y2;
>>>   };
>>>
>>> +/**
>>> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
>>> + * @x: x coordinate
>>> + * @y: y coordinate
>>> + * @w: width
>>> + * @h: height
>>> + *
>>> + * RETURNS:
>>> + * A new rectangle of the specified size.
>>> + */
>>> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
>>> +               .x1 = (x), \
>>> +               .y1 = (y), \
>>> +               .x2 = (x) + (w), \
>>> +               .y2 = (y) + (h) })
>>> +
>> 
>> My only slight concern here is that it might be a little bit confusing
>> that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
>> actual struct drm_rect is x1/y1/x2/y2. If the macro were called
>> something like DRM_RECT_INIT_FROM_XYWH() or similar.
>
> The existing drm_rect_init() function uses xywh arguments. So the 
> current name is consistent with existing practice. I don't think we 
> refer to x2,y2 much, if ever.

Agreed, and if we initialized with x1,y1,x2,y2 we wouldn't need the
function/macro in the first place.

BR,
Jani.

>
> Best regards
> Thomas
>
>> 
>> 
>>>   /**
>>>    * DRM_RECT_FMT - printf string for &struct drm_rect
>>>    */
>>> --
>>> 2.25.1
>>>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hi David,

On Tue, Jun 21, 2022 at 05:38:33PM +0800, David Gow wrote:
> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
> <jose.exposito89@gmail.com> wrote:
> >
> > Test the conversion from XRGB8888 to RGB332.
> >
> > What is tested?
> >
> >  - Different values for the X in XRGB8888 to make sure it is ignored
> >  - Different clip values: Single pixel and full and partial buffer
> >  - Well known colors: White, black, red, green, blue, magenta, yellow
> >    and cyan
> >  - Other colors: Randomly picked
> >  - Destination pitch
> >
> > How to run the tests?
> >
> >  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
> >          --kconfig_add CONFIG_VIRTIO_UML=y \
> >          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> 
> These tests all pass properly on my system, and look good to me from a
> KUnit point of view. Thanks very much.
> 
> A couple of small notes below, which you can take or leave as you
> wish: they mostly focus on potential future tests.
> 
> Regardless,
> Reviewed-by: David Gow <davidgow@google.com>

Thanks a lot for your review :)
 
> Cheers,
> -- David
> 
> >  drivers/gpu/drm/Kconfig                       |  16 ++
> >  drivers/gpu/drm/Makefile                      |   1 +
> >  drivers/gpu/drm/tests/.kunitconfig            |   3 +
> >  drivers/gpu/drm/tests/Makefile                |   3 +
> >  .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
> >  5 files changed, 184 insertions(+)
> >  create mode 100644 drivers/gpu/drm/tests/.kunitconfig
> >  create mode 100644 drivers/gpu/drm/tests/Makefile
> >  create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 22e7fa48d693..6c2256e8474b 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
> >
> >           If in doubt, say "N".
> >
> > +config DRM_KUNIT_TEST
> > +       tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> > +       depends on DRM && KUNIT=y
> > +       select DRM_KMS_HELPER
> > +       default KUNIT_ALL_TESTS
> > +       help
> > +         This builds unit tests for DRM. This option is not useful for
> > +         distributions or general kernels, but only for kernel
> > +         developers working on DRM and associated drivers.
> > +
> > +         For more information on KUnit and unit tests in general,
> > +         please refer to the KUnit documentation in
> > +         Documentation/dev-tools/kunit/.
> > +
> > +         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
> > index 13ef240b3d2b..db8ffcf4e048 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >  #
> >
> >  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> > +obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
> >
> >  obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
> >  obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> > diff --git a/drivers/gpu/drm/tests/.kunitconfig b/drivers/gpu/drm/tests/.kunitconfig
> > new file mode 100644
> > index 000000000000..6ec04b4c979d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tests/.kunitconfig
> > @@ -0,0 +1,3 @@
> > +CONFIG_KUNIT=y
> > +CONFIG_DRM=y
> > +CONFIG_DRM_KUNIT_TEST=y
> > diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> > new file mode 100644
> > index 000000000000..2c8273796d9d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tests/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
> > diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> > new file mode 100644
> > index 000000000000..98583bf56044
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <kunit/test.h>
> > +
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_file.h>
> > +#include <drm/drm_format_helper.h>
> > +#include <drm/drm_fourcc.h>
> > +#include <drm/drm_framebuffer.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_mode.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_rect.h>
> > +
> > +#include "../drm_crtc_internal.h"
> > +
> > +#define TEST_BUF_SIZE 50
> > +
> > +struct xrgb8888_to_rgb332_case {
> > +       const char *name;
> > +       unsigned int pitch;
> > +       unsigned int dst_pitch;
> > +       struct drm_rect clip;
> > +       const u32 xrgb8888[TEST_BUF_SIZE];
> > +       const u8 expected[4 * TEST_BUF_SIZE];
> 
> Why is this 4*TEST_BUF_SIZE if there are the same number of pixels
> (which in rgb332 are 8-bit, not 32-bit) as in xrgb8888. I see there's
> a pitch test, which does need some extra memory, but not a full 4
> times (less than double, by the looks of things). Having this be 4 *
> implies (to me) that the aim is to have the same total memory
> available between xrgb8888 and expected, which doesn't seem to need to
> be the case. Maybe make this 2 * or similar?

To be honest, TEST_BUF_SIZE length is quite arbitrary. I chose a
number big enough so large formats, like RGB565, fit in the buffer
without changing the constant in future patches unnecessarily.
I added some extra space so other possible test cases don't need to
change it if testing with larger input.

The *4 multiplier is there so both buffers have the same size.

> Relatedly, if instead of naming this 'expected', it were named rgb332,
> it'd be possible to extend this struct to add other formats expected
> values, and test several formats with the same list of test inputs.
> (dst_pitch would probably need to become dst_pitch_rgb332 eventually,
> too). This is all something which could wait for a later patch, but is
> food for thought. I'd love to see an xrgb8888_to_rgb565 test at some
> point, too.

The patches for RGB565, including the small refactors you mentioned
plus the avility to swap or not bytes are already available in my
tree [1] (last 4 patches, in case you want to look at them).

They are waiting to be rebased once this series is merged.

As you pointed out, some minor refactoring is required and you are
right about the destination pitch. Hopefully, the end result is simple
and easy to follow despite the refactor patches.

Thanks again for your review,
Jose

[1] https://github.com/JoseExposito/linux/commits/patch-drm-format-helper-rgb565-kunit

> > +};
> > +
> > +static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
> > +       {
> > +               .name = "single_pixel_source_buffer",
> > +               .pitch = 1 * 4,
> > +               .dst_pitch = 0,
> > +               .clip = DRM_RECT_INIT(0, 0, 1, 1),
> > +               .xrgb8888 = { 0x01FF0000 },
> > +               .expected = { 0xE0 },
> > +       },
> > +       {
> > +               .name = "single_pixel_clip_rectangle",
> > +               .pitch = 2 * 4,
> > +               .dst_pitch = 0,
> > +               .clip = DRM_RECT_INIT(1, 1, 1, 1),
> > +               .xrgb8888 = {
> > +                       0x00000000, 0x00000000,
> > +                       0x00000000, 0x10FF0000,
> > +               },
> > +               .expected = { 0xE0 },
> > +       },
> > +       {
> > +               /* Well known colors: White, black, red, green, blue, magenta,
> > +                * yellow and cyan. Different values for the X in XRGB8888 to
> > +                * make sure it is ignored. Partial clip area.
> > +                */
> > +               .name = "well_known_colors",
> > +               .pitch = 4 * 4,
> > +               .dst_pitch = 0,
> > +               .clip = DRM_RECT_INIT(1, 1, 2, 4),
> > +               .xrgb8888 = {
> > +                       0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +                       0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000,
> > +                       0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000,
> > +                       0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
> > +                       0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
> > +               },
> > +               .expected = {
> > +                       0xFF, 0x00,
> > +                       0xE0, 0x1C,
> > +                       0x03, 0xE3,
> > +                       0xFC, 0x1F,
> > +               },
> > +       },
> > +       {
> > +               /* Randomly picked colors. Full buffer within the clip area. */
> > +               .name = "destination_pitch",
> > +               .pitch = 3 * 4,
> > +               .dst_pitch = 5,
> > +               .clip = DRM_RECT_INIT(0, 0, 3, 3),
> > +               .xrgb8888 = {
> > +                       0xA10E449C, 0xB1114D05, 0xC1A80303,
> > +                       0xD16C7073, 0xA20E449C, 0xB2114D05,
> > +                       0xC2A80303, 0xD26C7073, 0xA30E449C,
> > +               },
> > +               .expected = {
> > +                       0x0A, 0x08, 0xA0, 0x00, 0x00,
> > +                       0x6D, 0x0A, 0x08, 0x00, 0x00,
> > +                       0xA0, 0x6D, 0x0A, 0x00, 0x00,
> > +               },
> > +       },
> > +};
> > +
> > +/*
> > + * conversion_buf_size - Return the destination buffer size required to convert
> > + * between formats.
> > + * @dst_format: destination buffer pixel format (DRM_FORMAT_*)
> > + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> > + * @clip: Clip rectangle area to convert
> > + *
> > + * Returns:
> > + * The size of the destination buffer or negative value on error.
> > + */
> > +static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
> > +                                 const struct drm_rect *clip)
> > +{
> > +       const struct drm_format_info *dst_fi = drm_format_info(dst_format);
> > +
> > +       if (!dst_fi)
> > +               return -EINVAL;
> > +
> > +       if (!dst_pitch)
> > +               dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
> > +
> > +       return dst_pitch * drm_rect_height(clip);
> > +}
> > +
> > +static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
> > +                                        char *desc)
> > +{
> > +       strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
> > +}
> > +
> > +KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
> > +                 xrgb8888_to_rgb332_case_desc);
> > +
> > +static void xrgb8888_to_rgb332_test(struct kunit *test)
> > +{
> > +       const struct xrgb8888_to_rgb332_case *params = test->param_value;
> > +       size_t dst_size;
> > +       __u8 *dst = NULL;
> > +
> > +       struct drm_framebuffer fb = {
> > +               .format = drm_format_info(DRM_FORMAT_XRGB8888),
> > +               .pitches = { params->pitch, 0, 0 },
> > +       };
> > +
> > +       dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
> > +                                      &params->clip);
> > +       KUNIT_ASSERT_GT(test, dst_size, 0);
> > +
> > +       dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> > +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> > +
> > +       drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
> > +                                 &fb, &params->clip);
> > +       KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> > +}
> > +
> > +static struct kunit_case drm_format_helper_test_cases[] = {
> > +       KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
> > +                        xrgb8888_to_rgb332_gen_params),
> > +       {}
> > +};
> > +
> > +static struct kunit_suite drm_format_helper_test_suite = {
> > +       .name = "drm_format_helper_test",
> > +       .test_cases = drm_format_helper_test_cases,
> > +};
> > +
> > +kunit_test_suite(drm_format_helper_test_suite);
> > +
> > +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
> > --
> > 2.25.1
> >

Re: [PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hi David,

On Tue, Jun 21, 2022 at 05:38:33PM +0800, David Gow wrote:
> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
> <jose.exposito89@gmail.com> wrote:
> >
> > Test the conversion from XRGB8888 to RGB332.
> >
> > What is tested?
> >
> >  - Different values for the X in XRGB8888 to make sure it is ignored
> >  - Different clip values: Single pixel and full and partial buffer
> >  - Well known colors: White, black, red, green, blue, magenta, yellow
> >    and cyan
> >  - Other colors: Randomly picked
> >  - Destination pitch
> >
> > How to run the tests?
> >
> >  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
> >          --kconfig_add CONFIG_VIRTIO_UML=y \
> >          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> 
> These tests all pass properly on my system, and look good to me from a
> KUnit point of view. Thanks very much.
> 
> A couple of small notes below, which you can take or leave as you
> wish: they mostly focus on potential future tests.
> 
> Regardless,
> Reviewed-by: David Gow <davidgow@google.com>

Thanks a lot for your review :)
 
> Cheers,
> -- David
> 
> >  drivers/gpu/drm/Kconfig                       |  16 ++
> >  drivers/gpu/drm/Makefile                      |   1 +
> >  drivers/gpu/drm/tests/.kunitconfig            |   3 +
> >  drivers/gpu/drm/tests/Makefile                |   3 +
> >  .../gpu/drm/tests/drm_format_helper_test.c    | 161 ++++++++++++++++++
> >  5 files changed, 184 insertions(+)
> >  create mode 100644 drivers/gpu/drm/tests/.kunitconfig
> >  create mode 100644 drivers/gpu/drm/tests/Makefile
> >  create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 22e7fa48d693..6c2256e8474b 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
> >
> >           If in doubt, say "N".
> >
> > +config DRM_KUNIT_TEST
> > +       tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> > +       depends on DRM && KUNIT=y
> > +       select DRM_KMS_HELPER
> > +       default KUNIT_ALL_TESTS
> > +       help
> > +         This builds unit tests for DRM. This option is not useful for
> > +         distributions or general kernels, but only for kernel
> > +         developers working on DRM and associated drivers.
> > +
> > +         For more information on KUnit and unit tests in general,
> > +         please refer to the KUnit documentation in
> > +         Documentation/dev-tools/kunit/.
> > +
> > +         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
> > index 13ef240b3d2b..db8ffcf4e048 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >  #
> >
> >  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> > +obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
> >
> >  obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
> >  obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> > diff --git a/drivers/gpu/drm/tests/.kunitconfig b/drivers/gpu/drm/tests/.kunitconfig
> > new file mode 100644
> > index 000000000000..6ec04b4c979d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tests/.kunitconfig
> > @@ -0,0 +1,3 @@
> > +CONFIG_KUNIT=y
> > +CONFIG_DRM=y
> > +CONFIG_DRM_KUNIT_TEST=y
> > diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> > new file mode 100644
> > index 000000000000..2c8273796d9d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tests/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
> > diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
> > new file mode 100644
> > index 000000000000..98583bf56044
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
> > @@ -0,0 +1,161 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <kunit/test.h>
> > +
> > +#include <drm/drm_device.h>
> > +#include <drm/drm_file.h>
> > +#include <drm/drm_format_helper.h>
> > +#include <drm/drm_fourcc.h>
> > +#include <drm/drm_framebuffer.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +#include <drm/drm_mode.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_rect.h>
> > +
> > +#include "../drm_crtc_internal.h"
> > +
> > +#define TEST_BUF_SIZE 50
> > +
> > +struct xrgb8888_to_rgb332_case {
> > +       const char *name;
> > +       unsigned int pitch;
> > +       unsigned int dst_pitch;
> > +       struct drm_rect clip;
> > +       const u32 xrgb8888[TEST_BUF_SIZE];
> > +       const u8 expected[4 * TEST_BUF_SIZE];
> 
> Why is this 4*TEST_BUF_SIZE if there are the same number of pixels
> (which in rgb332 are 8-bit, not 32-bit) as in xrgb8888. I see there's
> a pitch test, which does need some extra memory, but not a full 4
> times (less than double, by the looks of things). Having this be 4 *
> implies (to me) that the aim is to have the same total memory
> available between xrgb8888 and expected, which doesn't seem to need to
> be the case. Maybe make this 2 * or similar?

To be honest, TEST_BUF_SIZE length is quite arbitrary. I chose a
number big enough so large formats, like RGB565, fit in the buffer
without changing the constant in future patches unnecessarily.
I added some extra space so other possible test cases don't need to
change it if testing with larger input.

The *4 multiplier is there so both buffers have the same size.

> Relatedly, if instead of naming this 'expected', it were named rgb332,
> it'd be possible to extend this struct to add other formats expected
> values, and test several formats with the same list of test inputs.
> (dst_pitch would probably need to become dst_pitch_rgb332 eventually,
> too). This is all something which could wait for a later patch, but is
> food for thought. I'd love to see an xrgb8888_to_rgb565 test at some
> point, too.

The patches for RGB565, including the small refactors you mentioned
plus the avility to swap or not bytes are already available in my
tree [1] (last 4 patches, in case you want to look at them).

They are waiting to be rebased once this series is merged.

As you pointed out, some minor refactoring is required and you are
right about the destination pitch. Hopefully, the end result is simple
and easy to follow despite the refactor patches.

Thanks again for your review,
Jose

[1] https://github.com/JoseExposito/linux/commits/patch-drm-format-helper-rgb565-kunit

> > +};
> > +
> > +static struct xrgb8888_to_rgb332_case xrgb8888_to_rgb332_cases[] = {
> > +       {
> > +               .name = "single_pixel_source_buffer",
> > +               .pitch = 1 * 4,
> > +               .dst_pitch = 0,
> > +               .clip = DRM_RECT_INIT(0, 0, 1, 1),
> > +               .xrgb8888 = { 0x01FF0000 },
> > +               .expected = { 0xE0 },
> > +       },
> > +       {
> > +               .name = "single_pixel_clip_rectangle",
> > +               .pitch = 2 * 4,
> > +               .dst_pitch = 0,
> > +               .clip = DRM_RECT_INIT(1, 1, 1, 1),
> > +               .xrgb8888 = {
> > +                       0x00000000, 0x00000000,
> > +                       0x00000000, 0x10FF0000,
> > +               },
> > +               .expected = { 0xE0 },
> > +       },
> > +       {
> > +               /* Well known colors: White, black, red, green, blue, magenta,
> > +                * yellow and cyan. Different values for the X in XRGB8888 to
> > +                * make sure it is ignored. Partial clip area.
> > +                */
> > +               .name = "well_known_colors",
> > +               .pitch = 4 * 4,
> > +               .dst_pitch = 0,
> > +               .clip = DRM_RECT_INIT(1, 1, 2, 4),
> > +               .xrgb8888 = {
> > +                       0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +                       0x00000000, 0x11FFFFFF, 0x22000000, 0x00000000,
> > +                       0x00000000, 0x33FF0000, 0x4400FF00, 0x00000000,
> > +                       0x00000000, 0x550000FF, 0x66FF00FF, 0x00000000,
> > +                       0x00000000, 0x77FFFF00, 0x8800FFFF, 0x00000000,
> > +               },
> > +               .expected = {
> > +                       0xFF, 0x00,
> > +                       0xE0, 0x1C,
> > +                       0x03, 0xE3,
> > +                       0xFC, 0x1F,
> > +               },
> > +       },
> > +       {
> > +               /* Randomly picked colors. Full buffer within the clip area. */
> > +               .name = "destination_pitch",
> > +               .pitch = 3 * 4,
> > +               .dst_pitch = 5,
> > +               .clip = DRM_RECT_INIT(0, 0, 3, 3),
> > +               .xrgb8888 = {
> > +                       0xA10E449C, 0xB1114D05, 0xC1A80303,
> > +                       0xD16C7073, 0xA20E449C, 0xB2114D05,
> > +                       0xC2A80303, 0xD26C7073, 0xA30E449C,
> > +               },
> > +               .expected = {
> > +                       0x0A, 0x08, 0xA0, 0x00, 0x00,
> > +                       0x6D, 0x0A, 0x08, 0x00, 0x00,
> > +                       0xA0, 0x6D, 0x0A, 0x00, 0x00,
> > +               },
> > +       },
> > +};
> > +
> > +/*
> > + * conversion_buf_size - Return the destination buffer size required to convert
> > + * between formats.
> > + * @dst_format: destination buffer pixel format (DRM_FORMAT_*)
> > + * @dst_pitch: Number of bytes between two consecutive scanlines within dst
> > + * @clip: Clip rectangle area to convert
> > + *
> > + * Returns:
> > + * The size of the destination buffer or negative value on error.
> > + */
> > +static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
> > +                                 const struct drm_rect *clip)
> > +{
> > +       const struct drm_format_info *dst_fi = drm_format_info(dst_format);
> > +
> > +       if (!dst_fi)
> > +               return -EINVAL;
> > +
> > +       if (!dst_pitch)
> > +               dst_pitch = drm_rect_width(clip) * dst_fi->cpp[0];
> > +
> > +       return dst_pitch * drm_rect_height(clip);
> > +}
> > +
> > +static void xrgb8888_to_rgb332_case_desc(struct xrgb8888_to_rgb332_case *t,
> > +                                        char *desc)
> > +{
> > +       strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
> > +}
> > +
> > +KUNIT_ARRAY_PARAM(xrgb8888_to_rgb332, xrgb8888_to_rgb332_cases,
> > +                 xrgb8888_to_rgb332_case_desc);
> > +
> > +static void xrgb8888_to_rgb332_test(struct kunit *test)
> > +{
> > +       const struct xrgb8888_to_rgb332_case *params = test->param_value;
> > +       size_t dst_size;
> > +       __u8 *dst = NULL;
> > +
> > +       struct drm_framebuffer fb = {
> > +               .format = drm_format_info(DRM_FORMAT_XRGB8888),
> > +               .pitches = { params->pitch, 0, 0 },
> > +       };
> > +
> > +       dst_size = conversion_buf_size(DRM_FORMAT_RGB332, params->dst_pitch,
> > +                                      &params->clip);
> > +       KUNIT_ASSERT_GT(test, dst_size, 0);
> > +
> > +       dst = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> > +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> > +
> > +       drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
> > +                                 &fb, &params->clip);
> > +       KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> > +}
> > +
> > +static struct kunit_case drm_format_helper_test_cases[] = {
> > +       KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
> > +                        xrgb8888_to_rgb332_gen_params),
> > +       {}
> > +};
> > +
> > +static struct kunit_suite drm_format_helper_test_suite = {
> > +       .name = "drm_format_helper_test",
> > +       .test_cases = drm_format_helper_test_cases,
> > +};
> > +
> > +kunit_test_suite(drm_format_helper_test_suite);
> > +
> > +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
> > --
> > 2.25.1
> >

Re: [PATCH v4 3/3] drm/doc: Add KUnit documentation

[José Expósito]

Hi David,

On Tue, Jun 21, 2022 at 05:38:38PM +0800, David Gow wrote:
> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
> <jose.exposito89@gmail.com> wrote:
> >
> > Explain how to run the KUnit tests present in the DRM subsystem and
> > clarify why the UML-only options were not added to the configuration
> > file present in drivers/gpu/drm/.kunitconfig [1] [2].
> >
> > [1] https://lore.kernel.org/dri-devel/CABVgOSn8i=LO5p7830h2XU1Jgg0KrN0qTnxkOMhf1oTgxjaKKw@mail.gmail.com/
> > [2] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=T7ydk-dYcEw@mail.gmail.com/
> >
> > Reviewed-by: Maxime Ripard <maxime@cerno.tech>
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> 
> This looks good (and doesn't seem to introduce any 'make htmldocs'
> build issues on my machine).
> 
> You could also mention that using --arch=x86_64 (or similar) instead
> of the UML options is another, equally viable option for running the
> tests. That'd make it more obvious how to run on different
> architectures: UML, while a good default, is quite different to other
> architectures in not having any PCI support out-of-the-box.
> 
> (Maybe we should make the --arch=um default config include these
> options? Or have um-pci as another architecture. We did decide not to
> bother with SMP and x86, though...)

Javier suggested the same:
https://lore.kernel.org/dri-devel/20220614180952.GA7067@elementary/

I prefer to keep the docs as simple as possible and link the KUnit
docs for more information. However, you both have way more experience 
than me and agree on the topic, so I'll be happy to include it in v5
if you think it is a good idea.
 
> Regardless, this is
> Reviewed-by: David Gow <davidgow@google.com>

Thanks for reviewing and building the docs, appreciate it.
Jose

> Cheers,
> -- David

Re: [PATCH v4 3/3] drm/doc: Add KUnit documentation

[José Expósito]

Hi David,

On Tue, Jun 21, 2022 at 05:38:38PM +0800, David Gow wrote:
> On Tue, Jun 21, 2022 at 12:06 AM José Expósito
> <jose.exposito89@gmail.com> wrote:
> >
> > Explain how to run the KUnit tests present in the DRM subsystem and
> > clarify why the UML-only options were not added to the configuration
> > file present in drivers/gpu/drm/.kunitconfig [1] [2].
> >
> > [1] https://lore.kernel.org/dri-devel/CABVgOSn8i=LO5p7830h2XU1Jgg0KrN0qTnxkOMhf1oTgxjaKKw@mail.gmail.com/
> > [2] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=T7ydk-dYcEw@mail.gmail.com/
> >
> > Reviewed-by: Maxime Ripard <maxime@cerno.tech>
> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> 
> This looks good (and doesn't seem to introduce any 'make htmldocs'
> build issues on my machine).
> 
> You could also mention that using --arch=x86_64 (or similar) instead
> of the UML options is another, equally viable option for running the
> tests. That'd make it more obvious how to run on different
> architectures: UML, while a good default, is quite different to other
> architectures in not having any PCI support out-of-the-box.
> 
> (Maybe we should make the --arch=um default config include these
> options? Or have um-pci as another architecture. We did decide not to
> bother with SMP and x86, though...)

Javier suggested the same:
https://lore.kernel.org/dri-devel/20220614180952.GA7067@elementary/

I prefer to keep the docs as simple as possible and link the KUnit
docs for more information. However, you both have way more experience 
than me and agree on the topic, so I'll be happy to include it in v5
if you think it is a good idea.
 
> Regardless, this is
> Reviewed-by: David Gow <davidgow@google.com>

Thanks for reviewing and building the docs, appreciate it.
Jose

> Cheers,
> -- David

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

On Tue, Jun 21, 2022 at 6:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 21.06.22 um 11:38 schrieb David Gow:
> > On Tue, Jun 21, 2022 at 12:06 AM José Expósito
 [ ... ]
> >> +/**
> >> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
> >> + * @x: x coordinate
> >> + * @y: y coordinate
> >> + * @w: width
> >> + * @h: height
> >> + *
> >> + * RETURNS:
> >> + * A new rectangle of the specified size.
> >> + */
> >> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
> >> +               .x1 = (x), \
> >> +               .y1 = (y), \
> >> +               .x2 = (x) + (w), \
> >> +               .y2 = (y) + (h) })
> >> +
> >
> > My only slight concern here is that it might be a little bit confusing
> > that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
> > actual struct drm_rect is x1/y1/x2/y2. If the macro were called
> > something like DRM_RECT_INIT_FROM_XYWH() or similar.
>
> The existing drm_rect_init() function uses xywh arguments. So the
> current name is consistent with existing practice. I don't think we
> refer to x2,y2 much, if ever.
>

Ah, fair enough. I wasn't aware of the existing drm_rect_init()
function, so this makes sense as-is.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

On Tue, Jun 21, 2022 at 6:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 21.06.22 um 11:38 schrieb David Gow:
> > On Tue, Jun 21, 2022 at 12:06 AM José Expósito
 [ ... ]
> >> +/**
> >> + * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
> >> + * @x: x coordinate
> >> + * @y: y coordinate
> >> + * @w: width
> >> + * @h: height
> >> + *
> >> + * RETURNS:
> >> + * A new rectangle of the specified size.
> >> + */
> >> +#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
> >> +               .x1 = (x), \
> >> +               .y1 = (y), \
> >> +               .x2 = (x) + (w), \
> >> +               .y2 = (y) + (h) })
> >> +
> >
> > My only slight concern here is that it might be a little bit confusing
> > that a macro called DRM_RECT_INIT() accepts x/y/w/h, whereas the
> > actual struct drm_rect is x1/y1/x2/y2. If the macro were called
> > something like DRM_RECT_INIT_FROM_XYWH() or similar.
>
> The existing drm_rect_init() function uses xywh arguments. So the
> current name is consistent with existing practice. I don't think we
> refer to x2,y2 much, if ever.
>

Ah, fair enough. I wasn't aware of the existing drm_rect_init()
function, so this makes sense as-is.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]