| Message ID | 3f1a5f56213f3e4584773eb2813e212b2dff6d14.1718305355.git.geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | drm/panic: Fixes and graphical logo | expand |
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 <jfalempe@redhat.com> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > 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 <linux/font.h> > +#include <linux/init.h> > #include <linux/iosys-map.h> > #include <linux/kdebug.h> > #include <linux/kmsg_dump.h> > +#include <linux/linux_logo.h> > #include <linux/list.h> > +#include <linux/math.h> > #include <linux/module.h> > +#include <linux/overflow.h> > #include <linux/printk.h> > #include <linux/types.h> > > @@ -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_xrgb8888(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, &r_screen, bg_color); > > - if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) && > - drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) { > - draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_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, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), > + fg_color); > + else > + draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo, > + fg_color); > } > draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_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/Kconfig > @@ -8,6 +8,8 @@ menuconfig LOGO > depends on FB_CORE || SGI_NEWPORT_CONSOLE > help > Enable and select frame buffer bootup logos. > + Monochrome logos will also be used by the DRM panic handler, if > + enabled. > > if LOGO >
Hi Jocelyn, On Fri, Jun 14, 2024 at 11:55 AM Jocelyn Falempe <jfalempe@redhat.com> 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. > > Would it be possible to have the logo not in the __init section if > DRM_PANIC is set ? That would be rather complicated. The C source files for the logos (there can be multiple) are generated by drivers/video/logo/pnmtologo.c. > The patch looks good to me anyway. > > Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com> Thanks! Gr{oetje,eeting}s, Geert
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 <geert+renesas@glider.be> > --- > 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 <linux/font.h> > +#include <linux/init.h> > #include <linux/iosys-map.h> > #include <linux/kdebug.h> > #include <linux/kmsg_dump.h> > +#include <linux/linux_logo.h> > #include <linux/list.h> > +#include <linux/math.h> > #include <linux/module.h> > +#include <linux/overflow.h> > #include <linux/printk.h> > #include <linux/types.h> > > @@ -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.
Hi Jocelyn, On Mon, Jun 17, 2024 at 11:59 AM Jocelyn Falempe <jfalempe@redhat.com> 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 <geert+renesas@glider.be> > > --- 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. > 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? Gr{oetje,eeting}s, Geert
On 17/06/2024 14:49, Geert Uytterhoeven wrote: > Hi Jocelyn, > > On Mon, Jun 17, 2024 at 11:59 AM Jocelyn Falempe <jfalempe@redhat.com> 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 <geert+renesas@glider.be> > >>> --- 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 >
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 <linux/font.h> +#include <linux/init.h> #include <linux/iosys-map.h> #include <linux/kdebug.h> #include <linux/kmsg_dump.h> +#include <linux/linux_logo.h> #include <linux/list.h> +#include <linux/math.h> #include <linux/module.h> +#include <linux/overflow.h> #include <linux/printk.h> #include <linux/types.h> @@ -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_xrgb8888(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, &r_screen, bg_color); - if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) && - drm_rect_width(&r_logo) <= sb->width && drm_rect_height(&r_logo) <= sb->height) { - draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_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, &r_logo, logo_mono->data, DIV_ROUND_UP(logo_width, 8), + fg_color); + else + draw_txt_rectangle(sb, font, logo_ascii, logo_ascii_lines, false, &r_logo, + fg_color); } draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_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/Kconfig @@ -8,6 +8,8 @@ menuconfig LOGO depends on FB_CORE || SGI_NEWPORT_CONSOLE help Enable and select frame buffer bootup logos. + Monochrome logos will also be used by the DRM panic handler, if + enabled. if LOGO
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 <geert+renesas@glider.be> --- 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(-)