Re: [PATCH] printk: Add a short description string to kmsg_dump()
On 26/06/2024 10:00, Petr Mladek wrote: On Tue 2024-06-25 14:39:29, Jocelyn Falempe wrote: kmsg_dump doesn't forward the panic reason string to the kmsg_dumper callback. This patch adds a new parameter "const char *desc" to the kmsg_dumper dump() callback, and update all drivers that are using it. To avoid updating all kmsg_dump() call, it adds a kmsg_dump_desc() function and a macro for backward compatibility. I've written this for drm_panic, but it can be useful for other kmsg_dumper. It allows to see the panic reason, like "sysrq triggered crash" or "VFS: Unable to mount root fs on " on the drm panic screen. Signed-off-by: Jocelyn Falempe --- arch/powerpc/kernel/nvram_64.c | 3 ++- arch/powerpc/platforms/powernv/opal-kmsg.c | 3 ++- drivers/gpu/drm/drm_panic.c| 3 ++- drivers/hv/hv_common.c | 3 ++- drivers/mtd/mtdoops.c | 3 ++- fs/pstore/platform.c | 3 ++- include/linux/kmsg_dump.h | 13 ++--- kernel/panic.c | 2 +- kernel/printk/printk.c | 8 +--- 9 files changed, 28 insertions(+), 13 deletions(-) The parameter is added into all dumpers. I guess that it would be used only drm_panic() because it is graphics and might be "fancy". The others simply dump the log buffer and the reason is in the dumped log as well. Ok, I also tried to retrieve the reason from the dumped log, but that's really fragile. Anyway, the passed buffer is static. Alternative solution would be to make it global and export it like, for example, panic_cpu. It's not a static buffer, because the string is generated at runtime. eg: https://elixir.bootlin.com/linux/latest/source/arch/arm/mm/init.c#L158 So it will be hard to avoid race conditions. Best Regards, Petr
Re: [PATCH 2/2] drm/panic: Restrict graphical logo handling to built-in
On 26/06/2024 10:41, Geert Uytterhoeven wrote: When CONFIG_DRM_PANIC=y, but CONFIG_DRM=m: ld: drivers/gpu/drm/drm_panic.o: in function `drm_panic_setup_logo': drivers/gpu/drm/drm_panic.c:99: multiple definition of `init_module'; drivers/gpu/drm/drm_drv.o:drivers/gpu/drm/drm_drv.c:1079: first defined here Fix this by restricting the graphical logo handling and its device_initcall() to the built-in case. Logos are freed during late kernel initialization, so they are no longer available at module load time anyway. Thanks a lot for this fix. Reviewed-by: Jocelyn Falempe Fixes: 294bbd1f2697ff28 ("drm/panic: Add support for drawing a monochrome graphical logo") Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202406261341.gysblpn1-...@intel.com/ Signed-off-by: Geert Uytterhoeven --- drivers/gpu/drm/drm_panic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 67f78b5a76b61e3d..948aed00595eb6dd 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -91,7 +91,7 @@ static const struct drm_panic_line logo_ascii[] = { PANIC_LINE(" \\___)=(___/"), }; -#ifdef CONFIG_LOGO +#if defined(CONFIG_LOGO) && !defined(MODULE) static const struct linux_logo *logo_mono; static int drm_panic_setup_logo(void)
Re: [PATCH 1/2] drm/panic: Do not select DRM_KMS_HELPER
On 26/06/2024 10:41, Geert Uytterhoeven wrote: DRM core code cannot call into DRM helper code, as this would lead to circular references in the modular case. Hence drop the selection of DRM_KMS_HELPER. It was unused anyway, as v10 switched from using the DRM format helpers to its own color format conversion, cfr. commit 9544309775c334c9 ("drm/panic: Add support for color format conversion")). Thanks a lot for this cleanup, Reviewed-by: Jocelyn Falempe Remove the unneeded include of . Fixes: bf9fb17c6672868d ("drm/panic: Add a drm panic handler") Signed-off-by: Geert Uytterhoeven --- drivers/gpu/drm/Kconfig | 1 - drivers/gpu/drm/drm_panic.c | 1 - 2 files changed, 2 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index b903a2c0b5e8f95c..ce9bf2b6e9d332d4 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -108,7 +108,6 @@ config DRM_KMS_HELPER config DRM_PANIC bool "Display a user-friendly message when a kernel panic occurs" depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) - select DRM_KMS_HELPER select FONT_SUPPORT help Enable a drm panic handler, which will display a user-friendly message diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 8d2eded1fd19ff6c..67f78b5a76b61e3d 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -20,7 +20,6 @@ #include #include -#include #include #include #include
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On 25/06/2024 15:18, Thomas Zimmermann wrote: The function drm_simple_encoder_init() is a trivial helper and deprecated. Replace it with the regular call to drm_encoder_init(). Resolves the dependency on drm_simple_kms_helper.h. No functional changes. Thanks for your patch, it looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_mode.c | 45 ++ 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 6695af70768f..2fd9c78eab73 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -45,7 +45,6 @@ #include #include #include -#include #include "ast_ddc.h" #include "ast_drv.h" @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev) return 0; } +/* + * VGA Encoder + */ + +static const struct drm_encoder_funcs ast_vga_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * VGA Connector */ @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast) struct drm_connector *connector = >output.vga.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC); + ret = drm_encoder_init(dev, encoder, _vga_encoder_funcs, + DRM_MODE_ENCODER_DAC, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc); @@ -1427,6 +1435,14 @@ static int ast_vga_output_init(struct ast_device *ast) return 0; } +/* + * SIL164 Encoder + */ + +static const struct drm_encoder_funcs ast_sil164_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * SIL164 Connector */ @@ -1480,7 +1496,8 @@ static int ast_sil164_output_init(struct ast_device *ast) struct drm_connector *connector = >output.sil164.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS); + ret = drm_encoder_init(dev, encoder, _sil164_encoder_funcs, + DRM_MODE_ENCODER_TMDS, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc); @@ -1496,6 +1513,14 @@ static int ast_sil164_output_init(struct ast_device *ast) return 0; } +/* + * DP501 Encoder + */ + +static const struct drm_encoder_funcs ast_dp501_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * DP501 Connector */ @@ -1578,7 +1603,8 @@ static int ast_dp501_output_init(struct ast_device *ast) struct drm_connector *connector = >output.dp501.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS); + ret = drm_encoder_init(dev, encoder, _dp501_encoder_funcs, + DRM_MODE_ENCODER_TMDS, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc); @@ -1594,6 +1620,14 @@ static int ast_dp501_output_init(struct ast_device *ast) return 0; } +/* + * ASPEED Display-Port Encoder + */ + +static const struct drm_encoder_funcs ast_astdp_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * ASPEED Display-Port Connector */ @@ -1688,7 +1722,8 @@ static int ast_astdp_output_init(struct ast_device *ast) struct drm_connector *connector = >output.astdp.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS); + ret = drm_encoder_init(dev, encoder, _astdp_encoder_funcs, + DRM_MODE_ENCODER_TMDS, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc);
Re: [PATCH] drm/ast: Inline drm_simple_encoder_init()
On 25/06/2024 15:18, Thomas Zimmermann wrote: The function drm_simple_encoder_init() is a trivial helper and deprecated. Replace it with the regular call to drm_encoder_init(). Resolves the dependency on drm_simple_kms_helper.h. No functional changes. Do you think it's possible to add a default to drm_encoder_init() to avoid having to declare the same struct for each encoder ? something like: drm_encoder_init(...) { if (!funcs) funcs = _encoder_default_funcs; So you can call it like this to get the default funcs: drm_encoder_init(dev, encoder, NULL, DRM_MODE_ENCODER_DAC, NULL); I don't see this pattern in other drm functions, so it might not fit the current coding style. Best regards, -- Jocelyn Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_mode.c | 45 ++ 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 6695af70768f..2fd9c78eab73 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -45,7 +45,6 @@ #include #include #include -#include #include "ast_ddc.h" #include "ast_drv.h" @@ -1358,6 +1357,14 @@ static int ast_crtc_init(struct drm_device *dev) return 0; } +/* + * VGA Encoder + */ + +static const struct drm_encoder_funcs ast_vga_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * VGA Connector */ @@ -1411,7 +1418,8 @@ static int ast_vga_output_init(struct ast_device *ast) struct drm_connector *connector = >output.vga.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC); + ret = drm_encoder_init(dev, encoder, _vga_encoder_funcs, + DRM_MODE_ENCODER_DAC, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc); @@ -1427,6 +1435,14 @@ static int ast_vga_output_init(struct ast_device *ast) return 0; } +/* + * SIL164 Encoder + */ + +static const struct drm_encoder_funcs ast_sil164_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * SIL164 Connector */ @@ -1480,7 +1496,8 @@ static int ast_sil164_output_init(struct ast_device *ast) struct drm_connector *connector = >output.sil164.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS); + ret = drm_encoder_init(dev, encoder, _sil164_encoder_funcs, + DRM_MODE_ENCODER_TMDS, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc); @@ -1496,6 +1513,14 @@ static int ast_sil164_output_init(struct ast_device *ast) return 0; } +/* + * DP501 Encoder + */ + +static const struct drm_encoder_funcs ast_dp501_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * DP501 Connector */ @@ -1578,7 +1603,8 @@ static int ast_dp501_output_init(struct ast_device *ast) struct drm_connector *connector = >output.dp501.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS); + ret = drm_encoder_init(dev, encoder, _dp501_encoder_funcs, + DRM_MODE_ENCODER_TMDS, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc); @@ -1594,6 +1620,14 @@ static int ast_dp501_output_init(struct ast_device *ast) return 0; } +/* + * ASPEED Display-Port Encoder + */ + +static const struct drm_encoder_funcs ast_astdp_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + /* * ASPEED Display-Port Connector */ @@ -1688,7 +1722,8 @@ static int ast_astdp_output_init(struct ast_device *ast) struct drm_connector *connector = >output.astdp.connector; int ret; - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS); + ret = drm_encoder_init(dev, encoder, _astdp_encoder_funcs, + DRM_MODE_ENCODER_TMDS, NULL); if (ret) return ret; encoder->possible_crtcs = drm_crtc_mask(crtc);
[PATCH] printk: Add a short description string to kmsg_dump()
kmsg_dump doesn't forward the panic reason string to the kmsg_dumper callback. This patch adds a new parameter "const char *desc" to the kmsg_dumper dump() callback, and update all drivers that are using it. To avoid updating all kmsg_dump() call, it adds a kmsg_dump_desc() function and a macro for backward compatibility. I've written this for drm_panic, but it can be useful for other kmsg_dumper. It allows to see the panic reason, like "sysrq triggered crash" or "VFS: Unable to mount root fs on " on the drm panic screen. Signed-off-by: Jocelyn Falempe --- arch/powerpc/kernel/nvram_64.c | 3 ++- arch/powerpc/platforms/powernv/opal-kmsg.c | 3 ++- drivers/gpu/drm/drm_panic.c| 3 ++- drivers/hv/hv_common.c | 3 ++- drivers/mtd/mtdoops.c | 3 ++- fs/pstore/platform.c | 3 ++- include/linux/kmsg_dump.h | 13 ++--- kernel/panic.c | 2 +- kernel/printk/printk.c | 8 +--- 9 files changed, 28 insertions(+), 13 deletions(-) diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c index e385d3164648c..6b3a80d8cfa64 100644 --- a/arch/powerpc/kernel/nvram_64.c +++ b/arch/powerpc/kernel/nvram_64.c @@ -643,7 +643,8 @@ void __init nvram_init_oops_partition(int rtas_partition_exists) * partition. If that's too much, go back and capture uncompressed text. */ static void oops_to_nvram(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) + enum kmsg_dump_reason reason, + const char *desc) { struct oops_log_info *oops_hdr = (struct oops_log_info *)oops_buf; static unsigned int oops_count = 0; diff --git a/arch/powerpc/platforms/powernv/opal-kmsg.c b/arch/powerpc/platforms/powernv/opal-kmsg.c index 6c3bc4b4da983..49b60de6feb04 100644 --- a/arch/powerpc/platforms/powernv/opal-kmsg.c +++ b/arch/powerpc/platforms/powernv/opal-kmsg.c @@ -20,7 +20,8 @@ * message, it just ensures that OPAL completely flushes the console buffer. */ static void kmsg_dump_opal_console_flush(struct kmsg_dumper *dumper, -enum kmsg_dump_reason reason) +enum kmsg_dump_reason reason, +const char *desc) { /* * Outside of a panic context the pollers will continue to run, diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 293d4dcbc80da..88e9359fe6d78 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -604,7 +604,8 @@ static struct drm_plane *to_drm_plane(struct kmsg_dumper *kd) return container_of(kd, struct drm_plane, kmsg_panic); } -static void drm_panic(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason) +static void drm_panic(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason, + const char *desc) { struct drm_plane *plane = to_drm_plane(dumper); diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c index 9c452bfbd5719..b0786ee9c94e3 100644 --- a/drivers/hv/hv_common.c +++ b/drivers/hv/hv_common.c @@ -207,7 +207,8 @@ static int hv_die_panic_notify_crash(struct notifier_block *self, * buffer and call into Hyper-V to transfer the data. */ static void hv_kmsg_dump(struct kmsg_dumper *dumper, -enum kmsg_dump_reason reason) +enum kmsg_dump_reason reason, +const char *desc) { struct kmsg_dump_iter iter; size_t bytes_written; diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c index 2f11585b5613e..c618999a96832 100644 --- a/drivers/mtd/mtdoops.c +++ b/drivers/mtd/mtdoops.c @@ -298,7 +298,8 @@ static void find_next_position(struct mtdoops_context *cxt) } static void mtdoops_do_dump(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) + enum kmsg_dump_reason reason, + const char *desc) { struct mtdoops_context *cxt = container_of(dumper, struct mtdoops_context, dump); diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 3497ede88aa01..a6ed5d56021ef 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -275,7 +275,8 @@ void pstore_record_init(struct pstore_record *record, * end of the buffer. */ static void pstore_dump(struct kmsg_dumper *dumper, - enum kmsg_dump_reason reason) + enum kmsg_dump_reason reason, + const char *desc) { struct kmsg_dump_iter iter; unsigned long total = 0; diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h index 906521c2329ca..a8f8a6204542d 100644 --- a/include/linux/kmsg_dump.h +++ b/include/linux/kmsg_dump.h @@ -49,13 +
Re: [PATCH v2 0/7] drm/panic: Fixes and graphical logo
On 21/06/2024 10:55, Jocelyn Falempe wrote: Hi, I want to push at least the first patch that is an important fix. But if there are no objections, I can push the whole series except patch 5 "drm/panic: Convert to drm_fb_clip_offset()" which causes some build issue. I just pushed them to drm-misc-next. Thanks all. Best regards,
Re: [PATCH] drm/ssd130x: Add drm_panic support
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 --- 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 #include #include +#include #include #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, _plane_state->data[0], _state->dst, +ssd130x_plane_state->buffer, ssd130x_crtc_state->data_array, +_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, _plane_state->data[0], _state->dst, +ssd130x_plane_state->buffer, ssd130x_crtc_state->data_array, +_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, _plane_state->data[0], _state->dst, +ssd130x_crtc_state->data_array, _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, +
Re: [PATCH v2 0/7] drm/panic: Fixes and graphical logo
Hi, I want to push at least the first patch that is an important fix. But if there are no objections, I can push the whole series except patch 5 "drm/panic: Convert to drm_fb_clip_offset()" which causes some build issue. Best regards, -- Jocelyn On 13/06/2024 21:17, Geert Uytterhoeven wrote: Hi all, If drm/panic is enabled, a user-friendly message is shown on screen when a kernel panic occurs, together with an ASCII art penguin logo. Of course we can do better ;-) Hence this patch series extends drm/panic to draw the monochrome graphical boot logo, when available, preceded by the customary fixes. Changes compared to v1: - Rebase against today's drm-misc-next, where drm_panic is broken on all current drivers due to an uninitialized pointer dereference. Presumably this was only tested with an out-of-tree driver change? - New fixes [1/7], [3/7], and [4/7], - New cleanup [5/7], - Inline trivial draw_logo_mono(). This has been tested with rcar-du. Thanks for your comments! Geert Uytterhoeven (7): drm/panic: Fix uninitialized drm_scanout_buffer.set_pixel() crash drm/panic: Fix off-by-one logo size checks lib/fonts: Fix visiblity of SUN12x22 and TER16x32 if DRM_PANIC drm/panic: Spelling s/formater/formatter/ drm/panic: Convert to drm_fb_clip_offset() drm/panic: Rename logo to logo_ascii drm/panic: Add support for drawing a monochrome graphical logo drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/drm_panic.c | 74 +++-- drivers/video/logo/Kconfig | 2 + lib/fonts/Kconfig | 6 ++- 4 files changed, 70 insertions(+), 14 deletions(-)
Re: [PATCH] drm/panic: depends on !VT_CONSOLE
On 17/06/2024 11:27, Javier Martinez Canillas wrote: Jocelyn Falempe writes: On 17/06/2024 10:25, Geert Uytterhoeven wrote: Hi Jocelyn, On Mon, Jun 17, 2024 at 10:16 AM Jocelyn Falempe wrote: On 16/06/2024 14:43, Javier Martinez Canillas wrote: Jocelyn Falempe writes: The race condition between fbcon and drm_panic can only occurs if VT_CONSOLE is set. So update drm_panic dependency accordingly. This will make it easier for Linux distributions to enable drm_panic by disabling VT_CONSOLE, and keeping fbcon terminal. The only drawback is that fbcon won't display the boot kmsg, so it should rely on userspace to do that. At least plymouth already handle this case with https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 Suggested-by: Daniel Vetter Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index a9df94291622..f5c989aed7e9 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -107,7 +107,7 @@ config DRM_KMS_HELPER config DRM_PANIC bool "Display a user-friendly message when a kernel panic occurs" -depends on DRM && !FRAMEBUFFER_CONSOLE +depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) I thought the idea was to only make it depend on !VT_CONSOLE, so that distros could also enable fbcon / VT but prevent the race condition to happen due the VT not being a system console for the kernel to print messages ? Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and drm_panic can be enabled safely. I don't know if that really matters, and if VT_CONSOLE has any usage apart from fbcon. It is used for any kind of virtual terminal, so also for vgacon. Ah thanks, but I think vgacon is safe to use with drm_panic. The problem with fbcon, is that it can also uses the fbdev emulation from the drm driver, and in this case, drm_panic will write to the same framebuffer as fbcon during a panic, leading to corrupted output. This can't occur with other graphical console, so !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) is probably good. Yeah, I agre that !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) as you have in your patch is what makes sense. !VT_CONSOLE alone as I argued in my first email isn't correct since as you pointed out, it shouldn't affect other consoles besides fbcon. So please feel free to also add: Reviewed-by: Javier Martinez Canillas Thanks all for your comments and reviews. I just pushed it to drm-misc-next. -- Jocelyn
Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo
On 17/06/2024 14:49, Geert Uytterhoeven wrote: Hi Jocelyn, On Mon, Jun 17, 2024 at 11:59 AM Jocelyn Falempe wrote: On 13/06/2024 21:18, Geert Uytterhoeven wrote: Re-use the existing support for boot-up logos to draw a monochrome graphical logo in the DRM panic handler. When no suitable graphical logo is available, the code falls back to the ASCII art penguin logo. Note that all graphical boot-up logos are freed during late kernel initialization, hence a copy must be made for later use. Signed-off-by: Geert Uytterhoeven --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c PANIC_LINE(" \\___)=(___/"), }; +#ifdef CONFIG_LOGO +static const struct linux_logo *logo_mono; + +static int drm_panic_setup_logo(void) +{ + const struct linux_logo *logo = fb_find_logo(1); + const unsigned char *logo_data; + struct linux_logo *logo_dup; + + if (!logo || logo->type != LINUX_LOGO_MONO) + return 0; + + /* The logo is __init, so we must make a copy for later use */ + logo_data = kmemdup(logo->data, + size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height), + GFP_KERNEL); + if (!logo_data) + return -ENOMEM; + + logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL); + if (!logo_dup) { + kfree(logo_data); + return -ENOMEM; + } + + logo_dup->data = logo_data; + logo_mono = logo_dup; + + return 0; +} + +device_initcall(drm_panic_setup_logo); +#else +#define logo_mono((const struct linux_logo *)NULL) +#endif I didn't notice that the first time, but the core drm can be built as a module. That means this will leak memory each time the module is removed. While I hadn't considered a modular DRM core, there is no memory leak: after the logos are freed (from late_initcall_sync()), fb_find_logo() returns NULL. This does mean there won't be a graphical logo on panic, though. Yes, you're right, thanks for the precision. But to solve the circular dependency between drm_kms_helper and drm_panic, one solution is to depends on drm core to be built-in. In this case there won't be a leak. So depending on how we solve the circular dependency, it can be acceptable. So far there is no reason to select DRM_KMS_HELPER, right? The current version doesn't need DRM_KMS_HELPER. But for example, it uses struct drm_rect, and a few functions (drm_rect_width()) that are defined in .h, but other drm_rect_* functions are defined in DRM_KMS_HELPER. And as you pointed out in your patch, it duplicates the drm_fb_clip_offset(). So I'm not sure if it can avoid the dependency on DRM_KMS_HELPER in the long term. Gr{oetje,eeting}s, Geert
Re: [PATCH] drm/tidss: Add drm_panic support
On 15/06/2024 10:53, Javier Martinez Canillas wrote: Add support for the drm_panic module, which displays a pretty user friendly message on the screen when a Linux kernel panic occurs. Thanks for enabling drm panic on another hardware. It looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Javier Martinez Canillas --- Tested on an AM625 BeaglePlay board by triggering a panic using the `echo c > /proc/sysrq-trigger` command. drivers/gpu/drm/tidss/tidss_plane.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c index 68fed531f6a7..a5d86822c9e3 100644 --- a/drivers/gpu/drm/tidss/tidss_plane.c +++ b/drivers/gpu/drm/tidss/tidss_plane.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -166,6 +167,14 @@ static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = { .atomic_disable = tidss_plane_atomic_disable, }; +static const struct drm_plane_helper_funcs tidss_primary_plane_helper_funcs = { + .atomic_check = tidss_plane_atomic_check, + .atomic_update = tidss_plane_atomic_update, + .atomic_enable = tidss_plane_atomic_enable, + .atomic_disable = tidss_plane_atomic_disable, + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer, +}; + static const struct drm_plane_funcs tidss_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -211,7 +220,10 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss, if (ret < 0) goto err; - drm_plane_helper_add(>plane, _plane_helper_funcs); + if (type == DRM_PLANE_TYPE_PRIMARY) + drm_plane_helper_add(>plane, _primary_plane_helper_funcs); + else + drm_plane_helper_add(>plane, _plane_helper_funcs); drm_plane_create_zpos_property(>plane, tidss->num_planes, 0, num_planes - 1);
Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo
On 13/06/2024 21:18, Geert Uytterhoeven wrote: Re-use the existing support for boot-up logos to draw a monochrome graphical logo in the DRM panic handler. When no suitable graphical logo is available, the code falls back to the ASCII art penguin logo. Note that all graphical boot-up logos are freed during late kernel initialization, hence a copy must be made for later use. Signed-off-by: Geert Uytterhoeven --- v2: - Rebased, - Inline trivial draw_logo_mono(). --- drivers/gpu/drm/drm_panic.c | 65 + drivers/video/logo/Kconfig | 2 ++ 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index f7e22b69bb25d3be..af30f243b2802ad7 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -7,11 +7,15 @@ */ #include +#include #include #include #include +#include #include +#include #include +#include #include #include @@ -88,6 +92,42 @@ static const struct drm_panic_line logo_ascii[] = { PANIC_LINE(" \\___)=(___/"), }; +#ifdef CONFIG_LOGO +static const struct linux_logo *logo_mono; + +static int drm_panic_setup_logo(void) +{ + const struct linux_logo *logo = fb_find_logo(1); + const unsigned char *logo_data; + struct linux_logo *logo_dup; + + if (!logo || logo->type != LINUX_LOGO_MONO) + return 0; + + /* The logo is __init, so we must make a copy for later use */ + logo_data = kmemdup(logo->data, + size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height), + GFP_KERNEL); + if (!logo_data) + return -ENOMEM; + + logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL); + if (!logo_dup) { + kfree(logo_data); + return -ENOMEM; + } + + logo_dup->data = logo_data; + logo_mono = logo_dup; + + return 0; +} + +device_initcall(drm_panic_setup_logo); +#else +#define logo_mono ((const struct linux_logo *)NULL) +#endif I didn't notice that the first time, but the core drm can be built as a module. That means this will leak memory each time the module is removed. But to solve the circular dependency between drm_kms_helper and drm_panic, one solution is to depends on drm core to be built-in. In this case there won't be a leak. So depending on how we solve the circular dependency, it can be acceptable. -- Jocelyn
Re: [PATCH] drm/panic: depends on !VT_CONSOLE
On 17/06/2024 10:25, Geert Uytterhoeven wrote: Hi Jocelyn, On Mon, Jun 17, 2024 at 10:16 AM Jocelyn Falempe wrote: On 16/06/2024 14:43, Javier Martinez Canillas wrote: Jocelyn Falempe writes: The race condition between fbcon and drm_panic can only occurs if VT_CONSOLE is set. So update drm_panic dependency accordingly. This will make it easier for Linux distributions to enable drm_panic by disabling VT_CONSOLE, and keeping fbcon terminal. The only drawback is that fbcon won't display the boot kmsg, so it should rely on userspace to do that. At least plymouth already handle this case with https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 Suggested-by: Daniel Vetter Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index a9df94291622..f5c989aed7e9 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -107,7 +107,7 @@ config DRM_KMS_HELPER config DRM_PANIC bool "Display a user-friendly message when a kernel panic occurs" -depends on DRM && !FRAMEBUFFER_CONSOLE +depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) I thought the idea was to only make it depend on !VT_CONSOLE, so that distros could also enable fbcon / VT but prevent the race condition to happen due the VT not being a system console for the kernel to print messages ? Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and drm_panic can be enabled safely. I don't know if that really matters, and if VT_CONSOLE has any usage apart from fbcon. It is used for any kind of virtual terminal, so also for vgacon. Ah thanks, but I think vgacon is safe to use with drm_panic. The problem with fbcon, is that it can also uses the fbdev emulation from the drm driver, and in this case, drm_panic will write to the same framebuffer as fbcon during a panic, leading to corrupted output. This can't occur with other graphical console, so !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) is probably good. -- Jocelyn Gr{oetje,eeting}s, Geert
Re: [PATCH] drm/panic: depends on !VT_CONSOLE
On 16/06/2024 14:43, Javier Martinez Canillas wrote: Jocelyn Falempe writes: Hello Jocelyn, The race condition between fbcon and drm_panic can only occurs if VT_CONSOLE is set. So update drm_panic dependency accordingly. This will make it easier for Linux distributions to enable drm_panic by disabling VT_CONSOLE, and keeping fbcon terminal. The only drawback is that fbcon won't display the boot kmsg, so it should rely on userspace to do that. At least plymouth already handle this case with https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 Suggested-by: Daniel Vetter Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index a9df94291622..f5c989aed7e9 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -107,7 +107,7 @@ config DRM_KMS_HELPER config DRM_PANIC bool "Display a user-friendly message when a kernel panic occurs" - depends on DRM && !FRAMEBUFFER_CONSOLE + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) I thought the idea was to only make it depend on !VT_CONSOLE, so that distros could also enable fbcon / VT but prevent the race condition to happen due the VT not being a system console for the kernel to print messages ? Yes, but when writing the patch, I thought that if you have VT_CONSOLE=y and FRAMEBUFFER_CONSOLE=n, then there won't be any race condition, and drm_panic can be enabled safely. I don't know if that really matters, and if VT_CONSOLE has any usage apart from fbcon. In other words, my understanding from the discussion with Sima was that this should be instead: + depends on DRM && !VT_CONSOLE I've tested that and at least I see that a framebuffer console is present and `echo c > /proc/sysrq-trigger` triggers the DRM panic handler message (but don't know if the race exists and is just that I was not hitting it). If my understanding is correct and should only be a depends on !VT_CONSOLE then feel free to add my: Tested-by: Javier Martinez Canillas Best regards, -- Jocelyn
Re: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()
On 16/06/2024 11:12, Geert Uytterhoeven wrote: On Sun, Jun 16, 2024 at 11:08 AM Geert Uytterhoeven wrote: On Sat, Jun 15, 2024 at 12:55 PM kernel test robot wrote: kernel test robot noticed the following build errors: [auto build test ERROR on drm-misc/drm-misc-next] [cannot apply to linus/master v6.10-rc3 next-20240613] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Geert-Uytterhoeven/drm-panic-Fix-uninitialized-drm_scanout_buffer-set_pixel-crash/20240614-032053 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/3121082eb4beb461773ebb6f656ed9b4286967ee.1718305355.git.geert%2Brenesas%40glider.be patch subject: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset() config: x86_64-randconfig-003-20240615 (https://download.01.org/0day-ci/archive/20240615/202406151811.yeiz6203-...@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240615/202406151811.yeiz6203-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202406151811.yeiz6203-...@intel.com/ All errors (new ones prefixed by >>): depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm depmod: ERROR: Found 2 modules in dependency cycles! Oops, so DRM core cannot call any of the helpers, and DRM_PANIC selecting DRM_KMS_HELPER was wrong in the first place? Q: So how does this work with DRM_PANIC calling drm_fb_helper_emergency_disable()? A: drm_fb_helper_emergency_disable() is a dummy if !CONFIG_DRM_FBDEV_EMULATION, so I guess no one tried to build a failing randconfig with CONFIG_DRM_FBDEV_EMULATION=y yet. drm_fb_helper_emergency_disable() is part of the series https://patchwork.freedesktop.org/series/132720/ which, after discussing it on IRC with sima and Javier, is not a good solution, and is abandoned. I think the "select DRM_KMS_HELPER" is a leftover from earlier version of drm_panic, that used the color conversion function from drm_kms_helper, but that has changed in v10 and later. drm_panic is called from the drm_core code, so in fact it can't depends on the kms helper. I don't see a good solution to workaround this circular dependency, maybe depends on DRM_KMS_HELPER being built-in ? (so that means drm_core will also be built-in). But that means platform that build drm core as module, won't be able to use drm_panic. There are a few of them in the kernel tree: rg -l CONFIG_DRM=m arch/powerpc/configs/85xx/stx_gp3_defconfig arch/powerpc/configs/ppc6xx_defconfig arch/powerpc/configs/skiroot_defconfig arch/powerpc/configs/pmac32_defconfig arch/mips/configs/ci20_defconfig arch/arc/configs/axs101_defconfig arch/arc/configs/axs103_smp_defconfig arch/riscv/configs/defconfig arch/parisc/configs/generic-32bit_defconfig arch/arm/configs/davinci_all_defconfig arch/arm/configs/omap2plus_defconfig arch/arm/configs/pxa_defconfig arch/arm64/configs/defconfig Gr{oetje,eeting}s, Geert -- Jocelyn
Re: [PATCH v2 7/7] drm/panic: Add support for drawing a monochrome graphical logo
On 13/06/2024 21:18, Geert Uytterhoeven wrote: Re-use the existing support for boot-up logos to draw a monochrome graphical logo in the DRM panic handler. When no suitable graphical logo is available, the code falls back to the ASCII art penguin logo. Note that all graphical boot-up logos are freed during late kernel initialization, hence a copy must be made for later use. Would it be possible to have the logo not in the __init section if DRM_PANIC is set ? The patch looks good to me anyway. Reviewed-by: Jocelyn Falempe Signed-off-by: Geert Uytterhoeven --- v2: - Rebased, - Inline trivial draw_logo_mono(). --- drivers/gpu/drm/drm_panic.c | 65 + drivers/video/logo/Kconfig | 2 ++ 2 files changed, 60 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index f7e22b69bb25d3be..af30f243b2802ad7 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -7,11 +7,15 @@ */ #include +#include #include #include #include +#include #include +#include #include +#include #include #include @@ -88,6 +92,42 @@ static const struct drm_panic_line logo_ascii[] = { PANIC_LINE(" \\___)=(___/"), }; +#ifdef CONFIG_LOGO +static const struct linux_logo *logo_mono; + +static int drm_panic_setup_logo(void) +{ + const struct linux_logo *logo = fb_find_logo(1); + const unsigned char *logo_data; + struct linux_logo *logo_dup; + + if (!logo || logo->type != LINUX_LOGO_MONO) + return 0; + + /* The logo is __init, so we must make a copy for later use */ + logo_data = kmemdup(logo->data, + size_mul(DIV_ROUND_UP(logo->width, BITS_PER_BYTE), logo->height), + GFP_KERNEL); + if (!logo_data) + return -ENOMEM; + + logo_dup = kmemdup(logo, sizeof(*logo), GFP_KERNEL); + if (!logo_dup) { + kfree(logo_data); + return -ENOMEM; + } + + logo_dup->data = logo_data; + logo_mono = logo_dup; + + return 0; +} + +device_initcall(drm_panic_setup_logo); +#else +#define logo_mono ((const struct linux_logo *)NULL) +#endif + /* * Color conversion */ @@ -452,15 +492,22 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); struct drm_rect r_screen, r_logo, r_msg; + unsigned int logo_width, logo_height; if (!font) return; r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); - r_logo = DRM_RECT_INIT(0, 0, - get_max_line_len(logo_ascii, logo_ascii_lines) * font->width, - logo_ascii_lines * font->height); + if (logo_mono) { + logo_width = logo_mono->width; + logo_height = logo_mono->height; + } else { + logo_width = get_max_line_len(logo_ascii, logo_ascii_lines) * font->width; + logo_height = logo_ascii_lines * font->height; + } + + r_logo = DRM_RECT_INIT(0, 0, logo_width, logo_height); r_msg = DRM_RECT_INIT(0, 0, min(get_max_line_len(panic_msg, msg_lines) * font->width, sb->width), min(msg_lines * font->height, sb->height)); @@ -471,10 +518,14 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) /* Fill with the background color, and draw text on top */ drm_panic_fill(sb, _screen, bg_color); - if ((r_msg.x1 >= drm_rect_width(_logo) || r_msg.y1 >= drm_rect_height(_logo)) && - drm_rect_width(_logo) <= sb->width && drm_rect_height(_logo) <= sb->height) { - draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, _logo, - fg_color); + if ((r_msg.x1 >= logo_width || r_msg.y1 >= logo_height) && + logo_width <= sb->width && logo_height <= sb->height) { + if (logo_mono) + drm_panic_blit(sb, _logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), + fg_color); + else + draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, _logo, + fg_color); } draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, _msg, fg_color); } diff --git a/drivers/video/logo/Kconfig b/drivers/video/logo/Kconfig index b7d94d1dd1585a84..ce6bb753522d215d 100644 --- a/drivers/video/logo/Kconfig +++ b/drivers/video/logo/Kconfi
Re: [PATCH v2 6/7] drm/panic: Rename logo to logo_ascii
On 13/06/2024 21:18, Geert Uytterhoeven wrote: Rename variables related to the ASCII logo, to prepare for the advent of support for graphical logos. Thanks, that looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Geert Uytterhoeven --- v2: - Rebased. --- drivers/gpu/drm/drm_panic.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 5b0acf8c86e402a8..f7e22b69bb25d3be 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -78,7 +78,7 @@ static struct drm_panic_line panic_msg[] = { PANIC_LINE("Please reboot your computer."), }; -static const struct drm_panic_line logo[] = { +static const struct drm_panic_line logo_ascii[] = { PANIC_LINE(" .--._"), PANIC_LINE("|o_o | | |"), PANIC_LINE("|:_/ | | |"), @@ -447,7 +447,7 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb, static void draw_panic_static_user(struct drm_scanout_buffer *sb) { size_t msg_lines = ARRAY_SIZE(panic_msg); - size_t logo_lines = ARRAY_SIZE(logo); + size_t logo_ascii_lines = ARRAY_SIZE(logo_ascii); u32 fg_color = convert_from_xrgb(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); @@ -459,8 +459,8 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); r_logo = DRM_RECT_INIT(0, 0, - get_max_line_len(logo, logo_lines) * font->width, - logo_lines * font->height); + get_max_line_len(logo_ascii, logo_ascii_lines) * font->width, + logo_ascii_lines * font->height); r_msg = DRM_RECT_INIT(0, 0, min(get_max_line_len(panic_msg, msg_lines) * font->width, sb->width), min(msg_lines * font->height, sb->height)); @@ -473,7 +473,8 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) if ((r_msg.x1 >= drm_rect_width(_logo) || r_msg.y1 >= drm_rect_height(_logo)) && drm_rect_width(_logo) <= sb->width && drm_rect_height(_logo) <= sb->height) { - draw_txt_rectangle(sb, font, logo, logo_lines, false, _logo, fg_color); + draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, _logo, + fg_color); } draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, _msg, fg_color); }
Re: [PATCH v2 5/7] drm/panic: Convert to drm_fb_clip_offset()
On 13/06/2024 21:18, Geert Uytterhoeven wrote: Use the drm_fb_clip_offset() helper instead of open-coding the same operation. Thanks, it's a nice cleanup. Reviewed-by: Jocelyn Falempe Signed-off-by: Geert Uytterhoeven --- DRM_PANIC already selects DRM_KMS_HELPER. v2: - New. --- drivers/gpu/drm/drm_panic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 814ef5c20c08ee42..5b0acf8c86e402a8 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -285,7 +285,7 @@ static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip, return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, fg_color); map = sb->map[0]; - iosys_map_incr(, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]); + iosys_map_incr(, drm_fb_clip_offset(sb->pitch[0], sb->format, clip)); switch (sb->format->cpp[0]) { case 2: @@ -373,7 +373,7 @@ static void drm_panic_fill(struct drm_scanout_buffer *sb, struct drm_rect *clip, return drm_panic_fill_pixel(sb, clip, color); map = sb->map[0]; - iosys_map_incr(, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]); + iosys_map_incr(, drm_fb_clip_offset(sb->pitch[0], sb->format, clip)); switch (sb->format->cpp[0]) { case 2:
Re: [PATCH v2 4/7] drm/panic: Spelling s/formater/formatter/
On 13/06/2024 21:18, Geert Uytterhoeven wrote: Fix a misspelling of "formatter". It's probably because the same word has only one t in my native language. Thanks for the fix. Reviewed-by: Jocelyn Falempe Fixes: 54034bebb22fd4be ("drm/panic: Add a kmsg panic screen") Signed-off-by: Geert Uytterhoeven --- v2: - New. --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index a9972ce05d7e6fe4..e3c51009d9b476b3 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -138,7 +138,7 @@ config DRM_PANIC_DEBUG If in doubt, say "N". config DRM_PANIC_SCREEN - string "Panic screen formater" + string "Panic screen formatter" default "user" depends on DRM_PANIC help
Re: [PATCH v2 3/7] lib/fonts: Fix visiblity of SUN12x22 and TER16x32 if DRM_PANIC
On 13/06/2024 21:18, Geert Uytterhoeven wrote: When CONFIG_FONTS ("Select compiled-in fonts") is not enabled, the user should not be asked about any fonts. However, when CONFIG_DRM_PANIC is enabled, the user is still asked about the Sparc console 12x22 and Terminus 16x32 fonts. Fix this by moving the "|| DRM_PANIC" to where it belongs. Split the dependency in two rules to improve readability. Sorry I think I misunderstood the SPARC && FONTS condition. Your fix is much clearer, thanks. Reviewed-by: Jocelyn Falempe Fixes: b94605a3889b9084 ("lib/fonts: Allow to select fonts for drm_panic") Signed-off-by: Geert Uytterhoeven --- v2: - New. --- lib/fonts/Kconfig | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/fonts/Kconfig b/lib/fonts/Kconfig index befcb463f7381d1a..3ac26bdbc3ff01a3 100644 --- a/lib/fonts/Kconfig +++ b/lib/fonts/Kconfig @@ -105,7 +105,8 @@ config FONT_SUN8x16 config FONT_SUN12x22 bool "Sparc console 12x22 font (not supported by all drivers)" - depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC + depends on !SPARC && FONTS help This is the high resolution console font for Sun machines with very big letters (like the letters used in the SPARC PROM). If the @@ -113,7 +114,8 @@ config FONT_SUN12x22 config FONT_TER16x32 bool "Terminus 16x32 font (not supported by all drivers)" - depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC + depends on !SPARC && FONTS || SPARC help Terminus Font is a clean, fixed width bitmap font, designed for long (8 and more hours per day) work with computers.
Re: [PATCH v2 2/7] drm/panic: Fix off-by-one logo size checks
On 13/06/2024 21:18, Geert Uytterhoeven wrote: Logos that are either just as wide or just as high as the display work fine. Sure, that looks good to me. Reviewed-by: Jocelyn Falempe Fixes: bf9fb17c6672868d ("drm/panic: Add a drm panic handler") Signed-off-by: Geert Uytterhoeven --- v2: - Rebased. --- drivers/gpu/drm/drm_panic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index fc04ed4e0b399f55..814ef5c20c08ee42 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -472,7 +472,7 @@ static void draw_panic_static_user(struct drm_scanout_buffer *sb) drm_panic_fill(sb, _screen, bg_color); if ((r_msg.x1 >= drm_rect_width(_logo) || r_msg.y1 >= drm_rect_height(_logo)) && - drm_rect_width(_logo) < sb->width && drm_rect_height(_logo) < sb->height) { + drm_rect_width(_logo) <= sb->width && drm_rect_height(_logo) <= sb->height) { draw_txt_rectangle(sb, font, logo, logo_lines, false, _logo, fg_color); } draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, _msg, fg_color);
Re: [PATCH v2 1/7] drm/panic: Fix uninitialized drm_scanout_buffer.set_pixel() crash
On 13/06/2024 21:17, Geert Uytterhoeven wrote: No implementations of drm_plane_helper_funcs.get_scanout_buffer() fill in the optional drm_scanout_buffer.set_pixel() member. Hence the member may contain non-zero garbage, causing a crash when deferencing it during drm panic. Fix this by pre-initializing the drm_scanout_buffer object before calling drm_plane_helper_funcs.get_scanout_buffer(). Good catch, I don't know how I didn't hit this bug before. Thanks for the fix. Reviewed-by: Jocelyn Falempe Fixes: 24d07f114e4ec760 ("drm/panic: Add a set_pixel() callback to drm_scanout_buffer") Signed-off-by: Geert Uytterhoeven --- v2: - New. --- drivers/gpu/drm/drm_panic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 293d4dcbc80da7ba..fc04ed4e0b399f55 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -582,7 +582,7 @@ static void draw_panic_dispatch(struct drm_scanout_buffer *sb) static void draw_panic_plane(struct drm_plane *plane) { - struct drm_scanout_buffer sb; + struct drm_scanout_buffer sb = { }; int ret; unsigned long flags;
[PATCH] drm/panic: depends on !VT_CONSOLE
The race condition between fbcon and drm_panic can only occurs if VT_CONSOLE is set. So update drm_panic dependency accordingly. This will make it easier for Linux distributions to enable drm_panic by disabling VT_CONSOLE, and keeping fbcon terminal. The only drawback is that fbcon won't display the boot kmsg, so it should rely on userspace to do that. At least plymouth already handle this case with https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/224 Suggested-by: Daniel Vetter Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index a9df94291622..f5c989aed7e9 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -107,7 +107,7 @@ config DRM_KMS_HELPER config DRM_PANIC bool "Display a user-friendly message when a kernel panic occurs" - depends on DRM && !FRAMEBUFFER_CONSOLE + depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE) select DRM_KMS_HELPER select FONT_SUPPORT help base-commit: 2bae076f3e35234e42bd7c90acd8caae8368ba90 -- 2.45.1
Re: [PATCH 0/3] drm/panic: Fixes and graphical logo
On 13/06/2024 11:32, Geert Uytterhoeven wrote: Hi Jocelyn, On Thu, Jun 13, 2024 at 10:38 AM Jocelyn Falempe wrote: On 12/06/2024 15:54, Geert Uytterhoeven wrote: If drm/panic is enabled, a user-friendly message is shown on screen when a kernel panic occurs, together with an ASCII art penguin logo. Of course we can do better ;-) Hence this patch series extends drm/panic to draw the monochrome graphical boot logo, when available, preceded by the customary fix. Thanks for your patch. I've tested it, and it works great. Thank you! You need to rebase your series on top of drm-misc-next, because it conflicts with a series I pushed last week: https://patchwork.freedesktop.org/series/134286/ I had seen that you said you had pushed this to drm-misc-next[1] before I posted my series, but couldn't find the actual commits in drm-misc/for-linux-next, which is still at commit dfc1209ed5a3861c ("arm/komeda: Remove all CONFIG_DEBUG_FS conditional compilations", so I assumed you just forgot to push? However, the latest pull request[2] does include them, while linux-next does not. Has the drm-misc git repo moved? It moved to gitlab recently, the new url is g...@gitlab.freedesktop.org:drm/misc/kernel.git and the drm_panic kmsg screen commit is there: https://gitlab.freedesktop.org/drm/misc/kernel/-/commits/drm-misc-next?ref_type=heads Thanks! [1] https://lore.kernel.org/all/3649ff15-df2b-49ba-920f-c418355d7...@redhat.com/ [2] "[PULL] drm-misc-next" https://lore.kernel.org/all/20240613-cicada-of-infinite-unity-0955ca@houat/ Gr{oetje,eeting}s, Geert
Re: [PATCH 0/3] drm/panic: Fixes and graphical logo
On 12/06/2024 15:54, Geert Uytterhoeven wrote: Hi all, If drm/panic is enabled, a user-friendly message is shown on screen when a kernel panic occurs, together with an ASCII art penguin logo. Of course we can do better ;-) Hence this patch series extends drm/panic to draw the monochrome graphical boot logo, when available, preceded by the customary fix. Thanks for your patch. I've tested it, and it works great. You need to rebase your series on top of drm-misc-next, because it conflicts with a series I pushed last week: https://patchwork.freedesktop.org/series/134286/ This rebase should simplify the draw_logo_mono() function. This has been tested with rcar-du. Thanks for your comments! Geert Uytterhoeven (3): drm/panic: Fix off-by-one logo size checks drm/panic: Rename logo to logo_ascii drm/panic: Add support for drawing a monochrome graphical logo drivers/gpu/drm/drm_panic.c | 81 + drivers/video/logo/Kconfig | 2 + 2 files changed, 75 insertions(+), 8 deletions(-)
Re: [PATCH v2 3/3] drm/mgag200: Set .detect_ctx() and enable connector polling
On 10/06/2024 16:06, Thomas Zimmermann wrote: Set .detect_ctx() in struct drm_connector_helper_funcs to the common helper drm_connector_helper_detect_from_ddc() and enable polling for the connector. Mgag200 will now test for the monitor's presence by probing the DDC in regular intervals. I've tested this series on two servers, one with physical VGA plugged, and one remote system, and it works as intended. Thanks a lot for this work. Reviewed-by: Jocelyn Falempe Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_g200.c| 1 + drivers/gpu/drm/mgag200/mgag200_g200eh.c | 1 + drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 1 + drivers/gpu/drm/mgag200/mgag200_g200er.c | 1 + drivers/gpu/drm/mgag200/mgag200_g200ev.c | 1 + drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 1 + drivers/gpu/drm/mgag200/mgag200_g200se.c | 1 + drivers/gpu/drm/mgag200/mgag200_g200wb.c | 1 + drivers/gpu/drm/mgag200/mgag200_vga.c | 6 +- 9 files changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_g200.c b/drivers/gpu/drm/mgag200/mgag200_g200.c index ff467b0f9cbf3..f874e29498409 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200.c @@ -401,6 +401,7 @@ struct mga_device *mgag200_g200_device_create(struct pci_dev *pdev, const struct return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c index 6f31c5249f0b1..52bf49ead5c50 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c @@ -277,6 +277,7 @@ struct mga_device *mgag200_g200eh_device_create(struct pci_dev *pdev, const stru return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c index 5befe8da4beb2..e7f89b2a59fd0 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c @@ -182,6 +182,7 @@ struct mga_device *mgag200_g200eh3_device_create(struct pci_dev *pdev, return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c index 55c275180cde2..4e8a1756138d7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200er.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c @@ -316,6 +316,7 @@ struct mga_device *mgag200_g200er_device_create(struct pci_dev *pdev, const stru return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c index 2466126140db6..d884f3cb0ec79 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c @@ -321,6 +321,7 @@ struct mga_device *mgag200_g200ev_device_create(struct pci_dev *pdev, const stru return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c index a52e60609c3de..839401e8b4654 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c @@ -202,6 +202,7 @@ struct mga_device *mgag200_g200ew3_device_create(struct pci_dev *pdev, return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c index 212770acdd477..a824bb8ad5791 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c @@ -521,6 +521,7 @@ struct mga_device *mgag200_g200se_device_create(struct pci_dev *pdev, const stru return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c index cb6daa0426fbc..835df0f4fc13d 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c @@ -326,6 +326,7 @@ struct mga_device *mgag200_g200wb_device_create(struct pci_dev *pdev, const stru return ERR_PTR(ret); drm_mode_config_reset(dev); + drm_kms_helper_poll_init(dev); return mdev; } diff --git a/drivers/gpu/drm/mgag200/mgag200_vga.c b/drivers/gpu/drm/mgag200/mgag200_vga.c index 6d8982990c2c3..60568f32736dd 100644 --- a/drivers/gpu/drm/mgag200/mgag200_vga.c +++ b/drivers/gpu/drm/mgag200/mgag200_vga.c
Re: [PATCH v2 2/3] drm/mgag200: Add BMC output
On 10/06/2024 16:06, Thomas Zimmermann wrote: The BMC output can be viewed via the BMC's web interface or a similar client. Represent it as virtual encoder and connector. It's attached to the same CRTC as the VGA connector. The connector's status depends on the physical connector's status. The BMC is only connected if the physical connector is not. This is necessary to support userspace clients that can only handle a single output per CRTC. The BMC is a server feature. Add a BMC output for all server chips, but not the desktop models. Thanks, I think it makes thing clearer for userspace, even if it's not perfect yet. It also allows to have bigger resolution when no vga monitor are connected. Reviewed-by: Jocelyn Falempe Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_bmc.c | 107 ++ drivers/gpu/drm/mgag200/mgag200_drv.h | 10 ++ drivers/gpu/drm/mgag200/mgag200_g200eh.c | 4 + drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 4 + drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 + drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 + drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 4 + drivers/gpu/drm/mgag200/mgag200_g200se.c | 4 + drivers/gpu/drm/mgag200/mgag200_g200wb.c | 4 + 9 files changed, 145 insertions(+) diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c index 2ba2e3c5086a5..23ef85aa7e378 100644 --- a/drivers/gpu/drm/mgag200/mgag200_bmc.c +++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c @@ -2,8 +2,18 @@ #include +#include +#include +#include +#include + #include "mgag200_drv.h" +static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connector *connector) +{ + return container_of(connector, struct mgag200_bmc_connector, base); +} + void mgag200_bmc_disable_vidrst(struct mga_device *mdev) { u8 tmp; @@ -97,3 +107,100 @@ void mgag200_bmc_enable_vidrst(struct mga_device *mdev) tmp &= ~0x10; WREG_DAC(MGA1064_GEN_IO_DATA, tmp); } + +static const struct drm_encoder_funcs mgag200_bmc_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + +static int mgag200_bmc_connector_helper_detect_ctx(struct drm_connector *connector, + struct drm_modeset_acquire_ctx *ctx, + bool force) +{ + struct mgag200_bmc_connector *bmc_connector = to_mgag200_bmc_connector(connector); + struct drm_connector *physical_connector = bmc_connector->physical_connector; + + /* +* Most user-space compositors cannot handle more than one connected +* connector per CRTC. Hence, we only mark the BMC as connected if the +* physical connector is disconnected. If the physical connector's status +* is connected or unknown, the BMC remains disconnected. This has no +* effect on the output of the BMC. +* +* FIXME: Remove this logic once user-space compositors can handle more +*than one connector per CRTC. The BMC should always be connected. +*/ + + if (physical_connector && physical_connector->status == connector_status_disconnected) + return connector_status_connected; + + return connector_status_disconnected; +} + +static int mgag200_bmc_connector_helper_get_modes(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + struct mga_device *mdev = to_mga_device(dev); + const struct mgag200_device_info *minfo = mdev->info; + + return drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay); +} + +static const struct drm_connector_helper_funcs mgag200_bmc_connector_helper_funcs = { + .get_modes = mgag200_bmc_connector_helper_get_modes, + .detect_ctx = mgag200_bmc_connector_helper_detect_ctx, +}; + +static const struct drm_connector_funcs mgag200_bmc_connector_funcs = { + .reset = drm_atomic_helper_connector_reset, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = drm_connector_cleanup, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +static int mgag200_bmc_connector_init(struct drm_device *dev, + struct mgag200_bmc_connector *bmc_connector, + struct drm_connector *physical_connector) +{ + struct drm_connector *connector = _connector->base; + int ret; + + ret = drm_connector_init(dev, connector, _bmc_connector_funcs, +DRM_MODE_CONNECTOR_VIRTUAL); + if (ret) + return ret; + drm_connector_helper_add(connector, _bmc_connector_helper_funcs); + + bmc_connector->physical_connector = physical_connector; + +
Re: [PATCH v2 1/3] drm/mgag200: Consolidate VGA output
On 10/06/2024 16:06, Thomas Zimmermann wrote: The various models have common code for the VGA output's encoder and connector. Move everything into a single shared source file. Remove some obsolete initializer macros. No functional changes. Thanks for this patch, it removes some duplication, as vga code is the same for all variant. Reviewed-by: Jocelyn Falempe Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/Makefile | 3 +- drivers/gpu/drm/mgag200/mgag200_drv.h | 24 +++- drivers/gpu/drm/mgag200/mgag200_g200.c| 46 +-- drivers/gpu/drm/mgag200/mgag200_g200eh.c | 46 +-- drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 46 +-- drivers/gpu/drm/mgag200/mgag200_g200er.c | 46 +-- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 46 +-- drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 46 +-- drivers/gpu/drm/mgag200/mgag200_g200se.c | 46 +-- drivers/gpu/drm/mgag200/mgag200_g200wb.c | 46 +-- drivers/gpu/drm/mgag200/mgag200_vga.c | 68 +++ 11 files changed, 95 insertions(+), 368 deletions(-) create mode 100644 drivers/gpu/drm/mgag200/mgag200_vga.c diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile index 0b919352046eb..d1b25f9f6586e 100644 --- a/drivers/gpu/drm/mgag200/Makefile +++ b/drivers/gpu/drm/mgag200/Makefile @@ -11,6 +11,7 @@ mgag200-y := \ mgag200_g200ew3.o \ mgag200_g200se.o \ mgag200_g200wb.o \ - mgag200_mode.o + mgag200_mode.o \ + mgag200_vga.o obj-$(CONFIG_DRM_MGAG200) += mgag200.o diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 20e3710e056b3..db89fddc26dcb 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -283,8 +283,12 @@ struct mga_device { struct drm_plane primary_plane; struct drm_crtc crtc; - struct drm_encoder encoder; - struct drm_connector connector; + struct { + struct { + struct drm_encoder encoder; + struct drm_connector connector; + } vga; + } output; }; static inline struct mga_device *to_mga_device(struct drm_device *dev) @@ -417,25 +421,15 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st .atomic_duplicate_state = mgag200_crtc_atomic_duplicate_state, \ .atomic_destroy_state = mgag200_crtc_atomic_destroy_state -#define MGAG200_DAC_ENCODER_FUNCS \ - .destroy = drm_encoder_cleanup - -#define MGAG200_VGA_CONNECTOR_HELPER_FUNCS \ - .get_modes = drm_connector_helper_get_modes - -#define MGAG200_VGA_CONNECTOR_FUNCS \ - .reset = drm_atomic_helper_connector_reset, \ - .fill_modes = drm_helper_probe_single_connector_modes, \ - .destroy= drm_connector_cleanup, \ - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, \ - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state - void mgag200_set_mode_regs(struct mga_device *mdev, const struct drm_display_mode *mode); void mgag200_set_format_regs(struct mga_device *mdev, const struct drm_format_info *format); void mgag200_enable_display(struct mga_device *mdev); void mgag200_init_registers(struct mga_device *mdev); int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_available); +/* mgag200_vga.c */ +int mgag200_vga_output_init(struct mga_device *mdev); + /* mgag200_bmc.c */ void mgag200_bmc_disable_vidrst(struct mga_device *mdev); void mgag200_bmc_enable_vidrst(struct mga_device *mdev); diff --git a/drivers/gpu/drm/mgag200/mgag200_g200.c b/drivers/gpu/drm/mgag200/mgag200_g200.c index 39a29d8ffca6e..ff467b0f9cbf3 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200.c @@ -9,7 +9,6 @@ #include #include -#include "mgag200_ddc.h" #include "mgag200_drv.h" static int mgag200_g200_init_pci_options(struct pci_dev *pdev) @@ -184,26 +183,11 @@ static const struct drm_crtc_funcs mgag200_g200_crtc_funcs = { MGAG200_CRTC_FUNCS, }; -static const struct drm_encoder_funcs mgag200_g200_dac_encoder_funcs = { - MGAG200_DAC_ENCODER_FUNCS, -}; - -static const struct drm_connector_helper_funcs mgag200_g200_vga_connector_helper_funcs = { - MGAG200_VGA_CONNECTOR_HELPER_FUNCS, -}; - -static const struct drm_connector_funcs mgag200_g200_vga_connector_funcs = { - MGAG200_VGA_CONNECTOR_FUNCS, -}; - static int mgag200_g200_pipeline_init(struct mga_device *mdev) { struct drm_device *dev = >base; struct drm_plane *primary_plane = >primary_plane; struct drm_crtc *crtc = >crtc; - struct drm_encoder *encoder = >encod
Re: [PATCH v2 3/3] drm/panic: Add a kmsg panic screen
On 07/06/2024 11:16, Javier Martinez Canillas wrote: Jocelyn Falempe writes: Add a kmsg option, which will display the last lines of kmsg, and should be similar to fbcon. Add a drm.panic_screen module parameter, so you can choose between the different panic screens available. two options currently, but more will be added later: * "user": a short message telling the user to reboot the machine. * "kmsg": fill the screen with the last lines of kmsg. You can even change it at runtime by writing to /sys/module/drm/parameters/panic_screen Great! v2: * use module parameter instead of Kconfig choice (Javier Martinez Canillas) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 11 drivers/gpu/drm/drm_panic.c | 108 2 files changed, 109 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 9703429de6b9..944815cee080 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -137,6 +137,17 @@ config DRM_PANIC_DEBUG This is unsafe and should not be enabled on a production build. If in doubt, say "N". +config DRM_PANIC_SCREEN + string "Panic screen formater" + default "user" + depends on DRM_PANIC + help + This option enable to choose what will be displayed when a kernel + panic occurs. You can choose between "user", a short message telling + the user to reboot the system, or "kmsg" which will display the last + lines of kmsg. Maybe I would mention here that this is only about the default, but that can be changed using the "drm.panic_screen=" kernel cmdline parameter or writting to the /sys/module/drm/parameters/panic_screen sysfs entry. [...] Done +static void draw_panic_static_kmsg(struct drm_scanout_buffer *sb) +{ + u32 fg_color = convert_from_xrgb(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); + u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); + const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); Dan reported that get_default_font() can return NULL + struct drm_rect r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); + struct kmsg_dump_iter iter; + char kmsg_buf[512]; + size_t kmsg_len; + struct drm_panic_line line; + int yoffset = sb->height - font->height - (sb->height % font->height) / 2; + + if (!font) + return; + ... so you have to calculate yoffset after checking if the font is not NULL. Yes I fixed that too. with that fixed: Reviewed-by: Javier Martinez Canillas Thanks a lot. I just pushed this series to drm-misc-next. Best regards, -- Jocelyn
[PATCH v2 1/3] drm/panic: only draw the foreground color in drm_panic_blit()
The whole framebuffer is cleared, so it's useless to rewrite the background colored pixels. It allows to simplify the drawing functions, and prepare the work for the set_pixel() callback. v2: * keep fg16/fg24/fg32 as variable name for the blit function. * add drm_panic_is_pixel_fg() to avoid code duplication. both suggested by Javier Martinez Canillas Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 67 + 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 7ece67086cec..056494ae1ede 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -194,40 +194,42 @@ static u32 convert_from_xrgb(u32 color, u32 format) /* * Blit & Fill */ +/* check if the pixel at coord x,y is 1 (foreground) or 0 (background) */ +static bool drm_panic_is_pixel_fg(const u8 *sbuf8, unsigned int spitch, int x, int y) +{ + return (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) != 0; +} + static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u16 fg16, u16 bg16) +u16 fg16) { unsigned int y, x; - u16 val16; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val16 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg16 : bg16; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, val16); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) + if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, fg16); } static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32, u32 bg32) +u32 fg32) { unsigned int y, x; - u32 val32; for (y = 0; y < height; y++) { for (x = 0; x < width; x++) { u32 off = y * dpitch + x * 3; - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - - /* write blue-green-red to output in little endianness */ - iosys_map_wr(dmap, off, u8, (val32 & 0x00FF) >> 0); - iosys_map_wr(dmap, off + 1, u8, (val32 & 0xFF00) >> 8); - iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF) >> 16); + if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) { + /* write blue-green-red to output in little endianness */ + iosys_map_wr(dmap, off, u8, (fg32 & 0x00FF) >> 0); + iosys_map_wr(dmap, off + 1, u8, (fg32 & 0xFF00) >> 8); + iosys_map_wr(dmap, off + 2, u8, (fg32 & 0x00FF) >> 16); + } } } } @@ -235,17 +237,14 @@ static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32, u32 bg32) +u32 fg32) { unsigned int y, x; - u32 val32; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, val32); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) + if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, fg32); } /* @@ -257,7 +256,6 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, * @height: height of the image to copy, in pixels * @width: width of the image to copy, in pixels * @fg_color: foreground color, in destination format - * @bg_color: background color, in destination format * @pixel_width: pixel width in bytes. * * This can be used to draw a font character, which is a monochrome image, to a @@ -266,21 +264,20 @@ static void
[PATCH v2 3/3] drm/panic: Add a kmsg panic screen
Add a kmsg option, which will display the last lines of kmsg, and should be similar to fbcon. Add a drm.panic_screen module parameter, so you can choose between the different panic screens available. two options currently, but more will be added later: * "user": a short message telling the user to reboot the machine. * "kmsg": fill the screen with the last lines of kmsg. You can even change it at runtime by writing to /sys/module/drm/parameters/panic_screen v2: * use module parameter instead of Kconfig choice (Javier Martinez Canillas) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 11 drivers/gpu/drm/drm_panic.c | 108 2 files changed, 109 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 9703429de6b9..944815cee080 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -137,6 +137,17 @@ config DRM_PANIC_DEBUG This is unsafe and should not be enabled on a production build. If in doubt, say "N". +config DRM_PANIC_SCREEN + string "Panic screen formater" + default "user" + depends on DRM_PANIC + help + This option enable to choose what will be displayed when a kernel + panic occurs. You can choose between "user", a short message telling + the user to reboot the system, or "kmsg" which will display the last + lines of kmsg. + Default is "user" + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 5dc9e98108ed..2e11273a8ad6 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -27,6 +28,12 @@ MODULE_AUTHOR("Jocelyn Falempe"); MODULE_DESCRIPTION("DRM panic handler"); MODULE_LICENSE("GPL"); +static char drm_panic_screen[16] = CONFIG_DRM_PANIC_SCREEN; +module_param_string(panic_screen, drm_panic_screen, sizeof(drm_panic_screen), 0644); +MODULE_PARM_DESC(panic_screen, +"Choose what will be displayed by drm_panic, 'user' or 'kmsg' [default=" +CONFIG_DRM_PANIC_SCREEN "]"); + /** * DOC: overview * @@ -437,24 +444,18 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb, } } -/* - * Draw the panic message at the center of the screen - */ -static void draw_panic_static(struct drm_scanout_buffer *sb) +static void draw_panic_static_user(struct drm_scanout_buffer *sb) { size_t msg_lines = ARRAY_SIZE(panic_msg); size_t logo_lines = ARRAY_SIZE(logo); - u32 fg_color = CONFIG_DRM_PANIC_FOREGROUND_COLOR; - u32 bg_color = CONFIG_DRM_PANIC_BACKGROUND_COLOR; + u32 fg_color = convert_from_xrgb(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); + u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); struct drm_rect r_screen, r_logo, r_msg; if (!font) return; - fg_color = convert_from_xrgb(fg_color, sb->format->format); - bg_color = convert_from_xrgb(bg_color, sb->format->format); - r_screen = DRM_RECT_INIT(0, 0, sb->width, sb->height); r_logo = DRM_RECT_INIT(0, 0, @@ -477,6 +478,84 @@ static void draw_panic_static(struct drm_scanout_buffer *sb) draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, _msg, fg_color); } + +/* + * Draw one line of kmsg, and handle wrapping if it won't fit in the screen width. + * Return the y-offset of the next line. + */ +static int draw_line_with_wrap(struct drm_scanout_buffer *sb, const struct font_desc *font, + struct drm_panic_line *line, int yoffset, u32 fg_color) +{ + int chars_per_row = sb->width / font->width; + struct drm_rect r_txt = DRM_RECT_INIT(0, yoffset, sb->width, sb->height); + struct drm_panic_line line_wrap; + + if (line->len > chars_per_row) { + line_wrap.len = line->len % chars_per_row; + line_wrap.txt = line->txt + line->len - line_wrap.len; + draw_txt_rectangle(sb, font, _wrap, 1, false, _txt, fg_color); + r_txt.y1 -= font->height; + if (r_txt.y1 < 0) + return r_txt.y1; + while (line_wrap.txt > line->txt) { + line_wrap.txt -= chars_per_row; + line_wrap.len = chars_per_row; + draw_txt_rectangle(sb,
[PATCH v2 0/3] drm/panic: Add a kmsg dump screen
Add a kmsg option, which will display the last lines of kmsg, and should be similar to fbcon. Add a drm.panic_screen module parameter, so you can choose between the different panic screens available. Two options currently, but more will be added later: * "user": a short message telling the user to reboot the machine. * "kmsg": fill the screen with the last lines of kmsg. You can even change it at runtime by writing to /sys/module/drm/parameters/panic_screen Patch 1-2 are the same as https://patchwork.freedesktop.org/series/133963/ and are here to avoid conflicts. v2: * use module parameter instead of Kconfig choice, so it can be changed at runtime. (Javier Martinez Canillas) * keep fg16/fg24/fg32 as variable name for the blit functions (Javier Martinez Canillas) * Add drm_panic_is_pixel_fg() to avoid code duplication. (Javier Martinez Canillas) Jocelyn Falempe (3): drm/panic: only draw the foreground color in drm_panic_blit() drm/panic: Add a set_pixel() callback to drm_scanout_buffer drm/panic: Add a kmsg panic screen drivers/gpu/drm/Kconfig | 11 ++ drivers/gpu/drm/drm_panic.c | 283 +--- include/drm/drm_panic.h | 9 ++ 3 files changed, 219 insertions(+), 84 deletions(-) base-commit: 86266829ea755f737762ebda614c59b136c8feac -- 2.45.1
[PATCH v2 2/3] drm/panic: Add a set_pixel() callback to drm_scanout_buffer
This allows drivers to draw the pixel, and handle tiling, or specific color formats. v2: * Use fg_color for blit() functions (Javier Martinez Canillas) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 120 +++- include/drm/drm_panic.h | 9 +++ 2 files changed, 85 insertions(+), 44 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 056494ae1ede..5dc9e98108ed 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -247,40 +247,54 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, fg32); } +static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect *clip, +const u8 *sbuf8, unsigned int spitch, u32 fg_color) +{ + unsigned int y, x; + + for (y = 0; y < drm_rect_height(clip); y++) + for (x = 0; x < drm_rect_width(clip); x++) + if (drm_panic_is_pixel_fg(sbuf8, spitch, x, y)) + sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, fg_color); +} + /* * drm_panic_blit - convert a monochrome image to a linear framebuffer - * @dmap: destination iosys_map - * @dpitch: destination pitch in bytes + * @sb: destination scanout buffer + * @clip: destination rectangle * @sbuf8: source buffer, in monochrome format, 8 pixels per byte. * @spitch: source pitch in bytes - * @height: height of the image to copy, in pixels - * @width: width of the image to copy, in pixels * @fg_color: foreground color, in destination format - * @pixel_width: pixel width in bytes. * * This can be used to draw a font character, which is a monochrome image, to a * framebuffer in other supported format. */ -static void drm_panic_blit(struct iosys_map *dmap, unsigned int dpitch, - const u8 *sbuf8, unsigned int spitch, - unsigned int height, unsigned int width, - u32 fg_color, unsigned int pixel_width) +static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip, + const u8 *sbuf8, unsigned int spitch, u32 fg_color) { - switch (pixel_width) { + struct iosys_map map; + + if (sb->set_pixel) + return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, fg_color); + + map = sb->map[0]; + iosys_map_incr(, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]); + + switch (sb->format->cpp[0]) { case 2: - drm_panic_blit16(dmap, dpitch, sbuf8, spitch, -height, width, fg_color); + drm_panic_blit16(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), fg_color); break; case 3: - drm_panic_blit24(dmap, dpitch, sbuf8, spitch, -height, width, fg_color); + drm_panic_blit24(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), fg_color); break; case 4: - drm_panic_blit32(dmap, dpitch, sbuf8, spitch, -height, width, fg_color); + drm_panic_blit32(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), fg_color); break; default: - WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width); + WARN_ONCE(1, "Can't blit with pixel width %d\n", sb->format->cpp[0]); } } @@ -324,33 +338,51 @@ static void drm_panic_fill32(struct iosys_map *dmap, unsigned int dpitch, iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } +static void drm_panic_fill_pixel(struct drm_scanout_buffer *sb, +struct drm_rect *clip, +u32 color) +{ + unsigned int y, x; + + for (y = 0; y < drm_rect_height(clip); y++) + for (x = 0; x < drm_rect_width(clip); x++) + sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, color); +} + /* * drm_panic_fill - Fill a rectangle with a color - * @dmap: destination iosys_map, pointing to the top left corner of the rectangle - * @dpitch: destination pitch in bytes - * @height: height of the rectangle, in pixels - * @width: width of the rectangle, in pixels - * @color: color to fill the rectangle. - * @pixel_width: pixel width in bytes + * @sb: destination scanout buffer + * @clip: destination rectangle + * @color: foreground color, in destination format * * Fill a rectangle with a color, in a linear framebuffer. */ -static void drm_panic_fill(struct iosys_
Re: [PATCH 3/3] drm/panic: Add a kmsg dump screen
On 31/05/2024 11:32, Javier Martinez Canillas wrote: Jocelyn Falempe writes: Add a kmsg dump option, which will display the last lines of kmsg, and should be similar to fbcon. Add a Kconfig choice for the panic screen, so that the user can choose between this new kmsg dump, or the userfriendly option. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 21 + drivers/gpu/drm/drm_panic.c | 151 +++- 2 files changed, 136 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 9703429de6b9..78d401b55102 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -137,6 +137,27 @@ config DRM_PANIC_DEBUG This is unsafe and should not be enabled on a production build. If in doubt, say "N". +choice + prompt "Panic screen formater" + default DRM_PANIC_SCREEN_USERFRIENDLY + depends on DRM_PANIC + help + This option enable to choose what will be displayed when a kernel + panic occurs. + + config DRM_PANIC_SCREEN_USERFRIENDLY + bool "Default user friendly message" + help + Only a short message telling the user to reboot the system. + + config DRM_PANIC_SCREEN_KMSG + bool "Display the last lines of kmsg" + help + Display kmsg last lines on panic. + Enable if you are a kernel developer, and want to debug a + kernel panic. +endchoice Why having it as a compile time option and not a runtime option ? AFAICT this could be a drm.panic_format= kernel command line parameter instead. Yes, I didn't think about it. That would allow to get more debug information from a user without rebuilding the kernel. I'll prepare a v2 with that. I prefer to use a drm.panic_screen=, as "format" might be confusing with the color format ? [...] -/* - * Draw the panic message at the center of the screen - */ +#if defined(CONFIG_DRM_PANIC_SCREEN_USERFRIENDLY) + And that could avoid ifdefery in the C file. [...] +#elif defined(CONFIG_DRM_PANIC_SCREEN_KMSG) + +#include +#include + I would add these even if guarded by DRM_PANIC_SCREEN_KMSG, to the start of the C file where are the other headers include directives. The patch looks good to me though, so if you prefer to keep it as a build time option: Reviewed-by: Javier Martinez Canillas
Re: [PATCH 2/3] drm/panic: Add a set_pixel() callback to drm_scanout_buffer
On 31/05/2024 11:20, Javier Martinez Canillas wrote: Jocelyn Falempe writes: This allows drivers to draw the pixel, and handle tiling, or specific color formats. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 120 +++- include/drm/drm_panic.h | 9 +++ 2 files changed, 85 insertions(+), 44 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 9d95c7eaae83..27e26b9d842c 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -241,40 +241,54 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } +static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect *clip, +const u8 *sbuf8, unsigned int spitch, u32 color) +{ + unsigned int y, x; + + for (y = 0; y < drm_rect_height(clip); y++) + for (x = 0; x < drm_rect_width(clip); x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) You have the same check for fb vs bg in all your blit helpers, so maybe this can be a macro or static inline function instead ? That would also help with the issue I mentioned about making the logic easier to read. Sure, I can do a v2, or send an additional patch for that. Reviewed-by: Javier Martinez Canillas Thanks, -- Jocelyn
Re: [PATCH 1/3] drm/panic: only draw the foreground color in drm_panic_blit()
On 31/05/2024 11:15, Javier Martinez Canillas wrote: Jocelyn Falempe writes: Hello Jocelyn, The whole framebuffer is cleared, so it's useless to rewrite the background colored pixels. It allows to simplify the drawing functions, and prepare the work for the set_pixel() callback. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 63 +++-- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 7ece67086cec..9d95c7eaae83 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -197,37 +197,33 @@ static u32 convert_from_xrgb(u32 color, u32 format) static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u16 fg16, u16 bg16) +u16 color) What about calling this fg16 instead of color? That way is clear that only the fb is written and not the background ? Yes I can keep the fg16 name. { unsigned int y, x; - u16 val16; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val16 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg16 : bg16; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, val16); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) I would add here a comment that this check is about determining if a color is suitable for foreground or background, depending on the luminance threshold (which I understand is the 0x80 value?). The source buffer is monochrome, so store 8 pixels per byte. the (0x80 >> (x % 8)) is a bit mask, to check if the source pixel is foreground or background. I will add a comment about this, to make it clear. + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, color); } static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32, u32 bg32) +u32 color) { unsigned int y, x; - u32 val32; Same here, I would left the variable name as fg32. [...] and also here would add a comment or use a variable to make it more readable. Same comments for drm_panic_blit32(). [...] /* @@ -256,8 +249,7 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, * @spitch: source pitch in bytes * @height: height of the image to copy, in pixels * @width: width of the image to copy, in pixels - * @fg_color: foreground color, in destination format - * @bg_color: background color, in destination format + * @color: foreground color, in destination format Leaving as fg_color would even be consistent with your comment. Feel free to ignore my comments though if you disagree, the patch looks good to me regardless. sure I will keep the fg_* name Reviewed-by: Javier Martinez Canillas Thanks for the reviews, -- Jocelyn
[PATCH 3/3] drm/panic: Add a kmsg dump screen
Add a kmsg dump option, which will display the last lines of kmsg, and should be similar to fbcon. Add a Kconfig choice for the panic screen, so that the user can choose between this new kmsg dump, or the userfriendly option. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 21 + drivers/gpu/drm/drm_panic.c | 151 +++- 2 files changed, 136 insertions(+), 36 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 9703429de6b9..78d401b55102 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -137,6 +137,27 @@ config DRM_PANIC_DEBUG This is unsafe and should not be enabled on a production build. If in doubt, say "N". +choice + prompt "Panic screen formater" + default DRM_PANIC_SCREEN_USERFRIENDLY + depends on DRM_PANIC + help + This option enable to choose what will be displayed when a kernel + panic occurs. + + config DRM_PANIC_SCREEN_USERFRIENDLY + bool "Default user friendly message" + help + Only a short message telling the user to reboot the system. + + config DRM_PANIC_SCREEN_KMSG + bool "Display the last lines of kmsg" + help + Display kmsg last lines on panic. + Enable if you are a kernel developer, and want to debug a + kernel panic. +endchoice + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 27e26b9d842c..71f6f566d64b 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -63,24 +63,6 @@ struct drm_panic_line { const char *txt; }; -#define PANIC_LINE(s) {.len = sizeof(s) - 1, .txt = s} - -static struct drm_panic_line panic_msg[] = { - PANIC_LINE("KERNEL PANIC !"), - PANIC_LINE(""), - PANIC_LINE("Please reboot your computer."), -}; - -static const struct drm_panic_line logo[] = { - PANIC_LINE(" .--._"), - PANIC_LINE("|o_o | | |"), - PANIC_LINE("|:_/ | | |"), - PANIC_LINE(" // \\ \\ |_|"), - PANIC_LINE(" (| | ) _"), - PANIC_LINE(" /'\\_ _/`\\(_)"), - PANIC_LINE(" \\___)=(___/"), -}; - /* * Color conversion */ @@ -385,16 +367,6 @@ static const u8 *get_char_bitmap(const struct font_desc *font, char c, size_t fo return font->data + (c * font->height) * font_pitch; } -static unsigned int get_max_line_len(const struct drm_panic_line *lines, int len) -{ - int i; - unsigned int max = 0; - - for (i = 0; i < len; i++) - max = max(lines[i].len, max); - return max; -} - /* * Draw a text in a rectangle on a framebuffer. The text is truncated if it overflows the rectangle */ @@ -431,24 +403,48 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb, } } -/* - * Draw the panic message at the center of the screen - */ +#if defined(CONFIG_DRM_PANIC_SCREEN_USERFRIENDLY) + +#define PANIC_LINE(s) {.len = sizeof(s) - 1, .txt = s} + +static struct drm_panic_line panic_msg[] = { + PANIC_LINE("KERNEL PANIC !"), + PANIC_LINE(""), + PANIC_LINE("Please reboot your computer."), +}; + +static const struct drm_panic_line logo[] = { + PANIC_LINE(" .--._"), + PANIC_LINE("|o_o | | |"), + PANIC_LINE("|:_/ | | |"), + PANIC_LINE(" // \\ \\ |_|"), + PANIC_LINE(" (| | ) _"), + PANIC_LINE(" /'\\_ _/`\\(_)"), + PANIC_LINE(" \\___)=(___/"), +}; + +static unsigned int get_max_line_len(const struct drm_panic_line *lines, int len) +{ + int i; + unsigned int max = 0; + + for (i = 0; i < len; i++) + max = max(lines[i].len, max); + return max; +} + static void draw_panic_static(struct drm_scanout_buffer *sb) { size_t msg_lines = ARRAY_SIZE(panic_msg); size_t logo_lines = ARRAY_SIZE(logo); - u32 fg_color = CONFIG_DRM_PANIC_FOREGROUND_COLOR; - u32 bg_color = CONFIG_DRM_PANIC_BACKGROUND_COLOR; + u32 fg_color = convert_from_xrgb(CONFIG_DRM_PANIC_FOREGROUND_COLOR, sb->format->format); + u32 bg_color = convert_from_xrgb(CONFIG_DRM_PANIC_BACKGROUND_COLOR, sb->format->format); const struct font_desc *font = get_default_font(sb->width, sb->height, NULL, NULL); struct drm_rect r_screen, r_logo, r_msg; if (!font) return; - fg_color = conver
[PATCH 1/3] drm/panic: only draw the foreground color in drm_panic_blit()
The whole framebuffer is cleared, so it's useless to rewrite the background colored pixels. It allows to simplify the drawing functions, and prepare the work for the set_pixel() callback. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 63 +++-- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 7ece67086cec..9d95c7eaae83 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -197,37 +197,33 @@ static u32 convert_from_xrgb(u32 color, u32 format) static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u16 fg16, u16 bg16) +u16 color) { unsigned int y, x; - u16 val16; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val16 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg16 : bg16; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, val16); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, color); } static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32, u32 bg32) +u32 color) { unsigned int y, x; - u32 val32; for (y = 0; y < height; y++) { for (x = 0; x < width; x++) { u32 off = y * dpitch + x * 3; - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - - /* write blue-green-red to output in little endianness */ - iosys_map_wr(dmap, off, u8, (val32 & 0x00FF) >> 0); - iosys_map_wr(dmap, off + 1, u8, (val32 & 0xFF00) >> 8); - iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF) >> 16); + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) { + /* write blue-green-red to output in little endianness */ + iosys_map_wr(dmap, off, u8, (color & 0x00FF) >> 0); + iosys_map_wr(dmap, off + 1, u8, (color & 0xFF00) >> 8); + iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF) >> 16); + } } } } @@ -235,17 +231,14 @@ static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32, u32 bg32) +u32 color) { unsigned int y, x; - u32 val32; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, val32); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } /* @@ -256,8 +249,7 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, * @spitch: source pitch in bytes * @height: height of the image to copy, in pixels * @width: width of the image to copy, in pixels - * @fg_color: foreground color, in destination format - * @bg_color: background color, in destination format + * @color: foreground color, in destination format * @pixel_width: pixel width in bytes. * * This can be used to draw a font character, which is a monochrome image, to a @@ -266,21 +258,20 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, - u32 fg_color, u32 bg_color, -
[PATCH 0/3] drm/panic: Add a kmsg dump screen
Add a kmsg dump option, which will display the last lines of kmsg, and should be similar to fbcon. Add a Kconfig choice for the panic screen, so that the user can choose between this new kmsg dump, or the userfriendly option. Patch 1-2 are the same as https://patchwork.freedesktop.org/series/133963/ and are here to avoid conflicts. Jocelyn Falempe (3): drm/panic: only draw the foreground color in drm_panic_blit() drm/panic: Add a set_pixel() callback to drm_scanout_buffer drm/panic: Add a kmsg dump screen drivers/gpu/drm/Kconfig | 21 +++ drivers/gpu/drm/drm_panic.c | 322 +++- include/drm/drm_panic.h | 9 + 3 files changed, 241 insertions(+), 111 deletions(-) base-commit: 86266829ea755f737762ebda614c59b136c8feac -- 2.45.1
[PATCH 2/3] drm/panic: Add a set_pixel() callback to drm_scanout_buffer
This allows drivers to draw the pixel, and handle tiling, or specific color formats. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 120 +++- include/drm/drm_panic.h | 9 +++ 2 files changed, 85 insertions(+), 44 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 9d95c7eaae83..27e26b9d842c 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -241,40 +241,54 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } +static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect *clip, +const u8 *sbuf8, unsigned int spitch, u32 color) +{ + unsigned int y, x; + + for (y = 0; y < drm_rect_height(clip); y++) + for (x = 0; x < drm_rect_width(clip); x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, color); +} + /* * drm_panic_blit - convert a monochrome image to a linear framebuffer - * @dmap: destination iosys_map - * @dpitch: destination pitch in bytes + * @sb: destination scanout buffer + * @clip: destination rectangle * @sbuf8: source buffer, in monochrome format, 8 pixels per byte. * @spitch: source pitch in bytes - * @height: height of the image to copy, in pixels - * @width: width of the image to copy, in pixels * @color: foreground color, in destination format - * @pixel_width: pixel width in bytes. * * This can be used to draw a font character, which is a monochrome image, to a * framebuffer in other supported format. */ -static void drm_panic_blit(struct iosys_map *dmap, unsigned int dpitch, - const u8 *sbuf8, unsigned int spitch, - unsigned int height, unsigned int width, - u32 color, unsigned int pixel_width) +static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip, + const u8 *sbuf8, unsigned int spitch, u32 color) { - switch (pixel_width) { + struct iosys_map map; + + if (sb->set_pixel) + return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, color); + + map = sb->map[0]; + iosys_map_incr(, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]); + + switch (sb->format->cpp[0]) { case 2: - drm_panic_blit16(dmap, dpitch, sbuf8, spitch, -height, width, color); + drm_panic_blit16(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), color); break; case 3: - drm_panic_blit24(dmap, dpitch, sbuf8, spitch, -height, width, color); + drm_panic_blit24(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), color); break; case 4: - drm_panic_blit32(dmap, dpitch, sbuf8, spitch, -height, width, color); + drm_panic_blit32(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), color); break; default: - WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width); + WARN_ONCE(1, "Can't blit with pixel width %d\n", sb->format->cpp[0]); } } @@ -318,33 +332,51 @@ static void drm_panic_fill32(struct iosys_map *dmap, unsigned int dpitch, iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } +static void drm_panic_fill_pixel(struct drm_scanout_buffer *sb, +struct drm_rect *clip, +u32 color) +{ + unsigned int y, x; + + for (y = 0; y < drm_rect_height(clip); y++) + for (x = 0; x < drm_rect_width(clip); x++) + sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, color); +} + /* * drm_panic_fill - Fill a rectangle with a color - * @dmap: destination iosys_map, pointing to the top left corner of the rectangle - * @dpitch: destination pitch in bytes - * @height: height of the rectangle, in pixels - * @width: width of the rectangle, in pixels - * @color: color to fill the rectangle. - * @pixel_width: pixel width in bytes + * @sb: destination scanout buffer + * @clip: destination rectangle + * @color: foreground color, in destination format * * Fill a rectangle with a color, in a linear framebuffer. */ -static void drm_panic_fill(struct iosys_map *dmap, unsigned int dpitch, - unsigned int height, unsigned
Re: [PATCH] drm: renesas: shmobile: Add drm_panic support
On 29/05/2024 15:33, Laurent Pinchart wrote: On Wed, May 29, 2024 at 04:28:44PM +0300, Dmitry Baryshkov wrote: On Wed, May 29, 2024 at 12:55:06PM +0300, Laurent Pinchart wrote: On Wed, May 29, 2024 at 12:25:56PM +0300, Dmitry Baryshkov wrote: On Wed, May 29, 2024 at 12:10:18PM +0300, Laurent Pinchart wrote: Hi Dmitry, On Wed, May 29, 2024 at 11:27:02AM +0300, Dmitry Baryshkov wrote: On Wed, May 29, 2024 at 04:03:20AM +0300, Laurent Pinchart wrote: On Mon, May 27, 2024 at 03:34:48PM +0200, Geert Uytterhoeven wrote: Add support for the drm_panic module, which displays a message on the screen when a kernel panic occurs. Signed-off-by: Geert Uytterhoeven --- Tested on Armadillo-800-EVA. --- drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c index 07ad17d24294d5e6..9d166ab2af8bd231 100644 --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs shmob_drm_plane_helper_funcs = { .atomic_disable = shmob_drm_plane_atomic_disable, }; +static const struct drm_plane_helper_funcs shmob_drm_primary_plane_helper_funcs = { + .atomic_check = shmob_drm_plane_atomic_check, + .atomic_update = shmob_drm_plane_atomic_update, + .atomic_disable = shmob_drm_plane_atomic_disable, + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer, +}; + static const struct drm_plane_funcs shmob_drm_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -310,7 +317,12 @@ struct drm_plane *shmob_drm_plane_create(struct shmob_drm_device *sdev, splane->index = index; - drm_plane_helper_add(>base, _drm_plane_helper_funcs); + if (type == DRM_PLANE_TYPE_PRIMARY) + drm_plane_helper_add(>base, +_drm_primary_plane_helper_funcs); + else + drm_plane_helper_add(>base, +_drm_plane_helper_funcs); It's not very nice to have to provide different operations for the primary and overlay planes. The documentation of drm_fb_dma_get_scanout_buffer() states * @plane: DRM primary plane If the intent is that only primary planes will be used to display the panic message, shouldn't drm_panic_register() skip overlay planes ? It would simplify drivers. What about the drivers where all the planes are actually universal? In such a case the planes registered as primary can easily get replaced by 'overlay' planes. Good point. Another option, if we wanted to avoid duplicating the drm_plane_funcs, would be to add a field to drm_plane to indicate whether the plane is suitable for drm_panic. ... or maybe let the driver decide. For the fully-universal-plane devices we probably want to select the planes which cover the largest part of the CRTC. Are there devices where certain planes can only cover a subset of the CRTC (apart from planes meant for cursors) ? On contemporary MSM devices any plane can cover any part of the screen, including not having a plane that covers the full screen at all. Ah, you meant picking the plane that is currently covering most of the screen. I thought you were talking about devices where some planes were restricted by the hardware to a subset of the CRTC. I agree it would make sense to take both plane position and z-pos, as well as visibility and other related parameters, to pick the plane that is the most visible. Ideally this should be handled in drm_panic, not duplicated in drivers. I'm not sure that drm_panic can figure out reliably on which plane it needs to draw. I think the driver has more information to take the right decision. Also if you prefer, you can add the get_scanout_buffer() callback for all planes (to use the same helper fops), and then filter out in the callback for planes that are not suitable. I just find it cleaner to not register planes that the driver knows they will never be suitable (like cursor planes). static void shmob_atomic_helper_get_scanout_buffer(struct drm_plane *plane, struct drm_scanout_buffer *sb)) { if (plane->type == DRM_PLANE_TYPE_PRIMARY) return drm_fb_dma_get_scanout_buffer(plane, sb); return -EOPNOTSUPP; } .get_scanout_buffer = shmob_atomic_helper_get_scanout_buffer, -- Jocelyn I think that what would matter the most in the end is selecting the plane that is on top of the stack, and that doesn't seem to be addressed by the drm_panic infrastructure. This is getting out of scope for this patch though :-) I don't think this patch should be blocked just for this reason, but I'm a bit bothered by duplicating the ops structure to indicate drm_panic support. return >base; }
Re: [PATCH] drm: renesas: rcar-du: Add drm_panic support for non-vsp
Hi, On 27/05/2024 15:35, Geert Uytterhoeven wrote: Add support for the drm_panic module for DU variants not using the VSP-compositor, to display a message on the screen when a kernel panic occurs. Thanks for your patch, I'm pleased that you find drm_panic useful. That looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Geert Uytterhoeven --- Tested on Koelsch (R-Car M2-W). Support for DU variants using the VSP-compositor is more convoluted, and left to the DU experts. --- drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c index e445fac8e0b46c21..c546ab0805d656f6 100644 --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_plane.c @@ -680,6 +680,12 @@ static const struct drm_plane_helper_funcs rcar_du_plane_helper_funcs = { .atomic_update = rcar_du_plane_atomic_update, }; +static const struct drm_plane_helper_funcs rcar_du_primary_plane_helper_funcs = { + .atomic_check = rcar_du_plane_atomic_check, + .atomic_update = rcar_du_plane_atomic_update, + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer, +}; + static struct drm_plane_state * rcar_du_plane_atomic_duplicate_state(struct drm_plane *plane) { @@ -812,8 +818,12 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) if (ret < 0) return ret; - drm_plane_helper_add(>plane, -_du_plane_helper_funcs); + if (type == DRM_PLANE_TYPE_PRIMARY) + drm_plane_helper_add(>plane, + _du_primary_plane_helper_funcs); + else + drm_plane_helper_add(>plane, +_du_plane_helper_funcs); drm_plane_create_alpha_property(>plane);
Re: [PATCH] drm: renesas: shmobile: Add drm_panic support
On 27/05/2024 15:34, Geert Uytterhoeven wrote: Add support for the drm_panic module, which displays a message on the screen when a kernel panic occurs. Thanks for your patch, I'm pleased that you find drm_panic useful. That looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Geert Uytterhoeven --- Tested on Armadillo-800-EVA. --- drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c index 07ad17d24294d5e6..9d166ab2af8bd231 100644 --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_plane.c @@ -273,6 +273,13 @@ static const struct drm_plane_helper_funcs shmob_drm_plane_helper_funcs = { .atomic_disable = shmob_drm_plane_atomic_disable, }; +static const struct drm_plane_helper_funcs shmob_drm_primary_plane_helper_funcs = { + .atomic_check = shmob_drm_plane_atomic_check, + .atomic_update = shmob_drm_plane_atomic_update, + .atomic_disable = shmob_drm_plane_atomic_disable, + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer, +}; + static const struct drm_plane_funcs shmob_drm_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -310,7 +317,12 @@ struct drm_plane *shmob_drm_plane_create(struct shmob_drm_device *sdev, splane->index = index; - drm_plane_helper_add(>base, _drm_plane_helper_funcs); + if (type == DRM_PLANE_TYPE_PRIMARY) + drm_plane_helper_add(>base, +_drm_primary_plane_helper_funcs); + else + drm_plane_helper_add(>base, +_drm_plane_helper_funcs); return >base; }
[PATCH 3/5] drm/panic: Add a set_pixel() callback to drm_scanout_buffer
This allows drivers to draw the pixel, and handle tiling, or specific color formats. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 120 +++- include/drm/drm_panic.h | 9 +++ 2 files changed, 85 insertions(+), 44 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 09d7d45f80c2..94558087bba3 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -251,40 +251,54 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } +static void drm_panic_blit_pixel(struct drm_scanout_buffer *sb, struct drm_rect *clip, +const u8 *sbuf8, unsigned int spitch, u32 color) +{ + unsigned int y, x; + + for (y = 0; y < drm_rect_height(clip); y++) + for (x = 0; x < drm_rect_width(clip); x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, color); +} + /* * drm_panic_blit - convert a monochrome image to a linear framebuffer - * @dmap: destination iosys_map - * @dpitch: destination pitch in bytes + * @sb: destination scanout buffer + * @clip: destination rectangle * @sbuf8: source buffer, in monochrome format, 8 pixels per byte. * @spitch: source pitch in bytes - * @height: height of the image to copy, in pixels - * @width: width of the image to copy, in pixels * @color: foreground color, in destination format - * @pixel_width: pixel width in bytes. * * This can be used to draw a font character, which is a monochrome image, to a * framebuffer in other supported format. */ -static void drm_panic_blit(struct iosys_map *dmap, unsigned int dpitch, - const u8 *sbuf8, unsigned int spitch, - unsigned int height, unsigned int width, - u32 color, unsigned int pixel_width) +static void drm_panic_blit(struct drm_scanout_buffer *sb, struct drm_rect *clip, + const u8 *sbuf8, unsigned int spitch, u32 color) { - switch (pixel_width) { + struct iosys_map map; + + if (sb->set_pixel) + return drm_panic_blit_pixel(sb, clip, sbuf8, spitch, color); + + map = sb->map[0]; + iosys_map_incr(, clip->y1 * sb->pitch[0] + clip->x1 * sb->format->cpp[0]); + + switch (sb->format->cpp[0]) { case 2: - drm_panic_blit16(dmap, dpitch, sbuf8, spitch, -height, width, color); + drm_panic_blit16(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), color); break; case 3: - drm_panic_blit24(dmap, dpitch, sbuf8, spitch, -height, width, color); + drm_panic_blit24(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), color); break; case 4: - drm_panic_blit32(dmap, dpitch, sbuf8, spitch, -height, width, color); + drm_panic_blit32(, sb->pitch[0], sbuf8, spitch, +drm_rect_height(clip), drm_rect_width(clip), color); break; default: - WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width); + WARN_ONCE(1, "Can't blit with pixel width %d\n", sb->format->cpp[0]); } } @@ -328,33 +342,51 @@ static void drm_panic_fill32(struct iosys_map *dmap, unsigned int dpitch, iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } +static void drm_panic_fill_pixel(struct drm_scanout_buffer *sb, +struct drm_rect *clip, +u32 color) +{ + unsigned int y, x; + + for (y = 0; y < drm_rect_height(clip); y++) + for (x = 0; x < drm_rect_width(clip); x++) + sb->set_pixel(sb, clip->x1 + x, clip->y1 + y, color); +} + /* * drm_panic_fill - Fill a rectangle with a color - * @dmap: destination iosys_map, pointing to the top left corner of the rectangle - * @dpitch: destination pitch in bytes - * @height: height of the rectangle, in pixels - * @width: width of the rectangle, in pixels - * @color: color to fill the rectangle. - * @pixel_width: pixel width in bytes + * @sb: destination scanout buffer + * @clip: destination rectangle + * @color: foreground color, in destination format * * Fill a rectangle with a color, in a linear framebuffer. */ -static void drm_panic_fill(struct iosys_map *dmap, unsigned int dpitch, - unsigned int height, unsigned
[PATCH 5/5] drm/nouveau: Add drm_panic support for nv50+
Add drm_panic support, for nv50+ cards. It's enough to get the panic screen while running Gnome/Wayland on a GTX 1650. It doesn't support multi-plane or compressed format. Support for other formats and older cards will come later. Tiling is only tested on GTX1650, and might be wrong for other cards. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 127 +++- 1 file changed, 125 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c index 7a2cceaee6e9..dd7aafb9198a 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c @@ -30,11 +30,16 @@ #include #include +#include + #include #include #include -#include #include +#include +#include +#include +#include #include "nouveau_bo.h" #include "nouveau_gem.h" @@ -577,6 +582,113 @@ nv50_wndw_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) return 0; } +/* Values found on GTX1650 */ +/* blocks level 0, 4x4 pixels */ +#define BL0_W 4 +/* blocks level 1, 8x8 pixels */ +#define BL1_W 8 +/* blocks level 2, Group of Bytes? 16x128 pixels */ +#define BL2_W 16 + +/* get the offset in bytes inside the framebuffer, after taking tiling into account */ +static unsigned int nv50_get_tiled_offset(struct drm_scanout_buffer *sb, unsigned int gobs, + unsigned int x, unsigned int y, unsigned int px_width) +{ + u32 blk2_x, blk2_y, bl2sz; + u32 blk1_x, blk1_y, bl1sz; + u32 blk0_x, blk0_y, bl0sz; + u32 nblk2w, bl2_h, off; + + /* fixme - block2 height depends of the "Group of Bytes" value */ + bl2_h = BL1_W * gobs; + + bl0sz = BL0_W * BL0_W * px_width; + bl1sz = BL1_W * BL1_W * px_width; + bl2sz = BL2_W * bl2_h * px_width; + + /* block level 2 coordinate */ + blk2_x = x / BL2_W; + blk2_y = y / bl2_h; + + x = x % BL2_W; + y = y % bl2_h; + + /* block level 1 coordinate */ + blk1_x = x / BL1_W; + blk1_y = y / BL1_W; + + x = x % BL1_W; + y = y % BL1_W; + + /* block level 0 coordinate */ + blk0_x = x / BL0_W; + blk0_y = y / BL0_W; + + x = x % BL0_W; + y = y % BL0_W; + + nblk2w = DIV_ROUND_UP(sb->width, BL2_W); + + off = ((blk2_y * nblk2w) + blk2_x) * bl2sz; + off += ((blk1_y * 2) + blk1_x) * bl1sz; + off += (blk0_y * 2 + blk0_x) * bl0sz; + off += (x + y * BL0_W) * px_width; + + return off; +} + +static void nv50_set_pixel(struct drm_scanout_buffer *sb, unsigned int x, unsigned int y, u32 color) +{ + struct drm_framebuffer *fb = sb->private; + unsigned int off; + /* According to DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D documentation, +* the last 4 bits of modifier is log2(height) of each block, in GOBs +*/ + unsigned int gobs = 1 << (fb->modifier & 0xf); + + off = nv50_get_tiled_offset(sb, gobs, x, y, sb->format->cpp[0]); + iosys_map_wr(>map[0], off, u32, color); +} + +static int +nv50_wndw_get_scanout_buffer(struct drm_plane *plane, struct drm_scanout_buffer *sb) +{ + struct drm_framebuffer *fb; + struct nouveau_bo *nvbo; + + if (!plane->state || !plane->state->fb) + return -EINVAL; + + fb = plane->state->fb; + nvbo = nouveau_gem_object(fb->obj[0]); + + /* Don't support compressed format, or multiplane yet */ + if (nvbo->comp || fb->format->num_planes != 1) + return -EOPNOTSUPP; + + if (nouveau_bo_map(nvbo)) { + pr_warn("nouveau bo map failed, panic won't be displayed\n"); + return -ENOMEM; + } + + if (nvbo->kmap.bo_kmap_type & TTM_BO_MAP_IOMEM_MASK) + iosys_map_set_vaddr_iomem(>map[0], nvbo->kmap.virtual); + else + iosys_map_set_vaddr(>map[0], nvbo->kmap.virtual); + + sb->height = fb->height; + sb->width = fb->width; + sb->pitch[0] = fb->pitches[0]; + sb->format = fb->format; + + /* If tiling is enabled, use the set_pixel() to display correctly */ + if (fb->modifier & 0xf) { + sb->private = (void *) fb; + sb->set_pixel = nv50_set_pixel; + } + return 0; +} + static const struct drm_plane_helper_funcs nv50_wndw_helper = { .prepare_fb = nv50_wndw_prepare_fb, @@ -584,6 +696,14 @@ nv50_wndw_helper = { .atomic_check = nv50_wndw_atomic_check, }; +static const struct drm_plane_helper_funcs +nv50_wndw_primary_helper = { + .prepare_fb = nv50_wndw_prepare_fb, + .cleanup_fb = nv50_wndw_cleanup_fb, + .atomic_check = nv50_wndw_atomic_check, + .get_scanout_buffer = nv50_wndw_get_scanout_buffer, +};
[PATCH 4/5] drm/panic: add a private pointer to drm_scanout_buffer
So you can pass a parameter to the set_pixel() callback. Signed-off-by: Jocelyn Falempe --- include/drm/drm_panic.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/include/drm/drm_panic.h b/include/drm/drm_panic.h index 73bb3f3d9ed9..f7c32d64af5f 100644 --- a/include/drm/drm_panic.h +++ b/include/drm/drm_panic.h @@ -51,6 +51,12 @@ struct drm_scanout_buffer { */ unsigned int pitch[DRM_FORMAT_MAX_PLANES]; + /** +* @private: Optional pointer to some private data you want to pass to +* the set_pixel() function. +*/ + void *private; + /** * @set_pixel: Optional function, to set a pixel color on the * framebuffer. It allows to handle special tiling format inside the -- 2.45.0
[PATCH 2/5] drm/panic: only draw the foreground color in drm_panic_blit()
The whole framebuffer is cleared, so it's useless to rewrite the background colored pixels. It allows to simplify the drawing functions, and prepare the work for the set_pixel() callback. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 63 +++-- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 78fdecfede84..09d7d45f80c2 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -207,37 +207,33 @@ static u32 convert_from_xrgb(u32 color, u32 format) static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u16 fg16, u16 bg16) +u16 color) { unsigned int y, x; - u16 val16; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val16 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg16 : bg16; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, val16); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, color); } static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32, u32 bg32) +u32 color) { unsigned int y, x; - u32 val32; for (y = 0; y < height; y++) { for (x = 0; x < width; x++) { u32 off = y * dpitch + x * 3; - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - - /* write blue-green-red to output in little endianness */ - iosys_map_wr(dmap, off, u8, (val32 & 0x00FF) >> 0); - iosys_map_wr(dmap, off + 1, u8, (val32 & 0xFF00) >> 8); - iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF) >> 16); + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) { + /* write blue-green-red to output in little endianness */ + iosys_map_wr(dmap, off, u8, (color & 0x00FF) >> 0); + iosys_map_wr(dmap, off + 1, u8, (color & 0xFF00) >> 8); + iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF) >> 16); + } } } } @@ -245,17 +241,14 @@ static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 fg32, u32 bg32) +u32 color) { unsigned int y, x; - u32 val32; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, val32); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } /* @@ -266,8 +259,7 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, * @spitch: source pitch in bytes * @height: height of the image to copy, in pixels * @width: width of the image to copy, in pixels - * @fg_color: foreground color, in destination format - * @bg_color: background color, in destination format + * @color: foreground color, in destination format * @pixel_width: pixel width in bytes. * * This can be used to draw a font character, which is a monochrome image, to a @@ -276,21 +268,20 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, - u32 fg_color, u32 bg_color, -
[PATCH 1/5] drm/panic: Add ABGR2101010 support
Add support for ABGR2101010, used by the nouveau driver. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 7ece67086cec..78fdecfede84 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -152,6 +152,14 @@ static u32 convert_xrgb_to_argb2101010(u32 pix) return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) & 0x00300C03); } +static u32 convert_xrgb_to_abgr2101010(u32 pix) +{ + pix = ((pix & 0x00FF) >> 14) | + ((pix & 0xFF00) << 4) | + ((pix & 0x00FF) << 22); + return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) & 0x00300C03); +} + /* * convert_from_xrgb - convert one pixel from xrgb to the desired format * @color: input color, in xrgb format @@ -185,6 +193,8 @@ static u32 convert_from_xrgb(u32 color, u32 format) return convert_xrgb_to_xrgb2101010(color); case DRM_FORMAT_ARGB2101010: return convert_xrgb_to_argb2101010(color); + case DRM_FORMAT_ABGR2101010: + return convert_xrgb_to_abgr2101010(color); default: WARN_ONCE(1, "Can't convert to %p4cc\n", ); return 0; -- 2.45.0
[PATCH 0/5] drm/nouveau: Add drm_panic support for nv50+
This series adds basic drm_panic support for nouveau. Patches 1-4 Add missing bits in drm_panic (ABGR2101010, set_pixel() for tiling, ...) Patch 5 registers nouveau to drm_panic, and handle tiling. I've tested on a GTX1650, while running Gnome/Wayland desktop. I didn't find documentation about nVidia tiling, so it may not work on other GPU than GTX1650. To test it, you need to build your kernel with CONFIG_DRM_PANIC=y, and run echo c > /proc/sysrq-trigger or you can enable CONFIG_DRM_PANIC_DEBUG and run echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0 Best regards, -- Jocelyn Jocelyn Falempe (5): drm/panic: Add ABGR2101010 support drm/panic: only draw the foreground color in drm_panic_blit() drm/panic: Add a set_pixel() callback to drm_scanout_buffer drm/panic: add a private pointer to drm_scanout_buffer drm/nouveau: Add drm_panic support for nv50+ drivers/gpu/drm/drm_panic.c | 183 ++-- drivers/gpu/drm/nouveau/dispnv50/wndw.c | 127 +++- include/drm/drm_panic.h | 15 ++ 3 files changed, 247 insertions(+), 78 deletions(-) base-commit: 484436ec5c2bffe8f346a09ae1cbc4cbf5e50005 -- 2.45.0
Re: [PATCH v2 2/2] drm/mgag200: Add an option to disable Write-Combine
On 17/05/2024 17:16, Thomas Zimmermann wrote: Hi Am 17.05.24 um 17:09 schrieb Jocelyn Falempe: Unfortunately, the G200 ioburst workaround doesn't work on some servers like Dell poweredge XR11, XR5610, or HPE XL260. In this case completely disabling WC is the only option to achieve low-latency. So this adds a new Kconfig option to disable WC mapping of the G200. Signed-off-by: Jocelyn Falempe Reviewed-by: Thomas Zimmermann Thanks a lot for the fix. Thanks for the review, I just merged it to drm-misc-next. Best regards Thomas --- drivers/gpu/drm/mgag200/Kconfig | 10 ++ drivers/gpu/drm/mgag200/mgag200_drv.c | 6 ++ 2 files changed, 16 insertions(+) diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig index b28c5e4828f47..3096944a8f0ab 100644 --- a/drivers/gpu/drm/mgag200/Kconfig +++ b/drivers/gpu/drm/mgag200/Kconfig @@ -11,3 +11,13 @@ config DRM_MGAG200 MGA G200 desktop chips and the server variants. It requires 0.3.0 of the modesetting userspace driver, and a version of mga driver that will fail on KMS enabled devices. + +config DRM_MGAG200_DISABLE_WRITECOMBINE + bool "Disable Write Combine mapping of VRAM" + depends on DRM_MGAG200 && PREEMPT_RT + help + The VRAM of the G200 is mapped with Write-Combine to improve + performances. This can interfere with real-time tasks; even if they + are running on other CPU cores than the graphics output. + Enable this option only if you run realtime tasks on a server with a + Matrox G200. \ No newline at end of file diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 3883f25ca4d8b..62080cf0f2da4 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -146,12 +146,18 @@ int mgag200_device_preinit(struct mga_device *mdev) } mdev->vram_res = res; +#if defined(CONFIG_DRM_MGAG200_DISABLE_WRITECOMBINE) + mdev->vram = devm_ioremap(dev->dev, res->start, resource_size(res)); + if (!mdev->vram) + return -ENOMEM; +#else mdev->vram = devm_ioremap_wc(dev->dev, res->start, resource_size(res)); if (!mdev->vram) return -ENOMEM; /* Don't fail on errors, but performance might be reduced. */ devm_arch_phys_wc_add(dev->dev, res->start, resource_size(res)); +#endif return 0; }
[PATCH v2 2/2] drm/mgag200: Add an option to disable Write-Combine
Unfortunately, the G200 ioburst workaround doesn't work on some servers like Dell poweredge XR11, XR5610, or HPE XL260. In this case completely disabling WC is the only option to achieve low-latency. So this adds a new Kconfig option to disable WC mapping of the G200. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/mgag200/Kconfig | 10 ++ drivers/gpu/drm/mgag200/mgag200_drv.c | 6 ++ 2 files changed, 16 insertions(+) diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig index b28c5e4828f47..3096944a8f0ab 100644 --- a/drivers/gpu/drm/mgag200/Kconfig +++ b/drivers/gpu/drm/mgag200/Kconfig @@ -11,3 +11,13 @@ config DRM_MGAG200 MGA G200 desktop chips and the server variants. It requires 0.3.0 of the modesetting userspace driver, and a version of mga driver that will fail on KMS enabled devices. + +config DRM_MGAG200_DISABLE_WRITECOMBINE + bool "Disable Write Combine mapping of VRAM" + depends on DRM_MGAG200 && PREEMPT_RT + help + The VRAM of the G200 is mapped with Write-Combine to improve + performances. This can interfere with real-time tasks; even if they + are running on other CPU cores than the graphics output. + Enable this option only if you run realtime tasks on a server with a + Matrox G200. \ No newline at end of file diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 3883f25ca4d8b..62080cf0f2da4 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -146,12 +146,18 @@ int mgag200_device_preinit(struct mga_device *mdev) } mdev->vram_res = res; +#if defined(CONFIG_DRM_MGAG200_DISABLE_WRITECOMBINE) + mdev->vram = devm_ioremap(dev->dev, res->start, resource_size(res)); + if (!mdev->vram) + return -ENOMEM; +#else mdev->vram = devm_ioremap_wc(dev->dev, res->start, resource_size(res)); if (!mdev->vram) return -ENOMEM; /* Don't fail on errors, but performance might be reduced. */ devm_arch_phys_wc_add(dev->dev, res->start, resource_size(res)); +#endif return 0; } -- 2.45.0
[PATCH v2 1/2] Revert "drm/mgag200: Add a workaround for low-latency"
This reverts commit bfa4437fd3938ae2e186e7664b2db65bb8775670. This workaround doesn't work reliably on all servers. I'll replace it with an option to disable Write-Combine, which has more impact on performance, but fix the latency issue on all hardware. Signed-off-by: Jocelyn Falempe Reviewed-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/Kconfig| 12 drivers/gpu/drm/mgag200/mgag200_drv.c | 17 - drivers/gpu/drm/mgag200/mgag200_mode.c | 8 3 files changed, 37 deletions(-) diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig index 5e4d48df4854c..b28c5e4828f47 100644 --- a/drivers/gpu/drm/mgag200/Kconfig +++ b/drivers/gpu/drm/mgag200/Kconfig @@ -11,15 +11,3 @@ config DRM_MGAG200 MGA G200 desktop chips and the server variants. It requires 0.3.0 of the modesetting userspace driver, and a version of mga driver that will fail on KMS enabled devices. - -config DRM_MGAG200_IOBURST_WORKAROUND - bool "Disable buffer caching" - depends on DRM_MGAG200 && PREEMPT_RT && X86 - help - Enable a workaround to avoid I/O bursts within the mgag200 driver at - the expense of overall display performance. - It restores the map_wc = true; - return >base; -} -#endif - /* * DRM driver */ @@ -113,9 +99,6 @@ static const struct drm_driver mgag200_driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, -#if defined(CONFIG_DRM_MGAG200_IOBURST_WORKAROUND) - .gem_create_object = mgag200_create_object, -#endif DRM_GEM_SHMEM_DRIVER_OPS, }; diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index fc54851d3384d..d3d820f7a77d7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -13,7 +13,6 @@ #include #include -#include #include #include #include @@ -438,13 +437,6 @@ static void mgag200_handle_damage(struct mga_device *mdev, const struct iosys_ma iosys_map_incr(, drm_fb_clip_offset(fb->pitches[0], fb->format, clip)); drm_fb_memcpy(, fb->pitches, vmap, fb, clip); - - /* Flushing the cache greatly improves latency on x86_64 */ -#if defined(CONFIG_DRM_MGAG200_IOBURST_WORKAROUND) - if (!vmap->is_iomem) - drm_clflush_virt_range(vmap->vaddr + clip->y1 * fb->pitches[0], - drm_rect_height(clip) * fb->pitches[0]); -#endif } /* -- 2.45.0
[PATCH v2 0/2] drm/mgag200: Add an option to disable Write-Combine
Unfortunately, the G200 ioburst workaround doesn't work on some servers like Dell poweredge XR11, XR5610, or HPE XL260 In this case completely disabling WC is the only option to achieve low-latency. It's probably useless to maintain two workarounds for this, so I reverted commit bfa4437fd3938 drm/mgag200: Add a workaround for low-latency and added a new and simpler option, to just disable Write-Combine. Jocelyn Falempe (2): Revert "drm/mgag200: Add a workaround for low-latency" drm/mgag200: Add an option to disable Write-Combine drivers/gpu/drm/mgag200/Kconfig| 18 -- drivers/gpu/drm/mgag200/mgag200_drv.c | 23 ++- drivers/gpu/drm/mgag200/mgag200_mode.c | 8 3 files changed, 14 insertions(+), 35 deletions(-) base-commit: 3179338750d83877bbc491493032bdf192266ad9 -- 2.45.0
Re: [PATCH 2/2] drm/mgag200: Add an option to disable Write-Combine
On 17/05/2024 11:39, Thomas Zimmermann wrote: Hi, just nits below. Am 16.05.24 um 18:17 schrieb Jocelyn Falempe: Unfortunately, the G200 ioburst workaround doesn't work on some servers like Dell poweredge XR11, XR5610, or HPE XL260 In this case completely disabling WC is the only option to achieve low-latency. So this adds a new Kconfig option, to disable WC mapping of the G200 The formatting look off. Maybe make this one single paragraph. Ok, No comma after 'option'. Ok, Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/mgag200/Kconfig | 10 ++ drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +++ 2 files changed, 17 insertions(+) diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig index b28c5e4828f47..73ab5730b74d9 100644 --- a/drivers/gpu/drm/mgag200/Kconfig +++ b/drivers/gpu/drm/mgag200/Kconfig @@ -11,3 +11,13 @@ config DRM_MGAG200 MGA G200 desktop chips and the server variants. It requires 0.3.0 of the modesetting userspace driver, and a version of mga driver that will fail on KMS enabled devices. + +config DRM_MGAG200_DISABLE_WRITECOMBINE + bool "Disable Write Combine mapping of VRAM" + depends on DRM_MGAG200 && PREEMPT_RT + help + The VRAM of the G200 is mapped with Write-Combine, to improve No comma after Write-Combine Ok + performances. However this increases the system latency a lot, even Just say "This can interfere with real-time tasks; even if they are running on other CPU cores then the graphics output." Ok + for realtime tasks running on other CPU cores. Typically 40us-80us + latencies are measured with hwlat when Write Combine is enabled. Leave out the next sentence: "Typically ..." The measureed numbers depend on the hardware and everyone is encouraged to test on their own system. You could mention the numbers in the commit description, as you already mention the affected systems there. Ok + Recommended if you run realtime tasks on a server with a Matrox G200. I still think that we should not encourage anyone to use this option. Maybe say "Enable this option only if you run..." Agreed, I will change this. \ No newline at end of file diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 3883f25ca4d8b..7461e3f984eff 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -146,12 +146,19 @@ int mgag200_device_preinit(struct mga_device *mdev) } mdev->vram_res = res; +#if defined(CONFIG_DRM_MGAG200_DISABLE_WRITECOMBINE) + drm_info(dev, "Disable Write Combine\n"); I would not print this drm_info() here. The user has selected the config option, so they should know what happens. It's also listed in /proc/mtrr IIRC. Sure, I can remove the drm_info(). Thanks for the review, I will send a v2 shortly. Best regards Thomas + mdev->vram = devm_ioremap(dev->dev, res->start, resource_size(res)); + if (!mdev->vram) + return -ENOMEM; +#else mdev->vram = devm_ioremap_wc(dev->dev, res->start, resource_size(res)); if (!mdev->vram) return -ENOMEM; /* Don't fail on errors, but performance might be reduced. */ devm_arch_phys_wc_add(dev->dev, res->start, resource_size(res)); +#endif return 0; }
[PATCH 0/2] drm/mgag200: Add an option to disable Write-Combine
Unfortunately, the G200 ioburst workaround doesn't work on some servers like Dell poweredge XR11, XR5610, or HPE XL260 In this case completely disabling WC is the only option to achieve low-latency. It's probably useless to maintain two workarounds for this, so I reverted commit bfa4437fd3938 drm/mgag200: Add a workaround for low-latency and added a new and simpler option, to just disable Write-Combine. Jocelyn Falempe (2): Revert "drm/mgag200: Add a workaround for low-latency" drm/mgag200: Add an option to disable Write-Combine drivers/gpu/drm/mgag200/Kconfig| 18 -- drivers/gpu/drm/mgag200/mgag200_drv.c | 24 +++- drivers/gpu/drm/mgag200/mgag200_mode.c | 8 3 files changed, 15 insertions(+), 35 deletions(-) base-commit: 2c3d1bd284c5141a85188f48e7f42112e81ffcd8 -- 2.45.0
[PATCH 1/2] Revert "drm/mgag200: Add a workaround for low-latency"
This reverts commit bfa4437fd3938ae2e186e7664b2db65bb8775670. This workaround doesn't work reliably on all servers. I'll replace it with an option to disable Write-Combine, which has more impact on performance, but fix the latency issue on all hardware. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/mgag200/Kconfig| 12 drivers/gpu/drm/mgag200/mgag200_drv.c | 17 - drivers/gpu/drm/mgag200/mgag200_mode.c | 8 3 files changed, 37 deletions(-) diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig index 5e4d48df4854c..b28c5e4828f47 100644 --- a/drivers/gpu/drm/mgag200/Kconfig +++ b/drivers/gpu/drm/mgag200/Kconfig @@ -11,15 +11,3 @@ config DRM_MGAG200 MGA G200 desktop chips and the server variants. It requires 0.3.0 of the modesetting userspace driver, and a version of mga driver that will fail on KMS enabled devices. - -config DRM_MGAG200_IOBURST_WORKAROUND - bool "Disable buffer caching" - depends on DRM_MGAG200 && PREEMPT_RT && X86 - help - Enable a workaround to avoid I/O bursts within the mgag200 driver at - the expense of overall display performance. - It restores the map_wc = true; - return >base; -} -#endif - /* * DRM driver */ @@ -113,9 +99,6 @@ static const struct drm_driver mgag200_driver = { .major = DRIVER_MAJOR, .minor = DRIVER_MINOR, .patchlevel = DRIVER_PATCHLEVEL, -#if defined(CONFIG_DRM_MGAG200_IOBURST_WORKAROUND) - .gem_create_object = mgag200_create_object, -#endif DRM_GEM_SHMEM_DRIVER_OPS, }; diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index fc54851d3384d..d3d820f7a77d7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -13,7 +13,6 @@ #include #include -#include #include #include #include @@ -438,13 +437,6 @@ static void mgag200_handle_damage(struct mga_device *mdev, const struct iosys_ma iosys_map_incr(, drm_fb_clip_offset(fb->pitches[0], fb->format, clip)); drm_fb_memcpy(, fb->pitches, vmap, fb, clip); - - /* Flushing the cache greatly improves latency on x86_64 */ -#if defined(CONFIG_DRM_MGAG200_IOBURST_WORKAROUND) - if (!vmap->is_iomem) - drm_clflush_virt_range(vmap->vaddr + clip->y1 * fb->pitches[0], - drm_rect_height(clip) * fb->pitches[0]); -#endif } /* -- 2.45.0
[PATCH 2/2] drm/mgag200: Add an option to disable Write-Combine
Unfortunately, the G200 ioburst workaround doesn't work on some servers like Dell poweredge XR11, XR5610, or HPE XL260 In this case completely disabling WC is the only option to achieve low-latency. So this adds a new Kconfig option, to disable WC mapping of the G200 Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/mgag200/Kconfig | 10 ++ drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +++ 2 files changed, 17 insertions(+) diff --git a/drivers/gpu/drm/mgag200/Kconfig b/drivers/gpu/drm/mgag200/Kconfig index b28c5e4828f47..73ab5730b74d9 100644 --- a/drivers/gpu/drm/mgag200/Kconfig +++ b/drivers/gpu/drm/mgag200/Kconfig @@ -11,3 +11,13 @@ config DRM_MGAG200 MGA G200 desktop chips and the server variants. It requires 0.3.0 of the modesetting userspace driver, and a version of mga driver that will fail on KMS enabled devices. + +config DRM_MGAG200_DISABLE_WRITECOMBINE + bool "Disable Write Combine mapping of VRAM" + depends on DRM_MGAG200 && PREEMPT_RT + help + The VRAM of the G200 is mapped with Write-Combine, to improve + performances. However this increases the system latency a lot, even + for realtime tasks running on other CPU cores. Typically 40us-80us + latencies are measured with hwlat when Write Combine is enabled. + Recommended if you run realtime tasks on a server with a Matrox G200. \ No newline at end of file diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 3883f25ca4d8b..7461e3f984eff 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -146,12 +146,19 @@ int mgag200_device_preinit(struct mga_device *mdev) } mdev->vram_res = res; +#if defined(CONFIG_DRM_MGAG200_DISABLE_WRITECOMBINE) + drm_info(dev, "Disable Write Combine\n"); + mdev->vram = devm_ioremap(dev->dev, res->start, resource_size(res)); + if (!mdev->vram) + return -ENOMEM; +#else mdev->vram = devm_ioremap_wc(dev->dev, res->start, resource_size(res)); if (!mdev->vram) return -ENOMEM; /* Don't fail on errors, but performance might be reduced. */ devm_arch_phys_wc_add(dev->dev, res->start, resource_size(res)); +#endif return 0; } -- 2.45.0
Re: [PATCH 00/10] drm/mgag200: Refactor DDC code
Thanks for this refactor of mgag200. for the whole series: Reviewed-by: Jocelyn Falempe -- Jocelyn On 13/05/2024 14:51, Thomas Zimmermann wrote: Clean up a the driver's DDC code, make it simpler, more robust, and mostly self contained. The patches in this patchset have previously been sent as part of rev 1 of [1]. Patches 1 and 2 fix long-standing problems in the DDC code. Patches 3 to 9 refactor the DDC code. The code then keeps its data structures internal, acquires locks automatically and is much more readable overall. Patch 10 replaces driver code with an equivalent helper. Tested on various Matrox hardware. [1] https://patchwork.freedesktop.org/series/131977/ Thomas Zimmermann (10): drm/mgag200: Set DDC timeout in milliseconds drm/mgag200: Bind I2C lifetime to DRM device drm/mgag200: Store pointer to struct mga_device in struct mga_i2c_chan drm/mgag200: Allocate instance of struct mga_i2c_chan dynamically drm/mgag200: Inline mgag200_i2c_init() drm/mgag200: Replace struct mga_i2c_chan with struct mgag200_ddc drm/mgag200: Rename mgag200_i2c.c to mgag200_ddc.c drm/mgag200: Rename struct i2c_algo_bit_data callbacks drm/mgag200: Acquire I/O-register lock in DDC code drm/mgag200: Use drm_connector_helper_get_modes() drivers/gpu/drm/mgag200/Makefile | 2 +- drivers/gpu/drm/mgag200/mgag200_ddc.c | 179 ++ drivers/gpu/drm/mgag200/mgag200_ddc.h | 11 ++ drivers/gpu/drm/mgag200/mgag200_drv.h | 18 +-- drivers/gpu/drm/mgag200/mgag200_g200.c| 11 +- drivers/gpu/drm/mgag200/mgag200_g200eh.c | 11 +- drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 11 +- drivers/gpu/drm/mgag200/mgag200_g200er.c | 11 +- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 11 +- drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 11 +- drivers/gpu/drm/mgag200/mgag200_g200se.c | 11 +- drivers/gpu/drm/mgag200/mgag200_g200wb.c | 11 +- drivers/gpu/drm/mgag200/mgag200_i2c.c | 129 drivers/gpu/drm/mgag200/mgag200_mode.c| 27 +--- 14 files changed, 241 insertions(+), 213 deletions(-) create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.c create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.h delete mode 100644 drivers/gpu/drm/mgag200/mgag200_i2c.c
Re: [PATCH] lib/fonts: Allow to select fonts for drm_panic
On 30/04/2024 14:58, Daniel Vetter wrote: On Fri, Apr 19, 2024 at 03:20:19PM +0200, Jocelyn Falempe wrote: drm_panic has been introduced recently, and uses the same fonts as FRAMEBUFFER_CONSOLE. Signed-off-by: Jocelyn Falempe Acked-by: Daniel Vetter lib/fonts/ doesn't seem to have a designated maintainer, so please push this through drm-misc. Thanks for the review, I just merged it to drm-misc-next. -- Jocelyn Thanks, Sima --- lib/fonts/Kconfig | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/fonts/Kconfig b/lib/fonts/Kconfig index 7e945fdcbf11..befcb463f738 100644 --- a/lib/fonts/Kconfig +++ b/lib/fonts/Kconfig @@ -10,7 +10,7 @@ if FONT_SUPPORT config FONTS bool "Select compiled-in fonts" - depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE + depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || DRM_PANIC help Say Y here if you would like to use fonts other than the default your frame buffer console usually use. @@ -23,7 +23,7 @@ config FONTS config FONT_8x8 bool "VGA 8x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE + depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS help This is the "high resolution" font for the VGA frame buffer (the one @@ -46,7 +46,7 @@ config FONT_8x16 config FONT_6x11 bool "Mac console 6x11 font (not supported by all drivers)" if FONTS - depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE + depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS && MAC help Small console font with Macintosh-style high-half glyphs. Some Mac @@ -54,7 +54,7 @@ config FONT_6x11 config FONT_7x14 bool "console 7x14 font (not supported by all drivers)" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC help Console font with characters just a bit smaller than the default. If the standard 8x16 font is a little too big for you, say Y. @@ -62,7 +62,7 @@ config FONT_7x14 config FONT_PEARL_8x8 bool "Pearl (old m68k) console 8x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS && AMIGA help Small console font with PC-style control-character and high-half @@ -70,7 +70,7 @@ config FONT_PEARL_8x8 config FONT_ACORN_8x8 bool "Acorn console 8x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS && ARM && ARCH_ACORN help Small console font with PC-style control characters and high-half @@ -90,7 +90,7 @@ config FONT_6x10 config FONT_10x18 bool "console 10x18 font (not supported by all drivers)" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC help This is a high resolution console font for machines with very big letters. It fits between the sun 12x22 and the normal 8x16 font. @@ -105,7 +105,7 @@ config FONT_SUN8x16 config FONT_SUN12x22 bool "Sparc console 12x22 font (not supported by all drivers)" - depends on FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC) + depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC help This is the high resolution console font for Sun machines with very big letters (like the letters used in the SPARC PROM). If the @@ -113,7 +113,7 @@ config FONT_SUN12x22 config FONT_TER16x32 bool "Terminus 16x32 font (not supported by all drivers)" - depends on FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC) + depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC help Terminus Font is a clean, fixed width bitmap font, designed for long (8 and more hours per day) work with computers. @@ -122,7 +122,7 @@ config FONT_TER16x32 config FONT_6x8 bool "OLED 6x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC help This font is useful for small displays (OLED). base-commit: a35e92ef04c07bd473404b9b73d489aea19a60a8 -- 2.44.0
Re: [PATCH] drm/fb_dma: Add checks in drm_fb_dma_get_scanout_buffer()
On 29/04/2024 09:24, Thomas Zimmermann wrote: Am 26.04.24 um 14:10 schrieb Jocelyn Falempe: plane->state and plane->state->fb can be NULL, so add a check before dereferencing them. Found by testing with the imx driver. Fixes: 879b3b6511fe9 ("drm/fb_dma: Add generic get_scanout_buffer() for drm_panic") Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_dma_helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index 96e5ab960f12..d7bffde94cc5 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -167,6 +167,9 @@ int drm_fb_dma_get_scanout_buffer(struct drm_plane *plane, struct drm_gem_dma_object *dma_obj; struct drm_framebuffer *fb; + if (!plane->state || !plane->state->fb) + return -ENODEV; + It's EINVAL here. With that fixed, free free to add Thanks, I just merged it to drm-misc-next with that fixed. Reviewed-by: Thomas Zimmermann Best regards Thomas fb = plane->state->fb; /* Only support linear modifier */ if (fb->modifier != DRM_FORMAT_MOD_LINEAR) base-commit: 2e3f08a1ac99cb9a19a5cb151593d4f9df5cc6a7 -- Jocelyn
[PATCH] drm/fb_dma: Add checks in drm_fb_dma_get_scanout_buffer()
plane->state and plane->state->fb can be NULL, so add a check before dereferencing them. Found by testing with the imx driver. Fixes: 879b3b6511fe9 ("drm/fb_dma: Add generic get_scanout_buffer() for drm_panic") Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_dma_helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index 96e5ab960f12..d7bffde94cc5 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -167,6 +167,9 @@ int drm_fb_dma_get_scanout_buffer(struct drm_plane *plane, struct drm_gem_dma_object *dma_obj; struct drm_framebuffer *fb; + if (!plane->state || !plane->state->fb) + return -ENODEV; + fb = plane->state->fb; /* Only support linear modifier */ if (fb->modifier != DRM_FORMAT_MOD_LINEAR) base-commit: 2e3f08a1ac99cb9a19a5cb151593d4f9df5cc6a7 -- 2.44.0
[PATCH 1/2] drm/fb-helper: Add drm_fb_helper_emergency_disable
This is needed for drm_panic, to disable fbcon output that will otherwise mix with drm_panic output. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_helper.c | 17 + include/drm/drm_fb_helper.h | 5 + 2 files changed, 22 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index d612133e2cf7e..e2e681e252e73 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -798,6 +798,23 @@ void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper, } EXPORT_SYMBOL(drm_fb_helper_set_suspend_unlocked); +/** + * drm_fb_helper_emergency_disable - disable fb output in panic situation + * @fb_helper: driver-allocated fbdev helper, can be NULL + * + * A wrapper around fb_set_suspend, to disable fb emulation when a panic occurs. + */ +void drm_fb_helper_emergency_disable(struct drm_fb_helper *fb_helper) +{ + if (fb_helper && fb_helper->info && fb_helper->info->state == FBINFO_STATE_RUNNING) { + if (console_trylock()) { + fb_set_suspend(fb_helper->info, FBINFO_STATE_SUSPENDED); + console_unlock(); + } + } +} +EXPORT_SYMBOL(drm_fb_helper_emergency_disable); + static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info) { u32 *palette = (u32 *)info->pseudo_palette; diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 375737fd6c36e..8c61c4fe93e3b 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -261,6 +261,7 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli void drm_fb_helper_set_suspend(struct drm_fb_helper *fb_helper, bool suspend); void drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper, bool suspend); +void drm_fb_helper_emergency_disable(struct drm_fb_helper *fb_helper); int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info); @@ -378,6 +379,10 @@ drm_fb_helper_set_suspend_unlocked(struct drm_fb_helper *fb_helper, bool suspend { } +static inline void drm_fb_helper_emergency_disable(struct drm_fb_helper *fb_helper) +{ +} + static inline int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { return 0; -- 2.44.0
[PATCH 2/2] drm/panic: Allows to run with fbcon
Disable the framebuffer emulation when a panic occurs, to avoid fbcon to overwrite the panic screen. So it's now safe to enable DRM_PANIC and FRAMEBUFFER_CONSOLE. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/drm_panic.c | 4 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 959b19a041018..7a8b1ef4c6bcf 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -106,7 +106,7 @@ config DRM_KMS_HELPER config DRM_PANIC bool "Display a user-friendly message when a kernel panic occurs" - depends on DRM && !FRAMEBUFFER_CONSOLE + depends on DRM select DRM_KMS_HELPER select FONT_SUPPORT help diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 7ece67086cecb..c46a2b878e759 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -470,6 +471,9 @@ static void draw_panic_plane(struct drm_plane *plane) int ret; unsigned long flags; + /* Prevent fbcon from overwriting the panic screen */ + drm_fb_helper_emergency_disable(plane->dev->fb_helper); + if (!drm_panic_trylock(plane->dev, flags)) return; -- 2.44.0
[PATCH 0/2] drm/panic: Allows to run with fbcon
fbcon and drm_panic cannot be built together because they both write to the screen when a panic occurs, leading to a corrupted panic image. With this 2 patches, drm_panic will disable the fbdev output before writing the panic screen, so only drm_panic screen will be shown. Jocelyn Falempe (2): drm/fb-helper: Add drm_fb_helper_emergency_disable drm/panic: Allows to run with fbcon drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/drm_fb_helper.c | 17 + drivers/gpu/drm/drm_panic.c | 4 include/drm/drm_fb_helper.h | 5 + 4 files changed, 27 insertions(+), 1 deletion(-) base-commit: 105aa4c65b76c3a344ca89a2d2dc96c84cca557f -- 2.44.0
[PATCH] lib/fonts: Allow to select fonts for drm_panic
drm_panic has been introduced recently, and uses the same fonts as FRAMEBUFFER_CONSOLE. Signed-off-by: Jocelyn Falempe --- lib/fonts/Kconfig | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/fonts/Kconfig b/lib/fonts/Kconfig index 7e945fdcbf11..befcb463f738 100644 --- a/lib/fonts/Kconfig +++ b/lib/fonts/Kconfig @@ -10,7 +10,7 @@ if FONT_SUPPORT config FONTS bool "Select compiled-in fonts" - depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE + depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || DRM_PANIC help Say Y here if you would like to use fonts other than the default your frame buffer console usually use. @@ -23,7 +23,7 @@ config FONTS config FONT_8x8 bool "VGA 8x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE + depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS help This is the "high resolution" font for the VGA frame buffer (the one @@ -46,7 +46,7 @@ config FONT_8x16 config FONT_6x11 bool "Mac console 6x11 font (not supported by all drivers)" if FONTS - depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE + depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS && MAC help Small console font with Macintosh-style high-half glyphs. Some Mac @@ -54,7 +54,7 @@ config FONT_6x11 config FONT_7x14 bool "console 7x14 font (not supported by all drivers)" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC help Console font with characters just a bit smaller than the default. If the standard 8x16 font is a little too big for you, say Y. @@ -62,7 +62,7 @@ config FONT_7x14 config FONT_PEARL_8x8 bool "Pearl (old m68k) console 8x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS && AMIGA help Small console font with PC-style control-character and high-half @@ -70,7 +70,7 @@ config FONT_PEARL_8x8 config FONT_ACORN_8x8 bool "Acorn console 8x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC default y if !SPARC && !FONTS && ARM && ARCH_ACORN help Small console font with PC-style control characters and high-half @@ -90,7 +90,7 @@ config FONT_6x10 config FONT_10x18 bool "console 10x18 font (not supported by all drivers)" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC help This is a high resolution console font for machines with very big letters. It fits between the sun 12x22 and the normal 8x16 font. @@ -105,7 +105,7 @@ config FONT_SUN8x16 config FONT_SUN12x22 bool "Sparc console 12x22 font (not supported by all drivers)" - depends on FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC) + depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC help This is the high resolution console font for Sun machines with very big letters (like the letters used in the SPARC PROM). If the @@ -113,7 +113,7 @@ config FONT_SUN12x22 config FONT_TER16x32 bool "Terminus 16x32 font (not supported by all drivers)" - depends on FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC) + depends on (FRAMEBUFFER_CONSOLE && (!SPARC && FONTS || SPARC)) || DRM_PANIC help Terminus Font is a clean, fixed width bitmap font, designed for long (8 and more hours per day) work with computers. @@ -122,7 +122,7 @@ config FONT_TER16x32 config FONT_6x8 bool "OLED 6x8 font" if FONTS - depends on FRAMEBUFFER_CONSOLE + depends on FRAMEBUFFER_CONSOLE || DRM_PANIC help This font is useful for small displays (OLED). base-commit: a35e92ef04c07bd473404b9b73d489aea19a60a8 -- 2.44.0
Re: [PATCH] drm/fb_dma: Fix parameter name in htmldocs
On 16/04/2024 14:12, Thomas Zimmermann wrote: Hi Am 16.04.24 um 11:05 schrieb Jocelyn Falempe: The parameter name is 'sb' and not 'drm_scanout_buffer'. It fixes the following warnings: drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Excess function parameter 'drm_scanout_buffer' description in 'drm_fb_dma_get_scanout_buffer' drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Function parameter or struct member 'sb' not described in 'drm_fb_dma_get_scanout_buffer' drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Excess function parameter 'drm_scanout_buffer' description in 'drm_fb_dma_get_scanout_buffer' Fixes: 879b3b6511fe ("drm/fb_dma: Add generic get_scanout_buffer() for drm_panic") Reported-by: Stephen Rothwell Signed-off-by: Jocelyn Falempe Reviewed-by: Thomas Zimmermann pushed to drm-misc-next Thanks, -- Jocelyn Best regards Thomas --- drivers/gpu/drm/drm_fb_dma_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index c79b5078303f..96e5ab960f12 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -153,7 +153,7 @@ EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent); /** * drm_fb_dma_get_scanout_buffer - Provide a scanout buffer in case of panic * @plane: DRM primary plane - * @drm_scanout_buffer: scanout buffer for the panic handler + * @sb: scanout buffer for the panic handler * Returns: 0 or negative error code * * Generic get_scanout_buffer() implementation, for drivers that uses the base-commit: 7b0062036c3b71b4a69e244ecf0502c06c4cf5f0
[PATCH] drm/fb_dma: Fix parameter name in htmldocs
The parameter name is 'sb' and not 'drm_scanout_buffer'. It fixes the following warnings: drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Excess function parameter 'drm_scanout_buffer' description in 'drm_fb_dma_get_scanout_buffer' drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Function parameter or struct member 'sb' not described in 'drm_fb_dma_get_scanout_buffer' drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Excess function parameter 'drm_scanout_buffer' description in 'drm_fb_dma_get_scanout_buffer' Fixes: 879b3b6511fe ("drm/fb_dma: Add generic get_scanout_buffer() for drm_panic") Reported-by: Stephen Rothwell Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_dma_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index c79b5078303f..96e5ab960f12 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -153,7 +153,7 @@ EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent); /** * drm_fb_dma_get_scanout_buffer - Provide a scanout buffer in case of panic * @plane: DRM primary plane - * @drm_scanout_buffer: scanout buffer for the panic handler + * @sb: scanout buffer for the panic handler * Returns: 0 or negative error code * * Generic get_scanout_buffer() implementation, for drivers that uses the base-commit: 7b0062036c3b71b4a69e244ecf0502c06c4cf5f0 -- 2.44.0
Re: linux-next: build warnings after merge of the drm-misc tree
On 16/04/2024 09:31, Stephen Rothwell wrote: Hi all, After merging the drm-misc tree, today's linux-next build (htmldocs) produced these warnings: drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Excess function parameter 'drm_scanout_buffer' description in 'drm_fb_dma_get_scanout_buffer' drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Function parameter or struct member 'sb' not described in 'drm_fb_dma_get_scanout_buffer' drivers/gpu/drm/drm_fb_dma_helper.c:166: warning: Excess function parameter 'drm_scanout_buffer' description in 'drm_fb_dma_get_scanout_buffer' Hi, Thanks for pointing that out. The parameter name is 'sb' and not 'drm_scanout_buffer', I will send a fix. Best regards, -- Jocelyn Introduced by commit 879b3b6511fe ("drm/fb_dma: Add generic get_scanout_buffer() for drm_panic")
Re: [PATCH] drm/fb_dma: s/drm_panic_gem_get_scanout_buffer/drm_fb_dma_get_scanout_buffer
Hi, You're right, I messed up the rename, and I mostly test on x86, where I don't build the imx driver. Reviewed-by: Jocelyn Falempe Best regards, -- Jocelyn On 15/04/2024 17:09, Maíra Canal wrote: On version 11, Thomas suggested to change the name of the function and this request was applied on version 12, which is the version that landed. Although the name of the function changed on the C file, it didn't changed on the header file, leading to a compilation error as such: drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c:780:24: error: use of undeclared identifier 'drm_fb_dma_get_scanout_buffer'; did you mean 'drm_panic_gem_get_scanout_buffer'? 780 | .get_scanout_buffer = drm_fb_dma_get_scanout_buffer, | ^ | drm_panic_gem_get_scanout_buffer ./include/drm/drm_fb_dma_helper.h:23:5: note: 'drm_panic_gem_get_scanout_buffer' declared here 23 | int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, | ^ 1 error generated. Best Regards, - Maíra Fixes: 879b3b6511fe ("drm/fb_dma: Add generic get_scanout_buffer() for drm_panic" Signed-off-by: Maíra Canal --- include/drm/drm_fb_dma_helper.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h index 61f24c2aba2f..c950732c6d36 100644 --- a/include/drm/drm_fb_dma_helper.h +++ b/include/drm/drm_fb_dma_helper.h @@ -6,6 +6,7 @@ struct drm_device; struct drm_framebuffer; +struct drm_plane; struct drm_plane_state; struct drm_scanout_buffer; @@ -20,8 +21,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, struct drm_plane_state *old_state, struct drm_plane_state *state); -int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, -struct drm_scanout_buffer *sb); +int drm_fb_dma_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb); #endif
Re: [PATCH v12 0/9] drm/panic: Add a drm panic handler
On 10/04/2024 10:12, Daniel Vetter wrote: On Tue, Apr 09, 2024 at 06:30:39PM +0200, Jocelyn Falempe wrote: drm/panic: Add a drm panic handler 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. It works with simpledrm, mgag200, ast, and imx. To test it, make sure you're using one of the supported driver, and trigger a panic: echo c > /proc/sysrq-trigger or you can enable CONFIG_DRM_PANIC_DEBUG and echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0 Even if this is not yet useful, it will allows to work on more driver support, and better debug information to be added. v2: * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann) * Add the panic reason to the panic message (Nerdopolis) * Add an exclamation mark (Nerdopolis) v3: * Rework the drawing functions, to write the pixels line by line and to use the drm conversion helper to support other formats. (Thomas Zimmermann) v4: * Fully support all simpledrm formats using drm conversion helpers * Rename dpanic_* to drm_panic_*, and have more coherent function name. (Thomas Zimmermann) * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann) * Remove the default y to DRM_PANIC config option (Thomas Zimmermann) * Add foreground/background color config option * Fix the bottom lines not painted if the framebuffer height is not a multiple of the font height. * Automatically register the driver to drm_panic, if the function get_scanout_buffer() exists. (Thomas Zimmermann) * Add mgag200 support. v5: * Change the drawing API, use drm_fb_blit_from_r1() to draw the font. (Thomas Zimmermann) * Also add drm_fb_fill() to fill area with background color. * Add draw_pixel_xy() API for drivers that can't provide a linear buffer. * Add a flush() callback for drivers that needs to synchronize the buffer. * Add a void *private field, so drivers can pass private data to draw_pixel_xy() and flush(). * Add ast support. * Add experimental imx/ipuv3 support, to test on an ARM hw. (Maxime Ripard) v6: * Fix sparse and __le32 warnings * Drop the IMX/IPUV3 experiment, it was just to show that it works also on ARM devices. v7: * Add a check to see if the 4cc format is supported by drm_panic. * Add a drm/plane helper to loop over all visible primary buffer, simplifying the get_scanout_buffer implementations * Add a generic implementation for drivers that uses drm_fb_dma. (Maxime Ripard) * Add back the IMX/IPUV3 support, and use the generic implementation. (Maxime Ripard) v8: * Directly register each plane to the panic notifier (Sima) * Replace get_scanout_buffer() with set_scanout_buffer() to simplify the locking. (Thomas Zimmermann) * Add a debugfs entry, to trigger the drm_panic without a real panic (Sima) * Fix the drm_panic Documentation, and include it in drm-kms.rst v9: * Revert to using get_scanout_buffer() (Sima) * Move get_scanout_buffer() and panic_flush() to the plane helper functions (Thomas Zimmermann) * Register all planes with get_scanout_buffer() to the panic notifier * Use drm_panic_lock() to protect against race (Sima) * Create a debugfs file for each plane in the device's debugfs directory. This allows to test for each plane of each GPU independently. v10: * Move blit and fill functions back in drm_panic (Thomas Zimmermann). * Simplify the text drawing functions. * Use kmsg_dumper instead of panic_notifier (Sima). * Use spinlock_irqsave/restore (John Ogness) v11: * Use macro instead of inline functions for drm_panic_lock/unlock (John Ogness) v12: * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) * Rename drm_panic_gem_get_scanout_buffer to drm_fb_dma_get_scanout_buffer and build it unconditionally (Thomas Zimmermann) * Better indent struct drm_scanout_buffer declaration. (Thomas Zimmermann) On the series: Acked-by: Daniel Vetter And apologies for the detours this patch set took and my part in the (too many) revisions. I think we should land this and do anything more once it's merged and we extend the panic support to more drivers. Thanks for your help, I just pushed it to drm-misc-next. Best regards, -- Jocelyn Thanks a lot to you for seeing this through! Cheers, Sima Best regards, Daniel Vetter (1): drm/panic: Add drm panic locking Jocelyn Falempe (8): drm/panic: Add a drm panic handler drm/panic: Add support for color format conversion drm/panic: Add debugfs entry to test without triggering panic. drm/fb_dma: Add generic get_scanout_
Re: [PATCH v12 2/9] drm/panic: Add a drm panic handler
Hi, On 10/04/2024 10:10, Daniel Vetter wrote: On Tue, Apr 09, 2024 at 06:30:41PM +0200, Jocelyn Falempe wrote: This module displays a user friendly message when a kernel panic occurs. It currently doesn't contain any debug information, but that can be added later. v2 * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann) * Add the panic reason to the panic message (Nerdopolis) * Add an exclamation mark (Nerdopolis) v3 * Rework the drawing functions, to write the pixels line by line and to use the drm conversion helper to support other formats. (Thomas Zimmermann) v4 * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann) * Remove the default y to DRM_PANIC config option (Thomas Zimmermann) * Add foreground/background color config option * Fix the bottom lines not painted if the framebuffer height is not a multiple of the font height. * Automatically register the device to drm_panic, if the function get_scanout_buffer exists. (Thomas Zimmermann) v5 * Change the drawing API, use drm_fb_blit_from_r1() to draw the font. * Also add drm_fb_fill() to fill area with background color. * Add draw_pixel_xy() API for drivers that can't provide a linear buffer. * Add a flush() callback for drivers that needs to synchronize the buffer. * Add a void *private field, so drivers can pass private data to draw_pixel_xy() and flush(). v6 * Fix sparse warning for panic_msg and logo. v7 * Add select DRM_KMS_HELPER for the color conversion functions. v8 * Register directly each plane to the panic notifier (Sima) * Add raw_spinlock to properly handle concurrency (Sima) * Register plane instead of device, to avoid looping through plane list, and simplify code. * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) * Removed the draw_pixel_xy() API, will see later if it can be added back. v9 * Revert to using get_scanout_buffer() (Sima) * Move get_scanout_buffer() and panic_flush() to the plane helper functions (Thomas Zimmermann) * Register all planes with get_scanout_buffer() to the panic notifier * Use drm_panic_lock() to protect against race (Sima) v10 * Move blit and fill functions back in drm_panic (Thomas Zimmermann). * Simplify the text drawing functions. * Use kmsg_dumper instead of panic_notifier (Sima). v12 * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) * Better indent struct drm_scanout_buffer declaration. (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe Some detail suggestions for the kerneldoc but those aside Acked-by: Daniel Vetter Thanks for the review. I fixed the documentation, and make sure the links are working. --- Documentation/gpu/drm-kms.rst| 12 + drivers/gpu/drm/Kconfig | 23 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_drv.c| 4 + drivers/gpu/drm/drm_panic.c | 289 +++ include/drm/drm_modeset_helper_vtables.h | 37 +++ include/drm/drm_panic.h | 57 + include/drm/drm_plane.h | 6 + 8 files changed, 429 insertions(+) create mode 100644 drivers/gpu/drm/drm_panic.c diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 13d3627d8bc0..b64334661aeb 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -398,6 +398,18 @@ Plane Damage Tracking Functions Reference .. kernel-doc:: include/drm/drm_damage_helper.h :internal: +Plane Panic Feature +--- + +.. kernel-doc:: drivers/gpu/drm/drm_panic.c + :doc: overview + +Plane Panic Functions Reference +--- + +.. kernel-doc:: drivers/gpu/drm/drm_panic.c + :export: + Display Modes Function Reference diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 3914aaf443a8..f8a26423369e 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -104,6 +104,29 @@ config DRM_KMS_HELPER help CRTC helpers for KMS drivers. +config DRM_PANIC + bool "Display a user-friendly message when a kernel panic occurs" + depends on DRM && !FRAMEBUFFER_CONSOLE + select DRM_KMS_HELPER + select FONT_SUPPORT + help + Enable a drm panic handler, which will display a user-friendly message + when a kernel panic occurs. It's useful when using a user-space + console instead of fbcon. + It will only work if your graphic driver supports this feature. + To support Hi-DPI Display, you can enable bigger fonts like + FONT_TER16x32 + +config DRM_PANIC_FOREGROUND_COLOR + hex "Drm panic screen foreground color, in RGB" + depends on DRM_PANIC + default 0xff + +config DRM_PANIC_BACKGROUND_COLOR +
Re: [PATCH v12 1/9] drm/panic: Add drm panic locking
Hi, On 10/04/2024 09:59, Daniel Vetter wrote: On Tue, Apr 09, 2024 at 06:30:40PM +0200, Jocelyn Falempe wrote: From: Daniel Vetter Rough sketch for the locking of drm panic printing code. The upshot of this approach is that we can pretty much entirely rely on the atomic commit flow, with the pair of raw_spin_lock/unlock providing any barriers we need, without having to create really big critical sections in code. This also avoids the need that drivers must explicitly update the panic handler state, which they might forget to do, or not do consistently, and then we blow up in the worst possible times. It is somewhat racy against a concurrent atomic update, and we might write into a buffer which the hardware will never display. But there's fundamentally no way to avoid that - if we do the panic state update explicitly after writing to the hardware, we might instead write to an old buffer that the user will barely ever see. Note that an rcu protected deference of plane->state would give us the the same guarantees, but it has the downside that we then need to protect the plane state freeing functions with call_rcu too. Which would very widely impact a lot of code and therefore doesn't seem worth the complexity compared to a raw spinlock with very tiny critical sections. Plus rcu cannot be used to protect access to peek/poke registers anyway, so we'd still need it for those cases. Peek/poke registers for vram access (or a gart pte reserved just for panic code) are also the reason I've gone with a per-device and not per-plane spinlock, since usually these things are global for the entire display. Going with per-plane locks would mean drivers for such hardware would need additional locks, which we don't want, since it deviates from the per-console takeoverlocks design. Longer term it might be useful if the panic notifiers grow a bit more structure than just the absolute bare EXPORT_SYMBOL(panic_notifier_list) - somewhat aside, why is that not EXPORT_SYMBOL_GPL ... If panic notifiers would be more like console drivers with proper register/unregister interfaces we could perhaps reuse the very fancy console lock with all it's check and takeover semantics that John Ogness is developing to fix the console_lock mess. But for the initial cut of a drm panic printing support I don't think we need that, because the critical sections are extremely small and only happen once per display refresh. So generally just 60 tiny locked sections per second, which is nothing compared to a serial console running a 115kbaud doing really slow mmio writes for each byte. So for now the raw spintrylock in drm panic notifier callback should be good enough. Another benefit of making panic notifiers more like full blown consoles (that are used in panics only) would be that we get the two stage design, where first all the safe outputs are used. And then the dangerous takeover tricks are deployed (where for display drivers we also might try to intercept any in-flight display buffer flips, which if we race and misprogram fifos and watermarks can hang the memory controller on some hw). For context the actual implementation on the drm side is by Jocelyn and this patch is meant to be combined with the overall approach in v7 (v8 is a bit less flexible, which I think is the wrong direction): https://lore.kernel.org/dri-devel/20240104160301.185915-1-jfale...@redhat.com/ Note that the locking is very much not correct there, hence this separate rfc. v2: - fix authorship, this was all my typing - some typo oopsies - link to the drm panic work by Jocelyn for context Please annotate that v10 and later is your work, credit where credit is due and all that :-) Sure, I'll add a comment about this. v10: - Use spinlock_irqsave/restore (John Ogness) v11: - Use macro instead of inline functions for drm_panic_lock/unlock (John Ogness) Signed-off-by: Daniel Vetter Cc: Jocelyn Falempe Cc: Andrew Morton Cc: "Peter Zijlstra (Intel)" Cc: Lukas Wunner Cc: Petr Mladek Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++ drivers/gpu/drm/drm_drv.c | 1 + include/drm/drm_mode_config.h | 10 +++ include/drm/drm_panic.h | 100 4 files changed, 115 insertions(+) create mode 100644 include/drm/drm_panic.h diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 39ef0a6addeb..fb97b51b38f1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -3016,6 +3017,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall) { int i, ret; +
Re: [PATCH 3/3] drm/ast: Define struct ast_ddc in ast_ddc.c
Hi, Thanks for the patch, it looks good to me. Reviewed-by: Jocelyn Falempe -- Jocelyn On 03/04/2024 12:31, Thomas Zimmermann wrote: Move the definition of struct ast_ddc to ast_ddc.c and return the i2c adapter from ast_ddc_create(). Update callers accordingly. Avoids including Linux i2c header files, except where required. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_ddc.c | 14 -- drivers/gpu/drm/ast/ast_ddc.h | 13 ++--- drivers/gpu/drm/ast/ast_mode.c | 8 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c index 4df52aeba4f7e..29cf5d157f344 100644 --- a/drivers/gpu/drm/ast/ast_ddc.c +++ b/drivers/gpu/drm/ast/ast_ddc.c @@ -21,12 +21,22 @@ * of the Software. */ +#include +#include + #include #include #include "ast_ddc.h" #include "ast_drv.h" +struct ast_ddc { + struct ast_device *ast; + + struct i2c_algo_bit_data bit; + struct i2c_adapter adapter; +}; + static void ast_ddc_algo_bit_data_setsda(void *data, int state) { struct ast_ddc *ddc = data; @@ -132,7 +142,7 @@ static void ast_ddc_release(struct drm_device *dev, void *res) i2c_del_adapter(>adapter); } -struct ast_ddc *ast_ddc_create(struct ast_device *ast) +struct i2c_adapter *ast_ddc_create(struct ast_device *ast) { struct drm_device *dev = >base; struct ast_ddc *ddc; @@ -173,5 +183,5 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast) if (ret) return ERR_PTR(ret); - return ddc; + return >adapter; } diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h index 08f3994e09ccd..85c93edc9ae12 100644 --- a/drivers/gpu/drm/ast/ast_ddc.h +++ b/drivers/gpu/drm/ast/ast_ddc.h @@ -3,18 +3,9 @@ #ifndef __AST_DDC_H__ #define __AST_DDC_H__ -#include -#include - struct ast_device; +struct i2c_adapter; -struct ast_ddc { - struct ast_device *ast; - - struct i2c_adapter adapter; - struct i2c_algo_bit_data bit; -}; - -struct ast_ddc *ast_ddc_create(struct ast_device *ast); +struct i2c_adapter *ast_ddc_create(struct ast_device *ast); #endif diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index bb9b66aba9ee9..ae21d89b899c8 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -1360,7 +1360,7 @@ static const struct drm_connector_funcs ast_vga_connector_funcs = { static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *connector) { struct ast_device *ast = to_ast_device(dev); - struct ast_ddc *ddc; + struct i2c_adapter *ddc; int ret; ddc = ast_ddc_create(ast); @@ -1371,7 +1371,7 @@ static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector * } ret = drm_connector_init_with_ddc(dev, connector, _vga_connector_funcs, - DRM_MODE_CONNECTOR_VGA, >adapter); + DRM_MODE_CONNECTOR_VGA, ddc); if (ret) return ret; @@ -1429,7 +1429,7 @@ static const struct drm_connector_funcs ast_sil164_connector_funcs = { static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connector *connector) { struct ast_device *ast = to_ast_device(dev); - struct ast_ddc *ddc; + struct i2c_adapter *ddc; int ret; ddc = ast_ddc_create(ast); @@ -1440,7 +1440,7 @@ static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connecto } ret = drm_connector_init_with_ddc(dev, connector, _sil164_connector_funcs, - DRM_MODE_CONNECTOR_DVII, >adapter); + DRM_MODE_CONNECTOR_DVII, ddc); if (ret) return ret;
Re: [PATCH 2/3] drm/ast: Group DDC init code by data structure
Hi, Thanks for the patch, it looks good to me. Reviewed-by: Jocelyn Falempe -- Jocelyn On 03/04/2024 12:31, Thomas Zimmermann wrote: Reorder the code to set up the DDC channel by data structure, so that each data structure's init is in a separate block: first the bit algo then the i2c adapter. Makes the code more readable. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_ddc.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c index 3e156a6b6831d..4df52aeba4f7e 100644 --- a/drivers/gpu/drm/ast/ast_ddc.c +++ b/drivers/gpu/drm/ast/ast_ddc.c @@ -145,15 +145,7 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast) return ERR_PTR(-ENOMEM); ddc->ast = ast; - adapter = >adapter; - adapter->owner = THIS_MODULE; - adapter->dev.parent = dev->dev; - i2c_set_adapdata(adapter, ddc); - snprintf(adapter->name, sizeof(adapter->name), "AST DDC bus"); - bit = >bit; - bit->udelay = 20; - bit->timeout = usecs_to_jiffies(2200); bit->data = ddc; bit->setsda = ast_ddc_algo_bit_data_setsda; bit->setscl = ast_ddc_algo_bit_data_setscl; @@ -161,8 +153,16 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast) bit->getscl = ast_ddc_algo_bit_data_getscl; bit->pre_xfer = ast_ddc_algo_bit_data_pre_xfer; bit->post_xfer = ast_ddc_algo_bit_data_post_xfer; + bit->udelay = 20; + bit->timeout = usecs_to_jiffies(2200); + adapter = >adapter; + adapter->owner = THIS_MODULE; adapter->algo_data = bit; + adapter->dev.parent = dev->dev; + snprintf(adapter->name, sizeof(adapter->name), "AST DDC bus"); + i2c_set_adapdata(adapter, ddc); + ret = i2c_bit_add_bus(adapter); if (ret) { drm_err(dev, "Failed to register bit i2c\n");
Re: [PATCH 1/3] drm/ast: Set DDC timeout in milliseconds
Hi, Thanks for the patch, it looks good to me. Reviewed-by: Jocelyn Falempe -- Jocelyn On 03/04/2024 12:31, Thomas Zimmermann wrote: Compute the i2c timeout in jiffies from a value in milliseconds. The original values of 2 jiffies equals 2 milliseconds if HZ has been configured to a value of 1000. This corresponds to 2.2 milliseconds used by most other DRM drivers. Update ast accordingly. Signed-off-by: Thomas Zimmermann Fixes: 312fec1405dd ("drm: Initial KMS driver for AST (ASpeed Technologies) 2000 series (v2)") Cc: Dave Airlie Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: Jocelyn Falempe Cc: dri-devel@lists.freedesktop.org Cc: # v3.5+ --- drivers/gpu/drm/ast/ast_ddc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c index b7718084422f3..3e156a6b6831d 100644 --- a/drivers/gpu/drm/ast/ast_ddc.c +++ b/drivers/gpu/drm/ast/ast_ddc.c @@ -153,7 +153,7 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast) bit = >bit; bit->udelay = 20; - bit->timeout = 2; + bit->timeout = usecs_to_jiffies(2200); bit->data = ddc; bit->setsda = ast_ddc_algo_bit_data_setsda; bit->setscl = ast_ddc_algo_bit_data_setscl;
[PATCH v12 8/9] drm/imx: Add drm_panic support
Add support for the drm_panic module, which displays a user-friendly message to the screen when a kernel panic occurs. v7: * use drm_panic_gem_get_scanout_buffer() helper v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions v12: * Rename drm_panic_gem_get_scanout_buffer to drm_fb_dma_get_scanout_buffer (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c index dade8b59feae..de010964ee7d 100644 --- a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c @@ -772,6 +772,12 @@ static const struct drm_plane_helper_funcs ipu_plane_helper_funcs = { .atomic_disable = ipu_plane_atomic_disable, .atomic_update = ipu_plane_atomic_update, }; +static const struct drm_plane_helper_funcs ipu_primary_plane_helper_funcs = { + .atomic_check = ipu_plane_atomic_check, + .atomic_disable = ipu_plane_atomic_disable, + .atomic_update = ipu_plane_atomic_update, + .get_scanout_buffer = drm_fb_dma_get_scanout_buffer, +}; bool ipu_plane_atomic_update_pending(struct drm_plane *plane) { @@ -916,7 +922,10 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu, ipu_plane->dma = dma; ipu_plane->dp_flow = dp; - drm_plane_helper_add(_plane->base, _plane_helper_funcs); + if (type == DRM_PLANE_TYPE_PRIMARY) + drm_plane_helper_add(_plane->base, _primary_plane_helper_funcs); + else + drm_plane_helper_add(_plane->base, _plane_helper_funcs); if (dp == IPU_DP_FLOW_SYNC_BG || dp == IPU_DP_FLOW_SYNC_FG) ret = drm_plane_create_zpos_property(_plane->base, zpos, 0, -- 2.44.0
[PATCH v12 4/9] drm/panic: Add debugfs entry to test without triggering panic.
Add a debugfs file, so you can test drm_panic without freezing your machine. This is unsafe, and should be enabled only for developer or tester. To display the drm_panic screen on the device 0: echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0 v9: * Create a debugfs file for each plane in the device's debugfs directory. This allows to test for each plane of each GPU independently. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 9 drivers/gpu/drm/drm_panic.c | 43 - 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index f8a26423369e..959b19a04101 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -127,6 +127,15 @@ config DRM_PANIC_BACKGROUND_COLOR depends on DRM_PANIC default 0x00 +config DRM_PANIC_DEBUG + bool "Add a debug fs entry to trigger drm_panic" + depends on DRM_PANIC && DEBUG_FS + help + Add dri/[device]/drm_panic_plane_x in the kernel debugfs, to force the + panic handler to write the panic message to this plane scanout buffer. + This is unsafe and should not be enabled on a production build. + If in doubt, say "N". + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index e1ec30b6c04a..78fd6d5d7adc 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -495,6 +495,45 @@ static void drm_panic(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason) draw_panic_plane(plane); } + +/* + * DEBUG FS, This is currently unsafe. + * Create one file per plane, so it's possible to debug one plane at a time. + */ +#ifdef CONFIG_DRM_PANIC_DEBUG +#include + +static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_buf, +size_t count, loff_t *ppos) +{ + bool run; + + if (kstrtobool_from_user(user_buf, count, ) == 0 && run) { + struct drm_plane *plane = file->private_data; + + draw_panic_plane(plane); + } + return count; +} + +static const struct file_operations dbg_drm_panic_ops = { + .owner = THIS_MODULE, + .write = debugfs_trigger_write, + .open = simple_open, +}; + +static void debugfs_register_plane(struct drm_plane *plane, int index) +{ + char fname[32]; + + snprintf(fname, 32, "drm_panic_plane_%d", index); + debugfs_create_file(fname, 0200, plane->dev->debugfs_root, + plane, _drm_panic_ops); +} +#else +static void debugfs_register_plane(struct drm_plane *plane, int index) {} +#endif /* CONFIG_DRM_PANIC_DEBUG */ + /** * drm_panic_register() - Initialize DRM panic for a device * @dev: the drm device on which the panic screen will be displayed. @@ -514,8 +553,10 @@ void drm_panic_register(struct drm_device *dev) plane->kmsg_panic.max_reason = KMSG_DUMP_PANIC; if (kmsg_dump_register(>kmsg_panic)) drm_warn(dev, "Failed to register panic handler\n"); - else + else { + debugfs_register_plane(plane, registered_plane); registered_plane++; + } } if (registered_plane) drm_info(dev, "Registered %d planes with drm panic\n", registered_plane); -- 2.44.0
[PATCH v12 9/9] drm/ast: Add drm_panic support
Add support for the drm_panic module, which displays a message to the screen when a kernel panic occurs. v7 * Use drm_for_each_primary_visible_plane() v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions v12: * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe Acked-by: Sui Jingfeng Tested-by: Sui Jingfeng Reviewed-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_mode.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index bb9b66aba9ee..417b111d90fa 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #include @@ -701,12 +702,29 @@ static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane, ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x1, 0xdf, 0x20); } +static int ast_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) +{ + struct ast_plane *ast_plane = to_ast_plane(plane); + + if (plane->state && plane->state->fb && ast_plane->vaddr) { + 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]; + iosys_map_set_vaddr_iomem(>map[0], ast_plane->vaddr); + return 0; + } + return -ENODEV; +} + static const struct drm_plane_helper_funcs ast_primary_plane_helper_funcs = { DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, .atomic_check = ast_primary_plane_helper_atomic_check, .atomic_update = ast_primary_plane_helper_atomic_update, .atomic_enable = ast_primary_plane_helper_atomic_enable, .atomic_disable = ast_primary_plane_helper_atomic_disable, + .get_scanout_buffer = ast_primary_plane_helper_get_scanout_buffer, }; static const struct drm_plane_funcs ast_primary_plane_funcs = { -- 2.44.0
[PATCH v12 6/9] drm/simpledrm: Add drm_panic support
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) v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions (Thomas Zimmermann) v12: * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe Reviewed-by: Thomas Zimmermann --- drivers/gpu/drm/tiny/simpledrm.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 7ce1c4617675..1d8fa07572c5 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -25,6 +25,7 @@ #include #include #include +#include #include #define DRIVER_NAME"simpledrm" @@ -671,11 +672,26 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan drm_dev_exit(idx); } +static int simpledrm_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb) +{ + struct simpledrm_device *sdev = simpledrm_device_of_dev(plane->dev); + + sb->width = sdev->mode.hdisplay; + sb->height = sdev->mode.vdisplay; + sb->format = sdev->format; + sb->pitch[0] = sdev->pitch; + sb->map[0] = sdev->screen_base; + + return 0; +} + static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = { DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, .atomic_check = simpledrm_primary_plane_helper_atomic_check, .atomic_update = simpledrm_primary_plane_helper_atomic_update, .atomic_disable = simpledrm_primary_plane_helper_atomic_disable, + .get_scanout_buffer = simpledrm_primary_plane_helper_get_scanout_buffer, }; static const struct drm_plane_funcs simpledrm_primary_plane_funcs = { -- 2.44.0
[PATCH v12 7/9] drm/mgag200: Add drm_panic support
Add support for the drm_panic module, which displays a message to the screen when a kernel panic occurs. v5: * Also check that the plane is visible and primary. (Thomas Zimmermann) v7: * use drm_for_each_primary_visible_plane() v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions (Thomas Zimmermann) v12: * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe Reviewed-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ++- drivers/gpu/drm/mgag200/mgag200_mode.c | 18 ++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 765e49fd8911..58a0e62eaf18 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -366,6 +366,7 @@ struct drm_crtc_state; struct drm_display_mode; struct drm_plane; struct drm_atomic_state; +struct drm_scanout_buffer; extern const uint32_t mgag200_primary_plane_formats[]; extern const size_t mgag200_primary_plane_formats_size; @@ -379,12 +380,16 @@ void mgag200_primary_plane_helper_atomic_enable(struct drm_plane *plane, struct drm_atomic_state *state); void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane, struct drm_atomic_state *old_state); +int mgag200_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb); + #define MGAG200_PRIMARY_PLANE_HELPER_FUNCS \ DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, \ .atomic_check = mgag200_primary_plane_helper_atomic_check, \ .atomic_update = mgag200_primary_plane_helper_atomic_update, \ .atomic_enable = mgag200_primary_plane_helper_atomic_enable, \ - .atomic_disable = mgag200_primary_plane_helper_atomic_disable + .atomic_disable = mgag200_primary_plane_helper_atomic_disable, \ + .get_scanout_buffer = mgag200_primary_plane_helper_get_scanout_buffer #define MGAG200_PRIMARY_PLANE_FUNCS \ .update_plane = drm_atomic_helper_update_plane, \ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index e17cb4c5f774..fc54851d3384 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include "mgag200_drv.h" @@ -546,6 +547,23 @@ void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane, msleep(20); } +int mgag200_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) +{ + struct mga_device *mdev = to_mga_device(plane->dev); + struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram); + + if (plane->state && plane->state->fb) { + 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] = map; + return 0; + } + return -ENODEV; +} + /* * CRTC */ -- 2.44.0
[PATCH v12 5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
This was initialy done for imx6, but should work on most drivers using drm_fb_dma_helper. v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * go back to get_scanout_buffer() * move get_scanout_buffer() to plane helper functions v12: * Rename drm_panic_gem_get_scanout_buffer to drm_fb_dma_get_scanout_buffer (Thomas Zimmermann) * Remove the #ifdef CONFIG_DRM_PANIC, and build it unconditionnaly, as it's a small function. (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_dma_helper.c | 42 + include/drm/drm_fb_dma_helper.h | 4 +++ 2 files changed, 46 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index 3b535ad1b07c..c79b5078303f 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -148,3 +149,44 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, } } EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent); + +/** + * drm_fb_dma_get_scanout_buffer - Provide a scanout buffer in case of panic + * @plane: DRM primary plane + * @drm_scanout_buffer: scanout buffer for the panic handler + * Returns: 0 or negative error code + * + * Generic get_scanout_buffer() implementation, for drivers that uses the + * drm_fb_dma_helper. It won't call vmap in the panic context, so the driver + * should make sure the primary plane is vmapped, otherwise the panic screen + * won't get displayed. + */ +int drm_fb_dma_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) +{ + struct drm_gem_dma_object *dma_obj; + struct drm_framebuffer *fb; + + fb = plane->state->fb; + /* Only support linear modifier */ + if (fb->modifier != DRM_FORMAT_MOD_LINEAR) + return -ENODEV; + + dma_obj = drm_fb_dma_get_gem_obj(fb, 0); + + /* Buffer should be accessible from the CPU */ + if (dma_obj->base.import_attach) + return -ENODEV; + + /* Buffer should be already mapped to CPU */ + if (!dma_obj->vaddr) + return -ENODEV; + + iosys_map_set_vaddr(>map[0], dma_obj->vaddr); + sb->format = fb->format; + sb->height = fb->height; + sb->width = fb->width; + sb->pitch[0] = fb->pitches[0]; + return 0; +} +EXPORT_SYMBOL(drm_fb_dma_get_scanout_buffer); diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h index d5e036c57801..61f24c2aba2f 100644 --- a/include/drm/drm_fb_dma_helper.h +++ b/include/drm/drm_fb_dma_helper.h @@ -7,6 +7,7 @@ struct drm_device; struct drm_framebuffer; struct drm_plane_state; +struct drm_scanout_buffer; struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb, unsigned int plane); @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, struct drm_plane_state *old_state, struct drm_plane_state *state); +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb); + #endif -- 2.44.0
[PATCH v12 0/9] drm/panic: Add a drm panic handler
drm/panic: Add a drm panic handler 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. It works with simpledrm, mgag200, ast, and imx. To test it, make sure you're using one of the supported driver, and trigger a panic: echo c > /proc/sysrq-trigger or you can enable CONFIG_DRM_PANIC_DEBUG and echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0 Even if this is not yet useful, it will allows to work on more driver support, and better debug information to be added. v2: * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann) * Add the panic reason to the panic message (Nerdopolis) * Add an exclamation mark (Nerdopolis) v3: * Rework the drawing functions, to write the pixels line by line and to use the drm conversion helper to support other formats. (Thomas Zimmermann) v4: * Fully support all simpledrm formats using drm conversion helpers * Rename dpanic_* to drm_panic_*, and have more coherent function name. (Thomas Zimmermann) * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann) * Remove the default y to DRM_PANIC config option (Thomas Zimmermann) * Add foreground/background color config option * Fix the bottom lines not painted if the framebuffer height is not a multiple of the font height. * Automatically register the driver to drm_panic, if the function get_scanout_buffer() exists. (Thomas Zimmermann) * Add mgag200 support. v5: * Change the drawing API, use drm_fb_blit_from_r1() to draw the font. (Thomas Zimmermann) * Also add drm_fb_fill() to fill area with background color. * Add draw_pixel_xy() API for drivers that can't provide a linear buffer. * Add a flush() callback for drivers that needs to synchronize the buffer. * Add a void *private field, so drivers can pass private data to draw_pixel_xy() and flush(). * Add ast support. * Add experimental imx/ipuv3 support, to test on an ARM hw. (Maxime Ripard) v6: * Fix sparse and __le32 warnings * Drop the IMX/IPUV3 experiment, it was just to show that it works also on ARM devices. v7: * Add a check to see if the 4cc format is supported by drm_panic. * Add a drm/plane helper to loop over all visible primary buffer, simplifying the get_scanout_buffer implementations * Add a generic implementation for drivers that uses drm_fb_dma. (Maxime Ripard) * Add back the IMX/IPUV3 support, and use the generic implementation. (Maxime Ripard) v8: * Directly register each plane to the panic notifier (Sima) * Replace get_scanout_buffer() with set_scanout_buffer() to simplify the locking. (Thomas Zimmermann) * Add a debugfs entry, to trigger the drm_panic without a real panic (Sima) * Fix the drm_panic Documentation, and include it in drm-kms.rst v9: * Revert to using get_scanout_buffer() (Sima) * Move get_scanout_buffer() and panic_flush() to the plane helper functions (Thomas Zimmermann) * Register all planes with get_scanout_buffer() to the panic notifier * Use drm_panic_lock() to protect against race (Sima) * Create a debugfs file for each plane in the device's debugfs directory. This allows to test for each plane of each GPU independently. v10: * Move blit and fill functions back in drm_panic (Thomas Zimmermann). * Simplify the text drawing functions. * Use kmsg_dumper instead of panic_notifier (Sima). * Use spinlock_irqsave/restore (John Ogness) v11: * Use macro instead of inline functions for drm_panic_lock/unlock (John Ogness) v12: * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) * Rename drm_panic_gem_get_scanout_buffer to drm_fb_dma_get_scanout_buffer and build it unconditionally (Thomas Zimmermann) * Better indent struct drm_scanout_buffer declaration. (Thomas Zimmermann) Best regards, Daniel Vetter (1): drm/panic: Add drm panic locking Jocelyn Falempe (8): drm/panic: Add a drm panic handler drm/panic: Add support for color format conversion drm/panic: Add debugfs entry to test without triggering panic. drm/fb_dma: Add generic get_scanout_buffer() for drm_panic drm/simpledrm: Add drm_panic support drm/mgag200: Add drm_panic support drm/imx: Add drm_panic support drm/ast: Add drm_panic support Documentation/gpu/drm-kms.rst| 12 + drivers/gpu/drm/Kconfig | 32 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/ast/ast_mode.c | 18 + drivers/gpu/drm/drm_atomic_helper.c | 4 + drivers/gpu/drm/drm_drv.c| 5 + drivers/gpu/drm/drm_fb_dma_helper.c | 42 ++ drivers/gpu/drm/drm_panic.c
[PATCH v12 3/9] drm/panic: Add support for color format conversion
Add support for the following formats: DRM_FORMAT_RGB565 DRM_FORMAT_RGBA5551 DRM_FORMAT_XRGB1555 DRM_FORMAT_ARGB1555 DRM_FORMAT_RGB888 DRM_FORMAT_XRGB DRM_FORMAT_ARGB DRM_FORMAT_XBGR DRM_FORMAT_XRGB2101010 DRM_FORMAT_ARGB2101010 v10: * move and simplify the functions from the drm format helper to drm_panic v12: * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 273 ++-- 1 file changed, 263 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index fa4b220534a4..e1ec30b6c04a 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -81,15 +81,155 @@ static const struct drm_panic_line logo[] = { PANIC_LINE(" \\___)=(___/"), }; -static void drm_panic_fill32(struct iosys_map *map, unsigned int pitch, +/* + * Color conversion + */ + +static u16 convert_xrgb_to_rgb565(u32 pix) +{ + return ((pix & 0x00F8) >> 8) | + ((pix & 0xFC00) >> 5) | + ((pix & 0x00F8) >> 3); +} + +static u16 convert_xrgb_to_rgba5551(u32 pix) +{ + return ((pix & 0x00f8) >> 8) | + ((pix & 0xf800) >> 5) | + ((pix & 0x00f8) >> 2) | + BIT(0); /* set alpha bit */ +} + +static u16 convert_xrgb_to_xrgb1555(u32 pix) +{ + return ((pix & 0x00f8) >> 9) | + ((pix & 0xf800) >> 6) | + ((pix & 0x00f8) >> 3); +} + +static u16 convert_xrgb_to_argb1555(u32 pix) +{ + return BIT(15) | /* set alpha bit */ + ((pix & 0x00f8) >> 9) | + ((pix & 0xf800) >> 6) | + ((pix & 0x00f8) >> 3); +} + +static u32 convert_xrgb_to_argb(u32 pix) +{ + return pix | GENMASK(31, 24); /* fill alpha bits */ +} + +static u32 convert_xrgb_to_xbgr(u32 pix) +{ + return ((pix & 0x00ff) >> 16) << 0 | + ((pix & 0xff00) >> 8) << 8 | + ((pix & 0x00ff) >> 0) << 16 | + ((pix & 0xff00) >> 24) << 24; +} + +static u32 convert_xrgb_to_abgr(u32 pix) +{ + return ((pix & 0x00ff) >> 16) << 0 | + ((pix & 0xff00) >> 8) << 8 | + ((pix & 0x00ff) >> 0) << 16 | + GENMASK(31, 24); /* fill alpha bits */ +} + +static u32 convert_xrgb_to_xrgb2101010(u32 pix) +{ + pix = ((pix & 0x00FF) << 2) | + ((pix & 0xFF00) << 4) | + ((pix & 0x00FF) << 6); + return pix | ((pix >> 8) & 0x00300C03); +} + +static u32 convert_xrgb_to_argb2101010(u32 pix) +{ + pix = ((pix & 0x00FF) << 2) | + ((pix & 0xFF00) << 4) | + ((pix & 0x00FF) << 6); + return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) & 0x00300C03); +} + +/* + * convert_from_xrgb - convert one pixel from xrgb to the desired format + * @color: input color, in xrgb format + * @format: output format + * + * Returns: + * Color in the format specified, casted to u32. + * Or 0 if the format is not supported. + */ +static u32 convert_from_xrgb(u32 color, u32 format) +{ + switch (format) { + case DRM_FORMAT_RGB565: + return convert_xrgb_to_rgb565(color); + case DRM_FORMAT_RGBA5551: + return convert_xrgb_to_rgba5551(color); + case DRM_FORMAT_XRGB1555: + return convert_xrgb_to_xrgb1555(color); + case DRM_FORMAT_ARGB1555: + return convert_xrgb_to_argb1555(color); + case DRM_FORMAT_RGB888: + case DRM_FORMAT_XRGB: + return color; + case DRM_FORMAT_ARGB: + return convert_xrgb_to_argb(color); + case DRM_FORMAT_XBGR: + return convert_xrgb_to_xbgr(color); + case DRM_FORMAT_ABGR: + return convert_xrgb_to_abgr(color); + case DRM_FORMAT_XRGB2101010: + return convert_xrgb_to_xrgb2101010(color); + case DRM_FORMAT_ARGB2101010: + return convert_xrgb_to_argb2101010(color); + default: + WARN_ONCE(1, "Can't convert to %p4cc\n", ); + return 0; + } +} + +/* + * Blit & Fill + */ +static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch, +const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -
[PATCH v12 2/9] drm/panic: Add a drm panic handler
This module displays a user friendly message when a kernel panic occurs. It currently doesn't contain any debug information, but that can be added later. v2 * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann) * Add the panic reason to the panic message (Nerdopolis) * Add an exclamation mark (Nerdopolis) v3 * Rework the drawing functions, to write the pixels line by line and to use the drm conversion helper to support other formats. (Thomas Zimmermann) v4 * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann) * Remove the default y to DRM_PANIC config option (Thomas Zimmermann) * Add foreground/background color config option * Fix the bottom lines not painted if the framebuffer height is not a multiple of the font height. * Automatically register the device to drm_panic, if the function get_scanout_buffer exists. (Thomas Zimmermann) v5 * Change the drawing API, use drm_fb_blit_from_r1() to draw the font. * Also add drm_fb_fill() to fill area with background color. * Add draw_pixel_xy() API for drivers that can't provide a linear buffer. * Add a flush() callback for drivers that needs to synchronize the buffer. * Add a void *private field, so drivers can pass private data to draw_pixel_xy() and flush(). v6 * Fix sparse warning for panic_msg and logo. v7 * Add select DRM_KMS_HELPER for the color conversion functions. v8 * Register directly each plane to the panic notifier (Sima) * Add raw_spinlock to properly handle concurrency (Sima) * Register plane instead of device, to avoid looping through plane list, and simplify code. * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) * Removed the draw_pixel_xy() API, will see later if it can be added back. v9 * Revert to using get_scanout_buffer() (Sima) * Move get_scanout_buffer() and panic_flush() to the plane helper functions (Thomas Zimmermann) * Register all planes with get_scanout_buffer() to the panic notifier * Use drm_panic_lock() to protect against race (Sima) v10 * Move blit and fill functions back in drm_panic (Thomas Zimmermann). * Simplify the text drawing functions. * Use kmsg_dumper instead of panic_notifier (Sima). v12 * Use array for map and pitch in struct drm_scanout_buffer to support multi-planar format later. (Thomas Zimmermann) * Better indent struct drm_scanout_buffer declaration. (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe --- Documentation/gpu/drm-kms.rst| 12 + drivers/gpu/drm/Kconfig | 23 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_drv.c| 4 + drivers/gpu/drm/drm_panic.c | 289 +++ include/drm/drm_modeset_helper_vtables.h | 37 +++ include/drm/drm_panic.h | 57 + include/drm/drm_plane.h | 6 + 8 files changed, 429 insertions(+) create mode 100644 drivers/gpu/drm/drm_panic.c diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 13d3627d8bc0..b64334661aeb 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -398,6 +398,18 @@ Plane Damage Tracking Functions Reference .. kernel-doc:: include/drm/drm_damage_helper.h :internal: +Plane Panic Feature +--- + +.. kernel-doc:: drivers/gpu/drm/drm_panic.c + :doc: overview + +Plane Panic Functions Reference +--- + +.. kernel-doc:: drivers/gpu/drm/drm_panic.c + :export: + Display Modes Function Reference diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 3914aaf443a8..f8a26423369e 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -104,6 +104,29 @@ config DRM_KMS_HELPER help CRTC helpers for KMS drivers. +config DRM_PANIC + bool "Display a user-friendly message when a kernel panic occurs" + depends on DRM && !FRAMEBUFFER_CONSOLE + select DRM_KMS_HELPER + select FONT_SUPPORT + help + Enable a drm panic handler, which will display a user-friendly message + when a kernel panic occurs. It's useful when using a user-space + console instead of fbcon. + It will only work if your graphic driver supports this feature. + To support Hi-DPI Display, you can enable bigger fonts like + FONT_TER16x32 + +config DRM_PANIC_FOREGROUND_COLOR + hex "Drm panic screen foreground color, in RGB" + depends on DRM_PANIC + default 0xff + +config DRM_PANIC_BACKGROUND_COLOR + hex "Drm panic screen background color, in RGB" + depends on DRM_PANIC + default 0x00 + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index
[PATCH v12 1/9] drm/panic: Add drm panic locking
From: Daniel Vetter Rough sketch for the locking of drm panic printing code. The upshot of this approach is that we can pretty much entirely rely on the atomic commit flow, with the pair of raw_spin_lock/unlock providing any barriers we need, without having to create really big critical sections in code. This also avoids the need that drivers must explicitly update the panic handler state, which they might forget to do, or not do consistently, and then we blow up in the worst possible times. It is somewhat racy against a concurrent atomic update, and we might write into a buffer which the hardware will never display. But there's fundamentally no way to avoid that - if we do the panic state update explicitly after writing to the hardware, we might instead write to an old buffer that the user will barely ever see. Note that an rcu protected deference of plane->state would give us the the same guarantees, but it has the downside that we then need to protect the plane state freeing functions with call_rcu too. Which would very widely impact a lot of code and therefore doesn't seem worth the complexity compared to a raw spinlock with very tiny critical sections. Plus rcu cannot be used to protect access to peek/poke registers anyway, so we'd still need it for those cases. Peek/poke registers for vram access (or a gart pte reserved just for panic code) are also the reason I've gone with a per-device and not per-plane spinlock, since usually these things are global for the entire display. Going with per-plane locks would mean drivers for such hardware would need additional locks, which we don't want, since it deviates from the per-console takeoverlocks design. Longer term it might be useful if the panic notifiers grow a bit more structure than just the absolute bare EXPORT_SYMBOL(panic_notifier_list) - somewhat aside, why is that not EXPORT_SYMBOL_GPL ... If panic notifiers would be more like console drivers with proper register/unregister interfaces we could perhaps reuse the very fancy console lock with all it's check and takeover semantics that John Ogness is developing to fix the console_lock mess. But for the initial cut of a drm panic printing support I don't think we need that, because the critical sections are extremely small and only happen once per display refresh. So generally just 60 tiny locked sections per second, which is nothing compared to a serial console running a 115kbaud doing really slow mmio writes for each byte. So for now the raw spintrylock in drm panic notifier callback should be good enough. Another benefit of making panic notifiers more like full blown consoles (that are used in panics only) would be that we get the two stage design, where first all the safe outputs are used. And then the dangerous takeover tricks are deployed (where for display drivers we also might try to intercept any in-flight display buffer flips, which if we race and misprogram fifos and watermarks can hang the memory controller on some hw). For context the actual implementation on the drm side is by Jocelyn and this patch is meant to be combined with the overall approach in v7 (v8 is a bit less flexible, which I think is the wrong direction): https://lore.kernel.org/dri-devel/20240104160301.185915-1-jfale...@redhat.com/ Note that the locking is very much not correct there, hence this separate rfc. v2: - fix authorship, this was all my typing - some typo oopsies - link to the drm panic work by Jocelyn for context v10: - Use spinlock_irqsave/restore (John Ogness) v11: - Use macro instead of inline functions for drm_panic_lock/unlock (John Ogness) Signed-off-by: Daniel Vetter Cc: Jocelyn Falempe Cc: Andrew Morton Cc: "Peter Zijlstra (Intel)" Cc: Lukas Wunner Cc: Petr Mladek Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++ drivers/gpu/drm/drm_drv.c | 1 + include/drm/drm_mode_config.h | 10 +++ include/drm/drm_panic.h | 100 4 files changed, 115 insertions(+) create mode 100644 include/drm/drm_panic.h diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 39ef0a6addeb..fb97b51b38f1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -3016,6 +3017,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall) { int i, ret; + unsigned long flags; struct drm_connector *connector; struct drm_connector_state *old_conn_state, *new_conn_state; struct drm_crtc *crtc; @@ -3099,6 +3101,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_
Re: [PATCH v11 5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
On 09/04/2024 10:21, Thomas Zimmermann wrote: Hi Am 28.03.24 um 13:03 schrieb Jocelyn Falempe: This was initialy done for imx6, but should work on most drivers using drm_fb_dma_helper. v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * go back to get_scanout_buffer() * move get_scanout_buffer() to plane helper functions Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_dma_helper.c | 47 + include/drm/drm_fb_dma_helper.h | 4 +++ 2 files changed, 51 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index 3b535ad1b07c..010327069ad4 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -148,3 +149,49 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, } } EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent); + +#if defined(CONFIG_DRM_PANIC) I would not bother with CONFIG_DRM_PANIC for now and make the helper generally available. yes, that's a small function, let's keep it simple. Done in v12 +/** + * @plane: DRM primary plane + * @drm_scanout_buffer: scanout buffer for the panic handler + * Returns: 0 or negative error code + * + * Generic get_scanout_buffer() implementation, for drivers that uses the + * drm_fb_dma_helper. + */ +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) It's neither really a function for panic handling nor a GEM function. This helper needs to be called drm_fb_dma_get_scanout_buffer() IMHO. I agree, it matches the other functions prefixes in this file. Done in v12 +{ + struct drm_gem_dma_object *dma_obj; + struct drm_framebuffer *fb; + + fb = plane->state->fb; + /* Only support linear modifier */ + if (fb->modifier != DRM_FORMAT_MOD_LINEAR) + return -ENODEV; + + dma_obj = drm_fb_dma_get_gem_obj(fb, 0); + + /* Buffer should be accessible from the CPU */ + if (dma_obj->base.import_attach) + return -ENODEV; + + /* Buffer should be already mapped to CPU */ + if (!dma_obj->vaddr) + return -ENODEV; + + iosys_map_set_vaddr(>map, dma_obj->vaddr); + sb->format = fb->format; + sb->height = fb->height; + sb->width = fb->width; + sb->pitch = fb->pitches[0]; + return 0; +} +#else +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) +{ + return -ENODEV; +} +#endif +EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer); diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h index d5e036c57801..61f24c2aba2f 100644 --- a/include/drm/drm_fb_dma_helper.h +++ b/include/drm/drm_fb_dma_helper.h @@ -7,6 +7,7 @@ struct drm_device; struct drm_framebuffer; struct drm_plane_state; +struct drm_scanout_buffer; struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb, unsigned int plane); @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, struct drm_plane_state *old_state, struct drm_plane_state *state); +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb); + #endif Thanks for the reviews, -- Jocelyn
Re: [PATCH v11 2/9] drm/panic: Add a drm panic handler
Hi, On 09/04/2024 10:30, Thomas Zimmermann wrote: Hi Am 28.03.24 um 13:03 schrieb Jocelyn Falempe: +/** + * struct drm_scanout_buffer - DRM scanout buffer + * + * This structure holds the information necessary for drm_panic to draw the + * panic screen, and display it. + */ +struct drm_scanout_buffer { + /** + * @format: + * + * drm format of the scanout buffer. + */ + const struct drm_format_info *format; Newline here and among the other fields please. Done in v12. + /** + * @map: + * + * Virtual address of the scanout buffer, either in memory or iomem. + * The scanout buffer should be in linear format, and can be directly + * sent to the display hardware. Tearing is not an issue for the panic + * screen. + */ + struct iosys_map map; I would make this an array of DRM_FORMAT_MAX_PLANES. Its functionality is then equivalent to the fields in struct drm_framebuffer. Supporting multiple color planes is the general expectation in the DRM code, even if not all parts actually implement it. In the panic code, you simply test that the scan-out format has only a single plane. struct iosys_map map[DRM_FORMAT_MAX_PLANES] Sure, that was on my todo list, done in v12. + /** + * @width: Width of the scanout buffer, in pixels. + */ + unsigned int width; + /** + * @height: Height of the scanout buffer, in pixels. + */ + unsigned int height; + /** + * @pitch: Length in bytes between the start of two consecutive lines. + */ + unsigned int pitch; Also use an array of DRM_FORMAT_MAX_PLANES. +}; This data structure looks like something I could use for the shadow-plane helpers. Expect it to be moved elsewhere at some point. Yes, this can even be part of the struct drm_framebuffer. Best regards Thomas Thanks for the reviews, -- Jocelyn
Re: [PATCH 00/11] drm/mgag200: Detect connector status
Hi Thomas, I've tested this series on my Dell T310, and it works well, when I plug/unplug the VGA screen, it's reflected in /sys/class/drm/.../status I've also tested it remotely on a Dell R640, which doesn't have a VGA monitor connected. After the patch, on the iDrac console, I only get a green screen saying: "Out of Range Reason: Video Capture Failure Detected Resolution: 0x768 Detected Color Depth-1bpp" and the file: /sys/class/drm/card0-VGA-1/modes is empty Before the patch, the driver reports VGA as connected, and modes contains 1024x768 and others. I think we may need to add a virtual connector for BMC, like I've done for AST ? So that when no VGA monitor is available, you can still choose an appropriate resolution. Best regards, -- Jocelyn On 03/04/2024 11:24, Thomas Zimmermann wrote: Detect the connector status by polling the DDC. Update the status at runtime. Clean up a the driver's DDC code in the process. Patches 1 and 2 fix long-standing problems in the DDC code. Patches 3 to 9 refactor the DDC code. The code then keeps its data structures internal, acquires locks automatically and it much more readable overall. With patches 10 and 11, mgag200 makes use of existing helpers for reading and probing the DDC. It then correctly updates the status and EDID at runtime. Tested on various Matrox hardware. Thomas Zimmermann (11): drm/mgag200: Set DDC timeout in milliseconds drm/mgag200: Bind I2C lifetime to DRM device drm/mgag200: Store pointer to struct mga_device in struct mga_i2c_chan drm/mgag200: Allocate instance of struct mga_i2c_chan dynamically drm/mgag200: Inline mgag200_i2c_init() drm/mgag200: Replace struct mga_i2c_chan with struct mgag200_ddc drm/mgag200: Rename mgag200_i2c.c to mgag200_ddc.c drm/mgag200: Rename struct i2c_algo_bit_data callbacks drm/mgag200: Acquire I/O-register lock in DDC code drm/mgag200: Use drm_connector_helper_get_modes() drm/mgag200: Set .detect_ctx() and enable connector polling drivers/gpu/drm/mgag200/Makefile | 2 +- drivers/gpu/drm/mgag200/mgag200_ddc.c | 179 ++ drivers/gpu/drm/mgag200/mgag200_ddc.h | 11 ++ drivers/gpu/drm/mgag200/mgag200_drv.h | 19 +-- drivers/gpu/drm/mgag200/mgag200_g200.c| 15 +- drivers/gpu/drm/mgag200/mgag200_g200eh.c | 15 +- drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 15 +- drivers/gpu/drm/mgag200/mgag200_g200er.c | 15 +- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 15 +- drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 15 +- drivers/gpu/drm/mgag200/mgag200_g200se.c | 15 +- drivers/gpu/drm/mgag200/mgag200_g200wb.c | 15 +- drivers/gpu/drm/mgag200/mgag200_i2c.c | 129 drivers/gpu/drm/mgag200/mgag200_mode.c| 27 +--- 14 files changed, 274 insertions(+), 213 deletions(-) create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.c create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.h delete mode 100644 drivers/gpu/drm/mgag200/mgag200_i2c.c
[PATCH v11 5/9] drm/fb_dma: Add generic get_scanout_buffer() for drm_panic
This was initialy done for imx6, but should work on most drivers using drm_fb_dma_helper. v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * go back to get_scanout_buffer() * move get_scanout_buffer() to plane helper functions Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_fb_dma_helper.c | 47 + include/drm/drm_fb_dma_helper.h | 4 +++ 2 files changed, 51 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c index 3b535ad1b07c..010327069ad4 100644 --- a/drivers/gpu/drm/drm_fb_dma_helper.c +++ b/drivers/gpu/drm/drm_fb_dma_helper.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -148,3 +149,49 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, } } EXPORT_SYMBOL_GPL(drm_fb_dma_sync_non_coherent); + +#if defined(CONFIG_DRM_PANIC) +/** + * @plane: DRM primary plane + * @drm_scanout_buffer: scanout buffer for the panic handler + * Returns: 0 or negative error code + * + * Generic get_scanout_buffer() implementation, for drivers that uses the + * drm_fb_dma_helper. + */ +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb) +{ + struct drm_gem_dma_object *dma_obj; + struct drm_framebuffer *fb; + + fb = plane->state->fb; + /* Only support linear modifier */ + if (fb->modifier != DRM_FORMAT_MOD_LINEAR) + return -ENODEV; + + dma_obj = drm_fb_dma_get_gem_obj(fb, 0); + + /* Buffer should be accessible from the CPU */ + if (dma_obj->base.import_attach) + return -ENODEV; + + /* Buffer should be already mapped to CPU */ + if (!dma_obj->vaddr) + return -ENODEV; + + iosys_map_set_vaddr(>map, dma_obj->vaddr); + sb->format = fb->format; + sb->height = fb->height; + sb->width = fb->width; + sb->pitch = fb->pitches[0]; + return 0; +} +#else +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb) +{ + return -ENODEV; +} +#endif +EXPORT_SYMBOL(drm_panic_gem_get_scanout_buffer); diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h index d5e036c57801..61f24c2aba2f 100644 --- a/include/drm/drm_fb_dma_helper.h +++ b/include/drm/drm_fb_dma_helper.h @@ -7,6 +7,7 @@ struct drm_device; struct drm_framebuffer; struct drm_plane_state; +struct drm_scanout_buffer; struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb, unsigned int plane); @@ -19,5 +20,8 @@ void drm_fb_dma_sync_non_coherent(struct drm_device *drm, struct drm_plane_state *old_state, struct drm_plane_state *state); +int drm_panic_gem_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb); + #endif -- 2.44.0
[PATCH v11 9/9] drm/ast: Add drm_panic support
Add support for the drm_panic module, which displays a message to the screen when a kernel panic occurs. v7 * Use drm_for_each_primary_visible_plane() v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions Signed-off-by: Jocelyn Falempe Acked-by: Sui Jingfeng Tested-by: Sui Jingfeng --- drivers/gpu/drm/ast/ast_mode.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index a718646a66b8..49f2d8bd3377 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -43,6 +43,7 @@ #include #include #include +#include #include #include @@ -700,12 +701,29 @@ static void ast_primary_plane_helper_atomic_disable(struct drm_plane *plane, ast_set_index_reg_mask(ast, AST_IO_VGASRI, 0x1, 0xdf, 0x20); } +static int ast_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) +{ + struct ast_plane *ast_plane = to_ast_plane(plane); + + if (plane->state && plane->state->fb && ast_plane->vaddr) { + sb->format = plane->state->fb->format; + sb->width = plane->state->fb->width; + sb->height = plane->state->fb->height; + sb->pitch = plane->state->fb->pitches[0]; + iosys_map_set_vaddr_iomem(>map, ast_plane->vaddr); + return 0; + } + return -ENODEV; +} + static const struct drm_plane_helper_funcs ast_primary_plane_helper_funcs = { DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, .atomic_check = ast_primary_plane_helper_atomic_check, .atomic_update = ast_primary_plane_helper_atomic_update, .atomic_enable = ast_primary_plane_helper_atomic_enable, .atomic_disable = ast_primary_plane_helper_atomic_disable, + .get_scanout_buffer = ast_primary_plane_helper_get_scanout_buffer, }; static const struct drm_plane_funcs ast_primary_plane_funcs = { -- 2.44.0
[PATCH v11 6/9] drm/simpledrm: Add drm_panic support
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) v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/tiny/simpledrm.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c index 7ce1c4617675..f498665be8c4 100644 --- a/drivers/gpu/drm/tiny/simpledrm.c +++ b/drivers/gpu/drm/tiny/simpledrm.c @@ -25,6 +25,7 @@ #include #include #include +#include #include #define DRIVER_NAME"simpledrm" @@ -671,11 +672,26 @@ static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plan drm_dev_exit(idx); } +static int simpledrm_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, +struct drm_scanout_buffer *sb) +{ + struct simpledrm_device *sdev = simpledrm_device_of_dev(plane->dev); + + sb->width = sdev->mode.hdisplay; + sb->height = sdev->mode.vdisplay; + sb->format = sdev->format; + sb->pitch = sdev->pitch; + sb->map = sdev->screen_base; + + return 0; +} + static const struct drm_plane_helper_funcs simpledrm_primary_plane_helper_funcs = { DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, .atomic_check = simpledrm_primary_plane_helper_atomic_check, .atomic_update = simpledrm_primary_plane_helper_atomic_update, .atomic_disable = simpledrm_primary_plane_helper_atomic_disable, + .get_scanout_buffer = simpledrm_primary_plane_helper_get_scanout_buffer, }; static const struct drm_plane_funcs simpledrm_primary_plane_funcs = { -- 2.44.0
[PATCH v11 7/9] drm/mgag200: Add drm_panic support
Add support for the drm_panic module, which displays a message to the screen when a kernel panic occurs. v5: * Also check that the plane is visible and primary. (Thomas Zimmermann) v7: * use drm_for_each_primary_visible_plane() v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions (Thomas Zimmermann) Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ++- drivers/gpu/drm/mgag200/mgag200_mode.c | 18 ++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 765e49fd8911..58a0e62eaf18 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -366,6 +366,7 @@ struct drm_crtc_state; struct drm_display_mode; struct drm_plane; struct drm_atomic_state; +struct drm_scanout_buffer; extern const uint32_t mgag200_primary_plane_formats[]; extern const size_t mgag200_primary_plane_formats_size; @@ -379,12 +380,16 @@ void mgag200_primary_plane_helper_atomic_enable(struct drm_plane *plane, struct drm_atomic_state *state); void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane, struct drm_atomic_state *old_state); +int mgag200_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb); + #define MGAG200_PRIMARY_PLANE_HELPER_FUNCS \ DRM_GEM_SHADOW_PLANE_HELPER_FUNCS, \ .atomic_check = mgag200_primary_plane_helper_atomic_check, \ .atomic_update = mgag200_primary_plane_helper_atomic_update, \ .atomic_enable = mgag200_primary_plane_helper_atomic_enable, \ - .atomic_disable = mgag200_primary_plane_helper_atomic_disable + .atomic_disable = mgag200_primary_plane_helper_atomic_disable, \ + .get_scanout_buffer = mgag200_primary_plane_helper_get_scanout_buffer #define MGAG200_PRIMARY_PLANE_FUNCS \ .update_plane = drm_atomic_helper_update_plane, \ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index e17cb4c5f774..8f368ecca4d4 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include "mgag200_drv.h" @@ -546,6 +547,23 @@ void mgag200_primary_plane_helper_atomic_disable(struct drm_plane *plane, msleep(20); } +int mgag200_primary_plane_helper_get_scanout_buffer(struct drm_plane *plane, + struct drm_scanout_buffer *sb) +{ + struct mga_device *mdev = to_mga_device(plane->dev); + struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram); + + if (plane->state && plane->state->fb) { + sb->format = plane->state->fb->format; + sb->width = plane->state->fb->width; + sb->height = plane->state->fb->height; + sb->pitch = plane->state->fb->pitches[0]; + sb->map = map; + return 0; + } + return -ENODEV; +} + /* * CRTC */ -- 2.44.0
[PATCH v11 8/9] drm/imx: Add drm_panic support
Add support for the drm_panic module, which displays a user-friendly message to the screen when a kernel panic occurs. v7: * use drm_panic_gem_get_scanout_buffer() helper v8: * Replace get_scanout_buffer() logic with drm_panic_set_buffer() v9: * Revert to using get_scanout_buffer() (Sima) * move get_scanout_buffer() to plane helper functions Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c index dade8b59feae..3e21d2a1a124 100644 --- a/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c @@ -772,6 +772,12 @@ static const struct drm_plane_helper_funcs ipu_plane_helper_funcs = { .atomic_disable = ipu_plane_atomic_disable, .atomic_update = ipu_plane_atomic_update, }; +static const struct drm_plane_helper_funcs ipu_primary_plane_helper_funcs = { + .atomic_check = ipu_plane_atomic_check, + .atomic_disable = ipu_plane_atomic_disable, + .atomic_update = ipu_plane_atomic_update, + .get_scanout_buffer = drm_panic_gem_get_scanout_buffer, +}; bool ipu_plane_atomic_update_pending(struct drm_plane *plane) { @@ -916,7 +922,10 @@ struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu, ipu_plane->dma = dma; ipu_plane->dp_flow = dp; - drm_plane_helper_add(_plane->base, _plane_helper_funcs); + if (type == DRM_PLANE_TYPE_PRIMARY) + drm_plane_helper_add(_plane->base, _primary_plane_helper_funcs); + else + drm_plane_helper_add(_plane->base, _plane_helper_funcs); if (dp == IPU_DP_FLOW_SYNC_BG || dp == IPU_DP_FLOW_SYNC_FG) ret = drm_plane_create_zpos_property(_plane->base, zpos, 0, -- 2.44.0
[PATCH v11 3/9] drm/panic: Add support for color format conversion
Add support for the following formats: DRM_FORMAT_RGB565 DRM_FORMAT_RGBA5551 DRM_FORMAT_XRGB1555 DRM_FORMAT_ARGB1555 DRM_FORMAT_RGB888 DRM_FORMAT_XRGB DRM_FORMAT_ARGB DRM_FORMAT_XBGR DRM_FORMAT_XRGB2101010 DRM_FORMAT_ARGB2101010 v10: * move and simplify the functions from the drm format helper to drm_panic Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_panic.c | 272 ++-- 1 file changed, 262 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 72b5b363bcee..7761779a884b 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -81,15 +81,155 @@ static const struct drm_panic_line logo[] = { PANIC_LINE(" \\___)=(___/"), }; -static void drm_panic_fill32(struct iosys_map *map, unsigned int pitch, +/* + * Color conversion + */ + +static u16 convert_xrgb_to_rgb565(u32 pix) +{ + return ((pix & 0x00F8) >> 8) | + ((pix & 0xFC00) >> 5) | + ((pix & 0x00F8) >> 3); +} + +static u16 convert_xrgb_to_rgba5551(u32 pix) +{ + return ((pix & 0x00f8) >> 8) | + ((pix & 0xf800) >> 5) | + ((pix & 0x00f8) >> 2) | + BIT(0); /* set alpha bit */ +} + +static u16 convert_xrgb_to_xrgb1555(u32 pix) +{ + return ((pix & 0x00f8) >> 9) | + ((pix & 0xf800) >> 6) | + ((pix & 0x00f8) >> 3); +} + +static u16 convert_xrgb_to_argb1555(u32 pix) +{ + return BIT(15) | /* set alpha bit */ + ((pix & 0x00f8) >> 9) | + ((pix & 0xf800) >> 6) | + ((pix & 0x00f8) >> 3); +} + +static u32 convert_xrgb_to_argb(u32 pix) +{ + return pix | GENMASK(31, 24); /* fill alpha bits */ +} + +static u32 convert_xrgb_to_xbgr(u32 pix) +{ + return ((pix & 0x00ff) >> 16) << 0 | + ((pix & 0xff00) >> 8) << 8 | + ((pix & 0x00ff) >> 0) << 16 | + ((pix & 0xff00) >> 24) << 24; +} + +static u32 convert_xrgb_to_abgr(u32 pix) +{ + return ((pix & 0x00ff) >> 16) << 0 | + ((pix & 0xff00) >> 8) << 8 | + ((pix & 0x00ff) >> 0) << 16 | + GENMASK(31, 24); /* fill alpha bits */ +} + +static u32 convert_xrgb_to_xrgb2101010(u32 pix) +{ + pix = ((pix & 0x00FF) << 2) | + ((pix & 0xFF00) << 4) | + ((pix & 0x00FF) << 6); + return pix | ((pix >> 8) & 0x00300C03); +} + +static u32 convert_xrgb_to_argb2101010(u32 pix) +{ + pix = ((pix & 0x00FF) << 2) | + ((pix & 0xFF00) << 4) | + ((pix & 0x00FF) << 6); + return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) & 0x00300C03); +} + +/* + * convert_from_xrgb - convert one pixel from xrgb to the desired format + * @color: input color, in xrgb format + * @format: output format + * + * Returns: + * Color in the format specified, casted to u32. + * Or 0 if the format is not supported. + */ +static u32 convert_from_xrgb(u32 color, u32 format) +{ + switch (format) { + case DRM_FORMAT_RGB565: + return convert_xrgb_to_rgb565(color); + case DRM_FORMAT_RGBA5551: + return convert_xrgb_to_rgba5551(color); + case DRM_FORMAT_XRGB1555: + return convert_xrgb_to_xrgb1555(color); + case DRM_FORMAT_ARGB1555: + return convert_xrgb_to_argb1555(color); + case DRM_FORMAT_RGB888: + case DRM_FORMAT_XRGB: + return color; + case DRM_FORMAT_ARGB: + return convert_xrgb_to_argb(color); + case DRM_FORMAT_XBGR: + return convert_xrgb_to_xbgr(color); + case DRM_FORMAT_ABGR: + return convert_xrgb_to_abgr(color); + case DRM_FORMAT_XRGB2101010: + return convert_xrgb_to_xrgb2101010(color); + case DRM_FORMAT_ARGB2101010: + return convert_xrgb_to_argb2101010(color); + default: + WARN_ONCE(1, "Can't convert to %p4cc\n", ); + return 0; + } +} + +/* + * Blit & Fill + */ +static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch, +const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, -u32 color) +u16 fg16, u16 bg16) { unsigned int y, x; + u16 val16; -
[PATCH v11 4/9] drm/panic: Add debugfs entry to test without triggering panic.
Add a debugfs file, so you can test drm_panic without freezing your machine. This is unsafe, and should be enabled only for developer or tester. To display the drm_panic screen on the device 0: echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0 v9: * Create a debugfs file for each plane in the device's debugfs directory. This allows to test for each plane of each GPU independently. Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/Kconfig | 9 drivers/gpu/drm/drm_panic.c | 43 - 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index f07ca38d3f98..6e41fbd16b3d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -125,6 +125,15 @@ config DRM_PANIC_BACKGROUND_COLOR depends on DRM_PANIC default 0x00 +config DRM_PANIC_DEBUG + bool "Add a debug fs entry to trigger drm_panic" + depends on DRM_PANIC && DEBUG_FS + help + Add dri/[device]/drm_panic_plane_x in the kernel debugfs, to force the + panic handler to write the panic message to this plane scanout buffer. + This is unsafe and should not be enabled on a production build. + If in doubt, say "N". + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c index 7761779a884b..b75e90da7f39 100644 --- a/drivers/gpu/drm/drm_panic.c +++ b/drivers/gpu/drm/drm_panic.c @@ -493,6 +493,45 @@ static void drm_panic(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason) draw_panic_plane(plane); } + +/* + * DEBUG FS, This is currently unsafe. + * Create one file per plane, so it's possible to debug one plane at a time. + */ +#ifdef CONFIG_DRM_PANIC_DEBUG +#include + +static ssize_t debugfs_trigger_write(struct file *file, const char __user *user_buf, +size_t count, loff_t *ppos) +{ + bool run; + + if (kstrtobool_from_user(user_buf, count, ) == 0 && run) { + struct drm_plane *plane = file->private_data; + + draw_panic_plane(plane); + } + return count; +} + +static const struct file_operations dbg_drm_panic_ops = { + .owner = THIS_MODULE, + .write = debugfs_trigger_write, + .open = simple_open, +}; + +static void debugfs_register_plane(struct drm_plane *plane, int index) +{ + char fname[32]; + + snprintf(fname, 32, "drm_panic_plane_%d", index); + debugfs_create_file(fname, 0200, plane->dev->debugfs_root, + plane, _drm_panic_ops); +} +#else +static void debugfs_register_plane(struct drm_plane *plane, int index) {} +#endif /* CONFIG_DRM_PANIC_DEBUG */ + /** * drm_panic_register() - Initialize DRM panic for a device * @dev: the drm device on which the panic screen will be displayed. @@ -512,8 +551,10 @@ void drm_panic_register(struct drm_device *dev) plane->kmsg_panic.max_reason = KMSG_DUMP_PANIC; if (kmsg_dump_register(>kmsg_panic)) drm_warn(dev, "Failed to register panic handler\n"); - else + else { + debugfs_register_plane(plane, registered_plane); registered_plane++; + } } if (registered_plane) drm_info(dev, "Registered %d planes with drm panic\n", registered_plane); -- 2.44.0
[PATCH v11 2/9] drm/panic: Add a drm panic handler
This module displays a user friendly message when a kernel panic occurs. It currently doesn't contain any debug information, but that can be added later. v2 * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann) * Add the panic reason to the panic message (Nerdopolis) * Add an exclamation mark (Nerdopolis) v3 * Rework the drawing functions, to write the pixels line by line and to use the drm conversion helper to support other formats. (Thomas Zimmermann) v4 * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann) * Remove the default y to DRM_PANIC config option (Thomas Zimmermann) * Add foreground/background color config option * Fix the bottom lines not painted if the framebuffer height is not a multiple of the font height. * Automatically register the device to drm_panic, if the function get_scanout_buffer exists. (Thomas Zimmermann) v5 * Change the drawing API, use drm_fb_blit_from_r1() to draw the font. * Also add drm_fb_fill() to fill area with background color. * Add draw_pixel_xy() API for drivers that can't provide a linear buffer. * Add a flush() callback for drivers that needs to synchronize the buffer. * Add a void *private field, so drivers can pass private data to draw_pixel_xy() and flush(). v6 * Fix sparse warning for panic_msg and logo. v7 * Add select DRM_KMS_HELPER for the color conversion functions. v8 * Register directly each plane to the panic notifier (Sima) * Add raw_spinlock to properly handle concurrency (Sima) * Register plane instead of device, to avoid looping through plane list, and simplify code. * Replace get_scanout_buffer() logic with drm_panic_set_buffer() (Thomas Zimmermann) * Removed the draw_pixel_xy() API, will see later if it can be added back. v9 * Revert to using get_scanout_buffer() (Sima) * Move get_scanout_buffer() and panic_flush() to the plane helper functions (Thomas Zimmermann) * Register all planes with get_scanout_buffer() to the panic notifier * Use drm_panic_lock() to protect against race (Sima) v10 * Move blit and fill functions back in drm_panic (Thomas Zimmermann). * Simplify the text drawing functions. * Use kmsg_dumper instead of panic_notifier (Sima). Signed-off-by: Jocelyn Falempe --- Documentation/gpu/drm-kms.rst| 12 + drivers/gpu/drm/Kconfig | 23 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_drv.c| 4 + drivers/gpu/drm/drm_panic.c | 288 +++ include/drm/drm_modeset_helper_vtables.h | 37 +++ include/drm/drm_panic.h | 52 include/drm/drm_plane.h | 6 + 8 files changed, 423 insertions(+) create mode 100644 drivers/gpu/drm/drm_panic.c diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 13d3627d8bc0..b64334661aeb 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -398,6 +398,18 @@ Plane Damage Tracking Functions Reference .. kernel-doc:: include/drm/drm_damage_helper.h :internal: +Plane Panic Feature +--- + +.. kernel-doc:: drivers/gpu/drm/drm_panic.c + :doc: overview + +Plane Panic Functions Reference +--- + +.. kernel-doc:: drivers/gpu/drm/drm_panic.c + :export: + Display Modes Function Reference diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 16029435b750..f07ca38d3f98 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -102,6 +102,29 @@ config DRM_KMS_HELPER help CRTC helpers for KMS drivers. +config DRM_PANIC + bool "Display a user-friendly message when a kernel panic occurs" + depends on DRM && !FRAMEBUFFER_CONSOLE + select DRM_KMS_HELPER + select FONT_SUPPORT + help + Enable a drm panic handler, which will display a user-friendly message + when a kernel panic occurs. It's useful when using a user-space + console instead of fbcon. + It will only work if your graphic driver supports this feature. + To support Hi-DPI Display, you can enable bigger fonts like + FONT_TER16x32 + +config DRM_PANIC_FOREGROUND_COLOR + hex "Drm panic screen foreground color, in RGB" + depends on DRM_PANIC + default 0xff + +config DRM_PANIC_BACKGROUND_COLOR + hex "Drm panic screen background color, in RGB" + depends on DRM_PANIC + default 0x00 + config DRM_DEBUG_DP_MST_TOPOLOGY_REFS bool "Enable refcount backtrace history in the DP MST helpers" depends on STACKTRACE_SUPPORT diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index a73c04d2d7a3..f9ca4f8fa6c5 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -88,6 +88,7 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \ drm_privacy_screen.o \
[PATCH v11 1/9] drm/panic: Add drm panic locking
From: Daniel Vetter Rough sketch for the locking of drm panic printing code. The upshot of this approach is that we can pretty much entirely rely on the atomic commit flow, with the pair of raw_spin_lock/unlock providing any barriers we need, without having to create really big critical sections in code. This also avoids the need that drivers must explicitly update the panic handler state, which they might forget to do, or not do consistently, and then we blow up in the worst possible times. It is somewhat racy against a concurrent atomic update, and we might write into a buffer which the hardware will never display. But there's fundamentally no way to avoid that - if we do the panic state update explicitly after writing to the hardware, we might instead write to an old buffer that the user will barely ever see. Note that an rcu protected deference of plane->state would give us the the same guarantees, but it has the downside that we then need to protect the plane state freeing functions with call_rcu too. Which would very widely impact a lot of code and therefore doesn't seem worth the complexity compared to a raw spinlock with very tiny critical sections. Plus rcu cannot be used to protect access to peek/poke registers anyway, so we'd still need it for those cases. Peek/poke registers for vram access (or a gart pte reserved just for panic code) are also the reason I've gone with a per-device and not per-plane spinlock, since usually these things are global for the entire display. Going with per-plane locks would mean drivers for such hardware would need additional locks, which we don't want, since it deviates from the per-console takeoverlocks design. Longer term it might be useful if the panic notifiers grow a bit more structure than just the absolute bare EXPORT_SYMBOL(panic_notifier_list) - somewhat aside, why is that not EXPORT_SYMBOL_GPL ... If panic notifiers would be more like console drivers with proper register/unregister interfaces we could perhaps reuse the very fancy console lock with all it's check and takeover semantics that John Ogness is developing to fix the console_lock mess. But for the initial cut of a drm panic printing support I don't think we need that, because the critical sections are extremely small and only happen once per display refresh. So generally just 60 tiny locked sections per second, which is nothing compared to a serial console running a 115kbaud doing really slow mmio writes for each byte. So for now the raw spintrylock in drm panic notifier callback should be good enough. Another benefit of making panic notifiers more like full blown consoles (that are used in panics only) would be that we get the two stage design, where first all the safe outputs are used. And then the dangerous takeover tricks are deployed (where for display drivers we also might try to intercept any in-flight display buffer flips, which if we race and misprogram fifos and watermarks can hang the memory controller on some hw). For context the actual implementation on the drm side is by Jocelyn and this patch is meant to be combined with the overall approach in v7 (v8 is a bit less flexible, which I think is the wrong direction): https://lore.kernel.org/dri-devel/20240104160301.185915-1-jfale...@redhat.com/ Note that the locking is very much not correct there, hence this separate rfc. v2: - fix authorship, this was all my typing - some typo oopsies - link to the drm panic work by Jocelyn for context v10: - Use spinlock_irqsave/restore (John Ogness) v11: - Use macro instead of inline functions for drm_panic_lock/unlock (John Ogness) Signed-off-by: Daniel Vetter Cc: Jocelyn Falempe Cc: Andrew Morton Cc: "Peter Zijlstra (Intel)" Cc: Lukas Wunner Cc: Petr Mladek Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Signed-off-by: Jocelyn Falempe --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++ drivers/gpu/drm/drm_drv.c | 1 + include/drm/drm_mode_config.h | 10 +++ include/drm/drm_panic.h | 100 4 files changed, 115 insertions(+) create mode 100644 include/drm/drm_panic.h diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 39ef0a6addeb..fb97b51b38f1 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -3016,6 +3017,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall) { int i, ret; + unsigned long flags; struct drm_connector *connector; struct drm_connector_state *old_conn_state, *new_conn_state; struct drm_crtc *crtc; @@ -3099,6 +3101,7 @@ int drm_atomic_helper_swap_state(struct drm_atomic_
[PATCH v11 0/9] drm/panic: Add a drm panic handler
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. It works with simpledrm, mgag200, ast, and imx. To test it, make sure you're using one of the supported driver, and trigger a panic: echo c > /proc/sysrq-trigger or you can enable CONFIG_DRM_PANIC_DEBUG and echo 1 > /sys/kernel/debug/dri/0/drm_panic_plane_0 There were not many comments on v10, so I think we're close to something that can be merged. Even if this is not yet useful, it will allows to work on more driver support, and better debug information. v2: * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann) * Add the panic reason to the panic message (Nerdopolis) * Add an exclamation mark (Nerdopolis) v3: * Rework the drawing functions, to write the pixels line by line and to use the drm conversion helper to support other formats. (Thomas Zimmermann) v4: * Fully support all simpledrm formats using drm conversion helpers * Rename dpanic_* to drm_panic_*, and have more coherent function name. (Thomas Zimmermann) * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann) * Remove the default y to DRM_PANIC config option (Thomas Zimmermann) * Add foreground/background color config option * Fix the bottom lines not painted if the framebuffer height is not a multiple of the font height. * Automatically register the driver to drm_panic, if the function get_scanout_buffer() exists. (Thomas Zimmermann) * Add mgag200 support. v5: * Change the drawing API, use drm_fb_blit_from_r1() to draw the font. (Thomas Zimmermann) * Also add drm_fb_fill() to fill area with background color. * Add draw_pixel_xy() API for drivers that can't provide a linear buffer. * Add a flush() callback for drivers that needs to synchronize the buffer. * Add a void *private field, so drivers can pass private data to draw_pixel_xy() and flush(). * Add ast support. * Add experimental imx/ipuv3 support, to test on an ARM hw. (Maxime Ripard) v6: * Fix sparse and __le32 warnings * Drop the IMX/IPUV3 experiment, it was just to show that it works also on ARM devices. v7: * Add a check to see if the 4cc format is supported by drm_panic. * Add a drm/plane helper to loop over all visible primary buffer, simplifying the get_scanout_buffer implementations * Add a generic implementation for drivers that uses drm_fb_dma. (Maxime Ripard) * Add back the IMX/IPUV3 support, and use the generic implementation. (Maxime Ripard) v8: * Directly register each plane to the panic notifier (Sima) * Replace get_scanout_buffer() with set_scanout_buffer() to simplify the locking. (Thomas Zimmermann) * Add a debugfs entry, to trigger the drm_panic without a real panic (Sima) * Fix the drm_panic Documentation, and include it in drm-kms.rst v9: * Revert to using get_scanout_buffer() (Sima) * Move get_scanout_buffer() and panic_flush() to the plane helper functions (Thomas Zimmermann) * Register all planes with get_scanout_buffer() to the panic notifier * Use drm_panic_lock() to protect against race (Sima) * Create a debugfs file for each plane in the device's debugfs directory. This allows to test for each plane of each GPU independently. v10: * Move blit and fill functions back in drm_panic (Thomas Zimmermann). * Simplify the text drawing functions. * Use kmsg_dumper instead of panic_notifier (Sima). * Use spinlock_irqsave/restore (John Ogness) v11: * Use macro instead of inline functions for drm_panic_lock/unlock (John Ogness) Best regards, Daniel Vetter (1): drm/panic: Add drm panic locking Jocelyn Falempe (8): drm/panic: Add a drm panic handler drm/panic: Add support for color format conversion drm/panic: Add debugfs entry to test without triggering panic. drm/fb_dma: Add generic get_scanout_buffer() for drm_panic drm/simpledrm: Add drm_panic support drm/mgag200: Add drm_panic support drm/imx: Add drm_panic support drm/ast: Add drm_panic support Documentation/gpu/drm-kms.rst| 12 + drivers/gpu/drm/Kconfig | 32 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/ast/ast_mode.c | 18 + drivers/gpu/drm/drm_atomic_helper.c | 4 + drivers/gpu/drm/drm_drv.c| 5 + drivers/gpu/drm/drm_fb_dma_helper.c | 47 ++ drivers/gpu/drm/drm_panic.c | 581 +++ drivers/gpu/drm/imx/ipuv3/ipuv3-plane.c | 11 +- drivers/gpu/drm/mgag200/mgag200_drv.h| 7 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 18 + drivers/gpu/drm/tiny/simpledrm.c | 16 + include/drm/drm_fb_dma_helper.h | 4 + include/drm/drm_mode_co
Re: [PATCH] vmwgfx: Create debugfs ttm_resource_manager entry only if needed
I just pushed it to drm-misc-fixes. Thanks for your reviews, -- Jocelyn On 14/03/2024 09:14, Jocelyn Falempe wrote: On 13/03/2024 18:57, Zack Rusin wrote: On Tue, Mar 12, 2024 at 5:36 AM Jocelyn Falempe wrote: [...] Thanks! That looks great. I can push it through drm-misc-fixes. Thanks, I think I only forget the "drm/" in the commit title, but yes you can push it with this small correction. Reviewed-by: Zack Rusin z Best regards,