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

[Javier Martinez Canillas <javierm@redhat.com>]

Daniel Vetter <daniel@ffwll.ch> writes:

Hello Sima,

Thanks for your comment and explanations.

> On Fri, Jun 21, 2024 at 05:42:53PM +0200, Javier Martinez Canillas wrote:
>> Javier Martinez Canillas <javierm@redhat.com> writes:
>> 
>> > Jocelyn Falempe <jfalempe@redhat.com> writes:
>> >
>> > Hello Jocelyn, thanks for your feedback!
>> >
>> >> 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.
>> >>
>> >
>> > Indeed, that's why I did it. Just to see if it could work :)
>> >
>> > [...]
>> >
>> >>> +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.
>> >
>> > That's a good point and I something haven't considered...
>> >
>> >> 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.
>> >>
>> >
>> > Yeah, I would prefer to not change the driver for normal operation.
>> >
>> 
>> Another option, that I believe makes more sense, is to just disable the
>> regmap locking (using struct regmap_config.disable_locking field [0]).
>> 
>> Since this regmap is not shared with other drivers and so any concurrent
>> access should already be prevented by the DRM core locking scheme.
>> 
>> Is my understanding correct?
>
> Quick irc discussion summary: Since these are panels that sit on i2c/spi
> buses, you need to put the raw spinlock panic locking into these
> subsystems. Which is going to be extreme amounts of fun, becuase:
>
> - You need to protect innermost register access with a raw spinlock, but
>   enough so that every access is still consistent.
>
> - You need separate panic paths which avoid all the existing subsystem
>   locking (i2c/spi have userspace apis, so they need mutexes) and only
>   rely on the caller having done the raw spinlock trylocking.
>
> - Bonus points: Who even owns that raw spinlock ...
>
> I'm afraid, this is going to be a tough nut to crack :-/
>

Yeah, not worth the effort then. I'll just drop this patch.

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat