Conversation
WalkthroughThis PR adds a tiled rasterizer library and integrates selectable rasterizer backends (sync/async, tiled/linear) into kernel graphics, updates framebuffer alignment and rectangle APIs, adds VBlank event plumbing and demo-mode GPIO/syscall, introduces demo and slideshow apps, and adds profiling/tools and build wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Graphics as Graphics
participant Queue as Render Queue
participant Worker as Render Worker
participant Raster as Rasterizer
participant FB as Framebuffer
App->>Graphics: render_async(call, inputs)
Graphics->>Graphics: check selected_rasterizer.is_async()
alt async backend
Graphics->>Queue: enqueue(task)
Graphics-->>App: return
Worker->>Queue: dequeue(task)
Worker->>Graphics: reconstruct Render from Arc
Worker->>Raster: render_one_task(inputs, target)
Raster->>FB: write pixels / overlays
Raster-->>Worker: complete
Worker->>Graphics: finalize async call
else sync backend
Graphics->>Raster: render_one_task(inputs, target)
Raster->>FB: write pixels / overlays
Raster-->>Graphics: complete
Graphics-->>App: return (synchronous)
end
sequenceDiagram
participant Video as Video Tick
participant VideoDev as VideoDevice
participant Driver as HSTX_DVI_2
participant Awaiter as VBlank Awaiter
Video->>VideoDev: tick()
VideoDev->>Driver: get_one_vblank_event()
Driver->>Driver: atomically swap vsync_has_happened
alt vsync true
Driver-->>VideoDev: true
VideoDev->>VideoDev: force_flush(output*)
VideoDev->>Awaiter: notify vblank awaiters
else false
Driver-->>VideoDev: false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…xes long-standing bug in agp-swrast
…hen the command would be clipped anyways. This improves rasterizer performance in the backend in general. Also fixes some of the remaining bugs in the tiled rasterizer, which now only misses text rendering.
…already, bitmaps are skipped yet
… the current tile.
…external stack which is useful for the renderer to run on the internal SRAM
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/abi/src/ports/zig.abi.zpatch (1)
281-293:⚠️ Potential issue | 🔴 CriticalFix
enclosingRegion()'s top bound.
_topmust use@min, not@max. As written, the returned rectangle can exclude whichever input starts higher on the screen, so the “enclosing” region is not actually a union.🧭 Suggested fix
pub fn enclosingRegion(a: Rectangle, b: Rectangle) Rectangle { const _left = `@min`(a.left(), b.left()); const _right = `@max`(a.right(), b.right()) +| 1; - const _top = `@max`(a.top(), b.top()); + const _top = `@min`(a.top(), b.top()); const _bottom = `@max`(a.bottom(), b.bottom()) +| 1; return .{ .x = _left, .y = _top, - .width = `@intCast`(_right -| _left), - .height = `@intCast`(_bottom -| _top), + .width = clamp_u16(`@as`(i32, _right) - `@as`(i32, _left)), + .height = clamp_u16(`@as`(i32, _bottom) - `@as`(i32, _top)), }; }src/userland/apps/desktop/classic/src/DamageTracking.zig (1)
39-58:⚠️ Potential issue | 🟠 MajorDrop empty intersections after clipping.
overlappedRegion()now returns a zero-sized rectangle whenregionfalls outsidetracked_area. Withoutif (target.empty()) return, this path can enqueue empty invalidations and also send an empty rectangle intocontainsRectangle().✂️ Suggested fix
const target = dt.tracked_area.overlappedRegion(region); + if (target.empty()) + return; // check if we already have this region invalidated for (dt.invalidation_areas.items) |rect| {
🟡 Minor comments (6)
src/userland/apps/dungeon/src/dungeon.zig-497-497 (1)
497-497:⚠️ Potential issue | 🟡 MinorDrop the debug tint from ceiling texture sampling.
Line 497 applies
.shift(16 * texture_id)only on the ceiling perspective-texture path, so the sampled bitmap will not render with its original colors. This looks like a leftover debug effect.src/userland/apps/dungeon/src/dungeon.zig-313-319 (1)
313-319:⚠️ Potential issue | 🟡 MinorUse the full texture dimensions for repeating floor/ceiling sampling.
Line 317-318 multiplies
fract()bytexture.width - 1/texture.height - 1, butfract()is always< 1.0. That makes the last texel row and column unreachable for perspective backgrounds, so the tiled pattern is compressed by one texel.🎯 Proposed fix
return .{ .texture = texture, - .u_scale = `@as`(f32, `@floatFromInt`(texture.width - 1)), - .v_scale = `@as`(f32, `@floatFromInt`(texture.height - 1)), + .u_scale = `@as`(f32, `@floatFromInt`(texture.width)), + .v_scale = `@as`(f32, `@floatFromInt`(texture.height)), };assets/slides/CREDITS.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorPolish intro wording for readability.
Line 3 reads awkwardly (“were from”). Suggest “are from” or “come from”.
Suggested wording tweak
-These slides were from different demoscene contributions over the last years: +These slides are from different demoscene contributions over the last years:justfile-283-301 (1)
283-301:⚠️ Potential issue | 🟡 MinorCreate output directories before writing generated files.
Line 284+ and Line 295+ assume
rootfs/dev/data/slideshowand.tmpalready exist. On a clean tree, both recipes can fail immediately.💡 Proposed fix
slideshow: + mkdir -p rootfs/dev/data/slideshow ./zig-out/bin/mkicon -g 600x350 -o rootfs/dev/data/slideshow/debris.abm assets/slides/debris.png ./zig-out/bin/mkicon -g 600x350 -o rootfs/dev/data/slideshow/heritages.abm assets/slides/heritages.png ./zig-out/bin/mkicon -g 600x350 -o rootfs/dev/data/slideshow/hoody.abm assets/slides/hoody.png @@ trace: + mkdir -p .tmp python scripts/profiler.py \ > .tmp/trace.logsrc/userland/libs/agp-tiled-rast/src/tiled-raster.zig-226-227 (1)
226-227:⚠️ Potential issue | 🟡 MinorAllow the documented 2048px limit.
max_image_sizeis 2048, but Line 226 rejects width/height equal to 2048 because of>=. That makes the effective limit 2047.Proposed fix
- if (target.width >= max_image_size or target.height >= max_image_size) + if (target.width > max_image_size or target.height > max_image_size) return error.ImageTooLarge;src/userland/apps/demos/revision2026/revision2026.zig-26-35 (1)
26-35:⚠️ Potential issue | 🟡 MinorUse strict bounds before indexing the bitmap.
ux == bmp.widthoruy == bmp.heightis already out of bounds forbmp.pixels[index]. With padded strides that reads padding; with tight strides it can fault. Use<here.Proposed fix
- std.debug.assert(ux <= bmp.width); - std.debug.assert(uy <= bmp.height); + std.debug.assert(ux < bmp.width); + std.debug.assert(uy < bmp.height);
🧹 Nitpick comments (2)
src/kernel/port/machine/arm/ashet-hc/psram.zig (1)
29-29: Remove redundant legacy-value inline comment.
// 109000000;is now noise and may drift from the actual constant over time. Prefer keeping only the canonical value.src/abi/src/ashet.abi (1)
13-17: Exposeget_demo_modeas an enum, not a rawu8.This is now part of the public ABI, so leaving it as an untyped byte makes the contract easy to drift across kernel and userland. A dedicated enum keeps the values self-documenting and prevents magic-number callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a70355a-556a-413f-9c89-28528d9f23da
⛔ Files ignored due to path filters (12)
assets/images/wallpaper-info.pngis excluded by!**/*.pngassets/slides/debris.pngis excluded by!**/*.pngassets/slides/heritages.pngis excluded by!**/*.pngassets/slides/hoody.pngis excluded by!**/*.pngassets/slides/modern-furnishing.pngis excluded by!**/*.pngassets/slides/opium.pngis excluded by!**/*.pngassets/slides/r-cade.pngis excluded by!**/*.pngassets/slides/velocity.pngis excluded by!**/*.pngsrc/userland/apps/demos/revision2026/data/Brick01.pngis excluded by!**/*.pngsrc/userland/apps/demos/revision2026/data/Doors10.pngis excluded by!**/*.pngsrc/userland/apps/demos/revision2026/data/Stone01.pngis excluded by!**/*.pngsrc/userland/apps/demos/revision2026/data/Tiles05.pngis excluded by!**/*.png
📒 Files selected for processing (50)
.gitignoreassets/images/wallpaper.xcfassets/slides/CREDITS.mdbuild.zigjustfilescripts/profiler.pysrc/abi/src/ashet.abisrc/abi/src/platforms.zigsrc/abi/src/ports/zig.abi.zpatchsrc/kernel/build.zigsrc/kernel/build.zig.zonsrc/kernel/components/graphics.zigsrc/kernel/components/gui.zigsrc/kernel/components/multi_tasking.zigsrc/kernel/components/scheduler.zigsrc/kernel/components/syscalls.zigsrc/kernel/components/video.zigsrc/kernel/drivers/input/PropIO_PS2_Device.zigsrc/kernel/drivers/video/HSTX_DVI_2.zigsrc/kernel/main.zigsrc/kernel/port/machine/arm/ashet-hc/ashet-hc.zigsrc/kernel/port/machine/arm/ashet-hc/hw_alloc.zigsrc/kernel/port/machine/arm/ashet-hc/psram.zigsrc/os/build.zigsrc/os/build.zig.zonsrc/tools/abi-mapper/src/syntax.zigsrc/userland/apps/demos/revision2026/build.zigsrc/userland/apps/demos/revision2026/build.zig.zonsrc/userland/apps/demos/revision2026/revision2026.zigsrc/userland/apps/desktop/classic/src/DamageTracking.zigsrc/userland/apps/desktop/classic/src/WindowManager.zigsrc/userland/apps/desktop/classic/src/classic-desktop.zigsrc/userland/apps/dungeon/src/dungeon.zigsrc/userland/apps/hello-gui/build.zigsrc/userland/apps/init/init.zigsrc/userland/apps/slideshow/build.zigsrc/userland/apps/slideshow/build.zig.zonsrc/userland/apps/slideshow/src/slideshow.zigsrc/userland/libs/agp-swrast/src/agp-swrast.zigsrc/userland/libs/agp-tiled-rast/build.zigsrc/userland/libs/agp-tiled-rast/build.zig.zonsrc/userland/libs/agp-tiled-rast/src/tiled-raster.zigsrc/userland/libs/agp-tiled-rast/test/exerciser.zigsrc/userland/libs/agp-tiled-rast/test/gif.zigsrc/userland/libs/agp-tiled-rast/test/mono-6.fontsrc/userland/libs/agp-tiled-rast/test/mono-8.fontsrc/userland/libs/agp-tiled-rast/test/sans.fontsrc/userland/libs/agp/src/agp.zigsrc/userland/libs/libAshetOS/build.zigsrc/userland/libs/libAshetOS/module/src/libashet/graphics.zig
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/userland/libs/agp-swrast/src/agp-swrast.zig (1)
425-430:⚠️ Potential issue | 🔴 CriticalClip negative source coordinates before the
u16cast.
source_pos.x/source_pos.ycan still be negative here. In that case@intCast(...)traps before any source-space clipping happens, so a partial blit with a negative source origin can take down the linear rasterizer path.src/kernel/components/graphics.zig (2)
74-82:⚠️ Potential issue | 🔴 CriticalDon't reuse the global decoder arena across concurrent renders.
This still makes
render_one_task()run on both the caller thread and the worker thread, butrender_sync()keeps resetting the single globalrender_temp_buffer. Two renders in flight can stomp each other's decoder arena and corrupt command decoding.Also applies to: 588-640
621-634:⚠️ Potential issue | 🔴 CriticalReturn a render error here instead of panicking the kernel.
This is still processing user-controlled AGP input. Converting rasterizer decode/resolve failures into
@panic()lets a bad draw request crash the whole kernel instead of surfacingerror.BadCode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28d0a1c5-8fc0-4abc-869b-e615d684a1a6
📒 Files selected for processing (6)
scripts/analyze-render-stats.pysrc/abi/src/ports/zig.abi.zpatchsrc/kernel/components/graphics.zigsrc/kernel/main.zigsrc/userland/libs/agp-swrast/src/agp-swrast.zigsrc/userland/libs/agp-tiled-rast/src/tiled-raster.zig
🚧 Files skipped from review as they are similar to previous changes (3)
- src/kernel/main.zig
- src/abi/src/ports/zig.abi.zpatch
- src/userland/libs/agp-tiled-rast/src/tiled-raster.zig
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/abi/src/ashet.abi (1)
3188-3193:⚠️ Potential issue | 🟠 Major
Rectangle.everythingstill excludes negative-space rectangles.Line 3189 and Line 3190 keep origin at
(0,0), so this sentinel is not “everything” for signed coordinates.Suggested fix
const everything: Rectangle = .{ - .x = 0, //? TODO: Fix parsing and numeric handling so we can use -32768 - .y = 0, //? TODO: Fix parsing and numeric handling so we can use -32768 + .x = -32767 - 1, + .y = -32767 - 1, .width = 65535, .height = 65535 };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1ba3b91-945a-4ec2-a034-faaa2165c304
📒 Files selected for processing (2)
src/abi/src/ashet.abisrc/kernel/components/graphics.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/kernel/components/graphics.zig
1 participant
Summary by CodeRabbit
New Features
Improvements
Tools
Documentation