[PATCH 3/3] drm/panic: Add a kmsg dump screen

[Javier Martinez Canillas]

Jocelyn Falempe <jfalempe at redhat.com> writes:

> Add a kmsg dump option, which will display the last lines of kmsg,
> and should be similar to fbcon.
> Add a Kconfig choice for the panic screen, so that the user can
> choose between this new kmsg dump, or the userfriendly option.
>
> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> ---
>  drivers/gpu/drm/Kconfig     |  21 +++++
>  drivers/gpu/drm/drm_panic.c | 151 +++++++++++++++++++++++++++---------
>  2 files changed, 136 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 9703429de6b9..78d401b55102 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -137,6 +137,27 @@ config DRM_PANIC_DEBUG
>  	  This is unsafe and should not be enabled on a production build.
>  	  If in doubt, say "N".
>  
> +choice
> +	prompt "Panic screen formater"
> +	default DRM_PANIC_SCREEN_USERFRIENDLY
> +	depends on DRM_PANIC
> +	help
> +	  This option enable to choose what will be displayed when a kernel
> +	  panic occurs.
> +
> +	config DRM_PANIC_SCREEN_USERFRIENDLY
> +		bool "Default user friendly message"
> +		help
> +		  Only a short message telling the user to reboot the system.
> +
> +	config DRM_PANIC_SCREEN_KMSG
> +		bool "Display the last lines of kmsg"
> +		help
> +		  Display kmsg last lines on panic.
> +		  Enable if you are a kernel developer, and want to debug a
> +		  kernel panic.
> +endchoice

Why having it as a compile time option and not a runtime option ? AFAICT
this could be a drm.panic_format= kernel command line parameter instead.

[...]
  
> -/*
> - * Draw the panic message at the center of the screen
> - */
> +#if defined(CONFIG_DRM_PANIC_SCREEN_USERFRIENDLY)
> +

And that could avoid ifdefery in the C file.

[...]

> +#elif defined(CONFIG_DRM_PANIC_SCREEN_KMSG)
> +
> +#include <linux/kmsg_dump.h>
> +#include <linux/printk.h>
> +

I would add these even if guarded by DRM_PANIC_SCREEN_KMSG, to the
start of the C file where are the other headers include directives.

The patch looks good to me though, so if you prefer to keep it as a
build time option:

Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat