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

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

[Jocelyn Falempe]

This series adds some kunit tests to drm_panic, and a debugfs interface to easily test the panic screen rendering at different resolutions/pixel format.

The kunit tests draws the panic screens to different framebuffer size and format, and ensure it doesn't crash or draw outside of the buffer.
However it doesn't check the resulting image, because it depends on other Kconfig options, like logo, fonts, or panic colors.

v2:
 * Use debugfs instead of sending the framebuffer through the kunit logs. (Thomas Zimmermann).
 * Add a few checks, and more comments in the kunit tests. (Maxime Ripard).

Jocelyn Falempe (3):
  drm/panic: Rename draw_panic_static_* to draw_panic_screen_*
  drm/panic: Add kunit tests for drm_panic
  drm/panic: Add a drm_panic/draw_test in debugfs

 MAINTAINERS                            |   1 +
 drivers/gpu/drm/Kconfig                |   2 +
 drivers/gpu/drm/drm_panic.c            | 150 +++++++++++++++++--
 drivers/gpu/drm/tests/drm_panic_test.c | 198 +++++++++++++++++++++++++
 4 files changed, 336 insertions(+), 15 deletions(-)
 create mode 100644 drivers/gpu/drm/tests/drm_panic_test.c


base-commit: 685e8dae19df73d5400734ee5ad9e96470f9c0b4
-- 
2.51.0

[PATCH v2 1/3] drm/panic: Rename draw_panic_static_* to draw_panic_screen_*

[Jocelyn Falempe]

I called them "static" because the panic screen is drawn only once,
but this can be confused with the static meaning in C.
Also remove some unnecessary braces in draw_panic_dispatch().
No functionnal change.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_panic.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 1d6312fa1429..1e06e3a18d09 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -437,7 +437,7 @@ static void drm_panic_logo_draw(struct drm_scanout_buffer *sb, struct drm_rect *
 				   fg_color);
 }
 
-static void draw_panic_static_user(struct drm_scanout_buffer *sb)
+static void draw_panic_screen_user(struct drm_scanout_buffer *sb)
 {
 	u32 fg_color = drm_draw_color_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR,
 						    sb->format->format);
@@ -506,7 +506,7 @@ static int draw_line_with_wrap(struct drm_scanout_buffer *sb, const struct font_
  * Draw the kmsg buffer to the screen, starting from the youngest message at the bottom,
  * and going up until reaching the top of the screen.
  */
-static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb)
+static void draw_panic_screen_kmsg(struct drm_scanout_buffer *sb)
 {
 	u32 fg_color = drm_draw_color_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR,
 						    sb->format->format);
@@ -694,7 +694,7 @@ static int drm_panic_get_qr_code(u8 **qr_image)
 /*
  * Draw the panic message at the center of the screen, with a QR Code
  */
-static int _draw_panic_static_qr_code(struct drm_scanout_buffer *sb)
+static int _draw_panic_screen_qr_code(struct drm_scanout_buffer *sb)
 {
 	u32 fg_color = drm_draw_color_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR,
 						    sb->format->format);
@@ -759,15 +759,15 @@ static int _draw_panic_static_qr_code(struct drm_scanout_buffer *sb)
 	return 0;
 }
 
-static void draw_panic_static_qr_code(struct drm_scanout_buffer *sb)
+static void draw_panic_screen_qr_code(struct drm_scanout_buffer *sb)
 {
-	if (_draw_panic_static_qr_code(sb))
-		draw_panic_static_user(sb);
+	if (_draw_panic_screen_qr_code(sb))
+		draw_panic_screen_user(sb);
 }
 #else
-static void draw_panic_static_qr_code(struct drm_scanout_buffer *sb)
+static void draw_panic_screen_qr_code(struct drm_scanout_buffer *sb)
 {
-	draw_panic_static_user(sb);
+	draw_panic_screen_user(sb);
 }
 
 static void drm_panic_qr_init(void) {};
@@ -790,13 +790,12 @@ static bool drm_panic_is_format_supported(const struct drm_format_info *format)
 
 static void draw_panic_dispatch(struct drm_scanout_buffer *sb)
 {
-	if (!strcmp(drm_panic_screen, "kmsg")) {
-		draw_panic_static_kmsg(sb);
-	} else if (!strcmp(drm_panic_screen, "qr_code")) {
-		draw_panic_static_qr_code(sb);
-	} else {
-		draw_panic_static_user(sb);
-	}
+	if (!strcmp(drm_panic_screen, "kmsg"))
+		draw_panic_screen_kmsg(sb);
+	else if (!strcmp(drm_panic_screen, "qr_code"))
+		draw_panic_screen_qr_code(sb);
+	else
+		draw_panic_screen_user(sb);
 }
 
 static void drm_panic_set_description(const char *description)
-- 
2.51.0

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

[Jocelyn Falempe]

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
+	vfree(fb);
+}
+
+/*
+ * Test drawing the panic screen, using a list of pages framebuffer
+ * No checks are performed
+ */
+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]);
+	}
+	sb->pages = pages;
+	sb->width = params->width;
+	sb->height = params->height;
+	sb->pitch[0] = params->width * sb->format->cpp[0];
+
+	params->draw_screen(sb);
+
+	for (i = 0; i < npages; i++)
+		__free_page(pages[i]);
+	kfree(pages);
+}
+
+static void drm_test_panic_set_pixel(struct drm_scanout_buffer *sb,
+				     unsigned int x,
+				     unsigned int y,
+				     u32 color)
+{
+	struct kunit *test = (struct kunit *) sb->private;
+
+	KUNIT_ASSERT_TRUE(test, x < sb->width && y < sb->height);
+}
+
+/*
+ * Test drawing the panic screen, using the set_pixel callback
+ * Check that all calls to set_pixel() are within the framebuffer
+ */
+static void drm_test_panic_screen_user_set_pixel(struct kunit *test)
+{
+	struct drm_scanout_buffer *sb = test->priv;
+	const struct drm_test_mode *params = test->param_value;
+
+	sb->format = drm_format_info(params->format);
+	sb->set_pixel = drm_test_panic_set_pixel;
+	sb->width = params->width;
+	sb->height = params->height;
+	sb->private = test;
+
+	params->draw_screen(sb);
+}
+
+static void drm_test_panic_desc(const struct drm_test_mode *t, char *desc)
+{
+	sprintf(desc, "Panic screen %s, mode: %d x %d \t%p4cc",
+		t->fname, t->width, t->height, &t->format);
+}
+
+KUNIT_ARRAY_PARAM(drm_test_panic_screen_user_map, drm_test_modes_cases, drm_test_panic_desc);
+KUNIT_ARRAY_PARAM(drm_test_panic_screen_user_page, drm_test_modes_cases, drm_test_panic_desc);
+KUNIT_ARRAY_PARAM(drm_test_panic_screen_user_set_pixel, drm_test_modes_cases, drm_test_panic_desc);
+
+static struct kunit_case drm_panic_screen_user_test[] = {
+	KUNIT_CASE_PARAM(drm_test_panic_screen_user_map,
+			 drm_test_panic_screen_user_map_gen_params),
+	KUNIT_CASE_PARAM(drm_test_panic_screen_user_page,
+			 drm_test_panic_screen_user_page_gen_params),
+	KUNIT_CASE_PARAM(drm_test_panic_screen_user_set_pixel,
+			 drm_test_panic_screen_user_set_pixel_gen_params),
+	{ }
+};
+
+static struct kunit_suite drm_panic_suite = {
+	.name = "drm_panic",
+	.init = drm_test_panic_init,
+	.test_cases = drm_panic_screen_user_test,
+};
+
+kunit_test_suite(drm_panic_suite);
-- 
2.51.0

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

[Maxime Ripard]

[-- 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 --]

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

[Jocelyn Falempe]

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

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

[Maxime Ripard]

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

On Wed, Sep 10, 2025 at 05:16:49PM +0200, Jocelyn Falempe wrote:
> 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.

I wasn't really arguing about adding more stuff, just that the
documentation didn't really explain what was going on. Just saying "I'm
checking that doing this succeeds" is definitely enough for me.

> > 
> > > + */
> > > +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?

kunit tests can be compiled as module and run on live systems, so yes.
It can also lead to subsequent test failures if you deplete the system
of a resource the next test will need.

Maxime

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

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

[Jocelyn Falempe]

On 23/09/2025 11:57, Maxime Ripard wrote:
> On Wed, Sep 10, 2025 at 05:16:49PM +0200, Jocelyn Falempe wrote:
>> 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.
> 
> I wasn't really arguing about adding more stuff, just that the
> documentation didn't really explain what was going on. Just saying "I'm
> checking that doing this succeeds" is definitely enough for me.
> 
Ok, I will update the documentation accordingly.

>>>
>>>> + */
>>>> +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?
> 
> kunit tests can be compiled as module and run on live systems, so yes.
> It can also lead to subsequent test failures if you deplete the system
> of a resource the next test will need.

ok, understood, I will fix the leaks in those tests.

> Maxime


-- 

Jocelyn

[PATCH v2 3/3] drm/panic: Add a drm_panic/draw_test in debugfs

[Jocelyn Falempe]

This adds a new drm_panic/draw_test file in debugfs.
This file allows to test the panic screen rendering at different
resolution and pixel format.
It's useful only for kernel developers that want to create or
customize a panic screen.

If you want to check the result at 1024x768 using XRGB8888:

cd /sys/kernel/debug/drm_panic/
exec 3<> draw_test
echo 1024x768:XR24 >&3
cat <&3 > ~/panic_screen.raw
exec 3<&-

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---

v2:
 * Use debugfs instead of sending the framebuffer through the kunit logs. (Thomas Zimmermann).

 drivers/gpu/drm/Kconfig     |   2 +
 drivers/gpu/drm/drm_panic.c | 117 ++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f7ea8e895c0c..0d3146070d9c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -83,6 +83,8 @@ config DRM_PANIC_DEBUG
 	  Add dri/[device]/drm_panic_plane_x in the kernel debugfs, to force the
 	  panic handler to write the panic message to this plane scanout buffer.
 	  This is unsafe and should not be enabled on a production build.
+	  Also adds a drm_panic/draw_test file in debugfs, to easily test the
+	  panic screen rendering.
 	  If in doubt, say "N".
 
 config DRM_PANIC_SCREEN
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index d89812ff1935..0c01d6067eab 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -873,6 +873,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai
  */
 #ifdef CONFIG_DRM_PANIC_DEBUG
 #include <linux/debugfs.h>
+#include <linux/vmalloc.h>
 
 static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_buf,
 				     size_t count, loff_t *ppos)
@@ -901,8 +902,122 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
 	debugfs_create_file(fname, 0200, plane->dev->debugfs_root,
 			    plane, &dbg_drm_panic_ops);
 }
+
+/*
+ * Draw test interface
+ * This can be used to check the panic screen at any resolution/pixel format.
+ * The framebuffer memory is freed when the file is closed, so use this sh
+ * script to write the parameters and read the result without closing the file.
+ * cd /sys/kernel/debug/drm_panic/
+ * exec 3<> draw_test
+ * echo 1024x768:XR24 >&3
+ * cat <&3 > ~/panic_screen.raw
+ * exec 3<&-
+ */
+static ssize_t debugfs_drawtest_write(struct file *file, const char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct drm_scanout_buffer *sb = (struct drm_scanout_buffer *) file->private_data;
+	size_t fb_size;
+	void *fb;
+	char buf[64];
+	int width;
+	int height;
+	char cc1, cc2, cc3, cc4;
+	u32 drm_format;
+
+	if (count >= sizeof(buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+
+	if (sscanf(buf, "%dx%d:%c%c%c%c", &width, &height, &cc1, &cc2, &cc3, &cc4) != 6) {
+		pr_err("Invalid format. Expected: <width>x<height>:<fourcc>\n");
+		return -EINVAL;
+	}
+
+	drm_format = fourcc_code(cc1, cc2, cc3, cc4);
+	sb->format = drm_format_info(drm_format);
+	if (!sb->format)
+		return -EINVAL;
+
+	drm_panic_set_description("Test drawing from debugfs");
+
+	sb->width = width;
+	sb->height = height;
+	sb->pitch[0] = width * sb->format->cpp[0];
+
+	if (sb->map[0].vaddr)
+		vfree(sb->map[0].vaddr);
+
+	fb_size = height * sb->pitch[0];
+	fb = vmalloc(fb_size);
+	iosys_map_set_vaddr(&sb->map[0], fb);
+
+	draw_panic_dispatch(sb);
+
+	drm_panic_clear_description();
+	return count;
+}
+
+static ssize_t debugfs_drawtest_read(struct file *file, char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct drm_scanout_buffer *sb = (struct drm_scanout_buffer *) file->private_data;
+	int fb_size = sb->height * sb->pitch[0];
+
+	if (!sb->map[0].vaddr)
+		return 0;
+	return simple_read_from_buffer(user_buf, count, ppos, sb->map[0].vaddr, fb_size);
+}
+
+static int debugfs_drawtest_open(struct inode *inode, struct file *file)
+{
+	struct drm_scanout_buffer *sb = kzalloc(sizeof(*sb), GFP_KERNEL);
+
+	if (!sb)
+		return -ENOMEM;
+
+	file->private_data = sb;
+	return 0;
+}
+
+static int debugfs_drawtest_release(struct inode *inode, struct file *file)
+{
+	struct drm_scanout_buffer *sb = (struct drm_scanout_buffer *) file->private_data;
+
+	vfree(sb->map[0].vaddr);
+	kfree(sb);
+	return 0;
+}
+
+static const struct file_operations dbg_drm_panic_test_ops = {
+	.owner = THIS_MODULE,
+	.write = debugfs_drawtest_write,
+	.read = debugfs_drawtest_read,
+	.open = debugfs_drawtest_open,
+	.release = debugfs_drawtest_release,
+};
+
+static struct dentry *drm_panic_debugfs_dir;
+
+static void debugfs_register_drawtest(void)
+{
+	drm_panic_debugfs_dir = debugfs_create_dir("drm_panic", NULL);
+	debugfs_create_file("draw_test", 0600, drm_panic_debugfs_dir,
+			    NULL, &dbg_drm_panic_test_ops);
+}
+
+static void debugfs_unregister_drawtest(void)
+{
+	debugfs_remove(drm_panic_debugfs_dir);
+}
+
 #else
 static void debugfs_register_plane(struct drm_plane *plane, int index) {}
+static void debugfs_register_drawtest(void) {}
+static void debugfs_unregister_drawtest(void) {}
 #endif /* CONFIG_DRM_PANIC_DEBUG */
 
 /**
@@ -977,6 +1092,7 @@ void drm_panic_unregister(struct drm_device *dev)
 void __init drm_panic_init(void)
 {
 	drm_panic_qr_init();
+	debugfs_register_drawtest();
 }
 
 /**
@@ -985,6 +1101,7 @@ void __init drm_panic_init(void)
 void drm_panic_exit(void)
 {
 	drm_panic_qr_exit();
+	debugfs_unregister_drawtest();
 }
 
 #ifdef CONFIG_DRM_KUNIT_TEST
-- 
2.51.0

Re: [PATCH v2 3/3] drm/panic: Add a drm_panic/draw_test in debugfs

[Maxime Ripard]

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

Hi,

On Mon, Sep 08, 2025 at 11:00:31AM +0200, Jocelyn Falempe wrote:
> This adds a new drm_panic/draw_test file in debugfs.
> This file allows to test the panic screen rendering at different
> resolution and pixel format.
> It's useful only for kernel developers that want to create or
> customize a panic screen.
> 
> If you want to check the result at 1024x768 using XRGB8888:
> 
> cd /sys/kernel/debug/drm_panic/
> exec 3<> draw_test
> echo 1024x768:XR24 >&3
> cat <&3 > ~/panic_screen.raw
> exec 3<&-
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>

I see what you meant in your previous version, and I misunderstood what
you were saying, sorry.

> v2:
>  * Use debugfs instead of sending the framebuffer through the kunit logs. (Thomas Zimmermann).
> 
>  drivers/gpu/drm/Kconfig     |   2 +
>  drivers/gpu/drm/drm_panic.c | 117 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 119 insertions(+)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f7ea8e895c0c..0d3146070d9c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -83,6 +83,8 @@ config DRM_PANIC_DEBUG
>  	  Add dri/[device]/drm_panic_plane_x in the kernel debugfs, to force the
>  	  panic handler to write the panic message to this plane scanout buffer.
>  	  This is unsafe and should not be enabled on a production build.
> +	  Also adds a drm_panic/draw_test file in debugfs, to easily test the
> +	  panic screen rendering.
>  	  If in doubt, say "N".
>  
>  config DRM_PANIC_SCREEN
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index d89812ff1935..0c01d6067eab 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -873,6 +873,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai
>   */
>  #ifdef CONFIG_DRM_PANIC_DEBUG
>  #include <linux/debugfs.h>
> +#include <linux/vmalloc.h>
>  
>  static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_buf,
>  				     size_t count, loff_t *ppos)
> @@ -901,8 +902,122 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
>  	debugfs_create_file(fname, 0200, plane->dev->debugfs_root,
>  			    plane, &dbg_drm_panic_ops);
>  }
> +
> +/*
> + * Draw test interface
> + * This can be used to check the panic screen at any resolution/pixel format.
> + * The framebuffer memory is freed when the file is closed, so use this sh
> + * script to write the parameters and read the result without closing the file.
> + * cd /sys/kernel/debug/drm_panic/
> + * exec 3<> draw_test
> + * echo 1024x768:XR24 >&3
> + * cat <&3 > ~/panic_screen.raw
> + * exec 3<&-
> + */

This should be documented properly, and I'm also kind of wondering how
that would fit in the larger testing ecosystem.

Ie, how can someone that just starts contributing to Linux, or is
setting up a CI platform, can have that test running.

kunit is great for that, kselftests to some extent too, but I'm not sure
an ad-hoc interface is.

Unless we create IGT tests for it too maybe?

Maxime

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

Re: [PATCH v2 3/3] drm/panic: Add a drm_panic/draw_test in debugfs

[Jocelyn Falempe]

On 10/09/2025 12:49, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Sep 08, 2025 at 11:00:31AM +0200, Jocelyn Falempe wrote:
>> This adds a new drm_panic/draw_test file in debugfs.
>> This file allows to test the panic screen rendering at different
>> resolution and pixel format.
>> It's useful only for kernel developers that want to create or
>> customize a panic screen.
>>
>> If you want to check the result at 1024x768 using XRGB8888:
>>
>> cd /sys/kernel/debug/drm_panic/
>> exec 3<> draw_test
>> echo 1024x768:XR24 >&3
>> cat <&3 > ~/panic_screen.raw
>> exec 3<&-
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> I see what you meant in your previous version, and I misunderstood what
> you were saying, sorry.
> 
>> v2:
>>   * Use debugfs instead of sending the framebuffer through the kunit logs. (Thomas Zimmermann).
>>
>>   drivers/gpu/drm/Kconfig     |   2 +
>>   drivers/gpu/drm/drm_panic.c | 117 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 119 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index f7ea8e895c0c..0d3146070d9c 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -83,6 +83,8 @@ config DRM_PANIC_DEBUG
>>   	  Add dri/[device]/drm_panic_plane_x in the kernel debugfs, to force the
>>   	  panic handler to write the panic message to this plane scanout buffer.
>>   	  This is unsafe and should not be enabled on a production build.
>> +	  Also adds a drm_panic/draw_test file in debugfs, to easily test the
>> +	  panic screen rendering.
>>   	  If in doubt, say "N".
>>   
>>   config DRM_PANIC_SCREEN
>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>> index d89812ff1935..0c01d6067eab 100644
>> --- a/drivers/gpu/drm/drm_panic.c
>> +++ b/drivers/gpu/drm/drm_panic.c
>> @@ -873,6 +873,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai
>>    */
>>   #ifdef CONFIG_DRM_PANIC_DEBUG
>>   #include <linux/debugfs.h>
>> +#include <linux/vmalloc.h>
>>   
>>   static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_buf,
>>   				     size_t count, loff_t *ppos)
>> @@ -901,8 +902,122 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
>>   	debugfs_create_file(fname, 0200, plane->dev->debugfs_root,
>>   			    plane, &dbg_drm_panic_ops);
>>   }
>> +
>> +/*
>> + * Draw test interface
>> + * This can be used to check the panic screen at any resolution/pixel format.
>> + * The framebuffer memory is freed when the file is closed, so use this sh
>> + * script to write the parameters and read the result without closing the file.
>> + * cd /sys/kernel/debug/drm_panic/
>> + * exec 3<> draw_test
>> + * echo 1024x768:XR24 >&3
>> + * cat <&3 > ~/panic_screen.raw
>> + * exec 3<&-
>> + */
> 
> This should be documented properly, and I'm also kind of wondering how
> that would fit in the larger testing ecosystem.
> 
> Ie, how can someone that just starts contributing to Linux, or is
> setting up a CI platform, can have that test running.
> 
> kunit is great for that, kselftests to some extent too, but I'm not sure
> an ad-hoc interface is.

It's a bit harder to setup, but also allows to do some useful things.
I've written a small GUI application which displays the contents of the 
debugfs drm_panic/draw_test file in a window.
The displayed content is automatically refreshed whenever the window is 
resized, making it easy to inspect the DRM panic output at any screen 
resolution.
https://gitlab.com/kdj0c/panicviewer

> 
> Unless we create IGT tests for it too maybe?

Yes, I should also take a look at what IGT can do.

> 
> Maxime


-- 

Jocelyn

Re: [PATCH v2 3/3] drm/panic: Add a drm_panic/draw_test in debugfs

[Maxime Ripard]

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

On Thu, Sep 11, 2025 at 02:00:42PM +0200, Jocelyn Falempe wrote:
> On 10/09/2025 12:49, Maxime Ripard wrote:
> > On Mon, Sep 08, 2025 at 11:00:31AM +0200, Jocelyn Falempe wrote:
> > > This adds a new drm_panic/draw_test file in debugfs.
> > > This file allows to test the panic screen rendering at different
> > > resolution and pixel format.
> > > It's useful only for kernel developers that want to create or
> > > customize a panic screen.
> > > 
> > > If you want to check the result at 1024x768 using XRGB8888:
> > > 
> > > cd /sys/kernel/debug/drm_panic/
> > > exec 3<> draw_test
> > > echo 1024x768:XR24 >&3
> > > cat <&3 > ~/panic_screen.raw
> > > exec 3<&-
> > > 
> > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> > 
> > I see what you meant in your previous version, and I misunderstood what
> > you were saying, sorry.
> > 
> > > v2:
> > >   * Use debugfs instead of sending the framebuffer through the kunit logs. (Thomas Zimmermann).
> > > 
> > >   drivers/gpu/drm/Kconfig     |   2 +
> > >   drivers/gpu/drm/drm_panic.c | 117 ++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 119 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index f7ea8e895c0c..0d3146070d9c 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -83,6 +83,8 @@ config DRM_PANIC_DEBUG
> > >   	  Add dri/[device]/drm_panic_plane_x in the kernel debugfs, to force the
> > >   	  panic handler to write the panic message to this plane scanout buffer.
> > >   	  This is unsafe and should not be enabled on a production build.
> > > +	  Also adds a drm_panic/draw_test file in debugfs, to easily test the
> > > +	  panic screen rendering.
> > >   	  If in doubt, say "N".
> > >   config DRM_PANIC_SCREEN
> > > diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> > > index d89812ff1935..0c01d6067eab 100644
> > > --- a/drivers/gpu/drm/drm_panic.c
> > > +++ b/drivers/gpu/drm/drm_panic.c
> > > @@ -873,6 +873,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai
> > >    */
> > >   #ifdef CONFIG_DRM_PANIC_DEBUG
> > >   #include <linux/debugfs.h>
> > > +#include <linux/vmalloc.h>
> > >   static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_buf,
> > >   				     size_t count, loff_t *ppos)
> > > @@ -901,8 +902,122 @@ static void debugfs_register_plane(struct drm_plane *plane, int index)
> > >   	debugfs_create_file(fname, 0200, plane->dev->debugfs_root,
> > >   			    plane, &dbg_drm_panic_ops);
> > >   }
> > > +
> > > +/*
> > > + * Draw test interface
> > > + * This can be used to check the panic screen at any resolution/pixel format.
> > > + * The framebuffer memory is freed when the file is closed, so use this sh
> > > + * script to write the parameters and read the result without closing the file.
> > > + * cd /sys/kernel/debug/drm_panic/
> > > + * exec 3<> draw_test
> > > + * echo 1024x768:XR24 >&3
> > > + * cat <&3 > ~/panic_screen.raw
> > > + * exec 3<&-
> > > + */
> > 
> > This should be documented properly, and I'm also kind of wondering how
> > that would fit in the larger testing ecosystem.
> > 
> > Ie, how can someone that just starts contributing to Linux, or is
> > setting up a CI platform, can have that test running.
> > 
> > kunit is great for that, kselftests to some extent too, but I'm not sure
> > an ad-hoc interface is.
> 
> It's a bit harder to setup, but also allows to do some useful things.

I'm not saying that the original test has no use. I'm saying a test that
can't be discovered and run automatically is a lot less useful, and even
more so when it's using an ad-hoc interface.

Because that means that CI, all the other devs, etc. probably won't know
anything about it and you'll end up being the only one running the test.

That's why I've been insisting on a standard solution, because that
would solve that problem.

I still believe that your use-case is legit, and the test can be useful,
but it needs to be somewhat standard. Getting the opinion from the kunit
maintainers would be a great first step for example.

> I've written a small GUI application which displays the contents of the
> debugfs drm_panic/draw_test file in a window.
> The displayed content is automatically refreshed whenever the window is
> resized, making it easy to inspect the DRM panic output at any screen
> resolution.
> https://gitlab.com/kdj0c/panicviewer

And I'm sure that part is useful to you, I wonder if it's something that
should be upstream.

Maxime

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