| Message ID | 20240620222222.155933-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | drm/ssd130x: Add drm_panic support | expand |
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 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. > 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. > I don't know either. I guess that after a panic everything is best effort anyways so it may be acceptable? But let's see what others think about it. > -- > > Jocelyn
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? [0]: https://elixir.bootlin.com/linux/v6.10-rc1/source/include/linux/regmap.h#L326
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 :-/ Cheers, Sima
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 >
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); +} + +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, } };
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. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/gpu/drm/solomon/ssd130x.c | 64 +++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)