Re: [PATCH] drm/ssd130x: Add drm_panic support

[Jocelyn Falempe <jfalempe@redhat.com>]

On 21/06/2024 00:22, Javier Martinez Canillas wrote:
> Add support for the drm_panic infrastructure, which allows to display
> a user friendly message on the screen when a Linux kernel panic occurs.
> 
> The display controller doesn't scanout the framebuffer, but instead the
> pixels are sent to the device using a transport bus. For this reason, a
> .panic_flush handler is needed to flush the panic image to the display.

Thanks for this patch, that's really cool that drm_panic can work on 
this device too.

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/solomon/ssd130x.c | 64 +++++++++++++++++++++++++++++++
>   1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 6f51bcf774e2..0bac97bd39b9 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -32,6 +32,7 @@
>   #include <drm/drm_managed.h>
>   #include <drm/drm_modes.h>
>   #include <drm/drm_rect.h>
> +#include <drm/drm_panic.h>
>   #include <drm/drm_probe_helper.h>
>   
>   #include "ssd130x.h"
> @@ -1386,6 +1387,63 @@ static void ssd133x_primary_plane_atomic_disable(struct drm_plane *plane,
>   	drm_dev_exit(idx);
>   }
>   
> +static int ssd130x_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane,
> +							   struct drm_scanout_buffer *sb)
> +{
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_shadow_plane_state *shadow_plane_state;
> +
> +	if (!plane_state || !plane_state->fb || !plane_state->crtc)
> +		return -EINVAL;
> +
> +	shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +
> +	sb->format = plane->state->fb->format;
> +	sb->width = plane->state->fb->width;
> +	sb->height = plane->state->fb->height;
> +	sb->pitch[0] = plane->state->fb->pitches[0];
> +	sb->map[0] = shadow_plane_state->data[0];
> +
> +	return 0;
> +}
> +
> +static void ssd130x_primary_plane_helper_panic_flush(struct drm_plane *plane)
> +{
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct ssd130x_plane_state *ssd130x_plane_state = to_ssd130x_plane_state(plane_state);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc *crtc = plane_state->crtc;
> +	struct ssd130x_crtc_state *ssd130x_crtc_state = to_ssd130x_crtc_state(crtc->state);
> +
> +	ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &plane_state->dst,
> +			     ssd130x_plane_state->buffer, ssd130x_crtc_state->data_array,
> +			     &shadow_plane_state->fmtcnv_state);

ssd130x_fb_blit_rect() will call regmap->write(), which involve mutex 
and might sleep. And if the mutex is taken when the panic occurs, it 
might deadlock the panic handling.
One solution would be to configure the regmap with config->fast_io and 
config->use_raw_spinlock, and check that the lock is available with 
try_lock(map->raw_spin_lock)
But that means it will waste cpu cycle with busy waiting for normal 
operation, which is not good.

So for this particular device, I think it's ok, because it's unlikely 
you'll run kdump or other kernel panic handlers.
But I would like to know what others think about it, and if it's 
acceptable or not.

-- 

Jocelyn



> +}
> +
> +static void ssd132x_primary_plane_helper_panic_flush(struct drm_plane *plane)
> +{
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct ssd130x_plane_state *ssd130x_plane_state = to_ssd130x_plane_state(plane_state);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc *crtc = plane_state->crtc;
> +	struct ssd130x_crtc_state *ssd130x_crtc_state = to_ssd130x_crtc_state(crtc->state);
> +
> +	ssd132x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &plane_state->dst,
> +			     ssd130x_plane_state->buffer, ssd130x_crtc_state->data_array,
> +			     &shadow_plane_state->fmtcnv_state);
> +}
> +
> +static void ssd133x_primary_plane_helper_panic_flush(struct drm_plane *plane)
> +{
> +	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_crtc *crtc = plane_state->crtc;
> +	struct ssd130x_crtc_state *ssd130x_crtc_state = to_ssd130x_crtc_state(crtc->state);
> +
> +	ssd133x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &plane_state->dst,
> +			     ssd130x_crtc_state->data_array, &shadow_plane_state->fmtcnv_state);
> +}
> +
>   /* Called during init to allocate the plane's atomic state. */
>   static void ssd130x_primary_plane_reset(struct drm_plane *plane)
>   {
> @@ -1442,18 +1500,24 @@ static const struct drm_plane_helper_funcs ssd130x_primary_plane_helper_funcs[]
>   		.atomic_check = ssd130x_primary_plane_atomic_check,
>   		.atomic_update = ssd130x_primary_plane_atomic_update,
>   		.atomic_disable = ssd130x_primary_plane_atomic_disable,
> +		.get_scanout_buffer = ssd130x_primary_plane_helper_get_scanout_buffer,
> +		.panic_flush = ssd130x_primary_plane_helper_panic_flush,
>   	},
>   	[SSD132X_FAMILY] = {
>   		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>   		.atomic_check = ssd132x_primary_plane_atomic_check,
>   		.atomic_update = ssd132x_primary_plane_atomic_update,
>   		.atomic_disable = ssd132x_primary_plane_atomic_disable,
> +		.get_scanout_buffer = ssd130x_primary_plane_helper_get_scanout_buffer,
> +		.panic_flush = ssd132x_primary_plane_helper_panic_flush,
>   	},
>   	[SSD133X_FAMILY] = {
>   		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>   		.atomic_check = ssd133x_primary_plane_atomic_check,
>   		.atomic_update = ssd133x_primary_plane_atomic_update,
>   		.atomic_disable = ssd133x_primary_plane_atomic_disable,
> +		.get_scanout_buffer = ssd130x_primary_plane_helper_get_scanout_buffer,
> +		.panic_flush = ssd133x_primary_plane_helper_panic_flush,
>   	}
>   };
>