[RFC PATCH 0/1] KUnit tests for drm_format_helper

[RFC PATCH 0/1] KUnit tests for drm_format_helper

[José Expósito]

Hello everyone,

Recently Javier added a new task in the ToDo list [1] to create KUnit
tests for the functions present in "drm_format_helper".

This patch, marked as RFC to start the conversation, includes tests for
"drm_fb_xrgb8888_to_rgb332".

Since the conversion functions present in "drm_format_helper" convert
from XRGB8888 to another format, my plan is to create a set of XRGB8888
data to test all the available conversions.

To illustrate it with an example, the code present in this patch:

	{
		.name = "Single pixel source",
		.pitch = 1 * 4,
		.dst_pitch = 0,
		.clip = CLIP(0, 0, 1, 1),
		.xrgb8888 = { 0x01FF0000 },
		.expected = { 0xE0 },
	}

In a follow up patch, should look like:

	{
		.name = "Single pixel source",
		.pitch = 1 * 4,
		.clip = CLIP(0, 0, 1, 1),
		.xrgb8888 = { 0x01FF0000 },
		.expected = {
			{
				.dst_format = DRM_FORMAT_RGB332,
				.dst_pitch = 0,
				.conv_func = drm_fb_xrgb8888_to_rgb332,
				.result = { 0xE0 },
			},
			{ ... },
		},
	}

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 know colors: White, black, red, green, blue, magenta, yellow
   and cyan
 - Other colors: Randomly picked
 - Destination pitch

What is NOT tested?

 - NULL destination buffer: Because there is not validation in place
 - Out of bounds destination buffer: The size of the buffer is not
   checked. The conversion function could cause out of bound errors
 - Out of bounds source buffer: Similary to the destination buffer, the
   source buffer is read without checking its size
 - Invalid clip values: Because there is no clip validation.
   Example: clip out of or bigger than the source buffer
 - Invalid destination pitch: A dst_pitch < (clip->width * dst_pixsize)
   could cause issues.
 - "Big" source buffers. I don't know if this kind of tests could be of
   interest

How to run the tests?

 My .kunitconfig:

 	CONFIG_KUNIT=y
 	CONFIG_DRM=y
 	CONFIG_DRM_FORMAR_HELPER_TEST=y

 $ ./tools/testing/kunit/kunit.py run --arch=x86_64

Feedback?

 It'd be great to know your ideas about what else we could test, if you
 think that we should make the functions safer by checking the buffers
 and clip sizes or anything else.

Thanks a lot,
José Expósito

[1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=596c35b1440e

José Expósito (1):
  drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

 drivers/gpu/drm/Kconfig                  |  12 ++
 drivers/gpu/drm/Makefile                 |   3 +
 drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_format_helper_test.c

-- 
2.25.1

[RFC PATCH 0/1] KUnit tests for drm_format_helper

[José Expósito]

Hello everyone,

Recently Javier added a new task in the ToDo list [1] to create KUnit
tests for the functions present in "drm_format_helper".

This patch, marked as RFC to start the conversation, includes tests for
"drm_fb_xrgb8888_to_rgb332".

Since the conversion functions present in "drm_format_helper" convert
from XRGB8888 to another format, my plan is to create a set of XRGB8888
data to test all the available conversions.

To illustrate it with an example, the code present in this patch:

	{
		.name = "Single pixel source",
		.pitch = 1 * 4,
		.dst_pitch = 0,
		.clip = CLIP(0, 0, 1, 1),
		.xrgb8888 = { 0x01FF0000 },
		.expected = { 0xE0 },
	}

In a follow up patch, should look like:

	{
		.name = "Single pixel source",
		.pitch = 1 * 4,
		.clip = CLIP(0, 0, 1, 1),
		.xrgb8888 = { 0x01FF0000 },
		.expected = {
			{
				.dst_format = DRM_FORMAT_RGB332,
				.dst_pitch = 0,
				.conv_func = drm_fb_xrgb8888_to_rgb332,
				.result = { 0xE0 },
			},
			{ ... },
		},
	}

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 know colors: White, black, red, green, blue, magenta, yellow
   and cyan
 - Other colors: Randomly picked
 - Destination pitch

What is NOT tested?

 - NULL destination buffer: Because there is not validation in place
 - Out of bounds destination buffer: The size of the buffer is not
   checked. The conversion function could cause out of bound errors
 - Out of bounds source buffer: Similary to the destination buffer, the
   source buffer is read without checking its size
 - Invalid clip values: Because there is no clip validation.
   Example: clip out of or bigger than the source buffer
 - Invalid destination pitch: A dst_pitch < (clip->width * dst_pixsize)
   could cause issues.
 - "Big" source buffers. I don't know if this kind of tests could be of
   interest

How to run the tests?

 My .kunitconfig:

 	CONFIG_KUNIT=y
 	CONFIG_DRM=y
 	CONFIG_DRM_FORMAR_HELPER_TEST=y

 $ ./tools/testing/kunit/kunit.py run --arch=x86_64

Feedback?

 It'd be great to know your ideas about what else we could test, if you
 think that we should make the functions safer by checking the buffers
 and clip sizes or anything else.

Thanks a lot,
José Expósito

[1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=596c35b1440e

José Expósito (1):
  drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

 drivers/gpu/drm/Kconfig                  |  12 ++
 drivers/gpu/drm/Makefile                 |   3 +
 drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_format_helper_test.c

-- 
2.25.1

[RFC PATCH 1/1] 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 know colors: White, black, red, green, blue, magenta, yellow
   and cyan
 - Other colors: Randomly picked
 - Destination pitch

Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/Kconfig                  |  12 ++
 drivers/gpu/drm/Makefile                 |   3 +
 drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_format_helper_test.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e88c497fa010..d92be6faef15 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -76,6 +76,18 @@ config DRM_KMS_HELPER
 	help
 	  CRTC helpers for KMS drivers.
 
+config DRM_FORMAR_HELPER_TEST
+	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
+	depends on DRM && KUNIT=y
+	select DRM_KMS_HELPER
+	default KUNIT_ALL_TESTS
+	help
+	  KUnit tests for the drm_format_helper APIs. This option is not
+	  useful for distributions or general kernels, but only for kernel
+	  developers working on DRM and associated drivers.
+
+	  If in doubt, say "N".
+
 config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
         bool "Enable refcount backtrace history in the DP MST helpers"
 	depends on STACKTRACE_SUPPORT
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 15fe3163f822..6a7d7655d38c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -76,6 +76,9 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 #
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \
+		drm_format_helper_test.o
+CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
 
 obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
diff --git a/drivers/gpu/drm/drm_format_helper_test.c b/drivers/gpu/drm/drm_format_helper_test.c
new file mode 100644
index 000000000000..9c8a3007346c
--- /dev/null
+++ b/drivers/gpu/drm/drm_format_helper_test.c
@@ -0,0 +1,166 @@
+// 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
+#define CLIP(x, y, w, h) { (x), (y), (x) + (w), (y) + (h) }
+
+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",
+		.pitch = 1 * 4,
+		.dst_pitch = 0,
+		.clip = CLIP(0, 0, 1, 1),
+		.xrgb8888 = { 0x01FF0000 },
+		.expected = { 0xE0 },
+	},
+	{
+		.name = "Single pixel clip",
+		.pitch = 2 * 4,
+		.dst_pitch = 0,
+		.clip = CLIP(1, 1, 1, 1),
+		.xrgb8888 = {
+			0x00000000, 0x00000000,
+			0x00000000, 0x10FF0000,
+		},
+		.expected = { 0xE0 },
+	},
+	{
+		.name = "White, black, red, green, blue, magenta, yellow, cyan",
+		.pitch = 4 * 4,
+		.dst_pitch = 0,
+		.clip = CLIP(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,
+		},
+	},
+	{
+		.name = "Destination pitch",
+		.pitch = 3 * 4,
+		.dst_pitch = 5,
+		.clip = CLIP(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.
+ * @src_format: source buffer pixel format (DRM_FORMAT_*)
+ * @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 src_format, u32 dst_format,
+				  unsigned int dst_pitch,
+				  const struct drm_rect *clip)
+{
+	const struct drm_format_info *src_fi = drm_format_info(src_format);
+	const struct drm_format_info *dst_fi = drm_format_info(dst_format);
+	size_t width = drm_rect_width(clip);
+	size_t src_nbytes;
+
+	if (!src_fi || !dst_fi)
+		return -EINVAL;
+
+	if (dst_pitch)
+		width = dst_pitch;
+
+	src_nbytes = width * drm_rect_height(clip) * src_fi->cpp[0];
+	if (!src_nbytes)
+		return 0;
+
+	return (src_nbytes * dst_fi->cpp[0]) / src_fi->cpp[0];
+}
+
+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_XRGB8888, DRM_FORMAT_RGB332,
+				       params->dst_pitch, &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	dst = kzalloc(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

[RFC PATCH 1/1] 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 know colors: White, black, red, green, blue, magenta, yellow
   and cyan
 - Other colors: Randomly picked
 - Destination pitch

Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/Kconfig                  |  12 ++
 drivers/gpu/drm/Makefile                 |   3 +
 drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
 3 files changed, 181 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_format_helper_test.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e88c497fa010..d92be6faef15 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -76,6 +76,18 @@ config DRM_KMS_HELPER
 	help
 	  CRTC helpers for KMS drivers.
 
+config DRM_FORMAR_HELPER_TEST
+	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
+	depends on DRM && KUNIT=y
+	select DRM_KMS_HELPER
+	default KUNIT_ALL_TESTS
+	help
+	  KUnit tests for the drm_format_helper APIs. This option is not
+	  useful for distributions or general kernels, but only for kernel
+	  developers working on DRM and associated drivers.
+
+	  If in doubt, say "N".
+
 config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
         bool "Enable refcount backtrace history in the DP MST helpers"
 	depends on STACKTRACE_SUPPORT
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 15fe3163f822..6a7d7655d38c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -76,6 +76,9 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 #
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \
+		drm_format_helper_test.o
+CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
 
 obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
diff --git a/drivers/gpu/drm/drm_format_helper_test.c b/drivers/gpu/drm/drm_format_helper_test.c
new file mode 100644
index 000000000000..9c8a3007346c
--- /dev/null
+++ b/drivers/gpu/drm/drm_format_helper_test.c
@@ -0,0 +1,166 @@
+// 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
+#define CLIP(x, y, w, h) { (x), (y), (x) + (w), (y) + (h) }
+
+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",
+		.pitch = 1 * 4,
+		.dst_pitch = 0,
+		.clip = CLIP(0, 0, 1, 1),
+		.xrgb8888 = { 0x01FF0000 },
+		.expected = { 0xE0 },
+	},
+	{
+		.name = "Single pixel clip",
+		.pitch = 2 * 4,
+		.dst_pitch = 0,
+		.clip = CLIP(1, 1, 1, 1),
+		.xrgb8888 = {
+			0x00000000, 0x00000000,
+			0x00000000, 0x10FF0000,
+		},
+		.expected = { 0xE0 },
+	},
+	{
+		.name = "White, black, red, green, blue, magenta, yellow, cyan",
+		.pitch = 4 * 4,
+		.dst_pitch = 0,
+		.clip = CLIP(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,
+		},
+	},
+	{
+		.name = "Destination pitch",
+		.pitch = 3 * 4,
+		.dst_pitch = 5,
+		.clip = CLIP(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.
+ * @src_format: source buffer pixel format (DRM_FORMAT_*)
+ * @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 src_format, u32 dst_format,
+				  unsigned int dst_pitch,
+				  const struct drm_rect *clip)
+{
+	const struct drm_format_info *src_fi = drm_format_info(src_format);
+	const struct drm_format_info *dst_fi = drm_format_info(dst_format);
+	size_t width = drm_rect_width(clip);
+	size_t src_nbytes;
+
+	if (!src_fi || !dst_fi)
+		return -EINVAL;
+
+	if (dst_pitch)
+		width = dst_pitch;
+
+	src_nbytes = width * drm_rect_height(clip) * src_fi->cpp[0];
+	if (!src_nbytes)
+		return 0;
+
+	return (src_nbytes * dst_fi->cpp[0]) / src_fi->cpp[0];
+}
+
+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_XRGB8888, DRM_FORMAT_RGB332,
+				       params->dst_pitch, &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	dst = kzalloc(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: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Maxime Ripard]

Hi,

On Mon, May 30, 2022 at 12:20:17PM +0200, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
> 
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>

It looks mostly good to me, but I think we should Cc
kunit-dev@googlegroups.com to have their feedback.

> ---
>  drivers/gpu/drm/Kconfig                  |  12 ++
>  drivers/gpu/drm/Makefile                 |   3 +
>  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e88c497fa010..d92be6faef15 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
>  	help
>  	  CRTC helpers for KMS drivers. 
>  
> +config DRM_FORMAR_HELPER_TEST
> +	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> +	depends on DRM && KUNIT=y
> +	select DRM_KMS_HELPER
> +	default KUNIT_ALL_TESTS
> +	help
> +	  KUnit tests for the drm_format_helper APIs. This option is not
> +	  useful for distributions or general kernels, but only for kernel
> +	  developers working on DRM and associated drivers.
> +
> +	  If in doubt, say "N".
> +

AFAIK, kunit test cases are supposed to have a .kunitconfig too to
enable the kunit tests easily.

Maxime

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Maxime Ripard]

Hi,

On Mon, May 30, 2022 at 12:20:17PM +0200, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
> 
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>

It looks mostly good to me, but I think we should Cc
kunit-dev@googlegroups.com to have their feedback.

> ---
>  drivers/gpu/drm/Kconfig                  |  12 ++
>  drivers/gpu/drm/Makefile                 |   3 +
>  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e88c497fa010..d92be6faef15 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
>  	help
>  	  CRTC helpers for KMS drivers. 
>  
> +config DRM_FORMAR_HELPER_TEST
> +	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> +	depends on DRM && KUNIT=y
> +	select DRM_KMS_HELPER
> +	default KUNIT_ALL_TESTS
> +	help
> +	  KUnit tests for the drm_format_helper APIs. This option is not
> +	  useful for distributions or general kernels, but only for kernel
> +	  developers working on DRM and associated drivers.
> +
> +	  If in doubt, say "N".
> +

AFAIK, kunit test cases are supposed to have a .kunitconfig too to
enable the kunit tests easily.

Maxime

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hi Maxime,

On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote:
> 
> Hi,
> 
> On Mon, May 30, 2022 at 12:20:17PM +0200, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
> >    and cyan
> >  - Other colors: Randomly picked
> >  - Destination pitch
> > 
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> 
> It looks mostly good to me, but I think we should Cc
> kunit-dev@googlegroups.com to have their feedback.

Thanks a lot for the quick feedback.

I just cc'ed kunit-dev@googlegroups.com. For anyone joining the
conversation, here is the link to the patch and the cover letter with
some questions:

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

> 
> > ---
> >  drivers/gpu/drm/Kconfig                  |  12 ++
> >  drivers/gpu/drm/Makefile                 |   3 +
> >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> >  3 files changed, 181 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index e88c497fa010..d92be6faef15 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> >  	help
> >  	  CRTC helpers for KMS drivers. 
> >  
> > +config DRM_FORMAR_HELPER_TEST
> > +	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > +	depends on DRM && KUNIT=y
> > +	select DRM_KMS_HELPER
> > +	default KUNIT_ALL_TESTS
> > +	help
> > +	  KUnit tests for the drm_format_helper APIs. This option is not
> > +	  useful for distributions or general kernels, but only for kernel
> > +	  developers working on DRM and associated drivers.
> > +
> > +	  If in doubt, say "N".
> > +
> 
> AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> enable the kunit tests easily.
> 
> Maxime

A .kuniconfig example is present in the cover letter. My understanding
from the docs:

https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file

Is that, like the .config file, the .kunitconfig file is not meant to
be included in git, but I'm sure someone else will clarify this point.

Thanks again,
José Expósito

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hi Maxime,

On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote:
> 
> Hi,
> 
> On Mon, May 30, 2022 at 12:20:17PM +0200, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
> >    and cyan
> >  - Other colors: Randomly picked
> >  - Destination pitch
> > 
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> 
> It looks mostly good to me, but I think we should Cc
> kunit-dev@googlegroups.com to have their feedback.

Thanks a lot for the quick feedback.

I just cc'ed kunit-dev@googlegroups.com. For anyone joining the
conversation, here is the link to the patch and the cover letter with
some questions:

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

> 
> > ---
> >  drivers/gpu/drm/Kconfig                  |  12 ++
> >  drivers/gpu/drm/Makefile                 |   3 +
> >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> >  3 files changed, 181 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index e88c497fa010..d92be6faef15 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> >  	help
> >  	  CRTC helpers for KMS drivers. 
> >  
> > +config DRM_FORMAR_HELPER_TEST
> > +	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > +	depends on DRM && KUNIT=y
> > +	select DRM_KMS_HELPER
> > +	default KUNIT_ALL_TESTS
> > +	help
> > +	  KUnit tests for the drm_format_helper APIs. This option is not
> > +	  useful for distributions or general kernels, but only for kernel
> > +	  developers working on DRM and associated drivers.
> > +
> > +	  If in doubt, say "N".
> > +
> 
> AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> enable the kunit tests easily.
> 
> Maxime

A .kuniconfig example is present in the cover letter. My understanding
from the docs:

https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file

Is that, like the .config file, the .kunitconfig file is not meant to
be included in git, but I'm sure someone else will clarify this point.

Thanks again,
José Expósito

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote:
> I just cc'ed kunit-dev@googlegroups.com. For anyone joining the
> conversation, here is the link to the patch and the cover letter with
> some questions:
>
> https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/

Thanks for the link.
A few initial notes:
a) normally, `select`ing other kconfigs is discouraged. It's not
explicitly called out in
https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
but this was the conclusion after  some debate on the mailing lists
earlier.
b) I see `dst` is allocated with kzalloc but not freed. Should we use
`kunit_kzalloc()` instead so it does get automatically freed?

>
> >
> > > ---
> > >  drivers/gpu/drm/Kconfig                  |  12 ++
> > >  drivers/gpu/drm/Makefile                 |   3 +
> > >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > >  3 files changed, 181 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index e88c497fa010..d92be6faef15 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > >     help
> > >       CRTC helpers for KMS drivers.
> > >
> > > +config DRM_FORMAR_HELPER_TEST
> > > +   bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > +   depends on DRM && KUNIT=y
> > > +   select DRM_KMS_HELPER

From above, a)
Specifically here, it'd be encouraged to instead do
  depends on DRM && KUNIT=y && DRM_KMS_HELPER

Ideally, using a .kunitconfig file would make it so having to select
DRM_KMS_HELPER manually isn't that annoying.

> > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > enable the kunit tests easily.
> >
> > Maxime
>
> A .kuniconfig example is present in the cover letter. My understanding
> from the docs:
>
> https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file

The bit of the documentation you're looking for is
https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests
You can create a drivers/gpu/drm/.kunitconfig file and run with
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86

The contents of that file would be just like
  CONFIG_KUNIT=y
  CONFIG_DRM=y
  CONFIG_DRM_KMS_HELPER=y  # if no `select`
  CONFIG_DRM_FORMAR_HELPER_TEST=y

Re "kunit test cases are supposed to have a .kunitconfig too"
As I noted in the docs:
  This is a relatively new feature (5.12+) so we don’t have any
conventions yet about on what files should be checked in versus just
kept around locally. It’s up to you and your maintainer to decide if a
config is useful enough to submit (and therefore have to maintain).

So it's up to whatever people think works best/is worth it.
I think in this case, it makes sense to add the file.

> Is that, like the .config file, the .kunitconfig file is not meant to
> be included in git, but I'm sure someone else will clarify this point.

That bit of the docs needs to be rewritten.
You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git.

Context: `kunit.py` used to use the "<root>/.kunitconfig" file.
Anytime you wanted to run more tests, you'd append to that file.
So we agreed that no one should check in that file specifically.

Now kunit.py
* uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig"
* has the --kunitconfig flag so you can use different files
so it's no longer as relevant.

Hope that helps,
Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote:
> I just cc'ed kunit-dev@googlegroups.com. For anyone joining the
> conversation, here is the link to the patch and the cover letter with
> some questions:
>
> https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/

Thanks for the link.
A few initial notes:
a) normally, `select`ing other kconfigs is discouraged. It's not
explicitly called out in
https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
but this was the conclusion after  some debate on the mailing lists
earlier.
b) I see `dst` is allocated with kzalloc but not freed. Should we use
`kunit_kzalloc()` instead so it does get automatically freed?

>
> >
> > > ---
> > >  drivers/gpu/drm/Kconfig                  |  12 ++
> > >  drivers/gpu/drm/Makefile                 |   3 +
> > >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > >  3 files changed, 181 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index e88c497fa010..d92be6faef15 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > >     help
> > >       CRTC helpers for KMS drivers.
> > >
> > > +config DRM_FORMAR_HELPER_TEST
> > > +   bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > +   depends on DRM && KUNIT=y
> > > +   select DRM_KMS_HELPER

From above, a)
Specifically here, it'd be encouraged to instead do
  depends on DRM && KUNIT=y && DRM_KMS_HELPER

Ideally, using a .kunitconfig file would make it so having to select
DRM_KMS_HELPER manually isn't that annoying.

> > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > enable the kunit tests easily.
> >
> > Maxime
>
> A .kuniconfig example is present in the cover letter. My understanding
> from the docs:
>
> https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file

The bit of the documentation you're looking for is
https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests
You can create a drivers/gpu/drm/.kunitconfig file and run with
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86

The contents of that file would be just like
  CONFIG_KUNIT=y
  CONFIG_DRM=y
  CONFIG_DRM_KMS_HELPER=y  # if no `select`
  CONFIG_DRM_FORMAR_HELPER_TEST=y

Re "kunit test cases are supposed to have a .kunitconfig too"
As I noted in the docs:
  This is a relatively new feature (5.12+) so we don’t have any
conventions yet about on what files should be checked in versus just
kept around locally. It’s up to you and your maintainer to decide if a
config is useful enough to submit (and therefore have to maintain).

So it's up to whatever people think works best/is worth it.
I think in this case, it makes sense to add the file.

> Is that, like the .config file, the .kunitconfig file is not meant to
> be included in git, but I'm sure someone else will clarify this point.

That bit of the docs needs to be rewritten.
You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git.

Context: `kunit.py` used to use the "<root>/.kunitconfig" file.
Anytime you wanted to run more tests, you'd append to that file.
So we agreed that no one should check in that file specifically.

Now kunit.py
* uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig"
* has the --kunitconfig flag so you can use different files
so it's no longer as relevant.

Hope that helps,
Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[kernel test robot]

