fix(rtg): close race and split-screen holes in dirty-page tracking - #2335
Conversation
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).
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. |
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].
![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: 445ea13c2e
ℹ️ 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa5e2e0e36
ℹ️ 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: 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebc1c5c978
ℹ️ 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: 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07d27420e9
ℹ️ 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: 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c66db8238d
ℹ️ 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: 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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Review of the equivalent amiberry-lite fix (BlitterStudio/amiberry-lite#44, fixing BlitterStudio/amiberry-lite#27) surfaced three issues in the non-Windows dirty-page tracking that also apply here:
1. Split-screen drain loses the lower region
With VGA screen splitting active,
picasso_flushpixels()callspicasso_getwritewatch()once per source region, but the emulated write-watch drains the whole dirty map on the first call. Pages belonging to the wrapped lower region are returned during the first pass, discarded by that pass's range check, and then absent when the second pass runs — leaving the lower split region stale. (The Windows path is region-scoped viamman_GetWriteWatch(src_start[split], regionsize, ...), so it does not have this problem.)Fix: drain once per frame and reuse the same page list for both split regions.
2. Region base returned as bare board base
The non-Windows
picasso_getwritewatch()sets*startpto the board base, while Windows returns the board base plus the screen offset. The bare base widens the caller's range filter to pages below the visible screen (offscreen bitmaps, the split region), which get miscopied.Fix: return
base + offset, matching Windows semantics.3. Dirty bounds race with the RTG render thread
With
gfxcard_multithreadenabled,mark_dirty()runs from the gfxmem CPU write handlers whilerender_thread()drains the map viapicasso_getwritewatch()— an unsynchronized race on the bounds (C++ data race), with a concrete lost-update window: a drain can reset the bounds between a writer's bounds-compare and its bit-set, so the page is never scanned until some later write happens to expand the range over it.Fix: the map and bounds are now
std::atomic, andmark_dirty()publishes the bounds only after the page bits are set (monotonic CAS loops), so a concurrent drain always re-covers marked pages on its next pass.Also fixes the
mman_GetWriteWatchdeclaration typo (PULONG→PULONG_PTR lpdwGranularity) to match the real API.Verification