On Mon, Mar 11, 2019 at 06:42:16PM +0100, Noralf Trønnes wrote:

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

Bunch of comments/ideas for you or Darwish below, whoever picks this up. -Daniel

---

drivers/gpu/drm/Kconfig | 3 + drivers/gpu/drm/drm_drv.c | 3 + drivers/gpu/drm/drm_framebuffer.c | 117 ++++++++++ drivers/gpu/drm/drm_internal.h | 4 + drivers/gpu/drm/drm_panic.c | 363 ++++++++++++++++++++++++++++++ include/drm/drm_framebuffer.h | 41 ++++ 6 files changed, 531 insertions(+) create mode 100644 drivers/gpu/drm/drm_panic.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index bd943a71756c..74c37542f857 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -14,6 +14,9 @@ menuconfig DRM select I2C_ALGOBIT select DMA_SHARED_BUFFER select SYNC_FILE

• select FONT_SUPPORT
• select FONT_8x8
• select FONT_8x16 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/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 50d849d1bc6e..b0b870b5dd55 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -1147,6 +1147,7 @@ static const struct file_operations drm_stub_fops = {

static void drm_core_exit(void) {

• drm_panic_exit(drm_debugfs_root); unregister_chrdev(DRM_MAJOR, "drm"); debugfs_remove(drm_debugfs_root); drm_sysfs_destroy();

@@ -1178,6 +1179,8 @@ static int __init drm_core_init(void) if (ret < 0) goto error;

• drm_panic_init(drm_debugfs_root);

• drm_core_init_complete = true;

  DRM_DEBUG("Initialized\n");

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index d8d75e25f6fb..da2285c5ae90 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -1087,3 +1087,120 @@ int drm_framebuffer_debugfs_init(struct drm_minor *minor) minor->debugfs_root, minor); } #endif

+/**

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

+{

• void *pix = vmap + fb->offsets[0] + (y * fb->pitches[0]);
• u8 *pix8 = pix;
• u16 *pix16 = pix;
• u32 *pix32 = pix;
• switch (fb->format->format & ~DRM_FORMAT_BIG_ENDIAN) {
• case DRM_FORMAT_C8:
• case DRM_FORMAT_RGB332:
• case DRM_FORMAT_BGR233:
• case DRM_FORMAT_NV12:
• case DRM_FORMAT_NV21:
• case DRM_FORMAT_NV16:
• case DRM_FORMAT_NV61:
• case DRM_FORMAT_NV24:
• case DRM_FORMAT_NV42:
• case DRM_FORMAT_YUV410:
• case DRM_FORMAT_YVU410:
• case DRM_FORMAT_YUV411:
• case DRM_FORMAT_YVU411:
• case DRM_FORMAT_YUV420:
• case DRM_FORMAT_YVU420:
• case DRM_FORMAT_YUV422:
• case DRM_FORMAT_YVU422:
• case DRM_FORMAT_YUV444:
• case DRM_FORMAT_YVU444:

• pix8[x] = foreground ? 0xff : 0x00;
  

• break;
  

• case DRM_FORMAT_XRGB4444:
• case DRM_FORMAT_ARGB4444:
• case DRM_FORMAT_XBGR4444:
• case DRM_FORMAT_ABGR4444:

• pix16[x] = foreground ? 0xffff : 0xf000;
  

• break;
  

• case DRM_FORMAT_RGBX4444:
• case DRM_FORMAT_RGBA4444:
• case DRM_FORMAT_BGRX4444:
• case DRM_FORMAT_BGRA4444:

• pix16[x] = foreground ? 0xffff : 0x000f;
  

• break;
  

• case DRM_FORMAT_XRGB1555:
• case DRM_FORMAT_ARGB1555:
• case DRM_FORMAT_XBGR1555:
• case DRM_FORMAT_ABGR1555:

• pix16[x] = foreground ? 0xffff : 0x8000;
  

• break;
  

• case DRM_FORMAT_RGBX5551:
• case DRM_FORMAT_RGBA5551:
• case DRM_FORMAT_BGRX5551:
• case DRM_FORMAT_BGRA5551:

• pix16[x] = foreground ? 0xffff : 0x0001;
  

• break;
  

• case DRM_FORMAT_RGB565:
• case DRM_FORMAT_BGR565:

• pix16[x] = foreground ? 0xffff : 0x0000;
  

• break;
  

• case DRM_FORMAT_RGB888:
• case DRM_FORMAT_BGR888:

• pix8[x * 3] = foreground ? 0xff : 0x00;
  

• pix8[(x * 3) + 1] = pix8[x];
  

• pix8[(x * 3) + 2] = pix8[x];
  

• break;
  

• case DRM_FORMAT_XRGB8888:
• case DRM_FORMAT_ARGB8888:
• case DRM_FORMAT_XBGR8888:
• case DRM_FORMAT_ABGR8888:

• pix32[x] = foreground ? 0xffffffff : 0xff000000;
  

• break;
  

• case DRM_FORMAT_RGBX8888:
• case DRM_FORMAT_RGBA8888:
• case DRM_FORMAT_BGRX8888:
• case DRM_FORMAT_BGRA8888:

• pix32[x] = foreground ? 0xffffffff : 0x000000ff;
  

• break;
  

• case DRM_FORMAT_XRGB2101010:
• case DRM_FORMAT_ARGB2101010:
• case DRM_FORMAT_XBGR2101010:
• case DRM_FORMAT_ABGR2101010:

• pix32[x] = foreground ? 0xffffffff : 0xc0000000;
  

• break;
  

• case DRM_FORMAT_RGBX1010102:
• case DRM_FORMAT_RGBA1010102:
• case DRM_FORMAT_BGRX1010102:
• case DRM_FORMAT_BGRA1010102:

• pix32[x] = foreground ? 0xffffffff : 0x00000003;
  

• 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 251d67e04c2d..694a5803ac3c 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -191,3 +191,7 @@ int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, const struct drm_framebuffer *fb); int drm_framebuffer_debugfs_init(struct drm_minor *minor);

+/* drm_panic.c */ +void drm_panic_init(struct dentry *debugfs_root); +void drm_panic_exit(struct dentry *debugfs_root); diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c new file mode 100644 index 000000000000..4bca7f0bc369 --- /dev/null +++ b/drivers/gpu/drm/drm_panic.c @@ -0,0 +1,363 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright 2018 Noralf Trønnes

