[PATCH v9 0/9] drm/panic: Add a drm panic handler

[PATCH v9 0/9] 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, mgag200, ast, and imx.

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

or you can enable CONFIG_DRM_PANIC_DEBUG and echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0


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.
 
v5:
 * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
   (Thomas Zimmermann)
 * Also add drm_fb_fill() to fill area with background color.
 * Add draw_pixel_xy() API for drivers that can't provide a linear buffer.
 * Add a flush() callback for drivers that needs to synchronize the buffer.
 * Add a void *private field, so drivers can pass private data to
   draw_pixel_xy() and flush(). 
 * Add ast support.
 * Add experimental imx/ipuv3 support, to test on an ARM hw. (Maxime Ripard)

v6:
 * Fix sparse and __le32 warnings
 * Drop the IMX/IPUV3 experiment, it was just to show that it works also on
   ARM devices.

v7:
 * Add a check to see if the 4cc format is supported by drm_panic.
 * Add a drm/plane helper to loop over all visible primary buffer,
   simplifying the get_scanout_buffer implementations
 * Add a generic implementation for drivers that uses drm_fb_dma. (Maxime Ripard)
 * Add back the IMX/IPUV3 support, and use the generic implementation. (Maxime Ripard)

v8:
 * Directly register each plane to the panic notifier (Sima)
 * Replace get_scanout_buffer() with set_scanout_buffer() to simplify
   the locking. (Thomas Zimmermann)
 * Add a debugfs entry, to trigger the drm_panic without a real panic (Sima)
 * Fix the drm_panic Documentation, and include it in drm-kms.rst

v9:
 * Revert to using get_scanout_buffer() (Sima)
 * Move get_scanout_buffer() and panic_flush() to the plane helper
   functions (Thomas Zimmermann)
 * Register all planes with get_scanout_buffer() to the panic notifier
 * Use drm_panic_lock() to protect against race (Sima)
 * Create a debugfs file for each plane in the device's debugfs
   directory. This allows to test for each plane of each GPU
   independently.

Daniel Vetter (1):
  drm/panic: Add drm panic locking

Jocelyn Falempe (8):
  drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill
  drm/panic: Add a drm panic handler
  drm/panic: Add debugfs entry to test without triggering panic.
  drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
  drm/simpledrm: Add drm_panic support
  drm/mgag200: Add drm_panic support
  drm/imx: Add drm_panic support
  drm/ast: Add drm_panic support

 Documentation/gpu/drm-kms.rst            |  12 +
 drivers/gpu/drm/Kconfig                  |  32 ++
 drivers/gpu/drm/Makefile                 |   1 +
 drivers/gpu/drm/ast/ast_mode.c           |  18 +
 drivers/gpu/drm/drm_atomic_helper.c      |   3 +
 drivers/gpu/drm/drm_drv.c                |   5 +
 drivers/gpu/drm/drm_fb_dma_helper.c      |  47 +++
 drivers/gpu/drm/drm_format_helper.c      | 432 ++++++++++++++++++-----
 drivers/gpu/drm/drm_panic.c              | 360 +++++++++++++++++++
 drivers/gpu/drm/drm_plane.c              |   1 +
 drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c  |  11 +-
 drivers/gpu/drm/mgag200/mgag200_drv.h    |   7 +-
 drivers/gpu/drm/mgag200/mgag200_mode.c   |  18 +
 drivers/gpu/drm/tiny/simpledrm.c         |  16 +
 include/drm/drm_fb_dma_helper.h          |   4 +
 include/drm/drm_format_helper.h          |   9 +
 include/drm/drm_mode_config.h            |  10 +
 include/drm/drm_modeset_helper_vtables.h |  37 ++
 include/drm/drm_panic.h                  | 151 ++++++++
 include/drm/drm_plane.h                  |   5 +
 20 files changed, 1096 insertions(+), 83 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_panic.c
 create mode 100644 include/drm/drm_panic.h


base-commit: f89632a9e5fa6c4787c14458cd42a9ef42025434
-- 
2.43.2

[PATCH v9 1/9] drm/panic: Add drm panic locking

[Jocelyn Falempe]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

Rough sketch for the locking of drm panic printing code. The upshot of
this approach is that we can pretty much entirely rely on the atomic
commit flow, with the pair of raw_spin_lock/unlock providing any
barriers we need, without having to create really big critical
sections in code.

This also avoids the need that drivers must explicitly update the
panic handler state, which they might forget to do, or not do
consistently, and then we blow up in the worst possible times.

It is somewhat racy against a concurrent atomic update, and we might
write into a buffer which the hardware will never display. But there's
fundamentally no way to avoid that - if we do the panic state update
explicitly after writing to the hardware, we might instead write to an
old buffer that the user will barely ever see.

Note that an rcu protected deference of plane->state would give us the
the same guarantees, but it has the downside that we then need to
protect the plane state freeing functions with call_rcu too. Which
would very widely impact a lot of code and therefore doesn't seem
worth the complexity compared to a raw spinlock with very tiny
critical sections. Plus rcu cannot be used to protect access to
peek/poke registers anyway, so we'd still need it for those cases.

Peek/poke registers for vram access (or a gart pte reserved just for
panic code) are also the reason I've gone with a per-device and not
per-plane spinlock, since usually these things are global for the
entire display. Going with per-plane locks would mean drivers for such
hardware would need additional locks, which we don't want, since it
deviates from the per-console takeoverlocks design.

Longer term it might be useful if the panic notifiers grow a bit more
structure than just the absolute bare
EXPORT_SYMBOL(panic_notifier_list) - somewhat aside, why is that not
EXPORT_SYMBOL_GPL ... If panic notifiers would be more like console
drivers with proper register/unregister interfaces we could perhaps
reuse the very fancy console lock with all it's check and takeover
semantics that John Ogness is developing to fix the console_lock mess.
But for the initial cut of a drm panic printing support I don't think
we need that, because the critical sections are extremely small and
only happen once per display refresh. So generally just 60 tiny locked
sections per second, which is nothing compared to a serial console
running a 115kbaud doing really slow mmio writes for each byte. So for
now the raw spintrylock in drm panic notifier callback should be good
enough.

Another benefit of making panic notifiers more like full blown
consoles (that are used in panics only) would be that we get the two
stage design, where first all the safe outputs are used. And then the
dangerous takeover tricks are deployed (where for display drivers we
also might try to intercept any in-flight display buffer flips, which
if we race and misprogram fifos and watermarks can hang the memory
controller on some hw).

For context the actual implementation on the drm side is by Jocelyn
and this patch is meant to be combined with the overall approach in
v7 (v8 is a bit less flexible, which I think is the wrong direction):

https://lore.kernel.org/dri-devel/20240104160301.185915-1-jfalempe@redhat.com/

Note that the locking is very much not correct there, hence this
separate rfc.

v2:
- fix authorship, this was all my typing
- some typo oopsies
- link to the drm panic work by Jocelyn for context

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic_helper.c |  3 +
 drivers/gpu/drm/drm_drv.c           |  1 +
 include/drm/drm_mode_config.h       | 10 +++
 include/drm/drm_panic.h             | 99 +++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+)
 create mode 100644 include/drm/drm_panic.h

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 39ef0a6addeb..c0bb91312fb2 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -38,6 +38,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_print.h>
 #include <drm/drm_self_refresh_helper.h>
 #include <drm/drm_vblank.h>
@@ -3099,6 +3100,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		}
 	}
 
+	drm_panic_lock(state->dev);
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 		WARN_ON(plane->state != old_plane_state);
 
@@ -3108,6 +3110,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		state->planes[i].state = old_plane_state;
 		plane->state = new_plane_state;
 	}
+	drm_panic_unlock(state->dev);
 
 	for_each_oldnew_private_obj_in_state(state, obj, old_obj_state, new_obj_state, i) {
 		WARN_ON(obj->state != old_obj_state);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 243cacb3575c..c157500b3135 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -638,6 +638,7 @@ static int drm_dev_init(struct drm_device *dev,
 	mutex_init(&dev->filelist_mutex);
 	mutex_init(&dev->clientlist_mutex);
 	mutex_init(&dev->master_mutex);
+	raw_spin_lock_init(&dev->mode_config.panic_lock);
 
 	ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL);
 	if (ret)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 973119a9176b..e79f1a557a22 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -505,6 +505,16 @@ struct drm_mode_config {
 	 */
 	struct list_head plane_list;
 
+	/**
+	 * @panic_lock:
+	 *
+	 * Raw spinlock used to protect critical sections of code that access
+	 * the display hardware or modeset software state, which the panic
+	 * printing code must be protected against. See drm_panic_trylock(),
+	 * drm_panic_lock() and drm_panic_unlock().
+	 */
+	struct raw_spinlock panic_lock;
+
 	/**
 	 * @num_crtc:
 	 *
diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
new file mode 100644
index 000000000000..f2135d03f1eb
--- /dev/null
+++ b/include/drm/drm_panic.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+#ifndef __DRM_PANIC_H__
+#define __DRM_PANIC_H__
+
+#include <drm/drm_device.h>
+/*
+ * Copyright (c) 2024 Intel
+ */
+
+/**
+ * drm_panic_trylock - try to enter the panic printing critical section
+ * @dev: struct drm_device
+ *
+ * This function must be called by any panic printing code. The panic printing
+ * attempt must be aborted if the trylock fails.
+ *
+ * Panic printing code can make the following assumptions while holding the
+ * panic lock:
+ *
+ * - Anything protected by drm_panic_lock() and drm_panic_unlock() pairs is safe
+ *   to access.
+ *
+ * - Furthermore the panic printing code only registers in drm_dev_unregister()
+ *   and gets removed in drm_dev_unregister(). This allows the panic code to
+ *   safely access any state which is invariant in between these two function
+ *   calls, like the list of planes drm_mode_config.plane_list or most of the
+ *   struct drm_plane structure.
+ *
+ * Specifically thanks to the protection around plane updates in
+ * drm_atomic_helper_swap_state() the following additional guarantees hold:
+ *
+ * - It is safe to deference the drm_plane.state pointer.
+ *
+ * - Anything in struct drm_plane_state or the driver's subclass thereof which
+ *   stays invariant after the atomic check code has finished is safe to access.
+ *   Specifically this includes the reference counted pointers to framebuffer
+ *   and buffer objects.
+ *
+ * - Anything set up by drm_plane_helper_funcs.fb_prepare and cleaned up
+ *   drm_plane_helper_funcs.fb_cleanup is safe to access, as long as it stays
+ *   invariant between these two calls. This also means that for drivers using
+ *   dynamic buffer management the framebuffer is pinned, and therefer all
+ *   relevant datastructures can be accessed without taking any further locks
+ *   (which would be impossible in panic context anyway).
+ *
+ * - Importantly, software and hardware state set up by
+ *   drm_plane_helper_funcs.begin_fb_access and
+ *   drm_plane_helper_funcs.end_fb_access is not safe to access.
+ *
+ * Drivers must not make any assumptions about the actual state of the hardware,
+ * unless they explicitly protected these hardware access with drm_panic_lock()
+ * and drm_panic_unlock().
+ *
+ * Returns:
+ *
+ * 0 when failing to acquire the raw spinlock, nonzero on success.
+ */
+static inline int drm_panic_trylock(struct drm_device *dev)
+{
+	return raw_spin_trylock(&dev->mode_config.panic_lock);
+}
+
+/**
+ * drm_panic_lock - protect panic printing relevant state
+ * @dev: struct drm_device
+ *
+ * This function must be called to protect software and hardware state that the
+ * panic printing code must be able to rely on. The protected sections must be
+ * as small as possible. Examples include:
+ *
+ * - Access to peek/poke or other similar registers, if that is the way the
+ *   driver prints the pixels into the scanout buffer at panic time.
+ *
+ * - Updates to pointers like drm_plane.state, allowing the panic handler to
+ *   safely deference these. This is done in drm_atomic_helper_swap_state().
+ *
+ * - An state that isn't invariant and that the driver must be able to access
+ *   during panic printing.
+ *
+ * Call drm_panic_unlock() to unlock the locked spinlock.
+ */
+static inline void drm_panic_lock(struct drm_device *dev)
+{
+	return raw_spin_lock(&dev->mode_config.panic_lock);
+}
+
+/**
+ * drm_panic_unlock - end of the panic printing critical section
+ * @dev: struct drm_device
+ *
+ * Unlocks the raw spinlock acquired by either drm_panic_lock() or
+ * drm_panic_trylock().
+ */
+static inline void drm_panic_unlock(struct drm_device *dev)
+{
+	raw_spin_unlock(&dev->mode_config.panic_lock);
+}
+
+#endif /* __DRM_PANIC_H__ */
-- 
2.43.2

[PATCH v9 2/9] drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill

[Jocelyn Falempe]

This is needed for drm_panic, to draw the font, and fill
the background color, in multiple color format.

v5:
 * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
 * Also add drm_fb_fill() to fill area with background color.
v6:
 * fix __le32 conversion warning found by the kernel test bot

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

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index b1be458ed4dd..2d9646cefc4f 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -111,6 +111,153 @@ void drm_format_conv_state_release(struct drm_format_conv_state *state)
 }
 EXPORT_SYMBOL(drm_format_conv_state_release);
 
+static __le16 drm_format_xrgb8888_to_rgb565(__le32 val32)
+{
+	u16 val16;
+	u32 pix;
+
+	pix = le32_to_cpu(val32);
+	val16 = ((pix & 0x00F80000) >> 8) |
+		((pix & 0x0000FC00) >> 5) |
+		((pix & 0x000000F8) >> 3);
+	return cpu_to_le16(val16);
+}
+
+static __le16 drm_format_xrgb8888_to_rgba5551(__le32 val32)
+{
+	u16 val16;
+	u32 pix;
+
+	pix = le32_to_cpu(val32);
+	val16 = ((pix & 0x00f80000) >> 8) |
+		((pix & 0x0000f800) >> 5) |
+		((pix & 0x000000f8) >> 2) |
+		BIT(0); /* set alpha bit */
+	return cpu_to_le16(val16);
+}
+
+static __le16 drm_format_xrgb8888_to_xrgb1555(__le32 val32)
+{
+	u16 val16;
+	u32 pix;
+
+	pix = le32_to_cpu(val32);
+	val16 = ((pix & 0x00f80000) >> 9) |
+		((pix & 0x0000f800) >> 6) |
+		((pix & 0x000000f8) >> 3);
+	return cpu_to_le16(val16);
+}
+
+static __le16 drm_format_xrgb8888_to_argb1555(__le32 val32)
+{
+	u16 val16;
+	u32 pix;
+
+	pix = le32_to_cpu(val32);
+	val16 = BIT(15) | /* set alpha bit */
+		((pix & 0x00f80000) >> 9) |
+		((pix & 0x0000f800) >> 6) |
+		((pix & 0x000000f8) >> 3);
+	return cpu_to_le16(val16);
+}
+
+static __le32 drm_format_xrgb8888_to_argb8888(__le32 pix)
+{
+	u32 val32;
+
+	val32 = le32_to_cpu(pix);
+	val32 |= GENMASK(31, 24); /* fill alpha bits */
+	return cpu_to_le32(val32);
+}
+
+static __le32 drm_format_xrgb8888_to_xbgr8888(__le32 pix)
+{
+	u32 val32;
+
+	val32 = le32_to_cpu(pix);
+	val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
+		((val32 & 0x0000ff00) >>  8) <<  8 |
+		((val32 & 0x000000ff) >>  0) << 16 |
+		((val32 & 0xff000000) >> 24) << 24;
+	return cpu_to_le32(val32);
+}
+
+static __le32 drm_format_xrgb8888_to_abgr8888(__le32 pix)
+{
+	u32 val32;
+
+	val32 = le32_to_cpu(pix);
+	val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
+		((val32 & 0x0000ff00) >>  8) <<  8 |
+		((val32 & 0x000000ff) >>  0) << 16 |
+		GENMASK(31, 24); /* fill alpha bits */
+	return cpu_to_le32(val32);
+}
+
+static __le32 drm_format_xrgb8888_to_xrgb2101010(__le32 pix)
+{
+	u32 val32;
+
+	val32 = le32_to_cpu(pix);
+	val32 = ((val32 & 0x000000FF) << 2) |
+		((val32 & 0x0000FF00) << 4) |
+		((val32 & 0x00FF0000) << 6);
+	return cpu_to_le32(val32 | ((val32 >> 8) & 0x00300C03));
+}
+
+static __le32 drm_format_xrgb8888_to_argb2101010(__le32 pix)
+{
+	u32 val32;
+
+	val32 = le32_to_cpu(pix);
+	val32 = ((val32 & 0x000000FF) << 2) |
+		((val32 & 0x0000FF00) << 4) |
+		((val32 & 0x00FF0000) << 6);
+	val32 = GENMASK(31, 30) | /* set alpha bits */
+	      val32 | ((val32 >> 8) & 0x00300c03);
+	return cpu_to_le32(val32);
+}
+
+/**
+ * drm_fb_convert_from_xrgb8888 - convert one pixel from xrgb8888 to the desired format
+ * @color: input color, in xrgb8888 format
+ * @format: output format
+ *
+ * Returns:
+ * Color in the format specified, casted to u32.
+ * Or 0 if the format is unknown.
+ */
+u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format)
+{
+	__le32 pix = cpu_to_le32(color);
+
+	switch (format) {
+	case DRM_FORMAT_RGB565:
+		return le16_to_cpu(drm_format_xrgb8888_to_rgb565(pix));
+	case DRM_FORMAT_RGBA5551:
+		return le16_to_cpu(drm_format_xrgb8888_to_rgba5551(pix));
+	case DRM_FORMAT_XRGB1555:
+		return le16_to_cpu(drm_format_xrgb8888_to_xrgb1555(pix));
+	case DRM_FORMAT_ARGB1555:
+		return le16_to_cpu(drm_format_xrgb8888_to_argb1555(pix));
+	case DRM_FORMAT_RGB888:
+	case DRM_FORMAT_XRGB8888:
+		return le32_to_cpu(pix);
+	case DRM_FORMAT_ARGB8888:
+		return le32_to_cpu(drm_format_xrgb8888_to_argb8888(pix));
+	case DRM_FORMAT_XBGR8888:
+		return le32_to_cpu(drm_format_xrgb8888_to_xbgr8888(pix));
+	case DRM_FORMAT_XRGB2101010:
+		return le32_to_cpu(drm_format_xrgb8888_to_xrgb2101010(pix));
+	case DRM_FORMAT_ARGB2101010:
+		return le32_to_cpu(drm_format_xrgb8888_to_argb2101010(pix));
+	default:
+		WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
+		return 0;
+	}
+}
+EXPORT_SYMBOL(drm_fb_convert_from_xrgb8888);
+
 static unsigned int clip_offset(const struct drm_rect *clip, unsigned int pitch, unsigned int cpp)
 {
 	return clip->y1 * pitch + clip->x1 * cpp;
@@ -366,6 +513,193 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
 }
 EXPORT_SYMBOL(drm_fb_swab);
 
+static void drm_fb_r1_to_16bit(struct iosys_map *dmap, unsigned int dpitch,
+			       const u8 *sbuf8, unsigned int spitch,
+			       unsigned int height, unsigned int width,
+			       __le16 fg16, __le16 bg16)
+{
+	unsigned int l, x;
+	__le16 val16;
+
+	for (l = 0; l < height; l++) {
+		for (x = 0; x < width; x++) {
+			val16 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg16 : bg16;
+			iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, le16_to_cpu(val16));
+		}
+	}
+}
+
+static void drm_fb_r1_to_24bit(struct iosys_map *dmap, unsigned int dpitch,
+			       const u8 *sbuf8, unsigned int spitch,
+			       unsigned int height, unsigned int width,
+			       __le32 fg32, __le32 bg32)
+{
+	unsigned int l, x;
+	__le32 color;
+	u32 val32;
+
+	for (l = 0; l < height; l++) {
+		for (x = 0; x < width; x++) {
+			u32 off = l * dpitch + x * 3;
+
+			color = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32;
+			val32 = le32_to_cpu(color);
+
+			/* write blue-green-red to output in little endianness */
+			iosys_map_wr(dmap, off, u8, (val32 & 0x000000FF) >> 0);
+			iosys_map_wr(dmap, off + 1, u8, (val32 & 0x0000FF00) >> 8);
+			iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF0000) >> 16);
+		}
+	}
+}
+
+static void drm_fb_r1_to_32bit(struct iosys_map *dmap, unsigned int dpitch,
+			       const u8 *sbuf8, unsigned int spitch,
+			       unsigned int height, unsigned int width,
+			       __le32 fg32, __le32 bg32)
+{
+	unsigned int l, x;
+	__le32 val32;
+
+	for (l = 0; l < height; l++) {
+		for (x = 0; x < width; x++) {
+			val32 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32;
+			iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, le32_to_cpu(val32));
+		}
+	}
+}
+
+/**
+ * drm_fb_blit_from_r1 - convert a monochrome image to a linear framebuffer
+ * @dmap: destination iosys_map
+ * @dpitch: destination pitch in bytes
+ * @sbuf8: source buffer, in monochrome format, 8 pixels per byte.
+ * @spitch: source pitch in bytes
+ * @height: height of the image to copy, in pixels
+ * @width: width of the image to copy, in pixels
+ * @fg_color: foreground color, in destination format
+ * @bg_color: background color, in destination format
+ * @pixel_width: pixel width in bytes.
+ *
+ * This can be used to draw font which are monochrome images, to a framebuffer
+ * in other supported format.
+ */
+void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
+			 const u8 *sbuf8, unsigned int spitch,
+			 unsigned int height, unsigned int width,
+			 u32 fg_color, u32 bg_color,
+			 unsigned int pixel_width)
+{
+	switch (pixel_width) {
+	case 2:
+		drm_fb_r1_to_16bit(dmap, dpitch, sbuf8, spitch,
+				   height, width,
+				   cpu_to_le16(fg_color),
+				   cpu_to_le16(bg_color));
+	break;
+	case 3:
+		drm_fb_r1_to_24bit(dmap, dpitch, sbuf8, spitch,
+				   height, width,
+				   cpu_to_le32(fg_color),
+				   cpu_to_le32(bg_color));
+	break;
+	case 4:
+		drm_fb_r1_to_32bit(dmap, dpitch, sbuf8, spitch,
+				   height, width,
+				   cpu_to_le32(fg_color),
+				   cpu_to_le32(bg_color));
+	break;
+	default:
+		WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width);
+	}
+}
+EXPORT_SYMBOL(drm_fb_blit_from_r1);
+
+static void drm_fb_fill8(struct iosys_map *dmap, unsigned int dpitch,
+			 unsigned int height, unsigned int width,
+			 u8 color)
+{
+	unsigned int l, x;
+
+	for (l = 0; l < height; l++)
+		for (x = 0; x < width; x++)
+			iosys_map_wr(dmap, l * dpitch + x * sizeof(u8), u8, color);
+}
+
+static void drm_fb_fill16(struct iosys_map *dmap, unsigned int dpitch,
+			  unsigned int height, unsigned int width,
+			  u16 color)
+{
+	unsigned int l, x;
+
+	for (l = 0; l < height; l++)
+		for (x = 0; x < width; x++)
+			iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, color);
+}
+
+static void drm_fb_fill24(struct iosys_map *dmap, unsigned int dpitch,
+			  unsigned int height, unsigned int width,
+			  u32 color)
+{
+	unsigned int l, x;
+
+	for (l = 0; l < height; l++) {
+		for (x = 0; x < width; x++) {
+			unsigned int off = l * dpitch + x * 3;
+
+			/* write blue-green-red to output in little endianness */
+			iosys_map_wr(dmap, off, u8, (color & 0x000000FF) >> 0);
+			iosys_map_wr(dmap, off + 1, u8, (color & 0x0000FF00) >> 8);
+			iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 16);
+		}
+	}
+}
+
+static void drm_fb_fill32(struct iosys_map *dmap, unsigned int dpitch,
+			  unsigned int height, unsigned int width,
+			  u32 color)
+{
+	unsigned int l, x;
+
+	for (l = 0; l < height; l++)
+		for (x = 0; x < width; x++)
+			iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, color);
+}
+
+/**
+ * drm_fb_fill - Fill a rectangle with a color
+ * @dmap: destination iosys_map, pointing to the top left corner of the rectangle
+ * @dpitch: destination pitch in bytes
+ * @height: height of the rectangle, in pixels
+ * @width: width of the rectangle, in pixels
+ * @color: color to fill the rectangle.
+ * @pixel_width: pixel width in bytes
+ *
+ * Fill a rectangle with a color, in a linear framebuffer.
+ */
+void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
+			 unsigned int height, unsigned int width,
+			 u32 color, unsigned int pixel_width)
+{
+	switch (pixel_width) {
+	case 1:
+		drm_fb_fill8(dmap, dpitch, height, width, color);
+	break;
+	case 2:
+		drm_fb_fill16(dmap, dpitch, height, width, color);
+	break;
+	case 3:
+		drm_fb_fill24(dmap, dpitch, height, width, color);
+	break;
+	case 4:
+		drm_fb_fill32(dmap, dpitch, height, width, color);
+	break;
+	default:
+		WARN_ONCE(1, "Can't fill with pixel width %d\n", pixel_width);
+	}
+}
+EXPORT_SYMBOL(drm_fb_fill);
+
 static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	u8 *dbuf8 = dbuf;
@@ -420,15 +754,9 @@ static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigne
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u16 val16;
-	u32 pix;
 
 	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		val16 = ((pix & 0x00F80000) >> 8) |
-			((pix & 0x0000FC00) >> 5) |
-			((pix & 0x000000F8) >> 3);
-		dbuf16[x] = cpu_to_le16(val16);
+		dbuf16[x] = drm_format_xrgb8888_to_rgb565(sbuf32[x]);
 	}
 }
 
@@ -498,16 +826,9 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u16 val16;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		val16 = ((pix & 0x00f80000) >> 9) |
-			((pix & 0x0000f800) >> 6) |
-			((pix & 0x000000f8) >> 3);
-		dbuf16[x] = cpu_to_le16(val16);
-	}
+	for (x = 0; x < pixels; x++)
+		dbuf16[x] = drm_format_xrgb8888_to_xrgb1555(sbuf32[x]);
 }
 
 /**
@@ -550,17 +871,9 @@ static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u16 val16;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		val16 = BIT(15) | /* set alpha bit */
-			((pix & 0x00f80000) >> 9) |
-			((pix & 0x0000f800) >> 6) |
-			((pix & 0x000000f8) >> 3);
-		dbuf16[x] = cpu_to_le16(val16);
-	}
+	for (x = 0; x < pixels; x++)
+		dbuf16[x] = drm_format_xrgb8888_to_argb1555(sbuf32[x]);
 }
 
 /**
@@ -603,17 +916,9 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
 	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u16 val16;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		val16 = ((pix & 0x00f80000) >> 8) |
-			((pix & 0x0000f800) >> 5) |
-			((pix & 0x000000f8) >> 2) |
-			BIT(0); /* set alpha bit */
-		dbuf16[x] = cpu_to_le16(val16);
-	}
+	for (x = 0; x < pixels; x++)
+		dbuf16[x] = drm_format_xrgb8888_to_rgba5551(sbuf32[x]);
 }
 
 /**
@@ -707,13 +1012,9 @@ static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsig
 	__le32 *dbuf32 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		pix |= GENMASK(31, 24); /* fill alpha bits */
-		dbuf32[x] = cpu_to_le32(pix);
-	}
+	for (x = 0; x < pixels; x++)
+		dbuf32[x] = drm_format_xrgb8888_to_argb8888(sbuf32[x]);
 }
 
 /**
@@ -756,16 +1057,9 @@ static void drm_fb_xrgb8888_to_abgr8888_line(void *dbuf, const void *sbuf, unsig
 	__le32 *dbuf32 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		pix = ((pix & 0x00ff0000) >> 16) <<  0 |
-		      ((pix & 0x0000ff00) >>  8) <<  8 |
-		      ((pix & 0x000000ff) >>  0) << 16 |
-		      GENMASK(31, 24); /* fill alpha bits */
-		*dbuf32++ = cpu_to_le32(pix);
-	}
+	for (x = 0; x < pixels; x++)
+		*dbuf32++ = drm_format_xrgb8888_to_abgr8888(sbuf32[x]);
 }
 
 static void drm_fb_xrgb8888_to_abgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
@@ -787,16 +1081,9 @@ static void drm_fb_xrgb8888_to_xbgr8888_line(void *dbuf, const void *sbuf, unsig
 	__le32 *dbuf32 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		pix = ((pix & 0x00ff0000) >> 16) <<  0 |
-		      ((pix & 0x0000ff00) >>  8) <<  8 |
-		      ((pix & 0x000000ff) >>  0) << 16 |
-		      ((pix & 0xff000000) >> 24) << 24;
-		*dbuf32++ = cpu_to_le32(pix);
-	}
+	for (x = 0; x < pixels; x++)
+		*dbuf32++ = drm_format_xrgb8888_to_xbgr8888(sbuf32[x]);
 }
 
 static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
@@ -818,17 +1105,9 @@ static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
 	__le32 *dbuf32 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u32 val32;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		val32 = ((pix & 0x000000FF) << 2) |
-			((pix & 0x0000FF00) << 4) |
-			((pix & 0x00FF0000) << 6);
-		pix = val32 | ((val32 >> 8) & 0x00300C03);
-		*dbuf32++ = cpu_to_le32(pix);
-	}
+	for (x = 0; x < pixels; x++)
+		*dbuf32++ = drm_format_xrgb8888_to_xrgb2101010(sbuf32[x]);
 }
 
 /**
@@ -872,18 +1151,9 @@ static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
 	__le32 *dbuf32 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
-	u32 val32;
-	u32 pix;
 
-	for (x = 0; x < pixels; x++) {
-		pix = le32_to_cpu(sbuf32[x]);
-		val32 = ((pix & 0x000000ff) << 2) |
-			((pix & 0x0000ff00) << 4) |
-			((pix & 0x00ff0000) << 6);
-		pix = GENMASK(31, 30) | /* set alpha bits */
-		      val32 | ((val32 >> 8) & 0x00300c03);
-		*dbuf32++ = cpu_to_le32(pix);
-	}
+	for (x = 0; x < pixels; x++)
+		*dbuf32++ = drm_format_xrgb8888_to_argb2101010(sbuf32[x]);
 }
 
 /**
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index f13b34e0b752..f416f0bef52d 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -66,6 +66,7 @@ void *drm_format_conv_state_reserve(struct drm_format_conv_state *state,
 				    size_t new_size, gfp_t flags);
 void drm_format_conv_state_release(struct drm_format_conv_state *state);
 
+u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format);
 unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
 				const struct drm_rect *clip);
 
@@ -76,6 +77,14 @@ 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,
 		 struct drm_format_conv_state *state);
+void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
+			 const u8 *sbuf8, unsigned int spitch,
+			 unsigned int height, unsigned int width,
+			 u32 fg_color, u32 bg_color,
+			 unsigned int pixel_width);
+void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
+		 unsigned int height, unsigned int width,
+		 u32 color, unsigned int pixel_width);
 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, struct drm_format_conv_state *state);
-- 
2.43.2

[PATCH v9 3/9] 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)

v5
 * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
 * Also add drm_fb_fill() to fill area with background color.
 * Add draw_pixel_xy() API for drivers that can't provide a linear buffer.
 * Add a flush() callback for drivers that needs to synchronize the buffer.
 * Add a void *private field, so drivers can pass private data to
   draw_pixel_xy() and flush().

v6
 * Fix sparse warning for panic_msg and logo.

v7
 * Add select DRM_KMS_HELPER for the color conversion functions.

v8
 * Register directly each plane to the panic notifier (Sima)
 * Add raw_spinlock to properly handle concurrency (Sima)
 * Register plane instead of device, to avoid looping through plane
   list, and simplify code.
 * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
  (Thomas Zimmermann)
 * Removed the draw_pixel_xy() API, will see later if it can be added back.

v9
 * Revert to using get_scanout_buffer() (Sima)
 * Move get_scanout_buffer() and panic_flush() to the plane helper
   functions (Thomas Zimmermann)
 * Register all planes with get_scanout_buffer() to the panic notifier
 * Use drm_panic_lock() to protect against race (Sima)

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 Documentation/gpu/drm-kms.rst            |  12 +
 drivers/gpu/drm/Kconfig                  |  23 ++
 drivers/gpu/drm/Makefile                 |   1 +
 drivers/gpu/drm/drm_drv.c                |   4 +
 drivers/gpu/drm/drm_panic.c              | 322 +++++++++++++++++++++++
 drivers/gpu/drm/drm_plane.c              |   1 +
 include/drm/drm_modeset_helper_vtables.h |  37 +++
 include/drm/drm_panic.h                  |  52 ++++
 include/drm/drm_plane.h                  |   5 +
 9 files changed, 457 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_panic.c

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 13d3627d8bc0..b64334661aeb 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -398,6 +398,18 @@ Plane Damage Tracking Functions Reference
 .. kernel-doc:: include/drm/drm_damage_helper.h
    :internal:
 
+Plane Panic Feature
+-------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_panic.c
+   :doc: overview
+
+Plane Panic Functions Reference
+-------------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_panic.c
+   :export:
+
 Display Modes Function Reference
 ================================
 
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 16029435b750..f07ca38d3f98 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -102,6 +102,29 @@ 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 DRM_KMS_HELPER
+	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 a73c04d2d7a3..f9ca4f8fa6c5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -88,6 +88,7 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \
 	drm_privacy_screen.o \
 	drm_privacy_screen_x86.o
 drm-$(CONFIG_DRM_ACCEL) += ../../accel/drm_accel.o
+drm-$(CONFIG_DRM_PANIC) += drm_panic.o
 obj-$(CONFIG_DRM)	+= drm.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 c157500b3135..535b624d4c9d 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>
 
@@ -944,6 +945,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 		if (ret)
 			goto err_unload;
 	}
+	drm_panic_register(dev);
 
 	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
 		 driver->name, driver->major, driver->minor,
@@ -988,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))
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
new file mode 100644
index 000000000000..2e2a033d1267
--- /dev/null
+++ b/drivers/gpu/drm/drm_panic.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+/*
+ * Copyright (c) 2023 Red Hat.
+ * Author: 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/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_framebuffer.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_panic.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_print.h>
+
+
+MODULE_AUTHOR("Jocelyn Falempe");
+MODULE_DESCRIPTION("DRM panic handler");
+MODULE_LICENSE("GPL");
+
+/**
+ * DOC: overview
+ *
+ * To enable DRM panic for a driver, the primary plane must implement a
+ * get_scanout_buffer() helper function. It is then automatically registered to
+ * the drm panic handler.
+ * When a panic occurs, the get_scanout_buffer() will be called, and the driver
+ * can provide a framebuffer so the panic handler can draw the panic screen on
+ * it. The buffer provided must be linear, and use a supported color format.
+ * Optionally the driver can also provide a panic_flush() callback, that will
+ * be called after that, to send additional commands to the hardware to make the
+ * scanout buffer visible.
+ */
+
+/*
+ * 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.
+ */
+
+static inline struct drm_plane *to_drm_plane(struct notifier_block *nb)
+{
+	return container_of(nb, struct drm_plane, panic_notifier);
+}
+
+struct drm_panic_line {
+	u32 len;
+	const char *txt;
+};
+
+#define PANIC_LINE(s) {.len = sizeof(s) - 1, .txt = s}
+
+static 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 */
+};
+
+static const struct drm_panic_line logo[] = {
+	PANIC_LINE("     .--.        _"),
+	PANIC_LINE("    |o_o |      | |"),
+	PANIC_LINE("    |:_/ |      | |"),
+	PANIC_LINE("   //   \\ \\     |_|"),
+	PANIC_LINE("  (|     | )     _"),
+	PANIC_LINE(" /'\\_   _/`\\    (_)"),
+	PANIC_LINE(" \\___)=(___/"),
+};
+
+static void draw_empty_line(struct drm_scanout_buffer *sb, size_t top, size_t height, u32 color)
+{
+	struct iosys_map dst = sb->map;
+
+	iosys_map_incr(&dst, top * sb->pitch);
+	drm_fb_fill(&dst, sb->pitch, height, sb->width, color, sb->format->cpp[0]);
+}
+
+static void draw_txt_line(const struct drm_panic_line *msg, size_t left, size_t top,
+			      struct drm_scanout_buffer *sb, u32 fg_color, u32 bg_color,
+			      const struct font_desc *font)
+{
+	size_t i;
+	const u8 *src;
+	size_t src_stride = DIV_ROUND_UP(font->width, 8);
+	struct iosys_map dst = sb->map;
+	size_t end_text;
+	unsigned int px_width = sb->format->cpp[0];
+
+	iosys_map_incr(&dst, top * sb->pitch);
+	drm_fb_fill(&dst, sb->pitch, font->height, left, bg_color, px_width);
+	iosys_map_incr(&dst, left * px_width);
+	for (i = 0; i < msg->len; i++) {
+		src = font->data + (msg->txt[i] * font->height) * src_stride;
+		drm_fb_blit_from_r1(&dst, sb->pitch, src, src_stride, font->height, font->width,
+				    fg_color, bg_color, px_width);
+		iosys_map_incr(&dst, font->width * px_width);
+	}
+	end_text = (msg->len * font->width) + left;
+	if (sb->width > end_text)
+		drm_fb_fill(&dst, sb->pitch, font->height, sb->width - end_text,
+			    bg_color, px_width);
+}
+
+
+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;
+}
+
+/*
+ * 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, remaining, msgi;
+	size_t 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);
+	u32 fg_color = CONFIG_DRM_PANIC_FOREGROUND_COLOR;
+	u32 bg_color = CONFIG_DRM_PANIC_BACKGROUND_COLOR;
+	const struct font_desc *font = get_default_font(sb->width, sb->height,
+							0x80808080, 0x80808080);
+
+	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);
+
+	fg_color = drm_fb_convert_from_xrgb8888(fg_color, sb->format->format);
+	bg_color = drm_fb_convert_from_xrgb8888(bg_color, sb->format->format);
+
+	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 * font->height, sb, fg_color, bg_color, font);
+		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_line(sb, l * font->height, font->height, bg_color);
+				msgi++;
+			} else if (panic_line.len > chars_per_line) {
+				remaining = panic_line.len - chars_per_line;
+				panic_line.len = chars_per_line;
+				draw_txt_line(&panic_line, 0, l * font->height, sb, fg_color,
+					      bg_color, font);
+				panic_line.txt += chars_per_line;
+				panic_line.len = remaining;
+			} else {
+				draw_txt_line(&panic_line,
+					      font->width * (chars_per_line - panic_line.len) / 2,
+					      l * font->height, sb, fg_color, bg_color, font);
+				panic_line.len = 0;
+				msgi++;
+			}
+		} else {
+			draw_empty_line(sb, l * font->height, font->height, bg_color);
+		}
+	}
+	/* Fill the bottom of the screen, if sb->height is not a multiple of font->height */
+	if (sb->height % font->height)
+		draw_empty_line(sb, l * font->height, sb->height - l * font->height, bg_color);
+}
+
+/*
+ * drm_panic_is_format_supported()
+ * @format: a fourcc color code
+ * Returns: true if supported, false otherwise.
+ *
+ * Check if drm_panic will be able to use this color format.
+ */
+static bool drm_panic_is_format_supported(u32 format)
+{
+	return drm_fb_convert_from_xrgb8888(0xffffff, format) != 0;
+}
+
+static void draw_panic_plane(struct drm_plane *plane, const char *msg)
+{
+	struct drm_scanout_buffer sb;
+	int ret;
+
+	if (!drm_panic_trylock(plane->dev))
+		return;
+
+	ret = plane->helper_private->get_scanout_buffer(plane, &sb);
+
+
+	if (!ret && drm_panic_is_format_supported(sb.format->format)) {
+		draw_panic_static(&sb, msg);
+		if (plane->helper_private->panic_flush)
+			plane->helper_private->panic_flush(plane);
+	}
+	drm_panic_unlock(plane->dev);
+}
+
+static int drm_panic(struct notifier_block *this, unsigned long event,
+		     void *ptr)
+{
+	struct drm_plane *plane = to_drm_plane(this);
+
+	draw_panic_plane(plane, ptr);
+
+	return NOTIFY_OK;
+}
+
+/**
+ * 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_plane *plane;
+	int registered_plane = 0;
+
+	if (!dev->mode_config.num_total_plane)
+		return;
+
+	drm_for_each_plane(plane, dev) {
+		if (!plane->helper_private || !plane->helper_private->get_scanout_buffer)
+			continue;
+		plane->panic_notifier.notifier_call = drm_panic;
+		plane->panic_notifier.priority = -5;
+		if (atomic_notifier_chain_register(&panic_notifier_list,
+						   &plane->panic_notifier)) {
+			drm_warn(dev, "Failed to register panic handler\n");
+		} else {
+			registered_plane++;
+		}
+	}
+	if (registered_plane)
+		drm_info(dev, "Registered %d planes with drm panic\n", registered_plane);
+}
+EXPORT_SYMBOL(drm_panic_register);
+
+/**
+ * drm_panic_unregister()
+ * @dev: the drm device previously registered.
+ */
+void drm_panic_unregister(struct drm_device *dev)
+{
+	struct drm_plane *plane;
+
+	if (!dev->mode_config.num_total_plane)
+		return;
+
+	drm_for_each_plane(plane, dev) {
+		if (!plane->helper_private || !plane->helper_private->get_scanout_buffer)
+			continue;
+
+		atomic_notifier_chain_unregister(&panic_notifier_list,
+						 &plane->panic_notifier);
+	}
+}
+EXPORT_SYMBOL(drm_panic_unregister);
+
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 672c655c7a8e..18af71792e58 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -31,6 +31,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_managed.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_vblank.h>
 
 #include "drm_crtc_internal.h"
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index 881b03e4dc28..bd4752b1ce3f 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -48,6 +48,7 @@
  * To make this clear all the helper vtables are pulled together in this location here.
  */
 
+struct drm_scanout_buffer;
 struct drm_writeback_connector;
 struct drm_writeback_job;
 
@@ -1442,6 +1443,42 @@ struct drm_plane_helper_funcs {
 	 */
 	void (*atomic_async_update)(struct drm_plane *plane,
 				    struct drm_atomic_state *state);
+
+	/**
+	 * @get_scanout_buffer:
+	 *
+	 * Get the current scanout buffer, to display a message with drm_panic.
+	 * The driver should do the minimum changes to provide a linear buffer,
+	 * that can be used to display the panic screen.
+	 * The device dev->mode_config.panic_lock is taken before calling this
+	 * function, so you can safely access the plane->state
+	 * It is called from a panic callback, and must follow its restrictions.
+	 * (no locks, no memory allocation, no sleep, no thread/workqueue, ...)
+	 * It's a best effort mode, so it's expected that in some complex cases
+	 * the panic screen won't be displayed.
+	 * The returned scanout_buffer->map must be valid if no error code is
+	 * returned.
+	 *
+	 * Returns:
+	 *
+	 * Zero on success, negative errno on failure.
+	 */
+	int (*get_scanout_buffer)(struct drm_plane *plane,
+				  struct drm_scanout_buffer *sb);
+
+	/**
+	 * @panic_flush:
+	 *
+	 * It is used by drm_panic, and is called after the panic screen is
+	 * drawn to the scanout buffer. In this function, the driver
+	 * can send additional commands to the hardware, to make the scanout
+	 * buffer visible.
+	 * The device dev->mode_config.panic_lock is taken before calling this
+	 * function, so you can safely access the plane->state
+	 * It is called from a panic callback, and must follow its restrictions.
+	 * (no locks, no memory allocation, no sleep, no thread/workqueue, ...)
+	 */
+	void (*panic_flush)(struct drm_plane *plane);
 };
 
 /**
diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
index f2135d03f1eb..e19adbd0a71a 100644
--- a/include/drm/drm_panic.h
+++ b/include/drm/drm_panic.h
@@ -2,11 +2,51 @@
 #ifndef __DRM_PANIC_H__
 #define __DRM_PANIC_H__
 
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/iosys-map.h>
+
 #include <drm/drm_device.h>
 /*
  * Copyright (c) 2024 Intel
  */
 
+/**
+ * struct drm_scanout_buffer - DRM scanout buffer
+ *
+ * This structure holds the information necessary for drm_panic to draw the
+ * panic screen, and display it.
+ */
+struct drm_scanout_buffer {
+	/**
+	 * @format:
+	 *
+	 * drm format of the scanout buffer.
+	 */
+	const struct drm_format_info *format;
+	/**
+	 * @map:
+	 *
+	 * Virtual address of the scanout buffer, either in memory or iomem.
+	 * The scanout buffer should be in linear format, and can be directly
+	 * sent to the display hardware. Tearing is not an issue for the panic
+	 * screen.
+	 */
+	struct iosys_map map;
+	/**
+	 * @width: Width of the scanout buffer, in pixels.
+	 */
+	unsigned int width;
+	/**
+	 * @height: Height of the scanout buffer, in pixels.
+	 */
+	unsigned int height;
+	/**
+	 * @pitch: Length in bytes between the start of two consecutive lines.
+	 */
+	unsigned int pitch;
+};
+
 /**
  * drm_panic_trylock - try to enter the panic printing critical section
  * @dev: struct drm_device
@@ -96,4 +136,16 @@ static inline void drm_panic_unlock(struct drm_device *dev)
 	raw_spin_unlock(&dev->mode_config.panic_lock);
 }
 
+#ifdef CONFIG_DRM_PANIC
+
+void drm_panic_register(struct drm_device *dev);
+void drm_panic_unregister(struct drm_device *dev);
+
+#else
+
+static inline void drm_panic_register(struct drm_device *dev) {}
+static inline void drm_panic_unregister(struct drm_device *dev) {}
+
+#endif
+
 #endif /* __DRM_PANIC_H__ */
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 641fe298052d..c7119a1c47fe 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -779,6 +779,11 @@ struct drm_plane {
 	 * @hotspot_y_property: property to set mouse hotspot y offset.
 	 */
 	struct drm_property *hotspot_y_property;
+
+	/**
+	 * @panic_notifier: Used to register a panic notifier for this plane
+	 */
+	struct notifier_block panic_notifier;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)
-- 
2.43.2

[PATCH v9 4/9] drm/panic: Add debugfs entry to test without triggering panic.

[Jocelyn Falempe]

Add a debugfs file, so you can test drm_panic without freezing
your machine. This is unsafe, and should be enabled only for
developer or tester.

To display the drm_panic screen on the device 0:
echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0

v9:
 * Create a debugfs file for each plane in the device's debugfs
   directory. This allows to test for each plane of each GPU
   independently.

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/Kconfig     |  9 +++++++++
 drivers/gpu/drm/drm_panic.c | 38 +++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f07ca38d3f98..6e41fbd16b3d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -125,6 +125,15 @@ config DRM_PANIC_BACKGROUND_COLOR
 	depends on DRM_PANIC
 	default 0x000000
 
+config DRM_PANIC_DEBUG
+	bool "Add a debug fs entry to trigger drm_panic"
+	depends on DRM_PANIC && DEBUG_FS
+	help
+	  Add dri/[device]/drm_panic_plane_x in the kernel debugfs, to force the
+	  panic handler to write the panic message to this plane scanout buffer.
+	  This is unsafe and should not be enabled on a production build.
+	  If in doubt, say "N".
+
 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/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 2e2a033d1267..24dc1f1a388f 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -270,6 +270,43 @@ static int drm_panic(struct notifier_block *this, unsigned long event,
 	return NOTIFY_OK;
 }
 
+
+/*
+ * DEBUG FS, This is currently unsafe.
+ * Create one file per plane, so it's possible to debug one plane at a time.
+ */
+#ifdef CONFIG_DRM_PANIC_DEBUG
+#include <linux/debugfs.h>
+
+static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_buf,
+				   size_t count, loff_t *ppos)
+{
+	bool run;
+
+	if (kstrtobool_from_user(user_buf, count, &run) == 0 && run) {
+		struct drm_plane *plane = file->private_data;
+
+		draw_panic_plane(plane, "Test from debugfs");
+	}
+	return count;
+}
+
+static const struct file_operations dbg_drm_panic_ops = {
+	.owner = THIS_MODULE,
+	.write = debugfs_trigger_write,
+	.open = simple_open,
+};
+
+static void debugfs_register_plane(struct drm_plane *plane, int index)
+{
+	char fname[32];
+
+	snprintf(fname, 32, "drm_panic_plane_%d", index);
+	debugfs_create_file(fname, 0200, plane->dev->debugfs_root,
+			    plane, &dbg_drm_panic_ops);
+}
+#endif /* CONFIG_DRM_PANIC_DEBUG */
+
 /**
  * drm_panic_register() - Initialize DRM panic for a device
  * @dev: the drm device on which the panic screen will be displayed.
@@ -291,6 +328,7 @@ void drm_panic_register(struct drm_device *dev)
 						   &plane->panic_notifier)) {
 			drm_warn(dev, "Failed to register panic handler\n");
 		} else {
+			debugfs_register_plane(plane, registered_plane);
 			registered_plane++;
 		}
 	}
-- 
2.43.2

[PATCH v9 5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic

[Jocelyn Falempe]

This was initialy done for imx6, but should work on most drivers
using drm_fb_dma_helper.

v8:
 * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
   (Thomas Zimmermann)

v9:
 * go back to get_scanout_buffer()
 * move get_scanout_buffer() to plane helper functions

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_fb_dma_helper.c | 47 +++++++++++++++++++++++++++++
 include/drm/drm_fb_dma_helper.h     |  4 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
index 3b535ad1b07c..010327069ad4 100644
--- a/drivers/gpu/drm/drm_fb_dma_helper.c
+++ b/drivers/gpu/drm/drm_fb_dma_helper.c
@@ -15,6 +15,7 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_dma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_plane.h>
 #include <linux/dma-mapping.h>
 #include <linux/module.h>
@@ -148,3 +149,49 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
 	}
 }
 EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent);
+
+#if defined(CONFIG_DRM_PANIC)
+/**
+ * @plane: DRM primary plane
+ * @drm_scanout_buffer: scanout buffer for the panic handler
+ * Returns: 0 or negative error code
+ *
+ * Generic get_scanout_buffer() implementation, for drivers that uses the
+ * drm_fb_dma_helper.
+ */
+int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
+				     struct drm_scanout_buffer *sb)
+{
+	struct drm_gem_dma_object *dma_obj;
+	struct drm_framebuffer *fb;
+
+	fb = plane->state->fb;
+	/* Only support linear modifier */
+	if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
+		return -ENODEV;
+
+	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
+
+	/* Buffer should be accessible from the CPU */
+	if (dma_obj->base.import_attach)
+		return -ENODEV;
+
+	/* Buffer should be already mapped to CPU */
+	if (!dma_obj->vaddr)
+		return -ENODEV;
+
+	iosys_map_set_vaddr(&sb->map, dma_obj->vaddr);
+	sb->format = fb->format;
+	sb->height = fb->height;
+	sb->width = fb->width;
+	sb->pitch = fb->pitches[0];
+	return 0;
+}
+#else
+int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
+				     struct drm_scanout_buffer *sb)
+{
+	return -ENODEV;
+}
+#endif
+EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer);
diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h
index d5e036c57801..61f24c2aba2f 100644
--- a/include/drm/drm_fb_dma_helper.h
+++ b/include/drm/drm_fb_dma_helper.h
@@ -7,6 +7,7 @@
 struct drm_device;
 struct drm_framebuffer;
 struct drm_plane_state;
+struct drm_scanout_buffer;
 
 struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb,
 	unsigned int plane);
@@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
 				  struct drm_plane_state *old_state,
 				  struct drm_plane_state *state);
 
+int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
+				     struct drm_scanout_buffer *sb);
+
 #endif
 
-- 
2.43.2

[PATCH v9 6/9] 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.

v8:
 * Replace get_scanout_buffer() with drm_panic_set_buffer()
   (Thomas Zimmermann)

v9:
 * Revert to using get_scanout_buffer() (Sima)
 * move get_scanout_buffer() to plane helper functions (Thomas Zimmermann)

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

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 7ce1c4617675..f498665be8c4 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -25,6 +25,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_probe_helper.h>
 
 #define DRIVER_NAME	"simpledrm"
@@ -671,11 +672,26 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan
 	drm_dev_exit(idx);
 }
 
+static int simpledrm_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
+							     struct drm_scanout_buffer *sb)
+{
+	struct simpledrm_device *sdev = simpledrm_device_of_dev(plane->dev);
+
+	sb->width = sdev->mode.hdisplay;
+	sb->height = sdev->mode.vdisplay;
+	sb->format = sdev->format;
+	sb->pitch = sdev->pitch;
+	sb->map = sdev->screen_base;
+
+	return 0;
+}
+
 static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 	.atomic_check = simpledrm_primary_plane_helper_atomic_check,
 	.atomic_update = simpledrm_primary_plane_helper_atomic_update,
 	.atomic_disable = simpledrm_primary_plane_helper_atomic_disable,
+	.get_scanout_buffer = simpledrm_primary_plane_helper_get_scanout_buffer,
 };
 
 static const struct drm_plane_funcs simpledrm_primary_plane_funcs = {
-- 
2.43.2

[PATCH v9 7/9] 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.

v5:
 * Also check that the plane is visible and primary. (Thomas Zimmermann)

v7:
 * use drm_for_each_primary_visible_plane()

v8:
 * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
   (Thomas Zimmermann)

v9:
 * Revert to using get_scanout_buffer() (Sima)
 * move get_scanout_buffer() to plane helper functions (Thomas Zimmermann)

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  7 ++++++-
 drivers/gpu/drm/mgag200/mgag200_mode.c | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 765e49fd8911..58a0e62eaf18 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -366,6 +366,7 @@ struct drm_crtc_state;
 struct drm_display_mode;
 struct drm_plane;
 struct drm_atomic_state;
+struct drm_scanout_buffer;
 
 extern const uint32_t mgag200_primary_plane_formats[];
 extern const size_t   mgag200_primary_plane_formats_size;
@@ -379,12 +380,16 @@ void mgag200_primary_plane_helper_atomic_enable(struct drm_plane *plane,
 						struct drm_atomic_state *state);
 void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 						 struct drm_atomic_state *old_state);
+int mgag200_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
+						    struct drm_scanout_buffer *sb);
+
 #define MGAG200_PRIMARY_PLANE_HELPER_FUNCS \
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, \
 	.atomic_check = mgag200_primary_plane_helper_atomic_check, \
 	.atomic_update = mgag200_primary_plane_helper_atomic_update, \
 	.atomic_enable = mgag200_primary_plane_helper_atomic_enable, \
-	.atomic_disable = mgag200_primary_plane_helper_atomic_disable
+	.atomic_disable = mgag200_primary_plane_helper_atomic_disable, \
+	.get_scanout_buffer = mgag200_primary_plane_helper_get_scanout_buffer
 
 #define MGAG200_PRIMARY_PLANE_FUNCS \
 	.update_plane = drm_atomic_helper_update_plane, \
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index e17cb4c5f774..8f368ecca4d4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -21,6 +21,7 @@
 #include <drm/drm_framebuffer.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_print.h>
 
 #include "mgag200_drv.h"
@@ -546,6 +547,23 @@ void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 	msleep(20);
 }
 
+int mgag200_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
+						    struct drm_scanout_buffer *sb)
+{
+	struct mga_device *mdev = to_mga_device(plane->dev);
+	struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
+
+	if (plane->state && plane->state->fb) {
+		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;
+}
+
 /*
  * CRTC
  */
-- 
2.43.2

[PATCH v9 8/9] drm/imx: 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.

v7:
 * use drm_panic_gem_get_scanout_buffer() helper

v8:
 * Replace get_scanout_buffer() logic with drm_panic_set_buffer()

v9:
 * Revert to using get_scanout_buffer() (Sima)
 * move get_scanout_buffer() to plane helper functions

Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c
index dade8b59feae..3e21d2a1a124 100644
--- a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c
@@ -772,6 +772,12 @@ static const struct drm_plane_helper_funcs ipu_plane_helper_funcs = {
 	.atomic_disable = ipu_plane_atomic_disable,
 	.atomic_update = ipu_plane_atomic_update,
 };
+static const struct drm_plane_helper_funcs ipu_primary_plane_helper_funcs = {
+	.atomic_check = ipu_plane_atomic_check,
+	.atomic_disable = ipu_plane_atomic_disable,
+	.atomic_update = ipu_plane_atomic_update,
+	.get_scanout_buffer = drm_panic_gem_get_scanout_buffer,
+};
 
 bool ipu_plane_atomic_update_pending(struct drm_plane *plane)
 {
@@ -916,7 +922,10 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
 	ipu_plane->dma = dma;
 	ipu_plane->dp_flow = dp;
 
-	drm_plane_helper_add(&ipu_plane->base, &ipu_plane_helper_funcs);
+	if (type == DRM_PLANE_TYPE_PRIMARY)
+		drm_plane_helper_add(&ipu_plane->base, &ipu_primary_plane_helper_funcs);
+	else
+		drm_plane_helper_add(&ipu_plane->base, &ipu_plane_helper_funcs);
 
 	if (dp == IPU_DP_FLOW_SYNC_BG || dp == IPU_DP_FLOW_SYNC_FG)
 		ret = drm_plane_create_zpos_property(&ipu_plane->base, zpos, 0,
-- 
2.43.2

[PATCH v9 9/9] drm/ast: 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.

v7
 * Use drm_for_each_primary_visible_plane()

v8:
 * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
   (Thomas Zimmermann)
v9:
 * Revert to using get_scanout_buffer() (Sima)
 * move get_scanout_buffer() to plane helper functions

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index a718646a66b8..49f2d8bd3377 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -43,6 +43,7 @@
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_managed.h>
+#include <drm/drm_panic.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
@@ -700,12 +701,29 @@ static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 	ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x1, 0xdf, 0x20);
 }
 
+static int ast_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
+						       struct drm_scanout_buffer *sb)
+{
+	struct ast_plane *ast_plane = to_ast_plane(plane);
+
+	if (plane->state && plane->state->fb && ast_plane->vaddr) {
+		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];
+		iosys_map_set_vaddr_iomem(&sb->map, ast_plane->vaddr);
+		return 0;
+	}
+	return -ENODEV;
+}
+
 static const struct drm_plane_helper_funcs ast_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 	.atomic_check = ast_primary_plane_helper_atomic_check,
 	.atomic_update = ast_primary_plane_helper_atomic_update,
 	.atomic_enable = ast_primary_plane_helper_atomic_enable,
 	.atomic_disable = ast_primary_plane_helper_atomic_disable,
+	.get_scanout_buffer = ast_primary_plane_helper_get_scanout_buffer,
 };
 
 static const struct drm_plane_funcs ast_primary_plane_funcs = {
-- 
2.43.2

Re: [PATCH v9 1/9] drm/panic: Add drm panic locking

[John Ogness]

On 2024-03-07, Jocelyn Falempe <jfalempe@redhat.com> wrote:
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 39ef0a6addeb..c0bb91312fb2 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -38,6 +38,7 @@
>  #include <drm/drm_drv.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_panic.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_self_refresh_helper.h>
>  #include <drm/drm_vblank.h>
> @@ -3099,6 +3100,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		}
>  	}
>  
> +	drm_panic_lock(state->dev);
>  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>  		WARN_ON(plane->state != old_plane_state);
>  
> @@ -3108,6 +3110,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		state->planes[i].state = old_plane_state;
>  		plane->state = new_plane_state;
>  	}
> +	drm_panic_unlock(state->dev);

Is there a reason irqsave/irqrestore variants are not used? Maybe this
code path is too hot?

By leaving interrupts enabled, there is the risk that a panic from
within any interrupt handler may block the drm panic handler.

John Ogness

Re: [PATCH v9 2/9] drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill

[Thomas Zimmermann]

Hi

Am 07.03.24 um 10:14 schrieb Jocelyn Falempe:
> This is needed for drm_panic, to draw the font, and fill
> the background color, in multiple color format.

As you know, I'm not happy about this patch and the the changes do not 
reflect my easier review. These format helpers are supposed to serve all 
of DRM and not just the panic handler. I know that the current code 
isn't perfect, but the R1 support is a step backwards IMHO.

I suggest to the drawing routines entirely within the panic-handler code 
and maybe try to sort out things later. IIRC that's how it was in 
earlier revisions of the patchset.

Best regards
Thomas

>
> v5:
>   * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
>   * Also add drm_fb_fill() to fill area with background color.
> v6:
>   * fix __le32 conversion warning found by the kernel test bot
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/drm_format_helper.c | 432 ++++++++++++++++++++++------
>   include/drm/drm_format_helper.h     |   9 +
>   2 files changed, 360 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index b1be458ed4dd..2d9646cefc4f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -111,6 +111,153 @@ void drm_format_conv_state_release(struct drm_format_conv_state *state)
>   }
>   EXPORT_SYMBOL(drm_format_conv_state_release);
>   
> +static __le16 drm_format_xrgb8888_to_rgb565(__le32 val32)
> +{
> +	u16 val16;
> +	u32 pix;
> +
> +	pix = le32_to_cpu(val32);
> +	val16 = ((pix & 0x00F80000) >> 8) |
> +		((pix & 0x0000FC00) >> 5) |
> +		((pix & 0x000000F8) >> 3);
> +	return cpu_to_le16(val16);
> +}
> +
> +static __le16 drm_format_xrgb8888_to_rgba5551(__le32 val32)
> +{
> +	u16 val16;
> +	u32 pix;
> +
> +	pix = le32_to_cpu(val32);
> +	val16 = ((pix & 0x00f80000) >> 8) |
> +		((pix & 0x0000f800) >> 5) |
> +		((pix & 0x000000f8) >> 2) |
> +		BIT(0); /* set alpha bit */
> +	return cpu_to_le16(val16);
> +}
> +
> +static __le16 drm_format_xrgb8888_to_xrgb1555(__le32 val32)
> +{
> +	u16 val16;
> +	u32 pix;
> +
> +	pix = le32_to_cpu(val32);
> +	val16 = ((pix & 0x00f80000) >> 9) |
> +		((pix & 0x0000f800) >> 6) |
> +		((pix & 0x000000f8) >> 3);
> +	return cpu_to_le16(val16);
> +}
> +
> +static __le16 drm_format_xrgb8888_to_argb1555(__le32 val32)
> +{
> +	u16 val16;
> +	u32 pix;
> +
> +	pix = le32_to_cpu(val32);
> +	val16 = BIT(15) | /* set alpha bit */
> +		((pix & 0x00f80000) >> 9) |
> +		((pix & 0x0000f800) >> 6) |
> +		((pix & 0x000000f8) >> 3);
> +	return cpu_to_le16(val16);
> +}
> +
> +static __le32 drm_format_xrgb8888_to_argb8888(__le32 pix)
> +{
> +	u32 val32;
> +
> +	val32 = le32_to_cpu(pix);
> +	val32 |= GENMASK(31, 24); /* fill alpha bits */
> +	return cpu_to_le32(val32);
> +}
> +
> +static __le32 drm_format_xrgb8888_to_xbgr8888(__le32 pix)
> +{
> +	u32 val32;
> +
> +	val32 = le32_to_cpu(pix);
> +	val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
> +		((val32 & 0x0000ff00) >>  8) <<  8 |
> +		((val32 & 0x000000ff) >>  0) << 16 |
> +		((val32 & 0xff000000) >> 24) << 24;
> +	return cpu_to_le32(val32);
> +}
> +
> +static __le32 drm_format_xrgb8888_to_abgr8888(__le32 pix)
> +{
> +	u32 val32;
> +
> +	val32 = le32_to_cpu(pix);
> +	val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
> +		((val32 & 0x0000ff00) >>  8) <<  8 |
> +		((val32 & 0x000000ff) >>  0) << 16 |
> +		GENMASK(31, 24); /* fill alpha bits */
> +	return cpu_to_le32(val32);
> +}
> +
> +static __le32 drm_format_xrgb8888_to_xrgb2101010(__le32 pix)
> +{
> +	u32 val32;
> +
> +	val32 = le32_to_cpu(pix);
> +	val32 = ((val32 & 0x000000FF) << 2) |
> +		((val32 & 0x0000FF00) << 4) |
> +		((val32 & 0x00FF0000) << 6);
> +	return cpu_to_le32(val32 | ((val32 >> 8) & 0x00300C03));
> +}
> +
> +static __le32 drm_format_xrgb8888_to_argb2101010(__le32 pix)
> +{
> +	u32 val32;
> +
> +	val32 = le32_to_cpu(pix);
> +	val32 = ((val32 & 0x000000FF) << 2) |
> +		((val32 & 0x0000FF00) << 4) |
> +		((val32 & 0x00FF0000) << 6);
> +	val32 = GENMASK(31, 30) | /* set alpha bits */
> +	      val32 | ((val32 >> 8) & 0x00300c03);
> +	return cpu_to_le32(val32);
> +}
> +
> +/**
> + * drm_fb_convert_from_xrgb8888 - convert one pixel from xrgb8888 to the desired format
> + * @color: input color, in xrgb8888 format
> + * @format: output format
> + *
> + * Returns:
> + * Color in the format specified, casted to u32.
> + * Or 0 if the format is unknown.
> + */
> +u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format)
> +{
> +	__le32 pix = cpu_to_le32(color);
> +
> +	switch (format) {
> +	case DRM_FORMAT_RGB565:
> +		return le16_to_cpu(drm_format_xrgb8888_to_rgb565(pix));
> +	case DRM_FORMAT_RGBA5551:
> +		return le16_to_cpu(drm_format_xrgb8888_to_rgba5551(pix));
> +	case DRM_FORMAT_XRGB1555:
> +		return le16_to_cpu(drm_format_xrgb8888_to_xrgb1555(pix));
> +	case DRM_FORMAT_ARGB1555:
> +		return le16_to_cpu(drm_format_xrgb8888_to_argb1555(pix));
> +	case DRM_FORMAT_RGB888:
> +	case DRM_FORMAT_XRGB8888:
> +		return le32_to_cpu(pix);
> +	case DRM_FORMAT_ARGB8888:
> +		return le32_to_cpu(drm_format_xrgb8888_to_argb8888(pix));
> +	case DRM_FORMAT_XBGR8888:
> +		return le32_to_cpu(drm_format_xrgb8888_to_xbgr8888(pix));
> +	case DRM_FORMAT_XRGB2101010:
> +		return le32_to_cpu(drm_format_xrgb8888_to_xrgb2101010(pix));
> +	case DRM_FORMAT_ARGB2101010:
> +		return le32_to_cpu(drm_format_xrgb8888_to_argb2101010(pix));
> +	default:
> +		WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
> +		return 0;
> +	}
> +}
> +EXPORT_SYMBOL(drm_fb_convert_from_xrgb8888);
> +
>   static unsigned int clip_offset(const struct drm_rect *clip, unsigned int pitch, unsigned int cpp)
>   {
>   	return clip->y1 * pitch + clip->x1 * cpp;
> @@ -366,6 +513,193 @@ void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>   }
>   EXPORT_SYMBOL(drm_fb_swab);
>   
> +static void drm_fb_r1_to_16bit(struct iosys_map *dmap, unsigned int dpitch,
> +			       const u8 *sbuf8, unsigned int spitch,
> +			       unsigned int height, unsigned int width,
> +			       __le16 fg16, __le16 bg16)
> +{
> +	unsigned int l, x;
> +	__le16 val16;
> +
> +	for (l = 0; l < height; l++) {
> +		for (x = 0; x < width; x++) {
> +			val16 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg16 : bg16;
> +			iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, le16_to_cpu(val16));
> +		}
> +	}
> +}
> +
> +static void drm_fb_r1_to_24bit(struct iosys_map *dmap, unsigned int dpitch,
> +			       const u8 *sbuf8, unsigned int spitch,
> +			       unsigned int height, unsigned int width,
> +			       __le32 fg32, __le32 bg32)
> +{
> +	unsigned int l, x;
> +	__le32 color;
> +	u32 val32;
> +
> +	for (l = 0; l < height; l++) {
> +		for (x = 0; x < width; x++) {
> +			u32 off = l * dpitch + x * 3;
> +
> +			color = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32;
> +			val32 = le32_to_cpu(color);
> +
> +			/* write blue-green-red to output in little endianness */
> +			iosys_map_wr(dmap, off, u8, (val32 & 0x000000FF) >> 0);
> +			iosys_map_wr(dmap, off + 1, u8, (val32 & 0x0000FF00) >> 8);
> +			iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF0000) >> 16);
> +		}
> +	}
> +}
> +
> +static void drm_fb_r1_to_32bit(struct iosys_map *dmap, unsigned int dpitch,
> +			       const u8 *sbuf8, unsigned int spitch,
> +			       unsigned int height, unsigned int width,
> +			       __le32 fg32, __le32 bg32)
> +{
> +	unsigned int l, x;
> +	__le32 val32;
> +
> +	for (l = 0; l < height; l++) {
> +		for (x = 0; x < width; x++) {
> +			val32 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32;
> +			iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, le32_to_cpu(val32));
> +		}
> +	}
> +}
> +
> +/**
> + * drm_fb_blit_from_r1 - convert a monochrome image to a linear framebuffer
> + * @dmap: destination iosys_map
> + * @dpitch: destination pitch in bytes
> + * @sbuf8: source buffer, in monochrome format, 8 pixels per byte.
> + * @spitch: source pitch in bytes
> + * @height: height of the image to copy, in pixels
> + * @width: width of the image to copy, in pixels
> + * @fg_color: foreground color, in destination format
> + * @bg_color: background color, in destination format
> + * @pixel_width: pixel width in bytes.
> + *
> + * This can be used to draw font which are monochrome images, to a framebuffer
> + * in other supported format.
> + */
> +void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
> +			 const u8 *sbuf8, unsigned int spitch,
> +			 unsigned int height, unsigned int width,
> +			 u32 fg_color, u32 bg_color,
> +			 unsigned int pixel_width)
> +{
> +	switch (pixel_width) {
> +	case 2:
> +		drm_fb_r1_to_16bit(dmap, dpitch, sbuf8, spitch,
> +				   height, width,
> +				   cpu_to_le16(fg_color),
> +				   cpu_to_le16(bg_color));
> +	break;
> +	case 3:
> +		drm_fb_r1_to_24bit(dmap, dpitch, sbuf8, spitch,
> +				   height, width,
> +				   cpu_to_le32(fg_color),
> +				   cpu_to_le32(bg_color));
> +	break;
> +	case 4:
> +		drm_fb_r1_to_32bit(dmap, dpitch, sbuf8, spitch,
> +				   height, width,
> +				   cpu_to_le32(fg_color),
> +				   cpu_to_le32(bg_color));
> +	break;
> +	default:
> +		WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width);
> +	}
> +}
> +EXPORT_SYMBOL(drm_fb_blit_from_r1);
> +
> +static void drm_fb_fill8(struct iosys_map *dmap, unsigned int dpitch,
> +			 unsigned int height, unsigned int width,
> +			 u8 color)
> +{
> +	unsigned int l, x;
> +
> +	for (l = 0; l < height; l++)
> +		for (x = 0; x < width; x++)
> +			iosys_map_wr(dmap, l * dpitch + x * sizeof(u8), u8, color);
> +}
> +
> +static void drm_fb_fill16(struct iosys_map *dmap, unsigned int dpitch,
> +			  unsigned int height, unsigned int width,
> +			  u16 color)
> +{
> +	unsigned int l, x;
> +
> +	for (l = 0; l < height; l++)
> +		for (x = 0; x < width; x++)
> +			iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, color);
> +}
> +
> +static void drm_fb_fill24(struct iosys_map *dmap, unsigned int dpitch,
> +			  unsigned int height, unsigned int width,
> +			  u32 color)
> +{
> +	unsigned int l, x;
> +
> +	for (l = 0; l < height; l++) {
> +		for (x = 0; x < width; x++) {
> +			unsigned int off = l * dpitch + x * 3;
> +
> +			/* write blue-green-red to output in little endianness */
> +			iosys_map_wr(dmap, off, u8, (color & 0x000000FF) >> 0);
> +			iosys_map_wr(dmap, off + 1, u8, (color & 0x0000FF00) >> 8);
> +			iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 16);
> +		}
> +	}
> +}
> +
> +static void drm_fb_fill32(struct iosys_map *dmap, unsigned int dpitch,
> +			  unsigned int height, unsigned int width,
> +			  u32 color)
> +{
> +	unsigned int l, x;
> +
> +	for (l = 0; l < height; l++)
> +		for (x = 0; x < width; x++)
> +			iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, color);
> +}
> +
> +/**
> + * drm_fb_fill - Fill a rectangle with a color
> + * @dmap: destination iosys_map, pointing to the top left corner of the rectangle
> + * @dpitch: destination pitch in bytes
> + * @height: height of the rectangle, in pixels
> + * @width: width of the rectangle, in pixels
> + * @color: color to fill the rectangle.
> + * @pixel_width: pixel width in bytes
> + *
> + * Fill a rectangle with a color, in a linear framebuffer.
> + */
> +void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
> +			 unsigned int height, unsigned int width,
> +			 u32 color, unsigned int pixel_width)
> +{
> +	switch (pixel_width) {
> +	case 1:
> +		drm_fb_fill8(dmap, dpitch, height, width, color);
> +	break;
> +	case 2:
> +		drm_fb_fill16(dmap, dpitch, height, width, color);
> +	break;
> +	case 3:
> +		drm_fb_fill24(dmap, dpitch, height, width, color);
> +	break;
> +	case 4:
> +		drm_fb_fill32(dmap, dpitch, height, width, color);
> +	break;
> +	default:
> +		WARN_ONCE(1, "Can't fill with pixel width %d\n", pixel_width);
> +	}
> +}
> +EXPORT_SYMBOL(drm_fb_fill);
> +
>   static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void *sbuf, unsigned int pixels)
>   {
>   	u8 *dbuf8 = dbuf;
> @@ -420,15 +754,9 @@ static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigne
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u16 val16;
> -	u32 pix;
>   
>   	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		val16 = ((pix & 0x00F80000) >> 8) |
> -			((pix & 0x0000FC00) >> 5) |
> -			((pix & 0x000000F8) >> 3);
> -		dbuf16[x] = cpu_to_le16(val16);
> +		dbuf16[x] = drm_format_xrgb8888_to_rgb565(sbuf32[x]);
>   	}
>   }
>   
> @@ -498,16 +826,9 @@ static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u16 val16;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		val16 = ((pix & 0x00f80000) >> 9) |
> -			((pix & 0x0000f800) >> 6) |
> -			((pix & 0x000000f8) >> 3);
> -		dbuf16[x] = cpu_to_le16(val16);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		dbuf16[x] = drm_format_xrgb8888_to_xrgb1555(sbuf32[x]);
>   }
>   
>   /**
> @@ -550,17 +871,9 @@ static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u16 val16;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		val16 = BIT(15) | /* set alpha bit */
> -			((pix & 0x00f80000) >> 9) |
> -			((pix & 0x0000f800) >> 6) |
> -			((pix & 0x000000f8) >> 3);
> -		dbuf16[x] = cpu_to_le16(val16);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		dbuf16[x] = drm_format_xrgb8888_to_argb1555(sbuf32[x]);
>   }
>   
>   /**
> @@ -603,17 +916,9 @@ static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
>   	__le16 *dbuf16 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u16 val16;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		val16 = ((pix & 0x00f80000) >> 8) |
> -			((pix & 0x0000f800) >> 5) |
> -			((pix & 0x000000f8) >> 2) |
> -			BIT(0); /* set alpha bit */
> -		dbuf16[x] = cpu_to_le16(val16);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		dbuf16[x] = drm_format_xrgb8888_to_rgba5551(sbuf32[x]);
>   }
>   
>   /**
> @@ -707,13 +1012,9 @@ static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsig
>   	__le32 *dbuf32 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		pix |= GENMASK(31, 24); /* fill alpha bits */
> -		dbuf32[x] = cpu_to_le32(pix);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		dbuf32[x] = drm_format_xrgb8888_to_argb8888(sbuf32[x]);
>   }
>   
>   /**
> @@ -756,16 +1057,9 @@ static void drm_fb_xrgb8888_to_abgr8888_line(void *dbuf, const void *sbuf, unsig
>   	__le32 *dbuf32 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		pix = ((pix & 0x00ff0000) >> 16) <<  0 |
> -		      ((pix & 0x0000ff00) >>  8) <<  8 |
> -		      ((pix & 0x000000ff) >>  0) << 16 |
> -		      GENMASK(31, 24); /* fill alpha bits */
> -		*dbuf32++ = cpu_to_le32(pix);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		*dbuf32++ = drm_format_xrgb8888_to_abgr8888(sbuf32[x]);
>   }
>   
>   static void drm_fb_xrgb8888_to_abgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
> @@ -787,16 +1081,9 @@ static void drm_fb_xrgb8888_to_xbgr8888_line(void *dbuf, const void *sbuf, unsig
>   	__le32 *dbuf32 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		pix = ((pix & 0x00ff0000) >> 16) <<  0 |
> -		      ((pix & 0x0000ff00) >>  8) <<  8 |
> -		      ((pix & 0x000000ff) >>  0) << 16 |
> -		      ((pix & 0xff000000) >> 24) << 24;
> -		*dbuf32++ = cpu_to_le32(pix);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		*dbuf32++ = drm_format_xrgb8888_to_xbgr8888(sbuf32[x]);
>   }
>   
>   static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, const unsigned int *dst_pitch,
> @@ -818,17 +1105,9 @@ static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
>   	__le32 *dbuf32 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u32 val32;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		val32 = ((pix & 0x000000FF) << 2) |
> -			((pix & 0x0000FF00) << 4) |
> -			((pix & 0x00FF0000) << 6);
> -		pix = val32 | ((val32 >> 8) & 0x00300C03);
> -		*dbuf32++ = cpu_to_le32(pix);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		*dbuf32++ = drm_format_xrgb8888_to_xrgb2101010(sbuf32[x]);
>   }
>   
>   /**
> @@ -872,18 +1151,9 @@ static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
>   	__le32 *dbuf32 = dbuf;
>   	const __le32 *sbuf32 = sbuf;
>   	unsigned int x;
> -	u32 val32;
> -	u32 pix;
>   
> -	for (x = 0; x < pixels; x++) {
> -		pix = le32_to_cpu(sbuf32[x]);
> -		val32 = ((pix & 0x000000ff) << 2) |
> -			((pix & 0x0000ff00) << 4) |
> -			((pix & 0x00ff0000) << 6);
> -		pix = GENMASK(31, 30) | /* set alpha bits */
> -		      val32 | ((val32 >> 8) & 0x00300c03);
> -		*dbuf32++ = cpu_to_le32(pix);
> -	}
> +	for (x = 0; x < pixels; x++)
> +		*dbuf32++ = drm_format_xrgb8888_to_argb2101010(sbuf32[x]);
>   }
>   
>   /**
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index f13b34e0b752..f416f0bef52d 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -66,6 +66,7 @@ void *drm_format_conv_state_reserve(struct drm_format_conv_state *state,
>   				    size_t new_size, gfp_t flags);
>   void drm_format_conv_state_release(struct drm_format_conv_state *state);
>   
> +u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format);
>   unsigned int drm_fb_clip_offset(unsigned int pitch, const struct drm_format_info *format,
>   				const struct drm_rect *clip);
>   
> @@ -76,6 +77,14 @@ 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,
>   		 struct drm_format_conv_state *state);
> +void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
> +			 const u8 *sbuf8, unsigned int spitch,
> +			 unsigned int height, unsigned int width,
> +			 u32 fg_color, u32 bg_color,
> +			 unsigned int pixel_width);
> +void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
> +		 unsigned int height, unsigned int width,
> +		 u32 color, unsigned int pixel_width);
>   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, struct drm_format_conv_state *state);

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

Re: [PATCH v9 2/9] drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill

[Thomas Zimmermann]

Am 07.03.24 um 15:08 schrieb Thomas Zimmermann:
> Hi
>
> Am 07.03.24 um 10:14 schrieb Jocelyn Falempe:
>> This is needed for drm_panic, to draw the font, and fill
>> the background color, in multiple color format.
>
> As you know, I'm not happy about this patch and the the changes do not 
> reflect my easier

s/easier/earlier

> review. These format helpers are supposed to serve all of DRM and not 
> just the panic handler. I know that the current code isn't perfect, 
> but the R1 support is a step backwards IMHO.
>
> I suggest to the drawing routines entirely within the panic-handler 
> code and maybe try to sort out things later. IIRC that's how it was in 
> earlier revisions of the patchset.
>
> Best regards
> Thomas
>
>>
>> v5:
>>   * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
>>   * Also add drm_fb_fill() to fill area with background color.
>> v6:
>>   * fix __le32 conversion warning found by the kernel test bot
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_format_helper.c | 432 ++++++++++++++++++++++------
>>   include/drm/drm_format_helper.h     |   9 +
>>   2 files changed, 360 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c 
>> b/drivers/gpu/drm/drm_format_helper.c
>> index b1be458ed4dd..2d9646cefc4f 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -111,6 +111,153 @@ void drm_format_conv_state_release(struct 
>> drm_format_conv_state *state)
>>   }
>>   EXPORT_SYMBOL(drm_format_conv_state_release);
>>   +static __le16 drm_format_xrgb8888_to_rgb565(__le32 val32)
>> +{
>> +    u16 val16;
>> +    u32 pix;
>> +
>> +    pix = le32_to_cpu(val32);
>> +    val16 = ((pix & 0x00F80000) >> 8) |
>> +        ((pix & 0x0000FC00) >> 5) |
>> +        ((pix & 0x000000F8) >> 3);
>> +    return cpu_to_le16(val16);
>> +}
>> +
>> +static __le16 drm_format_xrgb8888_to_rgba5551(__le32 val32)
>> +{
>> +    u16 val16;
>> +    u32 pix;
>> +
>> +    pix = le32_to_cpu(val32);
>> +    val16 = ((pix & 0x00f80000) >> 8) |
>> +        ((pix & 0x0000f800) >> 5) |
>> +        ((pix & 0x000000f8) >> 2) |
>> +        BIT(0); /* set alpha bit */
>> +    return cpu_to_le16(val16);
>> +}
>> +
>> +static __le16 drm_format_xrgb8888_to_xrgb1555(__le32 val32)
>> +{
>> +    u16 val16;
>> +    u32 pix;
>> +
>> +    pix = le32_to_cpu(val32);
>> +    val16 = ((pix & 0x00f80000) >> 9) |
>> +        ((pix & 0x0000f800) >> 6) |
>> +        ((pix & 0x000000f8) >> 3);
>> +    return cpu_to_le16(val16);
>> +}
>> +
>> +static __le16 drm_format_xrgb8888_to_argb1555(__le32 val32)
>> +{
>> +    u16 val16;
>> +    u32 pix;
>> +
>> +    pix = le32_to_cpu(val32);
>> +    val16 = BIT(15) | /* set alpha bit */
>> +        ((pix & 0x00f80000) >> 9) |
>> +        ((pix & 0x0000f800) >> 6) |
>> +        ((pix & 0x000000f8) >> 3);
>> +    return cpu_to_le16(val16);
>> +}
>> +
>> +static __le32 drm_format_xrgb8888_to_argb8888(__le32 pix)
>> +{
>> +    u32 val32;
>> +
>> +    val32 = le32_to_cpu(pix);
>> +    val32 |= GENMASK(31, 24); /* fill alpha bits */
>> +    return cpu_to_le32(val32);
>> +}
>> +
>> +static __le32 drm_format_xrgb8888_to_xbgr8888(__le32 pix)
>> +{
>> +    u32 val32;
>> +
>> +    val32 = le32_to_cpu(pix);
>> +    val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
>> +        ((val32 & 0x0000ff00) >>  8) <<  8 |
>> +        ((val32 & 0x000000ff) >>  0) << 16 |
>> +        ((val32 & 0xff000000) >> 24) << 24;
>> +    return cpu_to_le32(val32);
>> +}
>> +
>> +static __le32 drm_format_xrgb8888_to_abgr8888(__le32 pix)
>> +{
>> +    u32 val32;
>> +
>> +    val32 = le32_to_cpu(pix);
>> +    val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
>> +        ((val32 & 0x0000ff00) >>  8) <<  8 |
>> +        ((val32 & 0x000000ff) >>  0) << 16 |
>> +        GENMASK(31, 24); /* fill alpha bits */
>> +    return cpu_to_le32(val32);
>> +}
>> +
>> +static __le32 drm_format_xrgb8888_to_xrgb2101010(__le32 pix)
>> +{
>> +    u32 val32;
>> +
>> +    val32 = le32_to_cpu(pix);
>> +    val32 = ((val32 & 0x000000FF) << 2) |
>> +        ((val32 & 0x0000FF00) << 4) |
>> +        ((val32 & 0x00FF0000) << 6);
>> +    return cpu_to_le32(val32 | ((val32 >> 8) & 0x00300C03));
>> +}
>> +
>> +static __le32 drm_format_xrgb8888_to_argb2101010(__le32 pix)
>> +{
>> +    u32 val32;
>> +
>> +    val32 = le32_to_cpu(pix);
>> +    val32 = ((val32 & 0x000000FF) << 2) |
>> +        ((val32 & 0x0000FF00) << 4) |
>> +        ((val32 & 0x00FF0000) << 6);
>> +    val32 = GENMASK(31, 30) | /* set alpha bits */
>> +          val32 | ((val32 >> 8) & 0x00300c03);
>> +    return cpu_to_le32(val32);
>> +}
>> +
>> +/**
>> + * drm_fb_convert_from_xrgb8888 - convert one pixel from xrgb8888 to 
>> the desired format
>> + * @color: input color, in xrgb8888 format
>> + * @format: output format
>> + *
>> + * Returns:
>> + * Color in the format specified, casted to u32.
>> + * Or 0 if the format is unknown.
>> + */
>> +u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format)
>> +{
>> +    __le32 pix = cpu_to_le32(color);
>> +
>> +    switch (format) {
>> +    case DRM_FORMAT_RGB565:
>> +        return le16_to_cpu(drm_format_xrgb8888_to_rgb565(pix));
>> +    case DRM_FORMAT_RGBA5551:
>> +        return le16_to_cpu(drm_format_xrgb8888_to_rgba5551(pix));
>> +    case DRM_FORMAT_XRGB1555:
>> +        return le16_to_cpu(drm_format_xrgb8888_to_xrgb1555(pix));
>> +    case DRM_FORMAT_ARGB1555:
>> +        return le16_to_cpu(drm_format_xrgb8888_to_argb1555(pix));
>> +    case DRM_FORMAT_RGB888:
>> +    case DRM_FORMAT_XRGB8888:
>> +        return le32_to_cpu(pix);
>> +    case DRM_FORMAT_ARGB8888:
>> +        return le32_to_cpu(drm_format_xrgb8888_to_argb8888(pix));
>> +    case DRM_FORMAT_XBGR8888:
>> +        return le32_to_cpu(drm_format_xrgb8888_to_xbgr8888(pix));
>> +    case DRM_FORMAT_XRGB2101010:
>> +        return le32_to_cpu(drm_format_xrgb8888_to_xrgb2101010(pix));
>> +    case DRM_FORMAT_ARGB2101010:
>> +        return le32_to_cpu(drm_format_xrgb8888_to_argb2101010(pix));
>> +    default:
>> +        WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
>> +        return 0;
>> +    }
>> +}
>> +EXPORT_SYMBOL(drm_fb_convert_from_xrgb8888);
>> +
>>   static unsigned int clip_offset(const struct drm_rect *clip, 
>> unsigned int pitch, unsigned int cpp)
>>   {
>>       return clip->y1 * pitch + clip->x1 * cpp;
>> @@ -366,6 +513,193 @@ void drm_fb_swab(struct iosys_map *dst, const 
>> unsigned int *dst_pitch,
>>   }
>>   EXPORT_SYMBOL(drm_fb_swab);
>>   +static void drm_fb_r1_to_16bit(struct iosys_map *dmap, unsigned 
>> int dpitch,
>> +                   const u8 *sbuf8, unsigned int spitch,
>> +                   unsigned int height, unsigned int width,
>> +                   __le16 fg16, __le16 bg16)
>> +{
>> +    unsigned int l, x;
>> +    __le16 val16;
>> +
>> +    for (l = 0; l < height; l++) {
>> +        for (x = 0; x < width; x++) {
>> +            val16 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 
>> 8))) ? fg16 : bg16;
>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, 
>> le16_to_cpu(val16));
>> +        }
>> +    }
>> +}
>> +
>> +static void drm_fb_r1_to_24bit(struct iosys_map *dmap, unsigned int 
>> dpitch,
>> +                   const u8 *sbuf8, unsigned int spitch,
>> +                   unsigned int height, unsigned int width,
>> +                   __le32 fg32, __le32 bg32)
>> +{
>> +    unsigned int l, x;
>> +    __le32 color;
>> +    u32 val32;
>> +
>> +    for (l = 0; l < height; l++) {
>> +        for (x = 0; x < width; x++) {
>> +            u32 off = l * dpitch + x * 3;
>> +
>> +            color = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 
>> 8))) ? fg32 : bg32;
>> +            val32 = le32_to_cpu(color);
>> +
>> +            /* write blue-green-red to output in little endianness */
>> +            iosys_map_wr(dmap, off, u8, (val32 & 0x000000FF) >> 0);
>> +            iosys_map_wr(dmap, off + 1, u8, (val32 & 0x0000FF00) >> 8);
>> +            iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF0000) >> 
>> 16);
>> +        }
>> +    }
>> +}
>> +
>> +static void drm_fb_r1_to_32bit(struct iosys_map *dmap, unsigned int 
>> dpitch,
>> +                   const u8 *sbuf8, unsigned int spitch,
>> +                   unsigned int height, unsigned int width,
>> +                   __le32 fg32, __le32 bg32)
>> +{
>> +    unsigned int l, x;
>> +    __le32 val32;
>> +
>> +    for (l = 0; l < height; l++) {
>> +        for (x = 0; x < width; x++) {
>> +            val32 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 
>> 8))) ? fg32 : bg32;
>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, 
>> le32_to_cpu(val32));
>> +        }
>> +    }
>> +}
>> +
>> +/**
>> + * drm_fb_blit_from_r1 - convert a monochrome image to a linear 
>> framebuffer
>> + * @dmap: destination iosys_map
>> + * @dpitch: destination pitch in bytes
>> + * @sbuf8: source buffer, in monochrome format, 8 pixels per byte.
>> + * @spitch: source pitch in bytes
>> + * @height: height of the image to copy, in pixels
>> + * @width: width of the image to copy, in pixels
>> + * @fg_color: foreground color, in destination format
>> + * @bg_color: background color, in destination format
>> + * @pixel_width: pixel width in bytes.
>> + *
>> + * This can be used to draw font which are monochrome images, to a 
>> framebuffer
>> + * in other supported format.
>> + */
>> +void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
>> +             const u8 *sbuf8, unsigned int spitch,
>> +             unsigned int height, unsigned int width,
>> +             u32 fg_color, u32 bg_color,
>> +             unsigned int pixel_width)
>> +{
>> +    switch (pixel_width) {
>> +    case 2:
>> +        drm_fb_r1_to_16bit(dmap, dpitch, sbuf8, spitch,
>> +                   height, width,
>> +                   cpu_to_le16(fg_color),
>> +                   cpu_to_le16(bg_color));
>> +    break;
>> +    case 3:
>> +        drm_fb_r1_to_24bit(dmap, dpitch, sbuf8, spitch,
>> +                   height, width,
>> +                   cpu_to_le32(fg_color),
>> +                   cpu_to_le32(bg_color));
>> +    break;
>> +    case 4:
>> +        drm_fb_r1_to_32bit(dmap, dpitch, sbuf8, spitch,
>> +                   height, width,
>> +                   cpu_to_le32(fg_color),
>> +                   cpu_to_le32(bg_color));
>> +    break;
>> +    default:
>> +        WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width);
>> +    }
>> +}
>> +EXPORT_SYMBOL(drm_fb_blit_from_r1);
>> +
>> +static void drm_fb_fill8(struct iosys_map *dmap, unsigned int dpitch,
>> +             unsigned int height, unsigned int width,
>> +             u8 color)
>> +{
>> +    unsigned int l, x;
>> +
>> +    for (l = 0; l < height; l++)
>> +        for (x = 0; x < width; x++)
>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u8), u8, color);
>> +}
>> +
>> +static void drm_fb_fill16(struct iosys_map *dmap, unsigned int dpitch,
>> +              unsigned int height, unsigned int width,
>> +              u16 color)
>> +{
>> +    unsigned int l, x;
>> +
>> +    for (l = 0; l < height; l++)
>> +        for (x = 0; x < width; x++)
>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, 
>> color);
>> +}
>> +
>> +static void drm_fb_fill24(struct iosys_map *dmap, unsigned int dpitch,
>> +              unsigned int height, unsigned int width,
>> +              u32 color)
>> +{
>> +    unsigned int l, x;
>> +
>> +    for (l = 0; l < height; l++) {
>> +        for (x = 0; x < width; x++) {
>> +            unsigned int off = l * dpitch + x * 3;
>> +
>> +            /* write blue-green-red to output in little endianness */
>> +            iosys_map_wr(dmap, off, u8, (color & 0x000000FF) >> 0);
>> +            iosys_map_wr(dmap, off + 1, u8, (color & 0x0000FF00) >> 8);
>> +            iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 
>> 16);
>> +        }
>> +    }
>> +}
>> +
>> +static void drm_fb_fill32(struct iosys_map *dmap, unsigned int dpitch,
>> +              unsigned int height, unsigned int width,
>> +              u32 color)
>> +{
>> +    unsigned int l, x;
>> +
>> +    for (l = 0; l < height; l++)
>> +        for (x = 0; x < width; x++)
>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, 
>> color);
>> +}
>> +
>> +/**
>> + * drm_fb_fill - Fill a rectangle with a color
>> + * @dmap: destination iosys_map, pointing to the top left corner of 
>> the rectangle
>> + * @dpitch: destination pitch in bytes
>> + * @height: height of the rectangle, in pixels
>> + * @width: width of the rectangle, in pixels
>> + * @color: color to fill the rectangle.
>> + * @pixel_width: pixel width in bytes
>> + *
>> + * Fill a rectangle with a color, in a linear framebuffer.
>> + */
>> +void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
>> +             unsigned int height, unsigned int width,
>> +             u32 color, unsigned int pixel_width)
>> +{
>> +    switch (pixel_width) {
>> +    case 1:
>> +        drm_fb_fill8(dmap, dpitch, height, width, color);
>> +    break;
>> +    case 2:
>> +        drm_fb_fill16(dmap, dpitch, height, width, color);
>> +    break;
>> +    case 3:
>> +        drm_fb_fill24(dmap, dpitch, height, width, color);
>> +    break;
>> +    case 4:
>> +        drm_fb_fill32(dmap, dpitch, height, width, color);
>> +    break;
>> +    default:
>> +        WARN_ONCE(1, "Can't fill with pixel width %d\n", pixel_width);
>> +    }
>> +}
>> +EXPORT_SYMBOL(drm_fb_fill);
>> +
>>   static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void 
>> *sbuf, unsigned int pixels)
>>   {
>>       u8 *dbuf8 = dbuf;
>> @@ -420,15 +754,9 @@ static void drm_fb_xrgb8888_to_rgb565_line(void 
>> *dbuf, const void *sbuf, unsigne
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u16 val16;
>> -    u32 pix;
>>         for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        val16 = ((pix & 0x00F80000) >> 8) |
>> -            ((pix & 0x0000FC00) >> 5) |
>> -            ((pix & 0x000000F8) >> 3);
>> -        dbuf16[x] = cpu_to_le16(val16);
>> +        dbuf16[x] = drm_format_xrgb8888_to_rgb565(sbuf32[x]);
>>       }
>>   }
>>   @@ -498,16 +826,9 @@ static void 
>> drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u16 val16;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        val16 = ((pix & 0x00f80000) >> 9) |
>> -            ((pix & 0x0000f800) >> 6) |
>> -            ((pix & 0x000000f8) >> 3);
>> -        dbuf16[x] = cpu_to_le16(val16);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        dbuf16[x] = drm_format_xrgb8888_to_xrgb1555(sbuf32[x]);
>>   }
>>     /**
>> @@ -550,17 +871,9 @@ static void 
>> drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u16 val16;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        val16 = BIT(15) | /* set alpha bit */
>> -            ((pix & 0x00f80000) >> 9) |
>> -            ((pix & 0x0000f800) >> 6) |
>> -            ((pix & 0x000000f8) >> 3);
>> -        dbuf16[x] = cpu_to_le16(val16);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        dbuf16[x] = drm_format_xrgb8888_to_argb1555(sbuf32[x]);
>>   }
>>     /**
>> @@ -603,17 +916,9 @@ static void 
>> drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
>>       __le16 *dbuf16 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u16 val16;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        val16 = ((pix & 0x00f80000) >> 8) |
>> -            ((pix & 0x0000f800) >> 5) |
>> -            ((pix & 0x000000f8) >> 2) |
>> -            BIT(0); /* set alpha bit */
>> -        dbuf16[x] = cpu_to_le16(val16);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        dbuf16[x] = drm_format_xrgb8888_to_rgba5551(sbuf32[x]);
>>   }
>>     /**
>> @@ -707,13 +1012,9 @@ static void 
>> drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsig
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        pix |= GENMASK(31, 24); /* fill alpha bits */
>> -        dbuf32[x] = cpu_to_le32(pix);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        dbuf32[x] = drm_format_xrgb8888_to_argb8888(sbuf32[x]);
>>   }
>>     /**
>> @@ -756,16 +1057,9 @@ static void 
>> drm_fb_xrgb8888_to_abgr8888_line(void *dbuf, const void *sbuf, unsig
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        pix = ((pix & 0x00ff0000) >> 16) <<  0 |
>> -              ((pix & 0x0000ff00) >>  8) <<  8 |
>> -              ((pix & 0x000000ff) >>  0) << 16 |
>> -              GENMASK(31, 24); /* fill alpha bits */
>> -        *dbuf32++ = cpu_to_le32(pix);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        *dbuf32++ = drm_format_xrgb8888_to_abgr8888(sbuf32[x]);
>>   }
>>     static void drm_fb_xrgb8888_to_abgr8888(struct iosys_map *dst, 
>> const unsigned int *dst_pitch,
>> @@ -787,16 +1081,9 @@ static void 
>> drm_fb_xrgb8888_to_xbgr8888_line(void *dbuf, const void *sbuf, unsig
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        pix = ((pix & 0x00ff0000) >> 16) <<  0 |
>> -              ((pix & 0x0000ff00) >>  8) <<  8 |
>> -              ((pix & 0x000000ff) >>  0) << 16 |
>> -              ((pix & 0xff000000) >> 24) << 24;
>> -        *dbuf32++ = cpu_to_le32(pix);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        *dbuf32++ = drm_format_xrgb8888_to_xbgr8888(sbuf32[x]);
>>   }
>>     static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, 
>> const unsigned int *dst_pitch,
>> @@ -818,17 +1105,9 @@ static void 
>> drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u32 val32;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        val32 = ((pix & 0x000000FF) << 2) |
>> -            ((pix & 0x0000FF00) << 4) |
>> -            ((pix & 0x00FF0000) << 6);
>> -        pix = val32 | ((val32 >> 8) & 0x00300C03);
>> -        *dbuf32++ = cpu_to_le32(pix);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        *dbuf32++ = drm_format_xrgb8888_to_xrgb2101010(sbuf32[x]);
>>   }
>>     /**
>> @@ -872,18 +1151,9 @@ static void 
>> drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
>>       __le32 *dbuf32 = dbuf;
>>       const __le32 *sbuf32 = sbuf;
>>       unsigned int x;
>> -    u32 val32;
>> -    u32 pix;
>>   -    for (x = 0; x < pixels; x++) {
>> -        pix = le32_to_cpu(sbuf32[x]);
>> -        val32 = ((pix & 0x000000ff) << 2) |
>> -            ((pix & 0x0000ff00) << 4) |
>> -            ((pix & 0x00ff0000) << 6);
>> -        pix = GENMASK(31, 30) | /* set alpha bits */
>> -              val32 | ((val32 >> 8) & 0x00300c03);
>> -        *dbuf32++ = cpu_to_le32(pix);
>> -    }
>> +    for (x = 0; x < pixels; x++)
>> +        *dbuf32++ = drm_format_xrgb8888_to_argb2101010(sbuf32[x]);
>>   }
>>     /**
>> diff --git a/include/drm/drm_format_helper.h 
>> b/include/drm/drm_format_helper.h
>> index f13b34e0b752..f416f0bef52d 100644
>> --- a/include/drm/drm_format_helper.h
>> +++ b/include/drm/drm_format_helper.h
>> @@ -66,6 +66,7 @@ void *drm_format_conv_state_reserve(struct 
>> drm_format_conv_state *state,
>>                       size_t new_size, gfp_t flags);
>>   void drm_format_conv_state_release(struct drm_format_conv_state 
>> *state);
>>   +u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format);
>>   unsigned int drm_fb_clip_offset(unsigned int pitch, const struct 
>> drm_format_info *format,
>>                   const struct drm_rect *clip);
>>   @@ -76,6 +77,14 @@ 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,
>>            struct drm_format_conv_state *state);
>> +void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
>> +             const u8 *sbuf8, unsigned int spitch,
>> +             unsigned int height, unsigned int width,
>> +             u32 fg_color, u32 bg_color,
>> +             unsigned int pixel_width);
>> +void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
>> +         unsigned int height, unsigned int width,
>> +         u32 color, unsigned int pixel_width);
>>   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, struct 
>> drm_format_conv_state *state);
>

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

Re: [PATCH v9 2/9] drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill

[Jocelyn Falempe]

Hi,

On 07/03/2024 16:41, Thomas Zimmermann wrote:
> 
> 
> Am 07.03.24 um 15:08 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 07.03.24 um 10:14 schrieb Jocelyn Falempe:
>>> This is needed for drm_panic, to draw the font, and fill
>>> the background color, in multiple color format.
>>
>> As you know, I'm not happy about this patch and the the changes do not 
>> reflect my easier
> 
> s/easier/earlier
> 
>> review. These format helpers are supposed to serve all of DRM and not 
>> just the panic handler. I know that the current code isn't perfect, 
>> but the R1 support is a step backwards IMHO.
>>
>> I suggest to the drawing routines entirely within the panic-handler 
>> code and maybe try to sort out things later. IIRC that's how it was in 
>> earlier revisions of the patchset.

Sorry I didn't change this patch in v8 and v9, I was focusing on locking 
and the overall design.
I agree to move this back to drm_panic.c, so we can merge a first 
version of drm_panic. And when a generic format converter will be ready, 
I will do the patch to use it.

Best regards,

-- 

Jocelyn



>>
>> Best regards
>> Thomas
>>
>>>
>>> v5:
>>>   * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
>>>   * Also add drm_fb_fill() to fill area with background color.
>>> v6:
>>>   * fix __le32 conversion warning found by the kernel test bot
>>>
>>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>>> ---
>>>   drivers/gpu/drm/drm_format_helper.c | 432 ++++++++++++++++++++++------
>>>   include/drm/drm_format_helper.h     |   9 +
>>>   2 files changed, 360 insertions(+), 81 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_format_helper.c 
>>> b/drivers/gpu/drm/drm_format_helper.c
>>> index b1be458ed4dd..2d9646cefc4f 100644
>>> --- a/drivers/gpu/drm/drm_format_helper.c
>>> +++ b/drivers/gpu/drm/drm_format_helper.c
>>> @@ -111,6 +111,153 @@ void drm_format_conv_state_release(struct 
>>> drm_format_conv_state *state)
>>>   }
>>>   EXPORT_SYMBOL(drm_format_conv_state_release);
>>>   +static __le16 drm_format_xrgb8888_to_rgb565(__le32 val32)
>>> +{
>>> +    u16 val16;
>>> +    u32 pix;
>>> +
>>> +    pix = le32_to_cpu(val32);
>>> +    val16 = ((pix & 0x00F80000) >> 8) |
>>> +        ((pix & 0x0000FC00) >> 5) |
>>> +        ((pix & 0x000000F8) >> 3);
>>> +    return cpu_to_le16(val16);
>>> +}
>>> +
>>> +static __le16 drm_format_xrgb8888_to_rgba5551(__le32 val32)
>>> +{
>>> +    u16 val16;
>>> +    u32 pix;
>>> +
>>> +    pix = le32_to_cpu(val32);
>>> +    val16 = ((pix & 0x00f80000) >> 8) |
>>> +        ((pix & 0x0000f800) >> 5) |
>>> +        ((pix & 0x000000f8) >> 2) |
>>> +        BIT(0); /* set alpha bit */
>>> +    return cpu_to_le16(val16);
>>> +}
>>> +
>>> +static __le16 drm_format_xrgb8888_to_xrgb1555(__le32 val32)
>>> +{
>>> +    u16 val16;
>>> +    u32 pix;
>>> +
>>> +    pix = le32_to_cpu(val32);
>>> +    val16 = ((pix & 0x00f80000) >> 9) |
>>> +        ((pix & 0x0000f800) >> 6) |
>>> +        ((pix & 0x000000f8) >> 3);
>>> +    return cpu_to_le16(val16);
>>> +}
>>> +
>>> +static __le16 drm_format_xrgb8888_to_argb1555(__le32 val32)
>>> +{
>>> +    u16 val16;
>>> +    u32 pix;
>>> +
>>> +    pix = le32_to_cpu(val32);
>>> +    val16 = BIT(15) | /* set alpha bit */
>>> +        ((pix & 0x00f80000) >> 9) |
>>> +        ((pix & 0x0000f800) >> 6) |
>>> +        ((pix & 0x000000f8) >> 3);
>>> +    return cpu_to_le16(val16);
>>> +}
>>> +
>>> +static __le32 drm_format_xrgb8888_to_argb8888(__le32 pix)
>>> +{
>>> +    u32 val32;
>>> +
>>> +    val32 = le32_to_cpu(pix);
>>> +    val32 |= GENMASK(31, 24); /* fill alpha bits */
>>> +    return cpu_to_le32(val32);
>>> +}
>>> +
>>> +static __le32 drm_format_xrgb8888_to_xbgr8888(__le32 pix)
>>> +{
>>> +    u32 val32;
>>> +
>>> +    val32 = le32_to_cpu(pix);
>>> +    val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
>>> +        ((val32 & 0x0000ff00) >>  8) <<  8 |
>>> +        ((val32 & 0x000000ff) >>  0) << 16 |
>>> +        ((val32 & 0xff000000) >> 24) << 24;
>>> +    return cpu_to_le32(val32);
>>> +}
>>> +
>>> +static __le32 drm_format_xrgb8888_to_abgr8888(__le32 pix)
>>> +{
>>> +    u32 val32;
>>> +
>>> +    val32 = le32_to_cpu(pix);
>>> +    val32 = ((val32 & 0x00ff0000) >> 16) <<  0 |
>>> +        ((val32 & 0x0000ff00) >>  8) <<  8 |
>>> +        ((val32 & 0x000000ff) >>  0) << 16 |
>>> +        GENMASK(31, 24); /* fill alpha bits */
>>> +    return cpu_to_le32(val32);
>>> +}
>>> +
>>> +static __le32 drm_format_xrgb8888_to_xrgb2101010(__le32 pix)
>>> +{
>>> +    u32 val32;
>>> +
>>> +    val32 = le32_to_cpu(pix);
>>> +    val32 = ((val32 & 0x000000FF) << 2) |
>>> +        ((val32 & 0x0000FF00) << 4) |
>>> +        ((val32 & 0x00FF0000) << 6);
>>> +    return cpu_to_le32(val32 | ((val32 >> 8) & 0x00300C03));
>>> +}
>>> +
>>> +static __le32 drm_format_xrgb8888_to_argb2101010(__le32 pix)
>>> +{
>>> +    u32 val32;
>>> +
>>> +    val32 = le32_to_cpu(pix);
>>> +    val32 = ((val32 & 0x000000FF) << 2) |
>>> +        ((val32 & 0x0000FF00) << 4) |
>>> +        ((val32 & 0x00FF0000) << 6);
>>> +    val32 = GENMASK(31, 30) | /* set alpha bits */
>>> +          val32 | ((val32 >> 8) & 0x00300c03);
>>> +    return cpu_to_le32(val32);
>>> +}
>>> +
>>> +/**
>>> + * drm_fb_convert_from_xrgb8888 - convert one pixel from xrgb8888 to 
>>> the desired format
>>> + * @color: input color, in xrgb8888 format
>>> + * @format: output format
>>> + *
>>> + * Returns:
>>> + * Color in the format specified, casted to u32.
>>> + * Or 0 if the format is unknown.
>>> + */
>>> +u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format)
>>> +{
>>> +    __le32 pix = cpu_to_le32(color);
>>> +
>>> +    switch (format) {
>>> +    case DRM_FORMAT_RGB565:
>>> +        return le16_to_cpu(drm_format_xrgb8888_to_rgb565(pix));
>>> +    case DRM_FORMAT_RGBA5551:
>>> +        return le16_to_cpu(drm_format_xrgb8888_to_rgba5551(pix));
>>> +    case DRM_FORMAT_XRGB1555:
>>> +        return le16_to_cpu(drm_format_xrgb8888_to_xrgb1555(pix));
>>> +    case DRM_FORMAT_ARGB1555:
>>> +        return le16_to_cpu(drm_format_xrgb8888_to_argb1555(pix));
>>> +    case DRM_FORMAT_RGB888:
>>> +    case DRM_FORMAT_XRGB8888:
>>> +        return le32_to_cpu(pix);
>>> +    case DRM_FORMAT_ARGB8888:
>>> +        return le32_to_cpu(drm_format_xrgb8888_to_argb8888(pix));
>>> +    case DRM_FORMAT_XBGR8888:
>>> +        return le32_to_cpu(drm_format_xrgb8888_to_xbgr8888(pix));
>>> +    case DRM_FORMAT_XRGB2101010:
>>> +        return le32_to_cpu(drm_format_xrgb8888_to_xrgb2101010(pix));
>>> +    case DRM_FORMAT_ARGB2101010:
>>> +        return le32_to_cpu(drm_format_xrgb8888_to_argb2101010(pix));
>>> +    default:
>>> +        WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
>>> +        return 0;
>>> +    }
>>> +}
>>> +EXPORT_SYMBOL(drm_fb_convert_from_xrgb8888);
>>> +
>>>   static unsigned int clip_offset(const struct drm_rect *clip, 
>>> unsigned int pitch, unsigned int cpp)
>>>   {
>>>       return clip->y1 * pitch + clip->x1 * cpp;
>>> @@ -366,6 +513,193 @@ void drm_fb_swab(struct iosys_map *dst, const 
>>> unsigned int *dst_pitch,
>>>   }
>>>   EXPORT_SYMBOL(drm_fb_swab);
>>>   +static void drm_fb_r1_to_16bit(struct iosys_map *dmap, unsigned 
>>> int dpitch,
>>> +                   const u8 *sbuf8, unsigned int spitch,
>>> +                   unsigned int height, unsigned int width,
>>> +                   __le16 fg16, __le16 bg16)
>>> +{
>>> +    unsigned int l, x;
>>> +    __le16 val16;
>>> +
>>> +    for (l = 0; l < height; l++) {
>>> +        for (x = 0; x < width; x++) {
>>> +            val16 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 
>>> 8))) ? fg16 : bg16;
>>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, 
>>> le16_to_cpu(val16));
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void drm_fb_r1_to_24bit(struct iosys_map *dmap, unsigned int 
>>> dpitch,
>>> +                   const u8 *sbuf8, unsigned int spitch,
>>> +                   unsigned int height, unsigned int width,
>>> +                   __le32 fg32, __le32 bg32)
>>> +{
>>> +    unsigned int l, x;
>>> +    __le32 color;
>>> +    u32 val32;
>>> +
>>> +    for (l = 0; l < height; l++) {
>>> +        for (x = 0; x < width; x++) {
>>> +            u32 off = l * dpitch + x * 3;
>>> +
>>> +            color = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 
>>> 8))) ? fg32 : bg32;
>>> +            val32 = le32_to_cpu(color);
>>> +
>>> +            /* write blue-green-red to output in little endianness */
>>> +            iosys_map_wr(dmap, off, u8, (val32 & 0x000000FF) >> 0);
>>> +            iosys_map_wr(dmap, off + 1, u8, (val32 & 0x0000FF00) >> 8);
>>> +            iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF0000) >> 
>>> 16);
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void drm_fb_r1_to_32bit(struct iosys_map *dmap, unsigned int 
>>> dpitch,
>>> +                   const u8 *sbuf8, unsigned int spitch,
>>> +                   unsigned int height, unsigned int width,
>>> +                   __le32 fg32, __le32 bg32)
>>> +{
>>> +    unsigned int l, x;
>>> +    __le32 val32;
>>> +
>>> +    for (l = 0; l < height; l++) {
>>> +        for (x = 0; x < width; x++) {
>>> +            val32 = (sbuf8[(l * spitch) + x / 8] & (0x80 >> (x % 
>>> 8))) ? fg32 : bg32;
>>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, 
>>> le32_to_cpu(val32));
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +/**
>>> + * drm_fb_blit_from_r1 - convert a monochrome image to a linear 
>>> framebuffer
>>> + * @dmap: destination iosys_map
>>> + * @dpitch: destination pitch in bytes
>>> + * @sbuf8: source buffer, in monochrome format, 8 pixels per byte.
>>> + * @spitch: source pitch in bytes
>>> + * @height: height of the image to copy, in pixels
>>> + * @width: width of the image to copy, in pixels
>>> + * @fg_color: foreground color, in destination format
>>> + * @bg_color: background color, in destination format
>>> + * @pixel_width: pixel width in bytes.
>>> + *
>>> + * This can be used to draw font which are monochrome images, to a 
>>> framebuffer
>>> + * in other supported format.
>>> + */
>>> +void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
>>> +             const u8 *sbuf8, unsigned int spitch,
>>> +             unsigned int height, unsigned int width,
>>> +             u32 fg_color, u32 bg_color,
>>> +             unsigned int pixel_width)
>>> +{
>>> +    switch (pixel_width) {
>>> +    case 2:
>>> +        drm_fb_r1_to_16bit(dmap, dpitch, sbuf8, spitch,
>>> +                   height, width,
>>> +                   cpu_to_le16(fg_color),
>>> +                   cpu_to_le16(bg_color));
>>> +    break;
>>> +    case 3:
>>> +        drm_fb_r1_to_24bit(dmap, dpitch, sbuf8, spitch,
>>> +                   height, width,
>>> +                   cpu_to_le32(fg_color),
>>> +                   cpu_to_le32(bg_color));
>>> +    break;
>>> +    case 4:
>>> +        drm_fb_r1_to_32bit(dmap, dpitch, sbuf8, spitch,
>>> +                   height, width,
>>> +                   cpu_to_le32(fg_color),
>>> +                   cpu_to_le32(bg_color));
>>> +    break;
>>> +    default:
>>> +        WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width);
>>> +    }
>>> +}
>>> +EXPORT_SYMBOL(drm_fb_blit_from_r1);
>>> +
>>> +static void drm_fb_fill8(struct iosys_map *dmap, unsigned int dpitch,
>>> +             unsigned int height, unsigned int width,
>>> +             u8 color)
>>> +{
>>> +    unsigned int l, x;
>>> +
>>> +    for (l = 0; l < height; l++)
>>> +        for (x = 0; x < width; x++)
>>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u8), u8, color);
>>> +}
>>> +
>>> +static void drm_fb_fill16(struct iosys_map *dmap, unsigned int dpitch,
>>> +              unsigned int height, unsigned int width,
>>> +              u16 color)
>>> +{
>>> +    unsigned int l, x;
>>> +
>>> +    for (l = 0; l < height; l++)
>>> +        for (x = 0; x < width; x++)
>>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u16), u16, 
>>> color);
>>> +}
>>> +
>>> +static void drm_fb_fill24(struct iosys_map *dmap, unsigned int dpitch,
>>> +              unsigned int height, unsigned int width,
>>> +              u32 color)
>>> +{
>>> +    unsigned int l, x;
>>> +
>>> +    for (l = 0; l < height; l++) {
>>> +        for (x = 0; x < width; x++) {
>>> +            unsigned int off = l * dpitch + x * 3;
>>> +
>>> +            /* write blue-green-red to output in little endianness */
>>> +            iosys_map_wr(dmap, off, u8, (color & 0x000000FF) >> 0);
>>> +            iosys_map_wr(dmap, off + 1, u8, (color & 0x0000FF00) >> 8);
>>> +            iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 
>>> 16);
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void drm_fb_fill32(struct iosys_map *dmap, unsigned int dpitch,
>>> +              unsigned int height, unsigned int width,
>>> +              u32 color)
>>> +{
>>> +    unsigned int l, x;
>>> +
>>> +    for (l = 0; l < height; l++)
>>> +        for (x = 0; x < width; x++)
>>> +            iosys_map_wr(dmap, l * dpitch + x * sizeof(u32), u32, 
>>> color);
>>> +}
>>> +
>>> +/**
>>> + * drm_fb_fill - Fill a rectangle with a color
>>> + * @dmap: destination iosys_map, pointing to the top left corner of 
>>> the rectangle
>>> + * @dpitch: destination pitch in bytes
>>> + * @height: height of the rectangle, in pixels
>>> + * @width: width of the rectangle, in pixels
>>> + * @color: color to fill the rectangle.
>>> + * @pixel_width: pixel width in bytes
>>> + *
>>> + * Fill a rectangle with a color, in a linear framebuffer.
>>> + */
>>> +void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
>>> +             unsigned int height, unsigned int width,
>>> +             u32 color, unsigned int pixel_width)
>>> +{
>>> +    switch (pixel_width) {
>>> +    case 1:
>>> +        drm_fb_fill8(dmap, dpitch, height, width, color);
>>> +    break;
>>> +    case 2:
>>> +        drm_fb_fill16(dmap, dpitch, height, width, color);
>>> +    break;
>>> +    case 3:
>>> +        drm_fb_fill24(dmap, dpitch, height, width, color);
>>> +    break;
>>> +    case 4:
>>> +        drm_fb_fill32(dmap, dpitch, height, width, color);
>>> +    break;
>>> +    default:
>>> +        WARN_ONCE(1, "Can't fill with pixel width %d\n", pixel_width);
>>> +    }
>>> +}
>>> +EXPORT_SYMBOL(drm_fb_fill);
>>> +
>>>   static void drm_fb_xrgb8888_to_rgb332_line(void *dbuf, const void 
>>> *sbuf, unsigned int pixels)
>>>   {
>>>       u8 *dbuf8 = dbuf;
>>> @@ -420,15 +754,9 @@ static void drm_fb_xrgb8888_to_rgb565_line(void 
>>> *dbuf, const void *sbuf, unsigne
>>>       __le16 *dbuf16 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u16 val16;
>>> -    u32 pix;
>>>         for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        val16 = ((pix & 0x00F80000) >> 8) |
>>> -            ((pix & 0x0000FC00) >> 5) |
>>> -            ((pix & 0x000000F8) >> 3);
>>> -        dbuf16[x] = cpu_to_le16(val16);
>>> +        dbuf16[x] = drm_format_xrgb8888_to_rgb565(sbuf32[x]);
>>>       }
>>>   }
>>>   @@ -498,16 +826,9 @@ static void 
>>> drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsig
>>>       __le16 *dbuf16 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u16 val16;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        val16 = ((pix & 0x00f80000) >> 9) |
>>> -            ((pix & 0x0000f800) >> 6) |
>>> -            ((pix & 0x000000f8) >> 3);
>>> -        dbuf16[x] = cpu_to_le16(val16);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        dbuf16[x] = drm_format_xrgb8888_to_xrgb1555(sbuf32[x]);
>>>   }
>>>     /**
>>> @@ -550,17 +871,9 @@ static void 
>>> drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsig
>>>       __le16 *dbuf16 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u16 val16;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        val16 = BIT(15) | /* set alpha bit */
>>> -            ((pix & 0x00f80000) >> 9) |
>>> -            ((pix & 0x0000f800) >> 6) |
>>> -            ((pix & 0x000000f8) >> 3);
>>> -        dbuf16[x] = cpu_to_le16(val16);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        dbuf16[x] = drm_format_xrgb8888_to_argb1555(sbuf32[x]);
>>>   }
>>>     /**
>>> @@ -603,17 +916,9 @@ static void 
>>> drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsig
>>>       __le16 *dbuf16 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u16 val16;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        val16 = ((pix & 0x00f80000) >> 8) |
>>> -            ((pix & 0x0000f800) >> 5) |
>>> -            ((pix & 0x000000f8) >> 2) |
>>> -            BIT(0); /* set alpha bit */
>>> -        dbuf16[x] = cpu_to_le16(val16);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        dbuf16[x] = drm_format_xrgb8888_to_rgba5551(sbuf32[x]);
>>>   }
>>>     /**
>>> @@ -707,13 +1012,9 @@ static void 
>>> drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsig
>>>       __le32 *dbuf32 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        pix |= GENMASK(31, 24); /* fill alpha bits */
>>> -        dbuf32[x] = cpu_to_le32(pix);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        dbuf32[x] = drm_format_xrgb8888_to_argb8888(sbuf32[x]);
>>>   }
>>>     /**
>>> @@ -756,16 +1057,9 @@ static void 
>>> drm_fb_xrgb8888_to_abgr8888_line(void *dbuf, const void *sbuf, unsig
>>>       __le32 *dbuf32 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        pix = ((pix & 0x00ff0000) >> 16) <<  0 |
>>> -              ((pix & 0x0000ff00) >>  8) <<  8 |
>>> -              ((pix & 0x000000ff) >>  0) << 16 |
>>> -              GENMASK(31, 24); /* fill alpha bits */
>>> -        *dbuf32++ = cpu_to_le32(pix);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        *dbuf32++ = drm_format_xrgb8888_to_abgr8888(sbuf32[x]);
>>>   }
>>>     static void drm_fb_xrgb8888_to_abgr8888(struct iosys_map *dst, 
>>> const unsigned int *dst_pitch,
>>> @@ -787,16 +1081,9 @@ static void 
>>> drm_fb_xrgb8888_to_xbgr8888_line(void *dbuf, const void *sbuf, unsig
>>>       __le32 *dbuf32 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        pix = ((pix & 0x00ff0000) >> 16) <<  0 |
>>> -              ((pix & 0x0000ff00) >>  8) <<  8 |
>>> -              ((pix & 0x000000ff) >>  0) << 16 |
>>> -              ((pix & 0xff000000) >> 24) << 24;
>>> -        *dbuf32++ = cpu_to_le32(pix);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        *dbuf32++ = drm_format_xrgb8888_to_xbgr8888(sbuf32[x]);
>>>   }
>>>     static void drm_fb_xrgb8888_to_xbgr8888(struct iosys_map *dst, 
>>> const unsigned int *dst_pitch,
>>> @@ -818,17 +1105,9 @@ static void 
>>> drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, un
>>>       __le32 *dbuf32 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u32 val32;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        val32 = ((pix & 0x000000FF) << 2) |
>>> -            ((pix & 0x0000FF00) << 4) |
>>> -            ((pix & 0x00FF0000) << 6);
>>> -        pix = val32 | ((val32 >> 8) & 0x00300C03);
>>> -        *dbuf32++ = cpu_to_le32(pix);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        *dbuf32++ = drm_format_xrgb8888_to_xrgb2101010(sbuf32[x]);
>>>   }
>>>     /**
>>> @@ -872,18 +1151,9 @@ static void 
>>> drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, un
>>>       __le32 *dbuf32 = dbuf;
>>>       const __le32 *sbuf32 = sbuf;
>>>       unsigned int x;
>>> -    u32 val32;
>>> -    u32 pix;
>>>   -    for (x = 0; x < pixels; x++) {
>>> -        pix = le32_to_cpu(sbuf32[x]);
>>> -        val32 = ((pix & 0x000000ff) << 2) |
>>> -            ((pix & 0x0000ff00) << 4) |
>>> -            ((pix & 0x00ff0000) << 6);
>>> -        pix = GENMASK(31, 30) | /* set alpha bits */
>>> -              val32 | ((val32 >> 8) & 0x00300c03);
>>> -        *dbuf32++ = cpu_to_le32(pix);
>>> -    }
>>> +    for (x = 0; x < pixels; x++)
>>> +        *dbuf32++ = drm_format_xrgb8888_to_argb2101010(sbuf32[x]);
>>>   }
>>>     /**
>>> diff --git a/include/drm/drm_format_helper.h 
>>> b/include/drm/drm_format_helper.h
>>> index f13b34e0b752..f416f0bef52d 100644
>>> --- a/include/drm/drm_format_helper.h
>>> +++ b/include/drm/drm_format_helper.h
>>> @@ -66,6 +66,7 @@ void *drm_format_conv_state_reserve(struct 
>>> drm_format_conv_state *state,
>>>                       size_t new_size, gfp_t flags);
>>>   void drm_format_conv_state_release(struct drm_format_conv_state 
>>> *state);
>>>   +u32 drm_fb_convert_from_xrgb8888(u32 color, u32 format);
>>>   unsigned int drm_fb_clip_offset(unsigned int pitch, const struct 
>>> drm_format_info *format,
>>>                   const struct drm_rect *clip);
>>>   @@ -76,6 +77,14 @@ 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,
>>>            struct drm_format_conv_state *state);
>>> +void drm_fb_blit_from_r1(struct iosys_map *dmap, unsigned int dpitch,
>>> +             const u8 *sbuf8, unsigned int spitch,
>>> +             unsigned int height, unsigned int width,
>>> +             u32 fg_color, u32 bg_color,
>>> +             unsigned int pixel_width);
>>> +void drm_fb_fill(struct iosys_map *dmap, unsigned int dpitch,
>>> +         unsigned int height, unsigned int width,
>>> +         u32 color, unsigned int pixel_width);
>>>   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, struct 
>>> drm_format_conv_state *state);
>>
> 

Re: [PATCH v9 1/9] drm/panic: Add drm panic locking

[Jocelyn Falempe]

On 07/03/2024 11:27, John Ogness wrote:
> On 2024-03-07, Jocelyn Falempe <jfalempe@redhat.com> wrote:
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 39ef0a6addeb..c0bb91312fb2 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -38,6 +38,7 @@
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_print.h>
>>   #include <drm/drm_self_refresh_helper.h>
>>   #include <drm/drm_vblank.h>
>> @@ -3099,6 +3100,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>   		}
>>   	}
>>   
>> +	drm_panic_lock(state->dev);
>>   	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>>   		WARN_ON(plane->state != old_plane_state);
>>   
>> @@ -3108,6 +3110,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>   		state->planes[i].state = old_plane_state;
>>   		plane->state = new_plane_state;
>>   	}
>> +	drm_panic_unlock(state->dev);
> 
> Is there a reason irqsave/irqrestore variants are not used? Maybe this
> code path is too hot?

This lock will be taken for each page flip, so typically at 60Hz (or 
maybe 144Hz for gamers). I don't know what are the performance impacts 
of the irqsave/irqrestore variant.
> 
> By leaving interrupts enabled, there is the risk that a panic from
> within any interrupt handler may block the drm panic handler.

The current design is that the panic handler will just use try_lock(), 
and if it can't take it, the panic screen will not be seen.
The goal is to make sure drm_panic won't crash the machine and prevent 
kdump or other panic handler to run. So there is a very small race 
chance that the panic screen won't be seen, but that's ok.

So I think in this case the drm panic handler shouldn't be blocked, as 
it only use try_lock().

Best regards,

-- 

Jocelyn

> 
> John Ogness
> 

Re: [PATCH v9 1/9] drm/panic: Add drm panic locking

[Thomas Zimmermann]

Hi

Am 07.03.24 um 10:14 schrieb Jocelyn Falempe:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Rough sketch for the locking of drm panic printing code. The upshot of
> this approach is that we can pretty much entirely rely on the atomic
> commit flow, with the pair of raw_spin_lock/unlock providing any
> barriers we need, without having to create really big critical
> sections in code.

The ast driver has a lock to protect modesetting and concurrent EDID 
reads from each other. [1] That new panic_lock seems to serve the same 
purpose.

If we go that route, can we make this a bit more generic and call it 
commit_lock? I could then remove the dedicated lock from ast.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.7/source/drivers/gpu/drm/ast/ast_drv.h#L195

>
> This also avoids the need that drivers must explicitly update the
> panic handler state, which they might forget to do, or not do
> consistently, and then we blow up in the worst possible times.
>
> It is somewhat racy against a concurrent atomic update, and we might
> write into a buffer which the hardware will never display. But there's
> fundamentally no way to avoid that - if we do the panic state update
> explicitly after writing to the hardware, we might instead write to an
> old buffer that the user will barely ever see.
>
> Note that an rcu protected deference of plane->state would give us the
> the same guarantees, but it has the downside that we then need to
> protect the plane state freeing functions with call_rcu too. Which
> would very widely impact a lot of code and therefore doesn't seem
> worth the complexity compared to a raw spinlock with very tiny
> critical sections. Plus rcu cannot be used to protect access to
> peek/poke registers anyway, so we'd still need it for those cases.
>
> Peek/poke registers for vram access (or a gart pte reserved just for
> panic code) are also the reason I've gone with a per-device and not
> per-plane spinlock, since usually these things are global for the
> entire display. Going with per-plane locks would mean drivers for such
> hardware would need additional locks, which we don't want, since it
> deviates from the per-console takeoverlocks design.
>
> Longer term it might be useful if the panic notifiers grow a bit more
> structure than just the absolute bare
> EXPORT_SYMBOL(panic_notifier_list) - somewhat aside, why is that not
> EXPORT_SYMBOL_GPL ... If panic notifiers would be more like console
> drivers with proper register/unregister interfaces we could perhaps
> reuse the very fancy console lock with all it's check and takeover
> semantics that John Ogness is developing to fix the console_lock mess.
> But for the initial cut of a drm panic printing support I don't think
> we need that, because the critical sections are extremely small and
> only happen once per display refresh. So generally just 60 tiny locked
> sections per second, which is nothing compared to a serial console
> running a 115kbaud doing really slow mmio writes for each byte. So for
> now the raw spintrylock in drm panic notifier callback should be good
> enough.
>
> Another benefit of making panic notifiers more like full blown
> consoles (that are used in panics only) would be that we get the two
> stage design, where first all the safe outputs are used. And then the
> dangerous takeover tricks are deployed (where for display drivers we
> also might try to intercept any in-flight display buffer flips, which
> if we race and misprogram fifos and watermarks can hang the memory
> controller on some hw).
>
> For context the actual implementation on the drm side is by Jocelyn
> and this patch is meant to be combined with the overall approach in
> v7 (v8 is a bit less flexible, which I think is the wrong direction):
>
> https://lore.kernel.org/dri-devel/20240104160301.185915-1-jfalempe@redhat.com/
>
> Note that the locking is very much not correct there, hence this
> separate rfc.
>
> v2:
> - fix authorship, this was all my typing
> - some typo oopsies
> - link to the drm panic work by Jocelyn for context
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jocelyn Falempe <jfalempe@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: John Ogness <john.ogness@linutronix.de>
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c |  3 +
>   drivers/gpu/drm/drm_drv.c           |  1 +
>   include/drm/drm_mode_config.h       | 10 +++
>   include/drm/drm_panic.h             | 99 +++++++++++++++++++++++++++++
>   4 files changed, 113 insertions(+)
>   create mode 100644 include/drm/drm_panic.h
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 39ef0a6addeb..c0bb91312fb2 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -38,6 +38,7 @@
>   #include <drm/drm_drv.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_panic.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_self_refresh_helper.h>
>   #include <drm/drm_vblank.h>
> @@ -3099,6 +3100,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>   		}
>   	}
>   
> +	drm_panic_lock(state->dev);
>   	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>   		WARN_ON(plane->state != old_plane_state);
>   
> @@ -3108,6 +3110,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>   		state->planes[i].state = old_plane_state;
>   		plane->state = new_plane_state;
>   	}
> +	drm_panic_unlock(state->dev);
>   
>   	for_each_oldnew_private_obj_in_state(state, obj, old_obj_state, new_obj_state, i) {
>   		WARN_ON(obj->state != old_obj_state);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 243cacb3575c..c157500b3135 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -638,6 +638,7 @@ static int drm_dev_init(struct drm_device *dev,
>   	mutex_init(&dev->filelist_mutex);
>   	mutex_init(&dev->clientlist_mutex);
>   	mutex_init(&dev->master_mutex);
> +	raw_spin_lock_init(&dev->mode_config.panic_lock);
>   
>   	ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL);
>   	if (ret)
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 973119a9176b..e79f1a557a22 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -505,6 +505,16 @@ struct drm_mode_config {
>   	 */
>   	struct list_head plane_list;
>   
> +	/**
> +	 * @panic_lock:
> +	 *
> +	 * Raw spinlock used to protect critical sections of code that access
> +	 * the display hardware or modeset software state, which the panic
> +	 * printing code must be protected against. See drm_panic_trylock(),
> +	 * drm_panic_lock() and drm_panic_unlock().
> +	 */
> +	struct raw_spinlock panic_lock;
> +
>   	/**
>   	 * @num_crtc:
>   	 *
> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> new file mode 100644
> index 000000000000..f2135d03f1eb
> --- /dev/null
> +++ b/include/drm/drm_panic.h
> @@ -0,0 +1,99 @@
> +/* SPDX-License-Identifier: GPL-2.0 or MIT */
> +#ifndef __DRM_PANIC_H__
> +#define __DRM_PANIC_H__
> +
> +#include <drm/drm_device.h>
> +/*
> + * Copyright (c) 2024 Intel
> + */
> +
> +/**
> + * drm_panic_trylock - try to enter the panic printing critical section
> + * @dev: struct drm_device
> + *
> + * This function must be called by any panic printing code. The panic printing
> + * attempt must be aborted if the trylock fails.
> + *
> + * Panic printing code can make the following assumptions while holding the
> + * panic lock:
> + *
> + * - Anything protected by drm_panic_lock() and drm_panic_unlock() pairs is safe
> + *   to access.
> + *
> + * - Furthermore the panic printing code only registers in drm_dev_unregister()
> + *   and gets removed in drm_dev_unregister(). This allows the panic code to
> + *   safely access any state which is invariant in between these two function
> + *   calls, like the list of planes drm_mode_config.plane_list or most of the
> + *   struct drm_plane structure.
> + *
> + * Specifically thanks to the protection around plane updates in
> + * drm_atomic_helper_swap_state() the following additional guarantees hold:
> + *
> + * - It is safe to deference the drm_plane.state pointer.
> + *
> + * - Anything in struct drm_plane_state or the driver's subclass thereof which
> + *   stays invariant after the atomic check code has finished is safe to access.
> + *   Specifically this includes the reference counted pointers to framebuffer
> + *   and buffer objects.
> + *
> + * - Anything set up by drm_plane_helper_funcs.fb_prepare and cleaned up
> + *   drm_plane_helper_funcs.fb_cleanup is safe to access, as long as it stays
> + *   invariant between these two calls. This also means that for drivers using
> + *   dynamic buffer management the framebuffer is pinned, and therefer all
> + *   relevant datastructures can be accessed without taking any further locks
> + *   (which would be impossible in panic context anyway).
> + *
> + * - Importantly, software and hardware state set up by
> + *   drm_plane_helper_funcs.begin_fb_access and
> + *   drm_plane_helper_funcs.end_fb_access is not safe to access.
> + *
> + * Drivers must not make any assumptions about the actual state of the hardware,
> + * unless they explicitly protected these hardware access with drm_panic_lock()
> + * and drm_panic_unlock().
> + *
> + * Returns:
> + *
> + * 0 when failing to acquire the raw spinlock, nonzero on success.
> + */
> +static inline int drm_panic_trylock(struct drm_device *dev)
> +{
> +	return raw_spin_trylock(&dev->mode_config.panic_lock);
> +}
> +
> +/**
> + * drm_panic_lock - protect panic printing relevant state
> + * @dev: struct drm_device
> + *
> + * This function must be called to protect software and hardware state that the
> + * panic printing code must be able to rely on. The protected sections must be
> + * as small as possible. Examples include:
> + *
> + * - Access to peek/poke or other similar registers, if that is the way the
> + *   driver prints the pixels into the scanout buffer at panic time.
> + *
> + * - Updates to pointers like drm_plane.state, allowing the panic handler to
> + *   safely deference these. This is done in drm_atomic_helper_swap_state().
> + *
> + * - An state that isn't invariant and that the driver must be able to access
> + *   during panic printing.
> + *
> + * Call drm_panic_unlock() to unlock the locked spinlock.
> + */
> +static inline void drm_panic_lock(struct drm_device *dev)
> +{
> +	return raw_spin_lock(&dev->mode_config.panic_lock);
> +}
> +
> +/**
> + * drm_panic_unlock - end of the panic printing critical section
> + * @dev: struct drm_device
> + *
> + * Unlocks the raw spinlock acquired by either drm_panic_lock() or
> + * drm_panic_trylock().
> + */
> +static inline void drm_panic_unlock(struct drm_device *dev)
> +{
> +	raw_spin_unlock(&dev->mode_config.panic_lock);
> +}
> +
> +#endif /* __DRM_PANIC_H__ */

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

Re: [v9,3/9] drm/panic: Add a drm panic handler

[Sui Jingfeng]

Hi,


On 2024/3/7 17:14, 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)
>
> v5
>   * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
>   * Also add drm_fb_fill() to fill area with background color.
>   * Add draw_pixel_xy() API for drivers that can't provide a linear buffer.
>   * Add a flush() callback for drivers that needs to synchronize the buffer.
>   * Add a void *private field, so drivers can pass private data to
>     draw_pixel_xy() and flush().
>
> v6
>   * Fix sparse warning for panic_msg and logo.
>
> v7
>   * Add select DRM_KMS_HELPER for the color conversion functions.
>
> v8
>   * Register directly each plane to the panic notifier (Sima)
>   * Add raw_spinlock to properly handle concurrency (Sima)
>   * Register plane instead of device, to avoid looping through plane
>     list, and simplify code.
>   * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
>    (Thomas Zimmermann)
>   * Removed the draw_pixel_xy() API, will see later if it can be added back.
>
> v9
>   * Revert to using get_scanout_buffer() (Sima)
>   * Move get_scanout_buffer() and panic_flush() to the plane helper
>     functions (Thomas Zimmermann)
>   * Register all planes with get_scanout_buffer() to the panic notifier
>   * Use drm_panic_lock() to protect against race (Sima)
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   Documentation/gpu/drm-kms.rst            |  12 +
>   drivers/gpu/drm/Kconfig                  |  23 ++
>   drivers/gpu/drm/Makefile                 |   1 +
>   drivers/gpu/drm/drm_drv.c                |   4 +
>   drivers/gpu/drm/drm_panic.c              | 322 +++++++++++++++++++++++
>   drivers/gpu/drm/drm_plane.c              |   1 +
>   include/drm/drm_modeset_helper_vtables.h |  37 +++
>   include/drm/drm_panic.h                  |  52 ++++
>   include/drm/drm_plane.h                  |   5 +
>   9 files changed, 457 insertions(+)
>   create mode 100644 drivers/gpu/drm/drm_panic.c


While applying you patch, there is new blank line at EOF reported, see below.
This is not an issue, but I want to report this to you.


git am ../drm-panic-Add-a-drm-panic-handler.mbox
Applying: drm/panic: Add drm panic locking
Applying: drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill
Applying: drm/panic: Add a drm panic handler
.git/rebase-apply/patch:439: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: drm/panic: Add debugfs entry to test without triggering panic.
Applying: drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
Applying: drm/simpledrm: Add drm_panic support
Applying: drm/mgag200: Add drm_panic support
Applying: drm/imx: Add drm_panic support
Applying: drm/ast: Add drm_panic support

Re: [v9,3/9] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 12/03/2024 14:18, Sui Jingfeng wrote:
> Hi,
> 
> 
[...]
> 
> 
> While applying you patch, there is new blank line at EOF reported, see 
> below.
> This is not an issue, but I want to report this to you.
> 
Hi,

Thanks, I will remove it for the next version.

-- 

Jocelyn

> 
> git am ../drm-panic-Add-a-drm-panic-handler.mbox
> Applying: drm/panic: Add drm panic locking
> Applying: drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill
> Applying: drm/panic: Add a drm panic handler
> .git/rebase-apply/patch:439: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.
> Applying: drm/panic: Add debugfs entry to test without triggering panic.
> Applying: drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
> Applying: drm/simpledrm: Add drm_panic support
> Applying: drm/mgag200: Add drm_panic support
> Applying: drm/imx: Add drm_panic support
> Applying: drm/ast: Add drm_panic support
> 

Re: [PATCH v9 0/9] drm/panic: Add a drm panic handler

[Sui Jingfeng]

Hi,


On 2024/3/7 17:14, Jocelyn Falempe wrote:
> 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, mgag200, ast, and imx.
>
> To test it, make sure you're using one of the supported driver, and trigger a panic:
> echo c > /proc/sysrq-trigger
>
> or you can enable CONFIG_DRM_PANIC_DEBUG and echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0

Yes, the whole series works for me! I have tested with drm/loongson,
Are you willing to add a implement for drm/loongson driver?
If you do please add the fallowing code snippet info drm/loongson/lsdc_plane.c.
Thanks you.


static int
lsdc_primary_plane_get_scanout_buffer(struct drm_plane *plane,
                       struct drm_scanout_buffer *scanout)
{
     struct drm_framebuffer *fb;

     if (!plane->state || !plane->state->fb)
         return -ENODEV;

     fb = plane->state->fb;
     if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
         return -ENODEV;

     scanout->format = fb->format;
     scanout->width = fb->width;
     scanout->height = fb->height;
     scanout->pitch = fb->pitches[0];

     return drm_gem_vmap_unlocked(fb->obj[0], &scanout->map);
}

hook this lsdc_primary_plane_get_scanout_buffer() up to the .get_scanout_buffer
member of lsdc_primary_helper_funcs, and include the #include <drm/drm_panic.h>
Thanks you in advance!

Re: [v9,9/9] drm/ast: Add drm_panic support

[Sui Jingfeng]

Hi,

On 2024/3/7 17:14, Jocelyn Falempe wrote:
> Add support for the drm_panic module, which displays a message to
> the screen when a kernel panic occurs.
>
> v7
>   * Use drm_for_each_primary_visible_plane()
>
> v8:
>   * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
>     (Thomas Zimmermann)
> v9:
>   * Revert to using get_scanout_buffer() (Sima)
>   * move get_scanout_buffer() to plane helper functions
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>


If Thomas not objects,


Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>

Re: [v9,5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic

[Sui Jingfeng]

Hi,

I'm get attracted by your patch, so I want to comment.
Please correct or ignore me if I say something wrong.

On 2024/3/7 17:14, Jocelyn Falempe wrote:
> This was initialy done for imx6, but should work on most drivers
> using drm_fb_dma_helper.
>
> v8:
>   * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
>     (Thomas Zimmermann)
>
> v9:
>   * go back to get_scanout_buffer()
>   * move get_scanout_buffer() to plane helper functions
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/drm_fb_dma_helper.c | 47 +++++++++++++++++++++++++++++
>   include/drm/drm_fb_dma_helper.h     |  4 +++
>   2 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> index 3b535ad1b07c..010327069ad4 100644
> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> @@ -15,6 +15,7 @@
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem_dma_helper.h>
>   #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_panic.h>
>   #include <drm/drm_plane.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/module.h>
> @@ -148,3 +149,49 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
>   	}
>   }
>   EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent);
> +
> +#if defined(CONFIG_DRM_PANIC)
> +/**
> + * @plane: DRM primary plane
> + * @drm_scanout_buffer: scanout buffer for the panic handler
> + * Returns: 0 or negative error code
> + *
> + * Generic get_scanout_buffer() implementation, for drivers that uses the
> + * drm_fb_dma_helper.
> + */
> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
> +				     struct drm_scanout_buffer *sb)
> +{
> +	struct drm_gem_dma_object *dma_obj;
> +	struct drm_framebuffer *fb;
> +
> +	fb = plane->state->fb;
> +	/* Only support linear modifier */
> +	if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
> +		return -ENODEV;


I think, the framebuffer format check clause here should be moved to the core layer.
For example, move this check into thedrm_panic_is_format_supported() function. And update the 
drm_panic_is_format_supported() function  to make it take 'struct drm_framebuffer *fb'
as argument. Because this check is needed for all implement, not driver specific or
implement specific.

I know that some display controller support TILE format, but then you can try to trigger modeset
to let the display controller using linear format to display. Or, you can also convert local
linear buffer(with content pained) to the device specific TILED framebuffer, then feed TILED
framebuffer to the display controller to scanout. This is something like reverse the process of
resolve, but the convert program is running on the CPU.  As panic is not performance critical,
so it's possible. This maybe a bit more difficult, but the idea behind this is that the goal of
this drm_panic_gem_get_scanout_buffer() function is just to get a buffer scanout-able. Other
things beyond that (like format checking) can be moved to upper level(the caller of it). So you
don't need to modify(update) the specific implement if one day you have some fancy idea implemented.


> +	dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> +
> +	/* Buffer should be accessible from the CPU */
> +	if (dma_obj->base.import_attach)
> +		return -ENODEV;
> +
> +	/* Buffer should be already mapped to CPU */
> +	if (!dma_obj->vaddr)
> +		return -ENODEV;


How about to call drm_gem_vmap_unlocked(dma_obj, &sb->map); ?

    

> +	iosys_map_set_vaddr(&sb->map, dma_obj->vaddr);
> +	sb->format = fb->format;
> +	sb->height = fb->height;
> +	sb->width = fb->width;
> +	sb->pitch = fb->pitches[0];
> +	return 0;
> +}
> +#else
> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
> +				     struct drm_scanout_buffer *sb)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer);
> diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h
> index d5e036c57801..61f24c2aba2f 100644
> --- a/include/drm/drm_fb_dma_helper.h
> +++ b/include/drm/drm_fb_dma_helper.h
> @@ -7,6 +7,7 @@
>   struct drm_device;
>   struct drm_framebuffer;
>   struct drm_plane_state;
> +struct drm_scanout_buffer;
>   
>   struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb,
>   	unsigned int plane);
> @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
>   				  struct drm_plane_state *old_state,
>   				  struct drm_plane_state *state);
>   
> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
> +				     struct drm_scanout_buffer *sb);
> +
>   #endif
>   

Re: [v9,9/9] drm/ast: Add drm_panic support

[Sui Jingfeng]

On 2024/3/7 17:14, Jocelyn Falempe wrote:
> Add support for the drm_panic module, which displays a message to
> the screen when a kernel panic occurs.
>
> v7
>   * Use drm_for_each_primary_visible_plane()
>
> v8:
>   * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
>     (Thomas Zimmermann)
> v9:
>   * Revert to using get_scanout_buffer() (Sima)
>   * move get_scanout_buffer() to plane helper functions
>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>


I have done very limited test with ast2600, basically works by trigger via debugfs interface as the cover-letter said.
so,

Tested-by: Sui Jingfeng <sui.jingfeng@linux.dev>

Re: [PATCH v9 0/9] drm/panic: Add a drm panic handler

[Jocelyn Falempe]

On 12/03/2024 16:16, Sui Jingfeng wrote:
> Hi,
> 
> 
> On 2024/3/7 17:14, Jocelyn Falempe wrote:
>> 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, mgag200, ast, 
>> and imx.
>>
>> To test it, make sure you're using one of the supported driver, and 
>> trigger a panic:
>> echo c > /proc/sysrq-trigger
>>
>> or you can enable CONFIG_DRM_PANIC_DEBUG and echo 1 > 
>> /sys/kernel/debug/dri/0/drm_panic_plane_0
> 
> Yes, the whole series works for me! I have tested with drm/loongson,
> Are you willing to add a implement for drm/loongson driver?
> If you do please add the fallowing code snippet info 
> drm/loongson/lsdc_plane.c.
> Thanks you.
> 
Thanks for testing, and for enabling drm_panic on loongson.

My plan is to have a first version of drm_panic merged before adding 
more drivers. You will be able to send this patch when drm_panic is 
merged in drm-misc-next, and I will review it.

Best regards,

-- 

Jocelyn

> 
> static int
> lsdc_primary_plane_get_scanout_buffer(struct drm_plane *plane,
>                        struct drm_scanout_buffer *scanout)
> {
>      struct drm_framebuffer *fb;
> 
>      if (!plane->state || !plane->state->fb)
>          return -ENODEV;
> 
>      fb = plane->state->fb;
>      if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
>          return -ENODEV;
> 
>      scanout->format = fb->format;
>      scanout->width = fb->width;
>      scanout->height = fb->height;
>      scanout->pitch = fb->pitches[0];
> 
>      return drm_gem_vmap_unlocked(fb->obj[0], &scanout->map);
> }
> 
> hook this lsdc_primary_plane_get_scanout_buffer() up to the 
> .get_scanout_buffer
> member of lsdc_primary_helper_funcs, and include the #include 
> <drm/drm_panic.h>
> Thanks you in advance!
> 

Re: [v9,5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic

[Jocelyn Falempe]

On 12/03/2024 17:44, Sui Jingfeng wrote:
> Hi,
> 
> I'm get attracted by your patch, so I want to comment.
> Please correct or ignore me if I say something wrong.
> 
> On 2024/3/7 17:14, Jocelyn Falempe wrote:
>> This was initialy done for imx6, but should work on most drivers
>> using drm_fb_dma_helper.
>>
>> v8:
>>   * Replace get_scanout_buffer() logic with drm_panic_set_buffer()
>>     (Thomas Zimmermann)
>>
>> v9:
>>   * go back to get_scanout_buffer()
>>   * move get_scanout_buffer() to plane helper functions
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_fb_dma_helper.c | 47 +++++++++++++++++++++++++++++
>>   include/drm/drm_fb_dma_helper.h     |  4 +++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c 
>> b/drivers/gpu/drm/drm_fb_dma_helper.c
>> index 3b535ad1b07c..010327069ad4 100644
>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
>> @@ -15,6 +15,7 @@
>>   #include <drm/drm_framebuffer.h>
>>   #include <drm/drm_gem_dma_helper.h>
>>   #include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_panic.h>
>>   #include <drm/drm_plane.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/module.h>
>> @@ -148,3 +149,49 @@ void drm_fb_dma_sync_non_coherent(struct 
>> drm_device *drm,
>>       }
>>   }
>>   EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent);
>> +
>> +#if defined(CONFIG_DRM_PANIC)
>> +/**
>> + * @plane: DRM primary plane
>> + * @drm_scanout_buffer: scanout buffer for the panic handler
>> + * Returns: 0 or negative error code
>> + *
>> + * Generic get_scanout_buffer() implementation, for drivers that uses 
>> the
>> + * drm_fb_dma_helper.
>> + */
>> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
>> +                     struct drm_scanout_buffer *sb)
>> +{
>> +    struct drm_gem_dma_object *dma_obj;
>> +    struct drm_framebuffer *fb;
>> +
>> +    fb = plane->state->fb;
>> +    /* Only support linear modifier */
>> +    if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
>> +        return -ENODEV;
> 
> 
> I think, the framebuffer format check clause here should be moved to the 
> core layer.
> For example, move this check into thedrm_panic_is_format_supported() 
> function. And update the drm_panic_is_format_supported() function  to 
> make it take 'struct drm_framebuffer *fb'
> as argument. Because this check is needed for all implement, not driver 
> specific or
> implement specific.

I'm unsure about this. I think for some drivers it might be easier to 
give a memory buffer different from the plane's drm_framebuffer, and do 
their own specific things to display it. So forcing the use of a 
drm_framebuffer will remove some flexibility.

> 
> I know that some display controller support TILE format, but then you 
> can try to trigger modeset
> to let the display controller using linear format to display. Or, you 
> can also convert local
> linear buffer(with content pained) to the device specific TILED 
> framebuffer, then feed TILED
> framebuffer to the display controller to scanout. This is something like 
> reverse the process of
> resolve, but the convert program is running on the CPU.  As panic is not 
> performance critical,
> so it's possible. This maybe a bit more difficult, but the idea behind 
> this is that the goal of
> this drm_panic_gem_get_scanout_buffer() function is just to get a buffer 
> scanout-able. Other
> things beyond that (like format checking) can be moved to upper 
> level(the caller of it). So you
> don't need to modify(update) the specific implement if one day you have 
> some fancy idea implemented.
> 
> 

Correct me if I'm wrong, but the tiled format are mostly hardware 
dependent, and I don't think we can have a generic implementation, that 
can cover multiple hardware.

I want to add support for multi-planar format like YUV for drm_panic 
later, but for tiled buffer, I think it should be easier to deactivate 
tiling on the hardware itself.

>> +    dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> +
>> +    /* Buffer should be accessible from the CPU */
>> +    if (dma_obj->base.import_attach)
>> +        return -ENODEV;
>> +
>> +    /* Buffer should be already mapped to CPU */
>> +    if (!dma_obj->vaddr)
>> +        return -ENODEV;
> 
> 
> How about to call drm_gem_vmap_unlocked(dma_obj, &sb->map); ?

It is not safe to call it from panic context, because it takes locks:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_gem.c#L1212

It may lockup the panic handler, and prevent other panic routine to run 
(like kdump).
So it's better to call drm_gem_vmap_unlocked() when the driver prepares 
the buffer for the primary plane, than doing it from the panic handler.


Best regards,

-- 

Jocelyn


> 
> 
>> +    iosys_map_set_vaddr(&sb->map, dma_obj->vaddr);
>> +    sb->format = fb->format;
>> +    sb->height = fb->height;
>> +    sb->width = fb->width;
>> +    sb->pitch = fb->pitches[0];
>> +    return 0;
>> +}
>> +#else
>> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
>> +                     struct drm_scanout_buffer *sb)
>> +{
>> +    return -ENODEV;
>> +}
>> +#endif
>> +EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer);
>> diff --git a/include/drm/drm_fb_dma_helper.h 
>> b/include/drm/drm_fb_dma_helper.h
>> index d5e036c57801..61f24c2aba2f 100644
>> --- a/include/drm/drm_fb_dma_helper.h
>> +++ b/include/drm/drm_fb_dma_helper.h
>> @@ -7,6 +7,7 @@
>>   struct drm_device;
>>   struct drm_framebuffer;
>>   struct drm_plane_state;
>> +struct drm_scanout_buffer;
>>   struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct 
>> drm_framebuffer *fb,
>>       unsigned int plane);
>> @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device 
>> *drm,
>>                     struct drm_plane_state *old_state,
>>                     struct drm_plane_state *state);
>> +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane,
>> +                     struct drm_scanout_buffer *sb);
>> +
>>   #endif
> 

Re: [PATCH v9 1/9] drm/panic: Add drm panic locking

[Daniel Vetter]

On Fri, Mar 08, 2024 at 02:45:52PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.03.24 um 10:14 schrieb Jocelyn Falempe:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Rough sketch for the locking of drm panic printing code. The upshot of
> > this approach is that we can pretty much entirely rely on the atomic
> > commit flow, with the pair of raw_spin_lock/unlock providing any
> > barriers we need, without having to create really big critical
> > sections in code.
> 
> The ast driver has a lock to protect modesetting and concurrent EDID reads
> from each other. [1] That new panic_lock seems to serve the same purpose.
> 
> If we go that route, can we make this a bit more generic and call it
> commit_lock? I could then remove the dedicated lock from ast.

No, because the drm_panic_lock/unlock sections must be as small as
possible for two reasons:

- Anything we do while holding this lock that isn'st strictly needed for
  the panic code (like edid reading or more than the scanout address
  registers) will reduce the chances that the drm panic handler can run,
  since all we can do is try_lock.

- it's a raw spinlock, if you do more than a handful of instructions
  you'll really annoy the -rt people, because raw spinlocks are not
  converted to sleeping locks with the realtime config enabled. Reading an
  EDID (which takes upwards of tens of ms) is definitely about 4-5 orders
  of magnitudes to much at least.

The mutex is really the right lock here for protecting modesets against
edid reads here, and the panic lock is at most on top of that very, very
small and specific things.
-Sima

> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.7/source/drivers/gpu/drm/ast/ast_drv.h#L195
> 
> > 
> > This also avoids the need that drivers must explicitly update the
> > panic handler state, which they might forget to do, or not do
> > consistently, and then we blow up in the worst possible times.
> > 
> > It is somewhat racy against a concurrent atomic update, and we might
> > write into a buffer which the hardware will never display. But there's
> > fundamentally no way to avoid that - if we do the panic state update
> > explicitly after writing to the hardware, we might instead write to an
> > old buffer that the user will barely ever see.
> > 
> > Note that an rcu protected deference of plane->state would give us the
> > the same guarantees, but it has the downside that we then need to
> > protect the plane state freeing functions with call_rcu too. Which
> > would very widely impact a lot of code and therefore doesn't seem
> > worth the complexity compared to a raw spinlock with very tiny
> > critical sections. Plus rcu cannot be used to protect access to
> > peek/poke registers anyway, so we'd still need it for those cases.
> > 
> > Peek/poke registers for vram access (or a gart pte reserved just for
> > panic code) are also the reason I've gone with a per-device and not
> > per-plane spinlock, since usually these things are global for the
> > entire display. Going with per-plane locks would mean drivers for such
> > hardware would need additional locks, which we don't want, since it
> > deviates from the per-console takeoverlocks design.
> > 
> > Longer term it might be useful if the panic notifiers grow a bit more
> > structure than just the absolute bare
> > EXPORT_SYMBOL(panic_notifier_list) - somewhat aside, why is that not
> > EXPORT_SYMBOL_GPL ... If panic notifiers would be more like console
> > drivers with proper register/unregister interfaces we could perhaps
> > reuse the very fancy console lock with all it's check and takeover
> > semantics that John Ogness is developing to fix the console_lock mess.
> > But for the initial cut of a drm panic printing support I don't think
> > we need that, because the critical sections are extremely small and
> > only happen once per display refresh. So generally just 60 tiny locked
> > sections per second, which is nothing compared to a serial console
> > running a 115kbaud doing really slow mmio writes for each byte. So for
> > now the raw spintrylock in drm panic notifier callback should be good
> > enough.
> > 
> > Another benefit of making panic notifiers more like full blown
> > consoles (that are used in panics only) would be that we get the two
> > stage design, where first all the safe outputs are used. And then the
> > dangerous takeover tricks are deployed (where for display drivers we
> > also might try to intercept any in-flight display buffer flips, which
> > if we race and misprogram fifos and watermarks can hang the memory
> > controller on some hw).
> > 
> > For context the actual implementation on the drm side is by Jocelyn
> > and this patch is meant to be combined with the overall approach in
> > v7 (v8 is a bit less flexible, which I think is the wrong direction):
> > 
> > https://lore.kernel.org/dri-devel/20240104160301.185915-1-jfalempe@redhat.com/
> > 
> > Note that the locking is very much not correct there, hence this
> > separate rfc.
> > 
> > v2:
> > - fix authorship, this was all my typing
> > - some typo oopsies
> > - link to the drm panic work by Jocelyn for context
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jocelyn Falempe <jfalempe@redhat.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Petr Mladek <pmladek@suse.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: John Ogness <john.ogness@linutronix.de>
> > Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >   drivers/gpu/drm/drm_atomic_helper.c |  3 +
> >   drivers/gpu/drm/drm_drv.c           |  1 +
> >   include/drm/drm_mode_config.h       | 10 +++
> >   include/drm/drm_panic.h             | 99 +++++++++++++++++++++++++++++
> >   4 files changed, 113 insertions(+)
> >   create mode 100644 include/drm/drm_panic.h
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 39ef0a6addeb..c0bb91312fb2 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -38,6 +38,7 @@
> >   #include <drm/drm_drv.h>
> >   #include <drm/drm_framebuffer.h>
> >   #include <drm/drm_gem_atomic_helper.h>
> > +#include <drm/drm_panic.h>
> >   #include <drm/drm_print.h>
> >   #include <drm/drm_self_refresh_helper.h>
> >   #include <drm/drm_vblank.h>
> > @@ -3099,6 +3100,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >   		}
> >   	}
> > +	drm_panic_lock(state->dev);
> >   	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> >   		WARN_ON(plane->state != old_plane_state);
> > @@ -3108,6 +3110,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> >   		state->planes[i].state = old_plane_state;
> >   		plane->state = new_plane_state;
> >   	}
> > +	drm_panic_unlock(state->dev);
> >   	for_each_oldnew_private_obj_in_state(state, obj, old_obj_state, new_obj_state, i) {
> >   		WARN_ON(obj->state != old_obj_state);
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 243cacb3575c..c157500b3135 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -638,6 +638,7 @@ static int drm_dev_init(struct drm_device *dev,
> >   	mutex_init(&dev->filelist_mutex);
> >   	mutex_init(&dev->clientlist_mutex);
> >   	mutex_init(&dev->master_mutex);
> > +	raw_spin_lock_init(&dev->mode_config.panic_lock);
> >   	ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL);
> >   	if (ret)
> > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > index 973119a9176b..e79f1a557a22 100644
> > --- a/include/drm/drm_mode_config.h
> > +++ b/include/drm/drm_mode_config.h
> > @@ -505,6 +505,16 @@ struct drm_mode_config {
> >   	 */
> >   	struct list_head plane_list;
> > +	/**
> > +	 * @panic_lock:
> > +	 *
> > +	 * Raw spinlock used to protect critical sections of code that access
> > +	 * the display hardware or modeset software state, which the panic
> > +	 * printing code must be protected against. See drm_panic_trylock(),
> > +	 * drm_panic_lock() and drm_panic_unlock().
> > +	 */
> > +	struct raw_spinlock panic_lock;
> > +
> >   	/**
> >   	 * @num_crtc:
> >   	 *
> > diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> > new file mode 100644
> > index 000000000000..f2135d03f1eb
> > --- /dev/null
> > +++ b/include/drm/drm_panic.h
> > @@ -0,0 +1,99 @@
> > +/* SPDX-License-Identifier: GPL-2.0 or MIT */
> > +#ifndef __DRM_PANIC_H__
> > +#define __DRM_PANIC_H__
> > +
> > +#include <drm/drm_device.h>
> > +/*
> > + * Copyright (c) 2024 Intel
> > + */
> > +
> > +/**
> > + * drm_panic_trylock - try to enter the panic printing critical section
> > + * @dev: struct drm_device
> > + *
> > + * This function must be called by any panic printing code. The panic printing
> > + * attempt must be aborted if the trylock fails.
> > + *
> > + * Panic printing code can make the following assumptions while holding the
> > + * panic lock:
> > + *
> > + * - Anything protected by drm_panic_lock() and drm_panic_unlock() pairs is safe
> > + *   to access.
> > + *
> > + * - Furthermore the panic printing code only registers in drm_dev_unregister()
> > + *   and gets removed in drm_dev_unregister(). This allows the panic code to
> > + *   safely access any state which is invariant in between these two function
> > + *   calls, like the list of planes drm_mode_config.plane_list or most of the
> > + *   struct drm_plane structure.
> > + *
> > + * Specifically thanks to the protection around plane updates in
> > + * drm_atomic_helper_swap_state() the following additional guarantees hold:
> > + *
> > + * - It is safe to deference the drm_plane.state pointer.
> > + *
> > + * - Anything in struct drm_plane_state or the driver's subclass thereof which
> > + *   stays invariant after the atomic check code has finished is safe to access.
> > + *   Specifically this includes the reference counted pointers to framebuffer
> > + *   and buffer objects.
> > + *
> > + * - Anything set up by drm_plane_helper_funcs.fb_prepare and cleaned up
> > + *   drm_plane_helper_funcs.fb_cleanup is safe to access, as long as it stays
> > + *   invariant between these two calls. This also means that for drivers using
> > + *   dynamic buffer management the framebuffer is pinned, and therefer all
> > + *   relevant datastructures can be accessed without taking any further locks
> > + *   (which would be impossible in panic context anyway).
> > + *
> > + * - Importantly, software and hardware state set up by
> > + *   drm_plane_helper_funcs.begin_fb_access and
> > + *   drm_plane_helper_funcs.end_fb_access is not safe to access.
> > + *
> > + * Drivers must not make any assumptions about the actual state of the hardware,
> > + * unless they explicitly protected these hardware access with drm_panic_lock()
> > + * and drm_panic_unlock().
> > + *
> > + * Returns:
> > + *
> > + * 0 when failing to acquire the raw spinlock, nonzero on success.
> > + */
> > +static inline int drm_panic_trylock(struct drm_device *dev)
> > +{
> > +	return raw_spin_trylock(&dev->mode_config.panic_lock);
> > +}
> > +
> > +/**
> > + * drm_panic_lock - protect panic printing relevant state
> > + * @dev: struct drm_device
> > + *
> > + * This function must be called to protect software and hardware state that the
> > + * panic printing code must be able to rely on. The protected sections must be
> > + * as small as possible. Examples include:
> > + *
> > + * - Access to peek/poke or other similar registers, if that is the way the
> > + *   driver prints the pixels into the scanout buffer at panic time.
> > + *
> > + * - Updates to pointers like drm_plane.state, allowing the panic handler to
> > + *   safely deference these. This is done in drm_atomic_helper_swap_state().
> > + *
> > + * - An state that isn't invariant and that the driver must be able to access
> > + *   during panic printing.
> > + *
> > + * Call drm_panic_unlock() to unlock the locked spinlock.
> > + */
> > +static inline void drm_panic_lock(struct drm_device *dev)
> > +{
> > +	return raw_spin_lock(&dev->mode_config.panic_lock);
> > +}
> > +
> > +/**
> > + * drm_panic_unlock - end of the panic printing critical section
> > + * @dev: struct drm_device
> > + *
> > + * Unlocks the raw spinlock acquired by either drm_panic_lock() or
> > + * drm_panic_trylock().
> > + */
> > +static inline void drm_panic_unlock(struct drm_device *dev)
> > +{
> > +	raw_spin_unlock(&dev->mode_config.panic_lock);
> > +}
> > +
> > +#endif /* __DRM_PANIC_H__ */
> 
> -- 
> --
> 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)
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch