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

[Jocelyn Falempe <jfalempe@redhat.com>]

On 10/09/2025 10:33, Maxime Ripard wrote:
> 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?

Yes, I can rework this.
> 
>> +	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?

It tests that there are no access to an unmapped page.
But I can add the same check that with the "map" case.
It just requires more work to map the pages.

> 
>> + */
>> +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.
> 
yes, I can fix that, but is it important to not leak when the test fails?

-- 

Jocelyn

> Maxime