Re: [PATCH v2 2/3] drm/panic: Add kunit tests for drm_panic

[Maxime Ripard <mripard@kernel.org>]

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

Hi,

On Mon, Sep 08, 2025 at 11:00:30AM +0200, Jocelyn Falempe wrote:
> Add kunit tests for drm_panic.
> They check that drawing the panic screen doesn't crash, but they
> don't check the correctness of the resulting image.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
> 
> v2:
>  * Add a few checks, and more comments in the kunit tests. (Maxime Ripard).
> 
>  MAINTAINERS                            |   1 +
>  drivers/gpu/drm/drm_panic.c            |   4 +
>  drivers/gpu/drm/tests/drm_panic_test.c | 198 +++++++++++++++++++++++++
>  3 files changed, 203 insertions(+)
>  create mode 100644 drivers/gpu/drm/tests/drm_panic_test.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 402fe14091f1..e9be893d6741 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8480,6 +8480,7 @@ T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
>  F:	drivers/gpu/drm/drm_draw.c
>  F:	drivers/gpu/drm/drm_draw_internal.h
>  F:	drivers/gpu/drm/drm_panic*.c
> +F:	drivers/gpu/drm/tests/drm_panic_test.c
>  F:	include/drm/drm_panic*
>  
>  DRM PANIC QR CODE
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 1e06e3a18d09..d89812ff1935 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -986,3 +986,7 @@ void drm_panic_exit(void)
>  {
>  	drm_panic_qr_exit();
>  }
> +
> +#ifdef CONFIG_DRM_KUNIT_TEST
> +#include "tests/drm_panic_test.c"
> +#endif
> diff --git a/drivers/gpu/drm/tests/drm_panic_test.c b/drivers/gpu/drm/tests/drm_panic_test.c
> new file mode 100644
> index 000000000000..d5d20dd2aa7c
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_panic_test.c
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +/*
> + * Copyright (c) 2025 Red Hat.
> + * Author: Jocelyn Falempe <jfalempe@redhat.com>
> + *
> + * KUNIT tests for drm panic
> + */
> +
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_panic.h>
> +
> +#include <kunit/test.h>
> +
> +#include <linux/units.h>
> +#include <linux/vmalloc.h>
> +
> +/* Check the framebuffer color only if the panic colors are the default */
> +#if (CONFIG_DRM_PANIC_BACKGROUND_COLOR == 0 && \
> +	CONFIG_DRM_PANIC_FOREGROUND_COLOR == 0xffffff)
> +#define DRM_PANIC_CHECK_COLOR
> +#endif
> +
> +struct drm_test_mode {
> +	const int width;
> +	const int height;
> +	const u32 format;
> +	void (*draw_screen)(struct drm_scanout_buffer *sb);
> +	const char *fname;
> +};
> +
> +/*
> + * Run all tests for the 3 panic screens: user, kmsg and qr_code
> + */
> +#define DRM_TEST_MODE_LIST(func) \
> +	DRM_PANIC_TEST_MODE(1024, 768, DRM_FORMAT_XRGB8888, func) \
> +	DRM_PANIC_TEST_MODE(300, 200, DRM_FORMAT_XRGB8888, func) \
> +	DRM_PANIC_TEST_MODE(1920, 1080, DRM_FORMAT_XRGB8888, func) \
> +	DRM_PANIC_TEST_MODE(1024, 768, DRM_FORMAT_RGB565, func) \
> +	DRM_PANIC_TEST_MODE(1024, 768, DRM_FORMAT_RGB888, func) \
> +
> +#define DRM_PANIC_TEST_MODE(w, h, f, name) { \
> +	.width = w, \
> +	.height = h, \
> +	.format = f, \
> +	.draw_screen = draw_panic_screen_##name, \
> +	.fname = #name, \
> +	}, \
> +
> +static const struct drm_test_mode drm_test_modes_cases[] = {
> +	DRM_TEST_MODE_LIST(user)
> +	DRM_TEST_MODE_LIST(kmsg)
> +	DRM_TEST_MODE_LIST(qr_code)
> +};
> +#undef DRM_PANIC_TEST_MODE
> +
> +static int drm_test_panic_init(struct kunit *test)
> +{
> +	struct drm_scanout_buffer *priv;
> +
> +	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	test->priv = priv;
> +
> +	drm_panic_set_description("Kunit testing");
> +
> +	return 0;
> +}
> +
> +/*
> + * Test drawing the panic screen, using a memory mapped framebuffer
> + * Set the whole buffer to 0xa5, and then check that all pixels have been
> + * written.
> + */
> +static void drm_test_panic_screen_user_map(struct kunit *test)
> +{
> +	struct drm_scanout_buffer *sb = test->priv;
> +	const struct drm_test_mode *params = test->param_value;
> +	char *fb;
> +	int fb_size;
> +
> +	sb->format = drm_format_info(params->format);
> +	fb_size = params->width * params->height * sb->format->cpp[0];
> +
> +	fb = vmalloc(fb_size);
> +	KUNIT_ASSERT_NOT_NULL(test, fb);
> +
> +	memset(fb, 0xa5, fb_size);
> +
> +	iosys_map_set_vaddr(&sb->map[0], fb);
> +	sb->width = params->width;
> +	sb->height = params->height;
> +	sb->pitch[0] = params->width * sb->format->cpp[0];
> +
> +	params->draw_screen(sb);
> +
> +#ifdef DRM_PANIC_CHECK_COLOR
> +	{
> +		int i;
> +
> +		for (i = 0; i < fb_size; i++)
> +			KUNIT_ASSERT_TRUE(test, fb[i] == 0 || fb[i] == 0xff);
> +	}
> +#endif

I'm not really fond of the ifdef here. Could you turn this into a
function, and return that it's valid if the colors don't match what you
expect?

> +	vfree(fb);
> +}
> +
> +/*
> + * Test drawing the panic screen, using a list of pages framebuffer
> + * No checks are performed

What are you testing then if you aren't checking anything?

> + */
> +static void drm_test_panic_screen_user_page(struct kunit *test)
> +{
> +	struct drm_scanout_buffer *sb = test->priv;
> +	const struct drm_test_mode *params = test->param_value;
> +	int fb_size;
> +	struct page **pages;
> +	int i;
> +	int npages;
> +
> +	sb->format = drm_format_info(params->format);
> +	fb_size = params->width * params->height * sb->format->cpp[0];
> +	npages = DIV_ROUND_UP(fb_size, PAGE_SIZE);
> +
> +	pages = kmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, pages);
> +
> +	for (i = 0; i < npages; i++) {
> +		pages[i] = alloc_page(GFP_KERNEL);
> +		KUNIT_ASSERT_NOT_NULL(test, pages[i]);

KUNIT_ASSERT_* return immediately, so you're leaking the pages array
here.

Maxime

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