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

[Jocelyn Falempe]

On 07/06/2024 11:16, Javier Martinez Canillas wrote:
> Jocelyn Falempe <jfalempe at redhat.com> writes:
> 
>> Add a kmsg option, which will display the last lines of kmsg,
>> and should be similar to fbcon.
>> Add a drm.panic_screen module parameter, so you can choose between
>> the different panic screens available.
>> two options currently, but more will be added later:
>>   * "user": a short message telling the user to reboot the machine.
>>   * "kmsg": fill the screen with the last lines of kmsg.
>>
>> You can even change it at runtime by writing to
>> /sys/module/drm/parameters/panic_screen
>>
> 
> Great!
> 
>> v2:
>>   * use module parameter instead of Kconfig choice
>>     (Javier Martinez Canillas)
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>> ---
>>   drivers/gpu/drm/Kconfig     |  11 ++++
>>   drivers/gpu/drm/drm_panic.c | 108 ++++++++++++++++++++++++++++++++----
>>   2 files changed, 109 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 9703429de6b9..944815cee080 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -137,6 +137,17 @@ config DRM_PANIC_DEBUG
>>   	  This is unsafe and should not be enabled on a production build.
>>   	  If in doubt, say "N".
>>   
>> +config DRM_PANIC_SCREEN
>> +	string "Panic screen formater"
>> +	default "user"
>> +	depends on DRM_PANIC
>> +	help
>> +	  This option enable to choose what will be displayed when a kernel
>> +	  panic occurs. You can choose between "user", a short message telling
>> +	  the user to reboot the system, or "kmsg" which will display the last
>> +	  lines of kmsg.
> 
> Maybe I would mention here that this is only about the default, but that
> can be changed using the "drm.panic_screen=" kernel cmdline parameter or
> writting to the /sys/module/drm/parameters/panic_screen sysfs entry.
> 
> [...]

Done
> 
>> +static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb)
>> +{
>> +	u32 fg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format);
>> +	u32 bg_color = convert_from_xrgb8888(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format);
>> +	const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL);
> 
> Dan reported that get_default_font() can return NULL....

> 
>> +	struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height);
>> +	struct kmsg_dump_iter iter;
>> +	char kmsg_buf[512];
>> +	size_t kmsg_len;
>> +	struct drm_panic_line line;
>> +	int yoffset = sb->height - font->height - (sb->height % font->height) / 2;
>> +
>> +	if (!font)
>> +		return;
>> +
> 
> ... so you have to calculate yoffset after checking if the font is not NULL.

Yes I fixed that too.

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

Thanks a lot.

I just pushed this series to drm-misc-next.

Best regards,

-- 

Jocelyn