[PATCH v5 0/6] drm/log: Introduce a new boot logger to draw the kmsg on the screen

[PATCH v5 0/6] drm/log: Introduce a new boot logger to draw the kmsg on the screen

[Jocelyn Falempe]

drm_log is a simple logger that uses the drm_client API to print the kmsg boot log on the screen.
This is not a full replacement to fbcon, as it will only print the kmsg.
It will never handle user input, or a terminal because this is better done in userspace.

If you're curious on how it looks like, I've put a small demo here:
https://people.redhat.com/jfalempe/drm_log/drm_log_draft_boot_v2.mp4

Design decisions:
  * It uses the drm_client API, so it should work on all drm drivers from the start.
  * It doesn't scroll the message, that way it doesn't need to redraw the whole screen for each new message.
    It also means it doesn't have to keep drawn messages in memory, to redraw them when scrolling.
  * It uses the new non-blocking console API, so it should work well with PREEMPT_RT
 
v2:
 * Use vmap_local() api, with that change, I've tested it successfully on simpledrm, virtio-gpu, amdgpu, and nouveau.
 * Stop drawing when the drm_master is taken. This avoid wasting CPU cycle if the buffer is not visible.
 * Use deferred probe. Only do the probe the first time there is a log to draw. With this, if you boot with quiet, drm_log won't do any modeset.
 * Add color support for the timestamp prefix, like what dmesg does.
 * Add build dependency on  disabling the fbdev emulation, as they are both drm_client, and there is no way to choose which one gets the focus.

v3:
 * Remove the work thread and circular buffer, and use the new write_thread() console API.
 * Register a console for each drm driver.

v4:
 * Can be built as a module, even if that's not really useful.
 * Rebased on top of "drm: Introduce DRM client library" series from Thomas Zimmermann.
 * Add a Kconfig menu to choose between drm client.
 * Add suspend/resume callbacks.
 * Add integer scaling support.
 
v5:
 * Build drm_log in drm_client_lib module, to avoid circular dependency.
 * Export drm_draw symbols, so they can be used if drm_client_lib is built as module.
 * Change scale parameter to unsigned int (Jani Nikula)

Jocelyn Falempe (6):
  drm/panic: Move drawing functions to drm_draw
  drm/log: Introduce a new boot logger to draw the kmsg on the screen
  drm/log: Do not draw if drm_master is taken
  drm/log: Color the timestamp, to improve readability
  drm/log: Implement suspend/resume
  drm/log: Add integer scaling support

 drivers/gpu/drm/Kconfig            |  51 ++++
 drivers/gpu/drm/Makefile           |   2 +
 drivers/gpu/drm/drm_client_setup.c |  18 +-
 drivers/gpu/drm/drm_draw.c         | 223 +++++++++++++++
 drivers/gpu/drm/drm_draw.h         |  56 ++++
 drivers/gpu/drm/drm_log.c          | 426 +++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_log.h          |  11 +
 drivers/gpu/drm/drm_panic.c        | 247 ++---------------
 8 files changed, 807 insertions(+), 227 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_draw.c
 create mode 100644 drivers/gpu/drm/drm_draw.h
 create mode 100644 drivers/gpu/drm/drm_log.c
 create mode 100644 drivers/gpu/drm/drm_log.h


base-commit: 91e21479c81dd4e9e22a78d7446f92f6b96a7284
-- 
2.47.0

[PATCH v5 1/6] drm/panic: Move drawing functions to drm_draw

[Jocelyn Falempe]

Move the color conversions, blit and fill functions to drm_draw.c,
so that they can be re-used by drm_log.
drm_draw is internal to the drm subsystem, and shouldn't be used by
gpu drivers.

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

v5:
 * Export drm_draw symbols, so they can be used if drm_client_lib is built as module.


 drivers/gpu/drm/Kconfig     |   5 +
 drivers/gpu/drm/Makefile    |   1 +
 drivers/gpu/drm/drm_draw.c  | 223 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_draw.h  |  56 ++++++++
 drivers/gpu/drm/drm_panic.c | 247 ++++--------------------------------
 5 files changed, 308 insertions(+), 224 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_draw.c
 create mode 100644 drivers/gpu/drm/drm_draw.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 5504721007cc1..3f16dca0b6643 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -102,10 +102,15 @@ config DRM_KMS_HELPER
 	help
 	  CRTC helpers for KMS drivers.
 
+config DRM_DRAW
+	bool
+	depends on DRM
+
 config DRM_PANIC
 	bool "Display a user-friendly message when a kernel panic occurs"
 	depends on DRM
 	select FONT_SUPPORT
+	select DRM_DRAW
 	help
 	  Enable a drm panic handler, which will display a user-friendly message
 	  when a kernel panic occurs. It's useful when using a user-space
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 463afad1b5ca6..68a0a679a7b93 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -91,6 +91,7 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \
 	drm_privacy_screen_x86.o
 drm-$(CONFIG_DRM_ACCEL) += ../../accel/drm_accel.o
 drm-$(CONFIG_DRM_PANIC) += drm_panic.o
+drm-$(CONFIG_DRM_DRAW) += drm_draw.o
 drm-$(CONFIG_DRM_PANIC_SCREEN_QR_CODE) += drm_panic_qr.o
 obj-$(CONFIG_DRM)	+= drm.o
 
diff --git a/drivers/gpu/drm/drm_draw.c b/drivers/gpu/drm/drm_draw.c
new file mode 100644
index 0000000000000..13b9760bdb632
--- /dev/null
+++ b/drivers/gpu/drm/drm_draw.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+/*
+ * Copyright (c) 2023 Red Hat.
+ * Author: Jocelyn Falempe <jfalempe@redhat.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/iosys-map.h>
+#include <linux/types.h>
+
+#include <drm/drm_fourcc.h>
+
+#include "drm_draw.h"
+
+/*
+ * Conversions from xrgb8888
+ */
+
+static u16 convert_xrgb8888_to_rgb565(u32 pix)
+{
+	return ((pix & 0x00F80000) >> 8) |
+	       ((pix & 0x0000FC00) >> 5) |
+	       ((pix & 0x000000F8) >> 3);
+}
+
+static u16 convert_xrgb8888_to_rgba5551(u32 pix)
+{
+	return ((pix & 0x00f80000) >> 8) |
+	       ((pix & 0x0000f800) >> 5) |
+	       ((pix & 0x000000f8) >> 2) |
+	       BIT(0); /* set alpha bit */
+}
+
+static u16 convert_xrgb8888_to_xrgb1555(u32 pix)
+{
+	return ((pix & 0x00f80000) >> 9) |
+	       ((pix & 0x0000f800) >> 6) |
+	       ((pix & 0x000000f8) >> 3);
+}
+
+static u16 convert_xrgb8888_to_argb1555(u32 pix)
+{
+	return BIT(15) | /* set alpha bit */
+	       ((pix & 0x00f80000) >> 9) |
+	       ((pix & 0x0000f800) >> 6) |
+	       ((pix & 0x000000f8) >> 3);
+}
+
+static u32 convert_xrgb8888_to_argb8888(u32 pix)
+{
+	return pix | GENMASK(31, 24); /* fill alpha bits */
+}
+
+static u32 convert_xrgb8888_to_xbgr8888(u32 pix)
+{
+	return ((pix & 0x00ff0000) >> 16) <<  0 |
+	       ((pix & 0x0000ff00) >>  8) <<  8 |
+	       ((pix & 0x000000ff) >>  0) << 16 |
+	       ((pix & 0xff000000) >> 24) << 24;
+}
+
+static u32 convert_xrgb8888_to_abgr8888(u32 pix)
+{
+	return ((pix & 0x00ff0000) >> 16) <<  0 |
+	       ((pix & 0x0000ff00) >>  8) <<  8 |
+	       ((pix & 0x000000ff) >>  0) << 16 |
+	       GENMASK(31, 24); /* fill alpha bits */
+}
+
+static u32 convert_xrgb8888_to_xrgb2101010(u32 pix)
+{
+	pix = ((pix & 0x000000FF) << 2) |
+	      ((pix & 0x0000FF00) << 4) |
+	      ((pix & 0x00FF0000) << 6);
+	return pix | ((pix >> 8) & 0x00300C03);
+}
+
+static u32 convert_xrgb8888_to_argb2101010(u32 pix)
+{
+	pix = ((pix & 0x000000FF) << 2) |
+	      ((pix & 0x0000FF00) << 4) |
+	      ((pix & 0x00FF0000) << 6);
+	return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) & 0x00300C03);
+}
+
+/**
+ * drm_draw_color_from_xrgb8888 - convert one pixel from xrgb8888 to the desired format
+ * @color: input color, in xrgb8888 format
+ * @format: output format
+ *
+ * Returns:
+ * Color in the format specified, casted to u32.
+ * Or 0 if the format is not supported.
+ */
+u32 drm_draw_color_from_xrgb8888(u32 color, u32 format)
+{
+	switch (format) {
+	case DRM_FORMAT_RGB565:
+		return convert_xrgb8888_to_rgb565(color);
+	case DRM_FORMAT_RGBA5551:
+		return convert_xrgb8888_to_rgba5551(color);
+	case DRM_FORMAT_XRGB1555:
+		return convert_xrgb8888_to_xrgb1555(color);
+	case DRM_FORMAT_ARGB1555:
+		return convert_xrgb8888_to_argb1555(color);
+	case DRM_FORMAT_RGB888:
+	case DRM_FORMAT_XRGB8888:
+		return color;
+	case DRM_FORMAT_ARGB8888:
+		return convert_xrgb8888_to_argb8888(color);
+	case DRM_FORMAT_XBGR8888:
+		return convert_xrgb8888_to_xbgr8888(color);
+	case DRM_FORMAT_ABGR8888:
+		return convert_xrgb8888_to_abgr8888(color);
+	case DRM_FORMAT_XRGB2101010:
+		return convert_xrgb8888_to_xrgb2101010(color);
+	case DRM_FORMAT_ARGB2101010:
+		return convert_xrgb8888_to_argb2101010(color);
+	default:
+		WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
+		return 0;
+	}
+}
+EXPORT_SYMBOL(drm_draw_color_from_xrgb8888);
+
+/*
+ * Blit functions
+ */
+void drm_draw_blit16(struct iosys_map *dmap, unsigned int dpitch,
+		     const u8 *sbuf8, unsigned int spitch,
+		     unsigned int height, unsigned int width,
+		     unsigned int scale, u16 fg16)
+{
+	unsigned int y, x;
+
+	for (y = 0; y < height; y++)
+		for (x = 0; x < width; x++)
+			if (drm_draw_is_pixel_fg(sbuf8, spitch, x / scale, y / scale))
+				iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, fg16);
+}
+EXPORT_SYMBOL(drm_draw_blit16);
+
+void drm_draw_blit24(struct iosys_map *dmap, unsigned int dpitch,
+		     const u8 *sbuf8, unsigned int spitch,
+		     unsigned int height, unsigned int width,
+		     unsigned int scale, u32 fg32)
+{
+	unsigned int y, x;
+
+	for (y = 0; y < height; y++) {
+		for (x = 0; x < width; x++) {
+			u32 off = y * dpitch + x * 3;
+
+			if (drm_draw_is_pixel_fg(sbuf8, spitch, x / scale, y / scale)) {
+				/* write blue-green-red to output in little endianness */
+				iosys_map_wr(dmap, off, u8, (fg32 & 0x000000FF) >> 0);
+				iosys_map_wr(dmap, off + 1, u8, (fg32 & 0x0000FF00) >> 8);
+				iosys_map_wr(dmap, off + 2, u8, (fg32 & 0x00FF0000) >> 16);
+			}
+		}
+	}
+}
+EXPORT_SYMBOL(drm_draw_blit24);
+
+void drm_draw_blit32(struct iosys_map *dmap, unsigned int dpitch,
+		     const u8 *sbuf8, unsigned int spitch,
+		     unsigned int height, unsigned int width,
+		     unsigned int scale, u32 fg32)
+{
+	unsigned int y, x;
+
+	for (y = 0; y < height; y++)
+		for (x = 0; x < width; x++)
+			if (drm_draw_is_pixel_fg(sbuf8, spitch, x / scale, y / scale))
+				iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, fg32);
+}
+EXPORT_SYMBOL(drm_draw_blit32);
+
+/*
+ * Fill functions
+ */
+void drm_draw_fill16(struct iosys_map *dmap, unsigned int dpitch,
+		     unsigned int height, unsigned int width,
+		     u16 color)
+{
+	unsigned int y, x;
+
+	for (y = 0; y < height; y++)
+		for (x = 0; x < width; x++)
+			iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, color);
+}
+EXPORT_SYMBOL(drm_draw_fill16);
+
+void drm_draw_fill24(struct iosys_map *dmap, unsigned int dpitch,
+		     unsigned int height, unsigned int width,
+		     u16 color)
+{
+	unsigned int y, x;
+
+	for (y = 0; y < height; y++) {
+		for (x = 0; x < width; x++) {
+			unsigned int off = y * dpitch + x * 3;
+
+			/* write blue-green-red to output in little endianness */
+			iosys_map_wr(dmap, off, u8, (color & 0x000000FF) >> 0);
+			iosys_map_wr(dmap, off + 1, u8, (color & 0x0000FF00) >> 8);
+			iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 16);
+		}
+	}
+}
+EXPORT_SYMBOL(drm_draw_fill24);
+
+void drm_draw_fill32(struct iosys_map *dmap, unsigned int dpitch,
+		     unsigned int height, unsigned int width,
+		     u32 color)
+{
+	unsigned int y, x;
+
+	for (y = 0; y < height; y++)
+		for (x = 0; x < width; x++)
+			iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color);
+}
+EXPORT_SYMBOL(drm_draw_fill32);
diff --git a/drivers/gpu/drm/drm_draw.h b/drivers/gpu/drm/drm_draw.h
new file mode 100644
index 0000000000000..b14752e4c4acf
--- /dev/null
+++ b/drivers/gpu/drm/drm_draw.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+/*
+ * Copyright (c) 2023 Red Hat.
+ * Author: Jocelyn Falempe <jfalempe@redhat.com>
+ */
+
+#ifndef __DRM_DRAW_H__
+#define __DRM_DRAW_H__
+
+#include <linux/font.h>
+#include <linux/types.h>
+
+struct iosys_map;
+
+/* check if the pixel at coord x,y is 1 (foreground) or 0 (background) */
+static inline bool drm_draw_is_pixel_fg(const u8 *sbuf8, unsigned int spitch, int x, int y)
+{
+	return (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) != 0;
+}
+
+static inline const u8 *drm_draw_get_char_bitmap(const struct font_desc *font,
+						 char c, size_t font_pitch)
+{
+	return font->data + (c * font->height) * font_pitch;
+}
+
+u32 drm_draw_color_from_xrgb8888(u32 color, u32 format);
+
+void drm_draw_blit16(struct iosys_map *dmap, unsigned int dpitch,
+		     const u8 *sbuf8, unsigned int spitch,
+		     unsigned int height, unsigned int width,
+		     unsigned int scale, u16 fg16);
+
+void drm_draw_blit24(struct iosys_map *dmap, unsigned int dpitch,
+		     const u8 *sbuf8, unsigned int spitch,
+		     unsigned int height, unsigned int width,
+		     unsigned int scale, u32 fg32);
+
+void drm_draw_blit32(struct iosys_map *dmap, unsigned int dpitch,
+		     const u8 *sbuf8, unsigned int spitch,
+		     unsigned int height, unsigned int width,
+		     unsigned int scale, u32 fg32);
+
+void drm_draw_fill16(struct iosys_map *dmap, unsigned int dpitch,
+		     unsigned int height, unsigned int width,
+		     u16 color);
+
+void drm_draw_fill24(struct iosys_map *dmap, unsigned int dpitch,
+		     unsigned int height, unsigned int width,
+		     u16 color);
+
+void drm_draw_fill32(struct iosys_map *dmap, unsigned int dpitch,
+		     unsigned int height, unsigned int width,
+		     u32 color);
+
+#endif /* __DRM_DRAW_H__ */
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 74412b7bf936c..27fe83cfc854b 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -31,6 +31,7 @@
 #include <drm/drm_rect.h>
 
 #include "drm_crtc_internal.h"
+#include "drm_draw.h"
 
 MODULE_AUTHOR("Jocelyn Falempe");
 MODULE_DESCRIPTION("DRM panic handler");
@@ -139,171 +140,8 @@ device_initcall(drm_panic_setup_logo);
 #endif
 
 /*
- * Color conversion
+ *  Blit & Fill functions
  */
-
-static u16 convert_xrgb8888_to_rgb565(u32 pix)
-{
-	return ((pix & 0x00F80000) >> 8) |
-	       ((pix & 0x0000FC00) >> 5) |
-	       ((pix & 0x000000F8) >> 3);
-}
-
-static u16 convert_xrgb8888_to_rgba5551(u32 pix)
-{
-	return ((pix & 0x00f80000) >> 8) |
-	       ((pix & 0x0000f800) >> 5) |
-	       ((pix & 0x000000f8) >> 2) |
-	       BIT(0); /* set alpha bit */
-}
-
-static u16 convert_xrgb8888_to_xrgb1555(u32 pix)
-{
-	return ((pix & 0x00f80000) >> 9) |
-	       ((pix & 0x0000f800) >> 6) |
-	       ((pix & 0x000000f8) >> 3);
-}
-
-static u16 convert_xrgb8888_to_argb1555(u32 pix)
-{
-	return BIT(15) | /* set alpha bit */
-	       ((pix & 0x00f80000) >> 9) |
-	       ((pix & 0x0000f800) >> 6) |
-	       ((pix & 0x000000f8) >> 3);
-}
-
-static u32 convert_xrgb8888_to_argb8888(u32 pix)
-{
-	return pix | GENMASK(31, 24); /* fill alpha bits */
-}
-
-static u32 convert_xrgb8888_to_xbgr8888(u32 pix)
-{
-	return ((pix & 0x00ff0000) >> 16) <<  0 |
-	       ((pix & 0x0000ff00) >>  8) <<  8 |
-	       ((pix & 0x000000ff) >>  0) << 16 |
-	       ((pix & 0xff000000) >> 24) << 24;
-}
-
-static u32 convert_xrgb8888_to_abgr8888(u32 pix)
-{
-	return ((pix & 0x00ff0000) >> 16) <<  0 |
-	       ((pix & 0x0000ff00) >>  8) <<  8 |
-	       ((pix & 0x000000ff) >>  0) << 16 |
-	       GENMASK(31, 24); /* fill alpha bits */
-}
-
-static u32 convert_xrgb8888_to_xrgb2101010(u32 pix)
-{
-	pix = ((pix & 0x000000FF) << 2) |
-	      ((pix & 0x0000FF00) << 4) |
-	      ((pix & 0x00FF0000) << 6);
-	return pix | ((pix >> 8) & 0x00300C03);
-}
-
-static u32 convert_xrgb8888_to_argb2101010(u32 pix)
-{
-	pix = ((pix & 0x000000FF) << 2) |
-	      ((pix & 0x0000FF00) << 4) |
-	      ((pix & 0x00FF0000) << 6);
-	return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) & 0x00300C03);
-}
-
-/*
- * convert_from_xrgb8888 - convert one pixel from xrgb8888 to the desired format
- * @color: input color, in xrgb8888 format
- * @format: output format
- *
- * Returns:
- * Color in the format specified, casted to u32.
- * Or 0 if the format is not supported.
- */
-static u32 convert_from_xrgb8888(u32 color, u32 format)
-{
-	switch (format) {
-	case DRM_FORMAT_RGB565:
-		return convert_xrgb8888_to_rgb565(color);
-	case DRM_FORMAT_RGBA5551:
-		return convert_xrgb8888_to_rgba5551(color);
-	case DRM_FORMAT_XRGB1555:
-		return convert_xrgb8888_to_xrgb1555(color);
-	case DRM_FORMAT_ARGB1555:
-		return convert_xrgb8888_to_argb1555(color);
-	case DRM_FORMAT_RGB888:
-	case DRM_FORMAT_XRGB8888:
-		return color;
-	case DRM_FORMAT_ARGB8888:
-		return convert_xrgb8888_to_argb8888(color);
-	case DRM_FORMAT_XBGR8888:
-		return convert_xrgb8888_to_xbgr8888(color);
-	case DRM_FORMAT_ABGR8888:
-		return convert_xrgb8888_to_abgr8888(color);
-	case DRM_FORMAT_XRGB2101010:
-		return convert_xrgb8888_to_xrgb2101010(color);
-	case DRM_FORMAT_ARGB2101010:
-		return convert_xrgb8888_to_argb2101010(color);
-	default:
-		WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
-		return 0;
-	}
-}
-
-/*
- * Blit & Fill
- */
-/* check if the pixel at coord x,y is 1 (foreground) or 0 (background) */
-static bool drm_panic_is_pixel_fg(const u8 *sbuf8, unsigned int spitch, int x, int y)
-{
-	return (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) != 0;
-}
-
-static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch,
-			     const u8 *sbuf8, unsigned int spitch,
-			     unsigned int height, unsigned int width,
-			     unsigned int scale, u16 fg16)
-{
-	unsigned int y, x;
-
-	for (y = 0; y < height; y++)
-		for (x = 0; x < width; x++)
-			if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale))
-				iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, fg16);
-}
-
-static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch,
-			     const u8 *sbuf8, unsigned int spitch,
-			     unsigned int height, unsigned int width,
-			     unsigned int scale, u32 fg32)
-{
-	unsigned int y, x;
-
-	for (y = 0; y < height; y++) {
-		for (x = 0; x < width; x++) {
-			u32 off = y * dpitch + x * 3;
-
-			if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale)) {
-				/* write blue-green-red to output in little endianness */
-				iosys_map_wr(dmap, off, u8, (fg32 & 0x000000FF) >> 0);
-				iosys_map_wr(dmap, off + 1, u8, (fg32 & 0x0000FF00) >> 8);
-				iosys_map_wr(dmap, off + 2, u8, (fg32 & 0x00FF0000) >> 16);
-			}
-		}
-	}
-}
-
-static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch,
-			     const u8 *sbuf8, unsigned int spitch,
-			     unsigned int height, unsigned int width,
-			     unsigned int scale, u32 fg32)
-{
-	unsigned int y, x;
-
-	for (y = 0; y < height; y++)
-		for (x = 0; x < width; x++)
-			if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale))
-				iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, fg32);
-}
-
 static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect *clip,
 				 const u8 *sbuf8, unsigned int spitch, unsigned int scale,
 				 u32 fg_color)
@@ -312,7 +150,7 @@ static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect
 
 	for (y = 0; y < drm_rect_height(clip); y++)
 		for (x = 0; x < drm_rect_width(clip); x++)
-			if (drm_panic_is_pixel_fg(sbuf8, spitch, x / scale, y / scale))
+			if (drm_draw_is_pixel_fg(sbuf8, spitch, x / scale, y / scale))
 				sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, fg_color);
 }
 
@@ -344,15 +182,15 @@ static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip,
 
 	switch (sb->format->cpp[0]) {
 	case 2:
-		drm_panic_blit16(&map, sb->pitch[0], sbuf8, spitch,
+		drm_draw_blit16(&map, sb->pitch[0], sbuf8, spitch,
 				 drm_rect_height(clip), drm_rect_width(clip), scale, fg_color);
 	break;
 	case 3:
-		drm_panic_blit24(&map, sb->pitch[0], sbuf8, spitch,
+		drm_draw_blit24(&map, sb->pitch[0], sbuf8, spitch,
 				 drm_rect_height(clip), drm_rect_width(clip), scale, fg_color);
 	break;
 	case 4:
-		drm_panic_blit32(&map, sb->pitch[0], sbuf8, spitch,
+		drm_draw_blit32(&map, sb->pitch[0], sbuf8, spitch,
 				 drm_rect_height(clip), drm_rect_width(clip), scale, fg_color);
 	break;
 	default:
@@ -360,46 +198,6 @@ static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip,
 	}
 }
 
-static void drm_panic_fill16(struct iosys_map *dmap, unsigned int dpitch,
-			     unsigned int height, unsigned int width,
-			     u16 color)
-{
-	unsigned int y, x;
-
-	for (y = 0; y < height; y++)
-		for (x = 0; x < width; x++)
-			iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, color);
-}
-
-static void drm_panic_fill24(struct iosys_map *dmap, unsigned int dpitch,
-			     unsigned int height, unsigned int width,
-			     u32 color)
-{
-	unsigned int y, x;
-
-	for (y = 0; y < height; y++) {
-		for (x = 0; x < width; x++) {
-			unsigned int off = y * dpitch + x * 3;
-
-			/* write blue-green-red to output in little endianness */
-			iosys_map_wr(dmap, off, u8, (color & 0x000000FF) >> 0);
-			iosys_map_wr(dmap, off + 1, u8, (color & 0x0000FF00) >> 8);
-			iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 16);
-		}
-	}
-}
-
-static void drm_panic_fill32(struct iosys_map *dmap, unsigned int dpitch,
-			     unsigned int height, unsigned int width,
-			     u32 color)
-{
-	unsigned int y, x;
-
-	for (y = 0; y < height; y++)
-		for (x = 0; x < width; x++)
-			iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color);
-}
-
 static void drm_panic_fill_pixel(struct drm_scanout_buffer *sb,
 				 struct drm_rect *clip,
 				 u32 color)
@@ -432,15 +230,15 @@ static void drm_panic_fill(struct drm_scanout_buffer *sb, struct drm_rect *clip,
 
 	switch (sb->format->cpp[0]) {
 	case 2:
-		drm_panic_fill16(&map, sb->pitch[0], drm_rect_height(clip),
+		drm_draw_fill16(&map, sb->pitch[0], drm_rect_height(clip),
 				 drm_rect_width(clip), color);
 	break;
 	case 3:
-		drm_panic_fill24(&map, sb->pitch[0], drm_rect_height(clip),
+		drm_draw_fill24(&map, sb->pitch[0], drm_rect_height(clip),
 				 drm_rect_width(clip), color);
 	break;
 	case 4:
-		drm_panic_fill32(&map, sb->pitch[0], drm_rect_height(clip),
+		drm_draw_fill32(&map, sb->pitch[0], drm_rect_height(clip),
 				 drm_rect_width(clip), color);
 	break;
 	default:
@@ -448,11 +246,6 @@ static void drm_panic_fill(struct drm_scanout_buffer *sb, struct drm_rect *clip,
 	}
 }
 
-static const u8 *get_char_bitmap(const struct font_desc *font, char c, size_t font_pitch)
-{
-	return font->data + (c * font->height) * font_pitch;
-}
-
 static unsigned int get_max_line_len(const struct drm_panic_line *lines, int len)
 {
 	int i;
@@ -491,7 +284,7 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb,
 			rec.x1 += (drm_rect_width(clip) - (line_len * font->width)) / 2;
 
 		for (j = 0; j < line_len; j++) {
-			src = get_char_bitmap(font, msg[i].txt[j], font_pitch);
+			src = drm_draw_get_char_bitmap(font, msg[i].txt[j], font_pitch);
 			rec.x2 = rec.x1 + font->width;
 			drm_panic_blit(sb, &rec, src, font_pitch, 1, color);
 			rec.x1 += font->width;
@@ -523,8 +316,10 @@ static void drm_panic_logo_draw(struct drm_scanout_buffer *sb, struct drm_rect *
 
 static void draw_panic_static_user(struct drm_scanout_buffer *sb)
 {
-	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);
+	u32 fg_color = drm_draw_color_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR,
+						    sb->format->format);
+	u32 bg_color = drm_draw_color_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 msg_width, msg_height;
@@ -590,8 +385,10 @@ static int draw_line_with_wrap(struct drm_scanout_buffer *sb, const struct font_
  */
 static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb)
 {
-	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);
+	u32 fg_color = drm_draw_color_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR,
+						    sb->format->format);
+	u32 bg_color = drm_draw_color_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 = DRM_RECT_INIT(0, 0, sb->width, sb->height);
 	struct kmsg_dump_iter iter;
@@ -781,8 +578,10 @@ static int drm_panic_get_qr_code(u8 **qr_image)
  */
 static int _draw_panic_static_qr_code(struct drm_scanout_buffer *sb)
 {
-	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);
+	u32 fg_color = drm_draw_color_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR,
+						    sb->format->format);
+	u32 bg_color = drm_draw_color_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, r_qr, r_qr_canvas;
 	unsigned int max_qr_size, scale;
@@ -868,7 +667,7 @@ static bool drm_panic_is_format_supported(const struct drm_format_info *format)
 {
 	if (format->num_planes != 1)
 		return false;
-	return convert_from_xrgb8888(0xffffff, format->format) != 0;
+	return drm_draw_color_from_xrgb8888(0xffffff, format->format) != 0;
 }
 
 static void draw_panic_dispatch(struct drm_scanout_buffer *sb)
-- 
2.47.0

[PATCH v5 2/6] drm/log: Introduce a new boot logger to draw the kmsg on the screen

[Jocelyn Falempe]

drm_log is a simple logger that uses the drm_client API to print the
kmsg boot log on the screen. This is not a full replacement to fbcon,
as it will only print the kmsg. It will never handle user input, or a
terminal because this is better done in userspace.

Design decisions:
 * It uses the drm_client API, so it should work on all drm drivers
   from the start.
 * It doesn't scroll the message, that way it doesn't need to redraw
   the whole screen for each new message.
   It also means it doesn't have to keep drawn messages in memory, to
   redraw them when scrolling.
 * It uses the new non-blocking console API, so it should work well
   with PREEMPT_RT.

This patch also adds a Kconfig menu to select the drm client to use.
It can be overwritten on the kernel command line with:
drm_client_lib.default=log or drm_client_lib.default=fbdev

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
Reviewed-by: John Ogness <john.ogness@linutronix.de> # console API
---


v2:
 * Use vmap_local() api, with that change, I've tested it successfully on simpledrm, virtio-gpu, amdgpu, and nouveau.
 * Stop drawing when the drm_master is taken. This avoid wasting CPU cycle if the buffer is not visible.
 * Use deferred probe. Only do the probe the first time there is a log to draw. With this, if you boot with quiet, drm_log won't do any modeset.
 * Add color support for the timestamp prefix, like what dmesg does.
 * Add build dependency on  disabling the fbdev emulation, as they are both drm_client, and there is no way to choose which one gets the focus.

v3:
 * Remove the work thread and circular buffer, and use the new write_thread() console API.
 * Register a console for each drm driver.

v4:
 * Can be built as a module, even if that's not really useful.
 * Rebased on top of "drm: Introduce DRM client library" series from Thomas Zimmermann.
 * Add a Kconfig menu to choose between drm client.
 
v5:
 * Build drm_log in drm_client_lib module, to avoid circular dependency.

 drivers/gpu/drm/Kconfig            |  46 ++++
 drivers/gpu/drm/Makefile           |   1 +
 drivers/gpu/drm/drm_client_setup.c |  18 +-
 drivers/gpu/drm/drm_log.c          | 370 +++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_log.h          |  11 +
 5 files changed, 443 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_log.c
 create mode 100644 drivers/gpu/drm/drm_log.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 3f16dca0b6643..517a1a5cd3c2d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -292,6 +292,52 @@ config DRM_FBDEV_LEAK_PHYS_SMEM
 	  If in doubt, say "N" or spread the word to your closed source
 	  library vendor.
 
+config DRM_LOG
+	bool "Print the kernel boot message on the screen"
+	depends on DRM_CLIENT_SELECTION
+	select DRM_DRAW
+	select DRM_CLIENT_SETUP
+	help
+	  This enable a drm logger, that will print the kernel messages to the
+	  screen until the userspace is ready to take over.
+
+	  If you only need logs, but no terminal, or if you prefer userspace
+	  terminal, say "Y".
+
+choice
+	prompt "Default DRM Client"
+	depends on DRM_CLIENT_SELECTION
+	default DRM_CLIENT_DEFAULT_FBDEV
+	help
+	  Selects the default drm client.
+
+	  The selection made here can be overridden by using the kernel
+	  command line 'drm_client_lib.default=fbdev' option.
+
+config DRM_CLIENT_DEFAULT_FBDEV
+	bool "fbdev"
+	depends on DRM_FBDEV_EMULATION
+	help
+	  Use fbdev emulation as default drm client. This is needed to have
+	  fbcon on top of a drm driver.
+
+config DRM_CLIENT_DEFAULT_LOG
+	bool "log"
+	depends on DRM_LOG
+	help
+	  Use drm log as default drm client. This will display boot logs on the
+	  screen, but doesn't implement a full terminal. For that you will need
+	  a userspace terminal using drm/kms.
+
+endchoice
+
+config DRM_CLIENT_DEFAULT
+       string
+       depends on DRM_CLIENT
+       default "fbdev" if DRM_CLIENT_DEFAULT_FBDEV
+       default "log" if DRM_CLIENT_DEFAULT_LOG
+       default ""
+
 endmenu
 
 config DRM_LOAD_EDID_FIRMWARE
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 68a0a679a7b93..d4c028e839367 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 
 drm_client_lib-y := drm_client_setup.o
 drm_client_lib-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_client.o
+drm_client_lib-$(CONFIG_DRM_LOG) += drm_log.o
 obj-$(CONFIG_DRM_CLIENT_LIB) += drm_client_lib.o
 
 #
diff --git a/drivers/gpu/drm/drm_client_setup.c b/drivers/gpu/drm/drm_client_setup.c
index c14221ca5a0d8..fa44dbee3defc 100644
--- a/drivers/gpu/drm/drm_client_setup.c
+++ b/drivers/gpu/drm/drm_client_setup.c
@@ -6,6 +6,14 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_print.h>
 
+#include "drm_log.h"
+
+static char drm_client_default[16] = CONFIG_DRM_CLIENT_DEFAULT;
+module_param_string(client, drm_client_default, sizeof(drm_client_default), 0444);
+MODULE_PARM_DESC(client,
+		 "Choose which drm client to start, default is"
+		 CONFIG_DRM_CLIENT_DEFAULT "]");
+
 /**
  * drm_client_setup() - Setup in-kernel DRM clients
  * @dev: DRM device
@@ -26,9 +34,13 @@ void drm_client_setup(struct drm_device *dev, const struct drm_format_info *form
 {
 	int ret;
 
-	ret = drm_fbdev_client_setup(dev, format);
-	if (ret)
-		drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
+	if (!strcmp(drm_client_default, "log")) {
+		drm_log_register(dev);
+	} else if (!strcmp(drm_client_default, "fbdev")) {
+		ret = drm_fbdev_client_setup(dev, format);
+		if (ret)
+			drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
+	}
 }
 EXPORT_SYMBOL(drm_client_setup);
 
diff --git a/drivers/gpu/drm/drm_log.c b/drivers/gpu/drm/drm_log.c
new file mode 100644
index 0000000000000..376ee173d99d9
--- /dev/null
+++ b/drivers/gpu/drm/drm_log.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+/*
+ * Copyright (c) 2024 Red Hat.
+ * Author: Jocelyn Falempe <jfalempe@redhat.com>
+ */
+
+#include <linux/console.h>
+#include <linux/font.h>
+#include <linux/init.h>
+#include <linux/iosys-map.h>
+#include <linux/module.h>
+#include <linux/types.h>
+
+#include <drm/drm_client.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_print.h>
+
+#include "drm_draw.h"
+#include "drm_log.h"
+
+MODULE_AUTHOR("Jocelyn Falempe");
+MODULE_DESCRIPTION("DRM boot logger");
+MODULE_LICENSE("GPL");
+
+/**
+ * DOC: overview
+ *
+ * This is a simple graphic logger, to print the kernel message on screen, until
+ * a userspace application is able to take over.
+ * It is only for debugging purpose.
+ */
+
+struct drm_log_scanout {
+	struct drm_client_buffer *buffer;
+	const struct font_desc *font;
+	u32 rows;
+	u32 columns;
+	u32 line;
+	u32 format;
+	u32 px_width;
+	u32 front_color;
+};
+
+struct drm_log {
+	struct mutex lock;
+	struct drm_client_dev client;
+	struct console con;
+	bool probed;
+	u32 n_scanout;
+	struct drm_log_scanout *scanout;
+};
+
+static struct drm_log *client_to_drm_log(struct drm_client_dev *client)
+{
+	return container_of(client, struct drm_log, client);
+}
+
+static struct drm_log *console_to_drm_log(struct console *con)
+{
+	return container_of(con, struct drm_log, con);
+}
+
+static void drm_log_blit(struct iosys_map *dst, unsigned int dst_pitch,
+			 const u8 *src, unsigned int src_pitch,
+			 u32 height, u32 width, u32 scale, u32 px_width, u32 color)
+{
+	switch (px_width) {
+	case 2:
+		drm_draw_blit16(dst, dst_pitch, src, src_pitch, height, width, scale, color);
+		break;
+	case 3:
+		drm_draw_blit24(dst, dst_pitch, src, src_pitch, height, width, scale, color);
+		break;
+	case 4:
+		drm_draw_blit32(dst, dst_pitch, src, src_pitch, height, width, scale, color);
+		break;
+	default:
+		WARN_ONCE(1, "Can't blit with pixel width %d\n", px_width);
+	}
+}
+
+static void drm_log_clear_line(struct drm_log_scanout *scanout, u32 line)
+{
+	struct drm_framebuffer *fb = scanout->buffer->fb;
+	unsigned long height = scanout->font->height;
+	struct iosys_map map;
+	struct drm_rect r = DRM_RECT_INIT(0, line * height, fb->width, height);
+
+	if (drm_client_buffer_vmap_local(scanout->buffer, &map))
+		return;
+	iosys_map_memset(&map, r.y1 * fb->pitches[0], 0, height * fb->pitches[0]);
+	drm_client_buffer_vunmap_local(scanout->buffer);
+	drm_client_framebuffer_flush(scanout->buffer, &r);
+}
+
+static void drm_log_draw_line(struct drm_log_scanout *scanout, const char *s,
+			      unsigned int len)
+{
+	struct drm_framebuffer *fb = scanout->buffer->fb;
+	struct iosys_map map;
+	const struct font_desc *font = scanout->font;
+	size_t font_pitch = DIV_ROUND_UP(font->width, 8);
+	const u8 *src;
+	u32 px_width = fb->format->cpp[0];
+	struct drm_rect r = DRM_RECT_INIT(0, scanout->line * font->height,
+					  fb->width, (scanout->line + 1) * font->height);
+	u32 i;
+
+	if (drm_client_buffer_vmap_local(scanout->buffer, &map))
+		return;
+
+	iosys_map_incr(&map, r.y1 * fb->pitches[0]);
+	for (i = 0; i < len && i < scanout->columns; i++) {
+		src = drm_draw_get_char_bitmap(font, s[i], font_pitch);
+		drm_log_blit(&map, fb->pitches[0], src, font_pitch, font->height, font->width,
+			     1, px_width, scanout->front_color);
+		iosys_map_incr(&map, font->width * px_width);
+	}
+
+	scanout->line++;
+	if (scanout->line >= scanout->rows)
+		scanout->line = 0;
+	drm_client_buffer_vunmap_local(scanout->buffer);
+	drm_client_framebuffer_flush(scanout->buffer, &r);
+}
+
+static void drm_log_draw_new_line(struct drm_log_scanout *scanout,
+				  const char *s, unsigned int len)
+{
+	if (scanout->line == 0) {
+		drm_log_clear_line(scanout, 0);
+		drm_log_clear_line(scanout, 1);
+		drm_log_clear_line(scanout, 2);
+	} else if (scanout->line + 2 < scanout->rows)
+		drm_log_clear_line(scanout, scanout->line + 2);
+
+	drm_log_draw_line(scanout, s, len);
+}
+
+static void drm_log_draw_kmsg_record(struct drm_log_scanout *scanout,
+				     const char *s, unsigned int len)
+{
+	/* do not print the ending \n character */
+	if (s[len - 1] == '\n')
+		len--;
+
+	while (len > scanout->columns) {
+		drm_log_draw_new_line(scanout, s, scanout->columns);
+		s += scanout->columns;
+		len -= scanout->columns;
+	}
+	if (len)
+		drm_log_draw_new_line(scanout, s, len);
+}
+
+static u32 drm_log_find_usable_format(struct drm_plane *plane)
+{
+	int i;
+
+	for (i = 0; i < plane->format_count; i++)
+		if (drm_draw_color_from_xrgb8888(0xffffff, plane->format_types[i]) != 0)
+			return plane->format_types[i];
+	return DRM_FORMAT_INVALID;
+}
+
+static int drm_log_setup_modeset(struct drm_client_dev *client,
+				 struct drm_mode_set *mode_set,
+				 struct drm_log_scanout *scanout)
+{
+	struct drm_crtc *crtc = mode_set->crtc;
+	u32 width = mode_set->mode->hdisplay;
+	u32 height = mode_set->mode->vdisplay;
+	u32 format;
+
+	scanout->font = get_default_font(width, height, NULL, NULL);
+	if (!scanout->font)
+		return -ENOENT;
+
+	format = drm_log_find_usable_format(crtc->primary);
+	if (format == DRM_FORMAT_INVALID)
+		return -EINVAL;
+
+	scanout->buffer = drm_client_framebuffer_create(client, width, height, format);
+	if (IS_ERR(scanout->buffer)) {
+		drm_warn(client->dev, "drm_log can't create framebuffer %d %d %p4cc\n",
+			 width, height, &format);
+		return -ENOMEM;
+	}
+	mode_set->fb = scanout->buffer->fb;
+	scanout->rows = height / scanout->font->height;
+	scanout->columns = width / scanout->font->width;
+	scanout->front_color = drm_draw_color_from_xrgb8888(0xffffff, format);
+	return 0;
+}
+
+static int drm_log_count_modeset(struct drm_client_dev *client)
+{
+	struct drm_mode_set *mode_set;
+	int count = 0;
+
+	mutex_lock(&client->modeset_mutex);
+	drm_client_for_each_modeset(mode_set, client)
+		count++;
+	mutex_unlock(&client->modeset_mutex);
+	return count;
+}
+
+static void drm_log_init_client(struct drm_log *dlog)
+{
+	struct drm_client_dev *client = &dlog->client;
+	struct drm_mode_set *mode_set;
+	int i, max_modeset;
+	int n_modeset = 0;
+
+	dlog->probed = true;
+
+	if (drm_client_modeset_probe(client, 0, 0))
+		return;
+
+	max_modeset = drm_log_count_modeset(client);
+	if (!max_modeset)
+		return;
+
+	dlog->scanout = kcalloc(max_modeset, sizeof(*dlog->scanout), GFP_KERNEL);
+	if (!dlog->scanout)
+		return;
+
+	mutex_lock(&client->modeset_mutex);
+	drm_client_for_each_modeset(mode_set, client) {
+		if (!mode_set->mode)
+			continue;
+		if (drm_log_setup_modeset(client, mode_set, &dlog->scanout[n_modeset]))
+			continue;
+		n_modeset++;
+	}
+	mutex_unlock(&client->modeset_mutex);
+	if (n_modeset == 0)
+		goto err_nomodeset;
+
+	if (drm_client_modeset_commit(client))
+		goto err_failed_commit;
+
+	dlog->n_scanout = n_modeset;
+	return;
+
+err_failed_commit:
+	for (i = 0; i < n_modeset; i++)
+		drm_client_framebuffer_delete(dlog->scanout[i].buffer);
+
+err_nomodeset:
+	kfree(dlog->scanout);
+	dlog->scanout = NULL;
+}
+
+static void drm_log_free_scanout(struct drm_client_dev *client)
+{
+	struct drm_log *dlog = client_to_drm_log(client);
+	int i;
+
+	if (dlog->n_scanout) {
+		for (i = 0; i < dlog->n_scanout; i++)
+			drm_client_framebuffer_delete(dlog->scanout[i].buffer);
+		dlog->n_scanout = 0;
+		kfree(dlog->scanout);
+		dlog->scanout = NULL;
+	}
+}
+
+static void drm_log_client_unregister(struct drm_client_dev *client)
+{
+	struct drm_log *dlog = client_to_drm_log(client);
+	struct drm_device *dev = client->dev;
+
+	unregister_console(&dlog->con);
+
+	mutex_lock(&dlog->lock);
+	drm_log_free_scanout(client);
+	drm_client_release(client);
+	mutex_unlock(&dlog->lock);
+	kfree(dlog);
+	drm_info(dev, "Unregistered with drm log\n");
+}
+
+static int drm_log_client_hotplug(struct drm_client_dev *client)
+{
+	struct drm_log *dlog = client_to_drm_log(client);
+
+	mutex_lock(&dlog->lock);
+	drm_log_free_scanout(client);
+	dlog->probed = false;
+	mutex_unlock(&dlog->lock);
+	return 0;
+}
+
+static const struct drm_client_funcs drm_log_client_funcs = {
+	.owner		= THIS_MODULE,
+	.unregister	= drm_log_client_unregister,
+	.hotplug	= drm_log_client_hotplug,
+};
+
+static void drm_log_write_thread(struct console *con, struct nbcon_write_context *wctxt)
+{
+	struct drm_log *dlog = console_to_drm_log(con);
+	int i;
+
+	if (!dlog->probed)
+		drm_log_init_client(dlog);
+
+	for (i = 0; i < dlog->n_scanout; i++)
+		drm_log_draw_kmsg_record(&dlog->scanout[i], wctxt->outbuf, wctxt->len);
+}
+
+static void drm_log_lock(struct console *con, unsigned long *flags)
+{
+	struct drm_log *dlog = console_to_drm_log(con);
+
+	mutex_lock(&dlog->lock);
+	migrate_disable();
+}
+
+static void drm_log_unlock(struct console *con, unsigned long flags)
+{
+	struct drm_log *dlog = console_to_drm_log(con);
+
+	migrate_enable();
+	mutex_unlock(&dlog->lock);
+}
+
+static void drm_log_register_console(struct console *con)
+{
+	strscpy(con->name, "drm_log");
+	con->write_thread = drm_log_write_thread;
+	con->device_lock = drm_log_lock;
+	con->device_unlock = drm_log_unlock;
+	con->flags = CON_PRINTBUFFER | CON_NBCON;
+	con->index = -1;
+
+	register_console(con);
+}
+
+/**
+ * drm_log_register() - Register a drm device to drm_log
+ * @dev: the drm device to register.
+ */
+void drm_log_register(struct drm_device *dev)
+{
+	struct drm_log *new;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		goto err_warn;
+
+	mutex_init(&new->lock);
+	if (drm_client_init(dev, &new->client, "drm_log", &drm_log_client_funcs))
+		goto err_free;
+
+	drm_client_register(&new->client);
+
+	drm_log_register_console(&new->con);
+
+	drm_info(dev, "Registered with drm log as %s\n", new->con.name);
+	return;
+
+err_free:
+	kfree(new);
+err_warn:
+	drm_warn(dev, "Failed to register with drm log\n");
+}
diff --git a/drivers/gpu/drm/drm_log.h b/drivers/gpu/drm/drm_log.h
new file mode 100644
index 0000000000000..3a4ea81509474
--- /dev/null
+++ b/drivers/gpu/drm/drm_log.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+#ifndef __DRM_LOG_H__
+#define __DRM_LOG_H__
+
+#ifdef CONFIG_DRM_LOG
+void drm_log_register(struct drm_device *dev);
+#else
+static inline void drm_log_register(struct drm_device *dev) {}
+#endif
+
+#endif
-- 
2.47.0

[PATCH v5 3/6] drm/log: Do not draw if drm_master is taken

[Jocelyn Falempe]

When userspace takes drm_master, the drm_client buffer is no more
visible, so drm_log shouldn't waste CPU cycle to draw on it.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_log.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_log.c b/drivers/gpu/drm/drm_log.c
index 376ee173d99d9..226e206e8b6a3 100644
--- a/drivers/gpu/drm/drm_log.c
+++ b/drivers/gpu/drm/drm_log.c
@@ -18,6 +18,7 @@
 #include <drm/drm_print.h>
 
 #include "drm_draw.h"
+#include "drm_internal.h"
 #include "drm_log.h"
 
 MODULE_AUTHOR("Jocelyn Falempe");
@@ -308,8 +309,13 @@ static void drm_log_write_thread(struct console *con, struct nbcon_write_context
 	if (!dlog->probed)
 		drm_log_init_client(dlog);
 
-	for (i = 0; i < dlog->n_scanout; i++)
-		drm_log_draw_kmsg_record(&dlog->scanout[i], wctxt->outbuf, wctxt->len);
+	/* Check that we are still the master before drawing */
+	if (drm_master_internal_acquire(dlog->client.dev)) {
+		drm_master_internal_release(dlog->client.dev);
+
+		for (i = 0; i < dlog->n_scanout; i++)
+			drm_log_draw_kmsg_record(&dlog->scanout[i], wctxt->outbuf, wctxt->len);
+	}
 }
 
 static void drm_log_lock(struct console *con, unsigned long *flags)
-- 
2.47.0

[PATCH v5 4/6] drm/log: Color the timestamp, to improve readability

[Jocelyn Falempe]

Color the timesamp prefix, similar to dmesg.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_log.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_log.c b/drivers/gpu/drm/drm_log.c
index 226e206e8b6a3..635dff7b37ce5 100644
--- a/drivers/gpu/drm/drm_log.c
+++ b/drivers/gpu/drm/drm_log.c
@@ -42,6 +42,7 @@ struct drm_log_scanout {
 	u32 format;
 	u32 px_width;
 	u32 front_color;
+	u32 prefix_color;
 };
 
 struct drm_log {
@@ -97,7 +98,7 @@ static void drm_log_clear_line(struct drm_log_scanout *scanout, u32 line)
 }
 
 static void drm_log_draw_line(struct drm_log_scanout *scanout, const char *s,
-			      unsigned int len)
+			      unsigned int len, unsigned int prefix_len)
 {
 	struct drm_framebuffer *fb = scanout->buffer->fb;
 	struct iosys_map map;
@@ -114,9 +115,10 @@ static void drm_log_draw_line(struct drm_log_scanout *scanout, const char *s,
 
 	iosys_map_incr(&map, r.y1 * fb->pitches[0]);
 	for (i = 0; i < len && i < scanout->columns; i++) {
+		u32 color = (i < prefix_len) ? scanout->prefix_color : scanout->front_color;
 		src = drm_draw_get_char_bitmap(font, s[i], font_pitch);
 		drm_log_blit(&map, fb->pitches[0], src, font_pitch, font->height, font->width,
-			     1, px_width, scanout->front_color);
+			     1, px_width, color);
 		iosys_map_incr(&map, font->width * px_width);
 	}
 
@@ -128,7 +130,7 @@ static void drm_log_draw_line(struct drm_log_scanout *scanout, const char *s,
 }
 
 static void drm_log_draw_new_line(struct drm_log_scanout *scanout,
-				  const char *s, unsigned int len)
+				  const char *s, unsigned int len, unsigned int prefix_len)
 {
 	if (scanout->line == 0) {
 		drm_log_clear_line(scanout, 0);
@@ -137,23 +139,35 @@ static void drm_log_draw_new_line(struct drm_log_scanout *scanout,
 	} else if (scanout->line + 2 < scanout->rows)
 		drm_log_clear_line(scanout, scanout->line + 2);
 
-	drm_log_draw_line(scanout, s, len);
+	drm_log_draw_line(scanout, s, len, prefix_len);
 }
 
+/*
+ * Depends on print_time() in printk.c
+ * Timestamp is written with "[%5lu.%06lu]"
+ */
+#define TS_PREFIX_LEN 13
+
 static void drm_log_draw_kmsg_record(struct drm_log_scanout *scanout,
 				     const char *s, unsigned int len)
 {
+	u32 prefix_len = 0;
+
+	if (len > TS_PREFIX_LEN && s[0] == '[' && s[6] == '.' && s[TS_PREFIX_LEN] == ']')
+		prefix_len = TS_PREFIX_LEN + 1;
+
 	/* do not print the ending \n character */
 	if (s[len - 1] == '\n')
 		len--;
 
 	while (len > scanout->columns) {
-		drm_log_draw_new_line(scanout, s, scanout->columns);
+		drm_log_draw_new_line(scanout, s, scanout->columns, prefix_len);
 		s += scanout->columns;
 		len -= scanout->columns;
+		prefix_len = 0;
 	}
 	if (len)
-		drm_log_draw_new_line(scanout, s, len);
+		drm_log_draw_new_line(scanout, s, len, prefix_len);
 }
 
 static u32 drm_log_find_usable_format(struct drm_plane *plane)
@@ -193,6 +207,7 @@ static int drm_log_setup_modeset(struct drm_client_dev *client,
 	scanout->rows = height / scanout->font->height;
 	scanout->columns = width / scanout->font->width;
 	scanout->front_color = drm_draw_color_from_xrgb8888(0xffffff, format);
+	scanout->prefix_color = drm_draw_color_from_xrgb8888(0x4e9a06, format);
 	return 0;
 }
 
-- 
2.47.0

[PATCH v5 5/6] drm/log: Implement suspend/resume

[Jocelyn Falempe]

The console is already suspended in printk.c.
Just make sure we don't write to the framebuffer while the graphic
driver is suspended.
It may lose a few messages between graphic suspend and console
suspend.

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

diff --git a/drivers/gpu/drm/drm_log.c b/drivers/gpu/drm/drm_log.c
index 635dff7b37ce5..07d1513001468 100644
--- a/drivers/gpu/drm/drm_log.c
+++ b/drivers/gpu/drm/drm_log.c
@@ -50,6 +50,7 @@ struct drm_log {
 	struct drm_client_dev client;
 	struct console con;
 	bool probed;
+	bool suspended;
 	u32 n_scanout;
 	struct drm_log_scanout *scanout;
 };
@@ -310,10 +311,32 @@ static int drm_log_client_hotplug(struct drm_client_dev *client)
 	return 0;
 }
 
+static int drm_log_client_suspend(struct drm_client_dev *client, bool _console_lock)
+{
+	struct drm_log *dlog = client_to_drm_log(client);
+
+	mutex_lock(&dlog->lock);
+	dlog->suspended = true;
+	mutex_unlock(&dlog->lock);
+	return 0;
+}
+
+static int drm_log_client_resume(struct drm_client_dev *client, bool _console_lock)
+{
+	struct drm_log *dlog = client_to_drm_log(client);
+
+	mutex_lock(&dlog->lock);
+	dlog->suspended = false;
+	mutex_unlock(&dlog->lock);
+	return 0;
+}
+
 static const struct drm_client_funcs drm_log_client_funcs = {
 	.owner		= THIS_MODULE,
 	.unregister	= drm_log_client_unregister,
 	.hotplug	= drm_log_client_hotplug,
+	.suspend	= drm_log_client_suspend,
+	.resume		= drm_log_client_resume,
 };
 
 static void drm_log_write_thread(struct console *con, struct nbcon_write_context *wctxt)
@@ -321,6 +344,9 @@ static void drm_log_write_thread(struct console *con, struct nbcon_write_context
 	struct drm_log *dlog = console_to_drm_log(con);
 	int i;
 
+	if (dlog->suspended)
+		return;
+
 	if (!dlog->probed)
 		drm_log_init_client(dlog);
 
-- 
2.47.0

[PATCH v5 6/6] drm/log: Add integer scaling support

[Jocelyn Falempe]

Add a module parameter, to increase the font size for HiDPI screen.
Even with CONFIG_FONT_TER16x32, it can still be a bit small to read.
In this case, adding drm_log.scale=2 to your kernel command line will
double the character size.

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

v5:
 * Change scale parameter to unsigned int (Jani Nikula)

 drivers/gpu/drm/drm_log.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_log.c b/drivers/gpu/drm/drm_log.c
index 07d1513001468..2f378bc953bd9 100644
--- a/drivers/gpu/drm/drm_log.c
+++ b/drivers/gpu/drm/drm_log.c
@@ -25,6 +25,10 @@ MODULE_AUTHOR("Jocelyn Falempe");
 MODULE_DESCRIPTION("DRM boot logger");
 MODULE_LICENSE("GPL");
 
+static unsigned int scale = 1;
+module_param(scale, uint, 0444);
+MODULE_PARM_DESC(scale, "Integer scaling factor for drm_log, default is 1");
+
 /**
  * DOC: overview
  *
@@ -38,6 +42,8 @@ struct drm_log_scanout {
 	const struct font_desc *font;
 	u32 rows;
 	u32 columns;
+	u32 scaled_font_h;
+	u32 scaled_font_w;
 	u32 line;
 	u32 format;
 	u32 px_width;
@@ -67,7 +73,7 @@ static struct drm_log *console_to_drm_log(struct console *con)
 
 static void drm_log_blit(struct iosys_map *dst, unsigned int dst_pitch,
 			 const u8 *src, unsigned int src_pitch,
-			 u32 height, u32 width, u32 scale, u32 px_width, u32 color)
+			 u32 height, u32 width, u32 px_width, u32 color)
 {
 	switch (px_width) {
 	case 2:
@@ -87,7 +93,7 @@ static void drm_log_blit(struct iosys_map *dst, unsigned int dst_pitch,
 static void drm_log_clear_line(struct drm_log_scanout *scanout, u32 line)
 {
 	struct drm_framebuffer *fb = scanout->buffer->fb;
-	unsigned long height = scanout->font->height;
+	unsigned long height = scanout->scaled_font_h;
 	struct iosys_map map;
 	struct drm_rect r = DRM_RECT_INIT(0, line * height, fb->width, height);
 
@@ -107,8 +113,8 @@ static void drm_log_draw_line(struct drm_log_scanout *scanout, const char *s,
 	size_t font_pitch = DIV_ROUND_UP(font->width, 8);
 	const u8 *src;
 	u32 px_width = fb->format->cpp[0];
-	struct drm_rect r = DRM_RECT_INIT(0, scanout->line * font->height,
-					  fb->width, (scanout->line + 1) * font->height);
+	struct drm_rect r = DRM_RECT_INIT(0, scanout->line * scanout->scaled_font_h,
+					  fb->width, (scanout->line + 1) * scanout->scaled_font_h);
 	u32 i;
 
 	if (drm_client_buffer_vmap_local(scanout->buffer, &map))
@@ -118,9 +124,10 @@ static void drm_log_draw_line(struct drm_log_scanout *scanout, const char *s,
 	for (i = 0; i < len && i < scanout->columns; i++) {
 		u32 color = (i < prefix_len) ? scanout->prefix_color : scanout->front_color;
 		src = drm_draw_get_char_bitmap(font, s[i], font_pitch);
-		drm_log_blit(&map, fb->pitches[0], src, font_pitch, font->height, font->width,
-			     1, px_width, color);
-		iosys_map_incr(&map, font->width * px_width);
+		drm_log_blit(&map, fb->pitches[0], src, font_pitch,
+			     scanout->scaled_font_h, scanout->scaled_font_w,
+			     px_width, color);
+		iosys_map_incr(&map, scanout->scaled_font_w * px_width);
 	}
 
 	scanout->line++;
@@ -205,8 +212,10 @@ static int drm_log_setup_modeset(struct drm_client_dev *client,
 		return -ENOMEM;
 	}
 	mode_set->fb = scanout->buffer->fb;
-	scanout->rows = height / scanout->font->height;
-	scanout->columns = width / scanout->font->width;
+	scanout->scaled_font_h = scanout->font->height * scale;
+	scanout->scaled_font_w = scanout->font->width * scale;
+	scanout->rows = height / scanout->scaled_font_h;
+	scanout->columns = width / scanout->scaled_font_w;
 	scanout->front_color = drm_draw_color_from_xrgb8888(0xffffff, format);
 	scanout->prefix_color = drm_draw_color_from_xrgb8888(0x4e9a06, format);
 	return 0;
-- 
2.47.0

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Petr Mladek]

On Wed 2024-10-23 14:00:13, Jocelyn Falempe wrote:
> The console is already suspended in printk.c.

Does this mean that drm_log_client_suspend() is called
after suspend_console(), please?

By other words, does it mean that "dlog->suspended == true" is set
only when CON_SUSPENDED is already set in the related con->flags?

> Just make sure we don't write to the framebuffer while the graphic
> driver is suspended.
> It may lose a few messages between graphic suspend and console
> suspend.

The messages should not get lost when the console is properly
suspended by suspend_console(), set CON_SUSPENDED.

Or maybe, I do not understand it correctly. Maybe you want to say
that it should work correctly even without this patch. And this
patch creates just a safeguard to make sure that nothing wrong
happens even when suspend_console() was not called from some
reasons.


Note: I tried to check the order by reading the code. But
      drm_log_client_suspend() was called via too many layers.
      And I was not able to find where exactly it was called,
      for example, from hibernate() in kernel/power/hibernate.c


> --- a/drivers/gpu/drm/drm_log.c
> +++ b/drivers/gpu/drm/drm_log.c
> @@ -310,10 +311,32 @@ static int drm_log_client_hotplug(struct drm_client_dev *client)
>  	return 0;
>  }
>  
> +static int drm_log_client_suspend(struct drm_client_dev *client, bool _console_lock)
> +{
> +	struct drm_log *dlog = client_to_drm_log(client);
> +
> +	mutex_lock(&dlog->lock);
> +	dlog->suspended = true;
> +	mutex_unlock(&dlog->lock);

It might also be possible to explicitly set the CON_SUSPENDED flag
here to be always on the safe side. We could create variant of
suspend_console() just for one console. Something like:

void suspend_one_console(struct console *con)
{
	struct console *con;

	if (!console_suspend_enabled)
		return;

	pr_info("Suspending console(%s) (use no_console_suspend to debug)\n");
	pr_flush(1000, true);

	console_list_lock();
	if (con && console_is_registered_locked(con))
		console_srcu_write_flags(con, con->flags | CON_SUSPENDED);
	console_list_unlock();

	/*
	 * Ensure that all SRCU list walks have completed. All printing
	 * contexts must be able to see that they are suspended so that it
	 * is guaranteed that all printing has stopped when this function
	 * completes.
	 */
	synchronize_srcu(&console_srcu);
}

and call here:

	suspend_one_console(dlog->con);


But this is not needed when the console is already supposed to be
disabled here. If this is the case then it might be better
to just check and warn when it does not happen. Something like:

void assert_console_suspended(struct console *con)
{
	int cookie;

	cookie = console_srcu_read_lock();

	/* Do not care about unregistered console */
	if (!con || hlist_unhashed_lockless(&con->node))
		goto out;

	if (WARN_ON_ONCE(!(console_srcu_read_flags(con) & CON_SUSPENDED)))
		pr_flush(1000, true);
out:
	console_srcu_read_unlock(cookie);
}

> +	return 0;
> +}


Best Regards,
Petr

PS: I have vacation the following week and might not be able to
    follow the discussion before I am back.

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Jocelyn Falempe]

On 24/10/2024 16:34, Petr Mladek wrote:
> On Wed 2024-10-23 14:00:13, Jocelyn Falempe wrote:
>> The console is already suspended in printk.c.
> 
> Does this mean that drm_log_client_suspend() is called
> after suspend_console(), please?

To be honest, I wasn't able to tell which one is called first, and if 
the order is enforced (I didn't check if drivers can be suspended in 
parallel, or if it's all sequential)..

I then checked if it's possible to suspend the console, but didn't found 
an easy API to do so, so I went with this lazy patch, just ensuring 
we're not writing to a suspended graphic driver.

> 
> By other words, does it mean that "dlog->suspended == true" is set
> only when CON_SUSPENDED is already set in the related con->flags?
> 8
>> Just make sure we don't write to the framebuffer while the graphic
>> driver is suspended.
>> It may lose a few messages between graphic suspend and console
>> suspend.
> 
> The messages should not get lost when the console is properly
> suspended by suspend_console(), set CON_SUSPENDED.
> 
> Or maybe, I do not understand it correctly. Maybe you want to say
> that it should work correctly even without this patch. And this
> patch creates just a safeguard to make sure that nothing wrong
> happens even when suspend_console() was not called from some
> reasons.

I mean that with this patch if the console is suspended after the 
graphic driver, then the message between the suspend of the graphic 
driver and the suspend of the console won't be drawn. I don't see that 
as a big problem, if you debug suspend/resume with drm_log, and the 
screen goes blank, you won't see much anyway. And using dmesg when the 
system is resumed, would have all the messages.

Without this patch, it may crash if the framebuffer is no more 
accessible, and drm_log tries to draw a new line on it.
> 
> 
> Note: I tried to check the order by reading the code. But
>        drm_log_client_suspend() was called via too many layers.
>        And I was not able to find where exactly it was called,
>        for example, from hibernate() in kernel/power/hibernate.c
> 
> 
>> --- a/drivers/gpu/drm/drm_log.c
>> +++ b/drivers/gpu/drm/drm_log.c
>> @@ -310,10 +311,32 @@ static int drm_log_client_hotplug(struct drm_client_dev *client)
>>   	return 0;
>>   }
>>   
>> +static int drm_log_client_suspend(struct drm_client_dev *client, bool _console_lock)
>> +{
>> +	struct drm_log *dlog = client_to_drm_log(client);
>> +
>> +	mutex_lock(&dlog->lock);
>> +	dlog->suspended = true;
>> +	mutex_unlock(&dlog->lock);
> 
> It might also be possible to explicitly set the CON_SUSPENDED flag
> here to be always on the safe side. We could create variant of
> suspend_console() just for one console. Something like:
> 
> void suspend_one_console(struct console *con)
> {
> 	struct console *con;
> 
> 	if (!console_suspend_enabled)
> 		return;
> 
> 	pr_info("Suspending console(%s) (use no_console_suspend to debug)\n");
> 	pr_flush(1000, true);
> 
> 	console_list_lock();
> 	if (con && console_is_registered_locked(con))
> 		console_srcu_write_flags(con, con->flags | CON_SUSPENDED);
> 	console_list_unlock();
> 
> 	/*
> 	 * Ensure that all SRCU list walks have completed. All printing
> 	 * contexts must be able to see that they are suspended so that it
> 	 * is guaranteed that all printing has stopped when this function
> 	 * completes.
> 	 */
> 	synchronize_srcu(&console_srcu);
> }
> 
> and call here:
> 
> 	suspend_one_console(dlog->con);
> 
> 
> But this is not needed when the console is already supposed to be
> disabled here. If this is the case then it might be better
> to just check and warn when it does not happen. Something like:
> 
> void assert_console_suspended(struct console *con)
> {
> 	int cookie;
> 
> 	cookie = console_srcu_read_lock();
> 
> 	/* Do not care about unregistered console */
> 	if (!con || hlist_unhashed_lockless(&con->node))
> 		goto out;
> 
> 	if (WARN_ON_ONCE(!(console_srcu_read_flags(con) & CON_SUSPENDED)))
> 		pr_flush(1000, true);
> out:
> 	console_srcu_read_unlock(cookie);
> }
> 
>> +	return 0;
>> +}
> 
> 

Thanks for this two suggestions, this is really what I was looking for.
I will run some tests on real hardware, to see which one is suspended first.

Best regards,

-- 

Jocelyn

> Best Regards,
> Petr
> 
> PS: I have vacation the following week and might not be able to
>      follow the discussion before I am back.
> 

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Jocelyn Falempe]

On 25/10/2024 01:11, Jocelyn Falempe wrote:
> On 24/10/2024 16:34, Petr Mladek wrote:
>> On Wed 2024-10-23 14:00:13, Jocelyn Falempe wrote:
>>> The console is already suspended in printk.c.
>>
>> Does this mean that drm_log_client_suspend() is called
>> after suspend_console(), please?
> 
> To be honest, I wasn't able to tell which one is called first, and if 
> the order is enforced (I didn't check if drivers can be suspended in 
> parallel, or if it's all sequential)..
> 
> I then checked if it's possible to suspend the console, but didn't found 
> an easy API to do so, so I went with this lazy patch, just ensuring 
> we're not writing to a suspended graphic driver.

I've run some tests on my hardware, and the console is suspended before 
the graphic driver:

[   56.409604] printk: Suspending console(s) (use no_console_suspend to 
debug)
[   56.411430] serial 00:05: disabled
[   56.421877] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[   56.421954] sd 4:0:0:0: [sdb] Synchronizing SCSI cache
[   56.422545] ata1.00: Entering standby power mode
[   56.422793] DRM log suspend

But because there is the "no_console_suspend" parameter, and we should 
make sure to not draw when the graphic driver is suspended, I think this 
patch is needed, and good enough.
I will just rephrase the commit message, to make it clear, that some 
message won't be drawn, only if "no_console_suspend" is set.

Best regards,

-- 

Jocelyn

> 
>>
>> By other words, does it mean that "dlog->suspended == true" is set
>> only when CON_SUSPENDED is already set in the related con->flags?
>> 8
>>> Just make sure we don't write to the framebuffer while the graphic
>>> driver is suspended.
>>> It may lose a few messages between graphic suspend and console
>>> suspend.
>>
>> The messages should not get lost when the console is properly
>> suspended by suspend_console(), set CON_SUSPENDED.
>>
>> Or maybe, I do not understand it correctly. Maybe you want to say
>> that it should work correctly even without this patch. And this
>> patch creates just a safeguard to make sure that nothing wrong
>> happens even when suspend_console() was not called from some
>> reasons.
> 
> I mean that with this patch if the console is suspended after the 
> graphic driver, then the message between the suspend of the graphic 
> driver and the suspend of the console won't be drawn. I don't see that 
> as a big problem, if you debug suspend/resume with drm_log, and the 
> screen goes blank, you won't see much anyway. And using dmesg when the 
> system is resumed, would have all the messages.
> 
> Without this patch, it may crash if the framebuffer is no more 
> accessible, and drm_log tries to draw a new line on it.
>>
>>
>> Note: I tried to check the order by reading the code. But
>>        drm_log_client_suspend() was called via too many layers.
>>        And I was not able to find where exactly it was called,
>>        for example, from hibernate() in kernel/power/hibernate.c
>>
>>
>>> --- a/drivers/gpu/drm/drm_log.c
>>> +++ b/drivers/gpu/drm/drm_log.c
>>> @@ -310,10 +311,32 @@ static int drm_log_client_hotplug(struct 
>>> drm_client_dev *client)
>>>       return 0;
>>>   }
>>> +static int drm_log_client_suspend(struct drm_client_dev *client, 
>>> bool _console_lock)
>>> +{
>>> +    struct drm_log *dlog = client_to_drm_log(client);
>>> +
>>> +    mutex_lock(&dlog->lock);
>>> +    dlog->suspended = true;
>>> +    mutex_unlock(&dlog->lock);
>>
>> It might also be possible to explicitly set the CON_SUSPENDED flag
>> here to be always on the safe side. We could create variant of
>> suspend_console() just for one console. Something like:
>>
>> void suspend_one_console(struct console *con)
>> {
>>     struct console *con;
>>
>>     if (!console_suspend_enabled)
>>         return;
>>
>>     pr_info("Suspending console(%s) (use no_console_suspend to 
>> debug)\n");
>>     pr_flush(1000, true);
>>
>>     console_list_lock();
>>     if (con && console_is_registered_locked(con))
>>         console_srcu_write_flags(con, con->flags | CON_SUSPENDED);
>>     console_list_unlock();
>>
>>     /*
>>      * Ensure that all SRCU list walks have completed. All printing
>>      * contexts must be able to see that they are suspended so that it
>>      * is guaranteed that all printing has stopped when this function
>>      * completes.
>>      */
>>     synchronize_srcu(&console_srcu);
>> }
>>
>> and call here:
>>
>>     suspend_one_console(dlog->con);
>>
>>
>> But this is not needed when the console is already supposed to be
>> disabled here. If this is the case then it might be better
>> to just check and warn when it does not happen. Something like:
>>
>> void assert_console_suspended(struct console *con)
>> {
>>     int cookie;
>>
>>     cookie = console_srcu_read_lock();
>>
>>     /* Do not care about unregistered console */
>>     if (!con || hlist_unhashed_lockless(&con->node))
>>         goto out;
>>
>>     if (WARN_ON_ONCE(!(console_srcu_read_flags(con) & CON_SUSPENDED)))
>>         pr_flush(1000, true);
>> out:
>>     console_srcu_read_unlock(cookie);
>> }
>>
>>> +    return 0;
>>> +}
>>
>>
> 
> Thanks for this two suggestions, this is really what I was looking for.
> I will run some tests on real hardware, to see which one is suspended 
> first.
> 
> Best regards,
> 

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Petr Mladek]

On Fri 2024-10-25 11:46:16, Jocelyn Falempe wrote:
> On 25/10/2024 01:11, Jocelyn Falempe wrote:
> > On 24/10/2024 16:34, Petr Mladek wrote:
> > > On Wed 2024-10-23 14:00:13, Jocelyn Falempe wrote:
> > > > The console is already suspended in printk.c.
> > > 
> > > Does this mean that drm_log_client_suspend() is called
> > > after suspend_console(), please?
> > 
> > To be honest, I wasn't able to tell which one is called first, and if
> > the order is enforced (I didn't check if drivers can be suspended in
> > parallel, or if it's all sequential)..
> > 
> > I then checked if it's possible to suspend the console, but didn't found
> > an easy API to do so, so I went with this lazy patch, just ensuring
> > we're not writing to a suspended graphic driver.
> 
> I've run some tests on my hardware, and the console is suspended before the
> graphic driver:
> 
> [   56.409604] printk: Suspending console(s) (use no_console_suspend to
> debug)
> [   56.411430] serial 00:05: disabled
> [   56.421877] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [   56.421954] sd 4:0:0:0: [sdb] Synchronizing SCSI cache
> [   56.422545] ata1.00: Entering standby power mode
> [   56.422793] DRM log suspend
> 
> But because there is the "no_console_suspend" parameter, and we should make
> sure to not draw when the graphic driver is suspended, I think this patch is
> needed, and good enough.
> I will just rephrase the commit message, to make it clear, that some message
> won't be drawn, only if "no_console_suspend" is set.

Ah, I forgot about the "no_console_suspend" parameter. The problem
with this patch is that it would quietly drop all pending messages.

drm_log_write_thread() does not have any return value.
nbcon_emit_next_record() would assume that the message was printed.
And the kthread would continue emitting next message...

In compare, CON_SUSPENDED would cause that console_is_usable()
returns false. As a result, nbcon_kthread_func() would not try
to emit any message and go into a sleep.

If we set CON_SUSPENDED then the pending messages will get printed
after the resume. If we use this patch, the messages would get lost.


This is why I am not happy with this patch. I would prefer to
block the console. I see three better solutions:

  1. Set CON_SUSPENDED from drm_log_client_suspend even when
     "no_console_suspend" is used.

     It is a bit dirty and might cause some confusion.


  2. Add a new flag, e.g. CON_BLOCKED or CON_DRIVER_BLOCKED,
     which might be used for this purpose.


  3. Allow con->write_thread() to return an error code.

     The question is how exactly the error should be handled.
     The kthread would not know when the printing might succeed
     again.


I personally prefer the 2nd variant.


Best Regards,
Petr

PS: I am sorry for the late reply. I had vacation...

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Jocelyn Falempe]

On 01/11/2024 17:00, Petr Mladek wrote:
> On Fri 2024-10-25 11:46:16, Jocelyn Falempe wrote:
>> On 25/10/2024 01:11, Jocelyn Falempe wrote:
>>> On 24/10/2024 16:34, Petr Mladek wrote:
>>>> On Wed 2024-10-23 14:00:13, Jocelyn Falempe wrote:
>>>>> The console is already suspended in printk.c.
>>>>
>>>> Does this mean that drm_log_client_suspend() is called
>>>> after suspend_console(), please?
>>>
>>> To be honest, I wasn't able to tell which one is called first, and if
>>> the order is enforced (I didn't check if drivers can be suspended in
>>> parallel, or if it's all sequential)..
>>>
>>> I then checked if it's possible to suspend the console, but didn't found
>>> an easy API to do so, so I went with this lazy patch, just ensuring
>>> we're not writing to a suspended graphic driver.
>>
>> I've run some tests on my hardware, and the console is suspended before the
>> graphic driver:
>>
>> [   56.409604] printk: Suspending console(s) (use no_console_suspend to
>> debug)
>> [   56.411430] serial 00:05: disabled
>> [   56.421877] sd 0:0:0:0: [sda] Synchronizing SCSI cache
>> [   56.421954] sd 4:0:0:0: [sdb] Synchronizing SCSI cache
>> [   56.422545] ata1.00: Entering standby power mode
>> [   56.422793] DRM log suspend
>>
>> But because there is the "no_console_suspend" parameter, and we should make
>> sure to not draw when the graphic driver is suspended, I think this patch is
>> needed, and good enough.
>> I will just rephrase the commit message, to make it clear, that some message
>> won't be drawn, only if "no_console_suspend" is set.
> 
> Ah, I forgot about the "no_console_suspend" parameter. The problem
> with this patch is that it would quietly drop all pending messages.
> 
> drm_log_write_thread() does not have any return value.
> nbcon_emit_next_record() would assume that the message was printed.
> And the kthread would continue emitting next message...
> 
> In compare, CON_SUSPENDED would cause that console_is_usable()
> returns false. As a result, nbcon_kthread_func() would not try
> to emit any message and go into a sleep.
> 
> If we set CON_SUSPENDED then the pending messages will get printed
> after the resume. If we use this patch, the messages would get lost.
> 
> 
> This is why I am not happy with this patch. I would prefer to
> block the console. I see three better solutions:
> 
>    1. Set CON_SUSPENDED from drm_log_client_suspend even when
>       "no_console_suspend" is used.
> 
>       It is a bit dirty and might cause some confusion.
> 
> 
>    2. Add a new flag, e.g. CON_BLOCKED or CON_DRIVER_BLOCKED,
>       which might be used for this purpose.
> 
> 
>    3. Allow con->write_thread() to return an error code.
> 
>       The question is how exactly the error should be handled.
>       The kthread would not know when the printing might succeed
>       again.
> 
> 
> I personally prefer the 2nd variant.

I looked at what serial drivers are doing, because they can also have 
their clock gated in suspend.

Would calling console_stop() in the suspend and console_start() in 
resume work ?

https://elixir.bootlin.com/linux/v6.11.6/source/drivers/tty/serial/serial_core.c#L2462

https://elixir.bootlin.com/linux/v6.11.6/source/kernel/printk/printk.c#L3323

It looks like it should do exactly what we need ?

Best regards,

-- 

Jocelyn

> 
> 
> Best Regards,
> Petr
> 
> PS: I am sorry for the late reply. I had vacation...
> 

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[John Ogness]

On 2024-11-04, Jocelyn Falempe <jfalempe@redhat.com> wrote:
> I looked at what serial drivers are doing, because they can also have 
> their clock gated in suspend.
>
> Would calling console_stop() in the suspend and console_start() in 
> resume work ?

Yes. That is what it is for.

John Ogness

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Petr Mladek]

On Mon 2024-11-04 11:52:33, John Ogness wrote:
> On 2024-11-04, Jocelyn Falempe <jfalempe@redhat.com> wrote:
> > I looked at what serial drivers are doing, because they can also have 
> > their clock gated in suspend.
> >
> > Would calling console_stop() in the suspend and console_start() in 
> > resume work ?
> 
> Yes. That is what it is for.

It seems that you are right. I have never really investigated the purpose
of this API /o\

One problem with this API is that it does not check whether the
console is registered. I wonder whether it might cause problems.

For example, we should not set the CON_ENABLE flag when the console is not
registered. Doing so would cause register_console() to always enable
the console, even when it is not preferred.

Additionally, nbcon_kthread_wake() uses con->rcuwait, which is initialized
by nbcon_alloc() called from register_console(). Fortunately, nbcon_alloc()
is always called even if the console is not enabled in the end, but this
might change in the future and cause subtle errors.

[ After even more thinking ]

I wonder whether console_start()/console_stop() should really
manipulate CON_ENABLE flag. It might be historical solution when
@console_suspended was a global variable.

But it has changed with the commit 9e70a5e109a4a2336 ("printk: Add
per-console suspended state").

It might make more sense when console_start()/console_stop()
manipulates CON_SUSPENDED flag. Then it would make sense
to rename them suspend_this_console()/resume_this_console().

What do you think?

Best Regards,
Petr

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Jocelyn Falempe]

On 04/11/2024 15:15, Petr Mladek wrote:
> On Mon 2024-11-04 11:52:33, John Ogness wrote:
>> On 2024-11-04, Jocelyn Falempe <jfalempe@redhat.com> wrote:
>>> I looked at what serial drivers are doing, because they can also have
>>> their clock gated in suspend.
>>>
>>> Would calling console_stop() in the suspend and console_start() in
>>> resume work ?
>>
>> Yes. That is what it is for.
> 
> It seems that you are right. I have never really investigated the purpose
> of this API /o\
> 
Thanks, I will send a v6 with that change.

> One problem with this API is that it does not check whether the
> console is registered. I wonder whether it might cause problems.

At least for drm_log, register_console() will always be called before.
> 
> For example, we should not set the CON_ENABLE flag when the console is not
> registered. Doing so would cause register_console() to always enable
> the console, even when it is not preferred.
> 
> Additionally, nbcon_kthread_wake() uses con->rcuwait, which is initialized
> by nbcon_alloc() called from register_console(). Fortunately, nbcon_alloc()
> is always called even if the console is not enabled in the end, but this
> might change in the future and cause subtle errors.
> 
> [ After even more thinking ]
> 
> I wonder whether console_start()/console_stop() should really
> manipulate CON_ENABLE flag. It might be historical solution when
> @console_suspended was a global variable.
> 
> But it has changed with the commit 9e70a5e109a4a2336 ("printk: Add
> per-console suspended state").
> 
> It might make more sense when console_start()/console_stop()
> manipulates CON_SUSPENDED flag. Then it would make sense
> to rename them suspend_this_console()/resume_this_console().
> 
> What do you think?

Maybe when registering the console, having a flag to say "I want this 
console to be suspended with the console subsystem" or "I want to handle 
suspend/resume on my own, and call the relevant functions" would be better ?

That would avoid having the same console being suspended/resumed twice, 
and making clear what to expect.

Of course "no_console_suspend" won't really work for drivers handling 
suspend/resume themselves.

-- 

Jocelyn
> 
> Best Regards,
> Petr
> 

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[John Ogness]

On 2024-11-04, Petr Mladek <pmladek@suse.com> wrote:
> I wonder whether console_start()/console_stop() should really
> manipulate CON_ENABLE flag. It might be historical solution when
> @console_suspended was a global variable.
>
> But it has changed with the commit 9e70a5e109a4a2336 ("printk: Add
> per-console suspended state").
>
> It might make more sense when console_start()/console_stop()
> manipulates CON_SUSPENDED flag. Then it would make sense
> to rename them suspend_this_console()/resume_this_console().

I worry about letting console drivers and printk.c both modify this flag
during normal runtime. One might clear CON_SUSPENDED too soon and cause
trouble.

CON_ENABLE and @console_suspended were always orthogonal. Moving
@console_suspended to CON_SUSPENDED did not change that relationship.

IMHO we should continue to keep them separate. But your point about the
console not being registered is a good one. We should update
console_stop()/console_start() to only operate on @console if it is
registered. Since those functions take the console_list_lock anyway, it
would be a simple change.

John

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[Petr Mladek]

On Mon 2024-11-04 16:38:53, John Ogness wrote:
> On 2024-11-04, Petr Mladek <pmladek@suse.com> wrote:
> > I wonder whether console_start()/console_stop() should really
> > manipulate CON_ENABLE flag. It might be historical solution when
> > @console_suspended was a global variable.
> >
> > But it has changed with the commit 9e70a5e109a4a2336 ("printk: Add
> > per-console suspended state").
> >
> > It might make more sense when console_start()/console_stop()
> > manipulates CON_SUSPENDED flag. Then it would make sense
> > to rename them suspend_this_console()/resume_this_console().
> 
> I worry about letting console drivers and printk.c both modify this flag
> during normal runtime. One might clear CON_SUSPENDED too soon and cause
> trouble.
> 
> CON_ENABLE and @console_suspended were always orthogonal. Moving
> @console_suspended to CON_SUSPENDED did not change that relationship.
> 
> IMHO we should continue to keep them separate. But your point about the
> console not being registered is a good one. We should update
> console_stop()/console_start() to only operate on @console if it is
> registered. Since those functions take the console_list_lock anyway, it
> would be a simple change.

First, I am fine with using console_start()/console_stop() in this
patchset. I agree that this API was created for this purpose
and should still work fine.

But I think that the API is a bit messy and would deserve a clean up.
We should do it in a separate patchset.


History:

  + commit 56dafec3913935c997 ("Import 2.1.71") in v2.1.71, Nov 2007 [1]

    This version introduced "console=" parameter which allowed to
    choose the consoles on the commandline. Before, they were
    selected at build time.

    The @flags and CON_ENABLED flag were added here as well.
    It looks to me like all available console drivers were registered
    but only consoles with CON_ENABLE flag printed the messages.


  + commit 33c0d1b0c3ebb61243d9b ("[PATCH] Serial driver stuff")
    in v2.5.28, Jul 2002 [1]

    Added generic serial_core. The CON_ENABLED flag was re-used
    to disable console when suspending the serial drivers.


  + commit 557240b48e2dc4f6fa878 ("Add support for suspending and
    resuming the whole console subsystem") in v2.6.18, Jun 2006

    Added @console_suspended global variable. It was used as a big hammer
    to block all console drivers and avoid subtle problems during suspend.


  + commit 9e70a5e109a4a233678 ("printk: Add per-console suspended state")
    in v6.6, Jul 2023

    Replaced the global @console_supended global variable with
    per-console CON_SUSPENDED flag.

    The motivation seems to be to remove dependency on console_lock.
    The per-CPU flag allows to query the state via SRCU.

    But the flag is set for all consoles at the same time in
    console_suspend()/console_resume()

	=> it still works as the big hammer.


Observation:

  + CON_ENABLED is not needed for the original purpose. Only enabled
    consoles are added into @console_list.

  + CON_ENABLED is still used to explicitely block the console driver
    during suspend by console_stop()/console_start() in serial_core.c.

    It is not bad. But it is a bit confusing because we have
    CON_SUSPENDED flag now and this is about suspend/resume.


  + CON_SUSPENDED is per-console flag but it is set synchronously
    for all consoles.

    IMHO, a global variable would make more sense for the big hammer
    purpose.


Big question:

  Does the driver really needs to call console_stop()/console_start()
  when there is the big hammer?

  I would preserve it because it makes the design more robust.

  Anyway, the driver-specific handling looks like the right solution.
  The big hammer feels like a workaround.


Reasonable semantic:

  1. Rename:

	console_suspend() -> console_suspend_all()
	console_resume()  -> console_resume_all()

     and manipulate a global @consoles_suspended variable agagin.
     It is the big hammer API.


  2. Rename:

	console_stop(con)  -> console_suspend(con)
	console_start(con) -> console_resume(con)

     and manipulare the per-console CON_SUSPENDED flag here.


   3. Get rid of the ambiguous CON_ENABLED flag. It won't longer
      have any purpose.

      Except that it is also used to force console registration.
      But it can be done a better way, e.g. by introducing
      register_console_force() API.


As I said, we could/should this clean up in a separate patchset.
Like printk-people should fix the printk-mess.


[1] pre-git linux kernel history:
    git://git.kernel.org/pub/scm/linux/kernel/git/history/history.git

Best Regards,
Petr

Re: [PATCH v5 5/6] drm/log: Implement suspend/resume

[John Ogness]

On 2024-11-05, Petr Mladek <pmladek@suse.com> wrote:
> Observation:
>
>   + CON_ENABLED is not needed for the original purpose. Only enabled
>     consoles are added into @console_list.
>
>   + CON_ENABLED is still used to explicitely block the console driver
>     during suspend by console_stop()/console_start() in serial_core.c.
>
>     It is not bad. But it is a bit confusing because we have
>     CON_SUSPENDED flag now and this is about suspend/resume.

Also note that CON_ENABLED is used to gate ->unblank(). It should
probably consider CON_SUSPENDED as well.

>   + CON_SUSPENDED is per-console flag but it is set synchronously
>     for all consoles.
>
>     IMHO, a global variable would make more sense for the big hammer
>     purpose.
>
>
> Big question:
>
>   Does the driver really needs to call console_stop()/console_start()
>   when there is the big hammer?
>
>   I would preserve it because it makes the design more robust.

Agreed. They serve different purposes.

console_stop()/console_start() is a method for _drivers_ to communicate
that they must not be called because their hardware is not
available/functioning.

console_suspend()/console_resume() is a method for the _system_ to
communicate that consoles should be silent because they are annoying or
we do not trust that they won't cause problems.

>   Anyway, the driver-specific handling looks like the right solution.
>   The big hammer feels like a workaround.

Agreed. Do the 6 call sites even really need the big hammer? I am
guessing yes because there are probably console drivers that do not use
console_stop()/console_start() in their suspend/resume and thus rely on
the whole subsystem being disabled.

> Reasonable semantic:
>
>   1. Rename:
>
> 	console_suspend() -> console_suspend_all()
> 	console_resume()  -> console_resume_all()
>
>      and manipulate a global @consoles_suspended variable agagin.
>      It is the big hammer API.

Agreed. As a global variable, it can still rely on SRCU for
synchronization.

>   2. Rename:
>
> 	console_stop(con)  -> console_suspend(con)
> 	console_start(con) -> console_resume(con)
>
>      and manipulare the per-console CON_SUSPENDED flag here.

Agreed.

>    3. Get rid of the ambiguous CON_ENABLED flag. It won't longer
>       have any purpose.
>
>       Except that it is also used to force console registration.
>       But it can be done a better way, e.g. by introducing
>       register_console_force() API.

Agreed.

John