Re: [PATCH 01/12] drm/panic: Allocate QR-code buffers statically
Thomas Zimmermann <[email protected]>
| Newsgroups | org.kernel.vger.rust-for-linux,dev.linux.lists.imx,dev.linux.lists.sashiko-reviews,dev.linux.lists.virtualization,org.freedesktop.lists.amd-gfx,org.freedesktop.lists.dri-devel,org.freedesktop.lists.intel-gfx,org.freedesktop.lists.intel-xe,org.freedesktop.lists.nouveau,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-doc,org.kernel.vger.linux-hyperv,org.kernel.vger.linux-renesas-soc |
|---|---|
| Message-ID | <[email protected]> |
Hi Am 19.08.26 um 11:38 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Tue, 18 Aug 2026 at 15:00, Thomas Zimmermann <[email protected]> wrote: >> Declare qrbuf1 and qrbuf2 as static arrays so that the module loader >> allocates them for us. Avoids the kmalloc later on. Also allows for >> using sizeof() to get the number of bytes in each array. Access the >> arrays once with memset, so that the physical pages are available on >> a panic. >> >> Signed-off-by: Thomas Zimmermann <[email protected]> > Thanks for your patch! > >> --- a/drivers/gpu/drm/drm_panic.c >> +++ b/drivers/gpu/drm/drm_panic.c >> @@ -628,24 +628,23 @@ MODULE_PARM_DESC(panic_qr_version, "maximum version (size) of the QR code"); >> #define WINDOW_BITS 12 >> #define MEM_LEVEL 4 >> >> -static char *qrbuf1; >> -static char *qrbuf2; >> +static u8 qrbuf1[QR_BUFFER1_SIZE]; >> +static u8 qrbuf2[QR_BUFFER2_SIZE]; > I am not a big fan of increasing kernel size like this. > You may run (faster) into boot loader limitations. Which limitation would this be? > >> static struct z_stream_s stream; >> >> static void __init drm_panic_qr_init(void) >> { >> - qrbuf1 = kmalloc(QR_BUFFER1_SIZE, GFP_KERNEL); >> - qrbuf2 = kmalloc(QR_BUFFER2_SIZE, GFP_KERNEL); >> + /* best-effort allocation; can be NULL */ >> stream.workspace = kmalloc(zlib_deflate_workspacesize(WINDOW_BITS, MEM_LEVEL), >> GFP_KERNEL); > <ironic>Why not use a static array for this, too?</ironic> I tried, by the size is calculated at runtime. > >> + >> + /* touch memory so that pages are there in the case of a panic */ >> + memset(qrbuf1, 0, sizeof(qrbuf1)); >> + memset(qrbuf2, 0, sizeof(qrbuf2)); > Please clarify "there"? > In cache? Not all systems have sufficient data cache, so it may be > evicted at any time. > In RAM? AFAIK kernel and module memory is not demand-paged. > In TLB? Like cache, it may be evicted at any time. I'd like to avoid looking for a page after a panic has already occurred. But I guess it might not make a difference, given all the DRM that is involved. The motivation here was that I did not like that panic handling depends on a number of dynamic kmallocs, with which we don't even detect alloc failures until a panic has occurred. > > So IMHO this is futile. Noted. Best regards Thomas > >> } > Gr{oetje,eeting}s, > > Geert > -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)