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

[Jocelyn Falempe]

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

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