[PATCH v2 0/7] drm/panic: Fixes and graphical logo

[PATCH v2 0/7] drm/panic: Fixes and graphical logo

[Geert Uytterhoeven]

	Hi all,

If drm/panic is enabled, a user-friendly message is shown on screen when
a kernel panic occurs, together with an ASCII art penguin logo.
Of course we can do better ;-)
Hence this patch series extends drm/panic to draw the monochrome
graphical boot logo, when available, preceded by the customary fixes.

Changes compared to v1:
  - Rebase against today's drm-misc-next, where drm_panic is broken on
    all current drivers due to an uninitialized pointer dereference.
    Presumably this was only tested with an out-of-tree driver change?
  - New fixes [1/7], [3/7], and [4/7],
  - New cleanup [5/7],
  - Inline trivial draw_logo_mono().

This has been tested with rcar-du.

Thanks for your comments!

Geert Uytterhoeven (7):
  drm/panic: Fix uninitialized drm_scanout_buffer.set_pixel() crash
  drm/panic: Fix off-by-one logo size checks
  lib/fonts: Fix visiblity of SUN12x22 and TER16x32 if DRM_PANIC
  drm/panic: Spelling s/formater/formatter/
  drm/panic: Convert to drm_fb_clip_offset()
  drm/panic: Rename logo to logo_ascii
  drm/panic: Add support for drawing a monochrome graphical logo

 drivers/gpu/drm/Kconfig     |  2 +-
 drivers/gpu/drm/drm_panic.c | 74 +++++++++++++++++++++++++++++++------
 drivers/video/logo/Kconfig  |  2 +
 lib/fonts/Kconfig           |  6 ++-
 4 files changed, 70 insertions(+), 14 deletions(-)

-- 
2.34.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2 1/7] drm/panic: Fix uninitialized drm_scanout_buffer.set_pixel() crash

[Geert Uytterhoeven]

No implementations of drm_plane_helper_funcs.get_scanout_buffer() fill
in the optional drm_scanout_buffer.set_pixel() member.  Hence the member
may contain non-zero garbage, causing a crash when deferencing it during
drm panic.

Fix this by pre-initializing the drm_scanout_buffer object before
calling drm_plane_helper_funcs.get_scanout_buffer().

Fixes: 24d07f114e4ec760 ("drm/panic: Add a set_pixel() callback to drm_scanout_buffer")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New.
---
 drivers/gpu/drm/drm_panic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 293d4dcbc80da7ba..fc04ed4e0b399f55 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -582,7 +582,7 @@ static void draw_panic_dispatch(struct drm_scanout_buffer *sb)
 
 static void draw_panic_plane(struct drm_plane *plane)
 {
-	struct drm_scanout_buffer sb;
+	struct drm_scanout_buffer sb = { };
 	int ret;
 	unsigned long flags;
 
-- 
2.34.1

[PATCH v2 2/7] drm/panic: Fix off-by-one logo size checks

[Geert Uytterhoeven]

Logos that are either just as wide or just as high as the display work
fine.

Fixes: bf9fb17c6672868d ("drm/panic: Add a drm panic handler")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Rebased.
---
 drivers/gpu/drm/drm_panic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index fc04ed4e0b399f55..814ef5c20c08ee42 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -472,7 +472,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
 	drm_panic_fill(sb, &r_screen, bg_color);
 
 	if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) &&
-	    drm_rect_width(&r_logo) < sb->width && drm_rect_height(&r_logo) < sb->height) {
+	    drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) {
 		draw_txt_rectangle(sb, font, logo, logo_lines, false, &r_logo, fg_color);
 	}
 	draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color);
-- 
2.34.1

[PATCH v2 3/7] lib/fonts: Fix visiblity of SUN12x22 and TER16x32 if DRM_PANIC

[Geert Uytterhoeven]

When CONFIG_FONTS ("Select compiled-in fonts") is not enabled, the user
should not be asked about any fonts.  However, when CONFIG_DRM_PANIC is
enabled, the user is still asked about the Sparc console 12x22 and
Terminus 16x32 fonts.

Fix this by moving the "|| DRM_PANIC" to where it belongs.
Split the dependency in two rules to improve readability.

Fixes: b94605a3889b9084 ("lib/fonts: Allow to select fonts for drm_panic")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New.
---
 lib/fonts/Kconfig | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/fonts/Kconfig b/lib/fonts/Kconfig
index befcb463f7381d1a..3ac26bdbc3ff01a3 100644
--- a/lib/fonts/Kconfig
+++ b/lib/fonts/Kconfig
@@ -105,7 +105,8 @@ config FONT_SUN8x16
 
 config FONT_SUN12x22
 	bool "Sparc console 12x22 font (not supported by all drivers)"
-	depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC
+	depends on FRAMEBUFFER_CONSOLE || DRM_PANIC
+	depends on !SPARC && FONTS
 	help
 	  This is the high resolution console font for Sun machines with very
 	  big letters (like the letters used in the SPARC PROM). If the
@@ -113,7 +114,8 @@ config FONT_SUN12x22
 
 config FONT_TER16x32
 	bool "Terminus 16x32 font (not supported by all drivers)"
-	depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC
+	depends on FRAMEBUFFER_CONSOLE || DRM_PANIC
+	depends on !SPARC && FONTS || SPARC
 	help
 	  Terminus Font is a clean, fixed width bitmap font, designed
 	  for long (8 and more hours per day) work with computers.
-- 
2.34.1

[PATCH v2 4/7] drm/panic: Spelling s/formater/formatter/

[Geert Uytterhoeven]

Fix a misspelling of "formatter".

Fixes: 54034bebb22fd4be ("drm/panic: Add a kmsg panic screen")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New.
---
 drivers/gpu/drm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index a9972ce05d7e6fe4..e3c51009d9b476b3 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -138,7 +138,7 @@ config DRM_PANIC_DEBUG
 	  If in doubt, say "N".
 
 config DRM_PANIC_SCREEN
-	string "Panic screen formater"
+	string "Panic screen formatter"
 	default "user"
 	depends on DRM_PANIC
 	help
-- 
2.34.1

[PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()

[Geert Uytterhoeven]

Use the drm_fb_clip_offset() helper instead of open-coding the same
operation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
DRM_PANIC already selects DRM_KMS_HELPER.

v2:
  - New.
---
 drivers/gpu/drm/drm_panic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 814ef5c20c08ee42..5b0acf8c86e402a8 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -285,7 +285,7 @@ static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip,
 		return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, fg_color);
 
 	map = sb->map[0];
-	iosys_map_incr(&map, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]);
+	iosys_map_incr(&map, drm_fb_clip_offset(sb->pitch[0], sb->format, clip));
 
 	switch (sb->format->cpp[0]) {
 	case 2:
@@ -373,7 +373,7 @@ static void drm_panic_fill(struct drm_scanout_buffer *sb, struct drm_rect *clip,
 		return drm_panic_fill_pixel(sb, clip, color);
 
 	map = sb->map[0];
-	iosys_map_incr(&map, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]);
+	iosys_map_incr(&map, drm_fb_clip_offset(sb->pitch[0], sb->format, clip));
 
 	switch (sb->format->cpp[0]) {
 	case 2:
-- 
2.34.1

[PATCH v2 6/7] drm/panic: Rename logo to logo_ascii

[Geert Uytterhoeven]

Rename variables related to the ASCII logo, to prepare for the advent of
support for graphical logos.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Rebased.
---
 drivers/gpu/drm/drm_panic.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 5b0acf8c86e402a8..f7e22b69bb25d3be 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -78,7 +78,7 @@ static struct drm_panic_line panic_msg[] = {
 	PANIC_LINE("Please reboot your computer."),
 };
 
-static const struct drm_panic_line logo[] = {
+static const struct drm_panic_line logo_ascii[] = {
 	PANIC_LINE("     .--.        _"),
 	PANIC_LINE("    |o_o |      | |"),
 	PANIC_LINE("    |:_/ |      | |"),
@@ -447,7 +447,7 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb,
 static void draw_panic_static_user(struct drm_scanout_buffer *sb)
 {
 	size_t msg_lines = ARRAY_SIZE(panic_msg);
-	size_t logo_lines = ARRAY_SIZE(logo);
+	size_t logo_ascii_lines = ARRAY_SIZE(logo_ascii);
 	u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format);
 	u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format);
 	const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL);
@@ -459,8 +459,8 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
 	r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height);
 
 	r_logo = DRM_RECT_INIT(0, 0,
-			       get_max_line_len(logo, logo_lines) * font->width,
-			       logo_lines * font->height);
+			       get_max_line_len(logo_ascii, logo_ascii_lines) * font->width,
+			       logo_ascii_lines * font->height);
 	r_msg = DRM_RECT_INIT(0, 0,
 			      min(get_max_line_len(panic_msg, msg_lines) * font->width, sb->width),
 			      min(msg_lines * font->height, sb->height));
@@ -473,7 +473,8 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
 
 	if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) &&
 	    drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) {
-		draw_txt_rectangle(sb, font, logo, logo_lines, false, &r_logo, fg_color);
+		draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo,
+				   fg_color);
 	}
 	draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color);
 }
-- 
2.34.1

[PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo

[Geert Uytterhoeven]

Re-use the existing support for boot-up logos to draw a monochrome
graphical logo in the DRM panic handler.  When no suitable graphical
logo is available, the code falls back to the ASCII art penguin logo.

Note that all graphical boot-up logos are freed during late kernel
initialization, hence a copy must be made for later use.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Rebased,
  - Inline trivial draw_logo_mono().
---
 drivers/gpu/drm/drm_panic.c | 65 +++++++++++++++++++++++++++++++++----
 drivers/video/logo/Kconfig  |  2 ++
 2 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index f7e22b69bb25d3be..af30f243b2802ad7 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -7,11 +7,15 @@
  */
 
 #include <linux/font.h>
+#include <linux/init.h>
 #include <linux/iosys-map.h>
 #include <linux/kdebug.h>
 #include <linux/kmsg_dump.h>
+#include <linux/linux_logo.h>
 #include <linux/list.h>
+#include <linux/math.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/printk.h>
 #include <linux/types.h>
 
@@ -88,6 +92,42 @@ static const struct drm_panic_line logo_ascii[] = {
 	PANIC_LINE(" \\___)=(___/"),
 };
 
+#ifdef CONFIG_LOGO
+static const struct linux_logo *logo_mono;
+
+static int drm_panic_setup_logo(void)
+{
+	const struct linux_logo *logo = fb_find_logo(1);
+	const unsigned char *logo_data;
+	struct linux_logo *logo_dup;
+
+	if (!logo || logo->type != LINUX_LOGO_MONO)
+		return 0;
+
+	/* The logo is __init, so we must make a copy for later use */
+	logo_data = kmemdup(logo->data,
+			    size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height),
+			    GFP_KERNEL);
+	if (!logo_data)
+		return -ENOMEM;
+
+	logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL);
+	if (!logo_dup) {
+		kfree(logo_data);
+		return -ENOMEM;
+	}
+
+	logo_dup->data = logo_data;
+	logo_mono = logo_dup;
+
+	return 0;
+}
+
+device_initcall(drm_panic_setup_logo);
+#else
+#define logo_mono	((const struct linux_logo *)NULL)
+#endif
+
 /*
  * Color conversion
  */
@@ -452,15 +492,22 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
 	u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format);
 	const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL);
 	struct drm_rect r_screen, r_logo, r_msg;
+	unsigned int logo_width, logo_height;
 
 	if (!font)
 		return;
 
 	r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height);
 
-	r_logo = DRM_RECT_INIT(0, 0,
-			       get_max_line_len(logo_ascii, logo_ascii_lines) * font->width,
-			       logo_ascii_lines * font->height);
+	if (logo_mono) {
+		logo_width = logo_mono->width;
+		logo_height = logo_mono->height;
+	} else {
+		logo_width = get_max_line_len(logo_ascii, logo_ascii_lines) * font->width;
+		logo_height = logo_ascii_lines * font->height;
+	}
+
+	r_logo = DRM_RECT_INIT(0, 0, logo_width, logo_height);
 	r_msg = DRM_RECT_INIT(0, 0,
 			      min(get_max_line_len(panic_msg, msg_lines) * font->width, sb->width),
 			      min(msg_lines * font->height, sb->height));
@@ -471,10 +518,14 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
 	/* Fill with the background color, and draw text on top */
 	drm_panic_fill(sb, &r_screen, bg_color);
 
-	if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) &&
-	    drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) {
-		draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo,
-				   fg_color);
+	if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) &&
+	    logo_width <= sb->width && logo_height <= sb->height) {
+		if (logo_mono)
+			drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8),
+				       fg_color);
+		else
+			draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo,
+					   fg_color);
 	}
 	draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color);
 }
diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig
index b7d94d1dd1585a84..ce6bb753522d215d 100644
--- a/drivers/video/logo/Kconfig
+++ b/drivers/video/logo/Kconfig
@@ -8,6 +8,8 @@ menuconfig LOGO
 	depends on FB_CORE || SGI_NEWPORT_CONSOLE
 	help
 	  Enable and select frame buffer bootup logos.
+	  Monochrome logos will also be used by the DRM panic handler, if
+	  enabled.
 
 if LOGO
 
-- 
2.34.1

Re: [PATCH v2 1/7] drm/panic: Fix uninitialized drm_scanout_buffer.set_pixel() crash

[Jocelyn Falempe]

On 13/06/2024 21:17, Geert Uytterhoeven wrote:
> No implementations of drm_plane_helper_funcs.get_scanout_buffer() fill
> in the optional drm_scanout_buffer.set_pixel() member.  Hence the member
> may contain non-zero garbage, causing a crash when deferencing it during
> drm panic.
> 
> Fix this by pre-initializing the drm_scanout_buffer object before
> calling drm_plane_helper_funcs.get_scanout_buffer().

Good catch, I don't know how I didn't hit this bug before.
Thanks for the fix.

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

> 
> Fixes: 24d07f114e4ec760 ("drm/panic: Add a set_pixel() callback to drm_scanout_buffer")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>    - New.
> ---
>   drivers/gpu/drm/drm_panic.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 293d4dcbc80da7ba..fc04ed4e0b399f55 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -582,7 +582,7 @@ static void draw_panic_dispatch(struct drm_scanout_buffer *sb)
>   
>   static void draw_panic_plane(struct drm_plane *plane)
>   {
> -	struct drm_scanout_buffer sb;
> +	struct drm_scanout_buffer sb = { };
>   	int ret;
>   	unsigned long flags;
>   

Re: [PATCH v2 2/7] drm/panic: Fix off-by-one logo size checks

[Jocelyn Falempe]

On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> Logos that are either just as wide or just as high as the display work
> fine.

Sure, that looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Fixes: bf9fb17c6672868d ("drm/panic: Add a drm panic handler")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>    - Rebased.
> ---
>   drivers/gpu/drm/drm_panic.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index fc04ed4e0b399f55..814ef5c20c08ee42 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -472,7 +472,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
>   	drm_panic_fill(sb, &r_screen, bg_color);
>   
>   	if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) &&
> -	    drm_rect_width(&r_logo) < sb->width && drm_rect_height(&r_logo) < sb->height) {
> +	    drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) {
>   		draw_txt_rectangle(sb, font, logo, logo_lines, false, &r_logo, fg_color);
>   	}
>   	draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color);

Re: [PATCH v2 3/7] lib/fonts: Fix visiblity of SUN12x22 and TER16x32 if DRM_PANIC

[Jocelyn Falempe]

On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> When CONFIG_FONTS ("Select compiled-in fonts") is not enabled, the user
> should not be asked about any fonts.  However, when CONFIG_DRM_PANIC is
> enabled, the user is still asked about the Sparc console 12x22 and
> Terminus 16x32 fonts.
> 
> Fix this by moving the "|| DRM_PANIC" to where it belongs.
> Split the dependency in two rules to improve readability.

Sorry I think I misunderstood the SPARC && FONTS condition.
Your fix is much clearer, thanks.

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

> 
> Fixes: b94605a3889b9084 ("lib/fonts: Allow to select fonts for drm_panic")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>    - New.
> ---
>   lib/fonts/Kconfig | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/fonts/Kconfig b/lib/fonts/Kconfig
> index befcb463f7381d1a..3ac26bdbc3ff01a3 100644
> --- a/lib/fonts/Kconfig
> +++ b/lib/fonts/Kconfig
> @@ -105,7 +105,8 @@ config FONT_SUN8x16
>   
>   config FONT_SUN12x22
>   	bool "Sparc console 12x22 font (not supported by all drivers)"
> -	depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC
> +	depends on FRAMEBUFFER_CONSOLE || DRM_PANIC
> +	depends on !SPARC && FONTS
>   	help
>   	  This is the high resolution console font for Sun machines with very
>   	  big letters (like the letters used in the SPARC PROM). If the
> @@ -113,7 +114,8 @@ config FONT_SUN12x22
>   
>   config FONT_TER16x32
>   	bool "Terminus 16x32 font (not supported by all drivers)"
> -	depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC
> +	depends on FRAMEBUFFER_CONSOLE || DRM_PANIC
> +	depends on !SPARC && FONTS || SPARC
>   	help
>   	  Terminus Font is a clean, fixed width bitmap font, designed
>   	  for long (8 and more hours per day) work with computers.

Re: [PATCH v2 4/7] drm/panic: Spelling s/formater/formatter/

[Jocelyn Falempe]

On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> Fix a misspelling of "formatter".

It's probably because the same word has only one t in my native language.
Thanks for the fix.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Fixes: 54034bebb22fd4be ("drm/panic: Add a kmsg panic screen")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>    - New.
> ---
>   drivers/gpu/drm/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index a9972ce05d7e6fe4..e3c51009d9b476b3 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -138,7 +138,7 @@ config DRM_PANIC_DEBUG
>   	  If in doubt, say "N".
>   
>   config DRM_PANIC_SCREEN
> -	string "Panic screen formater"
> +	string "Panic screen formatter"
>   	default "user"
>   	depends on DRM_PANIC
>   	help

Re: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()

[Jocelyn Falempe]

On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> Use the drm_fb_clip_offset() helper instead of open-coding the same
> operation.

Thanks, it's a nice cleanup.

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

> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> DRM_PANIC already selects DRM_KMS_HELPER.
> 
> v2:
>    - New.
> ---
>   drivers/gpu/drm/drm_panic.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 814ef5c20c08ee42..5b0acf8c86e402a8 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -285,7 +285,7 @@ static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip,
>   		return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, fg_color);
>   
>   	map = sb->map[0];
> -	iosys_map_incr(&map, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]);
> +	iosys_map_incr(&map, drm_fb_clip_offset(sb->pitch[0], sb->format, clip));
>   
>   	switch (sb->format->cpp[0]) {
>   	case 2:
> @@ -373,7 +373,7 @@ static void drm_panic_fill(struct drm_scanout_buffer *sb, struct drm_rect *clip,
>   		return drm_panic_fill_pixel(sb, clip, color);
>   
>   	map = sb->map[0];
> -	iosys_map_incr(&map, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]);
> +	iosys_map_incr(&map, drm_fb_clip_offset(sb->pitch[0], sb->format, clip));
>   
>   	switch (sb->format->cpp[0]) {
>   	case 2:

Re: [PATCH v2 6/7] drm/panic: Rename logo to logo_ascii

[Jocelyn Falempe]

On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> Rename variables related to the ASCII logo, to prepare for the advent of
> support for graphical logos.

Thanks, that looks good to me.

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

> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>    - Rebased.
> ---
>   drivers/gpu/drm/drm_panic.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 5b0acf8c86e402a8..f7e22b69bb25d3be 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -78,7 +78,7 @@ static struct drm_panic_line panic_msg[] = {
>   	PANIC_LINE("Please reboot your computer."),
>   };
>   
> -static const struct drm_panic_line logo[] = {
> +static const struct drm_panic_line logo_ascii[] = {
>   	PANIC_LINE("     .--.        _"),
>   	PANIC_LINE("    |o_o |      | |"),
>   	PANIC_LINE("    |:_/ |      | |"),
> @@ -447,7 +447,7 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb,
>   static void draw_panic_static_user(struct drm_scanout_buffer *sb)
>   {
>   	size_t msg_lines = ARRAY_SIZE(panic_msg);
> -	size_t logo_lines = ARRAY_SIZE(logo);
> +	size_t logo_ascii_lines = ARRAY_SIZE(logo_ascii);
>   	u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format);
>   	u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format);
>   	const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL);
> @@ -459,8 +459,8 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
>   	r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height);
>   
>   	r_logo = DRM_RECT_INIT(0, 0,
> -			       get_max_line_len(logo, logo_lines) * font->width,
> -			       logo_lines * font->height);
> +			       get_max_line_len(logo_ascii, logo_ascii_lines) * font->width,
> +			       logo_ascii_lines * font->height);
>   	r_msg = DRM_RECT_INIT(0, 0,
>   			      min(get_max_line_len(panic_msg, msg_lines) * font->width, sb->width),
>   			      min(msg_lines * font->height, sb->height));
> @@ -473,7 +473,8 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
>   
>   	if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) &&
>   	    drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) {
> -		draw_txt_rectangle(sb, font, logo, logo_lines, false, &r_logo, fg_color);
> +		draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo,
> +				   fg_color);
>   	}
>   	draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color);
>   }

Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo

[Jocelyn Falempe]

On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> Re-use the existing support for boot-up logos to draw a monochrome
> graphical logo in the DRM panic handler.  When no suitable graphical
> logo is available, the code falls back to the ASCII art penguin logo.
> 
> Note that all graphical boot-up logos are freed during late kernel
> initialization, hence a copy must be made for later use.

Would it be possible to have the logo not in the __init section if 
DRM_PANIC is set ?

The patch looks good to me anyway.

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

> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>    - Rebased,
>    - Inline trivial draw_logo_mono().
> ---
>   drivers/gpu/drm/drm_panic.c | 65 +++++++++++++++++++++++++++++++++----
>   drivers/video/logo/Kconfig  |  2 ++
>   2 files changed, 60 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index f7e22b69bb25d3be..af30f243b2802ad7 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -7,11 +7,15 @@
>    */
>   
>   #include <linux/font.h>
> +#include <linux/init.h>
>   #include <linux/iosys-map.h>
>   #include <linux/kdebug.h>
>   #include <linux/kmsg_dump.h>
> +#include <linux/linux_logo.h>
>   #include <linux/list.h>
> +#include <linux/math.h>
>   #include <linux/module.h>
> +#include <linux/overflow.h>
>   #include <linux/printk.h>
>   #include <linux/types.h>
>   
> @@ -88,6 +92,42 @@ static const struct drm_panic_line logo_ascii[] = {
>   	PANIC_LINE(" \\___)=(___/"),
>   };
>   
> +#ifdef CONFIG_LOGO
> +static const struct linux_logo *logo_mono;
> +
> +static int drm_panic_setup_logo(void)
> +{
> +	const struct linux_logo *logo = fb_find_logo(1);
> +	const unsigned char *logo_data;
> +	struct linux_logo *logo_dup;
> +
> +	if (!logo || logo->type != LINUX_LOGO_MONO)
> +		return 0;
> +
> +	/* The logo is __init, so we must make a copy for later use */
> +	logo_data = kmemdup(logo->data,
> +			    size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height),
> +			    GFP_KERNEL);
> +	if (!logo_data)
> +		return -ENOMEM;
> +
> +	logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL);
> +	if (!logo_dup) {
> +		kfree(logo_data);
> +		return -ENOMEM;
> +	}
> +
> +	logo_dup->data = logo_data;
> +	logo_mono = logo_dup;
> +
> +	return 0;
> +}
> +
> +device_initcall(drm_panic_setup_logo);
> +#else
> +#define logo_mono	((const struct linux_logo *)NULL)
> +#endif
> +
>   /*
>    * Color conversion
>    */
> @@ -452,15 +492,22 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
>   	u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format);
>   	const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL);
>   	struct drm_rect r_screen, r_logo, r_msg;
> +	unsigned int logo_width, logo_height;
>   
>   	if (!font)
>   		return;
>   
>   	r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height);
>   
> -	r_logo = DRM_RECT_INIT(0, 0,
> -			       get_max_line_len(logo_ascii, logo_ascii_lines) * font->width,
> -			       logo_ascii_lines * font->height);
> +	if (logo_mono) {
> +		logo_width = logo_mono->width;
> +		logo_height = logo_mono->height;
> +	} else {
> +		logo_width = get_max_line_len(logo_ascii, logo_ascii_lines) * font->width;
> +		logo_height = logo_ascii_lines * font->height;
> +	}
> +
> +	r_logo = DRM_RECT_INIT(0, 0, logo_width, logo_height);
>   	r_msg = DRM_RECT_INIT(0, 0,
>   			      min(get_max_line_len(panic_msg, msg_lines) * font->width, sb->width),
>   			      min(msg_lines * font->height, sb->height));
> @@ -471,10 +518,14 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb)
>   	/* Fill with the background color, and draw text on top */
>   	drm_panic_fill(sb, &r_screen, bg_color);
>   
> -	if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) &&
> -	    drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) {
> -		draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo,
> -				   fg_color);
> +	if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) &&
> +	    logo_width <= sb->width && logo_height <= sb->height) {
> +		if (logo_mono)
> +			drm_panic_blit(sb, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8),
> +				       fg_color);
> +		else
> +			draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo,
> +					   fg_color);
>   	}
>   	draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color);
>   }
> diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig
> index b7d94d1dd1585a84..ce6bb753522d215d 100644
> --- a/drivers/video/logo/Kconfig
> +++ b/drivers/video/logo/Kconfig
> @@ -8,6 +8,8 @@ menuconfig LOGO
>   	depends on FB_CORE || SGI_NEWPORT_CONSOLE
>   	help
>   	  Enable and select frame buffer bootup logos.
> +	  Monochrome logos will also be used by the DRM panic handler, if
> +	  enabled.
>   
>   if LOGO
>   

Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo

[Geert Uytterhoeven]

Hi Jocelyn,

On Fri, Jun 14, 2024 at 11:55 AM Jocelyn Falempe <jfalempe@redhat.com> wrote:
> On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> > Re-use the existing support for boot-up logos to draw a monochrome
> > graphical logo in the DRM panic handler.  When no suitable graphical
> > logo is available, the code falls back to the ASCII art penguin logo.
> >
> > Note that all graphical boot-up logos are freed during late kernel
> > initialization, hence a copy must be made for later use.
>
> Would it be possible to have the logo not in the __init section if
> DRM_PANIC is set ?

That would be rather complicated.  The C source files for the logos
(there can be multiple) are generated by drivers/video/logo/pnmtologo.c.

> The patch looks good to me anyway.
>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()

[kernel test robot]

Hi Geert,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[cannot apply to linus/master v6.10-rc3 next-20240613]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Geert-Uytterhoeven/drm-panic-Fix-uninitialized-drm_scanout_buffer-set_pixel-crash/20240614-032053
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/3121082eb4beb461773ebb6f656ed9b4286967ee.1718305355.git.geert%2Brenesas%40glider.be
patch subject: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()
config: x86_64-randconfig-003-20240615 (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406151811.yEIZ6203-lkp@intel.com/

All errors (new ones prefixed by >>):

>> depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
   depmod: ERROR: Found 2 modules in dependency cycles!
   make[3]: *** [scripts/Makefile.modinst:128: depmod] Error 1 shuffle=844234264
   make[3]: Target '__modinst' not remade because of errors.
   make[2]: *** [Makefile:1842: modules_install] Error 2 shuffle=844234264
   make[1]: *** [Makefile:240: __sub-make] Error 2 shuffle=844234264
   make[1]: Target 'modules_install' not remade because of errors.
   make: *** [Makefile:240: __sub-make] Error 2 shuffle=844234264
   make: Target 'modules_install' not remade because of errors.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()

[Geert Uytterhoeven]

On Sat, Jun 15, 2024 at 12:55 PM kernel test robot <lkp@intel.com> wrote:
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on drm-misc/drm-misc-next]
> [cannot apply to linus/master v6.10-rc3 next-20240613]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Geert-Uytterhoeven/drm-panic-Fix-uninitialized-drm_scanout_buffer-set_pixel-crash/20240614-032053
> base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> patch link:    https://lore.kernel.org/r/3121082eb4beb461773ebb6f656ed9b4286967ee.1718305355.git.geert%2Brenesas%40glider.be
> patch subject: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()
> config: x86_64-randconfig-003-20240615 (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/config)
> compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202406151811.yEIZ6203-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> >> depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
>    depmod: ERROR: Found 2 modules in dependency cycles!

Oops, so DRM core cannot call any of the helpers, and DRM_PANIC
selecting DRM_KMS_HELPER was wrong in the first place?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()

[Geert Uytterhoeven]

On Sun, Jun 16, 2024 at 11:08 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sat, Jun 15, 2024 at 12:55 PM kernel test robot <lkp@intel.com> wrote:
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on drm-misc/drm-misc-next]
> > [cannot apply to linus/master v6.10-rc3 next-20240613]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Geert-Uytterhoeven/drm-panic-Fix-uninitialized-drm_scanout_buffer-set_pixel-crash/20240614-032053
> > base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> > patch link:    https://lore.kernel.org/r/3121082eb4beb461773ebb6f656ed9b4286967ee.1718305355.git.geert%2Brenesas%40glider.be
> > patch subject: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()
> > config: x86_64-randconfig-003-20240615 (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/config)
> > compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202406151811.yEIZ6203-lkp@intel.com/
> >
> > All errors (new ones prefixed by >>):
> >
> > >> depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> >    depmod: ERROR: Found 2 modules in dependency cycles!
>
> Oops, so DRM core cannot call any of the helpers, and DRM_PANIC
> selecting DRM_KMS_HELPER was wrong in the first place?

Q: So how does this work with DRM_PANIC calling
   drm_fb_helper_emergency_disable()?
A: drm_fb_helper_emergency_disable() is a dummy if
   !CONFIG_DRM_FBDEV_EMULATION, so I guess no one tried to build
   a failing randconfig with CONFIG_DRM_FBDEV_EMULATION=y yet.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()

[Jocelyn Falempe]

On 16/06/2024 11:12, Geert Uytterhoeven wrote:
> On Sun, Jun 16, 2024 at 11:08 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Sat, Jun 15, 2024 at 12:55 PM kernel test robot <lkp@intel.com> wrote:
>>> kernel test robot noticed the following build errors:
>>>
>>> [auto build test ERROR on drm-misc/drm-misc-next]
>>> [cannot apply to linus/master v6.10-rc3 next-20240613]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url:    https://github.com/intel-lab-lkp/linux/commits/Geert-Uytterhoeven/drm-panic-Fix-uninitialized-drm_scanout_buffer-set_pixel-crash/20240614-032053
>>> base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
>>> patch link:    https://lore.kernel.org/r/3121082eb4beb461773ebb6f656ed9b4286967ee.1718305355.git.geert%2Brenesas%40glider.be
>>> patch subject: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()
>>> config: x86_64-randconfig-003-20240615 (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/config)
>>> compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
>>> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/202406151811.yEIZ6203-lkp@intel.com/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202406151811.yEIZ6203-lkp@intel.com/
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>>> depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
>>>     depmod: ERROR: Found 2 modules in dependency cycles!
>>
>> Oops, so DRM core cannot call any of the helpers, and DRM_PANIC
>> selecting DRM_KMS_HELPER was wrong in the first place?
> 
> Q: So how does this work with DRM_PANIC calling
>     drm_fb_helper_emergency_disable()?
> A: drm_fb_helper_emergency_disable() is a dummy if
>     !CONFIG_DRM_FBDEV_EMULATION, so I guess no one tried to build
>     a failing randconfig with CONFIG_DRM_FBDEV_EMULATION=y yet.

drm_fb_helper_emergency_disable() is part of the series
https://patchwork.freedesktop.org/series/132720/
which, after discussing it on IRC with sima and Javier, is not a good 
solution, and is abandoned.

I think the "select DRM_KMS_HELPER" is a leftover from earlier version 
of drm_panic, that used the color conversion function from 
drm_kms_helper, but that has changed in v10 and later.

drm_panic is called from the drm_core code, so in fact it can't depends 
on the kms helper.

I don't see a good solution to workaround this circular dependency, 
maybe depends on DRM_KMS_HELPER being built-in ? (so that means drm_core 
will also be built-in). But that means platform that build drm core as 
module, won't be able to use drm_panic.

There are a few of them in the kernel tree:
rg -l CONFIG_DRM=m
arch/powerpc/configs/85xx/stx_gp3_defconfig
arch/powerpc/configs/ppc6xx_defconfig
arch/powerpc/configs/skiroot_defconfig
arch/powerpc/configs/pmac32_defconfig
arch/mips/configs/ci20_defconfig
arch/arc/configs/axs101_defconfig
arch/arc/configs/axs103_smp_defconfig
arch/riscv/configs/defconfig
arch/parisc/configs/generic-32bit_defconfig
arch/arm/configs/davinci_all_defconfig
arch/arm/configs/omap2plus_defconfig
arch/arm/configs/pxa_defconfig
arch/arm64/configs/defconfig

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 

Jocelyn

Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo

[Jocelyn Falempe]

On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> Re-use the existing support for boot-up logos to draw a monochrome
> graphical logo in the DRM panic handler.  When no suitable graphical
> logo is available, the code falls back to the ASCII art penguin logo.
> 
> Note that all graphical boot-up logos are freed during late kernel
> initialization, hence a copy must be made for later use.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>    - Rebased,
>    - Inline trivial draw_logo_mono().
> ---
>   drivers/gpu/drm/drm_panic.c | 65 +++++++++++++++++++++++++++++++++----
>   drivers/video/logo/Kconfig  |  2 ++
>   2 files changed, 60 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index f7e22b69bb25d3be..af30f243b2802ad7 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -7,11 +7,15 @@
>    */
>   
>   #include <linux/font.h>
> +#include <linux/init.h>
>   #include <linux/iosys-map.h>
>   #include <linux/kdebug.h>
>   #include <linux/kmsg_dump.h>
> +#include <linux/linux_logo.h>
>   #include <linux/list.h>
> +#include <linux/math.h>
>   #include <linux/module.h>
> +#include <linux/overflow.h>
>   #include <linux/printk.h>
>   #include <linux/types.h>
>   
> @@ -88,6 +92,42 @@ static const struct drm_panic_line logo_ascii[] = {
>   	PANIC_LINE(" \\___)=(___/"),
>   };
>   
> +#ifdef CONFIG_LOGO
> +static const struct linux_logo *logo_mono;
> +
> +static int drm_panic_setup_logo(void)
> +{
> +	const struct linux_logo *logo = fb_find_logo(1);
> +	const unsigned char *logo_data;
> +	struct linux_logo *logo_dup;
> +
> +	if (!logo || logo->type != LINUX_LOGO_MONO)
> +		return 0;
> +
> +	/* The logo is __init, so we must make a copy for later use */
> +	logo_data = kmemdup(logo->data,
> +			    size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height),
> +			    GFP_KERNEL);
> +	if (!logo_data)
> +		return -ENOMEM;
> +
> +	logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL);
> +	if (!logo_dup) {
> +		kfree(logo_data);
> +		return -ENOMEM;
> +	}
> +
> +	logo_dup->data = logo_data;
> +	logo_mono = logo_dup;
> +
> +	return 0;
> +}
> +
> +device_initcall(drm_panic_setup_logo);
> +#else
> +#define logo_mono	((const struct linux_logo *)NULL)
> +#endif

I didn't notice that the first time, but the core drm can be built as a 
module.
That means this will leak memory each time the module is removed.

But to solve the circular dependency between drm_kms_helper and 
drm_panic, one solution is to depends on drm core to be built-in.
In this case there won't be a leak.
So depending on how we solve the circular dependency, it can be acceptable.

-- 

Jocelyn

Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo

[Geert Uytterhoeven]

Hi Jocelyn,

On Mon, Jun 17, 2024 at 11:59 AM Jocelyn Falempe <jfalempe@redhat.com> wrote:
> On 13/06/2024 21:18, Geert Uytterhoeven wrote:
> > Re-use the existing support for boot-up logos to draw a monochrome
> > graphical logo in the DRM panic handler.  When no suitable graphical
> > logo is available, the code falls back to the ASCII art penguin logo.
> >
> > Note that all graphical boot-up logos are freed during late kernel
> > initialization, hence a copy must be made for later use.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- a/drivers/gpu/drm/drm_panic.c
> > +++ b/drivers/gpu/drm/drm_panic.c

> >       PANIC_LINE(" \\___)=(___/"),
> >   };
> >
> > +#ifdef CONFIG_LOGO
> > +static const struct linux_logo *logo_mono;
> > +
> > +static int drm_panic_setup_logo(void)
> > +{
> > +     const struct linux_logo *logo = fb_find_logo(1);
> > +     const unsigned char *logo_data;
> > +     struct linux_logo *logo_dup;
> > +
> > +     if (!logo || logo->type != LINUX_LOGO_MONO)
> > +             return 0;
> > +
> > +     /* The logo is __init, so we must make a copy for later use */
> > +     logo_data = kmemdup(logo->data,
> > +                         size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height),
> > +                         GFP_KERNEL);
> > +     if (!logo_data)
> > +             return -ENOMEM;
> > +
> > +     logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL);
> > +     if (!logo_dup) {
> > +             kfree(logo_data);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     logo_dup->data = logo_data;
> > +     logo_mono = logo_dup;
> > +
> > +     return 0;
> > +}
> > +
> > +device_initcall(drm_panic_setup_logo);
> > +#else
> > +#define logo_mono    ((const struct linux_logo *)NULL)
> > +#endif
>
> I didn't notice that the first time, but the core drm can be built as a
> module.
> That means this will leak memory each time the module is removed.

While I hadn't considered a modular DRM core, there is no memory leak:
after the logos are freed (from late_initcall_sync()), fb_find_logo()
returns NULL. This does mean there won't be a graphical logo on panic,
though.

> But to solve the circular dependency between drm_kms_helper and
> drm_panic, one solution is to depends on drm core to be built-in.
> In this case there won't be a leak.
> So depending on how we solve the circular dependency, it can be acceptable.

So far there is no reason to select DRM_KMS_HELPER, right?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo

[Jocelyn Falempe]

On 17/06/2024 14:49, Geert Uytterhoeven wrote:
> Hi Jocelyn,
> 
> On Mon, Jun 17, 2024 at 11:59 AM Jocelyn Falempe <jfalempe@redhat.com> wrote:
>> On 13/06/2024 21:18, Geert Uytterhoeven wrote:
>>> Re-use the existing support for boot-up logos to draw a monochrome
>>> graphical logo in the DRM panic handler.  When no suitable graphical
>>> logo is available, the code falls back to the ASCII art penguin logo.
>>>
>>> Note that all graphical boot-up logos are freed during late kernel
>>> initialization, hence a copy must be made for later use.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
>>> --- a/drivers/gpu/drm/drm_panic.c
>>> +++ b/drivers/gpu/drm/drm_panic.c
> 
>>>        PANIC_LINE(" \\___)=(___/"),
>>>    };
>>>
>>> +#ifdef CONFIG_LOGO
>>> +static const struct linux_logo *logo_mono;
>>> +
>>> +static int drm_panic_setup_logo(void)
>>> +{
>>> +     const struct linux_logo *logo = fb_find_logo(1);
>>> +     const unsigned char *logo_data;
>>> +     struct linux_logo *logo_dup;
>>> +
>>> +     if (!logo || logo->type != LINUX_LOGO_MONO)
>>> +             return 0;
>>> +
>>> +     /* The logo is __init, so we must make a copy for later use */
>>> +     logo_data = kmemdup(logo->data,
>>> +                         size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height),
>>> +                         GFP_KERNEL);
>>> +     if (!logo_data)
>>> +             return -ENOMEM;
>>> +
>>> +     logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL);
>>> +     if (!logo_dup) {
>>> +             kfree(logo_data);
>>> +             return -ENOMEM;
>>> +     }
>>> +
>>> +     logo_dup->data = logo_data;
>>> +     logo_mono = logo_dup;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +device_initcall(drm_panic_setup_logo);
>>> +#else
>>> +#define logo_mono    ((const struct linux_logo *)NULL)
>>> +#endif
>>
>> I didn't notice that the first time, but the core drm can be built as a
>> module.
>> That means this will leak memory each time the module is removed.
> 
> While I hadn't considered a modular DRM core, there is no memory leak:
> after the logos are freed (from late_initcall_sync()), fb_find_logo()
> returns NULL. This does mean there won't be a graphical logo on panic,
> though.

Yes, you're right, thanks for the precision.
> 
>> But to solve the circular dependency between drm_kms_helper and
>> drm_panic, one solution is to depends on drm core to be built-in.
>> In this case there won't be a leak.
>> So depending on how we solve the circular dependency, it can be acceptable.
> 
> So far there is no reason to select DRM_KMS_HELPER, right?

The current version doesn't need DRM_KMS_HELPER.
But for example, it uses struct drm_rect, and a few functions 
(drm_rect_width()) that are defined in .h, but other drm_rect_* 
functions are defined in DRM_KMS_HELPER.
And as you pointed out in your patch, it duplicates the 
drm_fb_clip_offset(). So I'm not sure if it can avoid the dependency on 
DRM_KMS_HELPER in the long term.


> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Re: [PATCH v2 0/7] drm/panic: Fixes and graphical logo

[Jocelyn Falempe]

Hi,

I want to push at least the first patch that is an important fix.
But if there are no objections, I can push the whole series except patch 
5 "drm/panic: Convert to drm_fb_clip_offset()" which causes some build 
issue.

Best regards,

-- 

Jocelyn

On 13/06/2024 21:17, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> If drm/panic is enabled, a user-friendly message is shown on screen when
> a kernel panic occurs, together with an ASCII art penguin logo.
> Of course we can do better ;-)
> Hence this patch series extends drm/panic to draw the monochrome
> graphical boot logo, when available, preceded by the customary fixes.
> 
> Changes compared to v1:
>    - Rebase against today's drm-misc-next, where drm_panic is broken on
>      all current drivers due to an uninitialized pointer dereference.
>      Presumably this was only tested with an out-of-tree driver change?
>    - New fixes [1/7], [3/7], and [4/7],
>    - New cleanup [5/7],
>    - Inline trivial draw_logo_mono().
> 
> This has been tested with rcar-du.
> 
> Thanks for your comments!
> 
> Geert Uytterhoeven (7):
>    drm/panic: Fix uninitialized drm_scanout_buffer.set_pixel() crash
>    drm/panic: Fix off-by-one logo size checks
>    lib/fonts: Fix visiblity of SUN12x22 and TER16x32 if DRM_PANIC
>    drm/panic: Spelling s/formater/formatter/
>    drm/panic: Convert to drm_fb_clip_offset()
>    drm/panic: Rename logo to logo_ascii
>    drm/panic: Add support for drawing a monochrome graphical logo
> 
>   drivers/gpu/drm/Kconfig     |  2 +-
>   drivers/gpu/drm/drm_panic.c | 74 +++++++++++++++++++++++++++++++------
>   drivers/video/logo/Kconfig  |  2 +
>   lib/fonts/Kconfig           |  6 ++-
>   4 files changed, 70 insertions(+), 14 deletions(-)
> 

Re: [PATCH v2 0/7] drm/panic: Fixes and graphical logo

[Jocelyn Falempe]

On 21/06/2024 10:55, Jocelyn Falempe wrote:
> Hi,
> 
> I want to push at least the first patch that is an important fix.
> But if there are no objections, I can push the whole series except patch 
> 5 "drm/panic: Convert to drm_fb_clip_offset()" which causes some build 
> issue.

I just pushed them to drm-misc-next.
Thanks all.

> 
> Best regards,
>