Message-ID: <0f1984c3-7dc0-0592-47ee-7ba421914c8b@suse.de>
Date:   Tue, 7 Jun 2022 09:22:38 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     José Expósito <jose.exposito89@...il.com>,
        javierm@...hat.com
Cc:     davidgow@...gle.com, dlatypov@...gle.com,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        airlied@...ux.ie, daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
        kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] drm/format-helper: Add KUnit tests for
 drm_fb_xrgb8888_to_rgb332()

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@...hat.com>
> Signed-off-by: José Expósito <jose.exposito89@...il.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@...il.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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists