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

[PATCH v8 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)

v6:
 * Use console_stop() and console_start() in the suspend/resume callback (Petr Mladek).
 * rebase and solve conflict with "drm/panic: Add ABGR2101010 support"

v7:
 * Add a patch fix a build issue due to missing DRM_CLIENT_LIB, reported by kernel test bot.

v8:
 * Rebased after drm client moved to drivers/gpu/drm/clients/
 * Rename DRM_LOG to DRM_CLIENT_LOG (Thomas Zimmermann)
 * Drop "Always select DRM_CLIENT_LIB", and select only if DRM_CLIENT_LOG is set
 * Add an info message if no clients are initialized in drm_client_setup()

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                       |   5 +
 drivers/gpu/drm/Makefile                      |   1 +
 drivers/gpu/drm/clients/Kconfig               |  48 ++
 drivers/gpu/drm/clients/Makefile              |   1 +
 drivers/gpu/drm/clients/drm_client_internal.h |   6 +
 drivers/gpu/drm/clients/drm_client_setup.c    |  29 +-
 drivers/gpu/drm/clients/drm_log.c             | 420 ++++++++++++++++++
 drivers/gpu/drm/drm_draw.c                    | 233 ++++++++++
 drivers/gpu/drm/drm_draw.h                    |  56 +++
 drivers/gpu/drm/drm_panic.c                   | 257 +----------
 10 files changed, 818 insertions(+), 238 deletions(-)
 create mode 100644 drivers/gpu/drm/clients/drm_log.c
 create mode 100644 drivers/gpu/drm/drm_draw.c
 create mode 100644 drivers/gpu/drm/drm_draw.h


base-commit: 7d2faa8dbb7055a115fe0cd6068d7090094a573d
-- 
2.47.0

[PATCH v8 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.

v6:
 * rebase and solve conflict with "drm/panic: Add ABGR2101010 support"

 drivers/gpu/drm/Kconfig     |   5 +
 drivers/gpu/drm/Makefile    |   1 +
 drivers/gpu/drm/drm_draw.c  | 233 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_draw.h  |  56 ++++++++
 drivers/gpu/drm/drm_panic.c | 257 ++++--------------------------------
 5 files changed, 318 insertions(+), 234 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 fd294ccca6bb7..aa31c718fa7cf 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 4f6585be14ccf..863866c8223dc 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..0fadbf4d9e235
--- /dev/null
+++ b/drivers/gpu/drm/drm_draw.c
@@ -0,0 +1,233 @@
+// 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);
+}
+
+static u32 convert_xrgb8888_to_abgr2101010(u32 pix)
+{
+	pix = ((pix & 0x00FF0000) >> 14) |
+	      ((pix & 0x0000FF00) << 4) |
+	      ((pix & 0x000000FF) << 22);
+	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);
+	case DRM_FORMAT_ABGR2101010:
+		return convert_xrgb8888_to_abgr2101010(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 0a9ecc1380d2a..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,181 +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);
-}
-
-static u32 convert_xrgb8888_to_abgr2101010(u32 pix)
-{
-	pix = ((pix & 0x00FF0000) >> 14) |
-	      ((pix & 0x0000FF00) << 4) |
-	      ((pix & 0x000000FF) << 22);
-	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);
-	case DRM_FORMAT_ABGR2101010:
-		return convert_xrgb8888_to_abgr2101010(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)
@@ -322,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);
 }
 
@@ -354,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:
@@ -370,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)
@@ -442,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:
@@ -458,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;
@@ -501,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;
@@ -533,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;
@@ -600,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;
@@ -791,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;
@@ -878,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 v8 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. 

v8:
 * Rebased after drm client moved to drivers/gpu/drm/clients/
 * Rename DRM_LOG to DRM_CLIENT_LOG (Thomas Zimmermann)
 * Add an info message if no clients are initialized in drm_client_setup()

 drivers/gpu/drm/clients/Kconfig               |  48 +++
 drivers/gpu/drm/clients/Makefile              |   1 +
 drivers/gpu/drm/clients/drm_client_internal.h |   6 +
 drivers/gpu/drm/clients/drm_client_setup.c    |  29 +-
 drivers/gpu/drm/clients/drm_log.c             | 370 ++++++++++++++++++
 5 files changed, 450 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/clients/drm_log.c

diff --git a/drivers/gpu/drm/clients/Kconfig b/drivers/gpu/drm/clients/Kconfig
index 01ad3b0001303..7cbdb1b881b45 100644
--- a/drivers/gpu/drm/clients/Kconfig
+++ b/drivers/gpu/drm/clients/Kconfig
@@ -13,6 +13,7 @@ config DRM_CLIENT_SELECTION
 	tristate
 	depends on DRM
 	select DRM_CLIENT_LIB if DRM_FBDEV_EMULATION
+	select DRM_CLIENT_LIB if DRM_CLIENT_LOG
 	help
 	  Drivers that support in-kernel DRM clients have to select this
 	  option.
@@ -70,4 +71,51 @@ config DRM_FBDEV_LEAK_PHYS_SMEM
 	  If in doubt, say "N" or spread the word to your closed source
 	  library vendor.
 
+config DRM_CLIENT_LOG
+	bool "Print the kernel boot message on the screen"
+	depends on DRM_CLIENT_SELECTION
+	select DRM_DRAW
+	select DRM_CLIENT
+	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_CLIENT_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
diff --git a/drivers/gpu/drm/clients/Makefile b/drivers/gpu/drm/clients/Makefile
index 1d004ec92e1ea..9fb776590efe7 100644
--- a/drivers/gpu/drm/clients/Makefile
+++ b/drivers/gpu/drm/clients/Makefile
@@ -2,4 +2,5 @@
 
 drm_client_lib-y := drm_client_setup.o
 drm_client_lib-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_client.o
+drm_client_lib-$(CONFIG_DRM_CLIENT_LOG) += drm_log.o
 obj-$(CONFIG_DRM_CLIENT_LIB) += drm_client_lib.o
diff --git a/drivers/gpu/drm/clients/drm_client_internal.h b/drivers/gpu/drm/clients/drm_client_internal.h
index 23258934956aa..6dc078bf6503b 100644
--- a/drivers/gpu/drm/clients/drm_client_internal.h
+++ b/drivers/gpu/drm/clients/drm_client_internal.h
@@ -16,4 +16,10 @@ static inline int drm_fbdev_client_setup(struct drm_device *dev,
 }
 #endif
 
+#ifdef CONFIG_DRM_CLIENT_LOG
+void drm_log_register(struct drm_device *dev);
+#else
+static inline void drm_log_register(struct drm_device *dev) {}
+#endif
+
 #endif
diff --git a/drivers/gpu/drm/clients/drm_client_setup.c b/drivers/gpu/drm/clients/drm_client_setup.c
index 4b211a4812b5b..92a7b3c180d19 100644
--- a/drivers/gpu/drm/clients/drm_client_setup.c
+++ b/drivers/gpu/drm/clients/drm_client_setup.c
@@ -7,6 +7,12 @@
 
 #include "drm_client_internal.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
@@ -25,11 +31,26 @@
  */
 void drm_client_setup(struct drm_device *dev, const struct drm_format_info *format)
 {
-	int ret;
 
-	ret = drm_fbdev_client_setup(dev, format);
-	if (ret)
-		drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
+#ifdef CONFIG_DRM_FBDEV_EMULATION
+	if (!strcmp(drm_client_default, "fbdev")) {
+		int ret;
+
+		ret = drm_fbdev_client_setup(dev, format);
+		if (ret)
+			drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
+		return;
+	}
+#endif
+
+#ifdef CONFIG_DRM_CLIENT_LOG
+	if (!strcmp(drm_client_default, "log")) {
+		drm_log_register(dev);
+		return;
+	}
+#endif
+
+	drm_info(dev, "No drm client registered\n");
 }
 EXPORT_SYMBOL(drm_client_setup);
 
diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/clients/drm_log.c
new file mode 100644
index 0000000000000..cb793a348f1f9
--- /dev/null
+++ b/drivers/gpu/drm/clients/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_client_internal.h"
+#include "../drm_draw.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");
+}
-- 
2.47.0

[PATCH v8 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/clients/drm_log.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/clients/drm_log.c
index cb793a348f1f9..8eca8044f1f08 100644
--- a/drivers/gpu/drm/clients/drm_log.c
+++ b/drivers/gpu/drm/clients/drm_log.c
@@ -19,6 +19,7 @@
 
 #include "drm_client_internal.h"
 #include "../drm_draw.h"
+#include "../drm_internal.h"
 
 MODULE_AUTHOR("Jocelyn Falempe");
 MODULE_DESCRIPTION("DRM boot logger");
@@ -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 v8 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/clients/drm_log.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/clients/drm_log.c
index 8eca8044f1f08..615396e7b3739 100644
--- a/drivers/gpu/drm/clients/drm_log.c
+++ b/drivers/gpu/drm/clients/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 v8 5/6] drm/log: Implement suspend/resume

[Jocelyn Falempe]

Normally the console is already suspended when the graphic driver
suspend callback is called, but if the parameter no_console_suspend
is set, it might still be active.
So call console_stop()/console_start() in the suspend/resume
callbacks, to make sure it won't try to write to the framebuffer
while the graphic driver is suspended.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
Acked-by: Petr Mladek <pmladek@suse.com>
---

v6:
 * Use console_stop() and console_start() in the suspend/resume callback (Petr Mladek).

 drivers/gpu/drm/clients/drm_log.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/clients/drm_log.c
index 615396e7b3739..03c59784c829f 100644
--- a/drivers/gpu/drm/clients/drm_log.c
+++ b/drivers/gpu/drm/clients/drm_log.c
@@ -310,10 +310,30 @@ 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);
+
+	console_stop(&dlog->con);
+
+	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);
+
+	console_start(&dlog->con);
+
+	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)
-- 
2.47.0

[PATCH v8 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/clients/drm_log.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/clients/drm_log.c
index 03c59784c829f..f9f8becf7f96a 100644
--- a/drivers/gpu/drm/clients/drm_log.c
+++ b/drivers/gpu/drm/clients/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;
@@ -66,7 +72,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:
@@ -86,7 +92,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);
 
@@ -106,8 +112,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))
@@ -117,9 +123,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++;
@@ -204,8 +211,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 v8 2/6] drm/log: Introduce a new boot logger to draw the kmsg on the screen

[Thomas Zimmermann]

Hi


Am 15.11.24 um 14:40 schrieb 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.
>
> v8:
>   * Rebased after drm client moved to drivers/gpu/drm/clients/
>   * Rename DRM_LOG to DRM_CLIENT_LOG (Thomas Zimmermann)
>   * Add an info message if no clients are initialized in drm_client_setup()
>
>   drivers/gpu/drm/clients/Kconfig               |  48 +++
>   drivers/gpu/drm/clients/Makefile              |   1 +
>   drivers/gpu/drm/clients/drm_client_internal.h |   6 +
>   drivers/gpu/drm/clients/drm_client_setup.c    |  29 +-
>   drivers/gpu/drm/clients/drm_log.c             | 370 ++++++++++++++++++
>   5 files changed, 450 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/gpu/drm/clients/drm_log.c
>
> diff --git a/drivers/gpu/drm/clients/Kconfig b/drivers/gpu/drm/clients/Kconfig
> index 01ad3b0001303..7cbdb1b881b45 100644
> --- a/drivers/gpu/drm/clients/Kconfig
> +++ b/drivers/gpu/drm/clients/Kconfig
> @@ -13,6 +13,7 @@ config DRM_CLIENT_SELECTION
>   	tristate
>   	depends on DRM
>   	select DRM_CLIENT_LIB if DRM_FBDEV_EMULATION
> +	select DRM_CLIENT_LIB if DRM_CLIENT_LOG

'if <>' sorted alphabetically please.

>   	help
>   	  Drivers that support in-kernel DRM clients have to select this
>   	  option.
> @@ -70,4 +71,51 @@ config DRM_FBDEV_LEAK_PHYS_SMEM
>   	  If in doubt, say "N" or spread the word to your closed source
>   	  library vendor.
>   
> +config DRM_CLIENT_LOG
> +	bool "Print the kernel boot message on the screen"
> +	depends on DRM_CLIENT_SELECTION
> +	select DRM_DRAW
> +	select DRM_CLIENT
> +	select DRM_CLIENT_SETUP

Select sorted alphabetically please.

> +	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_CLIENT_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
> diff --git a/drivers/gpu/drm/clients/Makefile b/drivers/gpu/drm/clients/Makefile
> index 1d004ec92e1ea..9fb776590efe7 100644
> --- a/drivers/gpu/drm/clients/Makefile
> +++ b/drivers/gpu/drm/clients/Makefile
> @@ -2,4 +2,5 @@
>   
>   drm_client_lib-y := drm_client_setup.o
>   drm_client_lib-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_client.o
> +drm_client_lib-$(CONFIG_DRM_CLIENT_LOG) += drm_log.o
>   obj-$(CONFIG_DRM_CLIENT_LIB) += drm_client_lib.o
> diff --git a/drivers/gpu/drm/clients/drm_client_internal.h b/drivers/gpu/drm/clients/drm_client_internal.h
> index 23258934956aa..6dc078bf6503b 100644
> --- a/drivers/gpu/drm/clients/drm_client_internal.h
> +++ b/drivers/gpu/drm/clients/drm_client_internal.h
> @@ -16,4 +16,10 @@ static inline int drm_fbdev_client_setup(struct drm_device *dev,
>   }
>   #endif
>   
> +#ifdef CONFIG_DRM_CLIENT_LOG
> +void drm_log_register(struct drm_device *dev);
> +#else
> +static inline void drm_log_register(struct drm_device *dev) {}
> +#endif
> +
>   #endif
> diff --git a/drivers/gpu/drm/clients/drm_client_setup.c b/drivers/gpu/drm/clients/drm_client_setup.c
> index 4b211a4812b5b..92a7b3c180d19 100644
> --- a/drivers/gpu/drm/clients/drm_client_setup.c
> +++ b/drivers/gpu/drm/clients/drm_client_setup.c
> @@ -7,6 +7,12 @@
>   
>   #include "drm_client_internal.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 "]");

CONFIG_DRM_CLIENT_DEFAULT is good for the config option. But using 'drm_client_lib.client=log' seems imprecise to me. The module parameter itself should maybe called something like 'setup' or 'active'.

(As a sidenote, if we ever have more clients, we could then modify the parameter to be separated by comma. The user would then be able to specify a set of clients from the ones available.)


> +
>   /**
>    * drm_client_setup() - Setup in-kernel DRM clients
>    * @dev: DRM device
> @@ -25,11 +31,26 @@
>    */
>   void drm_client_setup(struct drm_device *dev, const struct drm_format_info *format)
>   {
> -	int ret;
>   
> -	ret = drm_fbdev_client_setup(dev, format);
> -	if (ret)
> -		drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
> +#ifdef CONFIG_DRM_FBDEV_EMULATION
> +	if (!strcmp(drm_client_default, "fbdev")) {
> +		int ret;
> +
> +		ret = drm_fbdev_client_setup(dev, format);
> +		if (ret)
> +			drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
> +		return;
> +	}
> +#endif
> +
> +#ifdef CONFIG_DRM_CLIENT_LOG
> +	if (!strcmp(drm_client_default, "log")) {
> +		drm_log_register(dev);
> +		return;
> +	}
> +#endif
> +
> +	drm_info(dev, "No drm client registered\n");

Let's remain silent here. If anything, debug please.

>   }
>   EXPORT_SYMBOL(drm_client_setup);
>   
> diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/clients/drm_log.c
> new file mode 100644
> index 0000000000000..cb793a348f1f9
> --- /dev/null
> +++ b/drivers/gpu/drm/clients/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_client_internal.h"
> +#include "../drm_draw.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");

We don't do such messages anywhere. So if anything, debug output here.

> +}
> +
> +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);

Debug please. Don't spam the kernel log with status messages.

Best regards
Thomas

> +	return;
> +
> +err_free:
> +	kfree(new);
> +err_warn:
> +	drm_warn(dev, "Failed to register with drm log\n");
> +}

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Thomas Zimmermann]

Hi


Am 15.11.24 um 14:40 schrieb 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)
>
> v6:
>   * Use console_stop() and console_start() in the suspend/resume callback (Petr Mladek).
>   * rebase and solve conflict with "drm/panic: Add ABGR2101010 support"
>
> v7:
>   * Add a patch fix a build issue due to missing DRM_CLIENT_LIB, reported by kernel test bot.
>
> v8:
>   * Rebased after drm client moved to drivers/gpu/drm/clients/
>   * Rename DRM_LOG to DRM_CLIENT_LOG (Thomas Zimmermann)
>   * Drop "Always select DRM_CLIENT_LIB", and select only if DRM_CLIENT_LOG is set
>   * Add an info message if no clients are initialized in drm_client_setup()
>
> 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

I have a number of minor comments on this patch. For the rest of the series:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>


Best regards
Thomas

>    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                       |   5 +
>   drivers/gpu/drm/Makefile                      |   1 +
>   drivers/gpu/drm/clients/Kconfig               |  48 ++
>   drivers/gpu/drm/clients/Makefile              |   1 +
>   drivers/gpu/drm/clients/drm_client_internal.h |   6 +
>   drivers/gpu/drm/clients/drm_client_setup.c    |  29 +-
>   drivers/gpu/drm/clients/drm_log.c             | 420 ++++++++++++++++++
>   drivers/gpu/drm/drm_draw.c                    | 233 ++++++++++
>   drivers/gpu/drm/drm_draw.h                    |  56 +++
>   drivers/gpu/drm/drm_panic.c                   | 257 +----------
>   10 files changed, 818 insertions(+), 238 deletions(-)
>   create mode 100644 drivers/gpu/drm/clients/drm_log.c
>   create mode 100644 drivers/gpu/drm/drm_draw.c
>   create mode 100644 drivers/gpu/drm/drm_draw.h
>
>
> base-commit: 7d2faa8dbb7055a115fe0cd6068d7090094a573d

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Jocelyn Falempe]

On 03/12/2024 09:48, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 15.11.24 um 14:40 schrieb 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.
>>
>> v8:
>>   * Rebased after drm client moved to drivers/gpu/drm/clients/
>>   * Rename DRM_LOG to DRM_CLIENT_LOG (Thomas Zimmermann)
>>   * Add an info message if no clients are initialized in 
>> drm_client_setup()
>>
>>   drivers/gpu/drm/clients/Kconfig               |  48 +++
>>   drivers/gpu/drm/clients/Makefile              |   1 +
>>   drivers/gpu/drm/clients/drm_client_internal.h |   6 +
>>   drivers/gpu/drm/clients/drm_client_setup.c    |  29 +-
>>   drivers/gpu/drm/clients/drm_log.c             | 370 ++++++++++++++++++
>>   5 files changed, 450 insertions(+), 4 deletions(-)
>>   create mode 100644 drivers/gpu/drm/clients/drm_log.c
>>
>> diff --git a/drivers/gpu/drm/clients/Kconfig b/drivers/gpu/drm/ 
>> clients/Kconfig
>> index 01ad3b0001303..7cbdb1b881b45 100644
>> --- a/drivers/gpu/drm/clients/Kconfig
>> +++ b/drivers/gpu/drm/clients/Kconfig
>> @@ -13,6 +13,7 @@ config DRM_CLIENT_SELECTION
>>       tristate
>>       depends on DRM
>>       select DRM_CLIENT_LIB if DRM_FBDEV_EMULATION
>> +    select DRM_CLIENT_LIB if DRM_CLIENT_LOG
> 
> 'if <>' sorted alphabetically please.

Yes, sure.
> 
>>       help
>>         Drivers that support in-kernel DRM clients have to select this
>>         option.
>> @@ -70,4 +71,51 @@ config DRM_FBDEV_LEAK_PHYS_SMEM
>>         If in doubt, say "N" or spread the word to your closed source
>>         library vendor.
>> +config DRM_CLIENT_LOG
>> +    bool "Print the kernel boot message on the screen"
>> +    depends on DRM_CLIENT_SELECTION
>> +    select DRM_DRAW
>> +    select DRM_CLIENT
>> +    select DRM_CLIENT_SETUP
> 
> Select sorted alphabetically please.

Agreed too
> 
>> +    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_CLIENT_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
>> diff --git a/drivers/gpu/drm/clients/Makefile b/drivers/gpu/drm/ 
>> clients/Makefile
>> index 1d004ec92e1ea..9fb776590efe7 100644
>> --- a/drivers/gpu/drm/clients/Makefile
>> +++ b/drivers/gpu/drm/clients/Makefile
>> @@ -2,4 +2,5 @@
>>   drm_client_lib-y := drm_client_setup.o
>>   drm_client_lib-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_client.o
>> +drm_client_lib-$(CONFIG_DRM_CLIENT_LOG) += drm_log.o
>>   obj-$(CONFIG_DRM_CLIENT_LIB) += drm_client_lib.o
>> diff --git a/drivers/gpu/drm/clients/drm_client_internal.h b/drivers/ 
>> gpu/drm/clients/drm_client_internal.h
>> index 23258934956aa..6dc078bf6503b 100644
>> --- a/drivers/gpu/drm/clients/drm_client_internal.h
>> +++ b/drivers/gpu/drm/clients/drm_client_internal.h
>> @@ -16,4 +16,10 @@ static inline int drm_fbdev_client_setup(struct 
>> drm_device *dev,
>>   }
>>   #endif
>> +#ifdef CONFIG_DRM_CLIENT_LOG
>> +void drm_log_register(struct drm_device *dev);
>> +#else
>> +static inline void drm_log_register(struct drm_device *dev) {}
>> +#endif
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/clients/drm_client_setup.c b/drivers/gpu/ 
>> drm/clients/drm_client_setup.c
>> index 4b211a4812b5b..92a7b3c180d19 100644
>> --- a/drivers/gpu/drm/clients/drm_client_setup.c
>> +++ b/drivers/gpu/drm/clients/drm_client_setup.c
>> @@ -7,6 +7,12 @@
>>   #include "drm_client_internal.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 "]");
> 
> CONFIG_DRM_CLIENT_DEFAULT is good for the config option. But using 
> 'drm_client_lib.client=log' seems imprecise to me. The module parameter 
> itself should maybe called something like 'setup' or 'active'.

Ok, I can rename it to drm_client_lib.active=log

> 
> (As a sidenote, if we ever have more clients, we could then modify the 
> parameter to be separated by comma. The user would then be able to 
> specify a set of clients from the ones available.)

I'm a bit confused about this.
The goal is to allow to build any number of drm clients in the kernel, 
but only one can run. There are no way to switch from one client to 
another at runtime. So what would be the purpose of specifying multiple 
clients?

> 
> 
>> +
>>   /**
>>    * drm_client_setup() - Setup in-kernel DRM clients
>>    * @dev: DRM device
>> @@ -25,11 +31,26 @@
>>    */
>>   void drm_client_setup(struct drm_device *dev, const struct 
>> drm_format_info *format)
>>   {
>> -    int ret;
>> -    ret = drm_fbdev_client_setup(dev, format);
>> -    if (ret)
>> -        drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
>> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>> +    if (!strcmp(drm_client_default, "fbdev")) {
>> +        int ret;
>> +
>> +        ret = drm_fbdev_client_setup(dev, format);
>> +        if (ret)
>> +            drm_warn(dev, "Failed to set up DRM client; error %d\n", 
>> ret);
>> +        return;
>> +    }
>> +#endif
>> +
>> +#ifdef CONFIG_DRM_CLIENT_LOG
>> +    if (!strcmp(drm_client_default, "log")) {
>> +        drm_log_register(dev);
>> +        return;
>> +    }
>> +#endif
>> +
>> +    drm_info(dev, "No drm client registered\n");
> 
> Let's remain silent here. If anything, debug please.

If you get there, it's either a typo, or a misconfiguration, but yes I 
can move it as debug.

> 
>>   }
>>   EXPORT_SYMBOL(drm_client_setup);
>> diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/ 
>> clients/drm_log.c
>> new file mode 100644
>> index 0000000000000..cb793a348f1f9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/clients/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_client_internal.h"
>> +#include "../drm_draw.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");
> 
> We don't do such messages anywhere. So if anything, debug output here.

Agreed.
> 
>> +}
>> +
>> +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);
> 
> Debug please. Don't spam the kernel log with status messages.

Yes I will move that to debug, but it's still much less verbose than the 
current situation with fbdev/fbcon.

[    1.631473] fbcon: Deferring console take-over
[    1.631475] simple-framebuffer simple-framebuffer.0: [drm] fb0: 
simpledrmdrmfb frame buffer device
[    7.058770] fbcon: i915drmfb (fb0) is primary device
[    7.058772] fbcon: Deferring console take-over
[    7.058774] i915 0000:00:02.0: [drm] fb0: i915drmfb frame buffer device

> 
> Best regards
> Thomas
> 
>> +    return;
>> +
>> +err_free:
>> +    kfree(new);
>> +err_warn:
>> +    drm_warn(dev, "Failed to register with drm log\n");
>> +}
> 

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

[Thomas Zimmermann]

Hi


Am 03.12.24 um 10:22 schrieb Jocelyn Falempe:
> On 03/12/2024 09:48, Thomas Zimmermann wrote:
>> Hi
>>
>>
>> Am 15.11.24 um 14:40 schrieb 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.
>>>
>>> v8:
>>>   * Rebased after drm client moved to drivers/gpu/drm/clients/
>>>   * Rename DRM_LOG to DRM_CLIENT_LOG (Thomas Zimmermann)
>>>   * Add an info message if no clients are initialized in 
>>> drm_client_setup()
>>>
>>>   drivers/gpu/drm/clients/Kconfig               |  48 +++
>>>   drivers/gpu/drm/clients/Makefile              |   1 +
>>>   drivers/gpu/drm/clients/drm_client_internal.h |   6 +
>>>   drivers/gpu/drm/clients/drm_client_setup.c    |  29 +-
>>>   drivers/gpu/drm/clients/drm_log.c             | 370 
>>> ++++++++++++++++++
>>>   5 files changed, 450 insertions(+), 4 deletions(-)
>>>   create mode 100644 drivers/gpu/drm/clients/drm_log.c
>>>
>>> diff --git a/drivers/gpu/drm/clients/Kconfig b/drivers/gpu/drm/ 
>>> clients/Kconfig
>>> index 01ad3b0001303..7cbdb1b881b45 100644
>>> --- a/drivers/gpu/drm/clients/Kconfig
>>> +++ b/drivers/gpu/drm/clients/Kconfig
>>> @@ -13,6 +13,7 @@ config DRM_CLIENT_SELECTION
>>>       tristate
>>>       depends on DRM
>>>       select DRM_CLIENT_LIB if DRM_FBDEV_EMULATION
>>> +    select DRM_CLIENT_LIB if DRM_CLIENT_LOG
>>
>> 'if <>' sorted alphabetically please.
>
> Yes, sure.
>>
>>>       help
>>>         Drivers that support in-kernel DRM clients have to select this
>>>         option.
>>> @@ -70,4 +71,51 @@ config DRM_FBDEV_LEAK_PHYS_SMEM
>>>         If in doubt, say "N" or spread the word to your closed source
>>>         library vendor.
>>> +config DRM_CLIENT_LOG
>>> +    bool "Print the kernel boot message on the screen"
>>> +    depends on DRM_CLIENT_SELECTION
>>> +    select DRM_DRAW
>>> +    select DRM_CLIENT
>>> +    select DRM_CLIENT_SETUP
>>
>> Select sorted alphabetically please.
>
> Agreed too
>>
>>> +    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_CLIENT_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
>>> diff --git a/drivers/gpu/drm/clients/Makefile b/drivers/gpu/drm/ 
>>> clients/Makefile
>>> index 1d004ec92e1ea..9fb776590efe7 100644
>>> --- a/drivers/gpu/drm/clients/Makefile
>>> +++ b/drivers/gpu/drm/clients/Makefile
>>> @@ -2,4 +2,5 @@
>>>   drm_client_lib-y := drm_client_setup.o
>>>   drm_client_lib-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_client.o
>>> +drm_client_lib-$(CONFIG_DRM_CLIENT_LOG) += drm_log.o
>>>   obj-$(CONFIG_DRM_CLIENT_LIB) += drm_client_lib.o
>>> diff --git a/drivers/gpu/drm/clients/drm_client_internal.h 
>>> b/drivers/ gpu/drm/clients/drm_client_internal.h
>>> index 23258934956aa..6dc078bf6503b 100644
>>> --- a/drivers/gpu/drm/clients/drm_client_internal.h
>>> +++ b/drivers/gpu/drm/clients/drm_client_internal.h
>>> @@ -16,4 +16,10 @@ static inline int drm_fbdev_client_setup(struct 
>>> drm_device *dev,
>>>   }
>>>   #endif
>>> +#ifdef CONFIG_DRM_CLIENT_LOG
>>> +void drm_log_register(struct drm_device *dev);
>>> +#else
>>> +static inline void drm_log_register(struct drm_device *dev) {}
>>> +#endif
>>> +
>>>   #endif
>>> diff --git a/drivers/gpu/drm/clients/drm_client_setup.c 
>>> b/drivers/gpu/ drm/clients/drm_client_setup.c
>>> index 4b211a4812b5b..92a7b3c180d19 100644
>>> --- a/drivers/gpu/drm/clients/drm_client_setup.c
>>> +++ b/drivers/gpu/drm/clients/drm_client_setup.c
>>> @@ -7,6 +7,12 @@
>>>   #include "drm_client_internal.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 "]");
>>
>> CONFIG_DRM_CLIENT_DEFAULT is good for the config option. But using 
>> 'drm_client_lib.client=log' seems imprecise to me. The module 
>> parameter itself should maybe called something like 'setup' or 'active'.
>
> Ok, I can rename it to drm_client_lib.active=log
>
>>
>> (As a sidenote, if we ever have more clients, we could then modify 
>> the parameter to be separated by comma. The user would then be able 
>> to specify a set of clients from the ones available.)
>
> I'm a bit confused about this.
> The goal is to allow to build any number of drm clients in the kernel, 
> but only one can run. There are no way to switch from one client to 
> another at runtime. So what would be the purpose of specifying 
> multiple clients?

The user could then specify the order of trying them, like in 
'active=log, fdev, bootsplash,foo, bar'. And the kernel would try them 
in this precise order. Only make sense if there are more than two 
clients available of course.

>
>>
>>
>>> +
>>>   /**
>>>    * drm_client_setup() - Setup in-kernel DRM clients
>>>    * @dev: DRM device
>>> @@ -25,11 +31,26 @@
>>>    */
>>>   void drm_client_setup(struct drm_device *dev, const struct 
>>> drm_format_info *format)
>>>   {
>>> -    int ret;
>>> -    ret = drm_fbdev_client_setup(dev, format);
>>> -    if (ret)
>>> -        drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
>>> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>>> +    if (!strcmp(drm_client_default, "fbdev")) {
>>> +        int ret;
>>> +
>>> +        ret = drm_fbdev_client_setup(dev, format);
>>> +        if (ret)
>>> +            drm_warn(dev, "Failed to set up DRM client; error 
>>> %d\n", ret);
>>> +        return;
>>> +    }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_DRM_CLIENT_LOG
>>> +    if (!strcmp(drm_client_default, "log")) {
>>> +        drm_log_register(dev);
>>> +        return;
>>> +    }
>>> +#endif
>>> +
>>> +    drm_info(dev, "No drm client registered\n");
>>
>> Let's remain silent here. If anything, debug please.
>
> If you get there, it's either a typo, or a misconfiguration, but yes I 
> can move it as debug.

It should be a warning then. Something like

   if (strcmp(drm_client_log, ""))
       drm_warn("Unknown DRM client %s\n", drm_client_log)

>
>>
>>>   }
>>>   EXPORT_SYMBOL(drm_client_setup);
>>> diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/ 
>>> clients/drm_log.c
>>> new file mode 100644
>>> index 0000000000000..cb793a348f1f9
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/clients/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_client_internal.h"
>>> +#include "../drm_draw.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");
>>
>> We don't do such messages anywhere. So if anything, debug output here.
>
> Agreed.
>>
>>> +}
>>> +
>>> +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);
>>
>> Debug please. Don't spam the kernel log with status messages.
>
> Yes I will move that to debug, but it's still much less verbose than 
> the current situation with fbdev/fbcon.
>
> [    1.631473] fbcon: Deferring console take-over
> [    1.631475] simple-framebuffer simple-framebuffer.0: [drm] fb0: 
> simpledrmdrmfb frame buffer device
> [    7.058770] fbcon: i915drmfb (fb0) is primary device
> [    7.058772] fbcon: Deferring console take-over
> [    7.058774] i915 0000:00:02.0: [drm] fb0: i915drmfb frame buffer 
> device

I don't like this either. Such output distracts from the important 
messages. But it's been there for ages so I don't dare to remove it.

Best regards
Thomas

>
>>
>> Best regards
>> Thomas
>>
>>> +    return;
>>> +
>>> +err_free:
>>> +    kfree(new);
>>> +err_warn:
>>> +    drm_warn(dev, "Failed to register with drm log\n");
>>> +}
>>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

[Jocelyn Falempe]

On 03/12/2024 10:35, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 03.12.24 um 10:22 schrieb Jocelyn Falempe:
>> On 03/12/2024 09:48, Thomas Zimmermann wrote:
>>> Hi
>>>
>>>
>>> Am 15.11.24 um 14:40 schrieb 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.
>>>>
>>>> v8:
>>>>   * Rebased after drm client moved to drivers/gpu/drm/clients/
>>>>   * Rename DRM_LOG to DRM_CLIENT_LOG (Thomas Zimmermann)
>>>>   * Add an info message if no clients are initialized in 
>>>> drm_client_setup()
>>>>
>>>>   drivers/gpu/drm/clients/Kconfig               |  48 +++
>>>>   drivers/gpu/drm/clients/Makefile              |   1 +
>>>>   drivers/gpu/drm/clients/drm_client_internal.h |   6 +
>>>>   drivers/gpu/drm/clients/drm_client_setup.c    |  29 +-
>>>>   drivers/gpu/drm/clients/drm_log.c             | 370 ++++++++++++++ 
>>>> ++++
>>>>   5 files changed, 450 insertions(+), 4 deletions(-)
>>>>   create mode 100644 drivers/gpu/drm/clients/drm_log.c
>>>>
>>>> diff --git a/drivers/gpu/drm/clients/Kconfig b/drivers/gpu/drm/ 
>>>> clients/Kconfig
>>>> index 01ad3b0001303..7cbdb1b881b45 100644
>>>> --- a/drivers/gpu/drm/clients/Kconfig
>>>> +++ b/drivers/gpu/drm/clients/Kconfig
>>>> @@ -13,6 +13,7 @@ config DRM_CLIENT_SELECTION
>>>>       tristate
>>>>       depends on DRM
>>>>       select DRM_CLIENT_LIB if DRM_FBDEV_EMULATION
>>>> +    select DRM_CLIENT_LIB if DRM_CLIENT_LOG
>>>
>>> 'if <>' sorted alphabetically please.
>>
>> Yes, sure.
>>>
>>>>       help
>>>>         Drivers that support in-kernel DRM clients have to select this
>>>>         option.
>>>> @@ -70,4 +71,51 @@ config DRM_FBDEV_LEAK_PHYS_SMEM
>>>>         If in doubt, say "N" or spread the word to your closed source
>>>>         library vendor.
>>>> +config DRM_CLIENT_LOG
>>>> +    bool "Print the kernel boot message on the screen"
>>>> +    depends on DRM_CLIENT_SELECTION
>>>> +    select DRM_DRAW
>>>> +    select DRM_CLIENT
>>>> +    select DRM_CLIENT_SETUP
>>>
>>> Select sorted alphabetically please.
>>
>> Agreed too
>>>
>>>> +    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_CLIENT_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
>>>> diff --git a/drivers/gpu/drm/clients/Makefile b/drivers/gpu/drm/ 
>>>> clients/Makefile
>>>> index 1d004ec92e1ea..9fb776590efe7 100644
>>>> --- a/drivers/gpu/drm/clients/Makefile
>>>> +++ b/drivers/gpu/drm/clients/Makefile
>>>> @@ -2,4 +2,5 @@
>>>>   drm_client_lib-y := drm_client_setup.o
>>>>   drm_client_lib-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fbdev_client.o
>>>> +drm_client_lib-$(CONFIG_DRM_CLIENT_LOG) += drm_log.o
>>>>   obj-$(CONFIG_DRM_CLIENT_LIB) += drm_client_lib.o
>>>> diff --git a/drivers/gpu/drm/clients/drm_client_internal.h b/ 
>>>> drivers/ gpu/drm/clients/drm_client_internal.h
>>>> index 23258934956aa..6dc078bf6503b 100644
>>>> --- a/drivers/gpu/drm/clients/drm_client_internal.h
>>>> +++ b/drivers/gpu/drm/clients/drm_client_internal.h
>>>> @@ -16,4 +16,10 @@ static inline int drm_fbdev_client_setup(struct 
>>>> drm_device *dev,
>>>>   }
>>>>   #endif
>>>> +#ifdef CONFIG_DRM_CLIENT_LOG
>>>> +void drm_log_register(struct drm_device *dev);
>>>> +#else
>>>> +static inline void drm_log_register(struct drm_device *dev) {}
>>>> +#endif
>>>> +
>>>>   #endif
>>>> diff --git a/drivers/gpu/drm/clients/drm_client_setup.c b/drivers/ 
>>>> gpu/ drm/clients/drm_client_setup.c
>>>> index 4b211a4812b5b..92a7b3c180d19 100644
>>>> --- a/drivers/gpu/drm/clients/drm_client_setup.c
>>>> +++ b/drivers/gpu/drm/clients/drm_client_setup.c
>>>> @@ -7,6 +7,12 @@
>>>>   #include "drm_client_internal.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 "]");
>>>
>>> CONFIG_DRM_CLIENT_DEFAULT is good for the config option. But using 
>>> 'drm_client_lib.client=log' seems imprecise to me. The module 
>>> parameter itself should maybe called something like 'setup' or 'active'.
>>
>> Ok, I can rename it to drm_client_lib.active=log
>>
>>>
>>> (As a sidenote, if we ever have more clients, we could then modify 
>>> the parameter to be separated by comma. The user would then be able 
>>> to specify a set of clients from the ones available.)
>>
>> I'm a bit confused about this.
>> The goal is to allow to build any number of drm clients in the kernel, 
>> but only one can run. There are no way to switch from one client to 
>> another at runtime. So what would be the purpose of specifying 
>> multiple clients?
> 
> The user could then specify the order of trying them, like in 
> 'active=log, fdev, bootsplash,foo, bar'. And the kernel would try them 
> in this precise order. Only make sense if there are more than two 
> clients available of course.

Ok, thanks for the precisions.
> 
>>
>>>
>>>
>>>> +
>>>>   /**
>>>>    * drm_client_setup() - Setup in-kernel DRM clients
>>>>    * @dev: DRM device
>>>> @@ -25,11 +31,26 @@
>>>>    */
>>>>   void drm_client_setup(struct drm_device *dev, const struct 
>>>> drm_format_info *format)
>>>>   {
>>>> -    int ret;
>>>> -    ret = drm_fbdev_client_setup(dev, format);
>>>> -    if (ret)
>>>> -        drm_warn(dev, "Failed to set up DRM client; error %d\n", ret);
>>>> +#ifdef CONFIG_DRM_FBDEV_EMULATION
>>>> +    if (!strcmp(drm_client_default, "fbdev")) {
>>>> +        int ret;
>>>> +
>>>> +        ret = drm_fbdev_client_setup(dev, format);
>>>> +        if (ret)
>>>> +            drm_warn(dev, "Failed to set up DRM client; error 
>>>> %d\n", ret);
>>>> +        return;
>>>> +    }
>>>> +#endif
>>>> +
>>>> +#ifdef CONFIG_DRM_CLIENT_LOG
>>>> +    if (!strcmp(drm_client_default, "log")) {
>>>> +        drm_log_register(dev);
>>>> +        return;
>>>> +    }
>>>> +#endif
>>>> +
>>>> +    drm_info(dev, "No drm client registered\n");
>>>
>>> Let's remain silent here. If anything, debug please.
>>
>> If you get there, it's either a typo, or a misconfiguration, but yes I 
>> can move it as debug.
> 
> It should be a warning then. Something like
> 
>    if (strcmp(drm_client_log, ""))
>        drm_warn("Unknown DRM client %s\n", drm_client_log)

Good idea, maybe even allow to set it as "none" meaning no clients to 
start by default.

if (strcmp(drm_client_log, "") && strcmp(drm_client_log, "none"))
         drm_warn("Unknown DRM client %s\n", drm_client_log)

> 
>>
>>>
>>>>   }
>>>>   EXPORT_SYMBOL(drm_client_setup);
>>>> diff --git a/drivers/gpu/drm/clients/drm_log.c b/drivers/gpu/drm/ 
>>>> clients/drm_log.c
>>>> new file mode 100644
>>>> index 0000000000000..cb793a348f1f9
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/clients/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_client_internal.h"
>>>> +#include "../drm_draw.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");
>>>
>>> We don't do such messages anywhere. So if anything, debug output here.
>>
>> Agreed.
>>>
>>>> +}
>>>> +
>>>> +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);
>>>
>>> Debug please. Don't spam the kernel log with status messages.
>>
>> Yes I will move that to debug, but it's still much less verbose than 
>> the current situation with fbdev/fbcon.
>>
>> [    1.631473] fbcon: Deferring console take-over
>> [    1.631475] simple-framebuffer simple-framebuffer.0: [drm] fb0: 
>> simpledrmdrmfb frame buffer device
>> [    7.058770] fbcon: i915drmfb (fb0) is primary device
>> [    7.058772] fbcon: Deferring console take-over
>> [    7.058774] i915 0000:00:02.0: [drm] fb0: i915drmfb frame buffer 
>> device
> 
> I don't like this either. Such output distracts from the important 
> messages. But it's been there for ages so I don't dare to remove it.

Thanks a lot for the reviews.

Best regards,

-- 

Jocelyn

> 
> Best regards
> Thomas
> 
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>>> +    return;
>>>> +
>>>> +err_free:
>>>> +    kfree(new);
>>>> +err_warn:
>>>> +    drm_warn(dev, "Failed to register with drm log\n");
>>>> +}
>>>
>>
> 

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

[Jani Nikula]

On Fri, 15 Nov 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote:
> 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.

I started looking at this in patch 2:

> +#include "../drm_draw.h"

I think we should avoid #includes with ../ like this.

Either drm_draw.h belongs in include/drm, or maybe clients/Makefile
needs to add subdir-ccflags-y += -I$(src)/.. or something like that?

If it's supposed to be internal, I guess the latter, but then the
current convention is to have _internal.h suffix. All drm headers under
drivers/ have that.

Is this the first drm subsystem internal thing that's a separate module?
Should we use EXPORT_SYMBOL_NS() and MODULE_IMPORT_NS() to enforce it
being internal?


BR,
Jani.


-- 
Jani Nikula, Intel

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

[Jocelyn Falempe]

On 03/12/2024 12:06, Jani Nikula wrote:
> On Fri, 15 Nov 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote:
>> 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.
> 
> I started looking at this in patch 2:
> 
>> +#include "../drm_draw.h"
> 
> I think we should avoid #includes with ../ like this.

Sure, I've added it in v8, after the clients moved to drm/clients/, but 
I didn't think much about it.

> 
> Either drm_draw.h belongs in include/drm, or maybe clients/Makefile
> needs to add subdir-ccflags-y += -I$(src)/.. or something like that?
> 
> If it's supposed to be internal, I guess the latter, but then the
> current convention is to have _internal.h suffix. All drm headers under
> drivers/ have that.

ok, I can rename drm_draw.h to drm_draw_internal.h, and add the include 
in the Makefile.
> 
> Is this the first drm subsystem internal thing that's a separate module?
> Should we use EXPORT_SYMBOL_NS() and MODULE_IMPORT_NS() to enforce it
> being internal?

It's not really a separate module, as it's built in the core drm module. 
(the reason is that it's used by drm_panic too, which must be in the 
core drm module).

I don't know much about symbol namespace, but I can add that if needed.

Best regards,

-- 

Jocelyn

> 
> 
> BR,
> Jani.
> 
> 

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

[Jani Nikula]

On Tue, 03 Dec 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote:
> On 03/12/2024 12:06, Jani Nikula wrote:
>> On Fri, 15 Nov 2024, Jocelyn Falempe <jfalempe@redhat.com> wrote:
>>> 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.
>> 
>> I started looking at this in patch 2:
>> 
>>> +#include "../drm_draw.h"
>> 
>> I think we should avoid #includes with ../ like this.
>
> Sure, I've added it in v8, after the clients moved to drm/clients/, but 
> I didn't think much about it.
>
>> 
>> Either drm_draw.h belongs in include/drm, or maybe clients/Makefile
>> needs to add subdir-ccflags-y += -I$(src)/.. or something like that?
>> 
>> If it's supposed to be internal, I guess the latter, but then the
>> current convention is to have _internal.h suffix. All drm headers under
>> drivers/ have that.
>
> ok, I can rename drm_draw.h to drm_draw_internal.h, and add the include 
> in the Makefile.
>> 
>> Is this the first drm subsystem internal thing that's a separate module?
>> Should we use EXPORT_SYMBOL_NS() and MODULE_IMPORT_NS() to enforce it
>> being internal?
>
> It's not really a separate module, as it's built in the core drm module. 
> (the reason is that it's used by drm_panic too, which must be in the 
> core drm module).

Right, sorry, I got confused and looked at this the other way round.

drm_draw is part of drm.ko, drm log is part of drm_client_lib.ko, and
uses drm_draw, right?

So is drm_draw really internal if drm client uses it?

I don't really deeply care either way, but it bugs me when it's
somewhere in between. :)

Either include/drm/drm_draw.h or drivers/gpu/drm/drm_draw_internal.h,
the latter *possibly* with symbol namespace, but not a big deal.


BR,
Jani.


>
> I don't know much about symbol namespace, but I can add that if needed.
>
> Best regards,

-- 
Jani Nikula, Intel