fix(rtg): restore dirty-page tracking for RTG display updates - #44
Merged
Merged
Conversation
Switching between RTG and native modes left the display stale until the GUI was opened (fixes #27). The RTG dirty-page refresh optimization added in 5.9.0 (ported from Amiberry) only ever flushed full refreshes on non-Windows builds: - dirty_page_map was never allocated, so picasso_getwritewatch() always returned -1 and partial flushes copied zero pages; mark_dirty() from the P96 trap handlers was a no-op. - plain CPU stores to VRAM never marked pages dirty; only P96 API ops (FillRect, BlitRect, ...) did, and even those were lost to the unallocated map. Workbench renders mostly via direct CPU writes, so after a mode switch the RTG screen stayed grey/frozen until something forced a full refresh (e.g. opening and dismissing the GUI). Port the follow-up fixes from the main Amiberry repository: - allocate dirty_page_map in picasso_allocatewritewatch and track min/max dirty page bounds so picasso_getwritewatch scans only the touched range (amiberry 875c4198) - give the gfxmem banks custom put handlers (lput/wput/bput) that call mark_dirty() after every store, and flag the banks with S_WRITE so JIT-routed writes also go through the handlers (amiberry 63a74239) - clamp the unlockscr dirty rectangle to the current surface so a stale width_allocated after a mode switch cannot feed SDL_UpdateTexture an out-of-bounds region (amiberry 756ebf1e)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ea3470cef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- picasso_getwritewatch(): with VGA screen splitting active the getter was called once per source region but drained the whole dirty map on the first call, so pages belonging to the wrapped lower region were dropped by the first pass's range filter and missing from the second. Drain once per frame and reuse the page list for both regions. - picasso_getwritewatch(): return the region base as board base plus the screen offset, matching the Windows GetWriteWatch semantics. Returning the bare board base let the caller's range filter accept pages below the visible screen (offscreen bitmaps, split region). - mark_dirty()/picasso_getwritewatch(): synchronize the dirty bounds with the RTG render thread. With gfxcard_multithread enabled, mark_dirty() runs from the CPU write handlers while render_thread() drains the map, racing on the bounds (and allowing a drain to reset the bounds between a writer's compare and its bit-set, losing the page until another write covers it). The map and bounds are now std::atomic, and bounds are published only after the bits are set so a concurrent drain always re-covers marked pages on its next pass.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef58cf9a97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
picasso_getwritewatch() returns page-aligned addresses, but the region base was returned as the unaligned base + screen offset. With a panned (SetPanning) screen offset that is not page-aligned, the page holding the top-left of the visible screen compares below the returned base, so the range check in picasso_flushpixels() rejects it after its dirty bit was already cleared, leaving updates near the top-left stale. Round the base down to the page boundary (offset & ~gwwpagemask), matching how picasso_flushpixels() itself initializes src_start[0].
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 942eeed35f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review follow-up: making the two bound variables atomic does not make the drain's two-store reset atomic with respect to mark_dirty(). A writer can publish a page between the drain's load and its reset, so the reset then erases the freshly published bound: the page stays set but outside any published range and is not rendered until a later write happens to republish a range. Pack the bounds into a single word, (min_page << 32) | (max_page + 1): mark_dirty() widens the packed value with a compare_exchange loop after setting the page bits, and picasso_getwritewatch() claims the whole range with one compare_exchange to the empty state, retrying against the wider range if a concurrent widening made the exchange fail. A claimed range therefore always includes every page published before the claim, and pages marked after the claim are published anew for the next frame.
Review follow-up: the dirty page list drained once per frame spans both split regions (and can contain pages outside the visible screen, e.g. offscreen bitmaps), but the full-copy heuristic compared that total against each region's own size. A busy first region could push the second region over the 80% threshold and trigger a needless full copy. Count only the pages that fall inside the current region, matching the region-scoped Windows GetWriteWatch, and skip the region entirely when none apply.
Review follow-up: the per-region page filter must only apply to the drained dirty list. In the forced full-refresh path (full_refresh < 0 after a mode change, flash or RTG clear) the page count is synthetic and gwwbuf is filled with generated region pages, so filtering there could only ever discard required work. Move the filter into the drain branch and count the synthetic list directly.
Review follow-up: shortening gwwcnt to the region-filtered count made the copy loop examine only a prefix of the page-ordered list, but foreign pages (lower split region, offscreen bitmaps) can occupy that prefix, so matching pages later in the list were skipped after their dirty bits had already been cleared by the drain. Keep gwwcnt untruncated for iteration - the copy loop range-checks each entry itself - and use the filtered count only for the skip decision and the full-copy heuristic. Compacting the list in place instead would destroy the entries the second split region still has to examine.
Review follow-up: draining a page with a separate load and store could lose a concurrent write - the writer sets the bit and republishes the bounds between the drain's load and its unconditional clear, so the next drain claims the republished bounds but finds no set bit and never uploads the new pixels. Use exchange(false) so the test and the clear are one read-modify-write: a mark made before the exchange is consumed by this drain (the copy reads VRAM after it), and a mark made after it stays set for the next drain.
midwan
added a commit
to BlitterStudio/amiberry
that referenced
this pull request
Sep 16, 2026
…2335) * fix(rtg): close race and split-screen holes in dirty-page tracking Found by review of the equivalent fix in amiberry-lite (PR BlitterStudio/amiberry-lite#44); the same issues exist here: - picasso_getwritewatch(): with VGA screen splitting active the getter was called once per source region but drained the whole dirty map on the first call, so pages belonging to the wrapped lower region were dropped by the first pass's range filter and missing from the second. Drain once per frame and reuse the page list for both regions. - picasso_getwritewatch(): return the region base as board base plus the screen offset, matching the Windows GetWriteWatch semantics. Returning the bare board base let the caller's range filter accept pages below the visible screen (offscreen bitmaps, split region). - mark_dirty()/picasso_getwritewatch(): synchronize the dirty bounds with the RTG render thread. With gfxcard_multithread enabled, mark_dirty() runs from the CPU write handlers while render_thread() drains the map, racing on the bounds (and allowing a drain to reset the bounds between a writer's compare and its bit-set, losing the page until another write covers it). The map and bounds are now std::atomic, and bounds are published only after the bits are set so a concurrent drain always re-covers marked pages on its next pass. Also fix the mman_GetWriteWatch declaration to match the real API (PULONG_PTR lpdwGranularity). * fix(rtg): page-align the dirty-page region base picasso_getwritewatch() returns page-aligned addresses, but the region base was returned as the unaligned base + screen offset. With a panned (SetPanning) screen offset that is not page-aligned, the page holding the top-left of the visible screen compares below the returned base, so the range check in picasso_flushpixels() rejects it after its dirty bit was already cleared, leaving updates near the top-left stale. Round the base down to the page boundary (offset & ~gwwpagemask), matching how picasso_flushpixels() itself initializes src_start[0]. * fix(rtg): claim the dirty range with a single compare_exchange Review follow-up: making the two bound variables atomic does not make the drain's two-store reset atomic with respect to mark_dirty(). A writer can publish a page between the drain's load and its reset, so the reset then erases the freshly published bound: the page stays set but outside any published range and is not rendered until a later write happens to republish a range. Pack the bounds into a single word, (min_page << 32) | (max_page + 1): mark_dirty() widens the packed value with a compare_exchange loop after setting the page bits, and picasso_getwritewatch() claims the whole range with one compare_exchange to the empty state, retrying against the wider range if a concurrent widening made the exchange fail. A claimed range therefore always includes every page published before the claim, and pages marked after the claim are published anew for the next frame. * fix(rtg): filter the reused dirty-page count per split region Review follow-up: the dirty page list drained once per frame spans both split regions (and can contain pages outside the visible screen, e.g. offscreen bitmaps), but the full-copy heuristic compared that total against each region's own size. A busy first region could push the second region over the 80% threshold and trigger a needless full copy. Count only the pages that fall inside the current region, matching the region-scoped Windows GetWriteWatch, and skip the region entirely when none apply. * fix(rtg): keep forced full refreshes out of the region filter Review follow-up: the per-region page filter must only apply to the drained dirty list. In the forced full-refresh path (full_refresh < 0 after a mode change, flash or RTG clear) the page count is synthetic, so filtering there could only ever discard required work. Move the filter into the drain branch and count the synthetic list directly. Also restore the missing gwwbuf fill in the full-refresh branch (WinUAE parity, lost in a refactor): the copy loop consumes gwwbuf entries, and without the fill it iterated stale pointers from the last drain, which the region range checks would mostly filter out after a mode change - defeating the forced full copy. * fix(rtg): iterate the full dirty list when copying regions Review follow-up: shortening gwwcnt to the region-filtered count made the copy loop examine only a prefix of the page-ordered list, but foreign pages (lower split region, offscreen bitmaps) can occupy that prefix, so matching pages later in the list were skipped after their dirty bits had already been cleared by the drain. Keep gwwcnt untruncated for iteration - the copy loop range-checks each entry itself - and use the filtered count only for the skip decision and the full-copy heuristic. Compacting the list in place instead would destroy the entries the second split region still has to examine. * fix(rtg): clear dirty bits with an atomic exchange Review follow-up: draining a page with a separate load and store could lose a concurrent write - the writer sets the bit and republishes the bounds between the drain's load and its unconditional clear, so the next drain claims the republished bounds but finds no set bit and never uploads the new pixels. Use exchange(false) so the test and the clear are one read-modify-write: a mark made before the exchange is consumed by this drain (the copy reads VRAM after it), and a mark made after it stays set for the next drain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #27
Root cause
amiberry-lite 5.9.0 ported the RTG dirty-page refresh optimization from the main Amiberry repo, but two of its follow-up fixes were missed, which breaks RTG display updates entirely on non-Windows builds:
dirty_page_mapwas never allocated.picasso_allocatewritewatch()allocatedgwwbufbut never the map, sopicasso_getwritewatch()always returned-1and partial flushes copied zero pages — andmark_dirty()from the P96 trap handlers was a no-op. RTG output only updated when something forced a full refresh (mode-switch instant, palette change, GUI open/close).Net effect, exactly as reported in #27: after switching native→RTG the screen stays grey/blank, and after RTG→native the last RTG image stays frozen — until F12 forces
picasso_refresh()(a full refresh), which is why opening and dismissing the GUI "fixes" it.Changes (ported from the main Amiberry repository, adapted to lite)
dirty_page_mapinpicasso_allocatewritewatch()+ min/max dirty-page bounds sopicasso_getwritewatch()only scans the touched range875c4198lput/wput/bput) callingmark_dirty()after every store; banks flaggedS_WRITEso JIT-routed writes also pass through the handlers63a74239unlockscrdirty rectangle to the current surface so a stalewidth_allocatedafter a mode switch can't feedSDL_UpdateTexturean out-of-bounds region756ebf1eVerification
Tested on macOS (SDL2 build) with an A4000/KS 3.1 + Workbench 3.1 + Picasso96 3.6.3 install and a Zorro III UAEgfx board, using the
screentest program attached to issue #27:screen -m 1024x768opens the RTG screen — display switches and shows the test pattern immediately; closing it (LMB) returns to the RTG Workbench. ✅