Re: [PATCH v8 5/8] drm/simpledrm: Add drm_panic support

From: Daniel Vetter <daniel@ffwll.ch>
To: Jocelyn Falempe <jfalempe@redhat.com>
Cc: dri-devel@lists.freedesktop.org, tzimmermann@suse.de,
	airlied@redhat.com, maarten.lankhorst@linux.intel.com,
	mripard@kernel.org, daniel@ffwll.ch, javierm@redhat.com,
	bluescreen_avenger@verizon.net, noralf@tronnes.org,
	gpiccoli@igalia.com
Subject: Re: [PATCH v8 5/8] drm/simpledrm: Add drm_panic support
Date: Thu, 29 Feb 2024 12:17:46 +0100	[thread overview]
Message-ID: <ZeBn2sJmmQzY3Nxu@phenom.ffwll.local> (raw)
In-Reply-To: <20240227100459.194478-6-jfalempe@redhat.com>

On Tue, Feb 27, 2024 at 11:04:16AM +0100, Jocelyn Falempe wrote:
> Add support for the drm_panic module, which displays a user-friendly
> message to the screen when a kernel panic occurs.
> 
> v8:
>  * Replace get_scanout_buffer() with drm_panic_set_buffer()
>    (Thomas Zimmermann)
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
> index 7ce1c4617675..a2190995354a 100644
> --- a/drivers/gpu/drm/tiny/simpledrm.c
> +++ b/drivers/gpu/drm/tiny/simpledrm.c
> @@ -25,6 +25,7 @@
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_managed.h>
>  #include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_panic.h>
>  #include <drm/drm_probe_helper.h>
>  
>  #define DRIVER_NAME	"simpledrm"
> @@ -735,6 +736,20 @@ static const struct drm_connector_funcs simpledrm_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> +static void simpledrm_init_panic_buffer(struct drm_plane *plane)
> +{
> +	struct simpledrm_device *sdev = simpledrm_device_of_dev(plane->dev);
> +	struct drm_framebuffer fb;
> +
> +	/* Fake framebuffer struct for drm_panic_set_buffer */
> +	fb.width = sdev->mode.hdisplay;
> +	fb.height = sdev->mode.vdisplay;
> +	fb.format = sdev->format;
> +	fb.pitches[0] = sdev->pitch;
> +
> +	drm_panic_set_buffer(plane->panic_scanout, &fb, &sdev->screen_base);
> +}
> +
>  static const struct drm_mode_config_funcs simpledrm_mode_config_funcs = {
>  	.fb_create = drm_gem_fb_create_with_dirty,
>  	.atomic_check = drm_atomic_helper_check,
> @@ -945,6 +960,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  		return ERR_PTR(ret);
>  	drm_plane_helper_add(primary_plane, &simpledrm_primary_plane_helper_funcs);
>  	drm_plane_enable_fb_damage_clips(primary_plane);
> +	drm_panic_register(primary_plane);

Just a quick comment on this:

This does not work, the driver is not ready to handle panic calls at this
stage. Instead we need to automatically register all planes that support
panic handling in drm_dev_register(), and we need to remove them all again
in drm_dev_unregister(). Outside of these functions it is not safe to call
into driver code.

At that point it might be simpler to only register one panic notifier per
drm_device, and push the loop into the panic handler again.

Cheers, Sima

> +	simpledrm_init_panic_buffer(primary_plane);
>  
>  	/* CRTC */
>  
> -- 
> 2.43.0
> 

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