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

[Jocelyn Falempe]

On 31/05/2024 11:32, Javier Martinez Canillas wrote:
> 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.

Yes, I didn't think about it. That would allow to get more debug 
information from a user without rebuilding the kernel.

I'll prepare a v2 with that.

I prefer to use a drm.panic_screen=, as "format" might be confusing with 
the color format ?

> 
> [...]
>    
>> -/*
>> - * 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>
>