[RFC][PATCH v7 0/9] drm/panic: Add a drm panic handler

Fri Jan 12 14:36:28 UTC 2024

On 12/01/2024 15:00, Daniel Vetter wrote:
> On Thu, Jan 04, 2024 at 05:00:44PM +0100, Jocelyn Falempe wrote:
>> This introduces a new drm panic handler, which displays a message when a panic occurs.
>> So when fbcon is disabled, you can still see a kernel panic.
>>
>> This is one of the missing feature, when disabling VT/fbcon in the kernel:
>> https://www.reddit.com/r/linux/comments/10eccv9/config_vtn_in_2023/
>> Fbcon can be replaced by a userspace kms console, but the panic screen must be done in the kernel.
>>
>> This is a proof of concept, and works with simpledrm, mgag200, ast, and imx using a new get_scanout_buffer() api
>>
>> To test it, make sure you're using the simpledrm driver, and trigger a panic:
>> echo c > /proc/sysrq-trigger
> 
> Uh this is not great for testing in CI, we need something better. A
> drm-specific solution would be a debugfs file that triggers the panic
> dumping (which is the reason we should correctly unlock all locks we've
> taken too).

Yes, I already use a dumb file in debugfs to trigger the panic handler 
for development purpose. So it's a bit easier than crashing the machine.
I didn't add it in the series, because it's a bit rough, and the panic 
handler assumes there are no other thread running, and that it's the 
last frame the gpu will display before rebooting.

It gives some beautiful effect (if your driver uses double buffering, 
only one of them will be repainted with the panic screen, so it's 
flashing a bit afterward. It's also a good way to see if your driver 
uses the damage API correctly).

So I will add it in the next series.

> 
> Even better would be if the core code provides this infrastructure, so
> that ideally we could exercise running from an nmi context. For the drm
> testing the best we can probably do is disable local interrupts or maybe
> run from a timer that immediately fires.
> 
> I think adding that test infrastructure plus an igt that exercises should
> be done as part of merging the initial version. Otherwise there's just no
> way we can make sure that this code doesn't immediately bitrot like all
> the previous panic handlers.

ok, I will look into that. I didn't use igt yet, so I will see what I 
can do.
> 
> Cheers, Sima
> 
>>
>> v2:
>>   * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann)
>>   * Add the panic reason to the panic message (Nerdopolis)
>>   * Add an exclamation mark (Nerdopolis)
>>   
>> v3:
>>   * Rework the drawing functions, to write the pixels line by line and
>>   to use the drm conversion helper to support other formats.
>>   (Thomas Zimmermann)
>>   
>> v4:
>>   * Fully support all simpledrm formats using drm conversion helpers
>>   * Rename dpanic_* to drm_panic_*, and have more coherent function name.
>>     (Thomas Zimmermann)
>>   * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann)
>>   * Remove the default y to DRM_PANIC config option (Thomas Zimmermann)
>>   * Add foreground/background color config option
>>   * Fix the bottom lines not painted if the framebuffer height
>>     is not a multiple of the font height.
>>   * Automatically register the driver to drm_panic, if the function
>>     get_scanout_buffer() exists. (Thomas Zimmermann)
>>   * Add mgag200 support.
>>   
>> v5:
>>   * Change the drawing API, use drm_fb_blit_from_r1() to draw the font.
>>     (Thomas Zimmermann)
>>   * Also add drm_fb_fill() to fill area with background color.
>>   * Add draw_pixel_xy() API for drivers that can't provide a linear buffer.
>>   * Add a flush() callback for drivers that needs to synchronize the buffer.
>>   * Add a void *private field, so drivers can pass private data to
>>     draw_pixel_xy() and flush().
>>   * Add ast support.
>>   * Add experimental imx/ipuv3 support, to test on an ARM hw. (Maxime Ripard)
>>
>> v6:
>>   * Fix sparse and __le32 warnings
>>   * Drop the IMX/IPUV3 experiment, it was just to show that it works also on
>>     ARM devices.
>>
>> v7:
>>   * Add a check to see if the 4cc format is supported by drm_panic.
>>   * Add a drm/plane helper to loop over all visible primary buffer,
>>     simplifying the get_scanout_buffer implementations
>>   * Add a generic implementation for drivers that uses drm_fb_dma. (Maxime Ripard)
>>   * Add back the IMX/IPUV3 support, and use the generic implementation. (Maxime Ripard)
>>
>>
>> Best regards,
>>
>> Jocelyn Falempe (9):
>>    drm/format-helper: Add drm_fb_blit_from_r1 and drm_fb_fill
>>    drm/panic: Add a drm panic handler
>>    drm/plane: Add drm_for_each_primary_visible_plane macro
>>    drm/panic: Add drm_panic_is_format_supported()
>>    drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
>>    drm/simpledrm: Add drm_panic support
>>    drm/mgag200: Add drm_panic support
>>    drm/ast: Add drm_panic support
>>    drm/imx: Add drm_panic support
>>
>>   drivers/gpu/drm/Kconfig                  |  23 ++
>>   drivers/gpu/drm/Makefile                 |   1 +
>>   drivers/gpu/drm/ast/ast_drv.c            |  26 +-
>>   drivers/gpu/drm/drm_drv.c                |   8 +
>>   drivers/gpu/drm/drm_fb_dma_helper.c      |  55 +++
>>   drivers/gpu/drm/drm_format_helper.c      | 432 ++++++++++++++++++-----
>>   drivers/gpu/drm/drm_panic.c              | 382 ++++++++++++++++++++
>>   drivers/gpu/drm/imx/ipuv3/imx-drm-core.c |   2 +
>>   drivers/gpu/drm/mgag200/mgag200_drv.c    |  22 ++
>>   drivers/gpu/drm/tiny/simpledrm.c         |  15 +
>>   include/drm/drm_drv.h                    |  21 ++
>>   include/drm/drm_fb_dma_helper.h          |   4 +
>>   include/drm/drm_format_helper.h          |   9 +
>>   include/drm/drm_panic.h                  | 101 ++++++
>>   include/drm/drm_plane.h                  |  15 +
>>   15 files changed, 1033 insertions(+), 83 deletions(-)
>>   create mode 100644 drivers/gpu/drm/drm_panic.c
>>   create mode 100644 include/drm/drm_panic.h
>>
>>
>> base-commit: 50a3c772bd927dd409c484832ddd9f6bf00b7389
>> -- 
>> 2.43.0
>>
> 