On 07/08/2024 11:21, Daniel Vetter wrote:
On Tue, Jul 23, 2024 at 11:11:34AM +0200, Jocelyn Falempe wrote:
Now that kmsg dump callback has the description parameter, use it in
the user panic screen.
This is the string passed to panic(), like "VFS: Unable to mount root
fs on xxx" or "Attempted to kill init! exitcode=0xxxx".
It gives a hint on why the panic occurred, without being too cryptic.
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
---
drivers/gpu/drm/drm_panic.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 2efede7fa23a..fb283695f50e 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -81,6 +81,8 @@ static struct drm_panic_line panic_msg[] = {
PANIC_LINE("KERNEL PANIC!"),
PANIC_LINE(""),
PANIC_LINE("Please reboot your computer."),
+ PANIC_LINE(""),
+ PANIC_LINE(""), /* will be replaced by the panic description */
};
static const struct drm_panic_line logo_ascii[] = {
@@ -633,7 +635,20 @@ static void draw_panic_dispatch(struct drm_scanout_buffer *sb)
}
}
-static void draw_panic_plane(struct drm_plane *plane)
+static void drm_panic_set_description(const char *description)
+{
+ u32 len;
+ if (description) {
#define PANIC_REASON_LINE ARRAY_SIZE(panic_msg)
instead of open coding a magic for.
Agreed
+ panic_msg[4].txt = description;
+ len = strlen(description);
+ /* ignore the last newline character */
+ if (len && description[len - 1] == '\n')
+ len -= 1;
+ panic_msg[4].len = len;
+ }
+}
+
+static void draw_panic_plane(struct drm_plane *plane, const char *description)
{
struct drm_scanout_buffer sb = { };
int ret;
@@ -642,6 +657,8 @@ static void draw_panic_plane(struct drm_plane *plane)
if (!drm_panic_trylock(plane->dev, flags))
return;
+ drm_panic_set_description(description);
I think a drm_panic_clear_description at the end of this function would be
good, so we don't leve dangling pointers around (the passed-in line might
be dynamically generated and stack allocated).
Yes it is stack allocated. Currently it works because you only use the
panic_msg from this draw_panic_plane() function.
But I agree clearing the description at the end is better.
+
ret = plane->helper_private->get_scanout_buffer(plane, &sb);
if (!ret && drm_panic_is_format_supported(sb.format)) {
@@ -662,7 +679,7 @@ static void drm_panic(struct kmsg_dumper *dumper, struct kmsg_dump_detail *detai
struct drm_plane *plane = to_drm_plane(dumper);
if (detail->reason == KMSG_DUMP_PANIC)
- draw_panic_plane(plane);
+ draw_panic_plane(plane, detail->description);
}
@@ -682,7 +699,7 @@ static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_
if (kstrtobool_from_user(user_buf, count, &run) == 0 && run) {
struct drm_plane *plane = file->private_data;
- draw_panic_plane(plane);
+ draw_panic_plane(plane, "Test from debugfs");
Hm, should we make this line intentionally very long, to make sure we
exercise the overflow logic for too long lines? Like so long that even on
a 4k panel with a small font it will overflow. I worry otherwise that
there will be a bug in that logic, taking down the machine at the worst
possible moment ...
longer lines are truncated, so that the code stays simple, and ensure it
won't draw out of the screen.
Reminds me, I think a bunch of kunit testcases to make sure the drm panic
code is absolutely solid (there's a bunch of math in there after all)
would be really good to add.
Yes, I need to add some kunit testcases.
When writing the drawing code, I used a small C program and write the
result to a png, so I was able to check for different resolutions, and
it's easier to debug than in the kernel.
I will send a v2.
Cheers, Sima
}
return count;
}
--
2.45.2
