From 9dd09151123055e5a519bcebdc8a76f7944ef79c Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Thu, 16 Jul 2026 13:34:06 +0530 Subject: [PATCH] games/NXDoom: RGB565 support, loadable-module support, and crash fixes Add RGB565 framebuffer support to blit_screen() - it previously assumed a 32-bit ARGB framebuffer unconditionally, a real limitation for any board whose framebuffer is FB_FMT_RGB16_565. A single blit loop now branches on pinfo.bpp only at the pixel-write step (RGBTO16() for 16bpp, the existing ARGBTO32() path for 32bpp); anything else fails loudly via i_error() rather than reading/writing past the intended pixel bounds silently. Also centers the scaled viewport within the framebuffer instead of pinning it to the top-left corner, and adds CONFIG_GAMES_NXDOOM_PREFDIR to the IWAD search path (previously only used for the config/save file location; the Kconfig entry always has a default, so no #ifdef guard is needed around using it). Makes GAMES_NXDOOM tristate and lets MODULE follow it (MODULE = $(CONFIG_GAMES_NXDOOM)) instead of hardcoding MODULE = m, so it can still be built in as before or selected as a standalone loadable module installable via nxpkg/nxstore, same as every other tristate-capable app in apps/. Fix three real hardware crashes found while bringing this up as a loadable module: - A truncated/corrupted config line was silently overriding a variable's compiled-in default with an empty/unparsable value instead of being skipped - this let a corrupted "screenblocks" line through as screenblocks=0, and the renderer's view-size math divides by a value derived from screenblocks, reaching a divide-by-zero hardware exception. Also clamps screenblocks to its own valid range [3, 11] as an independent second layer of defense. - r_map_plane()'s bounds check was gated behind the debug-only CONFIG_GAMES_NXDOOM_RANGECHECK and, when tripped, called the fatal i_error() - both wrong: the check guards a real out-of-bounds array access (observed with values far past even viewheight), so it cannot be optional, and killing the whole process over one glitched plane span is worse than vanilla DOOM's own behavior of rendering the glitch. Now unconditional and clamps y into range instead of touching memory outside the buffers' bounds, so the span still renders (as one glitched row) rather than leaving a gap. - myargv was under-allocated: sized for argc pointers, but this codebase's own argument handling assumes argv is NULL-terminated at argv[argc] (standard C/exec convention). Root-caused via a real crash dump and symbol resolution against the built ELF; now allocates argc + 1 pointers and explicitly NULL-terminates. Also adds CONFIG_GAMES_NXDOOM_HEAP_BUFFERS: the renderer's visplanes/openings/drawsegs/vissprites scratch buffers remain static arrays by default (matching vanilla DOOM), with heap allocation available as an opt-in for targets where their combined size threatens the internal DRAM budget once linked into a full application image. CONFIG_GAMES_NXDOOM_STATDUMP_MAX_CAPTURES makes statdump's diagnostic capture-buffer size (unrelated to gameplay) a Kconfig option instead of a hardcoded value, default unchanged from vanilla (32). Assisted-by: Claude Signed-off-by: aviralgarg05 --- games/NXDoom/Kconfig | 22 +++++++- games/NXDoom/src/d_iwad.c | 10 ++++ games/NXDoom/src/doom/r_bsp.c | 4 ++ games/NXDoom/src/doom/r_bsp.h | 4 ++ games/NXDoom/src/doom/r_main.c | 18 +++++++ games/NXDoom/src/doom/r_plane.c | 87 +++++++++++++++++++++++++++++--- games/NXDoom/src/doom/r_plane.h | 1 + games/NXDoom/src/doom/r_things.c | 4 ++ games/NXDoom/src/doom/r_things.h | 4 ++ games/NXDoom/src/doom/statdump.c | 2 +- games/NXDoom/src/i_main.c | 15 +++++- games/NXDoom/src/i_video.c | 58 +++++++++++++++++---- games/NXDoom/src/m_config.c | 17 ++++++- 13 files changed, 226 insertions(+), 20 deletions(-) diff --git a/games/NXDoom/Kconfig b/games/NXDoom/Kconfig index cbf47a97a51..0c4d8d5b5a9 100644 --- a/games/NXDoom/Kconfig +++ b/games/NXDoom/Kconfig @@ -4,7 +4,7 @@ # config GAMES_NXDOOM - bool "NXDoom" + tristate "NXDoom" default n depends on ALLOW_GPL_COMPONENTS depends on VIDEO_FB @@ -178,6 +178,26 @@ config GAMES_NXDOOM_MAXDRAWSEGS memory, so you may reduce the number. However, too few will cause rendering issues (overflow is checked to avoid crashes). +config GAMES_NXDOOM_HEAP_BUFFERS + bool "Allocate renderer scratch buffers on the heap" + default n + ---help--- + The visplanes/openings/drawsegs/vissprites renderer scratch buffers + (sized by the options above) are static arrays by default, matching + vanilla DOOM. On a target where their combined size threatens the + internal DRAM budget once linked into a full application image, + enable this to allocate them from the heap instead (this target's + heap may be backed by external RAM/PSRAM). Static allocation is + preferred where DRAM budget is not a concern. + +config GAMES_NXDOOM_STATDUMP_MAX_CAPTURES + int "Maximum statdump capture buffer entries" + default 32 + ---help--- + Number of playtime-statistics capture slots statdump.c reserves. + This is diagnostic/debug capture storage, not required for normal + gameplay - reduce it on a DRAM-constrained target. + config GAMES_NXDOOM_RANGECHECK bool "Perform range checks" default y diff --git a/games/NXDoom/src/d_iwad.c b/games/NXDoom/src/d_iwad.c index a6f1cd9453d..5c072c90bf0 100644 --- a/games/NXDoom/src/d_iwad.c +++ b/games/NXDoom/src/d_iwad.c @@ -271,6 +271,16 @@ static void buld_iwad_dir_list(void) add_iwad_dir(m_dir_name(myargv[0])); + /* Add the board's configured DOOM data directory. Kconfig documents + * CONFIG_GAMES_NXDOOM_PREFDIR as "Directory where DOOM WAD files are + * stored", but until now it was only used for the config/save file + * location -- nothing actually searched it for IWADs, forcing every + * launch to rely on the current directory or DOOMWADDIR/DOOMWADPATH + * being set by hand first. + */ + + add_iwad_dir(CONFIG_GAMES_NXDOOM_PREFDIR); + /* Add DOOMWADDIR if it is in the environment */ env = getenv("DOOMWADDIR"); diff --git a/games/NXDoom/src/doom/r_bsp.c b/games/NXDoom/src/doom/r_bsp.c index 605b21beaa8..7dbd64b708e 100644 --- a/games/NXDoom/src/doom/r_bsp.c +++ b/games/NXDoom/src/doom/r_bsp.c @@ -78,7 +78,11 @@ line_t *linedef; sector_t *frontsector; sector_t *backsector; +#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS +drawseg_t *drawsegs; +#else drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS]; +#endif drawseg_t *ds_p; /* newend is one past the last valid seg */ diff --git a/games/NXDoom/src/doom/r_bsp.h b/games/NXDoom/src/doom/r_bsp.h index 66c6d611fff..c964b234c4a 100644 --- a/games/NXDoom/src/doom/r_bsp.h +++ b/games/NXDoom/src/doom/r_bsp.h @@ -52,7 +52,11 @@ extern boolean markceiling; extern boolean skymap; +#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS +extern drawseg_t *drawsegs; +#else extern drawseg_t drawsegs[CONFIG_GAMES_NXDOOM_MAXDRAWSEGS]; +#endif extern drawseg_t *ds_p; extern lighttable_t **hscalelight; diff --git a/games/NXDoom/src/doom/r_main.c b/games/NXDoom/src/doom/r_main.c index b50cdc33773..513185d4165 100644 --- a/games/NXDoom/src/doom/r_main.c +++ b/games/NXDoom/src/doom/r_main.c @@ -685,6 +685,24 @@ fixed_t r_scale_from_global_angle(angle_t visangle) void r_set_view_size(int blocks, int detail) { + /* screenblocks is only ever meant to hold 3..11 (set that way by the + * options menu and by the config default of 9). The renderer's view + * geometry math divides by values derived from it - notably + * pspriteiscale = FRACUNIT * SCREENWIDTH / viewwidth in + * r_execute_set_view_size() - so a 0 or otherwise out-of-range value + * turns into a divide-by-zero hardware exception (EXCCAUSE=6), which on + * this flat-memory build takes the whole board down rather than just + * this task. Clamp defensively so a bad/missing config value degrades + * to the default screen size instead of a system crash. + */ + + if (blocks < 3 || blocks > 11) + { + printf("r_set_view_size: screenblocks=%d out of range, using 10\n", + blocks); + blocks = 10; + } + setsizeneeded = true; setblocks = blocks; setdetail = detail; diff --git a/games/NXDoom/src/doom/r_plane.c b/games/NXDoom/src/doom/r_plane.c index 65a2c2fa78b..cc0d6ca8272 100644 --- a/games/NXDoom/src/doom/r_plane.c +++ b/games/NXDoom/src/doom/r_plane.c @@ -57,12 +57,17 @@ planefunction_t ceilingfunc; /* Here comes the obnoxious "visplane". */ +#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS +visplane_t *visplanes; +short *openings; +#else visplane_t visplanes[CONFIG_GAMES_NXDOOM_MAXVISPLANES]; +short openings[MAXOPENINGS]; +#endif visplane_t *lastvisplane; visplane_t *floorplane; visplane_t *ceilingplane; -short openings[MAXOPENINGS]; short *lastopening; /* Clip values are the solid pixel bounding the range. floorclip starts out @@ -114,12 +119,34 @@ static void r_map_plane(int y, int x1, int x2) fixed_t length; unsigned index; -#ifdef CONFIG_GAMES_NXDOOM_RANGECHECK - if (x2 < x1 || x1 < 0 || x2 >= viewwidth || y > viewheight) + /* y indexes cachedheight[]/cacheddistance[]/cachedxstep[]/cachedystep[] + * below, all sized SCREENHEIGHT - a y outside that range (observed on + * this port: y=255 against a 200-entry array, well past even + * viewheight) is an out-of-bounds array write, not just a "debug + * assertion". This used to be gated behind CONFIG_GAMES_NXDOOM_ + * RANGECHECK and fatal (i_error(), which tears down the whole process + * on what vanilla Doom would just render as one glitched span) - both + * wrong: the memory-safety check must not be optional, and killing the + * entire game over one bad plane span is worse than just not drawing + * it. Clamp y into range instead of touching memory outside the + * buffers' real bounds - this still renders the span (as one glitched + * row, the same "wrong but visible" failure mode vanilla DOOM has) so + * a bad plane doesn't leave a blank gap on screen either. + */ + + if (x2 < x1 || x1 < 0 || x2 >= viewwidth) { - i_error("R_MapPlane: %i, %i at %i", x1, x2, y); + return; + } + + if (y < 0) + { + y = 0; + } + else if (y >= SCREENHEIGHT) + { + y = SCREENHEIGHT - 1; } -#endif if (planeheight != cachedheight[y]) { @@ -195,7 +222,55 @@ static void r_make_spans(int x, int t1, int b1, int t2, int b2) void r_init_planes(void) { - /* Doh! */ +#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS + /* These renderer scratch buffers are sized for a comfortable margin + * above vanilla DOOM's original limits and, on a DRAM-constrained + * target, blow the internal DRAM budget as static arrays - opt-in + * heap allocation instead (comes out of the PSRAM-backed user heap + * on this target) via CONFIG_GAMES_NXDOOM_HEAP_BUFFERS. + */ + + visplanes = malloc(sizeof(visplane_t) * CONFIG_GAMES_NXDOOM_MAXVISPLANES); + openings = malloc(sizeof(short) * MAXOPENINGS); + drawsegs = malloc(sizeof(drawseg_t) * CONFIG_GAMES_NXDOOM_MAXDRAWSEGS); + vissprites = malloc(sizeof(vissprite_t) * + CONFIG_GAMES_NXDOOM_MAXVISSPRITES); + + if (visplanes == NULL || openings == NULL || drawsegs == NULL || + vissprites == NULL) + { + i_error("r_init_planes: failed to allocate renderer buffers"); + } + + /* i_quit() can be followed by another r_init_planes() call within the + * same boot (relaunching the game via nxpkg on this flat, single + * address-space build), so these heap buffers must be freed on exit + * or every relaunch leaks the previous allocation permanently. + */ + + i_at_exit(r_shutdown_planes, true); +#endif +} + +/* r_shutdown_planes + * Frees the renderer scratch buffers allocated by r_init_planes. Only + * registered as an exit handler when CONFIG_GAMES_NXDOOM_HEAP_BUFFERS is + * set - the static-array buffers have nothing to free. + */ + +void r_shutdown_planes(void) +{ +#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS + free(visplanes); + free(openings); + free(drawsegs); + free(vissprites); + + visplanes = NULL; + openings = NULL; + drawsegs = NULL; + vissprites = NULL; +#endif } /* r_clear_planes diff --git a/games/NXDoom/src/doom/r_plane.h b/games/NXDoom/src/doom/r_plane.h index 1ae3b79254b..f51ccfa37fa 100644 --- a/games/NXDoom/src/doom/r_plane.h +++ b/games/NXDoom/src/doom/r_plane.h @@ -58,6 +58,7 @@ extern fixed_t distscale[SCREENWIDTH]; ****************************************************************************/ void r_init_planes(void); +void r_shutdown_planes(void); void r_clear_planes(void); void r_draw_planes(void); diff --git a/games/NXDoom/src/doom/r_things.c b/games/NXDoom/src/doom/r_things.c index 019d83a7048..0084da909df 100644 --- a/games/NXDoom/src/doom/r_things.c +++ b/games/NXDoom/src/doom/r_things.c @@ -93,7 +93,11 @@ spriteframe_t sprtemp[29]; int maxframe; const char *spritename; +#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS +vissprite_t *vissprites; +#else vissprite_t vissprites[CONFIG_GAMES_NXDOOM_MAXVISSPRITES]; +#endif vissprite_t *vissprite_p; int newvissprite; diff --git a/games/NXDoom/src/doom/r_things.h b/games/NXDoom/src/doom/r_things.h index cdd29948368..eb730c28d7e 100644 --- a/games/NXDoom/src/doom/r_things.h +++ b/games/NXDoom/src/doom/r_things.h @@ -28,7 +28,11 @@ * Public Data ****************************************************************************/ +#ifdef CONFIG_GAMES_NXDOOM_HEAP_BUFFERS +extern vissprite_t *vissprites; +#else extern vissprite_t vissprites[CONFIG_GAMES_NXDOOM_MAXVISSPRITES]; +#endif extern vissprite_t *vissprite_p; extern vissprite_t vsprsortedhead; diff --git a/games/NXDoom/src/doom/statdump.c b/games/NXDoom/src/doom/statdump.c index 1db5657ab6c..977a4b9b160 100644 --- a/games/NXDoom/src/doom/statdump.c +++ b/games/NXDoom/src/doom/statdump.c @@ -39,7 +39,7 @@ * Pre-processor Definitions ****************************************************************************/ -#define MAX_CAPTURES 32 +#define MAX_CAPTURES CONFIG_GAMES_NXDOOM_STATDUMP_MAX_CAPTURES /**************************************************************************** * Private Data diff --git a/games/NXDoom/src/i_main.c b/games/NXDoom/src/i_main.c index bd9dab60910..e60fdb62c2e 100644 --- a/games/NXDoom/src/i_main.c +++ b/games/NXDoom/src/i_main.c @@ -57,10 +57,19 @@ void d_doom_main(void); int main(int argc, char **argv) { - /* save arguments */ + /* save arguments + * + * +1 and an explicit NULL terminator: argv is conventionally + * NULL-terminated at argv[argc] (this is what the OS/exec path + * guarantees for the `argv` parameter above), and some of this + * codebase's own argument handling was written assuming that holds + * for myargv too - an under-sized allocation here leaves myargv[argc] + * pointing at whatever the allocator happens to return next, which + * only reads as "probably zero" by chance depending on heap layout. + */ myargc = argc; - myargv = malloc(argc * sizeof(char *)); + myargv = malloc((argc + 1) * sizeof(char *)); assert(myargv != NULL); for (int i = 0; i < argc; i++) @@ -68,6 +77,8 @@ int main(int argc, char **argv) myargv[i] = m_string_duplicate(argv[i]); } + myargv[argc] = NULL; + /* Print the program version and exit. */ if (m_parm_exists("-version") || m_parm_exists("--version")) diff --git a/games/NXDoom/src/i_video.c b/games/NXDoom/src/i_video.c index 0d3b41af30e..544c0fe6580 100644 --- a/games/NXDoom/src/i_video.c +++ b/games/NXDoom/src/i_video.c @@ -92,6 +92,13 @@ struct graphics_state_s uint8_t scale; + /* Pixel offset to center the scaled game viewport within the frame + * buffer when the buffer is larger than SCREENWIDTH/HEIGHT * scale. + */ + + int xoffset; + int yoffset; + bool inited; /* Track initialization */ }; @@ -262,13 +269,25 @@ static void blit_screen(void) uint8_t p_idx; void *fbptr; - /* TODO: It would be best to do this more efficiently/with less memory. - * It also would be good if we could handle the palette translation here - * such that DOOM can be played on frame buffers with differing bit depths - * and pixel formats. - */ + /* TODO: It would be best to do this more efficiently/with less memory. */ + + if (g_graphics_state.pinfo.bpp != 16 && g_graphics_state.pinfo.bpp != 32) + { + /* The xoffset/stride math below assumes one of those two pixel + * sizes, so continuing would read/write past the intended pixel + * bounds on the very first frame. Fail loudly instead of + * silently corrupting framebuffer memory. + */ + + i_error("Unsupported framebuffer depth: %u bpp", + g_graphics_state.pinfo.bpp); + } + + fbptr = g_graphics_state.fbmem + + g_graphics_state.yoffset * g_graphics_state.pinfo.stride + + g_graphics_state.xoffset * + (g_graphics_state.pinfo.bpp == 16 ? 2 : 4); - fbptr = g_graphics_state.fbmem; for (unsigned y = 0; y < SCREENHEIGHT * g_graphics_state.scale; y++) { for (unsigned x = 0; x < SCREENWIDTH * g_graphics_state.scale; x++) @@ -277,9 +296,18 @@ static void blit_screen(void) .scrnbuf[(y / g_graphics_state.scale) * SCREENWIDTH + (x / g_graphics_state.scale)]; - ((uint32_t *)(fbptr))[x] = - ARGBTO32(g_palette[p_idx].a, g_palette[p_idx].r, - g_palette[p_idx].g, g_palette[p_idx].b); + if (g_graphics_state.pinfo.bpp == 16) + { + ((uint16_t *)(fbptr))[x] = + RGBTO16(g_palette[p_idx].r, g_palette[p_idx].g, + g_palette[p_idx].b); + } + else + { + ((uint32_t *)(fbptr))[x] = + ARGBTO32(g_palette[p_idx].a, g_palette[p_idx].r, + g_palette[p_idx].g, g_palette[p_idx].b); + } } fbptr += g_graphics_state.pinfo.stride; @@ -724,6 +752,18 @@ void i_init_graphics(void) yscale = g_graphics_state.vinfo.yres / SCREENHEIGHT; g_graphics_state.scale = xscale > yscale ? yscale : xscale; + /* Center the scaled viewport within the frame buffer rather than + * pinning it to the top-left corner, since the buffer is typically + * larger than SCREENWIDTH/HEIGHT * scale. + */ + + g_graphics_state.xoffset = + (g_graphics_state.vinfo.xres - + SCREENWIDTH * g_graphics_state.scale) / 2; + g_graphics_state.yoffset = + (g_graphics_state.vinfo.yres - + SCREENHEIGHT * g_graphics_state.scale) / 2; + /* Get frame buffer plane info */ if (ioctl(g_graphics_state.fd, FBIOGET_PLANEINFO, diff --git a/games/NXDoom/src/m_config.c b/games/NXDoom/src/m_config.c index d4441f5eaae..e7874eaac42 100644 --- a/games/NXDoom/src/m_config.c +++ b/games/NXDoom/src/m_config.c @@ -2098,7 +2098,9 @@ static void load_default_collection(default_collection_t *collection) while (!feof(f)) { - if (fscanf(f, "%79s %99[^\n]\n", defname, strparm) != 2) + strparm[0] = '\0'; + + if (fscanf(f, "%79s %99[^\n]\n", defname, strparm) < 1) { /* This line doesn't match */ @@ -2136,6 +2138,19 @@ static void load_default_collection(default_collection_t *collection) memmove(strparm, strparm + 1, sizeof(strparm) - 1); } + /* A line with a name but no (or an unparsable) value - e.g. a + * config file left truncated by an unclean shutdown mid-write - + * must not silently override this variable's compiled-in default + * with a bogus zero/empty value. This is what let a corrupted + * "screenblocks" line (empty value) through as screenblocks=0, + * which fed a divide-by-zero straight into the renderer. + */ + + if (strparm[0] == '\0') + { + continue; + } + set_variable(def, strparm); }