[PATCH 0/5] drm/panic: Cleanup and add panic description

[PATCH 0/5] drm/panic: Cleanup and add panic description

[Jocelyn Falempe]

This small series removes unused export symbols, moves internal include to drm_crtc_internal.h, and adds the panic description, now that kmsg_dump provides this information.

Jocelyn Falempe (5):
  drm/panic: Remove space before "!" in panic message
  drm/panic: Remove useless export symbols
  drm/panic: Move drm_panic_register prototype to drm_crtc_internal.h
  drm/panic: Move copyright notice to the top
  drm/panic: Add panic description

 drivers/gpu/drm/drm_crtc_internal.h |  4 ++++
 drivers/gpu/drm/drm_panic.c         | 27 +++++++++++++++++++++------
 include/drm/drm_panic.h             | 21 ++++++---------------
 3 files changed, 31 insertions(+), 21 deletions(-)


base-commit: f00bfaca704ca1a2c4e31501a0a7d4ee434e73a7
-- 
2.45.2

[PATCH 1/5] drm/panic: Remove space before "!" in panic message

[Jocelyn Falempe]

There is no space between the last word, and the punctuation mark in
English.

Suggested-by: Diego Viola <diego.viola@gmail.com>
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_panic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 072752b658f0..5e873469856f 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -78,7 +78,7 @@ struct drm_panic_line {
 #define PANIC_LINE(s) {.len = sizeof(s) - 1, .txt = s}
 
 static struct drm_panic_line panic_msg[] = {
-	PANIC_LINE("KERNEL PANIC !"),
+	PANIC_LINE("KERNEL PANIC!"),
 	PANIC_LINE(""),
 	PANIC_LINE("Please reboot your computer."),
 };
-- 
2.45.2

[PATCH 2/5] drm/panic: Remove useless export symbols

[Jocelyn Falempe]

drm_panic_[un]register() are called only from the core drm, so there
is no need to export them.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_panic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
index 5e873469856f..2efede7fa23a 100644
--- a/drivers/gpu/drm/drm_panic.c
+++ b/drivers/gpu/drm/drm_panic.c
@@ -752,7 +752,6 @@ void drm_panic_register(struct drm_device *dev)
 	if (registered_plane)
 		drm_info(dev, "Registered %d planes with drm panic\n", registered_plane);
 }
-EXPORT_SYMBOL(drm_panic_register);
 
 /**
  * drm_panic_unregister()
@@ -771,4 +770,3 @@ void drm_panic_unregister(struct drm_device *dev)
 		kmsg_dump_unregister(&plane->kmsg_panic);
 	}
 }
-EXPORT_SYMBOL(drm_panic_unregister);
-- 
2.45.2

[PATCH 3/5] drm/panic: Move drm_panic_register prototype to drm_crtc_internal.h

[Jocelyn Falempe]

drm_panic_[un]register() are only used by the core drm, and are not
intended to be called by other drm drivers, so move their prototypes
to drm_crtc_internal.h.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/drm_crtc_internal.h |  4 ++++
 include/drm/drm_panic.h             | 12 ------------
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 4314274c5e40..3090f8ce92fe 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -320,8 +320,12 @@ drm_edid_load_firmware(struct drm_connector *connector)
 /* drm_panic.c */
 #ifdef CONFIG_DRM_PANIC
 bool drm_panic_is_enabled(struct drm_device *dev);
+void drm_panic_register(struct drm_device *dev);
+void drm_panic_unregister(struct drm_device *dev);
 #else
 static inline bool drm_panic_is_enabled(struct drm_device *dev) { return false; }
+static inline void drm_panic_register(struct drm_device *dev) {}
+static inline void drm_panic_unregister(struct drm_device *dev) {}
 #endif
 
 #endif /* __DRM_CRTC_INTERNAL_H__ */
diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
index 73bb3f3d9ed9..a4bd3681920d 100644
--- a/include/drm/drm_panic.h
+++ b/include/drm/drm_panic.h
@@ -146,16 +146,4 @@ struct drm_scanout_buffer {
 #define drm_panic_unlock(dev, flags) \
 	raw_spin_unlock_irqrestore(&(dev)->mode_config.panic_lock, flags)
 
-#ifdef CONFIG_DRM_PANIC
-
-void drm_panic_register(struct drm_device *dev);
-void drm_panic_unregister(struct drm_device *dev);
-
-#else
-
-static inline void drm_panic_register(struct drm_device *dev) {}
-static inline void drm_panic_unregister(struct drm_device *dev) {}
-
-#endif
-
 #endif /* __DRM_PANIC_H__ */
-- 
2.45.2

[PATCH 4/5] drm/panic: Move copyright notice to the top

[Jocelyn Falempe]

Move the copyright notice to the top of drm_panic.h, and add the
missing Red Hat copyright notice.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 include/drm/drm_panic.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
index a4bd3681920d..54085d5d05c3 100644
--- a/include/drm/drm_panic.h
+++ b/include/drm/drm_panic.h
@@ -1,4 +1,10 @@
 /* SPDX-License-Identifier: GPL-2.0 or MIT */
+
+/*
+ * Copyright (c) 2024 Intel
+ * Copyright (c) 2024 Red Hat
+ */
+
 #ifndef __DRM_PANIC_H__
 #define __DRM_PANIC_H__
 
@@ -8,9 +14,6 @@
 
 #include <drm/drm_device.h>
 #include <drm/drm_fourcc.h>
-/*
- * Copyright (c) 2024 Intel
- */
 
 /**
  * struct drm_scanout_buffer - DRM scanout buffer
-- 
2.45.2

Re: [PATCH 4/5] drm/panic: Move copyright notice to the top

[Daniel Vetter]

On Tue, Jul 23, 2024 at 11:11:33AM +0200, Jocelyn Falempe wrote:
> Move the copyright notice to the top of drm_panic.h, and add the
> missing Red Hat copyright notice.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>

Patches 1-4 are Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Cheers, Sima
> ---
>  include/drm/drm_panic.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h
> index a4bd3681920d..54085d5d05c3 100644
> --- a/include/drm/drm_panic.h
> +++ b/include/drm/drm_panic.h
> @@ -1,4 +1,10 @@
>  /* SPDX-License-Identifier: GPL-2.0 or MIT */
> +
> +/*
> + * Copyright (c) 2024 Intel
> + * Copyright (c) 2024 Red Hat
> + */
> +
>  #ifndef __DRM_PANIC_H__
>  #define __DRM_PANIC_H__
>  
> @@ -8,9 +14,6 @@
>  
>  #include <drm/drm_device.h>
>  #include <drm/drm_fourcc.h>
> -/*
> - * Copyright (c) 2024 Intel
> - */
>  
>  /**
>   * struct drm_scanout_buffer - DRM scanout buffer
> -- 
> 2.45.2
> 

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

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

[Jocelyn Falempe]

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@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) {
+		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);
+
 	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");
 	}
 	return count;
 }
-- 
2.45.2

Re: [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@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

Re: [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@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
>>
> 

Re: [PATCH 0/5] drm/panic: Cleanup and add panic description

[Thomas Zimmermann]

Am 23.07.24 um 11:11 schrieb Jocelyn Falempe:
> This small series removes unused export symbols, moves internal include to drm_crtc_internal.h, and adds the panic description, now that kmsg_dump provides this information.
>
> Jocelyn Falempe (5):
>    drm/panic: Remove space before "!" in panic message
>    drm/panic: Remove useless export symbols
>    drm/panic: Move drm_panic_register prototype to drm_crtc_internal.h
>    drm/panic: Move copyright notice to the top
>    drm/panic: Add panic description

For the whole series:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

>
>   drivers/gpu/drm/drm_crtc_internal.h |  4 ++++
>   drivers/gpu/drm/drm_panic.c         | 27 +++++++++++++++++++++------
>   include/drm/drm_panic.h             | 21 ++++++---------------
>   3 files changed, 31 insertions(+), 21 deletions(-)
>
>
> base-commit: f00bfaca704ca1a2c4e31501a0a7d4ee434e73a7

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 0/5] drm/panic: Cleanup and add panic description

[Jocelyn Falempe]

On 23/07/2024 11:11, Jocelyn Falempe wrote:
> This small series removes unused export symbols, moves internal include to drm_crtc_internal.h, and adds the panic description, now that kmsg_dump provides this information.

I've pushed it to drm-misc-next.

Thanks,

-- 

Jocelyn

> 
> Jocelyn Falempe (5):
>    drm/panic: Remove space before "!" in panic message
>    drm/panic: Remove useless export symbols
>    drm/panic: Move drm_panic_register prototype to drm_crtc_internal.h
>    drm/panic: Move copyright notice to the top
>    drm/panic: Add panic description
> 
>   drivers/gpu/drm/drm_crtc_internal.h |  4 ++++
>   drivers/gpu/drm/drm_panic.c         | 27 +++++++++++++++++++++------
>   include/drm/drm_panic.h             | 21 ++++++---------------
>   3 files changed, 31 insertions(+), 21 deletions(-)
> 
> 
> base-commit: f00bfaca704ca1a2c4e31501a0a7d4ee434e73a7