[PATCH 0/1] KUnit tests for drm_format_helper

[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 includes the changes suggested in the RFC version [2].

Best wishes,
José Expósito

[1] https://cgit.freedesktop.org/drm/drm-misc/commit/?id=596c35b1440e
[2] https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/

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

 drivers/gpu/drm/.kunitconfig             |   3 +
 drivers/gpu/drm/Kconfig                  |  16 +++
 drivers/gpu/drm/Makefile                 |   2 +
 drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
 4 files changed, 187 insertions(+)
 create mode 100644 drivers/gpu/drm/.kunitconfig
 create mode 100644 drivers/gpu/drm/drm_format_helper_test.c

-- 
2.25.1

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

How to run the tests?

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

Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>

---

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

 - Add .kunitconfig (Maxime Ripard)
 - Fix memory leak (Daniel Latypov)
 - Make config option generic (Javier Martinez Canillas):
   DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
 - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)
---
 drivers/gpu/drm/.kunitconfig             |   3 +
 drivers/gpu/drm/Kconfig                  |  16 +++
 drivers/gpu/drm/Makefile                 |   2 +
 drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
 4 files changed, 187 insertions(+)
 create mode 100644 drivers/gpu/drm/.kunitconfig
 create mode 100644 drivers/gpu/drm/drm_format_helper_test.c

diff --git a/drivers/gpu/drm/.kunitconfig b/drivers/gpu/drm/.kunitconfig
new file mode 100644
index 000000000000..6ec04b4c979d
--- /dev/null
+++ b/drivers/gpu/drm/.kunitconfig
@@ -0,0 +1,3 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_KUNIT_TEST=y
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e88c497fa010..3c0b1faba439 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
 
 	  If in doubt, say "N".
 
+config DRM_KUNIT_TEST
+	tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
+	depends on DRM && KUNIT=y
+	select DRM_KMS_HELPER
+	default KUNIT_ALL_TESTS
+	help
+	  This builds unit tests for DRM. This option is not useful for
+	  distributions or general kernels, but only for kernel
+	  developers working on DRM and associated drivers.
+
+	  For more information on KUnit and unit tests in general,
+	  please refer to the KUnit documentation in
+	  Documentation/dev-tools/kunit/.
+
+	  If in doubt, say "N".
+
 config DRM_KMS_HELPER
 	tristate
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 15fe3163f822..6549471f09c7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -76,6 +76,8 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 #
 
 obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_KUNIT_TEST) += drm_kms_helper.o \
+		drm_format_helper_test.o
 
 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..e9302219f3f9
--- /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 = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
+
+	drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
+				  &fb, &params->clip);
+	KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
+}
+
+static struct kunit_case drm_format_helper_test_cases[] = {
+	KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
+			 xrgb8888_to_rgb332_gen_params),
+	{}
+};
+
+static struct kunit_suite drm_format_helper_test_suite = {
+	.name = "drm-format-helper-test",
+	.test_cases = drm_format_helper_test_cases,
+};
+
+kunit_test_suite(drm_format_helper_test_suite);
+
+MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
-- 
2.25.1

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

[Javier Martinez Canillas]

Hello José,

On 6/6/22 11:55, 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 known colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
> 
> How to run the tests?
> 
>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
>          --kconfig_add CONFIG_VIRTIO_UML=y \
>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> 
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> 
> ---

Thanks for addressing the issues pointed out. Patch looks good to me now.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

By the way, I think you should request an account [0], so that you can push
patches to drm-misc directly. Specially since AFAIU the plan is to add more
KUnit tests in future patch series.

[0]: https://www.freedesktop.org/wiki/AccountRequests/

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[José Expósito]

Hi!

Javier Martinez Canillas wrote:
> Hello José,
> 
> On 6/6/22 11:55, 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 known colors: White, black, red, green, blue, magenta, yellow
> >    and cyan
> >  - Other colors: Randomly picked
> >  - Destination pitch
> > 
> > How to run the tests?
> > 
> >  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >          --kconfig_add CONFIG_VIRTIO_UML=y \
> >          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> > 
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > 
> > ---
> 
> Thanks for addressing the issues pointed out. Patch looks good to me now.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks for the quick review Javier :)

Javier Martinez Canillas wrote:
> By the way, I think you should request an account [0], so that you can push
> patches to drm-misc directly. Specially since AFAIU the plan is to add more
> KUnit tests in future patch series.
> 
> [0]: https://www.freedesktop.org/wiki/AccountRequests/

I'll request one, thanks for the tip.

-------

Maxime Ripard wrote:
> > > The following works correctly but it won't use User Mode Linux:
> > >
> > > ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
> > >
> > > But then, can't we add them to .kunitconfig?
> > >
> >
> > That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
> > go there because is platform specific (AFAIU, one might want to test it on x86,
> > aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.
> >
> > The answer was that's not that simple and some agreement on how to do it is needed:
> >
> > https://lists.freedesktop.org/archives/dri-devel/2022-June/357617.html
>
> We should probably just document it somewhere in KMS then? It doesn't
> have to be in this patch series, but I have the feeling that we will end
> up with that discussion a lot from people frustrated to have spent too
> much time figuring it out :)

My understanding from Daniel's comment [1] is also that at the moment
it is not easy to support this use case, so yes, at least copy and
pasting the command in the docs should help everyone figure out how to
run the tests.

Documentation/gpu/drm-internals.rst seems like a good place to add some
information about how to run and add tests.
I'll send a patch with the docs ASAP.

Jose

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

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

[Maxime Ripard]

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

Hi,

On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
>    and cyan
>  - Other colors: Randomly picked
>  - Destination pitch
> 
> How to run the tests?
> 
>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
>          --kconfig_add CONFIG_VIRTIO_UML=y \
>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y

It's not clear to me why you would need VIRTIO here? The Kunit config
file should be enough to run the tests properly

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Javier Martinez Canillas]

Hello Maxime,

On 6/6/22 15:42, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
>>    and cyan
>>  - Other colors: Randomly picked
>>  - Destination pitch
>>
>> How to run the tests?
>>
>>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
>>          --kconfig_add CONFIG_VIRTIO_UML=y \
>>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> 
> It's not clear to me why you would need VIRTIO here? The Kunit config
> file should be enough to run the tests properly
>

It's needed or otherwise KUnit will complain with:

./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
[15:47:31] Configuring KUnit Kernel ...
Regenerating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
This is probably due to unsatisfied dependencies.
Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".

The following works correctly but it won't use User Mode Linux:

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

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[Maxime Ripard]

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

hi,

On Mon, Jun 06, 2022 at 03:49:57PM +0200, Javier Martinez Canillas wrote:
> Hello Maxime,
> 
> On 6/6/22 15:42, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
> >>    and cyan
> >>  - Other colors: Randomly picked
> >>  - Destination pitch
> >>
> >> How to run the tests?
> >>
> >>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >>          --kconfig_add CONFIG_VIRTIO_UML=y \
> >>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> > 
> > It's not clear to me why you would need VIRTIO here? The Kunit config
> > file should be enough to run the tests properly
> >
> 
> It's needed or otherwise KUnit will complain with:
> 
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
> [15:47:31] Configuring KUnit Kernel ...
> Regenerating .config ...
> Populating config with:
> $ make ARCH=um O=.kunit olddefconfig
> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> This is probably due to unsatisfied dependencies.
> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> 
> The following works correctly but it won't use User Mode Linux:
> 
> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64

But then, can't we add them to .kunitconfig?

We should avoid that gotcha entirely

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Javier Martinez Canillas]

Hello Maxime,

On 6/6/22 15:52, Maxime Ripard wrote:
> hi,
> 
> On Mon, Jun 06, 2022 at 03:49:57PM +0200, Javier Martinez Canillas wrote:
>> Hello Maxime,
>>
>> On 6/6/22 15:42, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
>>>>    and cyan
>>>>  - Other colors: Randomly picked
>>>>  - Destination pitch
>>>>
>>>> How to run the tests?
>>>>
>>>>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
>>>>          --kconfig_add CONFIG_VIRTIO_UML=y \
>>>>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
>>>
>>> It's not clear to me why you would need VIRTIO here? The Kunit config
>>> file should be enough to run the tests properly
>>>
>>
>> It's needed or otherwise KUnit will complain with:
>>
>> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
>> [15:47:31] Configuring KUnit Kernel ...
>> Regenerating .config ...
>> Populating config with:
>> $ make ARCH=um O=.kunit olddefconfig
>> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
>> This is probably due to unsatisfied dependencies.
>> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
>> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
>>
>> The following works correctly but it won't use User Mode Linux:
>>
>> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
> 
> But then, can't we add them to .kunitconfig?
>

That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
go there because is platform specific (AFAIU, one might want to test it on x86,
aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.

The answer was that's not that simple and some agreement on how to do it is needed:

https://lists.freedesktop.org/archives/dri-devel/2022-June/357617.html

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[Maxime Ripard]

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

On Mon, Jun 06, 2022 at 03:57:45PM +0200, Javier Martinez Canillas wrote:
> Hello Maxime,
> 
> On 6/6/22 15:52, Maxime Ripard wrote:
> > hi,
> > 
> > On Mon, Jun 06, 2022 at 03:49:57PM +0200, Javier Martinez Canillas wrote:
> >> Hello Maxime,
> >>
> >> On 6/6/22 15:42, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> On Mon, Jun 06, 2022 at 11:55:16AM +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 known colors: White, black, red, green, blue, magenta, yellow
> >>>>    and cyan
> >>>>  - Other colors: Randomly picked
> >>>>  - Destination pitch
> >>>>
> >>>> How to run the tests?
> >>>>
> >>>>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >>>>          --kconfig_add CONFIG_VIRTIO_UML=y \
> >>>>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >>>
> >>> It's not clear to me why you would need VIRTIO here? The Kunit config
> >>> file should be enough to run the tests properly
> >>>
> >>
> >> It's needed or otherwise KUnit will complain with:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
> >> [15:47:31] Configuring KUnit Kernel ...
> >> Regenerating .config ...
> >> Populating config with:
> >> $ make ARCH=um O=.kunit olddefconfig
> >> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> >> This is probably due to unsatisfied dependencies.
> >> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
> >> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> >>
> >> The following works correctly but it won't use User Mode Linux:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
> > 
> > But then, can't we add them to .kunitconfig?
> >
> 
> That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
> go there because is platform specific (AFAIU, one might want to test it on x86,
> aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.
> 
> The answer was that's not that simple and some agreement on how to do it is needed:
> 
> https://lists.freedesktop.org/archives/dri-devel/2022-June/357617.html

Thanks for pointing this out. So yeah, it's indeed not very optimal

We should probably just document it somewhere in KMS then? It doesn't
have to be in this patch series, but I have the feeling that we will end
up with that discussion a lot from people frustrated to have spent too
much time figuring it out :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Daniel Latypov]

On Mon, Jun 6, 2022 at 6:57 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
>

<snip>

> >>>>  $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >>>>          --kconfig_add CONFIG_VIRTIO_UML=y \
> >>>>          --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> >>>
> >>> It's not clear to me why you would need VIRTIO here? The Kunit config
> >>> file should be enough to run the tests properly
> >>>
> >>
> >> It's needed or otherwise KUnit will complain with:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig
> >> [15:47:31] Configuring KUnit Kernel ...
> >> Regenerating .config ...
> >> Populating config with:
> >> $ make ARCH=um O=.kunit olddefconfig
> >> ERROR:root:Not all Kconfig options selected in kunitconfig were in the generated .config.
> >> This is probably due to unsatisfied dependencies.
> >> Missing: CONFIG_DRM=y, CONFIG_DRM_KUNIT_TEST=y
> >> Note: many Kconfig options aren't available on UML. You can try running on a different architecture with something like "--arch=x86_64".
> >>
> >> The following works correctly but it won't use User Mode Linux:
> >>
> >> ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/.kunitconfig --arch=x86_64
> >
> > But then, can't we add them to .kunitconfig?
> >
>
> That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
> go there because is platform specific (AFAIU, one might want to test it on x86,

Slight correction, it was David who explicitly suggested it shouldn't
go in there.
https://lists.freedesktop.org/archives/dri-devel/2022-June/357611.html

> aarch64, etc) but then I asked why we couldn't have a arch/um/.kunitconfig.
>
> The answer was that's not that simple and some agreement on how to do it is needed:

I'm a bit more in favor of having UML-specific options in the drm
.kunitconfig file, but I agree it's a bit unclear.
If people want to easily run with --arch=x86_64 or others, then
they're indeed a liability.

Another option is to perhaps explicitly name the .kunitconfig file
something like drm/uml.kunitconfig, which doesn't solve the problem
but makes it less of a footgun.

Stepping back, I feel like perhaps a cleaner answer lies in adding a
new Kconfig option that selects CONFIG_UML_PCI_OVER_VIRTIO under UML
and just CONFIG_PCI otherwise.
But that's a bigger discussion still.

Daniel

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

[Javier Martinez Canillas]

Hello Daniel,

On 6/6/22 16:19, Daniel Latypov wrote:

[snip]

>> That's what I asked in the previous RFC too. Daniel mentioned that it shouldn't
>> go there because is platform specific (AFAIU, one might want to test it on x86,
> 
> Slight correction, it was David who explicitly suggested it shouldn't
> go in there.
> https://lists.freedesktop.org/archives/dri-devel/2022-June/357611.html
>

Ups, sorry for getting that wrong. I should had looked at the thread
again before writing my answer.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

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

[Thomas Zimmermann]

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

Hi,

ading Kunit tests for the conversion helpers is pretty cool. Thanks for 
doing that.

Am 06.06.22 um 11:55 schrieb José Expósito:
> Test the conversion from XRGB8888 to RGB332.
> 
> What is tested?
> 
>   - Different values for the X in XRGB8888 to make sure it is ignored
>   - Different clip values: Single pixel and full and partial buffer
>   - Well known colors: White, black, red, green, blue, magenta, yellow
>     and cyan
>   - Other colors: Randomly picked
>   - Destination pitch
> 
> How to run the tests?
> 
>   $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
>           --kconfig_add CONFIG_VIRTIO_UML=y \
>           --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> 
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> 
> ---
> 
> RFC -> v1: https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/
> 
>   - Add .kunitconfig (Maxime Ripard)
>   - Fix memory leak (Daniel Latypov)
>   - Make config option generic (Javier Martinez Canillas):
>     DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
>   - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)
> ---
>   drivers/gpu/drm/.kunitconfig             |   3 +
>   drivers/gpu/drm/Kconfig                  |  16 +++
>   drivers/gpu/drm/Makefile                 |   2 +
>   drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
>   4 files changed, 187 insertions(+)
>   create mode 100644 drivers/gpu/drm/.kunitconfig
>   create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> 
> diff --git a/drivers/gpu/drm/.kunitconfig b/drivers/gpu/drm/.kunitconfig
> new file mode 100644
> index 000000000000..6ec04b4c979d
> --- /dev/null
> +++ b/drivers/gpu/drm/.kunitconfig
> @@ -0,0 +1,3 @@
> +CONFIG_KUNIT=y
> +CONFIG_DRM=y
> +CONFIG_DRM_KUNIT_TEST=y
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e88c497fa010..3c0b1faba439 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
>   
>   	  If in doubt, say "N".
>   
> +config DRM_KUNIT_TEST
> +	tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> +	depends on DRM && KUNIT=y
> +	select DRM_KMS_HELPER
> +	default KUNIT_ALL_TESTS
> +	help
> +	  This builds unit tests for DRM. This option is not useful for
> +	  distributions or general kernels, but only for kernel
> +	  developers working on DRM and associated drivers.
> +
> +	  For more information on KUnit and unit tests in general,
> +	  please refer to the KUnit documentation in
> +	  Documentation/dev-tools/kunit/.
> +
> +	  If in doubt, say "N".
> +
>   config DRM_KMS_HELPER
>   	tristate
>   	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 15fe3163f822..6549471f09c7 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -76,6 +76,8 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
>   #
>   
>   obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_kms_helper.o \

You already selected DRM_KMS_HELPER in Kconfig. Why do you need to list 
the module here?

> +		drm_format_helper_test.o

One comment about source-code organization:

There is potentially a long list of test files that will contain unit 
tests. I would prefer to put the unit tests into their own subdirectory 
(e.g., kunit).

>   
>   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..e9302219f3f9
> --- /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) }

I have long wished for an initializer macro for drm_rect.
Please rename that CLIP macro to DRM_RECT_INIT and put it into 
<drm/drm_rect.h> with docs.

> +
> +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[] = {

The names of these tests are only mildly descriptive. Maybe add a 
single-line comment before each test case to describe what it does. Your 
commit description has a nice list of tests, which you can copy here 
almost as-is.

> +	{
> +		.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.
> + */

You don't need to document internal functions with formatted comments. 
It will only confuse readers of the generated documentation. If you 
don't want to outright remove the comment, at least remove the /** at 
the top.

Best regards
Thomas

> +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 = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> +
> +	drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
> +				  &fb, &params->clip);
> +	KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> +}
> +
> +static struct kunit_case drm_format_helper_test_cases[] = {
> +	KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
> +			 xrgb8888_to_rgb332_gen_params),
> +	{}
> +};
> +
> +static struct kunit_suite drm_format_helper_test_suite = {
> +	.name = "drm-format-helper-test",
> +	.test_cases = drm_format_helper_test_cases,
> +};
> +
> +kunit_test_suite(drm_format_helper_test_suite);
> +
> +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");

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

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

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

[José Expósito]

Hi Thomas,

Thanks a lot for your review.

On Tue, Jun 07, 2022 at 09:22:38AM +0200, Thomas Zimmermann wrote:
> Hi,
> 
> ading Kunit tests for the conversion helpers is pretty cool. Thanks for
> doing that.
> 
> Am 06.06.22 um 11:55 schrieb José Expósito:
> > Test the conversion from XRGB8888 to RGB332.
> > 
> > What is tested?
> > 
> >   - Different values for the X in XRGB8888 to make sure it is ignored
> >   - Different clip values: Single pixel and full and partial buffer
> >   - Well known colors: White, black, red, green, blue, magenta, yellow
> >     and cyan
> >   - Other colors: Randomly picked
> >   - Destination pitch
> > 
> > How to run the tests?
> > 
> >   $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm \
> >           --kconfig_add CONFIG_VIRTIO_UML=y \
> >           --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
> > 
> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > 
> > ---
> > 
> > RFC -> v1: https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposito89@gmail.com/T/
> > 
> >   - Add .kunitconfig (Maxime Ripard)
> >   - Fix memory leak (Daniel Latypov)
> >   - Make config option generic (Javier Martinez Canillas):
> >     DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST
> >   - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov)
> > ---
> >   drivers/gpu/drm/.kunitconfig             |   3 +
> >   drivers/gpu/drm/Kconfig                  |  16 +++
> >   drivers/gpu/drm/Makefile                 |   2 +
> >   drivers/gpu/drm/drm_format_helper_test.c | 166 +++++++++++++++++++++++
> >   4 files changed, 187 insertions(+)
> >   create mode 100644 drivers/gpu/drm/.kunitconfig
> >   create mode 100644 drivers/gpu/drm/drm_format_helper_test.c
> > 
> > diff --git a/drivers/gpu/drm/.kunitconfig b/drivers/gpu/drm/.kunitconfig
> > new file mode 100644
> > index 000000000000..6ec04b4c979d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/.kunitconfig
> > @@ -0,0 +1,3 @@
> > +CONFIG_KUNIT=y
> > +CONFIG_DRM=y
> > +CONFIG_DRM_KUNIT_TEST=y
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index e88c497fa010..3c0b1faba439 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
> >   	  If in doubt, say "N".
> > +config DRM_KUNIT_TEST
> > +	tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
> > +	depends on DRM && KUNIT=y
> > +	select DRM_KMS_HELPER
> > +	default KUNIT_ALL_TESTS
> > +	help
> > +	  This builds unit tests for DRM. This option is not useful for
> > +	  distributions or general kernels, but only for kernel
> > +	  developers working on DRM and associated drivers.
> > +
> > +	  For more information on KUnit and unit tests in general,
> > +	  please refer to the KUnit documentation in
> > +	  Documentation/dev-tools/kunit/.
> > +
> > +	  If in doubt, say "N".
> > +
> >   config DRM_KMS_HELPER
> >   	tristate
> >   	depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 15fe3163f822..6549471f09c7 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -76,6 +76,8 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >   #
> >   obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> > +obj-$(CONFIG_DRM_KUNIT_TEST) += drm_kms_helper.o \
> 
> You already selected DRM_KMS_HELPER in Kconfig. Why do you need to list the
> module here?

