[PATCH 0/3] drm: Add panic handling

[PATCH 0/3] drm: Add panic handling

[Noralf Trønnes]

This patchset adds a way for DRM drivers to display kernel messages
during panic().

This version have lots of changes based on Daniel's review of the RFC
where he proposed a different way of interacting with the driver.

I have used this script to run some tests on the rendering code:
https://gist.github.com/notro/4f42ec20e1be1d47c98fa749ca53c805


Noralf.


Noralf Trønnes (3):
  drm: Add support for panic message output
  drm/fb_cma_helper: Add panic handling
  drm/simpledrm: Add panic handling

 drivers/gpu/drm/Kconfig                   |   1 +
 drivers/gpu/drm/Makefile                  |   2 +-
 drivers/gpu/drm/drm_drv.c                 |   3 +
 drivers/gpu/drm/drm_fb_cma_helper.c       |  10 +
 drivers/gpu/drm/drm_framebuffer.c         | 108 +++++
 drivers/gpu/drm/drm_internal.h            |   4 +
 drivers/gpu/drm/drm_panic.c               | 640 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/simpledrm/simpledrm_kms.c |  11 +
 include/drm/drm_framebuffer.h             |  40 ++
 9 files changed, 818 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_panic.c

--
2.8.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm: Add support for panic message output

[Noralf Trønnes]

This adds support for outputting kernel messages on panic().
A circular buffer is used to collect kernel messages.
On panic() the notifier function loops over each DRM device and
it's crtc's to find suitable framebuffers. On the next
console->write(), the messages in the circular buffer are rendered
on each of the recorded framebuffers.

Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/Kconfig           |   1 +
 drivers/gpu/drm/Makefile          |   2 +-
 drivers/gpu/drm/drm_drv.c         |   3 +
 drivers/gpu/drm/drm_framebuffer.c | 108 +++++++
 drivers/gpu/drm/drm_internal.h    |   4 +
 drivers/gpu/drm/drm_panic.c       | 640 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_framebuffer.h     |  40 +++
 7 files changed, 797 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_panic.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 4a888e6..7ded50a 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -13,6 +13,7 @@ menuconfig DRM
 	select I2C_ALGOBIT
 	select DMA_SHARED_BUFFER
 	select SYSFB
+	select FONT_SUPPORT
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 3f2e43f..667685c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -13,7 +13,7 @@ drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
 		drm_trace_points.o drm_global.o drm_prime.o \
 		drm_rect.o drm_vma_manager.o drm_flip_work.o \
 		drm_modeset_lock.o drm_atomic.o drm_bridge.o \
-		drm_framebuffer.o drm_connector.o drm_blend.o
+		drm_framebuffer.o drm_connector.o drm_blend.o drm_panic.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index acf6a5f..fb36fba 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -845,6 +845,8 @@ static int __init drm_core_init(void)
 		goto err_p3;
 	}
 
+	drm_panic_init(drm_debugfs_root);
+
 	DRM_INFO("Initialized %s %d.%d.%d %s\n",
 		 CORE_NAME, CORE_MAJOR, CORE_MINOR, CORE_PATCHLEVEL, CORE_DATE);
 	return 0;
@@ -860,6 +862,7 @@ err_p1:
 
 static void __exit drm_core_exit(void)
 {
+	drm_panic_exit(drm_debugfs_root);
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
 
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 30dc01e..84ee4f7 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -20,6 +20,7 @@
  * OF THIS SOFTWARE.
  */
 
+#include <asm/unaligned.h>
 #include <linux/export.h>
 #include <drm/drmP.h>
 #include <drm/drm_auth.h>
@@ -829,3 +830,110 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
 	drm_framebuffer_unreference(fb);
 }
 EXPORT_SYMBOL(drm_framebuffer_remove);
+
+/**
+ * drm_framebuffer_panic_draw_xy - draw pixel on fb during panic()
+ * @fb: DRM framebuffer
+ * @vmap: Linear virtual mapping
+ * @x: X coordinate
+ * @y: Y coordinate
+ * @foreground: Foreground pixel
+ *
+ * This function can be used to draw a pixel during panic message rendering.
+ * It requires @vmap to be a linear mapping. This is the default implementation
+ * used if &drm_framebuffer_funcs->panic_draw_xy is not set.
+ */
+void drm_framebuffer_panic_draw_xy(struct drm_framebuffer *fb, void *vmap,
+				   int x, int y, bool foreground)
+{
+	u8 *dst = vmap + fb->offsets[0] + (y * fb->pitches[0]);
+
+	switch (fb->pixel_format & ~DRM_FORMAT_BIG_ENDIAN) {
+	case DRM_FORMAT_C8:
+
+	case DRM_FORMAT_RGB332:
+	case DRM_FORMAT_BGR233:
+		dst += x;
+		*dst = foreground ? 0xff : 0x00;
+		break;
+
+	case DRM_FORMAT_XRGB4444:
+	case DRM_FORMAT_ARGB4444:
+	case DRM_FORMAT_XBGR4444:
+	case DRM_FORMAT_ABGR4444:
+		dst += x * sizeof(u16);
+		put_unaligned(foreground ? 0x0fff : 0x0000, (u16 *)dst);
+		break;
+
+	case DRM_FORMAT_RGBX4444:
+	case DRM_FORMAT_RGBA4444:
+	case DRM_FORMAT_BGRX4444:
+	case DRM_FORMAT_BGRA4444:
+		dst += x * sizeof(u16);
+		put_unaligned(foreground ? 0xfff0 : 0x0000, (u16 *)dst);
+		break;
+
+	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
+	case DRM_FORMAT_XBGR1555:
+	case DRM_FORMAT_ABGR1555:
+		dst += x * sizeof(u16);
+		put_unaligned(foreground ? 0x7fff : 0x0000, (u16 *)dst);
+		break;
+
+	case DRM_FORMAT_RGBX5551:
+	case DRM_FORMAT_RGBA5551:
+	case DRM_FORMAT_BGRX5551:
+	case DRM_FORMAT_BGRA5551:
+		dst += x * sizeof(u16);
+		put_unaligned(foreground ? 0xfffe : 0x0000, (u16 *)dst);
+		break;
+
+	case DRM_FORMAT_RGB565:
+	case DRM_FORMAT_BGR565:
+		dst += x * sizeof(u16);
+		put_unaligned(foreground ? 0xffff : 0x0000, (u16 *)dst);
+		break;
+
+	case DRM_FORMAT_RGB888:
+	case DRM_FORMAT_BGR888:
+		dst += x * 3;
+		dst[0] = foreground ? 0xff : 0x00;
+		dst[1] = foreground ? 0xff : 0x00;
+		dst[2] = foreground ? 0xff : 0x00;
+		break;
+
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
+		dst += x * sizeof(u32);
+		put_unaligned(foreground ? 0x00ffffff : 0x0, (u32 *)dst);
+		break;
+
+	case DRM_FORMAT_RGBX8888:
+	case DRM_FORMAT_RGBA8888:
+	case DRM_FORMAT_BGRX8888:
+	case DRM_FORMAT_BGRA8888:
+		dst += x * sizeof(u32);
+		put_unaligned(foreground ? 0xffffff00 : 0x0, (u32 *)dst);
+		break;
+
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_ARGB2101010:
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ABGR2101010:
+		dst += x * sizeof(u32);
+		put_unaligned(foreground ? 0x3fffffff : 0x0, (u32 *)dst);
+		break;
+
+	case DRM_FORMAT_RGBX1010102:
+	case DRM_FORMAT_RGBA1010102:
+	case DRM_FORMAT_BGRX1010102:
+	case DRM_FORMAT_BGRA1010102:
+		dst += x * sizeof(u32);
+		put_unaligned(foreground ? 0xfffffffc : 0x0, (u32 *)dst);
+		break;
+	}
+}
+EXPORT_SYMBOL(drm_framebuffer_panic_draw_xy);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index b86dc9b..c47b6a6 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -90,6 +90,10 @@ int drm_gem_open_ioctl(struct drm_device *dev, void *data,
 void drm_gem_open(struct drm_device *dev, struct drm_file *file_private);
 void drm_gem_release(struct drm_device *dev, struct drm_file *file_private);
 
+/* drm_panic.c */
+void drm_panic_init(struct dentry *debugfs_root);
+void drm_panic_exit(struct dentry *debugfs_root);
+
 /* drm_debugfs.c */
 #if defined(CONFIG_DEBUG_FS)
 int drm_debugfs_init(struct drm_minor *minor, int minor_id,
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
new file mode 100644
index 0000000..a6d4653
--- /dev/null
+++ b/drivers/gpu/drm/drm_panic.c
@@ -0,0 +1,640 @@
+/*
+ * Copyright 2016 Noralf Trønnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <linux/console.h>
+#include <linux/debugfs.h>
+#include <linux/font.h>
+#include <linux/kernel.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include "drm_internal.h"
+
+struct drm_panic_fb {
+	struct drm_framebuffer *fb;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	unsigned int width;
+	unsigned int height;
+	void *vmap;
+	const struct font_desc *font;
+	unsigned int cols;
+	unsigned int rows;
+	unsigned int xpos;
+	unsigned int ypos;
+};
+
+#define DRM_PANIC_MAX_FBS	64
+static struct drm_panic_fb drm_panic_fbs[DRM_PANIC_MAX_FBS];
+static unsigned int drm_panic_fbs_num;
+
+/* circular kernel message buffer */
+#define DRM_PANIC_MAX_KMSGS	SZ_4K
+static char *drm_panic_kmsgs;
+static size_t drm_panic_kmsgs_pos;
+
+/* enables message rendering */
+static bool drm_panic_active;
+
+/*
+ * TODO
+ * This simple out-of-band logging has been very useful during the initial
+ * development. Not sure if we should keep it when the code has settled.
+ * It's roughly 80 lines.
+ */
+#define DRM_PANIC_LOG_SIZE	512
+
+static char *log_buf;
+static size_t log_pos;
+
+static __printf(1, 2) void drm_panic_log(const char *fmt, ...)
+{
+	unsigned int i;
+	char buf[128];
+	va_list args;
+	size_t len;
+
+	if (oops_in_progress || !log_buf || !IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
+	va_start(args, fmt);
+
+	if (log_pos >= DRM_PANIC_LOG_SIZE)
+		log_pos = 0;
+
+	len = vscnprintf(buf, sizeof(buf), fmt, args);
+
+	for (i = 0; i < len; i++) {
+		log_buf[log_pos++] = buf[i];
+		if (log_pos >= DRM_PANIC_LOG_SIZE)
+			log_pos = 0;
+	}
+
+	va_end(args);
+}
+
+static void drm_panic_draw_xy(struct drm_panic_fb *pfb, int x, int y, bool fg)
+{
+	struct drm_framebuffer *fb = pfb->fb;
+
+	if (x < 0 || x >= pfb->width || y < 0 || y >= pfb->height)
+		return;
+
+	if (fb->funcs->panic_draw_xy)
+		fb->funcs->panic_draw_xy(fb, pfb->vmap, x, y, fg);
+	else
+		drm_framebuffer_panic_draw_xy(fb, pfb->vmap, x, y, fg);
+}
+
+static void drm_panic_render_char(struct drm_panic_fb *pfb, unsigned int x,
+				  unsigned int y, char c)
+{
+	const struct font_desc *font = pfb->font;
+	unsigned int h, w, y_pix, x_pix;
+	u8 fontline;
+
+	for (h = 0; h < font->height; h++) {
+		fontline = *(u8 *)(font->data + c * font->height + h);
+		y_pix = y * font->height + h;
+
+		for (w = 0; w < font->width; w++) {
+			x_pix = x * font->width + w;
+
+			drm_panic_draw_xy(pfb, x_pix, y_pix,
+					  fontline & BIT(7 - w));
+		}
+	}
+}
+
+static void drm_panic_render_rest_of_screen_blank(struct drm_panic_fb *pfb)
+{
+	unsigned int x, y, ypos;
+
+	ypos = pfb->xpos ? pfb->ypos + 1 : pfb->ypos;
+
+	for (y = ypos * pfb->font->height; y < pfb->height; y++)
+		for (x = 0; x < pfb->width; x++)
+			drm_panic_draw_xy(pfb, x, y, 0);
+}
+
+static void drm_panic_render_rest_of_line_blank(struct drm_panic_fb *pfb)
+{
+	unsigned int h, x, y = pfb->ypos * pfb->font->height;
+
+	for (h = 0; h < pfb->font->height; h++)
+		for (x = pfb->xpos * pfb->font->width; x < pfb->width; x++)
+			drm_panic_draw_xy(pfb, x, y + h, 0);
+}
+
+static void drm_panic_render(struct drm_panic_fb *pfb,
+			     const char *text, unsigned int len)
+{
+	unsigned int i;
+
+	for (i = 0; i < len; i++) {
+		if (pfb->ypos >= pfb->rows)
+			return;
+
+		if (text[i] == '\n') {
+			drm_panic_render_rest_of_line_blank(pfb);
+			pfb->xpos = pfb->cols;
+		} else {
+			drm_panic_render_char(pfb, pfb->xpos++, pfb->ypos,
+					      text[i]);
+		}
+
+		if (pfb->xpos >= pfb->cols) {
+			pfb->xpos = 0;
+			pfb->ypos++;
+		}
+	}
+}
+
+static void drm_panic_try_write_kmsgs(struct drm_panic_fb *pfb, char **str,
+				      unsigned int *len, unsigned int *xpos,
+				      unsigned int *ypos)
+{
+	char *s = *str + *len - 1;
+	unsigned int l = 0;
+
+	if (*len == 0)
+		return;
+
+	if (*ypos == pfb->rows && *s == '\n') {
+		s--;
+		l++;
+		(*ypos)--;
+	}
+
+	while (l < *len) {
+		if (*xpos == pfb->cols) {
+			*xpos = 0;
+			if (*ypos == 0) {
+				s++;
+				l--;
+				break;
+			}
+
+			(*ypos)--;
+		}
+
+		if (*s-- == '\n')
+			*xpos = pfb->cols;
+		else
+			(*xpos)++;
+
+		l++;
+	}
+
+	*len = l;
+	*str = s + 1;
+}
+
+static void drm_panic_write_kmsgs(struct drm_panic_fb *pfb)
+{
+	char *str1 = &drm_panic_kmsgs[drm_panic_kmsgs_pos];
+	unsigned int len1 = DRM_PANIC_MAX_KMSGS - drm_panic_kmsgs_pos;
+	char *str2 = drm_panic_kmsgs;
+	unsigned int len2 = drm_panic_kmsgs_pos;
+	unsigned int xpos = 0;
+	unsigned int ypos = pfb->rows;
+
+	pfb->xpos = 0;
+	pfb->ypos = 0;
+
+	/* if the buffer hasn't wrapped around */
+	if (drm_panic_kmsgs[drm_panic_kmsgs_pos] == '\0')
+		len1 = 0;
+
+	/* try writing backwards to find where to begin */
+	drm_panic_try_write_kmsgs(pfb, &str2, &len2, &xpos, &ypos);
+
+	if (ypos == 0)
+		len1 = 0;
+	else
+		drm_panic_try_write_kmsgs(pfb, &str1, &len1, &xpos, &ypos);
+
+	drm_panic_render(pfb, str1, len1);
+	drm_panic_render(pfb, str2, len2);
+	drm_panic_render_rest_of_screen_blank(pfb);
+}
+
+static void drm_panic_write(struct drm_panic_fb *pfb, const char *str,
+			    unsigned int len)
+{
+	unsigned int xpos = pfb->xpos;
+	unsigned int ypos = pfb->ypos;
+	unsigned int i;
+
+	/* first time or screen is full: dump buffer */
+	if ((!pfb->xpos && !pfb->ypos) || pfb->ypos >= pfb->rows) {
+		drm_panic_write_kmsgs(pfb);
+		return;
+	}
+
+	/* see if there's room to append this string */
+	for (i = 0; i < len; i++) {
+		if (xpos == pfb->cols) {
+			xpos = 0;
+			ypos++;
+		}
+
+		if (str[i] == '\n')
+			xpos = pfb->cols;
+		else
+			xpos++;
+	}
+
+	if (ypos <= pfb->rows)
+		drm_panic_render(pfb, str, len);
+	else
+		drm_panic_write_kmsgs(pfb);
+}
+
+/*
+ * Calls to console.write() are serialized by console_lock().
+ * However, panic() calls console_flush_on_panic() which breaks the lock if
+ * necessary.
+ */
+static void drm_panic_console_write(struct console *con,
+				    const char *str, unsigned int len)
+{
+	unsigned int i;
+
+	if (!len)
+		return;
+
+	if (drm_panic_kmsgs_pos >= DRM_PANIC_MAX_KMSGS)
+		drm_panic_kmsgs_pos = 0;
+
+	for (i = 0; i < len; i++) {
+		drm_panic_kmsgs[drm_panic_kmsgs_pos++] = str[i];
+		if (drm_panic_kmsgs_pos >= DRM_PANIC_MAX_KMSGS)
+			drm_panic_kmsgs_pos = 0;
+	}
+
+	if (drm_panic_active)
+		for (i = 0; i < drm_panic_fbs_num; i++)
+			drm_panic_write(&drm_panic_fbs[i], str, len);
+}
+
+static struct console drm_panic_console = {
+	.name = "drmpanic",
+	.write = drm_panic_console_write,
+	.flags = CON_PRINTBUFFER | CON_ENABLED,
+	.index = 0,
+};
+
+static bool drm_panic_add_fb(struct drm_crtc *crtc, struct drm_plane *plane)
+{
+	unsigned int width = drm_rect_width(&plane->state->src) >> 16;
+	unsigned int height = drm_rect_height(&plane->state->src) >> 16;
+	struct drm_framebuffer *fb = plane->fb;
+	const struct font_desc *font;
+	struct drm_panic_fb *pfb;
+	void *vmap;
+
+	if (!width || !height || !fb || !fb->funcs || !fb->funcs->panic_vmap)
+		return false;
+
+	/* only 8-bit wide fonts are supported */
+	font = get_default_font(width, height, BIT(7), -1);
+	if (!font) {
+		drm_panic_log("Couldn't get font\n");
+		return false;
+	}
+
+	vmap = fb->funcs->panic_vmap(fb);
+	if (!vmap) {
+		drm_panic_log("panic_vmap() returned NULL\n");
+		return false;
+	}
+
+	pfb = &drm_panic_fbs[drm_panic_fbs_num++];
+	pfb->plane = plane;
+	pfb->crtc = crtc;
+	pfb->fb = fb;
+	pfb->width = width;
+	pfb->height = height;
+	pfb->vmap = vmap;
+	pfb->font = font;
+	pfb->cols = pfb->width / font->width;
+	pfb->rows = pfb->height / font->height;
+
+	drm_panic_log("    [FB:%d] %ux%u->%ux%u, %s, format=0x%08x\n",
+		      fb->base.id, pfb->width, pfb->height, pfb->cols,
+		      pfb->rows, font->name, fb->pixel_format);
+
+	return true;
+}
+
+static void drm_panic_add(struct drm_device *drm)
+{
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+
+	if (!drm || !drm->driver ||
+	    !(drm->driver->driver_features & DRIVER_ATOMIC))
+		return;
+
+	drm_panic_log("%s on minor %d\n", drm->driver->name,
+		      drm->primary ? drm->primary->index : -1);
+
+	drm_for_each_crtc(crtc, drm) {
+		drm_panic_log("  %s\n", crtc->name);
+
+		if (drm_panic_fbs_num >= DRM_PANIC_MAX_FBS)
+			return;
+
+		if (!ww_mutex_trylock(&crtc->mutex.mutex))
+			continue;
+
+		if (!crtc->enabled || !crtc->primary)
+			goto crtc_unlock;
+
+		if (!crtc->state || !crtc->state->active)
+			goto crtc_unlock;
+
+		plane = crtc->primary;
+		if (!ww_mutex_trylock(&plane->mutex.mutex))
+			goto crtc_unlock;
+
+		if (!plane->state || !plane->state->visible)
+			goto plane_unlock;
+
+		if (drm_panic_add_fb(crtc, plane))
+			continue;
+
+plane_unlock:
+		ww_mutex_unlock(&plane->mutex.mutex);
+crtc_unlock:
+		ww_mutex_unlock(&crtc->mutex.mutex);
+	}
+}
+
+static int drm_panic_class_iter(struct device *dev, void *data)
+{
+	struct drm_minor *minor;
+
+	minor = dev_get_drvdata(dev);
+
+	if (minor && minor->type == DRM_MINOR_PRIMARY)
+		drm_panic_add(minor->dev);
+
+	return 0;
+}
+
+/*
+ * The panic() function makes sure that only one CPU is allowed to run it's
+ * code, but a new panic can be triggered during it's processing.
+ *
+ * Prior to calling the panic handlers, panic() calls smp_send_stop(). If
+ * that went well, there's only one CPU running, but this is no guarantee.
+ */
+static int drm_panic_handler(struct notifier_block *this, unsigned long ev,
+			     void *ptr)
+{
+	drm_panic_log("%s\n", __func__);
+
+	/*
+	 * TODO
+	 * Maybe we need better protection here against reentrance in case
+	 * panic_vmap() triggered a new panic.
+	 */
+
+	/* Nested panic */
+	if (drm_panic_fbs_num)
+		return NOTIFY_DONE;
+
+	class_for_each_device(drm_class, NULL, NULL, drm_panic_class_iter);
+
+	if (drm_panic_fbs_num)
+		drm_panic_active = true;
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block drm_panic_block = {
+	.notifier_call = drm_panic_handler,
+};
+
+static void drm_panic_test(void)
+{
+	/* simulate calling panic_notifier_list */
+	drm_panic_handler(NULL, 0, NULL);
+}
+
+static void drm_panic_test_cleanup(void)
+{
+	struct drm_panic_fb *pfb;
+	unsigned int i;
+
+	drm_panic_active = false;
+
+	for (i = 0; i < drm_panic_fbs_num; i++) {
+		pfb = &drm_panic_fbs[i];
+		if (pfb->fb->funcs->panic_vunmap)
+			pfb->fb->funcs->panic_vunmap(pfb->fb, pfb->vmap);
+		ww_mutex_unlock(&pfb->plane->mutex.mutex);
+		ww_mutex_unlock(&pfb->crtc->mutex.mutex);
+	}
+
+	drm_panic_fbs_num = 0;
+	memset(drm_panic_fbs, 0, DRM_PANIC_MAX_FBS * sizeof(*drm_panic_fbs));
+}
+
+/*
+ * Partial replication of panic() for testing purposes. Some symbols are
+ * only available when builtin (they're not exported).
+ */
+static void drm_panic_fake_panic(unsigned int level)
+{
+#ifndef MODULE
+	int old_loglevel = console_loglevel;
+
+	if (level > 1)
+		local_irq_disable();
+
+	console_verbose();
+
+	if (level > 2)
+		bust_spinlocks(1);
+
+	pr_emerg("Kernel panic - not syncing: FAKING=%u, oops_in_progress=%d\n",
+		 level, oops_in_progress);
+
+	dump_stack();
+	drm_panic_test();
+
+	if (level > 2)
+		bust_spinlocks(0);
+
+	console_flush_on_panic();
+
+	pr_emerg("---[ end Kernel panic - not syncing: FAKING\n");
+
+	if (level > 1)
+		local_irq_enable();
+
+	console_loglevel = old_loglevel;
+
+#else /* MODULE */
+
+	if (level > 1)
+		local_irq_disable();
+
+	pr_emerg("Kernel panic - not syncing: FAKING=%u\n", level);
+	dump_stack();
+	drm_panic_test();
+	pr_emerg("---[ end Kernel panic - not syncing: FAKING\n");
+
+	if (level > 1)
+		local_irq_enable();
+
+#endif /* MODULE */
+
+	drm_panic_test_cleanup();
+}
+
+static void drm_panic_clear_kmsgs(void)
+{
+	console_lock();
+	memset(drm_panic_kmsgs, 0, DRM_PANIC_MAX_KMSGS);
+	drm_panic_kmsgs_pos = 0;
+	console_unlock();
+}
+
+/*
+ * Fake/simulate panic() at different levels:
+ * 1: only trigger panic handling internally
+ * 2: add local_irq_disable()
+ * 3: add bust_spinlocks();
+ *
+ * Test rendering code:
+ * 100: clear kmsgs buffer
+ * 101: call panic handler for testing
+ * 102: cleanup after testing
+ *
+ * The real deal:
+ * 200: don't fake it, do call panic()
+ */
+static ssize_t drm_panic_file_panic_write(struct file *file,
+					  const char __user *user_buf,
+					  size_t count, loff_t *ppos)
+{
+	unsigned long long val;
+	ssize_t ret = 0;
+	char buf[24];
+	size_t size;
+
+	size = min(sizeof(buf) - 1, count);
+	if (copy_from_user(buf, user_buf, size))
+		return -EFAULT;
+
+	buf[size] = '\0';
+	ret = kstrtoull(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val && val < 4)
+		drm_panic_fake_panic(val);
+	else if (val == 100)
+		drm_panic_clear_kmsgs();
+	else if (val == 101)
+		drm_panic_test();
+	else if (val == 102)
+		drm_panic_test_cleanup();
+	else if (val == 200)
+		panic("TESTING");
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static const struct file_operations drm_panic_panic_ops = {
+	.write =        drm_panic_file_panic_write,
+	.open =         simple_open,
+	.llseek =       default_llseek,
+};
+
+static int drm_panic_log_show(struct seq_file *m, void *v)
+{
+	size_t pos = log_pos;
+
+	if (log_buf[0] == '\0')
+		return 0;
+
+	if (!pos) {
+		seq_write(m, log_buf, DRM_PANIC_LOG_SIZE);
+	} else if (log_buf[pos] == '\0') {
+		seq_write(m, log_buf, pos);
+	} else {
+		seq_write(m, log_buf + pos, DRM_PANIC_LOG_SIZE - pos);
+		seq_write(m, log_buf, pos);
+	}
+
+	return 0;
+}
+
+static int drm_panic_log_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, drm_panic_log_show, NULL);
+}
+
+static const struct file_operations drm_panic_log_ops = {
+	.owner   = THIS_MODULE,
+	.open    = drm_panic_log_open,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = single_release,
+};
+
+static struct dentry *drm_panic_d_panic;
+static struct dentry *drm_panic_d_log;
+
+void __init drm_panic_init(struct dentry *debugfs_root)
+{
+	drm_panic_kmsgs = kzalloc(DRM_PANIC_MAX_KMSGS, GFP_KERNEL);
+	if (!drm_panic_kmsgs) {
+		DRM_ERROR("Failed to setup panic handler\n");
+		return;
+	}
+
+	drm_panic_d_panic = debugfs_create_file("panic-test", 0200,
+						debugfs_root, NULL,
+						&drm_panic_panic_ops);
+
+	drm_panic_d_log = debugfs_create_file("panic-log", 0444, debugfs_root,
+					      NULL, &drm_panic_log_ops);
+	if (!IS_ERR_OR_NULL(drm_panic_d_log))
+		log_buf = kzalloc(DRM_PANIC_LOG_SIZE, GFP_KERNEL);
+
+	register_console(&drm_panic_console);
+	atomic_notifier_chain_register(&panic_notifier_list, &drm_panic_block);
+}
+
+void __exit drm_panic_exit(struct dentry *debugfs_root)
+{
+	if (!drm_panic_kmsgs)
+		return;
+
+	debugfs_remove(drm_panic_d_log);
+	debugfs_remove(drm_panic_d_panic);
+	kfree(log_buf);
+	log_buf = NULL;
+
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &drm_panic_block);
+	unregister_console(&drm_panic_console);
+	kfree(drm_panic_kmsgs);
+}
diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
index 50deb40..33f4022 100644
--- a/include/drm/drm_framebuffer.h
+++ b/include/drm/drm_framebuffer.h
@@ -90,6 +90,44 @@ struct drm_framebuffer_funcs {
 		     struct drm_file *file_priv, unsigned flags,
 		     unsigned color, struct drm_clip_rect *clips,
 		     unsigned num_clips);
+
+	/**
+	 * @panic_vmap:
+	 *
+	 * Optional callback for panic handling.
+	 *
+	 * For vmapping the selected framebuffer in a panic context. Must
+	 * be super careful about locking (only trylocking allowed).
+	 *
+	 * RETURNS:
+	 *
+	 * NULL if it didn't work out, otherwise an opaque cookie which is
+	 * passed to @panic_draw_xy. It can be anything: vmap area, structure
+	 * with more details, just a few flags, ...
+	 */
+	void *(*panic_vmap)(struct drm_framebuffer *fb);
+
+	/**
+	 * @panic_vunmap:
+	 *
+	 * Optional callback for cleaning up after panic testing.
+	 *
+	 * Crtc and plane locks are released after this callback has run.
+	 * vmap is the cookie returned by @panic_vmap.
+	 */
+	void (*panic_vunmap)(struct drm_framebuffer *fb, void *vmap);
+
+	/**
+	 * @panic_draw_xy:
+	 *
+	 * Optional callback for drawing pixels during panic.
+	 *
+	 * For drawing pixels onto a framebuffer prepared with @panic_vmap.
+	 * vmap is the cookie returned by @panic_vmap.
+	 * If it's not set, drm_framebuffer_panic_draw_xy() is used.
+	 */
+	void (*panic_draw_xy)(struct drm_framebuffer *fb, void *vmap,
+			      int x, int y, bool foreground);
 };
 
 /**
@@ -214,6 +252,8 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 void drm_framebuffer_remove(struct drm_framebuffer *fb);
 void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
+void drm_framebuffer_panic_draw_xy(struct drm_framebuffer *fb, void *vmap,
+				   int x, int y, bool foreground);
 
 /**
  * drm_framebuffer_reference - incr the fb refcnt
-- 
2.8.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/fb_cma_helper: Add panic handling

[Noralf Trønnes]

This enables panic message output for fb cma helper framebuffers.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_fb_cma_helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 1fd6eac..2f1b012 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -126,9 +126,19 @@ int drm_fb_cma_create_handle(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_fb_cma_create_handle);
 
+static void *drm_fb_cma_panic_vmap(struct drm_framebuffer *fb)
+{
+	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
+	struct drm_gem_cma_object *cma_obj = fb_cma->obj[0];
+
+	/* A PRIME imported buffer will not have it's vaddr set. */
+	return cma_obj ? cma_obj->vaddr : NULL;
+}
+
 static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
 	.destroy	= drm_fb_cma_destroy,
 	.create_handle	= drm_fb_cma_create_handle,
+	.panic_vmap	= drm_fb_cma_panic_vmap,
 };
 
 static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,
-- 
2.8.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/simpledrm: Add panic handling

[Noralf Trønnes]

This enables panic message output support in simpledrm.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/simpledrm/simpledrm_kms.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/simpledrm/simpledrm_kms.c b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
index deb24cf..32fbf7c 100644
--- a/drivers/gpu/drm/simpledrm/simpledrm_kms.c
+++ b/drivers/gpu/drm/simpledrm/simpledrm_kms.c
@@ -151,10 +151,21 @@ pr_info("%s\n", __func__);
 	kfree(fb);
 }
 
+static void *sdrm_panic_vmap(struct drm_framebuffer *fb)
+{
+	struct sdrm_device *sdrm = fb->dev->dev_private;
+
+	fb->pixel_format = sdrm->hw->format;
+	fb->pitches[0] = sdrm->hw->stride;
+
+	return sdrm->hw->map;
+}
+
 static const struct drm_framebuffer_funcs sdrm_fb_ops = {
 	.create_handle		= sdrm_fb_create_handle,
 	.dirty			= sdrm_fb_dirty,
 	.destroy		= sdrm_fb_destroy,
+	.panic_vmap = sdrm_panic_vmap,
 };
 
 struct sdrm_fb *sdrm_fb_new(struct sdrm_bo *bo,
-- 
2.8.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Add support for panic message output

[Noralf Trønnes]

Den 11.09.2016 20:47, skrev Noralf Trønnes:
> This adds support for outputting kernel messages on panic().
> A circular buffer is used to collect kernel messages.
> On panic() the notifier function loops over each DRM device and
> it's crtc's to find suitable framebuffers. On the next
> console->write(), the messages in the circular buffer are rendered
> on each of the recorded framebuffers.
>
> Only atomic drivers are supported.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
[...]
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
[...]
> +static bool drm_panic_add_fb(struct drm_crtc *crtc, struct drm_plane *plane)
> +{
> +	unsigned int width = drm_rect_width(&plane->state->src) >> 16;
> +	unsigned int height = drm_rect_height(&plane->state->src) >> 16;

I just tried this on vc4 (which uses fb_cma_helper) and width/height is
zero. Additionally plane->state->visible is false.
Maybe something the driver should set?

If I work around that (use fb->width/height), then I get panic output on
vc4 also in addition to simpledrm.

Noralf.

> +	struct drm_framebuffer *fb = plane->fb;
> +	const struct font_desc *font;
> +	struct drm_panic_fb *pfb;
> +	void *vmap;
> +
> +	if (!width || !height || !fb || !fb->funcs || !fb->funcs->panic_vmap)
> +		return false;
> +
> +	/* only 8-bit wide fonts are supported */
> +	font = get_default_font(width, height, BIT(7), -1);
> +	if (!font) {
> +		drm_panic_log("Couldn't get font\n");
> +		return false;
> +	}
> +
> +	vmap = fb->funcs->panic_vmap(fb);
> +	if (!vmap) {
> +		drm_panic_log("panic_vmap() returned NULL\n");
> +		return false;
> +	}
> +
> +	pfb = &drm_panic_fbs[drm_panic_fbs_num++];
> +	pfb->plane = plane;
> +	pfb->crtc = crtc;
> +	pfb->fb = fb;
> +	pfb->width = width;
> +	pfb->height = height;
> +	pfb->vmap = vmap;
> +	pfb->font = font;
> +	pfb->cols = pfb->width / font->width;
> +	pfb->rows = pfb->height / font->height;
> +
> +	drm_panic_log("    [FB:%d] %ux%u->%ux%u, %s, format=0x%08x\n",
> +		      fb->base.id, pfb->width, pfb->height, pfb->cols,
> +		      pfb->rows, font->name, fb->pixel_format);
> +
> +	return true;
> +}
> +
> +static void drm_panic_add(struct drm_device *drm)
> +{
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +
> +	if (!drm || !drm->driver ||
> +	    !(drm->driver->driver_features & DRIVER_ATOMIC))
> +		return;
> +
> +	drm_panic_log("%s on minor %d\n", drm->driver->name,
> +		      drm->primary ? drm->primary->index : -1);
> +
> +	drm_for_each_crtc(crtc, drm) {
> +		drm_panic_log("  %s\n", crtc->name);
> +
> +		if (drm_panic_fbs_num >= DRM_PANIC_MAX_FBS)
> +			return;
> +
> +		if (!ww_mutex_trylock(&crtc->mutex.mutex))
> +			continue;
> +
> +		if (!crtc->enabled || !crtc->primary)
> +			goto crtc_unlock;
> +
> +		if (!crtc->state || !crtc->state->active)
> +			goto crtc_unlock;
> +
> +		plane = crtc->primary;
> +		if (!ww_mutex_trylock(&plane->mutex.mutex))
> +			goto crtc_unlock;
> +
> +		if (!plane->state || !plane->state->visible)
> +			goto plane_unlock;
> +
> +		if (drm_panic_add_fb(crtc, plane))
> +			continue;
> +
> +plane_unlock:
> +		ww_mutex_unlock(&plane->mutex.mutex);
> +crtc_unlock:
> +		ww_mutex_unlock(&crtc->mutex.mutex);
> +	}
> +}
> +
> +static int drm_panic_class_iter(struct device *dev, void *data)
> +{
> +	struct drm_minor *minor;
> +
> +	minor = dev_get_drvdata(dev);
> +
> +	if (minor && minor->type == DRM_MINOR_PRIMARY)
> +		drm_panic_add(minor->dev);
> +
> +	return 0;
> +}
> +
> +/*
> + * The panic() function makes sure that only one CPU is allowed to run it's
> + * code, but a new panic can be triggered during it's processing.
> + *
> + * Prior to calling the panic handlers, panic() calls smp_send_stop(). If
> + * that went well, there's only one CPU running, but this is no guarantee.
> + */
> +static int drm_panic_handler(struct notifier_block *this, unsigned long ev,
> +			     void *ptr)
> +{
> +	drm_panic_log("%s\n", __func__);
> +
> +	/*
> +	 * TODO
> +	 * Maybe we need better protection here against reentrance in case
> +	 * panic_vmap() triggered a new panic.
> +	 */
> +
> +	/* Nested panic */
> +	if (drm_panic_fbs_num)
> +		return NOTIFY_DONE;
> +
> +	class_for_each_device(drm_class, NULL, NULL, drm_panic_class_iter);
> +
> +	if (drm_panic_fbs_num)
> +		drm_panic_active = true;
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block drm_panic_block = {
> +	.notifier_call = drm_panic_handler,
> +};
> +
> +static void drm_panic_test(void)
> +{
> +	/* simulate calling panic_notifier_list */
> +	drm_panic_handler(NULL, 0, NULL);
> +}
> +
> +static void drm_panic_test_cleanup(void)
> +{
> +	struct drm_panic_fb *pfb;
> +	unsigned int i;
> +
> +	drm_panic_active = false;
> +
> +	for (i = 0; i < drm_panic_fbs_num; i++) {
> +		pfb = &drm_panic_fbs[i];
> +		if (pfb->fb->funcs->panic_vunmap)
> +			pfb->fb->funcs->panic_vunmap(pfb->fb, pfb->vmap);
> +		ww_mutex_unlock(&pfb->plane->mutex.mutex);
> +		ww_mutex_unlock(&pfb->crtc->mutex.mutex);
> +	}
> +
> +	drm_panic_fbs_num = 0;
> +	memset(drm_panic_fbs, 0, DRM_PANIC_MAX_FBS * sizeof(*drm_panic_fbs));
> +}
> +
> +/*
> + * Partial replication of panic() for testing purposes. Some symbols are
> + * only available when builtin (they're not exported).
> + */
> +static void drm_panic_fake_panic(unsigned int level)
> +{
> +#ifndef MODULE
> +	int old_loglevel = console_loglevel;
> +
> +	if (level > 1)
> +		local_irq_disable();
> +
> +	console_verbose();
> +
> +	if (level > 2)
> +		bust_spinlocks(1);
> +
> +	pr_emerg("Kernel panic - not syncing: FAKING=%u, oops_in_progress=%d\n",
> +		 level, oops_in_progress);
> +
> +	dump_stack();
> +	drm_panic_test();
> +
> +	if (level > 2)
> +		bust_spinlocks(0);
> +
> +	console_flush_on_panic();
> +
> +	pr_emerg("---[ end Kernel panic - not syncing: FAKING\n");
> +
> +	if (level > 1)
> +		local_irq_enable();
> +
> +	console_loglevel = old_loglevel;
> +
> +#else /* MODULE */
> +
> +	if (level > 1)
> +		local_irq_disable();
> +
> +	pr_emerg("Kernel panic - not syncing: FAKING=%u\n", level);
> +	dump_stack();
> +	drm_panic_test();
> +	pr_emerg("---[ end Kernel panic - not syncing: FAKING\n");
> +
> +	if (level > 1)
> +		local_irq_enable();
> +
> +#endif /* MODULE */
> +
> +	drm_panic_test_cleanup();
> +}
> +
> +static void drm_panic_clear_kmsgs(void)
> +{
> +	console_lock();
> +	memset(drm_panic_kmsgs, 0, DRM_PANIC_MAX_KMSGS);
> +	drm_panic_kmsgs_pos = 0;
> +	console_unlock();
> +}
> +
> +/*
> + * Fake/simulate panic() at different levels:
> + * 1: only trigger panic handling internally
> + * 2: add local_irq_disable()
> + * 3: add bust_spinlocks();
> + *
> + * Test rendering code:
> + * 100: clear kmsgs buffer
> + * 101: call panic handler for testing
> + * 102: cleanup after testing
> + *
> + * The real deal:
> + * 200: don't fake it, do call panic()
> + */
> +static ssize_t drm_panic_file_panic_write(struct file *file,
> +					  const char __user *user_buf,
> +					  size_t count, loff_t *ppos)
> +{
> +	unsigned long long val;
> +	ssize_t ret = 0;
> +	char buf[24];
> +	size_t size;
> +
> +	size = min(sizeof(buf) - 1, count);
> +	if (copy_from_user(buf, user_buf, size))
> +		return -EFAULT;
> +
> +	buf[size] = '\0';
> +	ret = kstrtoull(buf, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val && val < 4)
> +		drm_panic_fake_panic(val);
> +	else if (val == 100)
> +		drm_panic_clear_kmsgs();
> +	else if (val == 101)
> +		drm_panic_test();
> +	else if (val == 102)
> +		drm_panic_test_cleanup();
> +	else if (val == 200)
> +		panic("TESTING");
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +static const struct file_operations drm_panic_panic_ops = {
> +	.write =        drm_panic_file_panic_write,
> +	.open =         simple_open,
> +	.llseek =       default_llseek,
> +};
> +
> +static int drm_panic_log_show(struct seq_file *m, void *v)
> +{
> +	size_t pos = log_pos;
> +
> +	if (log_buf[0] == '\0')
> +		return 0;
> +
> +	if (!pos) {
> +		seq_write(m, log_buf, DRM_PANIC_LOG_SIZE);
> +	} else if (log_buf[pos] == '\0') {
> +		seq_write(m, log_buf, pos);
> +	} else {
> +		seq_write(m, log_buf + pos, DRM_PANIC_LOG_SIZE - pos);
> +		seq_write(m, log_buf, pos);
> +	}
> +
> +	return 0;
> +}
> +
> +static int drm_panic_log_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, drm_panic_log_show, NULL);
> +}
> +
> +static const struct file_operations drm_panic_log_ops = {
> +	.owner   = THIS_MODULE,
> +	.open    = drm_panic_log_open,
> +	.read    = seq_read,
> +	.llseek  = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static struct dentry *drm_panic_d_panic;
> +static struct dentry *drm_panic_d_log;
> +
> +void __init drm_panic_init(struct dentry *debugfs_root)
> +{
> +	drm_panic_kmsgs = kzalloc(DRM_PANIC_MAX_KMSGS, GFP_KERNEL);
> +	if (!drm_panic_kmsgs) {
> +		DRM_ERROR("Failed to setup panic handler\n");
> +		return;
> +	}
> +
> +	drm_panic_d_panic = debugfs_create_file("panic-test", 0200,
> +						debugfs_root, NULL,
> +						&drm_panic_panic_ops);
> +
> +	drm_panic_d_log = debugfs_create_file("panic-log", 0444, debugfs_root,
> +					      NULL, &drm_panic_log_ops);
> +	if (!IS_ERR_OR_NULL(drm_panic_d_log))
> +		log_buf = kzalloc(DRM_PANIC_LOG_SIZE, GFP_KERNEL);
> +
> +	register_console(&drm_panic_console);
> +	atomic_notifier_chain_register(&panic_notifier_list, &drm_panic_block);
> +}
> +
> +void __exit drm_panic_exit(struct dentry *debugfs_root)
> +{
> +	if (!drm_panic_kmsgs)
> +		return;
> +
> +	debugfs_remove(drm_panic_d_log);
> +	debugfs_remove(drm_panic_d_panic);
> +	kfree(log_buf);
> +	log_buf = NULL;
> +
> +	atomic_notifier_chain_unregister(&panic_notifier_list,
> +					 &drm_panic_block);
> +	unregister_console(&drm_panic_console);
> +	kfree(drm_panic_kmsgs);
> +}
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index 50deb40..33f4022 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -90,6 +90,44 @@ struct drm_framebuffer_funcs {
>   		     struct drm_file *file_priv, unsigned flags,
>   		     unsigned color, struct drm_clip_rect *clips,
>   		     unsigned num_clips);
> +
> +	/**
> +	 * @panic_vmap:
> +	 *
> +	 * Optional callback for panic handling.
> +	 *
> +	 * For vmapping the selected framebuffer in a panic context. Must
> +	 * be super careful about locking (only trylocking allowed).
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * NULL if it didn't work out, otherwise an opaque cookie which is
> +	 * passed to @panic_draw_xy. It can be anything: vmap area, structure
> +	 * with more details, just a few flags, ...
> +	 */
> +	void *(*panic_vmap)(struct drm_framebuffer *fb);
> +
> +	/**
> +	 * @panic_vunmap:
> +	 *
> +	 * Optional callback for cleaning up after panic testing.
> +	 *
> +	 * Crtc and plane locks are released after this callback has run.
> +	 * vmap is the cookie returned by @panic_vmap.
> +	 */
> +	void (*panic_vunmap)(struct drm_framebuffer *fb, void *vmap);
> +
> +	/**
> +	 * @panic_draw_xy:
> +	 *
> +	 * Optional callback for drawing pixels during panic.
> +	 *
> +	 * For drawing pixels onto a framebuffer prepared with @panic_vmap.
> +	 * vmap is the cookie returned by @panic_vmap.
> +	 * If it's not set, drm_framebuffer_panic_draw_xy() is used.
> +	 */
> +	void (*panic_draw_xy)(struct drm_framebuffer *fb, void *vmap,
> +			      int x, int y, bool foreground);
>   };
>   
>   /**
> @@ -214,6 +252,8 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>   void drm_framebuffer_remove(struct drm_framebuffer *fb);
>   void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>   void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> +void drm_framebuffer_panic_draw_xy(struct drm_framebuffer *fb, void *vmap,
> +				   int x, int y, bool foreground);
>   
>   /**
>    * drm_framebuffer_reference - incr the fb refcnt

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Add panic handling

[Noralf Trønnes]

Ping.

Den 11.09.2016 20:47, skrev Noralf Trønnes:
> This patchset adds a way for DRM drivers to display kernel messages
> during panic().
>
> This version have lots of changes based on Daniel's review of the RFC
> where he proposed a different way of interacting with the driver.
>
> I have used this script to run some tests on the rendering code:
> https://gist.github.com/notro/4f42ec20e1be1d47c98fa749ca53c805
>
>
> Noralf.
>
>
> Noralf Trønnes (3):
>    drm: Add support for panic message output
>    drm/fb_cma_helper: Add panic handling
>    drm/simpledrm: Add panic handling
>
>   drivers/gpu/drm/Kconfig                   |   1 +
>   drivers/gpu/drm/Makefile                  |   2 +-
>   drivers/gpu/drm/drm_drv.c                 |   3 +
>   drivers/gpu/drm/drm_fb_cma_helper.c       |  10 +
>   drivers/gpu/drm/drm_framebuffer.c         | 108 +++++
>   drivers/gpu/drm/drm_internal.h            |   4 +
>   drivers/gpu/drm/drm_panic.c               | 640 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/simpledrm/simpledrm_kms.c |  11 +
>   include/drm/drm_framebuffer.h             |  40 ++
>   9 files changed, 818 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/drm_panic.c
>
> --
> 2.8.2
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] drm: Add panic handling

[Daniel Vetter]

Needs review from David Herrmann, pls keep him on cc for anything
related to panic handling.
-Daniel

On Sun, Oct 2, 2016 at 3:47 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> Ping.
>
>
> Den 11.09.2016 20:47, skrev Noralf Trønnes:
>>
>> This patchset adds a way for DRM drivers to display kernel messages
>> during panic().
>>
>> This version have lots of changes based on Daniel's review of the RFC
>> where he proposed a different way of interacting with the driver.
>>
>> I have used this script to run some tests on the rendering code:
>> https://gist.github.com/notro/4f42ec20e1be1d47c98fa749ca53c805
>>
>>
>> Noralf.
>>
>>
>> Noralf Trønnes (3):
>>    drm: Add support for panic message output
>>    drm/fb_cma_helper: Add panic handling
>>    drm/simpledrm: Add panic handling
>>
>>   drivers/gpu/drm/Kconfig                   |   1 +
>>   drivers/gpu/drm/Makefile                  |   2 +-
>>   drivers/gpu/drm/drm_drv.c                 |   3 +
>>   drivers/gpu/drm/drm_fb_cma_helper.c       |  10 +
>>   drivers/gpu/drm/drm_framebuffer.c         | 108 +++++
>>   drivers/gpu/drm/drm_internal.h            |   4 +
>>   drivers/gpu/drm/drm_panic.c               | 640
>> ++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/simpledrm/simpledrm_kms.c |  11 +
>>   include/drm/drm_framebuffer.h             |  40 ++
>>   9 files changed, 818 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/drm_panic.c
>>
>> --
>> 2.8.2
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling

[Laurent Pinchart]

Hi Noralf,

Thank you for the patch.

On Sunday 11 Sep 2016 20:47:41 Noralf Trønnes wrote:
> This enables panic message output for fb cma helper framebuffers.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_fb_cma_helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac..2f1b012 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -126,9 +126,19 @@ int drm_fb_cma_create_handle(struct drm_framebuffer
> *fb, }
>  EXPORT_SYMBOL(drm_fb_cma_create_handle);
> 
> +static void *drm_fb_cma_panic_vmap(struct drm_framebuffer *fb)
> +{
> +	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> +	struct drm_gem_cma_object *cma_obj = fb_cma->obj[0];
> +
> +	/* A PRIME imported buffer will not have it's vaddr set. */

Does this mean that, if the framebuffer that happens to be displayed when a 
panic occurs is imported, no message will be printed ? I understand how 
supporting such cases is difficult, but I'm wondering how we could proceed to 
ensure that a panic can be displayed in most (if not all) cases.

Similarly, it looks like your code only handles single-planar formats, but 
there's no explicit check for that in patch 1/3. The easiest fix is to reject 
multi-planar framebuffers, but that would again result in silent panics in 
some cases.

> +	return cma_obj ? cma_obj->vaddr : NULL;

Can cma_obj be NULL here ? I thought that framebuffer objects were always 
created with at least one GEM object.

> +}
> +
>  static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
>  	.destroy	= drm_fb_cma_destroy,
>  	.create_handle	= drm_fb_cma_create_handle,
> +	.panic_vmap	= drm_fb_cma_panic_vmap,
>  };
> 
>  static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling

[Noralf Trønnes]

Den 05.10.2016 15:22, skrev Laurent Pinchart:
> Hi Noralf,
>
> Thank you for the patch.
>
> On Sunday 11 Sep 2016 20:47:41 Noralf Trønnes wrote:
>> This enables panic message output for fb cma helper framebuffers.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/drm_fb_cma_helper.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac..2f1b012 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -126,9 +126,19 @@ int drm_fb_cma_create_handle(struct drm_framebuffer
>> *fb, }
>>   EXPORT_SYMBOL(drm_fb_cma_create_handle);
>>
>> +static void *drm_fb_cma_panic_vmap(struct drm_framebuffer *fb)
>> +{
>> +	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
>> +	struct drm_gem_cma_object *cma_obj = fb_cma->obj[0];
>> +
>> +	/* A PRIME imported buffer will not have it's vaddr set. */
> Does this mean that, if the framebuffer that happens to be displayed when a
> panic occurs is imported, no message will be printed ? I understand how
> supporting such cases is difficult, but I'm wondering how we could proceed to
> ensure that a panic can be displayed in most (if not all) cases.

If we can vmap all cma buffers, then it's simple:
- Add dma_buf_vmap() call to drm_gem_cma_prime_import_sg_table()
- Add dma_buf_vunmap() call to drm_gem_cma_free_object()

If not then it looks more complicated, since this is atomic context.
There is dma_buf_kmap_atomic(), but there are no users.
And drm_gem_prime_dmabuf_ops doesn't support .kmap_atomic either
(returns NULL).

omapdrm is the only dma_buf provider I can find that actually uses
kmap_atomic() instead of just returning NULL or relying on an existing
virtual address. It has it's own .gem_prime_import/export functions to
accomplish this.

> Similarly, it looks like your code only handles single-planar formats, but
> there's no explicit check for that in patch 1/3. The easiest fix is to reject
> multi-planar framebuffers, but that would again result in silent panics in
> some cases.

That's correct if we talk about the default panic_draw_xy() function:
drm_framebuffer_panic_draw_xy(). Most likely this function can be extended
to support multi-planar formats, but Daniel said we could wait with that.
And the driver can also implement it's own .panic_draw_xy() function.

>> +	return cma_obj ? cma_obj->vaddr : NULL;
> Can cma_obj be NULL here ? I thought that framebuffer objects were always
> created with at least one GEM object.

I was trying to be very careful since a panic is about something gone
very wrong. But in that case I should have checked that fb_cma isn't NULL
also before dereferencing it.
Maybe I'm over the top paranoid :-)

Noralf.

>> +}
>> +
>>   static struct drm_framebuffer_funcs drm_fb_cma_funcs = {
>>   	.destroy	= drm_fb_cma_destroy,
>>   	.create_handle	= drm_fb_cma_create_handle,
>> +	.panic_vmap	= drm_fb_cma_panic_vmap,
>>   };
>>
>>   static struct drm_fb_cma *drm_fb_cma_alloc(struct drm_device *dev,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling

[Daniel Vetter]

On Wed, Oct 05, 2016 at 09:36:17PM +0200, Noralf Trønnes wrote:
> 
> Den 05.10.2016 15:22, skrev Laurent Pinchart:
> > Hi Noralf,
> > 
> > Thank you for the patch.
> > 
> > On Sunday 11 Sep 2016 20:47:41 Noralf Trønnes wrote:
> > > This enables panic message output for fb cma helper framebuffers.
> > > 
> > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > ---
> > >   drivers/gpu/drm/drm_fb_cma_helper.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac..2f1b012 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > @@ -126,9 +126,19 @@ int drm_fb_cma_create_handle(struct drm_framebuffer
> > > *fb, }
> > >   EXPORT_SYMBOL(drm_fb_cma_create_handle);
> > > 
> > > +static void *drm_fb_cma_panic_vmap(struct drm_framebuffer *fb)
> > > +{
> > > +	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> > > +	struct drm_gem_cma_object *cma_obj = fb_cma->obj[0];
> > > +
> > > +	/* A PRIME imported buffer will not have it's vaddr set. */
> > Does this mean that, if the framebuffer that happens to be displayed when a
> > panic occurs is imported, no message will be printed ? I understand how
> > supporting such cases is difficult, but I'm wondering how we could proceed to
> > ensure that a panic can be displayed in most (if not all) cases.
> 
> If we can vmap all cma buffers, then it's simple:
> - Add dma_buf_vmap() call to drm_gem_cma_prime_import_sg_table()
> - Add dma_buf_vunmap() call to drm_gem_cma_free_object()
> 
> If not then it looks more complicated, since this is atomic context.
> There is dma_buf_kmap_atomic(), but there are no users.
> And drm_gem_prime_dmabuf_ops doesn't support .kmap_atomic either
> (returns NULL).
> 
> omapdrm is the only dma_buf provider I can find that actually uses
> kmap_atomic() instead of just returning NULL or relying on an existing
> virtual address. It has it's own .gem_prime_import/export functions to
> accomplish this.

dma_buf's kmap_atomic is a bit ill-defined, since atm it's not clear how
to get the buffer pinned in the first place. Originally the plan was to
use begin/end_cpu_access for this (which again can block, so not suitable
for panic context). But that was kinda reused as the coherence control
interface, and doesn't really pin anything. I guess we should probably
just nuke the kmap interface part of dma_buf, the idea was to use that for
ttm, but that never really happened.

> > Similarly, it looks like your code only handles single-planar formats, but
> > there's no explicit check for that in patch 1/3. The easiest fix is to reject
> > multi-planar framebuffers, but that would again result in silent panics in
> > some cases.
> 
> That's correct if we talk about the default panic_draw_xy() function:
> drm_framebuffer_panic_draw_xy(). Most likely this function can be extended
> to support multi-planar formats, but Daniel said we could wait with that.
> And the driver can also implement it's own .panic_draw_xy() function.
> 
> > > +	return cma_obj ? cma_obj->vaddr : NULL;
> > Can cma_obj be NULL here ? I thought that framebuffer objects were always
> > created with at least one GEM object.
> 
> I was trying to be very careful since a panic is about something gone
> very wrong. But in that case I should have checked that fb_cma isn't NULL
> also before dereferencing it.
> Maybe I'm over the top paranoid :-)

Yeah I think this is a bit too much paranoia. I think you can remove these
NULL checks here, it's truly impossible. Where we really need to be
careful is with locking, both to make sure we exclusively use trylocks for
everything, and that we do grab all the locks (to avoid disaster when the
part that panicked is kms itself).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling

[Laurent Pinchart]

Hi Daniel,

On Thursday 06 Oct 2016 11:12:40 Daniel Vetter wrote:
> On Wed, Oct 05, 2016 at 09:36:17PM +0200, Noralf Trønnes wrote:
> > Den 05.10.2016 15:22, skrev Laurent Pinchart:
> > > On Sunday 11 Sep 2016 20:47:41 Noralf Trønnes wrote:
> > > > This enables panic message output for fb cma helper framebuffers.
> > > > 
> > > > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > > ---
> > > > 
> > > >   drivers/gpu/drm/drm_fb_cma_helper.c | 10 ++++++++++
> > > >   1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > b/drivers/gpu/drm/drm_fb_cma_helper.c index 1fd6eac..2f1b012 100644
> > > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > > @@ -126,9 +126,19 @@ int drm_fb_cma_create_handle(struct
> > > > drm_framebuffer
> > > > *fb, }
> > > > 
> > > >   EXPORT_SYMBOL(drm_fb_cma_create_handle);
> > > > 
> > > > +static void *drm_fb_cma_panic_vmap(struct drm_framebuffer *fb)
> > > > +{
> > > > +	struct drm_fb_cma *fb_cma = to_fb_cma(fb);
> > > > +	struct drm_gem_cma_object *cma_obj = fb_cma->obj[0];
> > > > +
> > > > +	/* A PRIME imported buffer will not have it's vaddr set. */
> > > 
> > > Does this mean that, if the framebuffer that happens to be displayed
> > > when a panic occurs is imported, no message will be printed ? I
> > > understand how supporting such cases is difficult, but I'm wondering how
> > > we could proceed to ensure that a panic can be displayed in most (if not
> > > all) cases.
> > 
> > If we can vmap all cma buffers, then it's simple:
> > - Add dma_buf_vmap() call to drm_gem_cma_prime_import_sg_table()
> > - Add dma_buf_vunmap() call to drm_gem_cma_free_object()
> > 
> > If not then it looks more complicated, since this is atomic context.
> > There is dma_buf_kmap_atomic(), but there are no users.
> > And drm_gem_prime_dmabuf_ops doesn't support .kmap_atomic either
> > (returns NULL).
> > 
> > omapdrm is the only dma_buf provider I can find that actually uses
> > kmap_atomic() instead of just returning NULL or relying on an existing
> > virtual address. It has it's own .gem_prime_import/export functions to
> > accomplish this.
> 
> dma_buf's kmap_atomic is a bit ill-defined, since atm it's not clear how
> to get the buffer pinned in the first place. Originally the plan was to
> use begin/end_cpu_access for this (which again can block, so not suitable
> for panic context). But that was kinda reused as the coherence control
> interface, and doesn't really pin anything. I guess we should probably
> just nuke the kmap interface part of dma_buf, the idea was to use that for
> ttm, but that never really happened.

I agree with you, and I'd go one step further: we need to define very 
precisely the semantics of the dmabuf operations. Drivers cache various 
information in different ways, making interoperability very fragile.

> > > Similarly, it looks like your code only handles single-planar formats,
> > > but there's no explicit check for that in patch 1/3. The easiest fix is
> > > to reject multi-planar framebuffers, but that would again result in
> > > silent panics in some cases.
> > 
> > That's correct if we talk about the default panic_draw_xy() function:
> > drm_framebuffer_panic_draw_xy(). Most likely this function can be extended
> > to support multi-planar formats, but Daniel said we could wait with that.
> > And the driver can also implement it's own .panic_draw_xy() function.
> > 
> > > > +	return cma_obj ? cma_obj->vaddr : NULL;
> > > 
> > > Can cma_obj be NULL here ? I thought that framebuffer objects were
> > > always created with at least one GEM object.
> > 
> > I was trying to be very careful since a panic is about something gone
> > very wrong. But in that case I should have checked that fb_cma isn't NULL
> > also before dereferencing it.
> > Maybe I'm over the top paranoid :-)
> 
> Yeah I think this is a bit too much paranoia. I think you can remove these
> NULL checks here, it's truly impossible. Where we really need to be
> careful is with locking, both to make sure we exclusively use trylocks for
> everything, and that we do grab all the locks (to avoid disaster when the
> part that panicked is kms itself).

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel