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

[Thomas Zimmermann]

Hi Jocelyn,

thanks for moving this effort forward. It's much appreciated. I looked 
through the patches and tried the patchset on my test machine.

Am 09.08.23 um 21:17 schrieb Jocelyn Falempe:
> 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 only with simpledrm, using the drm_client API.
> This implementation with the drm client API, allocates new framebuffers, and looks a bit too complex to run in a panic handler.
> Maybe we should add an API to "steal" the current framebuffer instead, because in a panic handler user-space is already stopped.

Yes, that was also my first thought. I'd use an extra callback in struct 
drm_driver, like this:

struct drm_driver {
   int (*get_scanout_buffer)(/* return HW scanout */)
}

The scanout buffer would be described by kernel virtual address address, 
resolution, color format and scanline pitch. And that's what the panic 
handler uses.

Any driver implementing this interface would support the panic handler. 
If there's a concurrent display update, we'd have to synchronize.

> 
> To test it, make sure you're using the simpledrm driver, and trigger a panic:
> echo c > /proc/sysrq-trigger

The penguin was cute. :)

This only works if the display is already running. I had to start Gnome 
to set a display mode. Then let the panic handler take over the output.

But with simpledrm, we could even display a message without an output, 
as the framebuffer is always there.

> 
> There is one thing I don't know how to do, is to unregister the drm_panic when the graphic driver is unloaded.
> drm_client_register() says it will automatically unregister on driver unload. But then I don't know how to remove it from my linked list, and free the drm_client_dev struct.

Unregistering wouldn't be necessary with this proposed 
get_scanout_buffer. In the case of a panic, just remain silent if 
there's no driver that provides such a callback.

> 
> This is a first draft, so let me know what do you think about it.

One thing that will need serious work is the raw output. The current 
blitting for XRGB8888 is really just a quick-and-dirty hack.

I think we should try to reuse fbdev's blitting code, if possible. The 
fbdev core, helpers and console come with all the features we need. We 
really only need to make them work without the struct fb_info, which is 
a full fbdev device.

In struct fb_ops, there are callbacks for modifying the framebuffer. [1] 
They are used by fbcon foir drawing. But they operate on fb_info.

For a while I've been thinking about using something like a drawable to 
provide some abstractions:

struct drawable {
	/* store buffer parameters here */
	...

	struct drawable_funcs *funcs;
};

struct drawable_funcs {
	/* have function pointers similar to struct fb_ops */
	fill_rect()
	copy_area()
	image_blit()
};

We cannot rewrite all the existing fbdev drivers. To make this work with 
fbdev, we'd need adapter code that converts from drawable to fb_info and 
forwards to the existing helpers in fb_ops.

But for DRM's panic output, drawable_funcs would have to point to the 
scanout buffer and compatible callback funcs, for which we have 
implementations in fbdev.

We might be able to create console-like output that is independent from 
the fb_info. Hence, we could possible reuse a good chunk of the current 
panic output.

Best regards
Thomas

[1] https://elixir.bootlin.com/linux/v6.5.1/source/include/linux/fb.h#L273

> 
> Best regards,
> 
> 
> 
> 
> Jocelyn Falempe (2):
>    drm/panic: Add a drm panic handler
>    drm/simpledrm: Add drm_panic support
> 
>   drivers/gpu/drm/Kconfig          |  11 ++
>   drivers/gpu/drm/Makefile         |   1 +
>   drivers/gpu/drm/drm_drv.c        |   3 +
>   drivers/gpu/drm/drm_panic.c      | 286 +++++++++++++++++++++++++++++++
>   drivers/gpu/drm/tiny/simpledrm.c |   2 +
>   include/drm/drm_panic.h          |  26 +++
>   6 files changed, 329 insertions(+)
>   create mode 100644 drivers/gpu/drm/drm_panic.c
>   create mode 100644 include/drm/drm_panic.h
> 
> 
> base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1

-- 
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230904/150a1d0a/attachment.sig>