[PATCH 5/5] drm/panic: Add panic description

[Daniel Vetter]

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 at redhat.com>
> ---
>  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.

> +		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).

> +
>  	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 ...

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.

Cheers, Sima

>  	}
>  	return count;
>  }
> -- 
> 2.45.2
> 

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