Hi "José,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on drm/drm-next]
[also build test ERROR on v5.18 next-20220527]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Jos-Exp-sito/KUnit-tests-for-drm_format_helper/20220530-182201
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: hexagon-randconfig-r045-20220531 (https://download.01.org/0day-ci/archive/20220531/202205311333.bD979r32-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c825abd6b0198fb088d9752f556a70705bc99dfd)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ed116da35ca09da26713d2192e8493c5d9c500f3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jos-Exp-sito/KUnit-tests-for-drm_format_helper/20220530-182201
        git checkout ed116da35ca09da26713d2192e8493c5d9c500f3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: drm_bridge_hpd_enable
   >>> referenced by drm_bridge_connector.c
   >>> gpu/drm/drm_bridge_connector.o:(drm_bridge_connector_enable_hpd) in archive drivers/built-in.a
   >>> referenced by drm_bridge_connector.c
   >>> gpu/drm/drm_bridge_connector.o:(drm_bridge_connector_enable_hpd) in archive drivers/built-in.a
   >>> referenced by drm_bridge_connector.c
   >>> gpu/drm/drm_bridge_connector.o:(drm_bridge_connector_init) in archive drivers/built-in.a
   >>> referenced 1 more times
--
>> ld.lld: error: undefined symbol: drm_bridge_hpd_disable
   >>> referenced by drm_bridge_connector.c
   >>> gpu/drm/drm_bridge_connector.o:(drm_bridge_connector_disable_hpd) in archive drivers/built-in.a
   >>> referenced by drm_bridge_connector.c
   >>> gpu/drm/drm_bridge_connector.o:(drm_bridge_connector_disable_hpd) in archive drivers/built-in.a
   >>> referenced by drm_bridge_connector.c
   >>> gpu/drm/drm_bridge_connector.o:(drm_bridge_connector_destroy) in archive drivers/built-in.a
   >>> referenced 1 more times
--
>> ld.lld: error: undefined symbol: drm_connector_list_iter_end
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_encoder_in_use) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_encoder_in_use) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_config) in archive drivers/built-in.a
   >>> referenced 35 more times
--
>> ld.lld: error: undefined symbol: drm_modeset_lock_all
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_disable_unused_functions) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_disable_unused_functions) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_resume_force_mode) in archive drivers/built-in.a
   >>> referenced 7 more times
--
>> ld.lld: error: undefined symbol: drm_modeset_unlock_all
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_disable_unused_functions) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_disable_unused_functions) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_helper_resume_force_mode) in archive drivers/built-in.a
   >>> referenced 7 more times
--
>> ld.lld: error: undefined symbol: drm_warn_on_modeset_not_all_locked
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(__drm_helper_disable_unused_functions) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(__drm_helper_disable_unused_functions) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced 3 more times
--
>> ld.lld: error: undefined symbol: drm_mode_duplicate
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
--
>> ld.lld: error: undefined symbol: drm_mode_init
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced 1 more times
--
>> ld.lld: error: undefined symbol: drm_mode_copy
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced 7 more times
--
>> ld.lld: error: undefined symbol: __drm_dbg
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced 45 more times
--
>> ld.lld: error: undefined symbol: drm_mode_destroy
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced by drm_crtc_helper.c
   >>> gpu/drm/drm_crtc_helper.o:(drm_crtc_helper_set_mode) in archive drivers/built-in.a
   >>> referenced 1 more times
..

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hi Daniel,

Thanks for looking into the patch and for your comments.

On Mon, May 30, 2022 at 03:57:56PM -0700, Daniel Latypov wrote:
> A few initial notes:
> a) normally, `select`ing other kconfigs is discouraged. It's not
> explicitly called out in
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
> but this was the conclusion after  some debate on the mailing lists
> earlier.
>
> b) I see `dst` is allocated with kzalloc but not freed. Should we use
> `kunit_kzalloc()` instead so it does get automatically freed?

Ooops yes, it was in my "I'll handle that once it works" list, but I
forgot to fix it, thanks for pointing it out
 
> > > > ---
> > > >  drivers/gpu/drm/Kconfig                  |  12 ++
> > > >  drivers/gpu/drm/Makefile                 |   3 +
> > > >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > > >  3 files changed, 181 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > > >
> > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > > index e88c497fa010..d92be6faef15 100644
> > > > --- a/drivers/gpu/drm/Kconfig
> > > > +++ b/drivers/gpu/drm/Kconfig
> > > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > > >     help
> > > >       CRTC helpers for KMS drivers.
> > > >
> > > > +config DRM_FORMAR_HELPER_TEST
> > > > +   bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > > +   depends on DRM && KUNIT=y
> > > > +   select DRM_KMS_HELPER
> 
> From above, a)
> Specifically here, it'd be encouraged to instead do
>   depends on DRM && KUNIT=y && DRM_KMS_HELPER

My first attempt was to go with:

	depends on KUNIT=y && DRM && DRM_KMS_HELPER

However, when I try to run the tests I get this error:

	$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_64
	Regenerating .config ...
	Populating config with:
	$ make ARCH=x86_64 olddefconfig O=.kunit
	ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
	This is probably due to unsatisfied dependencies.
	Missing: CONFIG_DRM_KMS_HELPER=y, CONFIG_DRM_FORMAR_HELPER_TEST=y

I wasn't able to figure out why that was happening, so I decided to use
"select", which seems to solve the problem.

Do you know why this could be happening?

> Ideally, using a .kunitconfig file would make it so having to select
> DRM_KMS_HELPER manually isn't that annoying.
> 
> > > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > > enable the kunit tests easily.
> > >
> > > Maxime
> >
> > A .kuniconfig example is present in the cover letter. My understanding
> > from the docs:
> >
> > https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file
> 
> The bit of the documentation you're looking for is
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests
> You can create a drivers/gpu/drm/.kunitconfig file and run with
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86
> 
> The contents of that file would be just like
>   CONFIG_KUNIT=y
>   CONFIG_DRM=y
>   CONFIG_DRM_KMS_HELPER=y  # if no `select`
>   CONFIG_DRM_FORMAR_HELPER_TEST=y

Noted, thanks a lot, I'll include it in the final version of the patch.

By the way, I also included it in an unrelated patch, just in case you
are wondering why I emailed you a random patch:
https://lore.kernel.org/linux-input/20220531181246.190729-1-jose.exposito89@gmail.com/T/

Thanks a lot for your help,
José Expósito

> Re "kunit test cases are supposed to have a .kunitconfig too"
> As I noted in the docs:
>   This is a relatively new feature (5.12+) so we don’t have any
> conventions yet about on what files should be checked in versus just
> kept around locally. It’s up to you and your maintainer to decide if a
> config is useful enough to submit (and therefore have to maintain).
> 
> So it's up to whatever people think works best/is worth it.
> I think in this case, it makes sense to add the file.
> 
> > Is that, like the .config file, the .kunitconfig file is not meant to
> > be included in git, but I'm sure someone else will clarify this point.
> 
> That bit of the docs needs to be rewritten.
> You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git.
> 
> Context: `kunit.py` used to use the "<root>/.kunitconfig" file.
> Anytime you wanted to run more tests, you'd append to that file.
> So we agreed that no one should check in that file specifically.
> 
> Now kunit.py
> * uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig"
> * has the --kunitconfig flag so you can use different files
> so it's no longer as relevant.
> 
> Hope that helps,
> Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hi Daniel,

Thanks for looking into the patch and for your comments.

On Mon, May 30, 2022 at 03:57:56PM -0700, Daniel Latypov wrote:
> A few initial notes:
> a) normally, `select`ing other kconfigs is discouraged. It's not
> explicitly called out in
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
> but this was the conclusion after  some debate on the mailing lists
> earlier.
>
> b) I see `dst` is allocated with kzalloc but not freed. Should we use
> `kunit_kzalloc()` instead so it does get automatically freed?

Ooops yes, it was in my "I'll handle that once it works" list, but I
forgot to fix it, thanks for pointing it out
 
> > > > ---
> > > >  drivers/gpu/drm/Kconfig                  |  12 ++
> > > >  drivers/gpu/drm/Makefile                 |   3 +
> > > >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > > >  3 files changed, 181 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > > >
> > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > > index e88c497fa010..d92be6faef15 100644
> > > > --- a/drivers/gpu/drm/Kconfig
> > > > +++ b/drivers/gpu/drm/Kconfig
> > > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > > >     help
> > > >       CRTC helpers for KMS drivers.
> > > >
> > > > +config DRM_FORMAR_HELPER_TEST
> > > > +   bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > > +   depends on DRM && KUNIT=y
> > > > +   select DRM_KMS_HELPER
> 
> From above, a)
> Specifically here, it'd be encouraged to instead do
>   depends on DRM && KUNIT=y && DRM_KMS_HELPER

My first attempt was to go with:

	depends on KUNIT=y && DRM && DRM_KMS_HELPER

However, when I try to run the tests I get this error:

	$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_64
	Regenerating .config ...
	Populating config with:
	$ make ARCH=x86_64 olddefconfig O=.kunit
	ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
	This is probably due to unsatisfied dependencies.
	Missing: CONFIG_DRM_KMS_HELPER=y, CONFIG_DRM_FORMAR_HELPER_TEST=y

I wasn't able to figure out why that was happening, so I decided to use
"select", which seems to solve the problem.

Do you know why this could be happening?

> Ideally, using a .kunitconfig file would make it so having to select
> DRM_KMS_HELPER manually isn't that annoying.
> 
> > > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > > enable the kunit tests easily.
> > >
> > > Maxime
> >
> > A .kuniconfig example is present in the cover letter. My understanding
> > from the docs:
> >
> > https://docs.kernel.org/dev-tools/kunit/run_wrapper.html#create-a-kunitconfig-file
> 
> The bit of the documentation you're looking for is
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/running_tips.html#defining-a-set-of-tests
> You can create a drivers/gpu/drm/.kunitconfig file and run with
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_86
> 
> The contents of that file would be just like
>   CONFIG_KUNIT=y
>   CONFIG_DRM=y
>   CONFIG_DRM_KMS_HELPER=y  # if no `select`
>   CONFIG_DRM_FORMAR_HELPER_TEST=y

Noted, thanks a lot, I'll include it in the final version of the patch.

By the way, I also included it in an unrelated patch, just in case you
are wondering why I emailed you a random patch:
https://lore.kernel.org/linux-input/20220531181246.190729-1-jose.exposito89@gmail.com/T/

Thanks a lot for your help,
José Expósito

> Re "kunit test cases are supposed to have a .kunitconfig too"
> As I noted in the docs:
>   This is a relatively new feature (5.12+) so we don’t have any
> conventions yet about on what files should be checked in versus just
> kept around locally. It’s up to you and your maintainer to decide if a
> config is useful enough to submit (and therefore have to maintain).
> 
> So it's up to whatever people think works best/is worth it.
> I think in this case, it makes sense to add the file.
> 
> > Is that, like the .config file, the .kunitconfig file is not meant to
> > be included in git, but I'm sure someone else will clarify this point.
> 
> That bit of the docs needs to be rewritten.
> You're recommended to check in a drivers/gpu/drm/.kunitconfig file into git.
> 
> Context: `kunit.py` used to use the "<root>/.kunitconfig" file.
> Anytime you wanted to run more tests, you'd append to that file.
> So we agreed that no one should check in that file specifically.
> 
> Now kunit.py
> * uses "<build-dir>/.kunitconfig", by default: ".kunit/.kunitconfig"
> * has the --kunitconfig flag so you can use different files
> so it's no longer as relevant.
> 
> Hope that helps,
> Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Tue, May 31, 2022 at 11:45 AM José Expósito
<jose.exposito89@gmail.com> wrote:
> >
> > From above, a)
> > Specifically here, it'd be encouraged to instead do
> >   depends on DRM && KUNIT=y && DRM_KMS_HELPER
>
> My first attempt was to go with:
>
>         depends on KUNIT=y && DRM && DRM_KMS_HELPER
>
> However, when I try to run the tests I get this error:
>
>         $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_64
>         Regenerating .config ...
>         Populating config with:
>         $ make ARCH=x86_64 olddefconfig O=.kunit
>         ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
>         This is probably due to unsatisfied dependencies.
>         Missing: CONFIG_DRM_KMS_HELPER=y, CONFIG_DRM_FORMAR_HELPER_TEST=y
>
> I wasn't able to figure out why that was happening, so I decided to use
> "select", which seems to solve the problem.
>
> Do you know why this could be happening?

Ah, you should probably ignore my suggestion then.

Looking at the Kconfig file, it's defined as
  config DRM_KMS_HELPER
  tristate
  depends on DRM
  help
    CRTC helpers for KMS drivers.

Notably, the config lacks a description string.
IIUC, this makes it a "hidden" kconfig option, i.e. you can't directly
select it yourself in a .config, it must be selected by another
kconfig option.

E.g. you can try selecting it manually and see it fail via:
$ ./tools/testing/kunit/kunit.py config --arch=x86_64
--kconfig_add=CONFIG_DRM=y --kconfig_add=CONFIG_DRM_KMS_HELPER=y
...
Missing: CONFIG_DRM_KMS_HELPER=y

So having the test select it makes the most sense, unless there's a
better kconfig option to automatically select it (I have no idea,
someone with knowledge of the DRM code might though).

Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Tue, May 31, 2022 at 11:45 AM José Expósito
<jose.exposito89@gmail.com> wrote:
> >
> > From above, a)
> > Specifically here, it'd be encouraged to instead do
> >   depends on DRM && KUNIT=y && DRM_KMS_HELPER
>
> My first attempt was to go with:
>
>         depends on KUNIT=y && DRM && DRM_KMS_HELPER
>
> However, when I try to run the tests I get this error:
>
>         $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm --arch=x86_64
>         Regenerating .config ...
>         Populating config with:
>         $ make ARCH=x86_64 olddefconfig O=.kunit
>         ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
>         This is probably due to unsatisfied dependencies.
>         Missing: CONFIG_DRM_KMS_HELPER=y, CONFIG_DRM_FORMAR_HELPER_TEST=y
>
> I wasn't able to figure out why that was happening, so I decided to use
> "select", which seems to solve the problem.
>
> Do you know why this could be happening?

Ah, you should probably ignore my suggestion then.

Looking at the Kconfig file, it's defined as
  config DRM_KMS_HELPER
  tristate
  depends on DRM
  help
    CRTC helpers for KMS drivers.

Notably, the config lacks a description string.
IIUC, this makes it a "hidden" kconfig option, i.e. you can't directly
select it yourself in a .config, it must be selected by another
kconfig option.

E.g. you can try selecting it manually and see it fail via:
$ ./tools/testing/kunit/kunit.py config --arch=x86_64
--kconfig_add=CONFIG_DRM=y --kconfig_add=CONFIG_DRM_KMS_HELPER=y
...
Missing: CONFIG_DRM_KMS_HELPER=y

So having the test select it makes the most sense, unless there's a
better kconfig option to automatically select it (I have no idea,
someone with knowledge of the DRM code might though).

Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Javier Martinez Canillas]

Hello José,

On 5/30/22 12:20, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
> 
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

Thanks a lot for working on this! It's very cool to see the first KUnit
tests added to DRM :)

>  drivers/gpu/drm/Kconfig                  |  12 ++
>  drivers/gpu/drm/Makefile                 |   3 +
>  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e88c497fa010..d92be6faef15 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
>  	help
>  	  CRTC helpers for KMS drivers.
>  
> +config DRM_FORMAR_HELPER_TEST

I wonder if we want this level of detail for the test Kconfig symbols.
Maybe we could just have a DRM_KUNIT_TEST symbol that will enable all
the KUnit test suites for DRM ?

> +	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> +	depends on DRM && KUNIT=y
> +	select DRM_KMS_HELPER

Daniel didn't like this `select DRM_KMS_HELPER` and I think that we can avoid
it if instead drm_format_helper_test.c is included in drm_format_helper.c, i.e:

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index a3ccd8bc966f..d63e02da528f 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -692,3 +692,7 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd
        kfree(src32);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
+
+#ifdef DRM_KUNIT_TEST
+#include "drm_format_helper_test.c"
+#endif

This also has the advantage that will allow to have KUnit tests for the static
functions that are only available in the drm_format_helper.c compilation unit.

>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \
> +		drm_format_helper_test.o

And doing that will also allow you to get rid of this, since just selecting
CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.

> +CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
>

A comment on why this is needed would useful.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Javier Martinez Canillas]

Hello José,

On 5/30/22 12:20, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
> 
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---

Thanks a lot for working on this! It's very cool to see the first KUnit
tests added to DRM :)

>  drivers/gpu/drm/Kconfig                  |  12 ++
>  drivers/gpu/drm/Makefile                 |   3 +
>  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e88c497fa010..d92be6faef15 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
>  	help
>  	  CRTC helpers for KMS drivers.
>  
> +config DRM_FORMAR_HELPER_TEST

I wonder if we want this level of detail for the test Kconfig symbols.
Maybe we could just have a DRM_KUNIT_TEST symbol that will enable all
the KUnit test suites for DRM ?

> +	bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> +	depends on DRM && KUNIT=y
> +	select DRM_KMS_HELPER

Daniel didn't like this `select DRM_KMS_HELPER` and I think that we can avoid
it if instead drm_format_helper_test.c is included in drm_format_helper.c, i.e:

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index a3ccd8bc966f..d63e02da528f 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -692,3 +692,7 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd
        kfree(src32);
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
+
+#ifdef DRM_KUNIT_TEST
+#include "drm_format_helper_test.c"
+#endif

This also has the advantage that will allow to have KUnit tests for the static
functions that are only available in the drm_format_helper.c compilation unit.

>  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \
> +		drm_format_helper_test.o

And doing that will also allow you to get rid of this, since just selecting
CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.

> +CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
>

A comment on why this is needed would useful.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> > +CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
> >
>
> A comment on why this is needed would useful.

Ah, I think that should not be necessary anymore.
We added this to some tests to mitigate against compilers that didn't
optimize away stack-local structs used internally in KUNIT_EXPECT*.
Functions with ~30 or so EXPECTs could get flagged for excessively
large stack frames.

But in 5.18, I had some patches to reduce the naive stack usage from
[48..88] => [8..32] bytes per EXPECT.
I also have some RFC patches out to get it down to [0, 24] bytes.

So going forward, this should only be necessary if you have something
like 100s of EXPECTs in a single function (in which case you should
also consider splitting that function up).

Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> > +CFLAGS_drm_format_helper_test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
> >
>
> A comment on why this is needed would useful.

Ah, I think that should not be necessary anymore.
We added this to some tests to mitigate against compilers that didn't
optimize away stack-local structs used internally in KUNIT_EXPECT*.
Functions with ~30 or so EXPECTs could get flagged for excessively
large stack frames.

But in 5.18, I had some patches to reduce the naive stack usage from
[48..88] => [8..32] bytes per EXPECT.
I also have some RFC patches out to get it down to [0, 24] bytes.

So going forward, this should only be necessary if you have something
like 100s of EXPECTs in a single function (in which case you should
also consider splitting that function up).

Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[David Gow]

On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello José,
>
> On 5/30/22 12:20, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
> >    and cyan
> >  - Other colors: Randomly picked
> >  - Destination pitch
> >
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
>
> Thanks a lot for working on this! It's very cool to see the first KUnit
> tests added to DRM :)
>
> >  drivers/gpu/drm/Kconfig                  |  12 ++
> >  drivers/gpu/drm/Makefile                 |   3 +
> >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> >  3 files changed, 181 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index e88c497fa010..d92be6faef15 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> >       help
> >         CRTC helpers for KMS drivers.
> >
> > +config DRM_FORMAR_HELPER_TEST
>
> I wonder if we want this level of detail for the test Kconfig symbols.
> Maybe we could just have a DRM_KUNIT_TEST symbol that will enable all
> the KUnit test suites for DRM ?
>
> > +     bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > +     depends on DRM && KUNIT=y
> > +     select DRM_KMS_HELPER
>
> Daniel didn't like this `select DRM_KMS_HELPER` and I think that we can avoid
> it if instead drm_format_helper_test.c is included in drm_format_helper.c, i.e:
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index a3ccd8bc966f..d63e02da528f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -692,3 +692,7 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd
>         kfree(src32);
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
> +
> +#ifdef DRM_KUNIT_TEST
> +#include "drm_format_helper_test.c"
> +#endif
>
> This also has the advantage that will allow to have KUnit tests for the static
> functions that are only available in the drm_format_helper.c compilation unit.
>
> >  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> > +obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \
> > +             drm_format_helper_test.o
>
> And doing that will also allow you to get rid of this, since just selecting
> CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.
>

This is definitely something other KUnit tests (apparmor, elf, etc)
are doing, and it's generally fine. You do lose the ability to build
the tests as a separate module, though. (This is not usually a big
problem, but there are some cases where it's useful.)

That being said, I don't think 'select' is enough of a problem that
you should feel the need to refactor in this way just to avoid it.
Given most of the other DRM drivers (as well as DRM_DEBUG_SELFTEST)
are select-ing DRM_KMS_HELPER, it seems like a sensible enough thing
to continue doing for the KUnit test. As Daniel pointed out, as a
hidden option it was clearly always meant to be select-ed anyway.

Cheers,
-- David

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[David Gow]

On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello José,
>
> On 5/30/22 12:20, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
> >    and cyan
> >  - Other colors: Randomly picked
> >  - Destination pitch
> >
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
>
> Thanks a lot for working on this! It's very cool to see the first KUnit
> tests added to DRM :)
>
> >  drivers/gpu/drm/Kconfig                  |  12 ++
> >  drivers/gpu/drm/Makefile                 |   3 +
> >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> >  3 files changed, 181 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index e88c497fa010..d92be6faef15 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> >       help
> >         CRTC helpers for KMS drivers.
> >
> > +config DRM_FORMAR_HELPER_TEST
>
> I wonder if we want this level of detail for the test Kconfig symbols.
> Maybe we could just have a DRM_KUNIT_TEST symbol that will enable all
> the KUnit test suites for DRM ?
>
> > +     bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > +     depends on DRM && KUNIT=y
> > +     select DRM_KMS_HELPER
>
> Daniel didn't like this `select DRM_KMS_HELPER` and I think that we can avoid
> it if instead drm_format_helper_test.c is included in drm_format_helper.c, i.e:
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index a3ccd8bc966f..d63e02da528f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -692,3 +692,7 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd
>         kfree(src32);
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
> +
> +#ifdef DRM_KUNIT_TEST
> +#include "drm_format_helper_test.c"
> +#endif
>
> This also has the advantage that will allow to have KUnit tests for the static
> functions that are only available in the drm_format_helper.c compilation unit.
>
> >  obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> > +obj-$(CONFIG_DRM_FORMAR_HELPER_TEST) += drm_kms_helper.o \
> > +             drm_format_helper_test.o
>
> And doing that will also allow you to get rid of this, since just selecting
> CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.
>

This is definitely something other KUnit tests (apparmor, elf, etc)
are doing, and it's generally fine. You do lose the ability to build
the tests as a separate module, though. (This is not usually a big
problem, but there are some cases where it's useful.)

That being said, I don't think 'select' is enough of a problem that
you should feel the need to refactor in this way just to avoid it.
Given most of the other DRM drivers (as well as DRM_DEBUG_SELFTEST)
are select-ing DRM_KMS_HELPER, it seems like a sensible enough thing
to continue doing for the KUnit test. As Daniel pointed out, as a
hidden option it was clearly always meant to be select-ed anyway.

Cheers,
-- David

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[David Gow]

On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote:
>
> Hi Maxime,
>
> On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote:
> >
> > Hi,
> >
> > On Mon, May 30, 2022 at 12:20:17PM +0200, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
> > >    and cyan
> > >  - Other colors: Randomly picked
> > >  - Destination pitch
> > >
> > > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> >
> > It looks mostly good to me, but I think we should Cc
> > kunit-dev@googlegroups.com to have their feedback.
>
> Thanks a lot for the quick feedback.
>
> I just cc'ed kunit-dev@googlegroups.com. For anyone joining the
> conversation, here is the link to the patch and the cover letter with
> some questions:
>
> https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/
>
> >
> > > ---
> > >  drivers/gpu/drm/Kconfig                  |  12 ++
> > >  drivers/gpu/drm/Makefile                 |   3 +
> > >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > >  3 files changed, 181 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index e88c497fa010..d92be6faef15 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > >     help
> > >       CRTC helpers for KMS drivers.
> > >
> > > +config DRM_FORMAR_HELPER_TEST
> > > +   bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > +   depends on DRM && KUNIT=y
> > > +   select DRM_KMS_HELPER
> > > +   default KUNIT_ALL_TESTS
> > > +   help
> > > +     KUnit tests for the drm_format_helper APIs. This option is not
> > > +     useful for distributions or general kernels, but only for kernel
> > > +     developers working on DRM and associated drivers.
> > > +
> > > +     If in doubt, say "N".
> > > +
> >
> > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > enable the kunit tests easily.
> >
> > Maxime
>
> A .kuniconfig example is present in the cover letter. (...)

FYI: it's also possible to run these tests under UML with the extra options:
CONFIG_VIRTIO_UML=y
CONFIG_UML_PCI_OVER_VIRTIO=y

I suspect it's probably better not to add those to your .kunitconfig,
as they're UML-specific and will therefore break other architectures,
but it does mean the tests can be run with, for example:

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

Or, without the .kunitconfig:
./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y
--kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add
CONFIG_VIRTIO_UML=y  --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
'drm-*'

Cheers,
-- David

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[David Gow]

On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote:
>
> Hi Maxime,
>
> On Mon, May 30, 2022 at 03:11:58PM +0200, Maxime Ripard wrote:
> >
> > Hi,
> >
> > On Mon, May 30, 2022 at 12:20:17PM +0200, José Expósito 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 know colors: White, black, red, green, blue, magenta, yellow
> > >    and cyan
> > >  - Other colors: Randomly picked
> > >  - Destination pitch
> > >
> > > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> >
> > It looks mostly good to me, but I think we should Cc
> > kunit-dev@googlegroups.com to have their feedback.
>
> Thanks a lot for the quick feedback.
>
> I just cc'ed kunit-dev@googlegroups.com. For anyone joining the
> conversation, here is the link to the patch and the cover letter with
> some questions:
>
> https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/
>
> >
> > > ---
> > >  drivers/gpu/drm/Kconfig                  |  12 ++
> > >  drivers/gpu/drm/Makefile                 |   3 +
> > >  drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> > >  3 files changed, 181 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index e88c497fa010..d92be6faef15 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -76,6 +76,18 @@ config DRM_KMS_HELPER
> > >     help
> > >       CRTC helpers for KMS drivers.
> > >
> > > +config DRM_FORMAR_HELPER_TEST
> > > +   bool "drm_format_helper tests" if !KUNIT_ALL_TESTS
> > > +   depends on DRM && KUNIT=y
> > > +   select DRM_KMS_HELPER
> > > +   default KUNIT_ALL_TESTS
> > > +   help
> > > +     KUnit tests for the drm_format_helper APIs. This option is not
> > > +     useful for distributions or general kernels, but only for kernel
> > > +     developers working on DRM and associated drivers.
> > > +
> > > +     If in doubt, say "N".
> > > +
> >
> > AFAIK, kunit test cases are supposed to have a .kunitconfig too to
> > enable the kunit tests easily.
> >
> > Maxime
>
> A .kuniconfig example is present in the cover letter. (...)

FYI: it's also possible to run these tests under UML with the extra options:
CONFIG_VIRTIO_UML=y
CONFIG_UML_PCI_OVER_VIRTIO=y

I suspect it's probably better not to add those to your .kunitconfig,
as they're UML-specific and will therefore break other architectures,
but it does mean the tests can be run with, for example:

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

Or, without the .kunitconfig:
./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y
--kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add
CONFIG_VIRTIO_UML=y  --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
'drm-*'

Cheers,
-- David

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Javier Martinez Canillas]

Hello David,

On 6/2/22 19:07, David Gow wrote:
> On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas

[snip]

>>
>> And doing that will also allow you to get rid of this, since just selecting
>> CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.
>>
> 
> This is definitely something other KUnit tests (apparmor, elf, etc)
> are doing, and it's generally fine. You do lose the ability to build
> the tests as a separate module, though. (This is not usually a big
> problem, but there are some cases where it's useful.)
> 
> That being said, I don't think 'select' is enough of a problem that
> you should feel the need to refactor in this way just to avoid it.

Oh, yes I didn't want to imply that this was the main reason but just
pointed out that wouldn't even be needed if done that way. And it is
something that we want to do anyway IMO, since as mentioned it would
allow to test the static functions, which are the majority the format
helpers in that file.

> Given most of the other DRM drivers (as well as DRM_DEBUG_SELFTEST)
> are select-ing DRM_KMS_HELPER, it seems like a sensible enough thing
> to continue doing for the KUnit test. As Daniel pointed out, as a
> hidden option it was clearly always meant to be select-ed anyway.
>

Yes, it can be done from the DRM_KUNIT_TEST symbol or just have it set
(and any other needed DRM helper libraries tested by other suites) in
the .kunitconfig file.

I don't think is that important, since at the end that dependency will
have to be maintained in some place.
 
> Cheers,
> -- David
> 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Javier Martinez Canillas]

Hello David,

On 6/2/22 19:07, David Gow wrote:
> On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas

[snip]

>>
>> And doing that will also allow you to get rid of this, since just selecting
>> CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.
>>
> 
> This is definitely something other KUnit tests (apparmor, elf, etc)
> are doing, and it's generally fine. You do lose the ability to build
> the tests as a separate module, though. (This is not usually a big
> problem, but there are some cases where it's useful.)
> 
> That being said, I don't think 'select' is enough of a problem that
> you should feel the need to refactor in this way just to avoid it.

Oh, yes I didn't want to imply that this was the main reason but just
pointed out that wouldn't even be needed if done that way. And it is
something that we want to do anyway IMO, since as mentioned it would
allow to test the static functions, which are the majority the format
helpers in that file.

> Given most of the other DRM drivers (as well as DRM_DEBUG_SELFTEST)
> are select-ing DRM_KMS_HELPER, it seems like a sensible enough thing
> to continue doing for the KUnit test. As Daniel pointed out, as a
> hidden option it was clearly always meant to be select-ed anyway.
>

Yes, it can be done from the DRM_KUNIT_TEST symbol or just have it set
(and any other needed DRM helper libraries tested by other suites) in
the .kunitconfig file.

I don't think is that important, since at the end that dependency will
have to be maintained in some place.
 
> Cheers,
> -- David
> 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Javier Martinez Canillas]

On 6/2/22 19:12, David Gow wrote:
> On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote:

[snip]

>>
>> A .kuniconfig example is present in the cover letter. (...)
>
> FYI: it's also possible to run these tests under UML with the extra options:
> CONFIG_VIRTIO_UML=y
> CONFIG_UML_PCI_OVER_VIRTIO=y
>

Oh, very interesting. I didn't notice before that his example had --arch=x86_64

> I suspect it's probably better not to add those to your .kunitconfig,
> as they're UML-specific and will therefore break other architectures,
> but it does mean the tests can be run with, for example:
> 
> ./tools/testing/kunit/kunit.py run --kunitconfig
> drivers/gpu/drm/.kunitconfig --kconfig_add CONFIG_VIRTIO_UML=y
> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> 
> Or, without the .kunitconfig:
> ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y
> --kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add
> CONFIG_VIRTIO_UML=y  --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> 'drm-*'
>

I wonder if would make sense to have for example an arch/um/.kunitconfig
with those symbols and maybe others and then the tests could also be run
with something like:

./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig \
--kunitconfig=arch/um/.kunitconfig

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Javier Martinez Canillas]

On 6/2/22 19:12, David Gow wrote:
> On Mon, May 30, 2022 at 9:29 AM José Expósito <jose.exposito89@gmail.com> wrote:

[snip]

>>
>> A .kuniconfig example is present in the cover letter. (...)
>
> FYI: it's also possible to run these tests under UML with the extra options:
> CONFIG_VIRTIO_UML=y
> CONFIG_UML_PCI_OVER_VIRTIO=y
>

Oh, very interesting. I didn't notice before that his example had --arch=x86_64

> I suspect it's probably better not to add those to your .kunitconfig,
> as they're UML-specific and will therefore break other architectures,
> but it does mean the tests can be run with, for example:
> 
> ./tools/testing/kunit/kunit.py run --kunitconfig
> drivers/gpu/drm/.kunitconfig --kconfig_add CONFIG_VIRTIO_UML=y
> --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> 
> Or, without the .kunitconfig:
> ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y
> --kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add
> CONFIG_VIRTIO_UML=y  --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> 'drm-*'
>

I wonder if would make sense to have for example an arch/um/.kunitconfig
with those symbols and maybe others and then the tests could also be run
with something like:

./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig \
--kunitconfig=arch/um/.kunitconfig

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Thu, Jun 2, 2022 at 10:29 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> > Or, without the .kunitconfig:
> > ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y
> > --kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add
> > CONFIG_VIRTIO_UML=y  --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> > 'drm-*'
> >
>
> I wonder if would make sense to have for example an arch/um/.kunitconfig
> with those symbols and maybe others and then the tests could also be run
> with something like:
>
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig \
> --kunitconfig=arch/um/.kunitconfig

Yeah, this came up before.
It'd be nice to have
* --kunitconfig be repeatable (it isn't right now)
* a "uml_pci.config" with the magic needed to set CONFIG_PCI=y on UML

Another example where this would be useful, coverage on UML
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_INFO=y
CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y
CONFIG_GCOV=y

I did prototype the changes to support this but never sent anything
out since I had some concerns, namely:
1. we'd be blindly appending them, but that won't always work. Would
users be ok with that?
2. people are already confused about .kunitconfig. It seems like the
most confusing part to new people, especially those new to kernel
development. Would adding this make them even more lost? Perhaps
making the docs clearer on this would a good pre-req.
3. What conventions should there be around these partial configs? I
think the idea should be more generic than just kunit.
4. --kconfig_add now makes this possible, even if in a noisier way
than another --kunitconfig
5. we didn't have a good way of reporting options set to different
values. https://lore.kernel.org/linux-kselftest/20220520224200.3764027-1-dlatypov@google.com/
would help by giving us an easier way to give clearer error messages.

That said, I'm willing to whip up another version based on the patch in #5.
I think we need a docs rewrite for #2, which I can take a stab at.
But I think we'll need some bikeshedding about naming (#3).

Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[Daniel Latypov]

On Thu, Jun 2, 2022 at 10:29 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> > Or, without the .kunitconfig:
> > ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_DRM=y
> > --kconfig_add CONFIG_DRM_FORMAR_HELPER_TEST=y --kconfig_add
> > CONFIG_VIRTIO_UML=y  --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> > 'drm-*'
> >
>
> I wonder if would make sense to have for example an arch/um/.kunitconfig
> with those symbols and maybe others and then the tests could also be run
> with something like:
>
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig \
> --kunitconfig=arch/um/.kunitconfig

Yeah, this came up before.
It'd be nice to have
* --kunitconfig be repeatable (it isn't right now)
* a "uml_pci.config" with the magic needed to set CONFIG_PCI=y on UML

Another example where this would be useful, coverage on UML
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_INFO=y
CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y
CONFIG_GCOV=y

I did prototype the changes to support this but never sent anything
out since I had some concerns, namely:
1. we'd be blindly appending them, but that won't always work. Would
users be ok with that?
2. people are already confused about .kunitconfig. It seems like the
most confusing part to new people, especially those new to kernel
development. Would adding this make them even more lost? Perhaps
making the docs clearer on this would a good pre-req.
3. What conventions should there be around these partial configs? I
think the idea should be more generic than just kunit.
4. --kconfig_add now makes this possible, even if in a noisier way
than another --kunitconfig
5. we didn't have a good way of reporting options set to different
values. https://lore.kernel.org/linux-kselftest/20220520224200.3764027-1-dlatypov@google.com/
would help by giving us an easier way to give clearer error messages.

That said, I'm willing to whip up another version based on the patch in #5.
I think we need a docs rewrite for #2, which I can take a stab at.
But I think we'll need some bikeshedding about naming (#3).

Daniel

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hello everyone,

On Thu, Jun 02, 2022 at 07:21:28PM +0200, Javier Martinez Canillas wrote:
> Hello David,
> 
> On 6/2/22 19:07, David Gow wrote:
> > On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
> 
> [snip]
> 
> >>
> >> And doing that will also allow you to get rid of this, since just selecting
> >> CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.
> >>
> > 
> > This is definitely something other KUnit tests (apparmor, elf, etc)
> > are doing, and it's generally fine. You do lose the ability to build
> > the tests as a separate module, though. (This is not usually a big
> > problem, but there are some cases where it's useful.)
> > 
> > That being said, I don't think 'select' is enough of a problem that
> > you should feel the need to refactor in this way just to avoid it.
> 
> Oh, yes I didn't want to imply that this was the main reason but just
> pointed out that wouldn't even be needed if done that way. And it is
> something that we want to do anyway IMO, since as mentioned it would
> allow to test the static functions, which are the majority the format
> helpers in that file.

Conversion functions alway call drm_fb_xfrm()/drm_fb_xfrm_toio() and
their *_line function. For example, drm_fb_xrgb8888_to_rgb332() calls
drm_fb_xfrm() and drm_fb_xrgb8888_to_rgb332_line().

The current tests already check that the *_line() function works as
expected. I'd like to test the high-level functions first and, if
required, go into more detail in the future. The refactor is pretty
easy, so I'd prefer to keep it as it is for the moment.

About the other changes suggested, I applied all of them over the
weekend. I'll send v1 of the patch to the mailing list including them
so we have an up to date code to comment on.

Thanks a lot for all of your comments and help,
José Expósito

Re: [RFC PATCH 1/1] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()

[José Expósito]

Hello everyone,

On Thu, Jun 02, 2022 at 07:21:28PM +0200, Javier Martinez Canillas wrote:
> Hello David,
> 
> On 6/2/22 19:07, David Gow wrote:
> > On Thu, Jun 2, 2022 at 9:27 AM Javier Martinez Canillas
> 
> [snip]
> 
> >>
> >> And doing that will also allow you to get rid of this, since just selecting
> >> CONFIG_DRM_KUNIT_TEST=y would be enough for the tests built and run by KUnit.
> >>
> > 
> > This is definitely something other KUnit tests (apparmor, elf, etc)
> > are doing, and it's generally fine. You do lose the ability to build
> > the tests as a separate module, though. (This is not usually a big
> > problem, but there are some cases where it's useful.)
> > 
> > That being said, I don't think 'select' is enough of a problem that
> > you should feel the need to refactor in this way just to avoid it.
> 
> Oh, yes I didn't want to imply that this was the main reason but just
> pointed out that wouldn't even be needed if done that way. And it is
> something that we want to do anyway IMO, since as mentioned it would
> allow to test the static functions, which are the majority the format
> helpers in that file.

Conversion functions alway call drm_fb_xfrm()/drm_fb_xfrm_toio() and
their *_line function. For example, drm_fb_xrgb8888_to_rgb332() calls
drm_fb_xfrm() and drm_fb_xrgb8888_to_rgb332_line().

The current tests already check that the *_line() function works as
expected. I'd like to test the high-level functions first and, if
required, go into more detail in the future. The refactor is pretty
easy, so I'd prefer to keep it as it is for the moment.

About the other changes suggested, I applied all of them over the
weekend. I'll send v1 of the patch to the mailing list including them
so we have an up to date code to comment on.

Thanks a lot for all of your comments and help,
José Expósito