* [PATCH 0/3] drm: Add panic handling
@ 2016-09-11 18:47 Noralf Trønnes
2016-09-11 18:47 ` [PATCH 1/3] drm: Add support for panic message output Noralf Trønnes
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Noralf Trønnes @ 2016-09-11 18:47 UTC (permalink / raw)
To: dri-devel; +Cc: laurent.pinchart
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] drm: Add support for panic message output 2016-09-11 18:47 [PATCH 0/3] drm: Add panic handling Noralf Trønnes @ 2016-09-11 18:47 ` Noralf Trønnes 2016-09-12 18:09 ` Noralf Trønnes 2016-09-11 18:47 ` [PATCH 2/3] drm/fb_cma_helper: Add panic handling Noralf Trønnes ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Noralf Trønnes @ 2016-09-11 18:47 UTC (permalink / raw) To: dri-devel; +Cc: laurent.pinchart 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] drm: Add support for panic message output 2016-09-11 18:47 ` [PATCH 1/3] drm: Add support for panic message output Noralf Trønnes @ 2016-09-12 18:09 ` Noralf Trønnes 0 siblings, 0 replies; 11+ messages in thread From: Noralf Trønnes @ 2016-09-12 18:09 UTC (permalink / raw) To: dri-devel; +Cc: laurent.pinchart 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] drm/fb_cma_helper: Add panic handling 2016-09-11 18:47 [PATCH 0/3] drm: Add panic handling Noralf Trønnes 2016-09-11 18:47 ` [PATCH 1/3] drm: Add support for panic message output Noralf Trønnes @ 2016-09-11 18:47 ` Noralf Trønnes 2016-10-05 13:22 ` Laurent Pinchart 2016-09-11 18:47 ` [PATCH 3/3] drm/simpledrm: " Noralf Trønnes 2016-10-02 13:47 ` [PATCH 0/3] drm: " Noralf Trønnes 3 siblings, 1 reply; 11+ messages in thread From: Noralf Trønnes @ 2016-09-11 18:47 UTC (permalink / raw) To: dri-devel; +Cc: laurent.pinchart 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling 2016-09-11 18:47 ` [PATCH 2/3] drm/fb_cma_helper: Add panic handling Noralf Trønnes @ 2016-10-05 13:22 ` Laurent Pinchart 2016-10-05 19:36 ` Noralf Trønnes 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2016-10-05 13:22 UTC (permalink / raw) To: Noralf Trønnes; +Cc: dri-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling 2016-10-05 13:22 ` Laurent Pinchart @ 2016-10-05 19:36 ` Noralf Trønnes 2016-10-06 9:12 ` Daniel Vetter 0 siblings, 1 reply; 11+ messages in thread From: Noralf Trønnes @ 2016-10-05 19:36 UTC (permalink / raw) To: Laurent Pinchart; +Cc: dri-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling 2016-10-05 19:36 ` Noralf Trønnes @ 2016-10-06 9:12 ` Daniel Vetter 2016-10-06 10:17 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2016-10-06 9:12 UTC (permalink / raw) To: Noralf Trønnes; +Cc: Laurent Pinchart, dri-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/fb_cma_helper: Add panic handling 2016-10-06 9:12 ` Daniel Vetter @ 2016-10-06 10:17 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2016-10-06 10:17 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] drm/simpledrm: Add panic handling 2016-09-11 18:47 [PATCH 0/3] drm: Add panic handling Noralf Trønnes 2016-09-11 18:47 ` [PATCH 1/3] drm: Add support for panic message output Noralf Trønnes 2016-09-11 18:47 ` [PATCH 2/3] drm/fb_cma_helper: Add panic handling Noralf Trønnes @ 2016-09-11 18:47 ` Noralf Trønnes 2016-10-02 13:47 ` [PATCH 0/3] drm: " Noralf Trønnes 3 siblings, 0 replies; 11+ messages in thread From: Noralf Trønnes @ 2016-09-11 18:47 UTC (permalink / raw) To: dri-devel; +Cc: laurent.pinchart 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] drm: Add panic handling 2016-09-11 18:47 [PATCH 0/3] drm: Add panic handling Noralf Trønnes ` (2 preceding siblings ...) 2016-09-11 18:47 ` [PATCH 3/3] drm/simpledrm: " Noralf Trønnes @ 2016-10-02 13:47 ` Noralf Trønnes 2016-10-05 13:14 ` Daniel Vetter 3 siblings, 1 reply; 11+ messages in thread From: Noralf Trønnes @ 2016-10-02 13:47 UTC (permalink / raw) To: dri-devel; +Cc: laurent.pinchart 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] drm: Add panic handling 2016-10-02 13:47 ` [PATCH 0/3] drm: " Noralf Trønnes @ 2016-10-05 13:14 ` Daniel Vetter 0 siblings, 0 replies; 11+ messages in thread From: Daniel Vetter @ 2016-10-05 13:14 UTC (permalink / raw) To: Noralf Trønnes, David Herrmann; +Cc: Laurent Pinchart, dri-devel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-10-06 10:17 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-11 18:47 [PATCH 0/3] drm: Add panic handling Noralf Trønnes 2016-09-11 18:47 ` [PATCH 1/3] drm: Add support for panic message output Noralf Trønnes 2016-09-12 18:09 ` Noralf Trønnes 2016-09-11 18:47 ` [PATCH 2/3] drm/fb_cma_helper: Add panic handling Noralf Trønnes 2016-10-05 13:22 ` Laurent Pinchart 2016-10-05 19:36 ` Noralf Trønnes 2016-10-06 9:12 ` Daniel Vetter 2016-10-06 10:17 ` Laurent Pinchart 2016-09-11 18:47 ` [PATCH 3/3] drm/simpledrm: " Noralf Trønnes 2016-10-02 13:47 ` [PATCH 0/3] drm: " Noralf Trønnes 2016-10-05 13:14 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).