+#include <linux/debugfs.h> +#include <linux/font.h> +#include <linux/kernel.h> +#include <linux/kmsg_dump.h> +#include <linux/seq_file.h> +#include <linux/slab.h> +#include <linux/uaccess.h>

+#include <drm/drm_crtc.h> +#include <drm/drm_device.h> +#include <drm/drm_drv.h> +#include <drm/drm_file.h> +#include <drm/drm_framebuffer.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_plane.h> +#include <drm/drm_print.h> +#include <drm/drmP.h>

+#include "drm_internal.h"

+/*

• • The log lines in an ARM stack dump are 92 characters long
• • and 120 is a nice multiple for HD and 4K.
• */

+#define DRM_PANIC_COLUMN_WIDTH 120

I think we should compute this dynamically from the actual fb width and font witdth.

+struct drm_panic_ctx {

• struct drm_framebuffer *fb;
• unsigned int width;
• unsigned int height;
• void *vmap;
• const struct font_desc *font;
• unsigned int col_width;
• unsigned int bottom_y;
• size_t max_line_length;
• unsigned int x;
• unsigned int y;

+};

+static const struct font_desc *drm_panic_font8x8, *drm_panic_font8x16;

+static void drm_panic_draw_xy(struct drm_framebuffer *fb, void *vmap,

• 	      int x, int y, bool fg)
  

+{

• if (fb->funcs->panic_draw_xy)

• fb->funcs->panic_draw_xy(fb, vmap, x, y, fg);
  

• else

• drm_framebuffer_panic_draw_xy(fb, vmap, x, y, fg);
  

+}

+static void drm_panic_render_char(struct drm_panic_ctx *ctx,

• 		  unsigned int offset, char c)
  

+{

• unsigned int h, w, x, y;
• u8 fontline;
• for (h = 0; h < ctx->font->height; h++) {

• fontline = *(u8 *)(ctx->font->data + c * ctx->font->height + h);
  

• for (w = 0; w < ctx->font->width; w++) {
  

• 	x = ctx->x + (offset * ctx->font->width) + w;
  

• 	y = ctx->y + h;
  

• 	drm_panic_draw_xy(ctx->fb, ctx->vmap, x, y,
  

• 			  fontline & BIT(7 - w));
  

• }
  

• }

+}

+static void drm_panic_render_line(struct drm_panic_ctx *ctx,

• 		  const char *line, size_t len)
  

+{

• unsigned int i;
• for (i = 0; i < len; i++)

• drm_panic_render_char(ctx, i, line[i]);
  

• /* Clear out the rest of the line */
• for (i = len; i < ctx->max_line_length; i++)

• drm_panic_render_char(ctx, i, ' ');
  

+}

+static bool drm_panic_newline(struct drm_panic_ctx *ctx) +{

• if (ctx->x == 0 && ctx->y == 0)

• return false;
  

• if (ctx->y == 0) {

• ctx->x -= ctx->col_width;
  

• ctx->y = ctx->bottom_y;
  

• } else {

• ctx->y -= ctx->font->height;
  

• }
• return true;

+}

+/* Render from bottom right most column */ +static bool drm_panic_render_lines(struct drm_panic_ctx *ctx,

• 		   const char *str, size_t len)
  

+{

• size_t l, line_length;
• const char *pos;
• int i;
• while (len) {

• pos = str + len - 1;
  

• if (*pos == '\n') {
  

• 	len--;
  

• 	if (!drm_panic_newline(ctx))
  

• 		return false;
  

• 	continue;
  

• }
  

• while (pos > str && *(pos - 1) != '\n')
  

• 	pos--;
  

• line_length = len - (pos - str);
  

• if (!line_length || len < line_length) {
  

• 	pr_err("%s: Bug! line_length=%zu len=%zu\n",
  

• 	       __func__, line_length, len);
  

• 	return false;
  

• }
  

• len -= line_length;
  

• for (i = DIV_ROUND_UP(line_length, ctx->max_line_length) - 1; i >= 0; i--) {
  

• 	l = min(ctx->max_line_length, line_length - i * ctx->max_line_length);
  

• 	drm_panic_render_line(ctx, pos + (i * ctx->max_line_length), l);
  

• 	if (i && !drm_panic_newline(ctx))
  

• 		return false;
  

• }
  

• }
• return true;

+}

+static void drm_panic_kmsg_render_screen(struct drm_plane *plane,

• 			 struct kmsg_dumper *dumper)
  

+{

• struct drm_framebuffer *fb = plane->state->fb;
• bool first_iteration = true;
• struct drm_panic_ctx ctx;
• static char text[1024];
• size_t len;
• ctx.vmap = fb->funcs->panic_vmap(fb);
• /* Print some info when testing */
• if (dumper->max_reason == KMSG_DUMP_OOPS) {

• struct drm_format_name_buf format_name;
  

• pr_info("%s: [FB:%d] %ux%u format=%s vmap=%p\n",
  

• 	__func__, fb->base.id, fb->width, fb->height,
  

• 	drm_get_format_name(fb->format->format, &format_name),
  

• 	ctx.vmap);
  

• }
• if (!ctx.vmap)

• return;
  

For some framebuffers it might be useful to not require vmap, but instead have some page-by-page kind of access helper. Since preemptively setting up a vmap for all the non-continous buffers is a bit much, and we really can't set up the vmap in the panic handler.

Maybe we should call this panic_prepare/panic_finish and s/vmap/cookie/, to make it entirely opaque?

But a bit overengineering perhaps :-)

• ctx.fb = fb;
• ctx.width = drm_rect_width(&plane->state->src) >> 16;
• ctx.height = drm_rect_height(&plane->state->src) >> 16;
• /*

• * TODO:
  

• * Find which part of the fb that is visible.
  

• * Width and height are zero on vc4
  

• */
  

• if (!ctx.width || !ctx.height) {

• ctx.width = fb->width;
  

• ctx.height = fb->height;
  

• }
• /* Try to fit 50 lines */
• if (ctx.height < 50 * 16 && drm_panic_font8x8)

• ctx.font = drm_panic_font8x8;
  

• else

• ctx.font = drm_panic_font8x16;
  

• ctx.col_width = DRM_PANIC_COLUMN_WIDTH * ctx.font->width;
• ctx.bottom_y = ((ctx.height / ctx.font->height) - 1) * ctx.font->height;
• if (ctx.width < 2 * ctx.col_width)

• ctx.max_line_length = ctx.width / ctx.font->width;
  

• else

• ctx.max_line_length = DRM_PANIC_COLUMN_WIDTH - 2; /* border=2 */
  

• ctx.x = 0;
• ctx.y = ctx.bottom_y;
• if (ctx.width > ctx.col_width)

• ctx.x = ((ctx.width / ctx.col_width) - 1) * ctx.col_width;
  

• pr_debug("%s: font=%s %ux%u max_line_length=%zu (%u,%u)\n",

•  __func__, ctx.font->name, ctx.width, ctx.height,
  

•  ctx.max_line_length, ctx.x, ctx.y);
  

• kmsg_dump_rewind(dumper);
• /* The latest messages are fetched first */
• while (kmsg_dump_get_buffer(dumper, false, text, sizeof(text), &len)) {

• /* Strip off the very last newline so we start at the bottom */
  

• if (first_iteration) {
  

• 	len--;
  

• 	first_iteration = false;
  

• }
  

• if (!drm_panic_render_lines(&ctx, text, len))
  

• 	break;
  

• }
• if (fb->funcs->panic_vunmap)

• fb->funcs->panic_vunmap(fb, vmap);
  

+}

+static void drm_panic_try_dev(struct drm_device *dev, struct kmsg_dumper *dumper) +{

• struct drm_framebuffer *fb;
• struct drm_plane *plane;
• struct drm_crtc *crtc;
• if (!drm_core_check_feature(dev, DRIVER_ATOMIC))

• return;
  

• if (dumper->max_reason == KMSG_DUMP_OOPS)

• pr_info("%s: %s on minor %d\n", __func__, dev->driver->name,
  

• 	dev->primary->index);
  

• drm_for_each_crtc(crtc, dev) {

• 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;
  

• /*
  

•  * TODO: Should we check plane_state->visible?
  

•  * It is not set on vc4
  

I think we should. We should also check whether that primary is connected to the crtc (some hw you can reassign planes freely between crtc, and the crtc->primary pointer is only used for compat with legacy ioctl).

• if (!plane->state || !plane->state->visible)
  

•  */
  

• if (!plane->state)
  

• 	goto plane_unlock;
  

• fb = plane->state->fb;
  

• if (!fb || !fb->funcs->panic_vmap)
  

Docs says this is optional, doesn't seem to be all that optional. I'd check for this or a driver-specific ->panic_draw_xy instead.

• 	goto plane_unlock;
  

• /*
  

•  * fbcon puts out panic messages so stay away to avoid jumbled
  

•  * output. If vc->vc_mode != KD_TEXT fbcon won't put out
  

•  * messages (see vt_console_print).
  

•  */
  

• if (dev->fb_helper && dev->fb_helper->fb == fb)
  

• 	goto plane_unlock;
  

• drm_panic_kmsg_render_screen(plane, dumper);
  

+plane_unlock:

• ww_mutex_unlock(&plane->mutex.mutex);
  

+crtc_unlock:

• ww_mutex_unlock(&crtc->mutex.mutex);
  

• }

+}

+static int drm_panic_dev_iter(struct device *dev, void *data) +{

• struct drm_minor *minor = dev_get_drvdata(dev);
• if (minor && minor->type == DRM_MINOR_PRIMARY)

• drm_panic_try_dev(minor->dev, data);
  

• return 0;

+}

+static void drm_panic_kmsg_dump(struct kmsg_dumper *dumper,

• 		enum kmsg_dump_reason reason)
  

+{

• class_for_each_device(drm_class, NULL, dumper, drm_panic_dev_iter);

class_for_each_device uses klist, which only uses an irqsave spinlock. I think that's good enough. Comment to that effect would be good e.g.

/* based on klist, which uses only a spin_lock_irqsave, which we * assume still works */

If we aim for perfect this should be a trylock still, maybe using our own device list.

+}

+static struct kmsg_dumper drm_panic_kmsg_dumper = {

• .dump = drm_panic_kmsg_dump,
• .max_reason = KMSG_DUMP_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;
• char buf[24];
• size_t size;
• ssize_t ret;
• 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;
  

• drm_panic_kmsg_dumper.max_reason = KMSG_DUMP_OOPS;
• wmb();
• /* Do a real test with: echo c > /proc/sysrq-trigger */
• if (val == 0) {

• pr_info("Test panic screen using kmsg_dump(OOPS)\n");
  

• kmsg_dump(KMSG_DUMP_OOPS);
  

• } else if (val == 1) {

• char *null_pointer = NULL;
  

• pr_info("Test panic screen using NULL pointer dereference\n");
  

• *null_pointer = 1;
  

• } else {

• return -EINVAL;
  

• }

This isn't quite what I had in mind, since it still kills the kernel (like sysrq-trigger). Instead what I had in mind is to recreate the worst possible panic context as much as feasible (disabling interrupts should be a good start, maybe we can even do an nmi callback), and then call our panic implementation. That way we can test the panic handler in a non-destructive way (i.e. aside from last dmesg lines printed to the screen nothing bad happens to the kernel: No real panic, no oops, no tainting).

And igt testcase could then exercise this, and CI run it.

• return count;

+}

+static const struct file_operations drm_panic_panic_ops = {

• .write = drm_panic_file_panic_write,
• .open = simple_open,
• .llseek = default_llseek,

+};

+static struct dentry *drm_panic_d_panic;

+void __init drm_panic_init(struct dentry *debugfs_root) +{

• drm_panic_font8x8 = find_font("VGA8x8");
• drm_panic_font8x16 = find_font("VGA8x16");
• if (!drm_panic_font8x16) {

• DRM_WARN("Couldn't find font, panic screen disabled\n");
  

• return;
  

• }
• drm_panic_d_panic = debugfs_create_file("panic-test", 0200,

• 				debugfs_root, NULL,
  

• 				&drm_panic_panic_ops);
  

• kmsg_dump_register(&drm_panic_kmsg_dumper);

+}

+void drm_panic_exit(struct dentry *debugfs_root) +{

• kmsg_dump_unregister(&drm_panic_kmsg_dumper);
• debugfs_remove(drm_panic_d_panic);

+} diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f0b34c977ec5..f3274798ecfe 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -94,6 +94,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);
  

};

/** @@ -293,4 +331,7 @@ int drm_framebuffer_plane_width(int width, int drm_framebuffer_plane_height(int height, const struct drm_framebuffer *fb, int plane);

+void drm_framebuffer_panic_draw_xy(struct drm_framebuffer *fb, void *vmap,

• 		   int x, int y, bool foreground);
  

#endif

2.20.1

On Tue, Mar 12, 2019 at 10:53:20AM +0100, Daniel Vetter wrote:

On Mon, Mar 11, 2019 at 11:40:36PM +0100, Noralf Trønnes wrote:

...

Den 11.03.2019 20.29, skrev Daniel Vetter:

...

On Mon, Mar 11, 2019 at 08:23:38PM +0100, Daniel Vetter wrote:

...

On Mon, Mar 11, 2019 at 06:42:16PM +0100, Noralf Trønnes wrote:

...

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

<snip>

...

...

...

+static void drm_panic_try_dev(struct drm_device *dev, struct kmsg_dumper *dumper) +{

• struct drm_framebuffer *fb;
• struct drm_plane *plane;
• struct drm_crtc *crtc;
• if (!drm_core_check_feature(dev, DRIVER_ATOMIC))

• return;
  

• if (dumper->max_reason == KMSG_DUMP_OOPS)

• pr_info("%s: %s on minor %d\n", __func__, dev->driver->name,
  

• 	dev->primary->index);
  

• drm_for_each_crtc(crtc, dev) {

• 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;
  

• /*
  

•  * TODO: Should we check plane_state->visible?
  

•  * It is not set on vc4
  

I think we should. We should also check whether that primary is connected to the crtc (some hw you can reassign planes freely between crtc, and the crtc->primary pointer is only used for compat with legacy ioctl).

...

• if (!plane->state || !plane->state->visible)
  

•  */
  

• if (!plane->state)
  

• 	goto plane_unlock;
  

• fb = plane->state->fb;
  

• if (!fb || !fb->funcs->panic_vmap)
  

Docs says this is optional, doesn't seem to be all that optional. I'd check for this or a driver-specific ->panic_draw_xy instead.

...

• 	goto plane_unlock;
  

• /*
  

•  * fbcon puts out panic messages so stay away to avoid jumbled
  

•  * output. If vc->vc_mode != KD_TEXT fbcon won't put out
  

•  * messages (see vt_console_print).
  

•  */
  

• if (dev->fb_helper && dev->fb_helper->fb == fb)
  

This is a bit a layering violation. Could we instead tell fbcon that it shouldn't do panic handling for drm drivers? I think that would resolve this overlap here in a much cleaner way. drm fbdev helpers already have lots of oops_in_progress checks all over to make sure fbcon doesn't do anything bad. That only leaves the actual rendering, which I think we can stop too with a simple flag.

Ofc only for atomic drivers which have this panic handling mode here implemented.

There used to be a fbdev flag FBINFO_CAN_FORCE_OUTPUT that controlled vc->vc_panic_force_write, but it's gone now I see.

I killed that :-)

Looking at those patches again, it's not what we wanted. It was used to force panic display even when not in KD_TEXT mode.

What we want instead here is a flag to tell fbcon/vt to _never_ write dmesg to our console, not even when the console is in KD_TEXT mode. Because we have a separate panic handler to display it. Heck maybe that QR code thing could be resurrected eventually again.

Totally untested snippet below is what I'm thinking of:

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 7f4c22a65f63..b08c63286ed0 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -282,6 +282,9 @@ static inline int fbcon_is_inactive(struct vc_data *vc, struct fb_info *info) { struct fbcon_ops *ops = info->fbcon_par;

• if (oops_in_progress && info->flags & NO_OOPS_OUTPUT)

• return false;
  

• return (info->state != FBINFO_STATE_RUNNING || vc->vc_mode != KD_TEXT || ops->graphics);

}

Plus then unconditionally rendering the oops output on the drm side, even if the current fb is the fbcon one.

Hm thinking about this some more, I think this would allow us to replace all the oops_in_progress checks we have in drm_fb_helper.c with WARN_ON(oops_in_progress). So worthy cleanup on its own.

Assuming I'm seeing this right ofc, very much possible I'm missing something :-) -Daniel

On Mon, Mar 11, 2019 at 11:33:15PM +0100, Noralf Trønnes wrote:

Den 11.03.2019 20.23, skrev Daniel Vetter:

...

On Mon, Mar 11, 2019 at 06:42:16PM +0100, Noralf Trønnes wrote:

...

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

Bunch of comments/ideas for you or Darwish below, whoever picks this up.

Actually it would ne nice if Darwish could pick it up since he will do it on i915 which will be useful to a much broader audience. If not I'll respin when I'm done with the drm_fb_helper refactoring.

<snip>

...

...

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c new file mode 100644 index 000000000000..4bca7f0bc369 --- /dev/null +++ b/drivers/gpu/drm/drm_panic.c @@ -0,0 +1,363 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright 2018 Noralf Trønnes

+#include <linux/debugfs.h> +#include <linux/font.h> +#include <linux/kernel.h> +#include <linux/kmsg_dump.h> +#include <linux/seq_file.h> +#include <linux/slab.h> +#include <linux/uaccess.h>

+#include <drm/drm_crtc.h> +#include <drm/drm_device.h> +#include <drm/drm_drv.h> +#include <drm/drm_file.h> +#include <drm/drm_framebuffer.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_plane.h> +#include <drm/drm_print.h> +#include <drm/drmP.h>

+#include "drm_internal.h"

+/*

• • The log lines in an ARM stack dump are 92 characters long
• • and 120 is a nice multiple for HD and 4K.
• */

+#define DRM_PANIC_COLUMN_WIDTH 120

I think we should compute this dynamically from the actual fb width and font witdth.

The font is picked based on whether it can fit 50 lines of text.

The next rule is to decide when to add another column. I decided to use a number that gets most log lines in one line on the screen. If most lines get broken into two lines, there's not much use in an extra column.

Do you have another idea on how to do this?

(There's even a 16x32 font now that wasn't available when I did this.)

Oh I had no idea that this is for multi-column oops printing. That's indeed neat for 4k screens.

...

...

+struct drm_panic_ctx {

• struct drm_framebuffer *fb;
• unsigned int width;
• unsigned int height;
• void *vmap;
• const struct font_desc *font;
• unsigned int col_width;
• unsigned int bottom_y;
• size_t max_line_length;
• unsigned int x;
• unsigned int y;

+};

+static const struct font_desc *drm_panic_font8x8, *drm_panic_font8x16;

+static void drm_panic_draw_xy(struct drm_framebuffer *fb, void *vmap,

• 	      int x, int y, bool fg)
  

+{

• if (fb->funcs->panic_draw_xy)

• fb->funcs->panic_draw_xy(fb, vmap, x, y, fg);
  

• else

• drm_framebuffer_panic_draw_xy(fb, vmap, x, y, fg);
  

+}

+static void drm_panic_render_char(struct drm_panic_ctx *ctx,

• 		  unsigned int offset, char c)
  

+{

• unsigned int h, w, x, y;
• u8 fontline;
• for (h = 0; h < ctx->font->height; h++) {

• fontline = *(u8 *)(ctx->font->data + c * ctx->font->height + h);
  

• for (w = 0; w < ctx->font->width; w++) {
  

• 	x = ctx->x + (offset * ctx->font->width) + w;
  

• 	y = ctx->y + h;
  

• 	drm_panic_draw_xy(ctx->fb, ctx->vmap, x, y,
  

• 			  fontline & BIT(7 - w));
  

• }
  

• }

+}

+static void drm_panic_render_line(struct drm_panic_ctx *ctx,

• 		  const char *line, size_t len)
  

+{

• unsigned int i;
• for (i = 0; i < len; i++)

• drm_panic_render_char(ctx, i, line[i]);
  

• /* Clear out the rest of the line */
• for (i = len; i < ctx->max_line_length; i++)

• drm_panic_render_char(ctx, i, ' ');
  

+}

+static bool drm_panic_newline(struct drm_panic_ctx *ctx) +{

• if (ctx->x == 0 && ctx->y == 0)

• return false;
  

• if (ctx->y == 0) {

• ctx->x -= ctx->col_width;
  

• ctx->y = ctx->bottom_y;
  

• } else {

• ctx->y -= ctx->font->height;
  

• }
• return true;

+}

+/* Render from bottom right most column */ +static bool drm_panic_render_lines(struct drm_panic_ctx *ctx,

• 		   const char *str, size_t len)
  

+{

• size_t l, line_length;
• const char *pos;
• int i;
• while (len) {

• pos = str + len - 1;
  

• if (*pos == '\n') {
  

• 	len--;
  

• 	if (!drm_panic_newline(ctx))
  

• 		return false;
  

• 	continue;
  

• }
  

• while (pos > str && *(pos - 1) != '\n')
  

• 	pos--;
  

• line_length = len - (pos - str);
  

• if (!line_length || len < line_length) {
  

• 	pr_err("%s: Bug! line_length=%zu len=%zu\n",
  

• 	       __func__, line_length, len);
  

• 	return false;
  

• }
  

• len -= line_length;
  

• for (i = DIV_ROUND_UP(line_length, ctx->max_line_length) - 1; i >= 0; i--) {
  

• 	l = min(ctx->max_line_length, line_length - i * ctx->max_line_length);
  

• 	drm_panic_render_line(ctx, pos + (i * ctx->max_line_length), l);
  

• 	if (i && !drm_panic_newline(ctx))
  

• 		return false;
  

• }
  

• }
• return true;

+}

+static void drm_panic_kmsg_render_screen(struct drm_plane *plane,

• 			 struct kmsg_dumper *dumper)
  

+{

• struct drm_framebuffer *fb = plane->state->fb;
• bool first_iteration = true;
• struct drm_panic_ctx ctx;
• static char text[1024];
• size_t len;
• ctx.vmap = fb->funcs->panic_vmap(fb);
• /* Print some info when testing */
• if (dumper->max_reason == KMSG_DUMP_OOPS) {

• struct drm_format_name_buf format_name;
  

• pr_info("%s: [FB:%d] %ux%u format=%s vmap=%p\n",
  

• 	__func__, fb->base.id, fb->width, fb->height,
  

• 	drm_get_format_name(fb->format->format, &format_name),
  

• 	ctx.vmap);
  

• }
• if (!ctx.vmap)

• return;
  

For some framebuffers it might be useful to not require vmap, but instead have some page-by-page kind of access helper. Since preemptively setting up a vmap for all the non-continous buffers is a bit much, and we really can't set up the vmap in the panic handler.

Maybe we should call this panic_prepare/panic_finish and s/vmap/cookie/, to make it entirely opaque?

But a bit overengineering perhaps :-)

I guess i915 wants something else than vmap, but I have no idea how a page-by-page solution is to be implemented.

i915 should be mostly fine, since we have a GTT for remapping and can make it look continuous. It might not be in the part of the GTT accessible by the cpu though.

Wrt implementation: My idea would be to extract the pixel writing function and export it to drivers. The driver would then implement a ->panic_draw_xy function which looks up the right page, computes it address, computes the x/y offset within (taking into account tiling and stuff like that), and then uses the helper function to draw the right pixel value for the format at the given address.

That's why I suggested that drivers need to either implement ->panic_prepare or ->panic_draw_xy. Since for this approach you might not need a ->panic_prepare/cleanup implementation, since it's all done in ->panic_draw_xy on a pixel-by-pixel basis.

When we get bootsplash, we will at least have a kernel address during that phase for all drivers supporting it.

Hm, not following what you mean here. Why is bootsplash special?

...

...

• ctx.fb = fb;
• ctx.width = drm_rect_width(&plane->state->src) >> 16;
• ctx.height = drm_rect_height(&plane->state->src) >> 16;
• /*

• * TODO:
  

• * Find which part of the fb that is visible.
  

• * Width and height are zero on vc4
  

• */
  

• if (!ctx.width || !ctx.height) {

• ctx.width = fb->width;
  

• ctx.height = fb->height;
  

• }
• /* Try to fit 50 lines */
• if (ctx.height < 50 * 16 && drm_panic_font8x8)

• ctx.font = drm_panic_font8x8;
  

• else

• ctx.font = drm_panic_font8x16;
  

• ctx.col_width = DRM_PANIC_COLUMN_WIDTH * ctx.font->width;
• ctx.bottom_y = ((ctx.height / ctx.font->height) - 1) * ctx.font->height;
• if (ctx.width < 2 * ctx.col_width)

• ctx.max_line_length = ctx.width / ctx.font->width;
  

• else

• ctx.max_line_length = DRM_PANIC_COLUMN_WIDTH - 2; /* border=2 */
  

• ctx.x = 0;
• ctx.y = ctx.bottom_y;
• if (ctx.width > ctx.col_width)

• ctx.x = ((ctx.width / ctx.col_width) - 1) * ctx.col_width;
  

• pr_debug("%s: font=%s %ux%u max_line_length=%zu (%u,%u)\n",

•  __func__, ctx.font->name, ctx.width, ctx.height,
  

•  ctx.max_line_length, ctx.x, ctx.y);
  

• kmsg_dump_rewind(dumper);
• /* The latest messages are fetched first */
• while (kmsg_dump_get_buffer(dumper, false, text, sizeof(text), &len)) {

• /* Strip off the very last newline so we start at the bottom */
  

• if (first_iteration) {
  

• 	len--;
  

• 	first_iteration = false;
  

• }
  

• if (!drm_panic_render_lines(&ctx, text, len))
  

• 	break;
  

• }
• if (fb->funcs->panic_vunmap)

• fb->funcs->panic_vunmap(fb, vmap);
  

+}

+static void drm_panic_try_dev(struct drm_device *dev, struct kmsg_dumper *dumper) +{

• struct drm_framebuffer *fb;
• struct drm_plane *plane;
• struct drm_crtc *crtc;
• if (!drm_core_check_feature(dev, DRIVER_ATOMIC))

• return;
  

• if (dumper->max_reason == KMSG_DUMP_OOPS)

• pr_info("%s: %s on minor %d\n", __func__, dev->driver->name,
  

• 	dev->primary->index);
  

• drm_for_each_crtc(crtc, dev) {

• 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;
  

• /*
  

•  * TODO: Should we check plane_state->visible?
  

•  * It is not set on vc4
  

I think we should.

I need to recheck vc4, because when I did this a year ago ->visible was not set on vc4.

I think if you look at plane_state->src (i.e. the clipped src rect) then if that is non-zero I think you can assume ->visible is also set. Becuase that's both computed by the same helpers.

And if that's not the case, fall back to ->src_x/y/h/w and just assume the plane is visible (usually those are the simpler drivers anyway).

...

We should also check whether that primary is connected to the crtc (some hw you can reassign planes freely between crtc, and the crtc->primary pointer is only used for compat with legacy ioctl).

Like this? /* Check that the plane has not been reassigned */ if (plane->state->crtc != crtc) goto plane_unlock;

Yup.

...

...

• if (!plane->state || !plane->state->visible)
  

•  */
  

• if (!plane->state)
  

• 	goto plane_unlock;
  

• fb = plane->state->fb;
  

• if (!fb || !fb->funcs->panic_vmap)
  

Docs says this is optional, doesn't seem to be all that optional. I'd check for this or a driver-specific ->panic_draw_xy instead.

->panic_vmap determines whether panic is supported or not, so in that sense it's optional. ->panic_draw_xy is optional with a default fallback for the linear case.

...

...

• 	goto plane_unlock;
  

• /*
  

•  * fbcon puts out panic messages so stay away to avoid jumbled
  

•  * output. If vc->vc_mode != KD_TEXT fbcon won't put out
  

•  * messages (see vt_console_print).
  

•  */
  

• if (dev->fb_helper && dev->fb_helper->fb == fb)
  

• 	goto plane_unlock;
  

• drm_panic_kmsg_render_screen(plane, dumper);
  

+plane_unlock:

• ww_mutex_unlock(&plane->mutex.mutex);
  

+crtc_unlock:

• ww_mutex_unlock(&crtc->mutex.mutex);
  

• }

+}

+static int drm_panic_dev_iter(struct device *dev, void *data) +{

• struct drm_minor *minor = dev_get_drvdata(dev);
• if (minor && minor->type == DRM_MINOR_PRIMARY)

• drm_panic_try_dev(minor->dev, data);
  

• return 0;

+}

+static void drm_panic_kmsg_dump(struct kmsg_dumper *dumper,

• 		enum kmsg_dump_reason reason)
  

+{

• class_for_each_device(drm_class, NULL, dumper, drm_panic_dev_iter);

class_for_each_device uses klist, which only uses an irqsave spinlock. I think that's good enough. Comment to that effect would be good e.g.

/* based on klist, which uses only a spin_lock_irqsave, which we * assume still works */

If we aim for perfect this should be a trylock still, maybe using our own device list.

...

+}

+static struct kmsg_dumper drm_panic_kmsg_dumper = {

• .dump = drm_panic_kmsg_dump,
• .max_reason = KMSG_DUMP_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;
• char buf[24];
• size_t size;
• ssize_t ret;
• 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;
  

• drm_panic_kmsg_dumper.max_reason = KMSG_DUMP_OOPS;
• wmb();
• /* Do a real test with: echo c > /proc/sysrq-trigger */
• if (val == 0) {

• pr_info("Test panic screen using kmsg_dump(OOPS)\n");
  

• kmsg_dump(KMSG_DUMP_OOPS);
  

• } else if (val == 1) {

• char *null_pointer = NULL;
  

• pr_info("Test panic screen using NULL pointer dereference\n");
  

• *null_pointer = 1;
  

• } else {

• return -EINVAL;
  

• }

This isn't quite what I had in mind, since it still kills the kernel (like sysrq-trigger).

If val == 0, it doesn't kill the kernel, it only dumps the kernel log. And it doesn't taint the kernel either.

Ah I didn't realize that. Sounds like a good option to keep.

...

Instead what I had in mind is to recreate the worst possible panic context as much as feasible (disabling interrupts should be a good start, maybe we can even do an nmi callback), and then call our panic implementation. That way we can test the panic handler in a non-destructive way (i.e. aside from last dmesg lines printed to the screen nothing bad happens to the kernel: No real panic, no oops, no tainting).

The interrupt case I can do, nmi I have no idea.

I just read the printk nmi code again and it looks like there's now even more special handling for issues happening in nmi context, so we should never see an oops from nmi context. So local_irq_disable() wrapping should be good enough for testing. -Daniel

Noralf.

...

And igt testcase could then exercise this, and CI run it.

...

• return count;

+}

+static const struct file_operations drm_panic_panic_ops = {

• .write = drm_panic_file_panic_write,
• .open = simple_open,
• .llseek = default_llseek,

+};

+static struct dentry *drm_panic_d_panic;

+void __init drm_panic_init(struct dentry *debugfs_root) +{

• drm_panic_font8x8 = find_font("VGA8x8");
• drm_panic_font8x16 = find_font("VGA8x16");
• if (!drm_panic_font8x16) {

• DRM_WARN("Couldn't find font, panic screen disabled\n");
  

• return;
  

• }
• drm_panic_d_panic = debugfs_create_file("panic-test", 0200,

• 				debugfs_root, NULL,
  

• 				&drm_panic_panic_ops);
  

• kmsg_dump_register(&drm_panic_kmsg_dumper);

+}

+void drm_panic_exit(struct dentry *debugfs_root) +{

• kmsg_dump_unregister(&drm_panic_kmsg_dumper);
• debugfs_remove(drm_panic_d_panic);

+} diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f0b34c977ec5..f3274798ecfe 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -94,6 +94,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);
  

};

/** @@ -293,4 +331,7 @@ int drm_framebuffer_plane_width(int width, int drm_framebuffer_plane_height(int height, const struct drm_framebuffer *fb, int plane);

+void drm_framebuffer_panic_draw_xy(struct drm_framebuffer *fb, void *vmap,

• 		   int x, int y, bool foreground);
  

#endif

2.20.1

Hi Noralf.

Nice!

+++ b/drivers/gpu/drm/drm_panic.c @@ -0,0 +1,363 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright 2018 Noralf Trønnes

+#include <linux/debugfs.h> +#include <linux/font.h> +#include <linux/kernel.h> +#include <linux/kmsg_dump.h> +#include <linux/seq_file.h> +#include <linux/slab.h> +#include <linux/uaccess.h>

+#include <drm/drm_crtc.h> +#include <drm/drm_device.h> +#include <drm/drm_drv.h> +#include <drm/drm_file.h> +#include <drm/drm_framebuffer.h> +#include <drm/drm_fb_helper.h> +#include <drm/drm_plane.h> +#include <drm/drm_print.h> +#include <drm/drmP.h>

Please do not use drmP.h in new stuff. And res-sort the list of include files when add it.

I did not read much further, so no further pedantic comments in this round.

Sam

On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:

On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:

...

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

[...]

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f0b34c977ec5..f3274798ecfe 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -94,6 +94,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);

FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon drivers:

Framebuffers are normally tiled, writing to them with the CPU results in garbled output.

With a discrete GPU having a large amount of VRAM, the framebuffer may not be directly CPU accessible at all.

There would need to be a mechanism for switching scanout to a linear, CPU accessible framebuffer.

I suppose panic_vmap() could just provide a linear temp buffer to the panic handler, and panic_unmap() could copy the contents over to the real fb.

That said, this approach of scribbling over the primary plane's framebuffer has some clear limitations: * something may overwrite the oops message before the user can even read it * there may be other planes obscuring part or all of the primary plane

Also scribbling over the user's framebuffer seems rather rude to me, so I'm thinking this approach should be limited to kernel panics only.

On Tue, Mar 12, 2019 at 06:15:24PM +0100, Noralf Trønnes wrote:

Den 12.03.2019 17.17, skrev Ville Syrjälä:

...

On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:

...

On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:

...

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

[...]

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f0b34c977ec5..f3274798ecfe 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -94,6 +94,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);

FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon drivers:

Framebuffers are normally tiled, writing to them with the CPU results in garbled output.

In which case the driver needs to support the ->panic_draw_xy callback, or maybe it's possible to make a generic helper for tiled buffers.

...

...

With a discrete GPU having a large amount of VRAM, the framebuffer may not be directly CPU accessible at all.

I would have been nice to know how Windows works around this.

...

...

There would need to be a mechanism for switching scanout to a linear, CPU accessible framebuffer.

I suppose panic_vmap() could just provide a linear temp buffer to the panic handler, and panic_unmap() could copy the contents over to the real fb.

That said, this approach of scribbling over the primary plane's framebuffer has some clear limitations:

• something may overwrite the oops message before the user can even read it

When the dumper drm_panic_kmsg_dump() runs, the other CPU's should have been stopped. See panic().

GPUs etc. may still be executing away.

...

• there may be other planes obscuring part or all of the primary plane

Yeah, this is a problem, again I wonder how Windows deals with this.

Probably just disables all other planes. Not that it uses planes all that heavily.

...

Also scribbling over the user's framebuffer seems rather rude to me, so I'm thinking this approach should be limited to kernel panics only.

Yes this will only happen on kernel panics:

panic() -> kmsg_dump() -> drm_panic_kmsg_dump()

(Unless invoking through debugfs ofc)

I thought you set the max_level or whatever to OOPS. Doesn't that mean it gets involved for non-panics as well?

Den 12.03.2019 18.37, skrev Noralf Trønnes:

Den 12.03.2019 18.25, skrev Ville Syrjälä:

...

On Tue, Mar 12, 2019 at 06:15:24PM +0100, Noralf Trønnes wrote:

...

Den 12.03.2019 17.17, skrev Ville Syrjälä:

...

On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:

...

On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:

...

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

[...]

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f0b34c977ec5..f3274798ecfe 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -94,6 +94,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);

FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon drivers:

Framebuffers are normally tiled, writing to them with the CPU results in garbled output.

In which case the driver needs to support the ->panic_draw_xy callback, or maybe it's possible to make a generic helper for tiled buffers.

...

...

With a discrete GPU having a large amount of VRAM, the framebuffer may not be directly CPU accessible at all.

I would have been nice to know how Windows works around this.

...

...

There would need to be a mechanism for switching scanout to a linear, CPU accessible framebuffer.

I suppose panic_vmap() could just provide a linear temp buffer to the panic handler, and panic_unmap() could copy the contents over to the real fb.

That said, this approach of scribbling over the primary plane's framebuffer has some clear limitations:

• something may overwrite the oops message before the user can even read it

When the dumper drm_panic_kmsg_dump() runs, the other CPU's should have been stopped. See panic().

GPUs etc. may still be executing away.

Would it be safe to stop it in a panic situation? It would ofc be bad to crash the box even harder.

...

...

...

• there may be other planes obscuring part or all of the primary plane

Yeah, this is a problem, again I wonder how Windows deals with this.

Probably just disables all other planes. Not that it uses planes all that heavily.

...

...

Also scribbling over the user's framebuffer seems rather rude to me, so I'm thinking this approach should be limited to kernel panics only.

Yes this will only happen on kernel panics:

panic() -> kmsg_dump() -> drm_panic_kmsg_dump()

(Unless invoking through debugfs ofc)

I thought you set the max_level or whatever to OOPS. Doesn't that mean it gets involved for non-panics as well?

I do that in the debugfs code, but I can't remember why I lower level,

Now I remember, it is so I can catch the null pointer debugfs test, but that's not necessary since this is designed for panics, so I should remove that test.

Noralf.

I think can just change the level when invoking the dumper: drm_panic_file_panic_write(...)

• kmsg_dump(KMSG_DUMP_OOPS);
  

• kmsg_dump(KMSG_DUMP_PANIC);
  
  
  

This is the dumper config:

static struct kmsg_dumper drm_panic_kmsg_dumper = { .dump = drm_panic_kmsg_dump, .max_reason = KMSG_DUMP_PANIC, };

---

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

Am 12.03.19 um 19:02 schrieb Ville Syrjälä:

On Tue, Mar 12, 2019 at 06:37:57PM +0100, Noralf Trønnes wrote:

...

Den 12.03.2019 18.25, skrev Ville Syrjälä:

...

On Tue, Mar 12, 2019 at 06:15:24PM +0100, Noralf Trønnes wrote:

...

Den 12.03.2019 17.17, skrev Ville Syrjälä:

...

On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:

...

On 2019-03-11 6:42 p.m., Noralf Trønnes wrote: > This adds support for outputting kernel messages on panic(). > A kernel message dumper is used to dump the log. The dumper iterates > over each DRM device and it's crtc's to find suitable framebuffers. > > All the other dumpers are run before this one except mtdoops. > Only atomic drivers are supported. > > Signed-off-by: Noralf Trønnes noralf@tronnes.org > --- > [...] > > diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h > index f0b34c977ec5..f3274798ecfe 100644 > --- a/include/drm/drm_framebuffer.h > +++ b/include/drm/drm_framebuffer.h > @@ -94,6 +94,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); FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon drivers:

Framebuffers are normally tiled, writing to them with the CPU results in garbled output.

In which case the driver needs to support the ->panic_draw_xy callback, or maybe it's possible to make a generic helper for tiled buffers.

...

...

With a discrete GPU having a large amount of VRAM, the framebuffer may not be directly CPU accessible at all.

I would have been nice to know how Windows works around this.

...

...

There would need to be a mechanism for switching scanout to a linear, CPU accessible framebuffer.

I suppose panic_vmap() could just provide a linear temp buffer to the panic handler, and panic_unmap() could copy the contents over to the real fb.

That said, this approach of scribbling over the primary plane's framebuffer has some clear limitations:

• something may overwrite the oops message before the user can even read it

When the dumper drm_panic_kmsg_dump() runs, the other CPU's should have been stopped. See panic().

GPUs etc. may still be executing away.

Would it be safe to stop it in a panic situation? It would ofc be bad to crash the box even harder.

Some drivers/devices may have working (and hopefully even reliable) gpu reset, some may not.

Even if GPU reset is working, it certainly doesn't under a panic() condition when all other CPUs are already stopped.

I don't see how this approach should ever work reliable.

Christian.

On 2019-03-12 6:15 p.m., Noralf Trønnes wrote:

Den 12.03.2019 17.17, skrev Ville Syrjälä:

...

On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:

...

On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:

...

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

[...]

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f0b34c977ec5..f3274798ecfe 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -94,6 +94,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);

FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon drivers:

Framebuffers are normally tiled, writing to them with the CPU results in garbled output.

In which case the driver needs to support the ->panic_draw_xy callback, or maybe it's possible to make a generic helper for tiled buffers.

I'm afraid that won't help, at least not without porting big chunks of https://gitlab.freedesktop.org/mesa/mesa/tree/master/src/amd/addrlib into the kernel, none of which will be used for anything else.

...

...

There would need to be a mechanism for switching scanout to a linear, CPU accessible framebuffer.

I suppose panic_vmap() could just provide a linear temp buffer to the panic handler, and panic_unmap() could copy the contents over to the real fb.

Copy how? Using a GPU engine?

Am 13.03.19 um 14:31 schrieb Ville Syrjälä:

On Wed, Mar 13, 2019 at 10:35:08AM +0100, Michel Dänzer wrote:

...

On 2019-03-12 6:15 p.m., Noralf Trønnes wrote:

...

Den 12.03.2019 17.17, skrev Ville Syrjälä:

...

On Tue, Mar 12, 2019 at 11:47:04AM +0100, Michel Dänzer wrote:

...

On 2019-03-11 6:42 p.m., Noralf Trønnes wrote:

...

This adds support for outputting kernel messages on panic(). A kernel message dumper is used to dump the log. The dumper iterates over each DRM device and it's crtc's to find suitable framebuffers.

All the other dumpers are run before this one except mtdoops. Only atomic drivers are supported.

Signed-off-by: Noralf Trønnes noralf@tronnes.org

[...]

diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h index f0b34c977ec5..f3274798ecfe 100644 --- a/include/drm/drm_framebuffer.h +++ b/include/drm/drm_framebuffer.h @@ -94,6 +94,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);

FWIW, the panic_vmap hook cannot work in general with the amdgpu/radeon drivers:

Framebuffers are normally tiled, writing to them with the CPU results in garbled output.

In which case the driver needs to support the ->panic_draw_xy callback, or maybe it's possible to make a generic helper for tiled buffers.

I'm afraid that won't help, at least not without porting big chunks of https://gitlab.freedesktop.org/mesa/mesa/tree/master/src/amd/addrlib into the kernel, none of which will be used for anything else.

...

...

...

There would need to be a mechanism for switching scanout to a linear, CPU accessible framebuffer.

I suppose panic_vmap() could just provide a linear temp buffer to the panic handler, and panic_unmap() could copy the contents over to the real fb.

Copy how? Using a GPU engine?

CPU maybe? Though I suppose that won't work if the buffer isn't CPU accesible :/

Well we do have a debug path for accessing invisible memory with the CPU.

E.g. three registers: DATA and auto increment OFFSET_LO/HI. So you can just read/write DATA over and over again if you want to access some memory.

But turning of tilling etc is still extremely tricky when the system is already unstable.

I mean from two miles high it looks like a nice to have feature, but up close is a different picture...

Christian.