Actually, it is not required. I'll remove it in v2.

> > +		drm_format_helper_test.o
> 
> One comment about source-code organization:
> 
> There is potentially a long list of test files that will contain unit tests.
> I would prefer to put the unit tests into their own subdirectory (e.g.,
> kunit).

It makes sense, and it'd also be more consistent with selftest tests.

> >   obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
> >   obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> > diff --git a/drivers/gpu/drm/drm_format_helper_test.c b/drivers/gpu/drm/drm_format_helper_test.c
> > new file mode 100644
> > index 000000000000..e9302219f3f9
> > --- /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) }
> 
> I have long wished for an initializer macro for drm_rect.
> Please rename that CLIP macro to DRM_RECT_INIT and put it into
> <drm/drm_rect.h> with docs.

I'll include an extra patch for it on v2.
 
> > +
> > +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[] = {
> 
> The names of these tests are only mildly descriptive. Maybe add a
> single-line comment before each test case to describe what it does. Your
> commit description has a nice list of tests, which you can copy here almost
> as-is.

Ok, written down for v2.
 
> > +	{
> > +		.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.
> > + */
> 
> You don't need to document internal functions with formatted comments. It
> will only confuse readers of the generated documentation. If you don't want
> to outright remove the comment, at least remove the /** at the top.

Cool, I'll remove the extra * on v2.

Speaking about documentation, I sent a patch explaining how to run
the tests:

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

I'll send it as part of the v2 of this series.

Best wishes,
Jose


> Best regards
> Thomas
> 
> > +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 = kunit_kzalloc(test, dst_size, GFP_KERNEL);
> > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dst);
> > +
> > +	drm_fb_xrgb8888_to_rgb332(dst, params->dst_pitch, params->xrgb8888,
> > +				  &fb, &params->clip);
> > +	KUNIT_EXPECT_EQ(test, memcmp(dst, params->expected, dst_size), 0);
> > +}
> > +
> > +static struct kunit_case drm_format_helper_test_cases[] = {
> > +	KUNIT_CASE_PARAM(xrgb8888_to_rgb332_test,
> > +			 xrgb8888_to_rgb332_gen_params),
> > +	{}
> > +};
> > +
> > +static struct kunit_suite drm_format_helper_test_suite = {
> > +	.name = "drm-format-helper-test",
> > +	.test_cases = drm_format_helper_test_cases,
> > +};
> > +
> > +kunit_test_suite(drm_format_helper_test_suite);
> > +
> > +MODULE_DESCRIPTION("KUnit tests for the drm_format_helper APIs");
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("José Expósito <jose.exposito89@gmail.com>");
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev