[RFC][PATCH v4 0/4] drm/panic: Add a drm panic handler

[RFC][PATCH v4 0/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

This introduces a new drm panic handler, which displays a message when a panic occurs.
So when fbcon is disabled, you can still see a kernel panic.

This is one of the missing feature, when disabling VT/fbcon in the kernel:
https://www.reddit.com/r/linux/comments/10eccv9/config_vtn_in_2023/
Fbcon can be replaced by a userspace kms console, but the panic screen must be done in the kernel.

This is a proof of concept, and works with simpledrm and mgag200, using a new get_scanout_buffer() api

To test it, make sure you're using the simpledrm driver, and trigger a panic:
echo c > /proc/sysrq-trigger

v2:
 * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann)
 * Add the panic reason to the panic message (Nerdopolis)
 * Add an exclamation mark (Nerdopolis)
 
v3:
 * Rework the drawing functions, to write the pixels line by line and
 to use the drm conversion helper to support other formats.
 (Thomas Zimmermann)
 
v4:
 * Fully support all simpledrm formats using drm conversion helpers
 * Rename dpanic_* to drm_panic_*, and have more coherent function name.
   (Thomas Zimmermann)
 * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann)
 * Remove the default y to DRM_PANIC config option (Thomas Zimmermann)
 * Add foreground/background color config option
 * Fix the bottom lines not painted if the framebuffer height
   is not a multiple of the font height.
 * Automatically register the driver to drm_panic, if the function
   get_scanout_buffer() exists. (Thomas Zimmermann)
 * Add mgag200 support.
 
With mgag200 support, I was able to test that the xrgb8888 to rgb565 conversion is working.

A few more though:
 1) what about gpu with multiple monitor connected ?
maybe get_scanout_buffer() could return a list of scanout buffers ?
 2) I think for some GPU drivers, there might need a flush_scanout_buffer() function, that should be called after the scanout buffer has been filled ?


Best regards,


Jocelyn Falempe (4):
  drm/format-helper: Export line conversion helper for drm_panic
  drm/panic: Add a drm panic handler
  drm/simpledrm: Add drm_panic support
  drm/mgag200: Add drm_panic support

 drivers/gpu/drm/Kconfig               |  22 ++
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/drm_drv.c             |   8 +
 drivers/gpu/drm/drm_format_helper.c   |  88 +++++-
 drivers/gpu/drm/drm_panic.c           | 413 ++++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.c |  24 ++
 drivers/gpu/drm/tiny/simpledrm.c      |  15 +
 include/drm/drm_drv.h                 |  14 +
 include/drm/drm_format_helper.h       |   9 +
 include/drm/drm_panic.h               |  41 +++
 10 files changed, 627 insertions(+), 8 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_panic.c
 create mode 100644 include/drm/drm_panic.h


base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
-- 
2.41.0

[PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[Jocelyn Falempe]

drm_panic will need the low-level drm_fb_xxxx_line functions.
Also add drm_fb_r1_to_xrgb8888 to render the fonts.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_format_helper.c | 88 ++++++++++++++++++++++++++---
 include/drm/drm_format_helper.h     |  9 +++
 2 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index f93a4efcee90..c238e5d84f1f 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -270,7 +270,30 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
 
 	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, swab_line);
 }
-EXPORT_SYMBOL(drm_fb_swab);
+
+/**
+ * drm_fb_r1_to_32bit_line - Convert one line from monochrome to any 32bit pixel format
+ * @dbuf: Pointer to the destination line (in any 32bit format)
+ * @sbuf: Pointer to the source line (in monochrome)
+ * @pixels: Number of pixels to convert.
+ * @fg_color: Foreground color, applied when R1 is 1
+ * @bg_color: Background color, applied when R1 is 0
+ *
+ * Convert monochrome to any format with 32bit pixel.
+ * There is a limitation, as sbuf is a pointer, it can only points to a multiple
+ * of 8 pixels in the source buffer.
+ */
+void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned int pixels,
+				u32 fg_color, u32 bg_color)
+{
+	unsigned int x;
+	const u8 *sbuf8 = sbuf;
+	u32 *dubf32 = dbuf;
+
+	for (x = 0; x < pixels; x++)
+		dubf32[x] = (sbuf8[x / 8] & (0x80 >> (x % 8))) ? fg_color : bg_color;
+}
+EXPORT_SYMBOL(drm_fb_r1_to_32bit_line);
 
 static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
@@ -320,7 +343,13 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pi
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
 
-static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
+/**
+ * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to RGB565
+ * @dbuf: Pointer to the destination line (in RGB565)
+ * @sbuf: Pointer to the source line (in XRGB8888)
+ * @pixels: Number of pixels to convert.
+ */
+void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
@@ -336,6 +365,7 @@ static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigne
 		dbuf16[x] = cpu_to_le16(val16);
 	}
 }
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565_line);
 
 /* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
 static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
@@ -396,7 +426,13 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
 
-static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
+/**
+ * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to XRGB1555
+ * @dbuf: Pointer to the destination line (in XRGB1555)
+ * @sbuf: Pointer to the source line (in XRGB8888)
+ * @pixels: Number of pixels to convert.
+ */
+void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
@@ -412,6 +448,7 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
 		dbuf16[x] = cpu_to_le16(val16);
 	}
 }
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555_line);
 
 /**
  * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
@@ -447,7 +484,13 @@ void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
 
-static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
+/**
+ * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to ARGB1555
+ * @dbuf: Pointer to the destination line (in ARGB1555)
+ * @sbuf: Pointer to the source line (in XRGB8888)
+ * @pixels: Number of pixels to convert.
+ */
+void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
@@ -464,6 +507,7 @@ static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
 		dbuf16[x] = cpu_to_le16(val16);
 	}
 }
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555_line);
 
 /**
  * drm_fb_xrgb8888_to_argb1555 - Convert XRGB8888 to ARGB1555 clip buffer
@@ -499,7 +543,13 @@ void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
 
-static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
+/**
+ * drm_fb_xrgb8888_to_rgba5551_line - Convert one line from XRGB8888 to ARGB5551
+ * @dbuf: Pointer to the destination line (in ARGB5551)
+ * @sbuf: Pointer to the source line (in XRGB8888)
+ * @pixels: Number of pixels to convert.
+ */
+void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
@@ -516,6 +566,7 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
 		dbuf16[x] = cpu_to_le16(val16);
 	}
 }
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551_line);
 
 /**
  * drm_fb_xrgb8888_to_rgba5551 - Convert XRGB8888 to RGBA5551 clip buffer
@@ -551,7 +602,13 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
 
-static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
+/**
+ * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to RGB888
+ * @dbuf: Pointer to the destination line (in RGB888)
+ * @sbuf: Pointer to the source line (in XRGB8888)
+ * @pixels: Number of pixels to convert.
+ */
+void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	u8 *dbuf8 = dbuf;
 	const __le32 *sbuf32 = sbuf;
@@ -566,6 +623,7 @@ static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigne
 		*dbuf8++ = (pix & 0x00FF0000) >> 16;
 	}
 }
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_line);
 
 /**
  * drm_fb_xrgb8888_to_rgb888 - Convert XRGB8888 to RGB888 clip buffer
@@ -709,7 +767,13 @@ static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned in
 		    drm_fb_xrgb8888_to_xbgr8888_line);
 }
 
-static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
+/**
+ * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to XRGB2101010
+ * @dbuf: Pointer to the destination line (in XRGB2101010)
+ * @sbuf: Pointer to the source line (in XRGB8888)
+ * @pixels: Number of pixels to convert.
+ */
+void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le32 *dbuf32 = dbuf;
 	const __le32 *sbuf32 = sbuf;
@@ -726,6 +790,7 @@ static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
 		*dbuf32++ = cpu_to_le32(pix);
 	}
 }
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_line);
 
 /**
  * drm_fb_xrgb8888_to_xrgb2101010 - Convert XRGB8888 to XRGB2101010 clip buffer
@@ -761,7 +826,13 @@ void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *d
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010);
 
-static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
+/**
+ * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to ARGB2101010
+ * @dbuf: Pointer to the destination line (in ARGB2101010)
+ * @sbuf: Pointer to the source line (in XRGB8888)
+ * @pixels: Number of pixels to convert.
+ */
+void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le32 *dbuf32 = dbuf;
 	const __le32 *sbuf32 = sbuf;
@@ -779,6 +850,7 @@ static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
 		*dbuf32++ = cpu_to_le32(pix);
 	}
 }
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb2101010_line);
 
 /**
  * drm_fb_xrgb8888_to_argb2101010 - Convert XRGB8888 to ARGB2101010 clip buffer
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 291deb09475b..31ab699128d5 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -24,30 +24,39 @@ void drm_fb_memcpy(struct iosys_map *dst, const unsigned int *dst_pitch,
 void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
 		 const struct iosys_map *src, const struct drm_framebuffer *fb,
 		 const struct drm_rect *clip, bool cached);
+void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned int pixels,
+			     u32 fg_color, u32 bg_color);
 void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
 			       const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels);
 void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
 			       const struct drm_rect *clip, bool swab);
+void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels);
 void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
 				 const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels);
 void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
 				 const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels);
 void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
 				 const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels);
 void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
 			       const struct drm_rect *clip);
 void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
 				 const struct iosys_map *src, const struct drm_framebuffer *fb,
 				 const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels);
 void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
 				    const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels);
 void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
 				    const struct drm_rect *clip);
-- 
2.41.0

[PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

This module displays a user friendly message when a kernel panic
occurs. It currently doesn't contain any debug information,
but that can be added later.

v2
 * Use get_scanout_buffer() instead of the drm client API.
  (Thomas Zimmermann)
 * Add the panic reason to the panic message (Nerdopolis)
 * Add an exclamation mark (Nerdopolis)

v3
 * Rework the drawing functions, to write the pixels line by line and
 to use the drm conversion helper to support other formats.
 (Thomas Zimmermann)

v4
 * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann)
 * Remove the default y to DRM_PANIC config option (Thomas Zimmermann)
 * Add foreground/background color config option
 * Fix the bottom lines not painted if the framebuffer height
   is not a multiple of the font height.
 * Automatically register the device to drm_panic, if the function
   get_scanout_buffer exists. (Thomas Zimmermann)

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/Kconfig     |  22 ++
 drivers/gpu/drm/Makefile    |   1 +
 drivers/gpu/drm/drm_drv.c   |   8 +
 drivers/gpu/drm/drm_panic.c | 413 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_drv.h       |  14 ++
 include/drm/drm_panic.h     |  41 ++++
 6 files changed, 499 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_panic.c
 create mode 100644 include/drm/drm_panic.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index afb3b2f5f425..17986f630a86 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -99,6 +99,28 @@ config DRM_KMS_HELPER
 	help
 	  CRTC helpers for KMS drivers.
 
+config DRM_PANIC
+	bool "Display a user-friendly message when a kernel panic occurs"
+	depends on DRM && !FRAMEBUFFER_CONSOLE
+	select FONT_SUPPORT
+	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
+	  console instead of fbcon.
+	  It will only work if your graphic driver supports this feature.
+	  To support Hi-DPI Display, you can enable bigger fonts like
+	  FONT_TER16x32
+
+config DRM_PANIC_FOREGROUND_COLOR
+	hex "Drm panic screen foreground color, in RGB"
+	depends on DRM_PANIC
+	default 0xffffff
+
+config DRM_PANIC_BACKGROUND_COLOR
+	hex "Drm panic screen background color, in RGB"
+	depends on DRM_PANIC
+	default 0x000000
+
 config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
         bool "Enable refcount backtrace history in the DP MST helpers"
 	depends on STACKTRACE_SUPPORT
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 7a09a89b493b..a525dd9a2751 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -72,6 +72,7 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \
 	drm_privacy_screen_x86.o
 drm-$(CONFIG_DRM_ACCEL) += ../../accel/drm_accel.o
 obj-$(CONFIG_DRM)	+= drm.o
+drm-$(CONFIG_DRM_PANIC) += drm_panic.o
 
 obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 12687dd9e1ac..41e211aadb15 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -43,6 +43,7 @@
 #include <drm/drm_file.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_mode_object.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_print.h>
 #include <drm/drm_privacy_screen_machine.h>
 
@@ -943,6 +944,9 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		drm_modeset_register_all(dev);
 
+	if (driver->get_scanout_buffer)
+		drm_panic_register(dev);
+
 	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
 		 driver->name, driver->major, driver->minor,
 		 driver->patchlevel, driver->date,
@@ -986,6 +990,8 @@ void drm_dev_unregister(struct drm_device *dev)
 
 	dev->registered = false;
 
+	drm_panic_unregister(dev);
+
 	drm_client_dev_unregister(dev);
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
@@ -1067,6 +1073,7 @@ static void drm_core_exit(void)
 	unregister_chrdev(DRM_MAJOR, "drm");
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
+	drm_panic_exit();
 	idr_destroy(&drm_minors_idr);
 	drm_connector_ida_destroy();
 }
@@ -1078,6 +1085,7 @@ static int __init drm_core_init(void)
 	drm_connector_ida_init();
 	idr_init(&drm_minors_idr);
 	drm_memcpy_init_early();
+	drm_panic_init();
 
 	ret = drm_sysfs_init();
 	if (ret < 0) {
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
new file mode 100644
index 000000000000..1b7ba7ebe725
--- /dev/null
+++ b/drivers/gpu/drm/drm_panic.c
@@ -0,0 +1,413 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+/*
+ * Copyright (c) 2023 Jocelyn Falempe <jfalempe@redhat.com>
+ * inspired by the drm_log driver from David Herrmann <dh.herrmann@gmail.com>
+ * Tux Ascii art taken from cowsay written by Tony Monroe
+ */
+
+#include <linux/font.h>
+#include <linux/iosys-map.h>
+#include <linux/kdebug.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/panic_notifier.h>
+#include <linux/types.h>
+
+#include <drm/drm_drv.h>
+#include <drm/drm_format_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_panic.h>
+#include <drm/drm_print.h>
+
+
+MODULE_AUTHOR("Jocelyn Falempe");
+MODULE_DESCRIPTION("DRM panic handler");
+MODULE_LICENSE("GPL");
+
+/**
+ * DOC: DRM Panic
+ *
+ * This module displays a user friendly message on screen when a kernel panic
+ * occurs. This is conflicting with fbcon, so you can only enable it when fbcon
+ * is disabled.
+ * It's intended for end-user, so have minimal technical/debug information.
+ */
+
+/*
+ * Implementation details:
+ *
+ * It is a panic handler, so it can't take lock, allocate memory, run tasks/irq,
+ * or attempt to sleep. It's a best effort, and it may not be able to display
+ * the message in all situations (like if the panic occurs in the middle of a
+ * modesetting).
+ * It will display only one static frame, so performance optimizations are low
+ * priority as the machine is already in an unusable state.
+ * The frame is drawn line by line, to allow easier conversion to the fb format.
+ */
+
+/*
+ * List of active drm devices that can render a panic
+ */
+struct drm_panic_device {
+	struct list_head head;
+	struct drm_device *dev;
+};
+
+struct drm_panic_line {
+	u32 len;
+	const char *txt;
+};
+
+#define PANIC_LINE(s) {.len = sizeof(s) - 1, .txt = s}
+
+struct drm_panic_line panic_msg[] = {
+	PANIC_LINE("KERNEL PANIC !"),
+	PANIC_LINE(""),
+	PANIC_LINE("Please reboot your computer."),
+	PANIC_LINE(""),
+	PANIC_LINE(""), /* overwritten with panic reason */
+};
+
+const struct drm_panic_line logo[] = {
+	PANIC_LINE("     .--.        _"),
+	PANIC_LINE("    |o_o |      | |"),
+	PANIC_LINE("    |:_/ |      | |"),
+	PANIC_LINE("   //   \\ \\     |_|"),
+	PANIC_LINE("  (|     | )     _"),
+	PANIC_LINE(" /'\\_   _/`\\    (_)"),
+	PANIC_LINE(" \\___)=(___/"),
+};
+
+static LIST_HEAD(drm_panic_devices);
+static DEFINE_MUTEX(drm_panic_lock);
+
+/*
+ * This buffer is used to convert xrgb8888 to the scanout buffer format.
+ * It is allocated when the first client register, and freed when the last client
+ * unregisters.
+ * There is no need for mutex, as the panic garantee that only 1 CPU is still
+ * running, and preemption is disabled.
+ */
+#define DRM_PANIC_MAX_WIDTH 8096
+static u32 *drm_panic_line_buf;
+
+static u32 fg_color;
+static u32 bg_color;
+
+static void draw_line(const struct drm_panic_line *msg, size_t left_margin,
+		      size_t l, size_t width, const struct font_desc *font)
+{
+	size_t x, i;
+	const u8 *src;
+	size_t src_stride = DIV_ROUND_UP(font->width, 8);
+	u32 *dst = drm_panic_line_buf;
+
+	for (x = 0; x < left_margin * font->width; x++)
+		*dst++ = bg_color;
+
+	for (i = 0; i < msg->len; i++) {
+		src = font->data + (msg->txt[i] * font->height + l) * src_stride;
+		drm_fb_r1_to_32bit_line(dst, src, font->width, fg_color, bg_color);
+		dst += font->width;
+	}
+	for (x = (left_margin + msg->len) * font->width; x < width ; x++)
+		*dst++ = bg_color;
+}
+
+static void draw_txt_line(const struct drm_panic_line *msg, size_t left_margin, size_t y,
+			  struct drm_scanout_buffer *sb,
+			  const struct font_desc *font,
+			  void (*convert)(void *dbuf, const void *sbuf, unsigned int npixels))
+{
+	size_t dst_off;
+	size_t l;
+
+	for (l = 0; l < font->height; l++) {
+		draw_line(msg, left_margin, l, sb->width, font);
+		if (convert)
+			convert(drm_panic_line_buf, drm_panic_line_buf, sb->width);
+
+		dst_off = (y * font->height + l) * sb->pitch;
+		iosys_map_memcpy_to(&sb->map, dst_off, drm_panic_line_buf,
+				    sb->width * sb->format->cpp[0]);
+	}
+}
+
+static void draw_empty_lines(struct drm_scanout_buffer *sb,
+			     size_t start_line,
+			     size_t len,
+			     void (*convert)(void *dbuf, const void *sbuf, unsigned int npixels))
+{
+	size_t i, dst_off;
+	u32 *dst = drm_panic_line_buf;
+
+	for (i = 0; i < sb->width; i++)
+		*dst++ = bg_color;
+
+	if (convert)
+		convert(drm_panic_line_buf, drm_panic_line_buf, sb->width);
+
+	for (i = 0; i < len; i++) {
+		dst_off = (start_line + i) * sb->pitch;
+		iosys_map_memcpy_to(&sb->map, dst_off, drm_panic_line_buf,
+				    sb->width * sb->format->cpp[0]);
+	}
+}
+
+static size_t panic_msg_needed_lines(size_t chars_per_line)
+{
+	size_t msg_len = ARRAY_SIZE(panic_msg);
+	size_t lines = 0;
+	size_t i;
+
+	for (i = 0; i < msg_len; i++)
+		lines += panic_msg[i].len ? DIV_ROUND_UP(panic_msg[i].len, chars_per_line) : 1;
+	return lines;
+}
+
+static bool can_draw_logo(size_t chars_per_line, size_t lines, size_t msg_lines)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(logo); i++) {
+		if (logo[i].len > chars_per_line)
+			return false;
+	}
+	if (lines < msg_lines + ARRAY_SIZE(logo))
+		return false;
+	return true;
+}
+
+static size_t get_start_line(size_t lines, size_t msg_lines, bool draw_logo)
+{
+	size_t remaining;
+	size_t logo_len = ARRAY_SIZE(logo);
+
+	if (lines < msg_lines)
+		return 0;
+	remaining = lines - msg_lines;
+	if (draw_logo && remaining / 2 <= logo_len)
+		return logo_len + (remaining - logo_len) / 4;
+	return remaining / 2;
+}
+
+/*
+ * Return the function to convert xrgb8888 to the scanout buffer format
+ * NULL if no conversion is needed, and ERR_PTR if format can't be converted.
+ * Also set fg_color and bg_color accordingly.
+ */
+static void (*get_convert_func(const struct drm_format_info *format))
+	    (void *, const void *, unsigned int)
+{
+	/* reset colors, this are le32 value as it's what drm_fb_xxxx_to_xxx() expect */
+	fg_color = CONFIG_DRM_PANIC_FOREGROUND_COLOR;
+	bg_color = CONFIG_DRM_PANIC_BACKGROUND_COLOR;
+
+	switch (format->format) {
+	/* 16 bit format */
+	case DRM_FORMAT_RGB565:
+		return drm_fb_xrgb8888_to_rgb565_line;
+	case DRM_FORMAT_RGBA5551:
+		return drm_fb_xrgb8888_to_rgba5551_line;
+	case DRM_FORMAT_XRGB1555:
+		return drm_fb_xrgb8888_to_xrgb1555_line;
+	case DRM_FORMAT_ARGB1555:
+		return drm_fb_xrgb8888_to_argb1555_line;
+	/* 24 bit format */
+	case DRM_FORMAT_RGB888:
+		return drm_fb_xrgb8888_to_rgb888_line;
+	/* 32 bit format: convert the fg_color and bg_color */
+	case DRM_FORMAT_XRGB8888:
+		return NULL;
+	case DRM_FORMAT_ARGB8888:
+		fg_color = fg_color | 0xff000000;
+		bg_color = bg_color | 0xff000000;
+		return NULL;
+	case DRM_FORMAT_XBGR8888:
+		fg_color = swab32(fg_color) >> 8;
+		bg_color = swab32(bg_color) >> 8;
+		return NULL;
+	case DRM_FORMAT_ABGR8888:
+		fg_color = (swab32(fg_color) >> 8) | 0xff000000;
+		bg_color = (swab32(bg_color) >> 8) | 0xff000000;
+		return NULL;
+	case DRM_FORMAT_XRGB2101010:
+		drm_fb_xrgb8888_to_xrgb2101010_line(&fg_color, &fg_color, 1);
+		drm_fb_xrgb8888_to_xrgb2101010_line(&bg_color, &bg_color, 1);
+		return NULL;
+	case DRM_FORMAT_ARGB2101010:
+		drm_fb_xrgb8888_to_argb2101010_line(&fg_color, &fg_color, 1);
+		drm_fb_xrgb8888_to_argb2101010_line(&bg_color, &bg_color, 1);
+		return NULL;
+	default:
+		return ERR_PTR(EINVAL);
+	}
+}
+
+/*
+ * Draw the panic message at the center of the screen
+ */
+static void draw_panic_static(struct drm_scanout_buffer *sb, const char *msg)
+{
+	size_t lines, msg_lines, l, msg_start_line, msgi;
+	size_t center, chars_per_line;
+	bool draw_logo;
+	struct drm_panic_line panic_line;
+	size_t msg_len = ARRAY_SIZE(panic_msg);
+	size_t logo_len = ARRAY_SIZE(logo);
+	void (*convert)(void *dbuf, const void *sbuf, unsigned int npixels);
+	const struct font_desc *font = get_default_font(sb->width, sb->height, 0x8080, 0x8080);
+
+	if (!font)
+		return;
+
+	/* Set the panic reason */
+	panic_msg[msg_len - 1].len = strlen(msg);
+	panic_msg[msg_len - 1].txt = msg;
+
+	lines = sb->height / font->height;
+	chars_per_line = sb->width / font->width;
+
+	msg_lines = panic_msg_needed_lines(chars_per_line);
+	draw_logo = can_draw_logo(chars_per_line, lines, msg_lines);
+	msg_start_line = get_start_line(lines, msg_lines, draw_logo);
+
+	convert = get_convert_func(sb->format);
+	if (IS_ERR(convert))
+		return;
+
+	msgi = 0;
+	panic_line.len = 0;
+	for (l = 0; l < lines; l++) {
+		if (draw_logo && l < logo_len)
+			draw_txt_line(&logo[l], 0, l, sb, font, convert);
+		else if (l >= msg_start_line && msgi < msg_len) {
+			if (!panic_line.len) {
+				panic_line.txt = panic_msg[msgi].txt;
+				panic_line.len = panic_msg[msgi].len;
+			}
+			if (!panic_line.len)
+				draw_empty_lines(sb, l * font->height, font->height, convert);
+			else {
+				center = panic_line.len > chars_per_line ?
+					 0 : (chars_per_line - panic_line.len) / 2;
+				draw_txt_line(&panic_line, center, l, sb, font, convert);
+			}
+			if (panic_line.len > chars_per_line) {
+				panic_line.len -= chars_per_line;
+				panic_line.txt += chars_per_line;
+			} else {
+				panic_line.len = 0;
+				msgi++;
+			}
+		} else {
+			draw_empty_lines(sb, l * font->height, font->height, convert);
+		}
+	}
+	/* Fill the bottom of the screen, if sb->height is not a multiple of font->height */
+	draw_empty_lines(sb, lines * font->height, sb->height - lines * font->height, convert);
+}
+
+static void draw_panic_device(struct drm_device *dev, const char *msg)
+{
+	struct drm_scanout_buffer sb;
+
+	if (dev->driver->get_scanout_buffer(dev, &sb))
+		return;
+	if (!drm_panic_line_buf)
+		return;
+
+	/* to avoid buffer overflow on drm_panic_line_buf */
+	if (sb.width > DRM_PANIC_MAX_WIDTH)
+		sb.width = DRM_PANIC_MAX_WIDTH;
+
+	draw_panic_static(&sb, msg);
+}
+
+static int drm_panic(struct notifier_block *this, unsigned long event,
+		     void *ptr)
+{
+	struct drm_panic_device *drm_panic_device;
+
+	list_for_each_entry(drm_panic_device, &drm_panic_devices, head) {
+		draw_panic_device(drm_panic_device->dev, ptr);
+	}
+	return NOTIFY_OK;
+}
+
+struct notifier_block drm_panic_notifier = {
+	.notifier_call = drm_panic,
+};
+
+/**
+ * drm_panic_register() - Initialize DRM panic for a device
+ * @dev: the DRM device on which the panic screen will be displayed.
+ */
+void drm_panic_register(struct drm_device *dev)
+{
+	struct drm_panic_device *new;
+
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
+	if (!new)
+		return;
+
+	new->dev = dev;
+	mutex_lock(&drm_panic_lock);
+	if (!drm_panic_line_buf)
+		drm_panic_line_buf = kmalloc(DRM_PANIC_MAX_WIDTH * sizeof(u32), GFP_KERNEL);
+	if (!drm_panic_line_buf)
+		goto unlock;
+	list_add_tail(&new->head, &drm_panic_devices);
+	drm_info(dev, "Registered with drm panic\n");
+unlock:
+	mutex_unlock(&drm_panic_lock);
+}
+EXPORT_SYMBOL(drm_panic_register);
+
+/**
+ * drm_panic_unregister()
+ * @dev: the DRM device previously registered.
+ */
+void drm_panic_unregister(struct drm_device *dev)
+{
+	struct drm_panic_device *drm_panic_device;
+	struct drm_panic_device *found = NULL;
+
+	mutex_lock(&drm_panic_lock);
+	list_for_each_entry(drm_panic_device, &drm_panic_devices, head) {
+		if (drm_panic_device->dev == dev)
+			found = drm_panic_device;
+	}
+	if (found) {
+		list_del(&found->head);
+		kfree(found);
+		drm_info(dev, "Unregistered with drm panic\n");
+	}
+	if (drm_panic_line_buf && list_empty(&drm_panic_devices)) {
+		kfree(drm_panic_line_buf);
+		drm_panic_line_buf = NULL;
+	}
+	mutex_unlock(&drm_panic_lock);
+}
+EXPORT_SYMBOL(drm_panic_unregister);
+
+/**
+ * drm_panic_init() - Initialize drm-panic subsystem
+ *
+ * register the panic notifier
+ */
+void drm_panic_init(void)
+{
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &drm_panic_notifier);
+}
+
+/**
+ * drm_panic_exit() - Shutdown drm-panic subsystem
+ */
+void drm_panic_exit(void)
+{
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &drm_panic_notifier);
+}
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 89e2706cac56..e538c87116d3 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -43,6 +43,7 @@ struct dma_buf_attachment;
 struct drm_display_mode;
 struct drm_mode_create_dumb;
 struct drm_printer;
+struct drm_scanout_buffer;
 struct sg_table;
 
 /**
@@ -408,6 +409,19 @@ struct drm_driver {
 	 */
 	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
 
+	/**
+	 * @get_scanout_buffer:
+	 *
+	 * Get the current scanout buffer, to display a panic message with drm_panic.
+	 * It is called from a panic callback, and must follow its restrictions.
+	 *
+	 * Returns:
+	 *
+	 * Zero on success, negative errno on failure.
+	 */
+	int (*get_scanout_buffer)(struct drm_device *dev,
+				  struct drm_scanout_buffer *sb);
+
 	/** @major: driver major number */
 	int major;
 	/** @minor: driver minor number */
diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
new file mode 100644
index 000000000000..db430b8dfbb2
--- /dev/null
+++ b/include/drm/drm_panic.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+#ifndef __DRM_PANIC_H__
+#define __DRM_PANIC_H__
+
+/*
+ * Copyright (c) 2023 Jocelyn Falempe <jfalempe@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/iosys-map.h>
+
+struct drm_device;
+
+struct drm_scanout_buffer {
+	const struct drm_format_info *format;
+	struct iosys_map map;
+	unsigned int pitch;
+	unsigned int width;
+	unsigned int height;
+};
+
+#ifdef CONFIG_DRM_PANIC
+
+void drm_panic_init(void);
+void drm_panic_exit(void);
+
+void drm_panic_register(struct drm_device *dev);
+void drm_panic_unregister(struct drm_device *dev);
+
+#else
+
+static inline void drm_panic_init(void) {}
+static inline void drm_panic_exit(void) {}
+
+static inline void drm_panic_register(struct drm_device *dev) {}
+static inline void drm_panic_unregister(struct drm_device *dev) {}
+
+#endif
+
+#endif /* __DRM_LOG_H__ */
-- 
2.41.0

[PATCH v4 3/4] drm/simpledrm: Add drm_panic support

[Jocelyn Falempe]

Add support for the drm_panic module, which displays a user-friendly
message to the screen when a kernel panic occurs.

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

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 25e11ef11c4c..ee62303ef68a 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -23,6 +23,7 @@
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 
@@ -842,6 +843,19 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	return sdev;
 }
 
+static int simpledrm_get_scanout_buffer(struct drm_device *dev,
+					struct drm_scanout_buffer *sb)
+{
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
+
+	sb->width = sdev->mode.hdisplay;
+	sb->height = sdev->mode.vdisplay;
+	sb->pitch = sdev->pitch;
+	sb->format = sdev->format;
+	sb->map = sdev->screen_base;
+	return 0;
+}
+
 /*
  * DRM driver
  */
@@ -857,6 +871,7 @@ static struct drm_driver simpledrm_driver = {
 	.minor			= DRIVER_MINOR,
 	.driver_features	= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
 	.fops			= &simpledrm_fops,
+	.get_scanout_buffer	= simpledrm_get_scanout_buffer,
 };
 
 /*
-- 
2.41.0

[PATCH v4 4/4] drm/mgag200: Add drm_panic support

[Jocelyn Falempe]

Add support for the drm_panic module, which displays a message to
the screen when a kernel panic occurs.

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 976f0ab2006b..229d9c116b42 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -12,10 +12,12 @@
 #include <drm/drm_aperture.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fbdev_generic.h>
+#include <drm/drm_framebuffer.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_module.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_pciids.h>
 
 #include "mgag200_drv.h"
@@ -83,6 +85,27 @@ resource_size_t mgag200_probe_vram(void __iomem *mem, resource_size_t size)
 	return offset - 65536;
 }
 
+static int mgag200_get_scanout_buffer(struct drm_device *dev,
+				      struct drm_scanout_buffer *sb)
+{
+	struct drm_plane *plane;
+	struct mga_device *mdev = to_mga_device(dev);
+	struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
+
+	/* mgag200 has only one plane */
+	drm_for_each_plane(plane, dev) {
+		if (!plane->state || !plane->state->fb)
+			return -ENODEV;
+		sb->format = plane->state->fb->format;
+		sb->width = plane->state->fb->width;
+		sb->height = plane->state->fb->height;
+		sb->pitch = plane->state->fb->pitches[0];
+		sb->map = map;
+		return 0;
+	}
+	return -ENODEV;
+}
+
 /*
  * DRM driver
  */
@@ -98,6 +121,7 @@ static const struct drm_driver mgag200_driver = {
 	.major = DRIVER_MAJOR,
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
+	.get_scanout_buffer = mgag200_get_scanout_buffer,
 	DRM_GEM_SHMEM_DRIVER_OPS,
 };
 
-- 
2.41.0

Re: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2dde18cd1d8fac735875f2e4987f11817cc0bc2c]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-format-helper-Export-line-conversion-helper-for-drm_panic/20231003-222642
base:   2dde18cd1d8fac735875f2e4987f11817cc0bc2c
patch link:    https://lore.kernel.org/r/20231003142508.190246-2-jfalempe%40redhat.com
patch subject: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231003/202310032302.DqsgLDE3-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231003/202310032302.DqsgLDE3-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_format_helper.c:436: warning: expecting prototype for drm_fb_xrgb8888_to_rgb565_line(). Prototype was for drm_fb_xrgb8888_to_xrgb1555_line() instead
>> drivers/gpu/drm/drm_format_helper.c:494: warning: expecting prototype for drm_fb_xrgb8888_to_rgb565_line(). Prototype was for drm_fb_xrgb8888_to_argb1555_line() instead
>> drivers/gpu/drm/drm_format_helper.c:777: warning: expecting prototype for drm_fb_xrgb8888_to_rgb888_line(). Prototype was for drm_fb_xrgb8888_to_xrgb2101010_line() instead
>> drivers/gpu/drm/drm_format_helper.c:836: warning: expecting prototype for drm_fb_xrgb8888_to_rgb888_line(). Prototype was for drm_fb_xrgb8888_to_argb2101010_line() instead


vim +436 drivers/gpu/drm/drm_format_helper.c

7415287e1f3675 Gerd Hoffmann     2019-04-05  428  
ce913131bdeb03 Jocelyn Falempe   2023-10-03  429  /**
ce913131bdeb03 Jocelyn Falempe   2023-10-03  430   * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to XRGB1555
ce913131bdeb03 Jocelyn Falempe   2023-10-03  431   * @dbuf: Pointer to the destination line (in XRGB1555)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  432   * @sbuf: Pointer to the source line (in XRGB8888)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  433   * @pixels: Number of pixels to convert.
ce913131bdeb03 Jocelyn Falempe   2023-10-03  434   */
ce913131bdeb03 Jocelyn Falempe   2023-10-03  435  void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
10cd592e639edc Thomas Zimmermann 2023-01-02 @436  {
10cd592e639edc Thomas Zimmermann 2023-01-02  437  	__le16 *dbuf16 = dbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  438  	const __le32 *sbuf32 = sbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  439  	unsigned int x;
10cd592e639edc Thomas Zimmermann 2023-01-02  440  	u16 val16;
10cd592e639edc Thomas Zimmermann 2023-01-02  441  	u32 pix;
10cd592e639edc Thomas Zimmermann 2023-01-02  442  
10cd592e639edc Thomas Zimmermann 2023-01-02  443  	for (x = 0; x < pixels; x++) {
10cd592e639edc Thomas Zimmermann 2023-01-02  444  		pix = le32_to_cpu(sbuf32[x]);
10cd592e639edc Thomas Zimmermann 2023-01-02  445  		val16 = ((pix & 0x00f80000) >> 9) |
10cd592e639edc Thomas Zimmermann 2023-01-02  446  			((pix & 0x0000f800) >> 6) |
10cd592e639edc Thomas Zimmermann 2023-01-02  447  			((pix & 0x000000f8) >> 3);
10cd592e639edc Thomas Zimmermann 2023-01-02  448  		dbuf16[x] = cpu_to_le16(val16);
10cd592e639edc Thomas Zimmermann 2023-01-02  449  	}
10cd592e639edc Thomas Zimmermann 2023-01-02  450  }
ce913131bdeb03 Jocelyn Falempe   2023-10-03  451  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555_line);
10cd592e639edc Thomas Zimmermann 2023-01-02  452  
10cd592e639edc Thomas Zimmermann 2023-01-02  453  /**
10cd592e639edc Thomas Zimmermann 2023-01-02  454   * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
10cd592e639edc Thomas Zimmermann 2023-01-02  455   * @dst: Array of XRGB1555 destination buffers
10cd592e639edc Thomas Zimmermann 2023-01-02  456   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
10cd592e639edc Thomas Zimmermann 2023-01-02  457   *             within @dst; can be NULL if scanlines are stored next to each other.
10cd592e639edc Thomas Zimmermann 2023-01-02  458   * @src: Array of XRGB8888 source buffer
10cd592e639edc Thomas Zimmermann 2023-01-02  459   * @fb: DRM framebuffer
10cd592e639edc Thomas Zimmermann 2023-01-02  460   * @clip: Clip rectangle area to copy
10cd592e639edc Thomas Zimmermann 2023-01-02  461   *
10cd592e639edc Thomas Zimmermann 2023-01-02  462   * This function copies parts of a framebuffer to display memory and converts
10cd592e639edc Thomas Zimmermann 2023-01-02  463   * the color format during the process. The parameters @dst, @dst_pitch and
10cd592e639edc Thomas Zimmermann 2023-01-02  464   * @src refer to arrays. Each array must have at least as many entries as
10cd592e639edc Thomas Zimmermann 2023-01-02  465   * there are planes in @fb's format. Each entry stores the value for the
10cd592e639edc Thomas Zimmermann 2023-01-02  466   * format's respective color plane at the same index.
10cd592e639edc Thomas Zimmermann 2023-01-02  467   *
10cd592e639edc Thomas Zimmermann 2023-01-02  468   * This function does not apply clipping on @dst (i.e. the destination is at the
10cd592e639edc Thomas Zimmermann 2023-01-02  469   * top-left corner).
10cd592e639edc Thomas Zimmermann 2023-01-02  470   *
10cd592e639edc Thomas Zimmermann 2023-01-02  471   * Drivers can use this function for XRGB1555 devices that don't support
10cd592e639edc Thomas Zimmermann 2023-01-02  472   * XRGB8888 natively.
10cd592e639edc Thomas Zimmermann 2023-01-02  473   */
10cd592e639edc Thomas Zimmermann 2023-01-02  474  void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
10cd592e639edc Thomas Zimmermann 2023-01-02  475  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
10cd592e639edc Thomas Zimmermann 2023-01-02  476  				 const struct drm_rect *clip)
10cd592e639edc Thomas Zimmermann 2023-01-02  477  {
10cd592e639edc Thomas Zimmermann 2023-01-02  478  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
10cd592e639edc Thomas Zimmermann 2023-01-02  479  		2,
10cd592e639edc Thomas Zimmermann 2023-01-02  480  	};
10cd592e639edc Thomas Zimmermann 2023-01-02  481  
10cd592e639edc Thomas Zimmermann 2023-01-02  482  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
10cd592e639edc Thomas Zimmermann 2023-01-02  483  		    drm_fb_xrgb8888_to_xrgb1555_line);
10cd592e639edc Thomas Zimmermann 2023-01-02  484  }
10cd592e639edc Thomas Zimmermann 2023-01-02  485  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
10cd592e639edc Thomas Zimmermann 2023-01-02  486  
ce913131bdeb03 Jocelyn Falempe   2023-10-03  487  /**
ce913131bdeb03 Jocelyn Falempe   2023-10-03  488   * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to ARGB1555
ce913131bdeb03 Jocelyn Falempe   2023-10-03  489   * @dbuf: Pointer to the destination line (in ARGB1555)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  490   * @sbuf: Pointer to the source line (in XRGB8888)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  491   * @pixels: Number of pixels to convert.
ce913131bdeb03 Jocelyn Falempe   2023-10-03  492   */
ce913131bdeb03 Jocelyn Falempe   2023-10-03  493  void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
10cd592e639edc Thomas Zimmermann 2023-01-02 @494  {
10cd592e639edc Thomas Zimmermann 2023-01-02  495  	__le16 *dbuf16 = dbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  496  	const __le32 *sbuf32 = sbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  497  	unsigned int x;
10cd592e639edc Thomas Zimmermann 2023-01-02  498  	u16 val16;
10cd592e639edc Thomas Zimmermann 2023-01-02  499  	u32 pix;
10cd592e639edc Thomas Zimmermann 2023-01-02  500  
10cd592e639edc Thomas Zimmermann 2023-01-02  501  	for (x = 0; x < pixels; x++) {
10cd592e639edc Thomas Zimmermann 2023-01-02  502  		pix = le32_to_cpu(sbuf32[x]);
10cd592e639edc Thomas Zimmermann 2023-01-02  503  		val16 = BIT(15) | /* set alpha bit */
10cd592e639edc Thomas Zimmermann 2023-01-02  504  			((pix & 0x00f80000) >> 9) |
10cd592e639edc Thomas Zimmermann 2023-01-02  505  			((pix & 0x0000f800) >> 6) |
10cd592e639edc Thomas Zimmermann 2023-01-02  506  			((pix & 0x000000f8) >> 3);
10cd592e639edc Thomas Zimmermann 2023-01-02  507  		dbuf16[x] = cpu_to_le16(val16);
10cd592e639edc Thomas Zimmermann 2023-01-02  508  	}
10cd592e639edc Thomas Zimmermann 2023-01-02  509  }
ce913131bdeb03 Jocelyn Falempe   2023-10-03  510  EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555_line);
10cd592e639edc Thomas Zimmermann 2023-01-02  511  

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

Re: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2dde18cd1d8fac735875f2e4987f11817cc0bc2c]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-format-helper-Export-line-conversion-helper-for-drm_panic/20231003-222642
base:   2dde18cd1d8fac735875f2e4987f11817cc0bc2c
patch link:    https://lore.kernel.org/r/20231003142508.190246-2-jfalempe%40redhat.com
patch subject: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231003/202310032302.DqsgLDE3-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231003/202310032302.DqsgLDE3-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/drm_format_helper.c:436: warning: expecting prototype for drm_fb_xrgb8888_to_rgb565_line(). Prototype was for drm_fb_xrgb8888_to_xrgb1555_line() instead
>> drivers/gpu/drm/drm_format_helper.c:494: warning: expecting prototype for drm_fb_xrgb8888_to_rgb565_line(). Prototype was for drm_fb_xrgb8888_to_argb1555_line() instead
>> drivers/gpu/drm/drm_format_helper.c:777: warning: expecting prototype for drm_fb_xrgb8888_to_rgb888_line(). Prototype was for drm_fb_xrgb8888_to_xrgb2101010_line() instead
>> drivers/gpu/drm/drm_format_helper.c:836: warning: expecting prototype for drm_fb_xrgb8888_to_rgb888_line(). Prototype was for drm_fb_xrgb8888_to_argb2101010_line() instead


vim +436 drivers/gpu/drm/drm_format_helper.c

7415287e1f3675 Gerd Hoffmann     2019-04-05  428  
ce913131bdeb03 Jocelyn Falempe   2023-10-03  429  /**
ce913131bdeb03 Jocelyn Falempe   2023-10-03  430   * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to XRGB1555
ce913131bdeb03 Jocelyn Falempe   2023-10-03  431   * @dbuf: Pointer to the destination line (in XRGB1555)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  432   * @sbuf: Pointer to the source line (in XRGB8888)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  433   * @pixels: Number of pixels to convert.
ce913131bdeb03 Jocelyn Falempe   2023-10-03  434   */
ce913131bdeb03 Jocelyn Falempe   2023-10-03  435  void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
10cd592e639edc Thomas Zimmermann 2023-01-02 @436  {
10cd592e639edc Thomas Zimmermann 2023-01-02  437  	__le16 *dbuf16 = dbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  438  	const __le32 *sbuf32 = sbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  439  	unsigned int x;
10cd592e639edc Thomas Zimmermann 2023-01-02  440  	u16 val16;
10cd592e639edc Thomas Zimmermann 2023-01-02  441  	u32 pix;
10cd592e639edc Thomas Zimmermann 2023-01-02  442  
10cd592e639edc Thomas Zimmermann 2023-01-02  443  	for (x = 0; x < pixels; x++) {
10cd592e639edc Thomas Zimmermann 2023-01-02  444  		pix = le32_to_cpu(sbuf32[x]);
10cd592e639edc Thomas Zimmermann 2023-01-02  445  		val16 = ((pix & 0x00f80000) >> 9) |
10cd592e639edc Thomas Zimmermann 2023-01-02  446  			((pix & 0x0000f800) >> 6) |
10cd592e639edc Thomas Zimmermann 2023-01-02  447  			((pix & 0x000000f8) >> 3);
10cd592e639edc Thomas Zimmermann 2023-01-02  448  		dbuf16[x] = cpu_to_le16(val16);
10cd592e639edc Thomas Zimmermann 2023-01-02  449  	}
10cd592e639edc Thomas Zimmermann 2023-01-02  450  }
ce913131bdeb03 Jocelyn Falempe   2023-10-03  451  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555_line);
10cd592e639edc Thomas Zimmermann 2023-01-02  452  
10cd592e639edc Thomas Zimmermann 2023-01-02  453  /**
10cd592e639edc Thomas Zimmermann 2023-01-02  454   * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
10cd592e639edc Thomas Zimmermann 2023-01-02  455   * @dst: Array of XRGB1555 destination buffers
10cd592e639edc Thomas Zimmermann 2023-01-02  456   * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
10cd592e639edc Thomas Zimmermann 2023-01-02  457   *             within @dst; can be NULL if scanlines are stored next to each other.
10cd592e639edc Thomas Zimmermann 2023-01-02  458   * @src: Array of XRGB8888 source buffer
10cd592e639edc Thomas Zimmermann 2023-01-02  459   * @fb: DRM framebuffer
10cd592e639edc Thomas Zimmermann 2023-01-02  460   * @clip: Clip rectangle area to copy
10cd592e639edc Thomas Zimmermann 2023-01-02  461   *
10cd592e639edc Thomas Zimmermann 2023-01-02  462   * This function copies parts of a framebuffer to display memory and converts
10cd592e639edc Thomas Zimmermann 2023-01-02  463   * the color format during the process. The parameters @dst, @dst_pitch and
10cd592e639edc Thomas Zimmermann 2023-01-02  464   * @src refer to arrays. Each array must have at least as many entries as
10cd592e639edc Thomas Zimmermann 2023-01-02  465   * there are planes in @fb's format. Each entry stores the value for the
10cd592e639edc Thomas Zimmermann 2023-01-02  466   * format's respective color plane at the same index.
10cd592e639edc Thomas Zimmermann 2023-01-02  467   *
10cd592e639edc Thomas Zimmermann 2023-01-02  468   * This function does not apply clipping on @dst (i.e. the destination is at the
10cd592e639edc Thomas Zimmermann 2023-01-02  469   * top-left corner).
10cd592e639edc Thomas Zimmermann 2023-01-02  470   *
10cd592e639edc Thomas Zimmermann 2023-01-02  471   * Drivers can use this function for XRGB1555 devices that don't support
10cd592e639edc Thomas Zimmermann 2023-01-02  472   * XRGB8888 natively.
10cd592e639edc Thomas Zimmermann 2023-01-02  473   */
10cd592e639edc Thomas Zimmermann 2023-01-02  474  void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
10cd592e639edc Thomas Zimmermann 2023-01-02  475  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
10cd592e639edc Thomas Zimmermann 2023-01-02  476  				 const struct drm_rect *clip)
10cd592e639edc Thomas Zimmermann 2023-01-02  477  {
10cd592e639edc Thomas Zimmermann 2023-01-02  478  	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
10cd592e639edc Thomas Zimmermann 2023-01-02  479  		2,
10cd592e639edc Thomas Zimmermann 2023-01-02  480  	};
10cd592e639edc Thomas Zimmermann 2023-01-02  481  
10cd592e639edc Thomas Zimmermann 2023-01-02  482  	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
10cd592e639edc Thomas Zimmermann 2023-01-02  483  		    drm_fb_xrgb8888_to_xrgb1555_line);
10cd592e639edc Thomas Zimmermann 2023-01-02  484  }
10cd592e639edc Thomas Zimmermann 2023-01-02  485  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
10cd592e639edc Thomas Zimmermann 2023-01-02  486  
ce913131bdeb03 Jocelyn Falempe   2023-10-03  487  /**
ce913131bdeb03 Jocelyn Falempe   2023-10-03  488   * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to ARGB1555
ce913131bdeb03 Jocelyn Falempe   2023-10-03  489   * @dbuf: Pointer to the destination line (in ARGB1555)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  490   * @sbuf: Pointer to the source line (in XRGB8888)
ce913131bdeb03 Jocelyn Falempe   2023-10-03  491   * @pixels: Number of pixels to convert.
ce913131bdeb03 Jocelyn Falempe   2023-10-03  492   */
ce913131bdeb03 Jocelyn Falempe   2023-10-03  493  void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
10cd592e639edc Thomas Zimmermann 2023-01-02 @494  {
10cd592e639edc Thomas Zimmermann 2023-01-02  495  	__le16 *dbuf16 = dbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  496  	const __le32 *sbuf32 = sbuf;
10cd592e639edc Thomas Zimmermann 2023-01-02  497  	unsigned int x;
10cd592e639edc Thomas Zimmermann 2023-01-02  498  	u16 val16;
10cd592e639edc Thomas Zimmermann 2023-01-02  499  	u32 pix;
10cd592e639edc Thomas Zimmermann 2023-01-02  500  
10cd592e639edc Thomas Zimmermann 2023-01-02  501  	for (x = 0; x < pixels; x++) {
10cd592e639edc Thomas Zimmermann 2023-01-02  502  		pix = le32_to_cpu(sbuf32[x]);
10cd592e639edc Thomas Zimmermann 2023-01-02  503  		val16 = BIT(15) | /* set alpha bit */
10cd592e639edc Thomas Zimmermann 2023-01-02  504  			((pix & 0x00f80000) >> 9) |
10cd592e639edc Thomas Zimmermann 2023-01-02  505  			((pix & 0x0000f800) >> 6) |
10cd592e639edc Thomas Zimmermann 2023-01-02  506  			((pix & 0x000000f8) >> 3);
10cd592e639edc Thomas Zimmermann 2023-01-02  507  		dbuf16[x] = cpu_to_le16(val16);
10cd592e639edc Thomas Zimmermann 2023-01-02  508  	}
10cd592e639edc Thomas Zimmermann 2023-01-02  509  }
ce913131bdeb03 Jocelyn Falempe   2023-10-03  510  EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555_line);
10cd592e639edc Thomas Zimmermann 2023-01-02  511  

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

Re: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[nerdopolis]

On Tuesday, October 3, 2023 10:22:44 AM EDT Jocelyn Falempe wrote:
> drm_panic will need the low-level drm_fb_xxxx_line functions.
> Also add drm_fb_r1_to_xrgb8888 to render the fonts.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/drm_format_helper.c | 88 ++++++++++++++++++++++++++---
>  include/drm/drm_format_helper.h     |  9 +++
>  2 files changed, 89 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index f93a4efcee90..c238e5d84f1f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -270,7 +270,30 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>  
>  	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, swab_line);
>  }
> -EXPORT_SYMBOL(drm_fb_swab);
I had to add this line back to get it to build, but once I did, it worked. 
> +
> +/**
> + * drm_fb_r1_to_32bit_line - Convert one line from monochrome to any 32bit pixel format
> + * @dbuf: Pointer to the destination line (in any 32bit format)
> + * @sbuf: Pointer to the source line (in monochrome)
> + * @pixels: Number of pixels to convert.
> + * @fg_color: Foreground color, applied when R1 is 1
> + * @bg_color: Background color, applied when R1 is 0
> + *
> + * Convert monochrome to any format with 32bit pixel.
> + * There is a limitation, as sbuf is a pointer, it can only points to a multiple
> + * of 8 pixels in the source buffer.
> + */
> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned int pixels,
> +				u32 fg_color, u32 bg_color)
> +{
> +	unsigned int x;
> +	const u8 *sbuf8 = sbuf;
> +	u32 *dubf32 = dbuf;
> +
> +	for (x = 0; x < pixels; x++)
> +		dubf32[x] = (sbuf8[x / 8] & (0x80 >> (x % 8))) ? fg_color : bg_color;
> +}
> +EXPORT_SYMBOL(drm_fb_r1_to_32bit_line);
>  
>  static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
> @@ -320,7 +343,13 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pi
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
>  
> -static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to RGB565
> + * @dbuf: Pointer to the destination line (in RGB565)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
>  	__le16 *dbuf16 = dbuf;
>  	const __le32 *sbuf32 = sbuf;
> @@ -336,6 +365,7 @@ static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigne
>  		dbuf16[x] = cpu_to_le16(val16);
>  	}
>  }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565_line);
>  
>  /* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
>  static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
> @@ -396,7 +426,13 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
>  
> -static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to XRGB1555
> + * @dbuf: Pointer to the destination line (in XRGB1555)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
>  	__le16 *dbuf16 = dbuf;
>  	const __le32 *sbuf32 = sbuf;
> @@ -412,6 +448,7 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
>  		dbuf16[x] = cpu_to_le16(val16);
>  	}
>  }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555_line);
>  
>  /**
>   * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
> @@ -447,7 +484,13 @@ void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
>  
> -static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to ARGB1555
> + * @dbuf: Pointer to the destination line (in ARGB1555)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
>  	__le16 *dbuf16 = dbuf;
>  	const __le32 *sbuf32 = sbuf;
> @@ -464,6 +507,7 @@ static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
>  		dbuf16[x] = cpu_to_le16(val16);
>  	}
>  }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555_line);
>  
>  /**
>   * drm_fb_xrgb8888_to_argb1555 - Convert XRGB8888 to ARGB1555 clip buffer
> @@ -499,7 +543,13 @@ void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
>  
> -static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgba5551_line - Convert one line from XRGB8888 to ARGB5551
> + * @dbuf: Pointer to the destination line (in ARGB5551)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
>  	__le16 *dbuf16 = dbuf;
>  	const __le32 *sbuf32 = sbuf;
> @@ -516,6 +566,7 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
>  		dbuf16[x] = cpu_to_le16(val16);
>  	}
>  }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551_line);
>  
>  /**
>   * drm_fb_xrgb8888_to_rgba5551 - Convert XRGB8888 to RGBA5551 clip buffer
> @@ -551,7 +602,13 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
>  
> -static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to RGB888
> + * @dbuf: Pointer to the destination line (in RGB888)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
>  	u8 *dbuf8 = dbuf;
>  	const __le32 *sbuf32 = sbuf;
> @@ -566,6 +623,7 @@ static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigne
>  		*dbuf8++ = (pix & 0x00FF0000) >> 16;
>  	}
>  }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_line);
>  
>  /**
>   * drm_fb_xrgb8888_to_rgb888 - Convert XRGB8888 to RGB888 clip buffer
> @@ -709,7 +767,13 @@ static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned in
>  		    drm_fb_xrgb8888_to_xbgr8888_line);
>  }
>  
> -static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to XRGB2101010
> + * @dbuf: Pointer to the destination line (in XRGB2101010)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
>  	__le32 *dbuf32 = dbuf;
>  	const __le32 *sbuf32 = sbuf;
> @@ -726,6 +790,7 @@ static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
>  		*dbuf32++ = cpu_to_le32(pix);
>  	}
>  }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_line);
>  
>  /**
>   * drm_fb_xrgb8888_to_xrgb2101010 - Convert XRGB8888 to XRGB2101010 clip buffer
> @@ -761,7 +826,13 @@ void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *d
>  }
>  EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010);
>  
> -static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to ARGB2101010
> + * @dbuf: Pointer to the destination line (in ARGB2101010)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
>  {
>  	__le32 *dbuf32 = dbuf;
>  	const __le32 *sbuf32 = sbuf;
> @@ -779,6 +850,7 @@ static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
>  		*dbuf32++ = cpu_to_le32(pix);
>  	}
>  }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb2101010_line);
>  
>  /**
>   * drm_fb_xrgb8888_to_argb2101010 - Convert XRGB8888 to ARGB2101010 clip buffer
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index 291deb09475b..31ab699128d5 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -24,30 +24,39 @@ void drm_fb_memcpy(struct iosys_map *dst, const unsigned int *dst_pitch,
>  void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>  		 const struct iosys_map *src, const struct drm_framebuffer *fb,
>  		 const struct drm_rect *clip, bool cached);
> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned int pixels,
> +			     u32 fg_color, u32 bg_color);
>  void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pitch,
>  			       const struct iosys_map *src, const struct drm_framebuffer *fb,
>  			       const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels);
>  void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
>  			       const struct iosys_map *src, const struct drm_framebuffer *fb,
>  			       const struct drm_rect *clip, bool swab);
> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels);
>  void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
>  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>  				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels);
>  void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
>  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>  				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels);
>  void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
>  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>  				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels);
>  void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
>  			       const struct iosys_map *src, const struct drm_framebuffer *fb,
>  			       const struct drm_rect *clip);
>  void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
>  				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>  				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels);
>  void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
>  				    const struct iosys_map *src, const struct drm_framebuffer *fb,
>  				    const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels);
>  void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
>  				    const struct iosys_map *src, const struct drm_framebuffer *fb,
>  				    const struct drm_rect *clip);
> 

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2dde18cd1d8fac735875f2e4987f11817cc0bc2c]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-format-helper-Export-line-conversion-helper-for-drm_panic/20231003-222642
base:   2dde18cd1d8fac735875f2e4987f11817cc0bc2c
patch link:    https://lore.kernel.org/r/20231003142508.190246-3-jfalempe%40redhat.com
patch subject: [PATCH v4 2/4] drm/panic: Add a drm panic handler
config: i386-randconfig-063-20231005 (https://download.01.org/0day-ci/archive/20231005/202310051128.nptC5NyW-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231005/202310051128.nptC5NyW-lkp@intel.com/reproduce)

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

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/drm_panic.c:339:23: sparse: sparse: symbol 'drm_panic_notifier' was not declared. Should it be static?

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[kernel test robot]

Hi Jocelyn,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2dde18cd1d8fac735875f2e4987f11817cc0bc2c]

url:    https://github.com/intel-lab-lkp/linux/commits/Jocelyn-Falempe/drm-format-helper-Export-line-conversion-helper-for-drm_panic/20231003-222642
base:   2dde18cd1d8fac735875f2e4987f11817cc0bc2c
patch link:    https://lore.kernel.org/r/20231003142508.190246-3-jfalempe%40redhat.com
patch subject: [PATCH v4 2/4] drm/panic: Add a drm panic handler
config: i386-randconfig-063-20231005 (https://download.01.org/0day-ci/archive/20231005/202310051128.nptC5NyW-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231005/202310051128.nptC5NyW-lkp@intel.com/reproduce)

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

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/drm_panic.c:339:23: sparse: sparse: symbol 'drm_panic_notifier' was not declared. Should it be static?

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

Re: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[Jocelyn Falempe]

On 04/10/2023 03:45, nerdopolis wrote:
> On Tuesday, October 3, 2023 10:22:44 AM EDT Jocelyn Falempe wrote:
>> drm_panic will need the low-level drm_fb_xxxx_line functions.
>> Also add drm_fb_r1_to_xrgb8888 to render the fonts.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 88 ++++++++++++++++++++++++++---
>>   include/drm/drm_format_helper.h     |  9 +++
>>   2 files changed, 89 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index f93a4efcee90..c238e5d84f1f 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -270,7 +270,30 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>>   
>>   	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, swab_line);
>>   }
>> -EXPORT_SYMBOL(drm_fb_swab);
> I had to add this line back to get it to build, but once I did, it worked.

Thanks, you're right that line shouldn't be removed.

Best regards,

-- 

Jocelyn

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

Hi,

On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 89e2706cac56..e538c87116d3 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -43,6 +43,7 @@ struct dma_buf_attachment;
>  struct drm_display_mode;
>  struct drm_mode_create_dumb;
>  struct drm_printer;
> +struct drm_scanout_buffer;
>  struct sg_table;
>  
>  /**
> @@ -408,6 +409,19 @@ struct drm_driver {
>  	 */
>  	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
>  
> +	/**
> +	 * @get_scanout_buffer:
> +	 *
> +	 * Get the current scanout buffer, to display a panic message with drm_panic.
> +	 * It is called from a panic callback, and must follow its restrictions.
> +	 *
> +	 * Returns:
> +	 *
> +	 * Zero on success, negative errno on failure.
> +	 */
> +	int (*get_scanout_buffer)(struct drm_device *dev,
> +				  struct drm_scanout_buffer *sb);
> +

What is the format of that buffer? What is supposed to happen if the
planes / CRTC are setup in a way that is incompatible with the buffer
format?

I've said it in that other series, but I really think we should be
having a proper state on the side to deal with those properly.

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 05/10/2023 10:18, Maxime Ripard wrote:
> Hi,
> 
> On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 89e2706cac56..e538c87116d3 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -43,6 +43,7 @@ struct dma_buf_attachment;
>>   struct drm_display_mode;
>>   struct drm_mode_create_dumb;
>>   struct drm_printer;
>> +struct drm_scanout_buffer;
>>   struct sg_table;
>>   
>>   /**
>> @@ -408,6 +409,19 @@ struct drm_driver {
>>   	 */
>>   	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
>>   
>> +	/**
>> +	 * @get_scanout_buffer:
>> +	 *
>> +	 * Get the current scanout buffer, to display a panic message with drm_panic.
>> +	 * It is called from a panic callback, and must follow its restrictions.
>> +	 *
>> +	 * Returns:
>> +	 *
>> +	 * Zero on success, negative errno on failure.
>> +	 */
>> +	int (*get_scanout_buffer)(struct drm_device *dev,
>> +				  struct drm_scanout_buffer *sb);
>> +
> 
> What is the format of that buffer? What is supposed to happen if the
> planes / CRTC are setup in a way that is incompatible with the buffer
> format?

Currently, it only supports linear format, either in system memory, or 
iomem.
But really what is needed is the screen size, and a way to write pixels 
to it.
For more complex GPU, I don't know if it's easier to reprogram the GPU 
to linear format, or to add a simple "tiled" support to drm_panic.
What would you propose as a panic interface to handle those complex format ?

Sometime it's also just not possible to write pixels to the screen, like 
if the panic occurs in the middle of suspend/resume, or during a 
mode-setting, and the hardware state is broken. In this case it's ok to 
return an error, and nothing will get displayed.

Best regards,

--

Jocelyn


> 
> I've said it in that other series, but I really think we should be
> having a proper state on the side to deal with those properly.
> 
> Maxime

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

Hi Jocelyn,

On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
> On 05/10/2023 10:18, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
> > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > index 89e2706cac56..e538c87116d3 100644
> > > --- a/include/drm/drm_drv.h
> > > +++ b/include/drm/drm_drv.h
> > > @@ -43,6 +43,7 @@ struct dma_buf_attachment;
> > >   struct drm_display_mode;
> > >   struct drm_mode_create_dumb;
> > >   struct drm_printer;
> > > +struct drm_scanout_buffer;
> > >   struct sg_table;
> > >   /**
> > > @@ -408,6 +409,19 @@ struct drm_driver {
> > >   	 */
> > >   	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
> > > +	/**
> > > +	 * @get_scanout_buffer:
> > > +	 *
> > > +	 * Get the current scanout buffer, to display a panic message with drm_panic.
> > > +	 * It is called from a panic callback, and must follow its restrictions.
> > > +	 *
> > > +	 * Returns:
> > > +	 *
> > > +	 * Zero on success, negative errno on failure.
> > > +	 */
> > > +	int (*get_scanout_buffer)(struct drm_device *dev,
> > > +				  struct drm_scanout_buffer *sb);
> > > +
> > 
> > What is the format of that buffer? What is supposed to happen if the
> > planes / CRTC are setup in a way that is incompatible with the buffer
> > format?
> 
> Currently, it only supports linear format, either in system memory, or
> iomem.
> But really what is needed is the screen size, and a way to write pixels to
> it.
> For more complex GPU, I don't know if it's easier to reprogram the GPU to
> linear format, or to add a simple "tiled" support to drm_panic.
> What would you propose as a panic interface to handle those complex format ?

It's not just about tiling, but also about YUV formats. If the display
engine is currently playing a video at the moment, it's probably going
to output some variation of multi-planar YUV and you won't have an RGB
buffer available.

Same story if you're using a dma-buf buffer. You might not even be able
to access that buffer at all from the CPU or the kernel.

I really think we should have some emergency state ready to commit on
the side, and possibly a panic_commit function to prevent things like
sleeping or waiting that regular atomic_commit can use.

That way, you know have all the resources available to you any time.

> Sometime it's also just not possible to write pixels to the screen, like if
> the panic occurs in the middle of suspend/resume, or during a mode-setting,
> and the hardware state is broken. In this case it's ok to return an error,
> and nothing will get displayed.

And yeah, you won't be able to do it every time, but if it's never for
some workload it's going to be a concern.

Anyway, we should at the very least document what we expect here.

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Noralf Trønnes]

On 10/6/23 16:35, Maxime Ripard wrote:
> Hi Jocelyn,
> 
> On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
>> On 05/10/2023 10:18, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>> index 89e2706cac56..e538c87116d3 100644
>>>> --- a/include/drm/drm_drv.h
>>>> +++ b/include/drm/drm_drv.h
>>>> @@ -43,6 +43,7 @@ struct dma_buf_attachment;
>>>>   struct drm_display_mode;
>>>>   struct drm_mode_create_dumb;
>>>>   struct drm_printer;
>>>> +struct drm_scanout_buffer;
>>>>   struct sg_table;
>>>>   /**
>>>> @@ -408,6 +409,19 @@ struct drm_driver {
>>>>   	 */
>>>>   	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
>>>> +	/**
>>>> +	 * @get_scanout_buffer:
>>>> +	 *
>>>> +	 * Get the current scanout buffer, to display a panic message with drm_panic.
>>>> +	 * It is called from a panic callback, and must follow its restrictions.
>>>> +	 *
>>>> +	 * Returns:
>>>> +	 *
>>>> +	 * Zero on success, negative errno on failure.
>>>> +	 */
>>>> +	int (*get_scanout_buffer)(struct drm_device *dev,
>>>> +				  struct drm_scanout_buffer *sb);
>>>> +
>>>
>>> What is the format of that buffer? What is supposed to happen if the
>>> planes / CRTC are setup in a way that is incompatible with the buffer
>>> format?
>>
>> Currently, it only supports linear format, either in system memory, or
>> iomem.
>> But really what is needed is the screen size, and a way to write pixels to
>> it.
>> For more complex GPU, I don't know if it's easier to reprogram the GPU to
>> linear format, or to add a simple "tiled" support to drm_panic.
>> What would you propose as a panic interface to handle those complex format ?
> 
> It's not just about tiling, but also about YUV formats. If the display
> engine is currently playing a video at the moment, it's probably going
> to output some variation of multi-planar YUV and you won't have an RGB
> buffer available.
> 

I had support for some YUV formats in my 2019 attempt on a panic
handler[1] and I made a recording of a test run as well[2] (see 4:30 for
YUV). There was a discussion about challenges and i915 can disable
tiling by flipping a bit in a register[3] and AMD has a debug
interface[4] they can use to write pixels.

Noralf.

[1]
https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@tronnes.org/
[2] https://youtu.be/lZ80vL4dgpE
[3]
https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
[4]
https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/

> Same story if you're using a dma-buf buffer. You might not even be able
> to access that buffer at all from the CPU or the kernel.
> 
> I really think we should have some emergency state ready to commit on
> the side, and possibly a panic_commit function to prevent things like
> sleeping or waiting that regular atomic_commit can use.
> 
> That way, you know have all the resources available to you any time.
> 
>> Sometime it's also just not possible to write pixels to the screen, like if
>> the panic occurs in the middle of suspend/resume, or during a mode-setting,
>> and the hardware state is broken. In this case it's ok to return an error,
>> and nothing will get displayed.
> 
> And yeah, you won't be able to do it every time, but if it's never for
> some workload it's going to be a concern.
> 
> Anyway, we should at the very least document what we expect here.
> 
> Maxime

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Noralf Trønnes]

On 10/3/23 16:22, Jocelyn Falempe wrote:
> This module displays a user friendly message when a kernel panic
> occurs. It currently doesn't contain any debug information,
> but that can be added later.
> 
> v2
>  * Use get_scanout_buffer() instead of the drm client API.
>   (Thomas Zimmermann)
>  * Add the panic reason to the panic message (Nerdopolis)
>  * Add an exclamation mark (Nerdopolis)
> 
> v3
>  * Rework the drawing functions, to write the pixels line by line and
>  to use the drm conversion helper to support other formats.
>  (Thomas Zimmermann)
> 
> v4
>  * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann)
>  * Remove the default y to DRM_PANIC config option (Thomas Zimmermann)
>  * Add foreground/background color config option
>  * Fix the bottom lines not painted if the framebuffer height
>    is not a multiple of the font height.
>  * Automatically register the device to drm_panic, if the function
>    get_scanout_buffer exists. (Thomas Zimmermann)
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/Kconfig     |  22 ++
>  drivers/gpu/drm/Makefile    |   1 +
>  drivers/gpu/drm/drm_drv.c   |   8 +
>  drivers/gpu/drm/drm_panic.c | 413 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_drv.h       |  14 ++
>  include/drm/drm_panic.h     |  41 ++++
>  6 files changed, 499 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_panic.c
>  create mode 100644 include/drm/drm_panic.h
> 

> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> new file mode 100644

> +static void draw_panic_device(struct drm_device *dev, const char *msg)
> +{
> +	struct drm_scanout_buffer sb;
> +
> +	if (dev->driver->get_scanout_buffer(dev, &sb))
> +		return;
> +	if (!drm_panic_line_buf)
> +		return;
> +

Unless something has changed since 2019 we need to make sure fbcon
hasn't already printed the panic to avoid jumbled output. See [1] for
more info.

Noralf.

[1]
https://lore.kernel.org/dri-devel/20190312095320.GX2665@phenom.ffwll.local/

> +	/* to avoid buffer overflow on drm_panic_line_buf */
> +	if (sb.width > DRM_PANIC_MAX_WIDTH)
> +		sb.width = DRM_PANIC_MAX_WIDTH;
> +
> +	draw_panic_static(&sb, msg);
> +}

Re: [PATCH v4 4/4] drm/mgag200: Add drm_panic support

[Noralf Trønnes]

On 10/3/23 16:22, Jocelyn Falempe wrote:
> Add support for the drm_panic module, which displays a message to
> the screen when a kernel panic occurs.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 976f0ab2006b..229d9c116b42 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -12,10 +12,12 @@
>  #include <drm/drm_aperture.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fbdev_generic.h>
> +#include <drm/drm_framebuffer.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_ioctl.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_module.h>
> +#include <drm/drm_panic.h>
>  #include <drm/drm_pciids.h>
>  
>  #include "mgag200_drv.h"
> @@ -83,6 +85,27 @@ resource_size_t mgag200_probe_vram(void __iomem *mem, resource_size_t size)
>  	return offset - 65536;
>  }
>  
> +static int mgag200_get_scanout_buffer(struct drm_device *dev,
> +				      struct drm_scanout_buffer *sb)
> +{
> +	struct drm_plane *plane;
> +	struct mga_device *mdev = to_mga_device(dev);
> +	struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
> +
> +	/* mgag200 has only one plane */
> +	drm_for_each_plane(plane, dev) {

In my first 2016 attempt on a panic handler I was told to trylock crtc
and plane and skip if it wasn't possible:

- We need locking. One of the biggest problems with the old oops handling
  was that it was very good at trampling over driver state, causing more
  (unrelated) oopses in kms code and making sure the original oops was no
  longer visible. I think the shared code must take care of all the
  locking needs to avoid fragile code in drivers. ww_mutex_trylock on the
  drm_crtc and drm_plane should be enough (we need both for drivers where
  planes can be reassigned between drivers).

See [1] for a list of other things to consider.

Noralf.

[1]
https://lore.kernel.org/dri-devel/20160810091529.GQ6232@phenom.ffwll.local/

> +		if (!plane->state || !plane->state->fb)
> +			return -ENODEV;
> +		sb->format = plane->state->fb->format;
> +		sb->width = plane->state->fb->width;
> +		sb->height = plane->state->fb->height;
> +		sb->pitch = plane->state->fb->pitches[0];
> +		sb->map = map;
> +		return 0;
> +	}
> +	return -ENODEV;
> +}
> +
>  /*
>   * DRM driver
>   */
> @@ -98,6 +121,7 @@ static const struct drm_driver mgag200_driver = {
>  	.major = DRIVER_MAJOR,
>  	.minor = DRIVER_MINOR,
>  	.patchlevel = DRIVER_PATCHLEVEL,
> +	.get_scanout_buffer = mgag200_get_scanout_buffer,
>  	DRM_GEM_SHMEM_DRIVER_OPS,
>  };
>  

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 06/10/2023 18:54, Noralf Trønnes wrote:
> 
> 
> On 10/6/23 16:35, Maxime Ripard wrote:
>> Hi Jocelyn,
>>
>> On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
>>> On 05/10/2023 10:18, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>> index 89e2706cac56..e538c87116d3 100644
>>>>> --- a/include/drm/drm_drv.h
>>>>> +++ b/include/drm/drm_drv.h
>>>>> @@ -43,6 +43,7 @@ struct dma_buf_attachment;
>>>>>    struct drm_display_mode;
>>>>>    struct drm_mode_create_dumb;
>>>>>    struct drm_printer;
>>>>> +struct drm_scanout_buffer;
>>>>>    struct sg_table;
>>>>>    /**
>>>>> @@ -408,6 +409,19 @@ struct drm_driver {
>>>>>    	 */
>>>>>    	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
>>>>> +	/**
>>>>> +	 * @get_scanout_buffer:
>>>>> +	 *
>>>>> +	 * Get the current scanout buffer, to display a panic message with drm_panic.
>>>>> +	 * It is called from a panic callback, and must follow its restrictions.
>>>>> +	 *
>>>>> +	 * Returns:
>>>>> +	 *
>>>>> +	 * Zero on success, negative errno on failure.
>>>>> +	 */
>>>>> +	int (*get_scanout_buffer)(struct drm_device *dev,
>>>>> +				  struct drm_scanout_buffer *sb);
>>>>> +
>>>>
>>>> What is the format of that buffer? What is supposed to happen if the
>>>> planes / CRTC are setup in a way that is incompatible with the buffer
>>>> format?
>>>
>>> Currently, it only supports linear format, either in system memory, or
>>> iomem.
>>> But really what is needed is the screen size, and a way to write pixels to
>>> it.
>>> For more complex GPU, I don't know if it's easier to reprogram the GPU to
>>> linear format, or to add a simple "tiled" support to drm_panic.
>>> What would you propose as a panic interface to handle those complex format ?
>>
>> It's not just about tiling, but also about YUV formats. If the display
>> engine is currently playing a video at the moment, it's probably going
>> to output some variation of multi-planar YUV and you won't have an RGB
>> buffer available.
>>
> 
> I had support for some YUV formats in my 2019 attempt on a panic
> handler[1] and I made a recording of a test run as well[2] (see 4:30 for
> YUV). There was a discussion about challenges and i915 can disable
> tiling by flipping a bit in a register[3] and AMD has a debug
> interface[4] they can use to write pixels.

I only added support for the format used by simpledrm, because I don't 
want to add support for all possible format if no driver are using it.
It should be possible to add YUV format too.

I also prefer to convert only the foreground/background color, and then 
write directly into the buffers, instead of converting line by line.
It works for all format where pixel size is a multiple of byte.

> 
> Noralf.
> 
> [1]
> https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@tronnes.org/
> [2] https://youtu.be/lZ80vL4dgpE
> [3]
> https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
> [4]
> https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
> 
>> Same story if you're using a dma-buf buffer. You might not even be able
>> to access that buffer at all from the CPU or the kernel.
>>
>> I really think we should have some emergency state ready to commit on
>> the side, and possibly a panic_commit function to prevent things like
>> sleeping or waiting that regular atomic_commit can use.
>>
>> That way, you know have all the resources available to you any time.

I think reusing the atomic commit functions might be hard, because there 
are locks/allocation/threads hidden in drivers callback. I'm more in 
favor of an emergency function, that each driver has to implement, and 
use what the hardware can do to display a simple frame quickly.
get_scanout_buffer() is a good start for simple driver, but will need 
refactoring for the more complex case, like adding a callback to write 
pixels one by one, if there is no memory mapped buffer available.

>>
>>> Sometime it's also just not possible to write pixels to the screen, like if
>>> the panic occurs in the middle of suspend/resume, or during a mode-setting,
>>> and the hardware state is broken. In this case it's ok to return an error,
>>> and nothing will get displayed.
>>
>> And yeah, you won't be able to do it every time, but if it's never for
>> some workload it's going to be a concern.
>>
>> Anyway, we should at the very least document what we expect here.

Yes I should better document the drm panic feature, and the 
get_scanout_buffer() interface.

>>
>> Maxime
> 

-- 

Jocelyn

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 07/10/2023 14:38, Noralf Trønnes wrote:
> 
> 
> On 10/3/23 16:22, Jocelyn Falempe wrote:
>> This module displays a user friendly message when a kernel panic
>> occurs. It currently doesn't contain any debug information,
>> but that can be added later.
>>
>> v2
>>   * Use get_scanout_buffer() instead of the drm client API.
>>    (Thomas Zimmermann)
>>   * Add the panic reason to the panic message (Nerdopolis)
>>   * Add an exclamation mark (Nerdopolis)
>>
>> v3
>>   * Rework the drawing functions, to write the pixels line by line and
>>   to use the drm conversion helper to support other formats.
>>   (Thomas Zimmermann)
>>
>> v4
>>   * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann)
>>   * Remove the default y to DRM_PANIC config option (Thomas Zimmermann)
>>   * Add foreground/background color config option
>>   * Fix the bottom lines not painted if the framebuffer height
>>     is not a multiple of the font height.
>>   * Automatically register the device to drm_panic, if the function
>>     get_scanout_buffer exists. (Thomas Zimmermann)
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/Kconfig     |  22 ++
>>   drivers/gpu/drm/Makefile    |   1 +
>>   drivers/gpu/drm/drm_drv.c   |   8 +
>>   drivers/gpu/drm/drm_panic.c | 413 ++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_drv.h       |  14 ++
>>   include/drm/drm_panic.h     |  41 ++++
>>   6 files changed, 499 insertions(+)
>>   create mode 100644 drivers/gpu/drm/drm_panic.c
>>   create mode 100644 include/drm/drm_panic.h
>>
> 
>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>> new file mode 100644
> 
>> +static void draw_panic_device(struct drm_device *dev, const char *msg)
>> +{
>> +	struct drm_scanout_buffer sb;
>> +
>> +	if (dev->driver->get_scanout_buffer(dev, &sb))
>> +		return;
>> +	if (!drm_panic_line_buf)
>> +		return;
>> +
> 
> Unless something has changed since 2019 we need to make sure fbcon
> hasn't already printed the panic to avoid jumbled output. See [1] for
> more info.

I think DRM_PANIC and fbcon are incompatible, so in Kconfig I prevent 
them to be built together:

config DRM_PANIC
   bool "Display a user-friendly message when a kernel panic occurs"
   depends on DRM && !FRAMEBUFFER_CONSOLE

So DRM_PANIC should be used with a userspace console, either kmscon, or 
some lightweight terminal emulator/wayland compositor, like cage/foot.

As fbcon has lost scrolling support, it's time to switch to something 
better. [1]

-- 

Jocelyn

[1]
https://www.reddit.com/r/linux/comments/10eccv9/config_vtn_in_2023/

> 
> Noralf.
> 
> [1]
> https://lore.kernel.org/dri-devel/20190312095320.GX2665@phenom.ffwll.local/
> 
>> +	/* to avoid buffer overflow on drm_panic_line_buf */
>> +	if (sb.width > DRM_PANIC_MAX_WIDTH)
>> +		sb.width = DRM_PANIC_MAX_WIDTH;
>> +
>> +	draw_panic_static(&sb, msg);
>> +}
> 

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

Hi,

On Mon, Oct 09, 2023 at 09:47:49AM +0200, Jocelyn Falempe wrote:
> On 06/10/2023 18:54, Noralf Trønnes wrote:
> > 
> > 
> > On 10/6/23 16:35, Maxime Ripard wrote:
> > > Hi Jocelyn,
> > > 
> > > On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
> > > > On 05/10/2023 10:18, Maxime Ripard wrote:
> > > > > Hi,
> > > > > 
> > > > > On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
> > > > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > > > > index 89e2706cac56..e538c87116d3 100644
> > > > > > --- a/include/drm/drm_drv.h
> > > > > > +++ b/include/drm/drm_drv.h
> > > > > > @@ -43,6 +43,7 @@ struct dma_buf_attachment;
> > > > > >    struct drm_display_mode;
> > > > > >    struct drm_mode_create_dumb;
> > > > > >    struct drm_printer;
> > > > > > +struct drm_scanout_buffer;
> > > > > >    struct sg_table;
> > > > > >    /**
> > > > > > @@ -408,6 +409,19 @@ struct drm_driver {
> > > > > >    	 */
> > > > > >    	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
> > > > > > +	/**
> > > > > > +	 * @get_scanout_buffer:
> > > > > > +	 *
> > > > > > +	 * Get the current scanout buffer, to display a panic message with drm_panic.
> > > > > > +	 * It is called from a panic callback, and must follow its restrictions.
> > > > > > +	 *
> > > > > > +	 * Returns:
> > > > > > +	 *
> > > > > > +	 * Zero on success, negative errno on failure.
> > > > > > +	 */
> > > > > > +	int (*get_scanout_buffer)(struct drm_device *dev,
> > > > > > +				  struct drm_scanout_buffer *sb);
> > > > > > +
> > > > > 
> > > > > What is the format of that buffer? What is supposed to happen if the
> > > > > planes / CRTC are setup in a way that is incompatible with the buffer
> > > > > format?
> > > > 
> > > > Currently, it only supports linear format, either in system memory, or
> > > > iomem.
> > > > But really what is needed is the screen size, and a way to write pixels to
> > > > it.
> > > > For more complex GPU, I don't know if it's easier to reprogram the GPU to
> > > > linear format, or to add a simple "tiled" support to drm_panic.
> > > > What would you propose as a panic interface to handle those complex format ?
> > > 
> > > It's not just about tiling, but also about YUV formats. If the display
> > > engine is currently playing a video at the moment, it's probably going
> > > to output some variation of multi-planar YUV and you won't have an RGB
> > > buffer available.
> > > 
> > 
> > I had support for some YUV formats in my 2019 attempt on a panic
> > handler[1] and I made a recording of a test run as well[2] (see 4:30 for
> > YUV). There was a discussion about challenges and i915 can disable
> > tiling by flipping a bit in a register[3] and AMD has a debug
> > interface[4] they can use to write pixels.
> 
> I only added support for the format used by simpledrm, because I don't want
> to add support for all possible format if no driver are using it.

Sure.

> It should be possible to add YUV format too.
>
> I also prefer to convert only the foreground/background color, and then
> write directly into the buffers, instead of converting line by line.
> It works for all format where pixel size is a multiple of byte.

My point was that there might not be a buffer to write to.
DMA_ATTR_NO_KERNEL_MAPPING exists, dma-buf might be unaccessible, unsafe
or be completely out of control of the kernel space, or even not be
accessible by the system at all (when doing secure playback for example,
where the "trusted" component will do the decoding and will only give
back a dma-buf handle to a secure memory buffer).

I appreciate that we probably don't want to address these scenarios
right now, but we should have a path forward to support them eventually.
Copying the panic handler content to the buffer is optimistic and won't
work in all the scenarios described above, pretty much requiring to
start from scratch that effort.

> > https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@tronnes.org/
> > [2] https://youtu.be/lZ80vL4dgpE
> > [3]
> > https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
> > [4]
> > https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
> > 
> > > Same story if you're using a dma-buf buffer. You might not even be able
> > > to access that buffer at all from the CPU or the kernel.
> > > 
> > > I really think we should have some emergency state ready to commit on
> > > the side, and possibly a panic_commit function to prevent things like
> > > sleeping or waiting that regular atomic_commit can use.
> > > 
> > > That way, you know have all the resources available to you any time.
> 
> I think reusing the atomic commit functions might be hard, because there are
> locks/allocation/threads hidden in drivers callback.

Allocations are bugs as far as I'm concerned. Allocations in
atomic_commit path are pretty big hint that you're doing something wrong
so I wouldn't worry too much about them. For locking, yeah... Which is
why I was suggesting an emergency atomic_commit of some sorts (for
planes only?). Switching back to whatever we were doing to an RGB plane
should be simple enough for most drivers and probably can be done safely
enough on most drivers without any locks.

And you don't need to support all kinds of tiling, YUV or RGB variants.

> I'm more in favor of an emergency function, that each driver has to
> implement, and use what the hardware can do to display a simple frame
> quickly. get_scanout_buffer() is a good start for simple driver, but
> will need refactoring for the more complex case, like adding a
> callback to write pixels one by one, if there is no memory mapped
> buffer available.

Sorry, I'm not quite sure what you mean there, where would you write the
pixel to?

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 09/10/2023 10:28, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Oct 09, 2023 at 09:47:49AM +0200, Jocelyn Falempe wrote:
>> On 06/10/2023 18:54, Noralf Trønnes wrote:
>>>
>>>
>>> On 10/6/23 16:35, Maxime Ripard wrote:
>>>> Hi Jocelyn,
>>>>
>>>> On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
>>>>> On 05/10/2023 10:18, Maxime Ripard wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
>>>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>>>> index 89e2706cac56..e538c87116d3 100644
>>>>>>> --- a/include/drm/drm_drv.h
>>>>>>> +++ b/include/drm/drm_drv.h
>>>>>>> @@ -43,6 +43,7 @@ struct dma_buf_attachment;
>>>>>>>     struct drm_display_mode;
>>>>>>>     struct drm_mode_create_dumb;
>>>>>>>     struct drm_printer;
>>>>>>> +struct drm_scanout_buffer;
>>>>>>>     struct sg_table;
>>>>>>>     /**
>>>>>>> @@ -408,6 +409,19 @@ struct drm_driver {
>>>>>>>     	 */
>>>>>>>     	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
>>>>>>> +	/**
>>>>>>> +	 * @get_scanout_buffer:
>>>>>>> +	 *
>>>>>>> +	 * Get the current scanout buffer, to display a panic message with drm_panic.
>>>>>>> +	 * It is called from a panic callback, and must follow its restrictions.
>>>>>>> +	 *
>>>>>>> +	 * Returns:
>>>>>>> +	 *
>>>>>>> +	 * Zero on success, negative errno on failure.
>>>>>>> +	 */
>>>>>>> +	int (*get_scanout_buffer)(struct drm_device *dev,
>>>>>>> +				  struct drm_scanout_buffer *sb);
>>>>>>> +
>>>>>>
>>>>>> What is the format of that buffer? What is supposed to happen if the
>>>>>> planes / CRTC are setup in a way that is incompatible with the buffer
>>>>>> format?
>>>>>
>>>>> Currently, it only supports linear format, either in system memory, or
>>>>> iomem.
>>>>> But really what is needed is the screen size, and a way to write pixels to
>>>>> it.
>>>>> For more complex GPU, I don't know if it's easier to reprogram the GPU to
>>>>> linear format, or to add a simple "tiled" support to drm_panic.
>>>>> What would you propose as a panic interface to handle those complex format ?
>>>>
>>>> It's not just about tiling, but also about YUV formats. If the display
>>>> engine is currently playing a video at the moment, it's probably going
>>>> to output some variation of multi-planar YUV and you won't have an RGB
>>>> buffer available.
>>>>
>>>
>>> I had support for some YUV formats in my 2019 attempt on a panic
>>> handler[1] and I made a recording of a test run as well[2] (see 4:30 for
>>> YUV). There was a discussion about challenges and i915 can disable
>>> tiling by flipping a bit in a register[3] and AMD has a debug
>>> interface[4] they can use to write pixels.
>>
>> I only added support for the format used by simpledrm, because I don't want
>> to add support for all possible format if no driver are using it.
> 
> Sure.
> 
>> It should be possible to add YUV format too.
>>
>> I also prefer to convert only the foreground/background color, and then
>> write directly into the buffers, instead of converting line by line.
>> It works for all format where pixel size is a multiple of byte.
> 
> My point was that there might not be a buffer to write to.
> DMA_ATTR_NO_KERNEL_MAPPING exists, dma-buf might be unaccessible, unsafe
> or be completely out of control of the kernel space, or even not be
> accessible by the system at all (when doing secure playback for example,
> where the "trusted" component will do the decoding and will only give
> back a dma-buf handle to a secure memory buffer).
> 
> I appreciate that we probably don't want to address these scenarios
> right now, but we should have a path forward to support them eventually.
> Copying the panic handler content to the buffer is optimistic and won't
> work in all the scenarios described above, pretty much requiring to
> start from scratch that effort.
> 
>>> https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@tronnes.org/
>>> [2] https://youtu.be/lZ80vL4dgpE
>>> [3]
>>> https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
>>> [4]
>>> https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
>>>
>>>> Same story if you're using a dma-buf buffer. You might not even be able
>>>> to access that buffer at all from the CPU or the kernel.
>>>>
>>>> I really think we should have some emergency state ready to commit on
>>>> the side, and possibly a panic_commit function to prevent things like
>>>> sleeping or waiting that regular atomic_commit can use.
>>>>
>>>> That way, you know have all the resources available to you any time.
>>
>> I think reusing the atomic commit functions might be hard, because there are
>> locks/allocation/threads hidden in drivers callback.
> 
> Allocations are bugs as far as I'm concerned. Allocations in
> atomic_commit path are pretty big hint that you're doing something wrong
> so I wouldn't worry too much about them. For locking, yeah... Which is
> why I was suggesting an emergency atomic_commit of some sorts (for
> planes only?). Switching back to whatever we were doing to an RGB plane
> should be simple enough for most drivers and probably can be done safely
> enough on most drivers without any locks.
> 
> And you don't need to support all kinds of tiling, YUV or RGB variants.

So if I understand correctly, drm_panic would pre-allocate a 
plane/commit, and use that when a panic occurs ?
I have two concern about this approach:
- How much memory would be allocated for this ? a whole framebuffer can 
be big for just this use case.
- I find it risky to completely reconfigure the hardware in a panic handler.

Also how many drivers would need this ?

Currently I was mostly considering x86 platform, so:

simpledrm/ast/mgag200 which works well with the get_scanout_buffer().

i915/amdgpu/nouveau, which are quite complex, and will need to do their 
own thing anyway.


> 
>> I'm more in favor of an emergency function, that each driver has to
>> implement, and use what the hardware can do to display a simple frame
>> quickly. get_scanout_buffer() is a good start for simple driver, but
>> will need refactoring for the more complex case, like adding a
>> callback to write pixels one by one, if there is no memory mapped
>> buffer available.
> 
> Sorry, I'm not quite sure what you mean there, where would you write the
> pixel to?

It was mentioned by Noralf, about the amdgpu driver:

https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/

They have a slow "debug interface" that you can write to, and can be 
used for the panic handler. It's not memory mapped, so you have to write 
pixels one by one.

So for the struct drm_scanout_buffer, I can add a function pointer to a 
write_pixel(u32 x, u32 y, u32 color)
So if the iosys map is null, it will use that instead.

> 
> Maxime

Re: [PATCH v4 4/4] drm/mgag200: Add drm_panic support

[Jocelyn Falempe]

On 07/10/2023 16:30, Noralf Trønnes wrote:
> 
> 
> On 10/3/23 16:22, Jocelyn Falempe wrote:
>> Add support for the drm_panic module, which displays a message to
>> the screen when a kernel panic occurs.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/mgag200/mgag200_drv.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index 976f0ab2006b..229d9c116b42 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -12,10 +12,12 @@
>>   #include <drm/drm_aperture.h>
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_fbdev_generic.h>
>> +#include <drm/drm_framebuffer.h>
>>   #include <drm/drm_file.h>
>>   #include <drm/drm_ioctl.h>
>>   #include <drm/drm_managed.h>
>>   #include <drm/drm_module.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_pciids.h>
>>   
>>   #include "mgag200_drv.h"
>> @@ -83,6 +85,27 @@ resource_size_t mgag200_probe_vram(void __iomem *mem, resource_size_t size)
>>   	return offset - 65536;
>>   }
>>   
>> +static int mgag200_get_scanout_buffer(struct drm_device *dev,
>> +				      struct drm_scanout_buffer *sb)
>> +{
>> +	struct drm_plane *plane;
>> +	struct mga_device *mdev = to_mga_device(dev);
>> +	struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
>> +
>> +	/* mgag200 has only one plane */
>> +	drm_for_each_plane(plane, dev) {
> 
> In my first 2016 attempt on a panic handler I was told to trylock crtc
> and plane and skip if it wasn't possible:
> 
> - We need locking. One of the biggest problems with the old oops handling
>    was that it was very good at trampling over driver state, causing more
>    (unrelated) oopses in kms code and making sure the original oops was no
>    longer visible. I think the shared code must take care of all the
>    locking needs to avoid fragile code in drivers. ww_mutex_trylock on the
>    drm_crtc and drm_plane should be enough (we need both for drivers where
>    planes can be reassigned between drivers).

drm_panic register a panic handler, so it won't get called on oopses.
So when the panic handler is called, no other task can run in parallel, 
and no drm code will run after it.

Also for the Matrox driver, it's always safe to write to the VRAM. The 
only risk is that if you're in the middle of a modeset, you may get 
garbage output.
There might still be a race condition for fb->format, width, height and 
pitches, and looping through the plane list.

I also need to check if crtc and plane locks are not taken when the 
driver is copying the damage region to the VRAM, otherwise the 
probability to actually see the panic will be very low.

I also need to wake up the monitor, if it's currently in DPMS off.

-- 

Jocelyn


> 
> See [1] for a list of other things to consider.
> 
> Noralf.
> 
> [1]
> https://lore.kernel.org/dri-devel/20160810091529.GQ6232@phenom.ffwll.local/
> 
>> +		if (!plane->state || !plane->state->fb)
>> +			return -ENODEV;
>> +		sb->format = plane->state->fb->format;
>> +		sb->width = plane->state->fb->width;
>> +		sb->height = plane->state->fb->height;
>> +		sb->pitch = plane->state->fb->pitches[0];
>> +		sb->map = map;
>> +		return 0;
>> +	}
>> +	return -ENODEV;
>> +}
>> +
>>   /*
>>    * DRM driver
>>    */
>> @@ -98,6 +121,7 @@ static const struct drm_driver mgag200_driver = {
>>   	.major = DRIVER_MAJOR,
>>   	.minor = DRIVER_MINOR,
>>   	.patchlevel = DRIVER_PATCHLEVEL,
>> +	.get_scanout_buffer = mgag200_get_scanout_buffer,
>>   	DRM_GEM_SHMEM_DRIVER_OPS,
>>   };
>>   
> 

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

On Mon, Oct 09, 2023 at 11:48:29AM +0200, Jocelyn Falempe wrote:
> On 09/10/2023 10:28, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, Oct 09, 2023 at 09:47:49AM +0200, Jocelyn Falempe wrote:
> > > On 06/10/2023 18:54, Noralf Trønnes wrote:
> > > > 
> > > > 
> > > > On 10/6/23 16:35, Maxime Ripard wrote:
> > > > > Hi Jocelyn,
> > > > > 
> > > > > On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
> > > > > > On 05/10/2023 10:18, Maxime Ripard wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
> > > > > > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > > > > > > > index 89e2706cac56..e538c87116d3 100644
> > > > > > > > --- a/include/drm/drm_drv.h
> > > > > > > > +++ b/include/drm/drm_drv.h
> > > > > > > > @@ -43,6 +43,7 @@ struct dma_buf_attachment;
> > > > > > > >     struct drm_display_mode;
> > > > > > > >     struct drm_mode_create_dumb;
> > > > > > > >     struct drm_printer;
> > > > > > > > +struct drm_scanout_buffer;
> > > > > > > >     struct sg_table;
> > > > > > > >     /**
> > > > > > > > @@ -408,6 +409,19 @@ struct drm_driver {
> > > > > > > >     	 */
> > > > > > > >     	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
> > > > > > > > +	/**
> > > > > > > > +	 * @get_scanout_buffer:
> > > > > > > > +	 *
> > > > > > > > +	 * Get the current scanout buffer, to display a panic message with drm_panic.
> > > > > > > > +	 * It is called from a panic callback, and must follow its restrictions.
> > > > > > > > +	 *
> > > > > > > > +	 * Returns:
> > > > > > > > +	 *
> > > > > > > > +	 * Zero on success, negative errno on failure.
> > > > > > > > +	 */
> > > > > > > > +	int (*get_scanout_buffer)(struct drm_device *dev,
> > > > > > > > +				  struct drm_scanout_buffer *sb);
> > > > > > > > +
> > > > > > > 
> > > > > > > What is the format of that buffer? What is supposed to happen if the
> > > > > > > planes / CRTC are setup in a way that is incompatible with the buffer
> > > > > > > format?
> > > > > > 
> > > > > > Currently, it only supports linear format, either in system memory, or
> > > > > > iomem.
> > > > > > But really what is needed is the screen size, and a way to write pixels to
> > > > > > it.
> > > > > > For more complex GPU, I don't know if it's easier to reprogram the GPU to
> > > > > > linear format, or to add a simple "tiled" support to drm_panic.
> > > > > > What would you propose as a panic interface to handle those complex format ?
> > > > > 
> > > > > It's not just about tiling, but also about YUV formats. If the display
> > > > > engine is currently playing a video at the moment, it's probably going
> > > > > to output some variation of multi-planar YUV and you won't have an RGB
> > > > > buffer available.
> > > > > 
> > > > 
> > > > I had support for some YUV formats in my 2019 attempt on a panic
> > > > handler[1] and I made a recording of a test run as well[2] (see 4:30 for
> > > > YUV). There was a discussion about challenges and i915 can disable
> > > > tiling by flipping a bit in a register[3] and AMD has a debug
> > > > interface[4] they can use to write pixels.
> > > 
> > > I only added support for the format used by simpledrm, because I don't want
> > > to add support for all possible format if no driver are using it.
> > 
> > Sure.
> > 
> > > It should be possible to add YUV format too.
> > > 
> > > I also prefer to convert only the foreground/background color, and then
> > > write directly into the buffers, instead of converting line by line.
> > > It works for all format where pixel size is a multiple of byte.
> > 
> > My point was that there might not be a buffer to write to.
> > DMA_ATTR_NO_KERNEL_MAPPING exists, dma-buf might be unaccessible, unsafe
> > or be completely out of control of the kernel space, or even not be
> > accessible by the system at all (when doing secure playback for example,
> > where the "trusted" component will do the decoding and will only give
> > back a dma-buf handle to a secure memory buffer).
> > 
> > I appreciate that we probably don't want to address these scenarios
> > right now, but we should have a path forward to support them eventually.
> > Copying the panic handler content to the buffer is optimistic and won't
> > work in all the scenarios described above, pretty much requiring to
> > start from scratch that effort.
> > 
> > > > https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@tronnes.org/
> > > > [2] https://youtu.be/lZ80vL4dgpE
> > > > [3]
> > > > https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
> > > > [4]
> > > > https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
> > > > 
> > > > > Same story if you're using a dma-buf buffer. You might not even be able
> > > > > to access that buffer at all from the CPU or the kernel.
> > > > > 
> > > > > I really think we should have some emergency state ready to commit on
> > > > > the side, and possibly a panic_commit function to prevent things like
> > > > > sleeping or waiting that regular atomic_commit can use.
> > > > > 
> > > > > That way, you know have all the resources available to you any time.
> > > 
> > > I think reusing the atomic commit functions might be hard, because there are
> > > locks/allocation/threads hidden in drivers callback.
> > 
> > Allocations are bugs as far as I'm concerned. Allocations in
> > atomic_commit path are pretty big hint that you're doing something wrong
> > so I wouldn't worry too much about them. For locking, yeah... Which is
> > why I was suggesting an emergency atomic_commit of some sorts (for
> > planes only?). Switching back to whatever we were doing to an RGB plane
> > should be simple enough for most drivers and probably can be done safely
> > enough on most drivers without any locks.
> > 
> > And you don't need to support all kinds of tiling, YUV or RGB variants.
> 
> So if I understand correctly, drm_panic would pre-allocate a plane/commit,
> and use that when a panic occurs ?

And have it checked already, yes. We would only wait for a panic to
happen to pull the trigger on the commit.

> I have two concern about this approach:
> - How much memory would be allocated for this ? a whole framebuffer can be
> big for just this use case.

I'dd expect a whole framebuffer for the current
configuration/resolution. It would be typically 4MB for a full-HD system
which isn't a lot really and I guess we can always add an option to
disable the mechanism if needed.

> - I find it risky to completely reconfigure the hardware in a panic handler.

I would expect to only change the format and base address of the
framebuffer. I guess it can fail, but it doesn't seem that different to
the async plane update we already have and works well.

> Also how many drivers would need this ?
> 
> Currently I was mostly considering x86 platform, so:
> 
> simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
> 
> i915/amdgpu/nouveau, which are quite complex, and will need to do their own
> thing anyway.

I guess we're not entirely aligned there then. I would expect that
mechanism to work with any atomic KMS driver. You are right that i915,
amdgpu and nouveau are special enough that some extra internal plumbing
is going to be required, but I'd expect it to be easy to support with
any other driver for a memory-mapped device.

I guess what I'm trying to say is, even though it's totally fine that
you only support those drivers at first, supporting in vc4 for example
shouldn't require to rewrite the whole thing.

> > > I'm more in favor of an emergency function, that each driver has to
> > > implement, and use what the hardware can do to display a simple frame
> > > quickly. get_scanout_buffer() is a good start for simple driver, but
> > > will need refactoring for the more complex case, like adding a
> > > callback to write pixels one by one, if there is no memory mapped
> > > buffer available.
> > 
> > Sorry, I'm not quite sure what you mean there, where would you write the
> > pixel to?
> 
> It was mentioned by Noralf, about the amdgpu driver:
> 
> https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
> 
> They have a slow "debug interface" that you can write to, and can be used
> for the panic handler. It's not memory mapped, so you have to write pixels
> one by one.
> 
> So for the struct drm_scanout_buffer, I can add a function pointer to a
> write_pixel(u32 x, u32 y, u32 color)
> So if the iosys map is null, it will use that instead.

It would be nice to support indeed, but it's a fairly rare feature afaik
so I'd rather focus on getting something that can work for everyone first

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 09/10/2023 13:33, Maxime Ripard wrote:
> On Mon, Oct 09, 2023 at 11:48:29AM +0200, Jocelyn Falempe wrote:
>> On 09/10/2023 10:28, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Mon, Oct 09, 2023 at 09:47:49AM +0200, Jocelyn Falempe wrote:
>>>> On 06/10/2023 18:54, Noralf Trønnes wrote:
>>>>>
>>>>>
>>>>> On 10/6/23 16:35, Maxime Ripard wrote:
>>>>>> Hi Jocelyn,
>>>>>>
>>>>>> On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
>>>>>>> On 05/10/2023 10:18, Maxime Ripard wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
>>>>>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>>>>>> index 89e2706cac56..e538c87116d3 100644
>>>>>>>>> --- a/include/drm/drm_drv.h
>>>>>>>>> +++ b/include/drm/drm_drv.h
>>>>>>>>> @@ -43,6 +43,7 @@ struct dma_buf_attachment;
>>>>>>>>>      struct drm_display_mode;
>>>>>>>>>      struct drm_mode_create_dumb;
>>>>>>>>>      struct drm_printer;
>>>>>>>>> +struct drm_scanout_buffer;
>>>>>>>>>      struct sg_table;
>>>>>>>>>      /**
>>>>>>>>> @@ -408,6 +409,19 @@ struct drm_driver {
>>>>>>>>>      	 */
>>>>>>>>>      	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
>>>>>>>>> +	/**
>>>>>>>>> +	 * @get_scanout_buffer:
>>>>>>>>> +	 *
>>>>>>>>> +	 * Get the current scanout buffer, to display a panic message with drm_panic.
>>>>>>>>> +	 * It is called from a panic callback, and must follow its restrictions.
>>>>>>>>> +	 *
>>>>>>>>> +	 * Returns:
>>>>>>>>> +	 *
>>>>>>>>> +	 * Zero on success, negative errno on failure.
>>>>>>>>> +	 */
>>>>>>>>> +	int (*get_scanout_buffer)(struct drm_device *dev,
>>>>>>>>> +				  struct drm_scanout_buffer *sb);
>>>>>>>>> +
>>>>>>>>
>>>>>>>> What is the format of that buffer? What is supposed to happen if the
>>>>>>>> planes / CRTC are setup in a way that is incompatible with the buffer
>>>>>>>> format?
>>>>>>>
>>>>>>> Currently, it only supports linear format, either in system memory, or
>>>>>>> iomem.
>>>>>>> But really what is needed is the screen size, and a way to write pixels to
>>>>>>> it.
>>>>>>> For more complex GPU, I don't know if it's easier to reprogram the GPU to
>>>>>>> linear format, or to add a simple "tiled" support to drm_panic.
>>>>>>> What would you propose as a panic interface to handle those complex format ?
>>>>>>
>>>>>> It's not just about tiling, but also about YUV formats. If the display
>>>>>> engine is currently playing a video at the moment, it's probably going
>>>>>> to output some variation of multi-planar YUV and you won't have an RGB
>>>>>> buffer available.
>>>>>>
>>>>>
>>>>> I had support for some YUV formats in my 2019 attempt on a panic
>>>>> handler[1] and I made a recording of a test run as well[2] (see 4:30 for
>>>>> YUV). There was a discussion about challenges and i915 can disable
>>>>> tiling by flipping a bit in a register[3] and AMD has a debug
>>>>> interface[4] they can use to write pixels.
>>>>
>>>> I only added support for the format used by simpledrm, because I don't want
>>>> to add support for all possible format if no driver are using it.
>>>
>>> Sure.
>>>
>>>> It should be possible to add YUV format too.
>>>>
>>>> I also prefer to convert only the foreground/background color, and then
>>>> write directly into the buffers, instead of converting line by line.
>>>> It works for all format where pixel size is a multiple of byte.
>>>
>>> My point was that there might not be a buffer to write to.
>>> DMA_ATTR_NO_KERNEL_MAPPING exists, dma-buf might be unaccessible, unsafe
>>> or be completely out of control of the kernel space, or even not be
>>> accessible by the system at all (when doing secure playback for example,
>>> where the "trusted" component will do the decoding and will only give
>>> back a dma-buf handle to a secure memory buffer).
>>>
>>> I appreciate that we probably don't want to address these scenarios
>>> right now, but we should have a path forward to support them eventually.
>>> Copying the panic handler content to the buffer is optimistic and won't
>>> work in all the scenarios described above, pretty much requiring to
>>> start from scratch that effort.
>>>
>>>>> https://lore.kernel.org/dri-devel/20190311174218.51899-1-noralf@tronnes.org/
>>>>> [2] https://youtu.be/lZ80vL4dgpE
>>>>> [3]
>>>>> https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
>>>>> [4]
>>>>> https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
>>>>>
>>>>>> Same story if you're using a dma-buf buffer. You might not even be able
>>>>>> to access that buffer at all from the CPU or the kernel.
>>>>>>
>>>>>> I really think we should have some emergency state ready to commit on
>>>>>> the side, and possibly a panic_commit function to prevent things like
>>>>>> sleeping or waiting that regular atomic_commit can use.
>>>>>>
>>>>>> That way, you know have all the resources available to you any time.
>>>>
>>>> I think reusing the atomic commit functions might be hard, because there are
>>>> locks/allocation/threads hidden in drivers callback.
>>>
>>> Allocations are bugs as far as I'm concerned. Allocations in
>>> atomic_commit path are pretty big hint that you're doing something wrong
>>> so I wouldn't worry too much about them. For locking, yeah... Which is
>>> why I was suggesting an emergency atomic_commit of some sorts (for
>>> planes only?). Switching back to whatever we were doing to an RGB plane
>>> should be simple enough for most drivers and probably can be done safely
>>> enough on most drivers without any locks.
>>>
>>> And you don't need to support all kinds of tiling, YUV or RGB variants.
>>
>> So if I understand correctly, drm_panic would pre-allocate a plane/commit,
>> and use that when a panic occurs ?
> 
> And have it checked already, yes. We would only wait for a panic to
> happen to pull the trigger on the commit.
> 
>> I have two concern about this approach:
>> - How much memory would be allocated for this ? a whole framebuffer can be
>> big for just this use case.
> 
> I'dd expect a whole framebuffer for the current
> configuration/resolution. It would be typically 4MB for a full-HD system
> which isn't a lot really and I guess we can always add an option to
> disable the mechanism if needed.

Ok, that's a bit big, but if there is no other way.

> 
>> - I find it risky to completely reconfigure the hardware in a panic handler.
> 
> I would expect to only change the format and base address of the
> framebuffer. I guess it can fail, but it doesn't seem that different to
> the async plane update we already have and works well.
> 
In this case it can work, but by using generic drm api, it's hard to 
know what the driver will do.


>> Also how many drivers would need this ?
>>
>> Currently I was mostly considering x86 platform, so:
>>
>> simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
>>
>> i915/amdgpu/nouveau, which are quite complex, and will need to do their own
>> thing anyway.
> 
> I guess we're not entirely aligned there then. I would expect that
> mechanism to work with any atomic KMS driver. You are right that i915,
> amdgpu and nouveau are special enough that some extra internal plumbing
> is going to be required, but I'd expect it to be easy to support with
> any other driver for a memory-mapped device.
> 
> I guess what I'm trying to say is, even though it's totally fine that
> you only support those drivers at first, supporting in vc4 for example
> shouldn't require to rewrite the whole thing.

Would that work for you to put that in a drm_panic_helper.c,
so that drivers can opt-in ?

So the driver can call a drm_panic_helper_prepare_commit() at 
initialization, and then in the get_scanout_buffer() function, run the 
atomic_update() on it, and return this commit's framebuffer ?

That way each driver have a better control on what the panic handler 
will do.
It can even call directly its internal functions, to avoid the locks of 
the drm generic functions, and make sure it will only change the format 
and base address.
That's a bit more work for each driver, but should be more reliable I think.

-- 

Jocelyn

> 
>>>> I'm more in favor of an emergency function, that each driver has to
>>>> implement, and use what the hardware can do to display a simple frame
>>>> quickly. get_scanout_buffer() is a good start for simple driver, but
>>>> will need refactoring for the more complex case, like adding a
>>>> callback to write pixels one by one, if there is no memory mapped
>>>> buffer available.
>>>
>>> Sorry, I'm not quite sure what you mean there, where would you write the
>>> pixel to?
>>
>> It was mentioned by Noralf, about the amdgpu driver:
>>
>> https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
>>
>> They have a slow "debug interface" that you can write to, and can be used
>> for the panic handler. It's not memory mapped, so you have to write pixels
>> one by one.
>>
>> So for the struct drm_scanout_buffer, I can add a function pointer to a
>> write_pixel(u32 x, u32 y, u32 color)
>> So if the iosys map is null, it will use that instead.
> 
> It would be nice to support indeed, but it's a fairly rare feature afaik
> so I'd rather focus on getting something that can work for everyone first
> 
> Maxime

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

On Mon, Oct 09, 2023 at 04:05:19PM +0200, Jocelyn Falempe wrote:
> > > - I find it risky to completely reconfigure the hardware in a panic handler.
> > 
> > I would expect to only change the format and base address of the
> > framebuffer. I guess it can fail, but it doesn't seem that different to
> > the async plane update we already have and works well.
> > 
> In this case it can work, but by using generic drm api, it's hard to know
> what the driver will do.

We should document extensively what we expect drivers to do in those
hooks, and possibly call cant_sleep() in the framework function to have
some reporting at least.

> > > Also how many drivers would need this ?
> > > 
> > > Currently I was mostly considering x86 platform, so:
> > > 
> > > simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
> > > 
> > > i915/amdgpu/nouveau, which are quite complex, and will need to do their own
> > > thing anyway.
> > 
> > I guess we're not entirely aligned there then. I would expect that
> > mechanism to work with any atomic KMS driver. You are right that i915,
> > amdgpu and nouveau are special enough that some extra internal plumbing
> > is going to be required, but I'd expect it to be easy to support with
> > any other driver for a memory-mapped device.
> > 
> > I guess what I'm trying to say is, even though it's totally fine that
> > you only support those drivers at first, supporting in vc4 for example
> > shouldn't require to rewrite the whole thing.
> 
> Would that work for you to put that in a drm_panic_helper.c,
> so that drivers can opt-in ?
> 
> So the driver can call a drm_panic_helper_prepare_commit() at
> initialization, and then in the get_scanout_buffer() function

If we have a full blown commit with a new framebuffer, why do we need
get_scanout_buffer? It should be either the framebuffer itself, or in
the plane state if you have a conversion.

> run the atomic_update() on it, and return this commit's framebuffer ?
> 
> That way each driver have a better control on what the panic handler will
> do.
> It can even call directly its internal functions, to avoid the locks of the
> drm generic functions, and make sure it will only change the format and base
> address.
> That's a bit more work for each driver, but should be more reliable I think.

I don't think that better control there is a good idea, it's a path that
won't get tested much so we'd be better off not allowing drivers to
deviate too much from the "ideal" design.

What I had in mind is something like:

  - Add a panic hook in drm_mode_config_funcs, with a
    drm_atomic_helper_panic helper;

  - Provide an atomic_panic hook or something in drm_plane_helper_funcs;

  - If they are set, we register the drm_panic handler;

  - The handler will call drm_mode_config_funcs.panic, which will take
    its prepared state, fill the framebuffer it allocated with the
    penguin and backtrace, call drm_plane_helper_funcs.atomic_panic().

  - The driver now updates the format and fb address.

  - Halt and catch fire

Does that make sense?
Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 09/10/2023 18:07, Maxime Ripard wrote:
> On Mon, Oct 09, 2023 at 04:05:19PM +0200, Jocelyn Falempe wrote:
>>>> - I find it risky to completely reconfigure the hardware in a panic handler.
>>>
>>> I would expect to only change the format and base address of the
>>> framebuffer. I guess it can fail, but it doesn't seem that different to
>>> the async plane update we already have and works well.
>>>
>> In this case it can work, but by using generic drm api, it's hard to know
>> what the driver will do.
> 
> We should document extensively what we expect drivers to do in those
> hooks, and possibly call cant_sleep() in the framework function to have
> some reporting at least.
> 
>>>> Also how many drivers would need this ?
>>>>
>>>> Currently I was mostly considering x86 platform, so:
>>>>
>>>> simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
>>>>
>>>> i915/amdgpu/nouveau, which are quite complex, and will need to do their own
>>>> thing anyway.
>>>
>>> I guess we're not entirely aligned there then. I would expect that
>>> mechanism to work with any atomic KMS driver. You are right that i915,
>>> amdgpu and nouveau are special enough that some extra internal plumbing
>>> is going to be required, but I'd expect it to be easy to support with
>>> any other driver for a memory-mapped device.
>>>
>>> I guess what I'm trying to say is, even though it's totally fine that
>>> you only support those drivers at first, supporting in vc4 for example
>>> shouldn't require to rewrite the whole thing.
>>
>> Would that work for you to put that in a drm_panic_helper.c,
>> so that drivers can opt-in ?
>>
>> So the driver can call a drm_panic_helper_prepare_commit() at
>> initialization, and then in the get_scanout_buffer() function
> 
> If we have a full blown commit with a new framebuffer, why do we need
> get_scanout_buffer? It should be either the framebuffer itself, or in
> the plane state if you have a conversion.
> 
>> run the atomic_update() on it, and return this commit's framebuffer ?
>>
>> That way each driver have a better control on what the panic handler will
>> do.
>> It can even call directly its internal functions, to avoid the locks of the
>> drm generic functions, and make sure it will only change the format and base
>> address.
>> That's a bit more work for each driver, but should be more reliable I think.
> 
> I don't think that better control there is a good idea, it's a path that
> won't get tested much so we'd be better off not allowing drivers to
> deviate too much from the "ideal" design.
> 
> What I had in mind is something like:
> 
>    - Add a panic hook in drm_mode_config_funcs, with a
>      drm_atomic_helper_panic helper;
> 
>    - Provide an atomic_panic hook or something in drm_plane_helper_funcs;
> 
>    - If they are set, we register the drm_panic handler;
> 
>    - The handler will call drm_mode_config_funcs.panic, which will take
>      its prepared state, fill the framebuffer it allocated with the
>      penguin and backtrace, call drm_plane_helper_funcs.atomic_panic().
> 
>    - The driver now updates the format and fb address.
> 
>    - Halt and catch fire
> 
> Does that make sense?

Yes, I will do some experiment with that, and see if I can make it work 
this way.
If possible I still want to have a way for simple drivers like 
simpledrm/mgag200 to not allocate a full framebuffer.

> Maxime

-- 

Jocelyn

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

On Tue, Oct 10, 2023 at 09:55:46AM +0200, Jocelyn Falempe wrote:
> On 09/10/2023 18:07, Maxime Ripard wrote:
> > On Mon, Oct 09, 2023 at 04:05:19PM +0200, Jocelyn Falempe wrote:
> > > > > - I find it risky to completely reconfigure the hardware in a panic handler.
> > > > 
> > > > I would expect to only change the format and base address of the
> > > > framebuffer. I guess it can fail, but it doesn't seem that different to
> > > > the async plane update we already have and works well.
> > > > 
> > > In this case it can work, but by using generic drm api, it's hard to know
> > > what the driver will do.
> > 
> > We should document extensively what we expect drivers to do in those
> > hooks, and possibly call cant_sleep() in the framework function to have
> > some reporting at least.
> > 
> > > > > Also how many drivers would need this ?
> > > > > 
> > > > > Currently I was mostly considering x86 platform, so:
> > > > > 
> > > > > simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
> > > > > 
> > > > > i915/amdgpu/nouveau, which are quite complex, and will need to do their own
> > > > > thing anyway.
> > > > 
> > > > I guess we're not entirely aligned there then. I would expect that
> > > > mechanism to work with any atomic KMS driver. You are right that i915,
> > > > amdgpu and nouveau are special enough that some extra internal plumbing
> > > > is going to be required, but I'd expect it to be easy to support with
> > > > any other driver for a memory-mapped device.
> > > > 
> > > > I guess what I'm trying to say is, even though it's totally fine that
> > > > you only support those drivers at first, supporting in vc4 for example
> > > > shouldn't require to rewrite the whole thing.
> > > 
> > > Would that work for you to put that in a drm_panic_helper.c,
> > > so that drivers can opt-in ?
> > > 
> > > So the driver can call a drm_panic_helper_prepare_commit() at
> > > initialization, and then in the get_scanout_buffer() function
> > 
> > If we have a full blown commit with a new framebuffer, why do we need
> > get_scanout_buffer? It should be either the framebuffer itself, or in
> > the plane state if you have a conversion.
> > 
> > > run the atomic_update() on it, and return this commit's framebuffer ?
> > > 
> > > That way each driver have a better control on what the panic handler will
> > > do.
> > > It can even call directly its internal functions, to avoid the locks of the
> > > drm generic functions, and make sure it will only change the format and base
> > > address.
> > > That's a bit more work for each driver, but should be more reliable I think.
> > 
> > I don't think that better control there is a good idea, it's a path that
> > won't get tested much so we'd be better off not allowing drivers to
> > deviate too much from the "ideal" design.
> > 
> > What I had in mind is something like:
> > 
> >    - Add a panic hook in drm_mode_config_funcs, with a
> >      drm_atomic_helper_panic helper;
> > 
> >    - Provide an atomic_panic hook or something in drm_plane_helper_funcs;
> > 
> >    - If they are set, we register the drm_panic handler;
> > 
> >    - The handler will call drm_mode_config_funcs.panic, which will take
> >      its prepared state, fill the framebuffer it allocated with the
> >      penguin and backtrace, call drm_plane_helper_funcs.atomic_panic().
> > 
> >    - The driver now updates the format and fb address.
> > 
> >    - Halt and catch fire
> > 
> > Does that make sense?
> 
> Yes, I will do some experiment with that, and see if I can make it
> work this way.

Thanks :)

> If possible I still want to have a way for simple drivers like
> simpledrm/mgag200 to not allocate a full framebuffer.

I guess the split isn't going to be whether the driver is simple or not,
but whether it always has access to the buffer used by the scanout.

Like for simpledrm, we have that guarantee so it makes sense. For other,
if we allow "direct" dma-buf, it's game over and we just can't.

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4387 bytes --]

Hi

Am 09.10.23 um 13:33 schrieb Maxime Ripard:
[...]
>>> And you don't need to support all kinds of tiling, YUV or RGB variants.

We should indeed not use YUV at all. For RGB, we already have plenty of 
conversion code from XRGB8888-to-<whatever>, so we are more flexible there.

>>
>> So if I understand correctly, drm_panic would pre-allocate a plane/commit,
>> and use that when a panic occurs ?
> 
> And have it checked already, yes. We would only wait for a panic to
> happen to pull the trigger on the commit.
> 
>> I have two concern about this approach:
>> - How much memory would be allocated for this ? a whole framebuffer can be
>> big for just this use case.

As I outlined in my email at [1], there are a number of different 
scenarios. The question of atomic state and commits is entirely separate 
from the DRM panic handler. We should not throw them together. Whatever 
is necessary is get a scanout buffer, should happen on the driver side 
of get_scanout_buffer, not on the drm_panic side.

[1] 
https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4ac6@suse.de/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1

> 
> I'dd expect a whole framebuffer for the current
> configuration/resolution. It would be typically 4MB for a full-HD system
> which isn't a lot really and I guess we can always add an option to
> disable the mechanism if needed.
> 
>> - I find it risky to completely reconfigure the hardware in a panic handler.
> 
> I would expect to only change the format and base address of the
> framebuffer. I guess it can fail, but it doesn't seem that different to
> the async plane update we already have and works well.

The one thing I don't understand is: Why should we use atomic commits in 
the first place? It doesn't make sense for firmware-based drivers. In 
some drivers, even the simple ast, we hold locks during the regular 
commit. Trying to run the panic commit concurrently will likely give a 
deadlock.

In the end it's a per-driver decision, but in most cases, the driver can 
easily switch to a default mode with some ad-hoc code.

Best regards
Thomas

> 
>> Also how many drivers would need this ?
>>
>> Currently I was mostly considering x86 platform, so:
>>
>> simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
>>
>> i915/amdgpu/nouveau, which are quite complex, and will need to do their own
>> thing anyway.
> 
> I guess we're not entirely aligned there then. I would expect that
> mechanism to work with any atomic KMS driver. You are right that i915,
> amdgpu and nouveau are special enough that some extra internal plumbing
> is going to be required, but I'd expect it to be easy to support with
> any other driver for a memory-mapped device.
> 
> I guess what I'm trying to say is, even though it's totally fine that
> you only support those drivers at first, supporting in vc4 for example
> shouldn't require to rewrite the whole thing.
> 
>>>> I'm more in favor of an emergency function, that each driver has to
>>>> implement, and use what the hardware can do to display a simple frame
>>>> quickly. get_scanout_buffer() is a good start for simple driver, but
>>>> will need refactoring for the more complex case, like adding a
>>>> callback to write pixels one by one, if there is no memory mapped
>>>> buffer available.
>>>
>>> Sorry, I'm not quite sure what you mean there, where would you write the
>>> pixel to?
>>
>> It was mentioned by Noralf, about the amdgpu driver:
>>
>> https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313dac7@amd.com/
>>
>> They have a slow "debug interface" that you can write to, and can be used
>> for the panic handler. It's not memory mapped, so you have to write pixels
>> one by one.
>>
>> So for the struct drm_scanout_buffer, I can add a function pointer to a
>> write_pixel(u32 x, u32 y, u32 color)
>> So if the iosys map is null, it will use that instead.
> 
> It would be nice to support indeed, but it's a fairly rare feature afaik
> so I'd rather focus on getting something that can work for everyone first
> 
> Maxime

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4133 bytes --]

Hi

Am 09.10.23 um 18:07 schrieb Maxime Ripard:
> On Mon, Oct 09, 2023 at 04:05:19PM +0200, Jocelyn Falempe wrote:
>>>> - I find it risky to completely reconfigure the hardware in a panic handler.
>>>
>>> I would expect to only change the format and base address of the
>>> framebuffer. I guess it can fail, but it doesn't seem that different to
>>> the async plane update we already have and works well.
>>>
>> In this case it can work, but by using generic drm api, it's hard to know
>> what the driver will do.
> 
> We should document extensively what we expect drivers to do in those
> hooks, and possibly call cant_sleep() in the framework function to have
> some reporting at least.
> 
>>>> Also how many drivers would need this ?
>>>>
>>>> Currently I was mostly considering x86 platform, so:
>>>>
>>>> simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
>>>>
>>>> i915/amdgpu/nouveau, which are quite complex, and will need to do their own
>>>> thing anyway.
>>>
>>> I guess we're not entirely aligned there then. I would expect that
>>> mechanism to work with any atomic KMS driver. You are right that i915,
>>> amdgpu and nouveau are special enough that some extra internal plumbing
>>> is going to be required, but I'd expect it to be easy to support with
>>> any other driver for a memory-mapped device.
>>>
>>> I guess what I'm trying to say is, even though it's totally fine that
>>> you only support those drivers at first, supporting in vc4 for example
>>> shouldn't require to rewrite the whole thing.
>>
>> Would that work for you to put that in a drm_panic_helper.c,
>> so that drivers can opt-in ?
>>
>> So the driver can call a drm_panic_helper_prepare_commit() at
>> initialization, and then in the get_scanout_buffer() function
> 
> If we have a full blown commit with a new framebuffer, why do we need
> get_scanout_buffer? It should be either the framebuffer itself, or in
> the plane state if you have a conversion.

We also have discussions about kexec/kdump support. Here we'd need to 
retrieve the scanout address, forward it to the kexec kernel and put 
simpledrm onto that framebuffer until the regular driver takes over. An 
interface like get_scanout_buffer will be helpful for this use case. So 
it makes sense to use it for the panic handler as well.

> 
>> run the atomic_update() on it, and return this commit's framebuffer ?
>>
>> That way each driver have a better control on what the panic handler will
>> do.
>> It can even call directly its internal functions, to avoid the locks of the
>> drm generic functions, and make sure it will only change the format and base
>> address.
>> That's a bit more work for each driver, but should be more reliable I think.
> 
> I don't think that better control there is a good idea, it's a path that
> won't get tested much so we'd be better off not allowing drivers to
> deviate too much from the "ideal" design.
> 
> What I had in mind is something like:
> 
>    - Add a panic hook in drm_mode_config_funcs, with a
>      drm_atomic_helper_panic helper;
> 
>    - Provide an atomic_panic hook or something in drm_plane_helper_funcs;
> 
>    - If they are set, we register the drm_panic handler;
> 
>    - The handler will call drm_mode_config_funcs.panic, which will take
>      its prepared state, fill the framebuffer it allocated with the
>      penguin and backtrace, call drm_plane_helper_funcs.atomic_panic().
> 
>    - The driver now updates the format and fb address.
> 
>    - Halt and catch fire
> 
> Does that make sense?

Please see my other replies. I find this fragile, and unnecessary for 
cases where there already is a working scanout buffer in place. It's 
something a driver could implement internally to provide a scanout 
buffer if none has been set up already.

Best regards
Thomas

> Maxime

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 4/4] drm/mgag200: Add drm_panic support

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2434 bytes --]

Hi

Am 03.10.23 um 16:22 schrieb Jocelyn Falempe:
> Add support for the drm_panic module, which displays a message to
> the screen when a kernel panic occurs.
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/mgag200/mgag200_drv.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 976f0ab2006b..229d9c116b42 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -12,10 +12,12 @@
>   #include <drm/drm_aperture.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_fbdev_generic.h>
> +#include <drm/drm_framebuffer.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_ioctl.h>
>   #include <drm/drm_managed.h>
>   #include <drm/drm_module.h>
> +#include <drm/drm_panic.h>
>   #include <drm/drm_pciids.h>
>   
>   #include "mgag200_drv.h"
> @@ -83,6 +85,27 @@ resource_size_t mgag200_probe_vram(void __iomem *mem, resource_size_t size)
>   	return offset - 65536;
>   }
>   
> +static int mgag200_get_scanout_buffer(struct drm_device *dev,
> +				      struct drm_scanout_buffer *sb)
> +{
> +	struct drm_plane *plane;
> +	struct mga_device *mdev = to_mga_device(dev);
> +	struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
> +
> +	/* mgag200 has only one plane */
> +	drm_for_each_plane(plane, dev) {
> +		if (!plane->state || !plane->state->fb)

Better test for plane->state->visible. You should also check if it's a 
primary plane.

Best regards
Thomas

> +			return -ENODEV;
> +		sb->format = plane->state->fb->format;
> +		sb->width = plane->state->fb->width;
> +		sb->height = plane->state->fb->height;
> +		sb->pitch = plane->state->fb->pitches[0];
> +		sb->map = map;
> +		return 0;
> +	}
> +	return -ENODEV;
> +}
> +
>   /*
>    * DRM driver
>    */
> @@ -98,6 +121,7 @@ static const struct drm_driver mgag200_driver = {
>   	.major = DRIVER_MAJOR,
>   	.minor = DRIVER_MINOR,
>   	.patchlevel = DRIVER_PATCHLEVEL,
> +	.get_scanout_buffer = mgag200_get_scanout_buffer,
>   	DRM_GEM_SHMEM_DRIVER_OPS,
>   };
>   

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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



On Tue, Oct 10, 2023 at 10:55:09AM +0200, Thomas Zimmermann wrote:
> > > So if I understand correctly, drm_panic would pre-allocate a plane/commit,
> > > and use that when a panic occurs ?
> > 
> > And have it checked already, yes. We would only wait for a panic to
> > happen to pull the trigger on the commit.
> > 
> > > I have two concern about this approach:
> > > - How much memory would be allocated for this ? a whole framebuffer can be
> > > big for just this use case.
> 
> As I outlined in my email at [1], there are a number of different scenarios.
> The question of atomic state and commits is entirely separate from the DRM
> panic handler. We should not throw them together. Whatever is necessary is
> get a scanout buffer, should happen on the driver side of
> get_scanout_buffer, not on the drm_panic side.
> 
> [1] https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4ac6@suse.de/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1
> 
> > 
> > I'dd expect a whole framebuffer for the current
> > configuration/resolution. It would be typically 4MB for a full-HD system
> > which isn't a lot really and I guess we can always add an option to
> > disable the mechanism if needed.
> > 
> > > - I find it risky to completely reconfigure the hardware in a panic handler.
> > 
> > I would expect to only change the format and base address of the
> > framebuffer. I guess it can fail, but it doesn't seem that different to
> > the async plane update we already have and works well.
> 
> The one thing I don't understand is: Why should we use atomic commits in the
> first place? It doesn't make sense for firmware-based drivers.

Because this is generic infrastructure that is valuable for any drivers
and not only firmware-based drivers?

> In some drivers, even the simple ast, we hold locks during the regular
> commit. Trying to run the panic commit concurrently will likely give a
> deadlock.

You're in the middle of a panic. Don't take any locks and you won't deadlock.

> In the end it's a per-driver decision, but in most cases, the driver can
> easily switch to a default mode with some ad-hoc code.

When was the last time a per-driver decision has been a good thing? I'm
sorry, but the get_scanout_buffer approach buffer won't work for any
driver out there.

I'm fine with discussing alternatives if you don't like the ones I
suggested, but they must allow the panic handler infrastructure to work
with any driver we have, not just 4.

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

On Tue, Oct 10, 2023 at 11:04:33AM +0200, Thomas Zimmermann wrote:
> Am 09.10.23 um 18:07 schrieb Maxime Ripard:
> > On Mon, Oct 09, 2023 at 04:05:19PM +0200, Jocelyn Falempe wrote:
> > > > > - I find it risky to completely reconfigure the hardware in a panic handler.
> > > > 
> > > > I would expect to only change the format and base address of the
> > > > framebuffer. I guess it can fail, but it doesn't seem that different to
> > > > the async plane update we already have and works well.
> > > > 
> > > In this case it can work, but by using generic drm api, it's hard to know
> > > what the driver will do.
> > 
> > We should document extensively what we expect drivers to do in those
> > hooks, and possibly call cant_sleep() in the framework function to have
> > some reporting at least.
> > 
> > > > > Also how many drivers would need this ?
> > > > > 
> > > > > Currently I was mostly considering x86 platform, so:
> > > > > 
> > > > > simpledrm/ast/mgag200 which works well with the get_scanout_buffer().
> > > > > 
> > > > > i915/amdgpu/nouveau, which are quite complex, and will need to do their own
> > > > > thing anyway.
> > > > 
> > > > I guess we're not entirely aligned there then. I would expect that
> > > > mechanism to work with any atomic KMS driver. You are right that i915,
> > > > amdgpu and nouveau are special enough that some extra internal plumbing
> > > > is going to be required, but I'd expect it to be easy to support with
> > > > any other driver for a memory-mapped device.
> > > > 
> > > > I guess what I'm trying to say is, even though it's totally fine that
> > > > you only support those drivers at first, supporting in vc4 for example
> > > > shouldn't require to rewrite the whole thing.
> > > 
> > > Would that work for you to put that in a drm_panic_helper.c,
> > > so that drivers can opt-in ?
> > > 
> > > So the driver can call a drm_panic_helper_prepare_commit() at
> > > initialization, and then in the get_scanout_buffer() function
> > 
> > If we have a full blown commit with a new framebuffer, why do we need
> > get_scanout_buffer? It should be either the framebuffer itself, or in
> > the plane state if you have a conversion.
> 
> We also have discussions about kexec/kdump support. Here we'd need to
> retrieve the scanout address, forward it to the kexec kernel and put
> simpledrm onto that framebuffer until the regular driver takes over.

Generically speaking, there's strictly no guarantee that the current
scanout buffer is accessible by the CPU. It's not even a driver issue,
it's a workload issue, so it will affect 100% of the times some users,
and some 0% of the time.

> An interface like get_scanout_buffer will be helpful for this use
> case. So it makes sense to use it for the panic handler as well.

It won't be helpful because the vast majority of the ARM drivers (and
thus the vast majority of the KMS drivers) won't be able to implement it
properly and reliably.

> > > run the atomic_update() on it, and return this commit's framebuffer ?
> > > 
> > > That way each driver have a better control on what the panic handler will
> > > do.
> > > It can even call directly its internal functions, to avoid the locks of the
> > > drm generic functions, and make sure it will only change the format and base
> > > address.
> > > That's a bit more work for each driver, but should be more reliable I think.
> > 
> > I don't think that better control there is a good idea, it's a path that
> > won't get tested much so we'd be better off not allowing drivers to
> > deviate too much from the "ideal" design.
> > 
> > What I had in mind is something like:
> > 
> >    - Add a panic hook in drm_mode_config_funcs, with a
> >      drm_atomic_helper_panic helper;
> > 
> >    - Provide an atomic_panic hook or something in drm_plane_helper_funcs;
> > 
> >    - If they are set, we register the drm_panic handler;
> > 
> >    - The handler will call drm_mode_config_funcs.panic, which will take
> >      its prepared state, fill the framebuffer it allocated with the
> >      penguin and backtrace, call drm_plane_helper_funcs.atomic_panic().
> > 
> >    - The driver now updates the format and fb address.
> > 
> >    - Halt and catch fire
> > 
> > Does that make sense?
> 
> Please see my other replies. I find this fragile, and unnecessary for cases
> where there already is a working scanout buffer in place.

And please see my other replies. Depending on a working scanout buffer
in place being accessible by the CPU is incredibly limiting. Ignoring it
when I'm pointing that out won't get us to a solution acceptable for
everyone.

> It's something a driver could implement internally to provide a
> scanout buffer if none has been set up already.

Some drivers need extra resources when setting up a plane. VC4 for
example need for every plane to allocate multiple internal SRAM buffers.
Just allocating an extra framebuffer won't cut it.

This is tied to the state so far, so I guess we would need to allocate a
new state. Oh, and if we have several drivers that need to allocate that
extra framebuffer and state, we could make it part of the core or turn
it into a helper?

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Noralf Trønnes]

On 10/10/23 11:25, Maxime Ripard wrote:
> 
> 
> On Tue, Oct 10, 2023 at 10:55:09AM +0200, Thomas Zimmermann wrote:
>>>> So if I understand correctly, drm_panic would pre-allocate a plane/commit,
>>>> and use that when a panic occurs ?
>>>
>>> And have it checked already, yes. We would only wait for a panic to
>>> happen to pull the trigger on the commit.
>>>
>>>> I have two concern about this approach:
>>>> - How much memory would be allocated for this ? a whole framebuffer can be
>>>> big for just this use case.
>>
>> As I outlined in my email at [1], there are a number of different scenarios.
>> The question of atomic state and commits is entirely separate from the DRM
>> panic handler. We should not throw them together. Whatever is necessary is
>> get a scanout buffer, should happen on the driver side of
>> get_scanout_buffer, not on the drm_panic side.
>>
>> [1] https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4ac6@suse.de/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1
>>
>>>
>>> I'dd expect a whole framebuffer for the current
>>> configuration/resolution. It would be typically 4MB for a full-HD system
>>> which isn't a lot really and I guess we can always add an option to
>>> disable the mechanism if needed.
>>>
>>>> - I find it risky to completely reconfigure the hardware in a panic handler.
>>>
>>> I would expect to only change the format and base address of the
>>> framebuffer. I guess it can fail, but it doesn't seem that different to
>>> the async plane update we already have and works well.
>>
>> The one thing I don't understand is: Why should we use atomic commits in the
>> first place? It doesn't make sense for firmware-based drivers.
> 
> Because this is generic infrastructure that is valuable for any drivers
> and not only firmware-based drivers?
> 
>> In some drivers, even the simple ast, we hold locks during the regular
>> commit. Trying to run the panic commit concurrently will likely give a
>> deadlock.
> 
> You're in the middle of a panic. Don't take any locks and you won't deadlock.
> 
>> In the end it's a per-driver decision, but in most cases, the driver can
>> easily switch to a default mode with some ad-hoc code.
> 
> When was the last time a per-driver decision has been a good thing? I'm
> sorry, but the get_scanout_buffer approach buffer won't work for any
> driver out there.
> 
> I'm fine with discussing alternatives if you don't like the ones I
> suggested, but they must allow the panic handler infrastructure to work
> with any driver we have, not just 4.
> 

Why can't we use the model[1] suggested by Daniel using a draw_pixel
callback giving drivers full control on how they can put a pixel on the
display?

This will even work for the AMD debug interface.
In the linear CPU accessible buffer case, we can provide a helper for
that, maybe we can do helpers for other common cases as well.

Adding to that we would need a panic_setup/enter and panic_teardown/exit
callback.

Noralf.

[1]
https://lore.kernel.org/dri-devel/20160810091529.GQ6232@phenom.ffwll.local/

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Maxime Ripard]

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

On Tue, Oct 10, 2023 at 01:29:52PM +0200, Noralf Trønnes wrote:
> 
> 
> On 10/10/23 11:25, Maxime Ripard wrote:
> > 
> > 
> > On Tue, Oct 10, 2023 at 10:55:09AM +0200, Thomas Zimmermann wrote:
> >>>> So if I understand correctly, drm_panic would pre-allocate a plane/commit,
> >>>> and use that when a panic occurs ?
> >>>
> >>> And have it checked already, yes. We would only wait for a panic to
> >>> happen to pull the trigger on the commit.
> >>>
> >>>> I have two concern about this approach:
> >>>> - How much memory would be allocated for this ? a whole framebuffer can be
> >>>> big for just this use case.
> >>
> >> As I outlined in my email at [1], there are a number of different scenarios.
> >> The question of atomic state and commits is entirely separate from the DRM
> >> panic handler. We should not throw them together. Whatever is necessary is
> >> get a scanout buffer, should happen on the driver side of
> >> get_scanout_buffer, not on the drm_panic side.
> >>
> >> [1] https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4ac6@suse.de/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1
> >>
> >>>
> >>> I'dd expect a whole framebuffer for the current
> >>> configuration/resolution. It would be typically 4MB for a full-HD system
> >>> which isn't a lot really and I guess we can always add an option to
> >>> disable the mechanism if needed.
> >>>
> >>>> - I find it risky to completely reconfigure the hardware in a panic handler.
> >>>
> >>> I would expect to only change the format and base address of the
> >>> framebuffer. I guess it can fail, but it doesn't seem that different to
> >>> the async plane update we already have and works well.
> >>
> >> The one thing I don't understand is: Why should we use atomic commits in the
> >> first place? It doesn't make sense for firmware-based drivers.
> > 
> > Because this is generic infrastructure that is valuable for any drivers
> > and not only firmware-based drivers?
> > 
> >> In some drivers, even the simple ast, we hold locks during the regular
> >> commit. Trying to run the panic commit concurrently will likely give a
> >> deadlock.
> > 
> > You're in the middle of a panic. Don't take any locks and you won't deadlock.
> > 
> >> In the end it's a per-driver decision, but in most cases, the driver can
> >> easily switch to a default mode with some ad-hoc code.
> > 
> > When was the last time a per-driver decision has been a good thing? I'm
> > sorry, but the get_scanout_buffer approach buffer won't work for any
> > driver out there.
> > 
> > I'm fine with discussing alternatives if you don't like the ones I
> > suggested, but they must allow the panic handler infrastructure to work
> > with any driver we have, not just 4.
> > 
> 
> Why can't we use the model[1] suggested by Daniel using a draw_pixel
> callback giving drivers full control on how they can put a pixel on the
> display?

I share kind of the same general ideas/conclusions: "qthe idea is that
all the fb selection and lookup is handled in shared code (and with
proper locking, but only for atomic drivers)."

2016 is a bit old though and multiple developments happened since
(secure playback is a thing now, framebuffer compression too), so I
still think that their expectation that the framebuffer is accessible to
/ writable by the CPU no longer holds true.

> This will even work for the AMD debug interface.
> In the linear CPU accessible buffer case, we can provide a helper for
> that, maybe we can do helpers for other common cases as well.

Yeah, their idea of a panic_draw would work great for that.

> Adding to that we would need a panic_setup/enter and panic_teardown/exit
> callback.

What for?

Maxime

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

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Daniel Vetter]

On Tue, Oct 10, 2023 at 02:15:47PM +0200, Maxime Ripard wrote:
> On Tue, Oct 10, 2023 at 01:29:52PM +0200, Noralf Trønnes wrote:
> > 
> > 
> > On 10/10/23 11:25, Maxime Ripard wrote:
> > > 
> > > 
> > > On Tue, Oct 10, 2023 at 10:55:09AM +0200, Thomas Zimmermann wrote:
> > >>>> So if I understand correctly, drm_panic would pre-allocate a plane/commit,
> > >>>> and use that when a panic occurs ?
> > >>>
> > >>> And have it checked already, yes. We would only wait for a panic to
> > >>> happen to pull the trigger on the commit.
> > >>>
> > >>>> I have two concern about this approach:
> > >>>> - How much memory would be allocated for this ? a whole framebuffer can be
> > >>>> big for just this use case.
> > >>
> > >> As I outlined in my email at [1], there are a number of different scenarios.
> > >> The question of atomic state and commits is entirely separate from the DRM
> > >> panic handler. We should not throw them together. Whatever is necessary is
> > >> get a scanout buffer, should happen on the driver side of
> > >> get_scanout_buffer, not on the drm_panic side.
> > >>
> > >> [1] https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4ac6@suse.de/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1
> > >>
> > >>>
> > >>> I'dd expect a whole framebuffer for the current
> > >>> configuration/resolution. It would be typically 4MB for a full-HD system
> > >>> which isn't a lot really and I guess we can always add an option to
> > >>> disable the mechanism if needed.
> > >>>
> > >>>> - I find it risky to completely reconfigure the hardware in a panic handler.
> > >>>
> > >>> I would expect to only change the format and base address of the
> > >>> framebuffer. I guess it can fail, but it doesn't seem that different to
> > >>> the async plane update we already have and works well.
> > >>
> > >> The one thing I don't understand is: Why should we use atomic commits in the
> > >> first place? It doesn't make sense for firmware-based drivers.
> > > 
> > > Because this is generic infrastructure that is valuable for any drivers
> > > and not only firmware-based drivers?
> > > 
> > >> In some drivers, even the simple ast, we hold locks during the regular
> > >> commit. Trying to run the panic commit concurrently will likely give a
> > >> deadlock.
> > > 
> > > You're in the middle of a panic. Don't take any locks and you won't deadlock.
> > > 
> > >> In the end it's a per-driver decision, but in most cases, the driver can
> > >> easily switch to a default mode with some ad-hoc code.
> > > 
> > > When was the last time a per-driver decision has been a good thing? I'm
> > > sorry, but the get_scanout_buffer approach buffer won't work for any
> > > driver out there.
> > > 
> > > I'm fine with discussing alternatives if you don't like the ones I
> > > suggested, but they must allow the panic handler infrastructure to work
> > > with any driver we have, not just 4.
> > > 
> > 
> > Why can't we use the model[1] suggested by Daniel using a draw_pixel
> > callback giving drivers full control on how they can put a pixel on the
> > display?
> 
> I share kind of the same general ideas/conclusions: "qthe idea is that
> all the fb selection and lookup is handled in shared code (and with
> proper locking, but only for atomic drivers)."
> 
> 2016 is a bit old though and multiple developments happened since
> (secure playback is a thing now, framebuffer compression too), so I
> still think that their expectation that the framebuffer is accessible to
> / writable by the CPU no longer holds true.

I think largely it should still be ok, because the idea behind the draw_xy
callback was that the driver could take care of anything special, like
- tiling
- clearing compression bits so that just writing the raw pixels works (if
  your compression format allows for that)
- handling any differences in how the pixels need to be written, like
  cache flushing, mmio_write vs normal memory, amd also has peek/poke
  registers to be able to write even into unmappable memory

What would probably be a good idea is to do an s/void */uinptr_t/ over my
interface proposal, or maybe an even more opaque cookie since really the
only thing you can do is get the void * that ->panic_vmap returns and pass
it into ->panic_draw_xy.

I thought (but I didn't dig through details) that the panic fb struct is
essentially meant to serve this purpose in the current series?

> > This will even work for the AMD debug interface.
> > In the linear CPU accessible buffer case, we can provide a helper for
> > that, maybe we can do helpers for other common cases as well.
> 
> Yeah, their idea of a panic_draw would work great for that.
> 
> > Adding to that we would need a panic_setup/enter and panic_teardown/exit
> > callback.
> 
> What for?

So panic teardown would be for testing in CI, to make it non-destructive
and clean up anything panic_vmap (or _enter or whatever you call it) has
done. I thought John Oggness was also looking into how the new panic
handlers/consoles could be made testable in the panic paths, including
lock stealing and getting called from nmi.
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4934 bytes --]

Hi

Am 10.10.23 um 11:33 schrieb Maxime Ripard:
[...]
>> We also have discussions about kexec/kdump support. Here we'd need to
>> retrieve the scanout address, forward it to the kexec kernel and put
>> simpledrm onto that framebuffer until the regular driver takes over.
> 
> Generically speaking, there's strictly no guarantee that the current
> scanout buffer is accessible by the CPU. It's not even a driver issue,
> it's a workload issue, so it will affect 100% of the times some users,
> and some 0% of the time.

And I'd be OK with that. It's all best effort anyway.

> 
>> An interface like get_scanout_buffer will be helpful for this use
>> case. So it makes sense to use it for the panic handler as well.
> 
> It won't be helpful because the vast majority of the ARM drivers (and
> thus the vast majority of the KMS drivers) won't be able to implement it
> properly and reliably.

The barrier for firmware-based drivers is low: a pre-configured display 
and mmap'able framebuffer memory. And it's assumed that a callback for 
kexec would attempt to configure hardware accordingly. If a system 
really cannot provide this, so be it.

> 
>>>> run the atomic_update() on it, and return this commit's framebuffer ?
>>>>
>>>> That way each driver have a better control on what the panic handler will
>>>> do.
>>>> It can even call directly its internal functions, to avoid the locks of the
>>>> drm generic functions, and make sure it will only change the format and base
>>>> address.
>>>> That's a bit more work for each driver, but should be more reliable I think.
>>>
>>> I don't think that better control there is a good idea, it's a path that
>>> won't get tested much so we'd be better off not allowing drivers to
>>> deviate too much from the "ideal" design.
>>>
>>> What I had in mind is something like:
>>>
>>>     - Add a panic hook in drm_mode_config_funcs, with a
>>>       drm_atomic_helper_panic helper;
>>>
>>>     - Provide an atomic_panic hook or something in drm_plane_helper_funcs;
>>>
>>>     - If they are set, we register the drm_panic handler;
>>>
>>>     - The handler will call drm_mode_config_funcs.panic, which will take
>>>       its prepared state, fill the framebuffer it allocated with the
>>>       penguin and backtrace, call drm_plane_helper_funcs.atomic_panic().
>>>
>>>     - The driver now updates the format and fb address.
>>>
>>>     - Halt and catch fire
>>>
>>> Does that make sense?
>>
>> Please see my other replies. I find this fragile, and unnecessary for cases
>> where there already is a working scanout buffer in place.
> 
> And please see my other replies. Depending on a working scanout buffer
> in place being accessible by the CPU is incredibly limiting. Ignoring it
> when I'm pointing that out won't get us to a solution acceptable for
> everyone.
> 
>> It's something a driver could implement internally to provide a
>> scanout buffer if none has been set up already.
> 
> Some drivers need extra resources when setting up a plane. VC4 for
> example need for every plane to allocate multiple internal SRAM buffers.
> Just allocating an extra framebuffer won't cut it.
> 
> This is tied to the state so far, so I guess we would need to allocate a
> new state. Oh, and if we have several drivers that need to allocate that
> extra framebuffer and state, we could make it part of the core or turn
> it into a helper?

I mentioned that even the simple drivers hold locks during the atomic 
commit. Some of the drivers use vmap/vunmap, which might make problems 
as well. It's used in the context of damage handling, which also makes 
no sense here. So the regular atomic helpers most likely won't work.

It sounds to me as if you're essentially asking for something like a 
flush or sync operation.

Stepping back from get_scanout_buffer for a moment and adopting your 
proposal from above, I can see something like this working:

   - the driver provides a panic callback in struct drm_driver

   - DRM registers a panic handler to invokes the callback

   - the panic callback has a scanout buffer from somewhere (currently 
active, prepared, from firmware, plain memory, etc)

   - we provide a helper that fills the scanout buffer with information

   - the driver then updates the hardware from/with the scanout buffer; 
details depend on the hardware

The final step is like a flush or commit operation. To give some 
examples: The simple drivers of this patchset probably don't have to do 
anything. Drivers with command/packet queues could attempt to send the 
necessary commands to the device.

Best regards
Thomas

> 
> Maxime

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3165 bytes --]

Hi

Am 10.10.23 um 14:59 schrieb Daniel Vetter:
[...]
>>>>
>>>
>>> Why can't we use the model[1] suggested by Daniel using a draw_pixel
>>> callback giving drivers full control on how they can put a pixel on the
>>> display?
>>
>> I share kind of the same general ideas/conclusions: "qthe idea is that
>> all the fb selection and lookup is handled in shared code (and with
>> proper locking, but only for atomic drivers)."
>>
>> 2016 is a bit old though and multiple developments happened since
>> (secure playback is a thing now, framebuffer compression too), so I
>> still think that their expectation that the framebuffer is accessible to
>> / writable by the CPU no longer holds true.
> 
> I think largely it should still be ok, because the idea behind the draw_xy
> callback was that the driver could take care of anything special, like
> - tiling
> - clearing compression bits so that just writing the raw pixels works (if
>    your compression format allows for that)
> - handling any differences in how the pixels need to be written, like
>    cache flushing, mmio_write vs normal memory, amd also has peek/poke
>    registers to be able to write even into unmappable memory
> 
> What would probably be a good idea is to do an s/void */uinptr_t/ over my
> interface proposal, or maybe an even more opaque cookie since really the
> only thing you can do is get the void * that ->panic_vmap returns and pass
> it into ->panic_draw_xy.
> 
> I thought (but I didn't dig through details) that the panic fb struct is
> essentially meant to serve this purpose in the current series?

I have one detail about the code: While working on the format-conversion 
code, I've always found struct drm_framebuffer to be clunky to work 
with. It's build around the idea of managing GEM buffers, but not 
accessing them.

So I've been thinking about something like a drm_pixmap that contains 
size, color format and data pointers in one place. Reads and writes 
would be callbacks. It could abstract access to any kind of buffer. 
IIRC Jocelyn had something like that in the very first patchset. or 
maybe fb_pixmap could be repurposed.

Best regards
Thomas

> 
>>> This will even work for the AMD debug interface.
>>> In the linear CPU accessible buffer case, we can provide a helper for
>>> that, maybe we can do helpers for other common cases as well.
>>
>> Yeah, their idea of a panic_draw would work great for that.
>>
>>> Adding to that we would need a panic_setup/enter and panic_teardown/exit
>>> callback.
>>
>> What for?
> 
> So panic teardown would be for testing in CI, to make it non-destructive
> and clean up anything panic_vmap (or _enter or whatever you call it) has
> done. I thought John Oggness was also looking into how the new panic
> handlers/consoles could be made testable in the panic paths, including
> lock stealing and getting called from nmi.
> -Sima

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 10/10/2023 14:59, Daniel Vetter wrote:
> On Tue, Oct 10, 2023 at 02:15:47PM +0200, Maxime Ripard wrote:
>> On Tue, Oct 10, 2023 at 01:29:52PM +0200, Noralf Trønnes wrote:
>>>
>>>
>>> On 10/10/23 11:25, Maxime Ripard wrote:
>>>>
>>>>
>>>> On Tue, Oct 10, 2023 at 10:55:09AM +0200, Thomas Zimmermann wrote:
>>>>>>> So if I understand correctly, drm_panic would pre-allocate a plane/commit,
>>>>>>> and use that when a panic occurs ?
>>>>>>
>>>>>> And have it checked already, yes. We would only wait for a panic to
>>>>>> happen to pull the trigger on the commit.
>>>>>>
>>>>>>> I have two concern about this approach:
>>>>>>> - How much memory would be allocated for this ? a whole framebuffer can be
>>>>>>> big for just this use case.
>>>>>
>>>>> As I outlined in my email at [1], there are a number of different scenarios.
>>>>> The question of atomic state and commits is entirely separate from the DRM
>>>>> panic handler. We should not throw them together. Whatever is necessary is
>>>>> get a scanout buffer, should happen on the driver side of
>>>>> get_scanout_buffer, not on the drm_panic side.
>>>>>
>>>>> [1] https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4ac6@suse.de/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1
>>>>>
>>>>>>
>>>>>> I'dd expect a whole framebuffer for the current
>>>>>> configuration/resolution. It would be typically 4MB for a full-HD system
>>>>>> which isn't a lot really and I guess we can always add an option to
>>>>>> disable the mechanism if needed.
>>>>>>
>>>>>>> - I find it risky to completely reconfigure the hardware in a panic handler.
>>>>>>
>>>>>> I would expect to only change the format and base address of the
>>>>>> framebuffer. I guess it can fail, but it doesn't seem that different to
>>>>>> the async plane update we already have and works well.
>>>>>
>>>>> The one thing I don't understand is: Why should we use atomic commits in the
>>>>> first place? It doesn't make sense for firmware-based drivers.
>>>>
>>>> Because this is generic infrastructure that is valuable for any drivers
>>>> and not only firmware-based drivers?
>>>>
>>>>> In some drivers, even the simple ast, we hold locks during the regular
>>>>> commit. Trying to run the panic commit concurrently will likely give a
>>>>> deadlock.
>>>>
>>>> You're in the middle of a panic. Don't take any locks and you won't deadlock.
>>>>
>>>>> In the end it's a per-driver decision, but in most cases, the driver can
>>>>> easily switch to a default mode with some ad-hoc code.
>>>>
>>>> When was the last time a per-driver decision has been a good thing? I'm
>>>> sorry, but the get_scanout_buffer approach buffer won't work for any
>>>> driver out there.
>>>>
>>>> I'm fine with discussing alternatives if you don't like the ones I
>>>> suggested, but they must allow the panic handler infrastructure to work
>>>> with any driver we have, not just 4.
>>>>
>>>
>>> Why can't we use the model[1] suggested by Daniel using a draw_pixel
>>> callback giving drivers full control on how they can put a pixel on the
>>> display?
>>
>> I share kind of the same general ideas/conclusions: "qthe idea is that
>> all the fb selection and lookup is handled in shared code (and with
>> proper locking, but only for atomic drivers)."
>>
>> 2016 is a bit old though and multiple developments happened since
>> (secure playback is a thing now, framebuffer compression too), so I
>> still think that their expectation that the framebuffer is accessible to
>> / writable by the CPU no longer holds true.
> 
> I think largely it should still be ok, because the idea behind the draw_xy
> callback was that the driver could take care of anything special, like
> - tiling
> - clearing compression bits so that just writing the raw pixels works (if
>    your compression format allows for that)
> - handling any differences in how the pixels need to be written, like
>    cache flushing, mmio_write vs normal memory, amd also has peek/poke
>    registers to be able to write even into unmappable memory
> 
> What would probably be a good idea is to do an s/void */uinptr_t/ over my
> interface proposal, or maybe an even more opaque cookie since really the
> only thing you can do is get the void * that ->panic_vmap returns and pass
> it into ->panic_draw_xy.
> 
> I thought (but I didn't dig through details) that the panic fb struct is
> essentially meant to serve this purpose in the current series?
> 

Yes, in this series there is a struct drm_scanout_buffer, that the 
driver has to provide when a panic occurs:

struct drm_scanout_buffer {
	const struct drm_format_info *format;
	struct iosys_map map;
	unsigned int pitch;
	unsigned int width;
	unsigned int height;
};

That works well for CPU accessible, linear format.
It should be possible to support YUV, or simple tiling with that, but 
for the more complex case, I proposed to add a draw_pixel() callback.

>>> This will even work for the AMD debug interface.
>>> In the linear CPU accessible buffer case, we can provide a helper for
>>> that, maybe we can do helpers for other common cases as well.
>>
>> Yeah, their idea of a panic_draw would work great for that.
>>
>>> Adding to that we would need a panic_setup/enter and panic_teardown/exit
>>> callback.
>>
>> What for?
> 
> So panic teardown would be for testing in CI, to make it non-destructive
> and clean up anything panic_vmap (or _enter or whatever you call it) has
> done. I thought John Oggness was also looking into how the new panic
> handlers/consoles could be made testable in the panic paths, including
> lock stealing and getting called from nmi.

Thanks, I was also wondering why a panic teardown would be necessary, 
since after the panic handler has run, the system will probably halt. 
(or maybe run kdump and reboot).

> -Sima

Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 10/10/2023 15:05, Thomas Zimmermann wrote:
> Hi
> 
> Am 10.10.23 um 11:33 schrieb Maxime Ripard:
> [...]
>>> We also have discussions about kexec/kdump support. Here we'd need to
>>> retrieve the scanout address, forward it to the kexec kernel and put
>>> simpledrm onto that framebuffer until the regular driver takes over.
>>
>> Generically speaking, there's strictly no guarantee that the current
>> scanout buffer is accessible by the CPU. It's not even a driver issue,
>> it's a workload issue, so it will affect 100% of the times some users,
>> and some 0% of the time.
> 
> And I'd be OK with that. It's all best effort anyway.
> 
>>
>>> An interface like get_scanout_buffer will be helpful for this use
>>> case. So it makes sense to use it for the panic handler as well.
>>
>> It won't be helpful because the vast majority of the ARM drivers (and
>> thus the vast majority of the KMS drivers) won't be able to implement it
>> properly and reliably.
> 
> The barrier for firmware-based drivers is low: a pre-configured display 
> and mmap'able framebuffer memory. And it's assumed that a callback for 
> kexec would attempt to configure hardware accordingly. If a system 
> really cannot provide this, so be it.
> 
>>
>>>>> run the atomic_update() on it, and return this commit's framebuffer ?
>>>>>
>>>>> That way each driver have a better control on what the panic 
>>>>> handler will
>>>>> do.
>>>>> It can even call directly its internal functions, to avoid the 
>>>>> locks of the
>>>>> drm generic functions, and make sure it will only change the format 
>>>>> and base
>>>>> address.
>>>>> That's a bit more work for each driver, but should be more reliable 
>>>>> I think.
>>>>
>>>> I don't think that better control there is a good idea, it's a path 
>>>> that
>>>> won't get tested much so we'd be better off not allowing drivers to
>>>> deviate too much from the "ideal" design.
>>>>
>>>> What I had in mind is something like:
>>>>
>>>>     - Add a panic hook in drm_mode_config_funcs, with a
>>>>       drm_atomic_helper_panic helper;
>>>>
>>>>     - Provide an atomic_panic hook or something in 
>>>> drm_plane_helper_funcs;
>>>>
>>>>     - If they are set, we register the drm_panic handler;
>>>>
>>>>     - The handler will call drm_mode_config_funcs.panic, which will 
>>>> take
>>>>       its prepared state, fill the framebuffer it allocated with the
>>>>       penguin and backtrace, call 
>>>> drm_plane_helper_funcs.atomic_panic().
>>>>
>>>>     - The driver now updates the format and fb address.
>>>>
>>>>     - Halt and catch fire
>>>>
>>>> Does that make sense?
>>>
>>> Please see my other replies. I find this fragile, and unnecessary for 
>>> cases
>>> where there already is a working scanout buffer in place.
>>
>> And please see my other replies. Depending on a working scanout buffer
>> in place being accessible by the CPU is incredibly limiting. Ignoring it
>> when I'm pointing that out won't get us to a solution acceptable for
>> everyone.
>>
>>> It's something a driver could implement internally to provide a
>>> scanout buffer if none has been set up already.
>>
>> Some drivers need extra resources when setting up a plane. VC4 for
>> example need for every plane to allocate multiple internal SRAM buffers.
>> Just allocating an extra framebuffer won't cut it.
>>
>> This is tied to the state so far, so I guess we would need to allocate a
>> new state. Oh, and if we have several drivers that need to allocate that
>> extra framebuffer and state, we could make it part of the core or turn
>> it into a helper?
> 
> I mentioned that even the simple drivers hold locks during the atomic 
> commit. Some of the drivers use vmap/vunmap, which might make problems 
> as well. It's used in the context of damage handling, which also makes 
> no sense here. So the regular atomic helpers most likely won't work.
> 
> It sounds to me as if you're essentially asking for something like a 
> flush or sync operation.
> 
> Stepping back from get_scanout_buffer for a moment and adopting your 
> proposal from above, I can see something like this working:
> 
>    - the driver provides a panic callback in struct drm_driver
> 
>    - DRM registers a panic handler to invokes the callback
> 
>    - the panic callback has a scanout buffer from somewhere (currently 
> active, prepared, from firmware, plain memory, etc)
> 
>    - we provide a helper that fills the scanout buffer with information
> 
>    - the driver then updates the hardware from/with the scanout buffer; 
> details depend on the hardware
> 
> The final step is like a flush or commit operation. To give some 
> examples: The simple drivers of this patchset probably don't have to do 
> anything. Drivers with command/packet queues could attempt to send the 
> necessary commands to the device.

Yes that was the second question in the cover letter.
Some drivers probably want a flush_scanout_buffer() to send the 
framebuffer to the hardware.
That callback can be in struct drm_driver as well.

-- 

Jocelyn

> 
> Best regards
> Thomas
> 
>>
>> Maxime
> 

Re: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 13197 bytes --]

Hi

Am 03.10.23 um 16:22 schrieb Jocelyn Falempe:
> drm_panic will need the low-level drm_fb_xxxx_line functions.

It seems like premature optimization to not use drm_fb_blit(); 
especially since drm_panic is not performance critical.

> Also add drm_fb_r1_to_xrgb8888 to render the fonts.

I think we should provide a helper function that returns a pointer to 
the correct function for each supported case. Essentially, it would move 
that if-else branching from drm_fb_blit() into its own function.  It's 
not typical DRM style, but cleaner than retyping the if-elses in drm_panic.

Something like:

typedef int (*drm_format_conv_func)(/* args here */);

drm_format_conv_func  drm_format_conv(u32 dst_fourcc, u32 src_fourcc)
{
	// do if-else from drm_fb_blit here
	
	return <correct-format-conv-helper>
}
EXPORT_SYMBOL(drm_format_conv)

That would be callable from anywhere. You can integrate any helpers for 
_R1 here as well.

Best regards
Thomas

> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/drm_format_helper.c | 88 ++++++++++++++++++++++++++---
>   include/drm/drm_format_helper.h     |  9 +++
>   2 files changed, 89 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index f93a4efcee90..c238e5d84f1f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -270,7 +270,30 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>   
>   	drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, swab_line);
>   }
> -EXPORT_SYMBOL(drm_fb_swab);
> +
> +/**
> + * drm_fb_r1_to_32bit_line - Convert one line from monochrome to any 32bit pixel format
> + * @dbuf: Pointer to the destination line (in any 32bit format)
> + * @sbuf: Pointer to the source line (in monochrome)
> + * @pixels: Number of pixels to convert.
> + * @fg_color: Foreground color, applied when R1 is 1
> + * @bg_color: Background color, applied when R1 is 0
> + *
> + * Convert monochrome to any format with 32bit pixel.
> + * There is a limitation, as sbuf is a pointer, it can only points to a multiple
> + * of 8 pixels in the source buffer.
> + */
> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned int pixels,
> +				u32 fg_color, u32 bg_color)
> +{
> +	unsigned int x;
> +	const u8 *sbuf8 = sbuf;
> +	u32 *dubf32 = dbuf;
> +
> +	for (x = 0; x < pixels; x++)
> +		dubf32[x] = (sbuf8[x / 8] & (0x80 >> (x % 8))) ? fg_color : bg_color;
> +}
> +EXPORT_SYMBOL(drm_fb_r1_to_32bit_line);
>   
>   static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
> @@ -320,7 +343,13 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pi
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
>   
> -static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to RGB565
> + * @dbuf: Pointer to the destination line (in RGB565)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
> @@ -336,6 +365,7 @@ static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigne
>   		dbuf16[x] = cpu_to_le16(val16);
>   	}
>   }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565_line);
>   
>   /* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
>   static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
> @@ -396,7 +426,13 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
>   
> -static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to XRGB1555
> + * @dbuf: Pointer to the destination line (in XRGB1555)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
> @@ -412,6 +448,7 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
>   		dbuf16[x] = cpu_to_le16(val16);
>   	}
>   }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555_line);
>   
>   /**
>    * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
> @@ -447,7 +484,13 @@ void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
>   
> -static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to ARGB1555
> + * @dbuf: Pointer to the destination line (in ARGB1555)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
> @@ -464,6 +507,7 @@ static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
>   		dbuf16[x] = cpu_to_le16(val16);
>   	}
>   }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555_line);
>   
>   /**
>    * drm_fb_xrgb8888_to_argb1555 - Convert XRGB8888 to ARGB1555 clip buffer
> @@ -499,7 +543,13 @@ void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
>   
> -static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgba5551_line - Convert one line from XRGB8888 to ARGB5551
> + * @dbuf: Pointer to the destination line (in ARGB5551)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
> @@ -516,6 +566,7 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
>   		dbuf16[x] = cpu_to_le16(val16);
>   	}
>   }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551_line);
>   
>   /**
>    * drm_fb_xrgb8888_to_rgba5551 - Convert XRGB8888 to RGBA5551 clip buffer
> @@ -551,7 +602,13 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
>   
> -static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to RGB888
> + * @dbuf: Pointer to the destination line (in RGB888)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	u8 *dbuf8 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
> @@ -566,6 +623,7 @@ static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigne
>   		*dbuf8++ = (pix & 0x00FF0000) >> 16;
>   	}
>   }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_line);
>   
>   /**
>    * drm_fb_xrgb8888_to_rgb888 - Convert XRGB8888 to RGB888 clip buffer
> @@ -709,7 +767,13 @@ static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned in
>   		    drm_fb_xrgb8888_to_xbgr8888_line);
>   }
>   
> -static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to XRGB2101010
> + * @dbuf: Pointer to the destination line (in XRGB2101010)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	__le32 *dbuf32 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
> @@ -726,6 +790,7 @@ static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
>   		*dbuf32++ = cpu_to_le32(pix);
>   	}
>   }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_line);
>   
>   /**
>    * drm_fb_xrgb8888_to_xrgb2101010 - Convert XRGB8888 to XRGB2101010 clip buffer
> @@ -761,7 +826,13 @@ void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *d
>   }
>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010);
>   
> -static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
> +/**
> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to ARGB2101010
> + * @dbuf: Pointer to the destination line (in ARGB2101010)
> + * @sbuf: Pointer to the source line (in XRGB8888)
> + * @pixels: Number of pixels to convert.
> + */
> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	__le32 *dbuf32 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
> @@ -779,6 +850,7 @@ static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
>   		*dbuf32++ = cpu_to_le32(pix);
>   	}
>   }
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb2101010_line);
>   
>   /**
>    * drm_fb_xrgb8888_to_argb2101010 - Convert XRGB8888 to ARGB2101010 clip buffer
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index 291deb09475b..31ab699128d5 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -24,30 +24,39 @@ void drm_fb_memcpy(struct iosys_map *dst, const unsigned int *dst_pitch,
>   void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>   		 const struct iosys_map *src, const struct drm_framebuffer *fb,
>   		 const struct drm_rect *clip, bool cached);
> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned int pixels,
> +			     u32 fg_color, u32 bg_color);
>   void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pitch,
>   			       const struct iosys_map *src, const struct drm_framebuffer *fb,
>   			       const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels);
>   void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
>   			       const struct iosys_map *src, const struct drm_framebuffer *fb,
>   			       const struct drm_rect *clip, bool swab);
> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels);
>   void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
>   				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>   				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels);
>   void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
>   				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>   				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels);
>   void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
>   				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>   				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels);
>   void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
>   			       const struct iosys_map *src, const struct drm_framebuffer *fb,
>   			       const struct drm_rect *clip);
>   void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
>   				 const struct iosys_map *src, const struct drm_framebuffer *fb,
>   				 const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels);
>   void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
>   				    const struct iosys_map *src, const struct drm_framebuffer *fb,
>   				    const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels);
>   void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
>   				    const struct iosys_map *src, const struct drm_framebuffer *fb,
>   				    const struct drm_rect *clip);

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 15055 bytes --]

Hi

Am 16.10.23 um 12:47 schrieb Thomas Zimmermann:
> Hi
> 
> Am 03.10.23 um 16:22 schrieb Jocelyn Falempe:
>> drm_panic will need the low-level drm_fb_xxxx_line functions.
> 
> It seems like premature optimization to not use drm_fb_blit(); 
> especially since drm_panic is not performance critical.

We should especially not export any _line fnctions. Those are callbacks 
for drm_fb_xfrm(). They are artifacts of the implementation.

Best regards
Thomas

> 
>> Also add drm_fb_r1_to_xrgb8888 to render the fonts.
> 
> I think we should provide a helper function that returns a pointer to 
> the correct function for each supported case. Essentially, it would move 
> that if-else branching from drm_fb_blit() into its own function.  It's 
> not typical DRM style, but cleaner than retyping the if-elses in drm_panic.
> 
> Something like:
> 
> typedef int (*drm_format_conv_func)(/* args here */);
> 
> drm_format_conv_func  drm_format_conv(u32 dst_fourcc, u32 src_fourcc)
> {
>      // do if-else from drm_fb_blit here
> 
>      return <correct-format-conv-helper>
> }
> EXPORT_SYMBOL(drm_format_conv)
> 
> That would be callable from anywhere. You can integrate any helpers for 
> _R1 here as well.
> 
> Best regards
> Thomas
> 
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 88 ++++++++++++++++++++++++++---
>>   include/drm/drm_format_helper.h     |  9 +++
>>   2 files changed, 89 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c 
>> b/drivers/gpu/drm/drm_format_helper.c
>> index f93a4efcee90..c238e5d84f1f 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -270,7 +270,30 @@ void drm_fb_swab(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>       drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, 
>> swab_line);
>>   }
>> -EXPORT_SYMBOL(drm_fb_swab);
>> +
>> +/**
>> + * drm_fb_r1_to_32bit_line - Convert one line from monochrome to any 
>> 32bit pixel format
>> + * @dbuf: Pointer to the destination line (in any 32bit format)
>> + * @sbuf: Pointer to the source line (in monochrome)
>> + * @pixels: Number of pixels to convert.
>> + * @fg_color: Foreground color, applied when R1 is 1
>> + * @bg_color: Background color, applied when R1 is 0
>> + *
>> + * Convert monochrome to any format with 32bit pixel.
>> + * There is a limitation, as sbuf is a pointer, it can only points to 
>> a multiple
>> + * of 8 pixels in the source buffer.
>> + */
>> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned 
>> int pixels,
>> +                u32 fg_color, u32 bg_color)
>> +{
>> +    unsigned int x;
>> +    const u8 *sbuf8 = sbuf;
>> +    u32 *dubf32 = dbuf;
>> +
>> +    for (x = 0; x < pixels; x++)
>> +        dubf32[x] = (sbuf8[x / 8] & (0x80 >> (x % 8))) ? fg_color : 
>> bg_color;
>> +}
>> +EXPORT_SYMBOL(drm_fb_r1_to_32bit_line);
>>   static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>>   {
>> @@ -320,7 +343,13 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map 
>> *dst, const unsigned int *dst_pi
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
>> -static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to 
>> RGB565
>> + * @dbuf: Pointer to the destination line (in RGB565)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -336,6 +365,7 @@ static void drm_fb_xrgb8888_to_rgb565_line(void 
>> *dbuf, const void *sbuf, unsigne
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565_line);
>>   /* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
>>   static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const 
>> void *sbuf,
>> @@ -396,7 +426,13 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map 
>> *dst, const unsigned int *dst_pi
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
>> -static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to 
>> XRGB1555
>> + * @dbuf: Pointer to the destination line (in XRGB1555)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -412,6 +448,7 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void 
>> *dbuf, const void *sbuf, unsig
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555_line);
>>   /**
>>    * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip 
>> buffer
>> @@ -447,7 +484,13 @@ void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map 
>> *dst, const unsigned int *dst_
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
>> -static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to 
>> ARGB1555
>> + * @dbuf: Pointer to the destination line (in ARGB1555)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -464,6 +507,7 @@ static void drm_fb_xrgb8888_to_argb1555_line(void 
>> *dbuf, const void *sbuf, unsig
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555_line);
>>   /**
>>    * drm_fb_xrgb8888_to_argb1555 - Convert XRGB8888 to ARGB1555 clip 
>> buffer
>> @@ -499,7 +543,13 @@ void drm_fb_xrgb8888_to_argb1555(struct iosys_map 
>> *dst, const unsigned int *dst_
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
>> -static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgba5551_line - Convert one line from XRGB8888 
>> to ARGB5551
>> + * @dbuf: Pointer to the destination line (in ARGB5551)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -516,6 +566,7 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void 
>> *dbuf, const void *sbuf, unsig
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551_line);
>>   /**
>>    * drm_fb_xrgb8888_to_rgba5551 - Convert XRGB8888 to RGBA5551 clip 
>> buffer
>> @@ -551,7 +602,13 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map 
>> *dst, const unsigned int *dst_
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
>> -static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to 
>> RGB888
>> + * @dbuf: Pointer to the destination line (in RGB888)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       u8 *dbuf8 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -566,6 +623,7 @@ static void drm_fb_xrgb8888_to_rgb888_line(void 
>> *dbuf, const void *sbuf, unsigne
>>           *dbuf8++ = (pix & 0x00FF0000) >> 16;
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_line);
>>   /**
>>    * drm_fb_xrgb8888_to_rgb888 - Convert XRGB8888 to RGB888 clip buffer
>> @@ -709,7 +767,13 @@ static void drm_fb_xrgb8888_to_xbgr8888(struct 
>> iosys_map *dst, const unsigned in
>>               drm_fb_xrgb8888_to_xbgr8888_line);
>>   }
>> -static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const 
>> void *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to 
>> XRGB2101010
>> + * @dbuf: Pointer to the destination line (in XRGB2101010)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>>   {
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -726,6 +790,7 @@ static void 
>> drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
>>           *dbuf32++ = cpu_to_le32(pix);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_line);
>>   /**
>>    * drm_fb_xrgb8888_to_xrgb2101010 - Convert XRGB8888 to XRGB2101010 
>> clip buffer
>> @@ -761,7 +826,13 @@ void drm_fb_xrgb8888_to_xrgb2101010(struct 
>> iosys_map *dst, const unsigned int *d
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010);
>> -static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const 
>> void *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to 
>> ARGB2101010
>> + * @dbuf: Pointer to the destination line (in ARGB2101010)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>>   {
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -779,6 +850,7 @@ static void 
>> drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
>>           *dbuf32++ = cpu_to_le32(pix);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb2101010_line);
>>   /**
>>    * drm_fb_xrgb8888_to_argb2101010 - Convert XRGB8888 to ARGB2101010 
>> clip buffer
>> diff --git a/include/drm/drm_format_helper.h 
>> b/include/drm/drm_format_helper.h
>> index 291deb09475b..31ab699128d5 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -24,30 +24,39 @@ void drm_fb_memcpy(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>   void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>>            const struct iosys_map *src, const struct drm_framebuffer *fb,
>>            const struct drm_rect *clip, bool cached);
>> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned 
>> int pixels,
>> +                 u32 fg_color, u32 bg_color);
>>   void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned 
>> int *dst_pitch,
>>                      const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                      const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned 
>> int *dst_pitch,
>>                      const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                      const struct drm_rect *clip, bool swab);
>> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned 
>> int *dst_pitch,
>>                      const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                      const struct drm_rect *clip);
>>   void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels);
>>   void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                       const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                       const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels);
>>   void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                       const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                       const struct drm_rect *clip);
> 

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 1/4] drm/format-helper: Export line conversion helper for drm_panic

[Jocelyn Falempe]

On 16/10/2023 12:47, Thomas Zimmermann wrote:
> Hi
> 
> Am 03.10.23 um 16:22 schrieb Jocelyn Falempe:
>> drm_panic will need the low-level drm_fb_xxxx_line functions.
> 
> It seems like premature optimization to not use drm_fb_blit(); 
> especially since drm_panic is not performance critical.
> 
>> Also add drm_fb_r1_to_xrgb8888 to render the fonts.
> 
> I think we should provide a helper function that returns a pointer to 
> the correct function for each supported case. Essentially, it would move 
> that if-else branching from drm_fb_blit() into its own function.  It's 
> not typical DRM style, but cleaner than retyping the if-elses in drm_panic.
> 
> Something like:
> 
> typedef int (*drm_format_conv_func)(/* args here */);
> 
> drm_format_conv_func  drm_format_conv(u32 dst_fourcc, u32 src_fourcc)
> {
>      // do if-else from drm_fb_blit here
> 
>      return <correct-format-conv-helper>
> }
> EXPORT_SYMBOL(drm_format_conv)
> 
> That would be callable from anywhere. You can integrate any helpers for 
> _R1 here as well.

Regarding the color conversion approach, I think we don't need to 
convert the whole buffer, like drm_fb_blit() or the xxxx_line() do. Just 
converting the fg_color and bg_color is enough, and then only the pixel 
size matters, when converting from R1.

So instead of having plenty of conversion functions, I will only need
R1_to_8bit(), R1_to_16bit(), R1_to_24bit() and R1_to_32bit()

I can encapsulate this in a drm_blit_from_r1(), that will also take the 
fg_color and bg_color as parameter.

If you agree with that, I will do it for the v5.

-- 

Jocelyn


> 
> Best regards
> Thomas
> 
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 88 ++++++++++++++++++++++++++---
>>   include/drm/drm_format_helper.h     |  9 +++
>>   2 files changed, 89 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c 
>> b/drivers/gpu/drm/drm_format_helper.c
>> index f93a4efcee90..c238e5d84f1f 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -270,7 +270,30 @@ void drm_fb_swab(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>       drm_fb_xfrm(dst, dst_pitch, &cpp, src, fb, clip, cached, 
>> swab_line);
>>   }
>> -EXPORT_SYMBOL(drm_fb_swab);
>> +
>> +/**
>> + * drm_fb_r1_to_32bit_line - Convert one line from monochrome to any 
>> 32bit pixel format
>> + * @dbuf: Pointer to the destination line (in any 32bit format)
>> + * @sbuf: Pointer to the source line (in monochrome)
>> + * @pixels: Number of pixels to convert.
>> + * @fg_color: Foreground color, applied when R1 is 1
>> + * @bg_color: Background color, applied when R1 is 0
>> + *
>> + * Convert monochrome to any format with 32bit pixel.
>> + * There is a limitation, as sbuf is a pointer, it can only points to 
>> a multiple
>> + * of 8 pixels in the source buffer.
>> + */
>> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned 
>> int pixels,
>> +                u32 fg_color, u32 bg_color)
>> +{
>> +    unsigned int x;
>> +    const u8 *sbuf8 = sbuf;
>> +    u32 *dubf32 = dbuf;
>> +
>> +    for (x = 0; x < pixels; x++)
>> +        dubf32[x] = (sbuf8[x / 8] & (0x80 >> (x % 8))) ? fg_color : 
>> bg_color;
>> +}
>> +EXPORT_SYMBOL(drm_fb_r1_to_32bit_line);
>>   static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>>   {
>> @@ -320,7 +343,13 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map 
>> *dst, const unsigned int *dst_pi
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
>> -static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to 
>> RGB565
>> + * @dbuf: Pointer to the destination line (in RGB565)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -336,6 +365,7 @@ static void drm_fb_xrgb8888_to_rgb565_line(void 
>> *dbuf, const void *sbuf, unsigne
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565_line);
>>   /* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
>>   static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const 
>> void *sbuf,
>> @@ -396,7 +426,13 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map 
>> *dst, const unsigned int *dst_pi
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
>> -static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to 
>> XRGB1555
>> + * @dbuf: Pointer to the destination line (in XRGB1555)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -412,6 +448,7 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void 
>> *dbuf, const void *sbuf, unsig
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555_line);
>>   /**
>>    * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip 
>> buffer
>> @@ -447,7 +484,13 @@ void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map 
>> *dst, const unsigned int *dst_
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
>> -static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb565_line - Convert one line from XRGB8888 to 
>> ARGB1555
>> + * @dbuf: Pointer to the destination line (in ARGB1555)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -464,6 +507,7 @@ static void drm_fb_xrgb8888_to_argb1555_line(void 
>> *dbuf, const void *sbuf, unsig
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555_line);
>>   /**
>>    * drm_fb_xrgb8888_to_argb1555 - Convert XRGB8888 to ARGB1555 clip 
>> buffer
>> @@ -499,7 +543,13 @@ void drm_fb_xrgb8888_to_argb1555(struct iosys_map 
>> *dst, const unsigned int *dst_
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
>> -static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgba5551_line - Convert one line from XRGB8888 
>> to ARGB5551
>> + * @dbuf: Pointer to the destination line (in ARGB5551)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -516,6 +566,7 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void 
>> *dbuf, const void *sbuf, unsig
>>           dbuf16[x] = cpu_to_le16(val16);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551_line);
>>   /**
>>    * drm_fb_xrgb8888_to_rgba5551 - Convert XRGB8888 to RGBA5551 clip 
>> buffer
>> @@ -551,7 +602,13 @@ void drm_fb_xrgb8888_to_rgba5551(struct iosys_map 
>> *dst, const unsigned int *dst_
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
>> -static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to 
>> RGB888
>> + * @dbuf: Pointer to the destination line (in RGB888)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels)
>>   {
>>       u8 *dbuf8 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -566,6 +623,7 @@ static void drm_fb_xrgb8888_to_rgb888_line(void 
>> *dbuf, const void *sbuf, unsigne
>>           *dbuf8++ = (pix & 0x00FF0000) >> 16;
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_line);
>>   /**
>>    * drm_fb_xrgb8888_to_rgb888 - Convert XRGB8888 to RGB888 clip buffer
>> @@ -709,7 +767,13 @@ static void drm_fb_xrgb8888_to_xbgr8888(struct 
>> iosys_map *dst, const unsigned in
>>               drm_fb_xrgb8888_to_xbgr8888_line);
>>   }
>> -static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const 
>> void *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to 
>> XRGB2101010
>> + * @dbuf: Pointer to the destination line (in XRGB2101010)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>>   {
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -726,6 +790,7 @@ static void 
>> drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
>>           *dbuf32++ = cpu_to_le32(pix);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_line);
>>   /**
>>    * drm_fb_xrgb8888_to_xrgb2101010 - Convert XRGB8888 to XRGB2101010 
>> clip buffer
>> @@ -761,7 +826,13 @@ void drm_fb_xrgb8888_to_xrgb2101010(struct 
>> iosys_map *dst, const unsigned int *d
>>   }
>>   EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010);
>> -static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const 
>> void *sbuf, unsigned int pixels)
>> +/**
>> + * drm_fb_xrgb8888_to_rgb888_line - Convert one line from XRGB8888 to 
>> ARGB2101010
>> + * @dbuf: Pointer to the destination line (in ARGB2101010)
>> + * @sbuf: Pointer to the source line (in XRGB8888)
>> + * @pixels: Number of pixels to convert.
>> + */
>> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>>   {
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>> @@ -779,6 +850,7 @@ static void 
>> drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
>>           *dbuf32++ = cpu_to_le32(pix);
>>       }
>>   }
>> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb2101010_line);
>>   /**
>>    * drm_fb_xrgb8888_to_argb2101010 - Convert XRGB8888 to ARGB2101010 
>> clip buffer
>> diff --git a/include/drm/drm_format_helper.h 
>> b/include/drm/drm_format_helper.h
>> index 291deb09475b..31ab699128d5 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -24,30 +24,39 @@ void drm_fb_memcpy(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>   void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>>            const struct iosys_map *src, const struct drm_framebuffer *fb,
>>            const struct drm_rect *clip, bool cached);
>> +void drm_fb_r1_to_32bit_line(void *dbuf, const void *sbuf, unsigned 
>> int pixels,
>> +                 u32 fg_color, u32 bg_color);
>>   void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned 
>> int *dst_pitch,
>>                      const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                      const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned 
>> int *dst_pitch,
>>                      const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                      const struct drm_rect *clip, bool swab);
>> +void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, 
>> unsigned int pixels);
>>   void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned 
>> int *dst_pitch,
>>                      const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                      const struct drm_rect *clip);
>>   void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                    const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                    const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels);
>>   void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                       const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                       const struct drm_rect *clip);
>> +void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels);
>>   void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>                       const struct iosys_map *src, const struct 
>> drm_framebuffer *fb,
>>                       const struct drm_rect *clip);
>