Aquamarine / patch review

Aquamarine · guided code review

Understand the patch.
Then own it.

A step-by-step explanation of why tiled connectors were hidden incorrectly, how commit c8dd371 makes that decision conservative, and where you should still push back before upstream submission.

Hardening connector suppression — not native tiled rendering
base 9d6fed9 commit c8dd371 5 files +410 −37 48 assertions
Begin the walkthrough Jump to the review plan Open the exact patch

Your learning contract

Target: explain it without this page

  • State the old failure.Why independent maximum dimensions can invent a mode that no connector actually advertises.
  • Trace the safe path.How bytes become tile metadata, a validated grid, and finally a suppression decision.
  • Challenge the boundary.What is intentionally absent, what remains untested, and which choices deserve upstream scrutiny.

01 · Review method

Read intent, mechanism, then evidence.

A raw diff makes you process syntax before you have a model. Use three passes instead. Each pass should answer a different question.

1

Intent

What bad outcome is prevented? What is the safety policy when the kernel’s topology data is malformed or changes mid-scan?

2

Mechanism

Follow one connector from its length-delimited TILE blob through pure validation into tilingRedundant.

3

Evidence

Map every promised invariant to an assertion, then name the remaining integration and hardware gaps yourself.

The governing model

Suppression needs conservative evidence.

Hiding a connector is destructive from the compositor’s point of view. This remains a dimensions-based heuristic, but the patch only acts after five conservative gates agree.

  1. 01Parse exactlyEight unsigned fields from the actual blob length.
  2. 02Group connected tilesSame nonzero TILE group ID; multi-member only.
  3. 03Read againEvery member must still exist and report connected in kernel-known state.
  4. 04Validate the gridComplete, consistent, unique, overflow-safe geometry.
  5. 05Find one exact anchorA single connector advertises the exact aggregate pair.
All gates succeedKeep the unique aggregate connector; mark only its peers redundant under the retained heuristic.
Any modeled gate failsSuppress nothing. Extra outputs are inconvenient; a hidden usable output is worse.
i

The core policy is “fail open.”

Here, “open” means exposing connectors to the compositor. Malformed, incomplete, ambiguous, unreadable, or detected kernel-known disconnected state produces a safe false negative instead of suppression.

The old failure

Two maxima do not make one mode.

The previous code stored the largest width and largest height independently. Those values could come from different advertised modes, yet the pair was treated as real.

Connector advertises
5120 × 1440 3840 × 2880 no 5120 × 2880

The old maxMode became width 5120 and height 2880, even though no mode contained both.

max(width) × max(height)
≠
an advertised mode

The old condition passed and could hide a usable peer.

− OLD CLASSIFICATIONDRM.cpp
- const auto& ti = members.at(0)->tileInfo;- int fullWidth  = ti.numHTile * ti.tileHSize;- int fullHeight = ti.numVTile * ti.tileVSize;- if (conn->maxMode.x >= fullWidth &&-     conn->maxMode.y >= fullHeight)-     fullResConn = conn;
WHY IT IS UNSAFEthree assumptions
// 1. First member represents every tile.
// 2. width × height is a real mode pair.
// 3. the first qualifying connector is unique.

All three can be false.
!

There was a second geometry assumption.

first tile width × column count only works for uniform grids. A valid 2×2 layout can have 1920- and 2560-pixel columns, plus 1080- and 1200-pixel rows. The patch sums distinct columns and rows instead.

Conceptual file order

Five files, one argument.

Review them in dependency order, not alphabetical order. The pure model explains the policy; the DRM adapter explains where it takes effect.

  1. src/backend/drm/Tiling.hppSmall hidden data model: parsed tile, concrete mode pair, connector, validated group.
  2. src/backend/drm/Tiling.cppLength-aware parser and hardware-independent topology validator.
  3. src/backend/drm/DRM.cppNon-probing libdrm state reads, grouping, and the operational suppression mutation: tilingRedundant.
  4. tests/DRMTiling.cpp48 assertions that act as the executable policy specification.
  5. CMakeLists.txtAdds one focused test target by compiling the pure helper directly.

02 · Guided diff

Follow the data, not the line count.

The 410 added lines are mostly a testable policy boundary and its specification. Four chapters cover every substantive change.

01

Parse bytes without guessing.

Tiling.cpp:11–60 · DRM.cpp:1822–1848

The old sscanf call saw a pointer but not the blob length. The new parser receives a string_view over exactly those bytes, consumes exactly eight fields, and refuses partial conversions.

− BEFORE · ABRIDGED SOURCEunbounded / permissive
- char* blobData = (char*)getDRMPropBlob(...);- if (sscanf(blobData,-   "%u:%d:%d:%d:%d:%d:%d:%d", ...) == 8)-     tileInfo = ...;- free(blobData);
+ AFTER · PSEUDOCODElength-aware / exact
+ auto blobData = std::unique_ptr<char,+   decltype(&std::free)>{..., &std::free};+ const auto PARSED = parseTileInfo(+   std::string_view{blobData.get(), blobLen});+ if (PARSED)+   tileInfo = validated values;
  • One optional trailing NUL; no embedded NUL
  • Exactly eight nonempty colon-separated fields
  • Unsigned decimal with complete from_chars consumption
  • Boolean flag is only 0 or 1
  • Nonzero grid and tile sizes
  • Locations must fall inside the declared grid
✓

Defensible domain check: groupId > INT_MAX is rejected.

The kernel’s tile-group ID is an int and its canonical blob serialization uses signed decimal. Positive kernel-produced IDs therefore fit INT_MAX, even though Aquamarine stores the value in uint32_t. A short source comment could make that provenance easier for the next reviewer to recover.

02

Prove the grid is whole.

Tiling.cpp:63–131 · Tiling.hpp:9–35

validateTileGroup is a pure function. It takes plain connector snapshots and either returns trustworthy aggregate geometry or no value. This is the architectural center of the patch.

Aggregate geometry
1920 + 2560 = 4480
1080 + 1200 = 2280

Every tile in one column must agree on width. Every tile in one row must agree on height. Then each column and row contributes exactly once.

GateWhat it rejectsWhy it matters
Countmembers ≠ columns × rowsNo connector disappears merely because only part of a group was discovered.
Identityzero/duplicate connector IDs, mixed group IDsKeeps the anchor sentinel unambiguous and the group coherent.
Coordinatesduplicate or out-of-range locationsEvery grid cell is represented exactly once.
Geometrywidth disagreement within a column; height disagreement within a rowThe aggregate can be derived without trusting the first tile.
Arithmeticaggregate over uint32_tSummation happens in uint64_t; impossible sizes fail closed to validation and open to outputs.
THE ANCHOR RULE · ABRIDGEDexact pair, exactly one connector
for (const auto& MODE : CONNECTOR.modes) {
    if (MODE.width == width && MODE.height == height) {
        hasFullMode = true;
        break; // duplicate modes on one connector are harmless
    }
}

if (another connector also has it)
    ambiguousFullMode = true;

return STileGroup{ .fullModeConnector =
    ambiguousFullMode ? 0U : fullModeConnector };
✓

Defensible separation: valid group ≠ suppressible group.

A complete grid may return successfully with anchor 0 when no connector, or more than one connector, advertises the aggregate pair. The caller then suppresses nothing. Geometry truth and policy certainty remain separate.

03

Re-read kernel-known state before mutation.

DRM.cpp:1161–1237 · operational effect at 969–1005 and 1239–1280

The thin DRM adapter calls drmModeGetConnectorCurrent for every candidate member. This is a second, non-probing read of the connector state already known to the kernel. If the object is missing or reports disconnected, classification aborts instead of hiding anything from that snapshot.

Reset scan-derived state.

parseTileInfo() starts from {}; tilingRedundant is cleared for every connector on every scan.

Group only connected connectors.

Disconnected or unparsed connectors never enter the candidate map; single-member groups take the existing fast path.

Call drmModeGetConnectorCurrent for every member.

Copy actual width/height pairs. A missing object or one whose kernel-known state reports disconnected returns nullopt.

Run the pure validator.

No libdrm object crosses the policy boundary, which keeps the core behavior deterministic and unit-testable.

Mark only peers of the unique anchor.

Existing downstream code disconnects redundant outputs and excludes them from CRTC assignment.

SECOND STATE-READ GUARD · ABRIDGEDDRM.cpp:1181–1191
auto drmConn = drmModeGetConnectorCurrent(fd, conn->id);
if (!drmConn || drmConn->connection != DRM_MODE_CONNECTED) {
    if (drmConn)
        drmModeFreeConnector(drmConn);
    return std::nullopt;
}

for (int i = 0; i < drmConn->count_modes; ++i)
    tile.modes.emplace_back(width, height);
SUPPRESSION MUTATIONDRM.cpp:1224–1234
if (GROUP->fullModeConnector == 0)
    continue;

for (const auto& conn : members) {
    if (conn->id == GROUP->fullModeConnector)
        continue;

    conn->tilingRedundant = true;
}
!

The second read is defensive; it is not a probe or transaction.

drmModeGetConnectorCurrent does not actively probe the hardware. It can observe kernel-known state changes, but a connector can still disappear after the read and before suppression. Later hotplug rechecks can repair state; there is no atomic topology transaction here.

04

Read tests as the policy document.

tests/DRMTiling.cpp:34–166 · CMakeLists.txt:171–177

The new drmTiling executable compiles Tiling.cpp directly. That lets the test call hidden, source-private helpers without exporting them from the shared library or linking the full libdrm backend.

Executable specification
48

assertions

Small enough to read linearly. Treat each test block as a policy decision, not just coverage.

Parser cases
ordinary validtrailing NULemptymissing fieldextra fieldtextnegativeoverflowgroup 0invalid flagzero gridbad locationzero sizeembedded NUL
Topology and policy cases
LG 2×1split maximalarger not exactambiguous anchormissing membermixed gridmixed enclosuremulti-housing acceptedmixed groupduplicate IDduplicate locationnonuniform 2×2geometry mismatchsum overflow
?

Coverage boundary: strong, not exhaustive.

The pure suite does not directly exercise empty validator input, zero first-member group/grid fields, validator-level zero sizes or out-of-range coordinates, row-height mismatch, or height overflow. Separately, state-read failure/disconnection, output/CRTC integration, and real hardware behavior are not unit-tested. Decide whether more cases or an injectable reader add enough confidence without overgrowing this patch.

03 · Contributor judgment

Defend the center. Question the edges.

A seasoned review is not blanket approval. These are the decisions I would defend, preserve with caveats, or ask to revisit.

JudgmentDesign choiceContributor reasoning
DefendFail openA false negative exposes an extra connector. A false positive hides a usable one and can strand the session.
DefendExact width × height pairA larger mode or independent maxima is not evidence of driver-managed tile aggregation.
DefendPure helper + thin adapterThe difficult policy becomes deterministic and testable without introducing a fake DRM stack.
DefendHidden source-private typesNo installed header churn and no new dynamic symbols for an internal classification detail.
DefendRetain existing maxModeClassification no longer relies on it, but the member lives in an installed public struct. Removing it here would change layout/API and mix cleanup into a focused safety fix.
Defend explicitlyisSingleMonitor is consistency-onlyA TILE group composes connectors into one logical screen; this bit describes whether they share one housing. Preserving all-false groups avoids a separate behavior change, while suppressibility still comes from the aggregate-mode heuristic. Make that rationale explicit upstream.
Preserve, caveatSecond connector state readIt conservatively catches a missing object or kernel-known disconnected state. It does not probe, is not atomic, and is not directly unit-tested.
DefendgroupId limited to INT_MAXThe kernel tile-group ID is signed int and canonical serialization uses signed decimal. The parser is validating the producer’s domain, not merely its local storage type.
QuestionResolution-only anchor identityThe patch intentionally preserves the old dimensions-only policy. Ask whether refresh/timing or preferred-mode identity is required, but do not expand scope casually.
MinorAnchor diagnostics collapse to 0“No exact mode” and “more than one anchor” both suppress nothing. Separate debug logs could help field diagnosis but are not required for safety.

● This patch does

  • Strictly parse the DRM TILE blob using its real length.
  • Validate complete, coherent tile-group geometry.
  • Handle nonuniform columns and rows.
  • Require one exact aggregate-mode anchor.
  • Fail open on ambiguity, bad data, or state-read failure.

○ This patch does not

  • Create one native framebuffer across multiple CRTCs.
  • Synchronize color transforms or Night Light CTM across tiles.
  • Coordinate DPMS, page flips, or async commits for a logical group.
  • Activate the larger experimental tiled-output stack.
  • Prove behavior on physical hardware.
!

Do not review this as the UltraFine 5K feature patch.

It is an independently useful hardening patch for Aquamarine’s existing redundant-tile heuristic. The larger aggregation and CTM work remains a separate architectural series with different risks.

04 · Verification ledger

Evidence, with provenance.

These results were reproduced against the clean branch on August 28, 2026. The commands deliberately remove a machine-specific library override.

Debug CTest
5 / 5

clean build, all passed

Release CTest
5 / 5

clean build, all passed

Diff check
clean

no whitespace errors

Public headers/API paths
0

installed interface files changed

!

Reproducibility trap discovered during this review.

The shell inherits an LD_LIBRARY_PATH pointing at a deployed experimental Aquamarine build. Old prototype test binaries then load that library before their own build output and fail with irrelevant missing symbols. Use a fresh build directory from this branch and env -u LD_LIBRARY_PATH.

cd "$HOME/Projects/tiled-display/aquamarine-upstream-v2-wt"
env -u LD_LIBRARY_PATH cmake -S . -B /tmp/aquamarine-c8dd371-debug -DCMAKE_BUILD_TYPE=Debug
env -u LD_LIBRARY_PATH cmake --build /tmp/aquamarine-c8dd371-debug -j4
env -u LD_LIBRARY_PATH ctest --test-dir /tmp/aquamarine-c8dd371-debug --output-on-failure
CheckResultHow to interpret it
Debug and ReleaseReproducedAll five configured branch tests pass: four pre-existing tests plus the new drmTiling target.
Patch replayVerified in final auditgit am onto base 9d6fed9 produced the exact commit tree.
ASan + UBSan5 / 5 in final auditThe full sanitizer suite passed with leak detection disabled. Focused drmTiling also passed with leak detection enabled. A separate full leak-detection run reported one 128-byte allocation beneath a Mesa/GBM stack while running environment-dependent simpleWindow, outside the patch path.
Dynamic symbolsUnchanged in final auditRelease defined-symbol names matched the base; hidden NDRM helpers did not enter the dynamic interface.
Nix / upstream CINot localNix is unavailable on this machine. Treat CI as a required gate, not as assumed success.
Physical hotplug/hardwareNot proved by unit testsThe patch adds conservative validation gates, but real driver and hardware behavior remains an integration check.
 
git -C "$HOME/Projects/tiled-display/aquamarine-upstream-v2-wt" diff --check 9d6fed9..c8dd371
sha256sum "$HOME/Projects/tiled-display/upstream-patches/aquamarine-9d6fed9-c8dd371/0001-drm-validate-tiled-groups-before-suppressing-connect.patch"
# expected: dc58953120adde209b93d8747bf66e9c21ba80b2b9da5087063bd86606dab89c

05 · Your review route

Make the patch yours in 45 minutes.

Do this with the code open beside the guide. The point is not to memorize every condition; it is to be able to justify every class of condition.

  1. 5 min · Say the bug aloud.Explain the split-maxima example and why hiding a usable connector is the dangerous failure mode.
  2. 8 min · Read the pure types and parser.Inspect Tiling.hpp, then Tiling.cpp:11–60. Challenge every accepted and rejected input class.
  3. 10 min · Prove the topology algorithm.Sketch a 2×2 grid. Confirm member count, uniqueness, column/row consistency, summation, overflow, and anchor selection.
  4. 8 min · Trace the DRM adapter.Follow markRedundantTiles() into validatedTileGroup(), then forward into disconnection and CRTC assignment.
  5. 8 min · Match promises to tests.For each test block, name the production condition it protects. Circle the libdrm state-read path and hardware behavior as untested integration seams.
  6. 6 min · Write your own verdict.Record what you defend, what you would change, and why the scope should remain independent from native aggregation.
git -C "$HOME/Projects/tiled-display/aquamarine-upstream-v2-wt" show --stat c8dd371
git -C "$HOME/Projects/tiled-display/aquamarine-upstream-v2-wt" diff 9d6fed9..c8dd371 -- src/backend/drm/Tiling.cpp
git -C "$HOME/Projects/tiled-display/aquamarine-upstream-v2-wt" diff 9d6fed9..c8dd371 -- src/backend/drm/DRM.cpp
git -C "$HOME/Projects/tiled-display/aquamarine-upstream-v2-wt" diff 9d6fed9..c8dd371 -- tests/DRMTiling.cpp
git -C "$HOME/Projects/tiled-display/aquamarine-upstream-v2-wt" diff 9d6fed9..c8dd371 -- CMakeLists.txt src/backend/drm/Tiling.hpp

Self-check · reveal answers only after responding

Can you explain these?

Why is maxMode.x ≥ width && maxMode.y ≥ height not enough?

The two maxima may come from different modes. Even one larger mode is not proof of the exact aggregate pair. The patch searches concrete width/height pairs and requires equality.

Why does the validator sum columns and rows instead of multiplying the first tile?

TILE permits nonuniform geometry. Every tile in a column must agree on that column’s width and every tile in a row on that row’s height; the aggregate is the sum of each distinct column and row.

What is the difference between an invalid group and a valid group with anchor 0?

An invalid group has untrustworthy topology and returns nullopt. A valid group may have trustworthy geometry but no unique exact aggregate-mode connector. It returns dimensions with fullModeConnector = 0. Both paths suppress nothing, but for different reasons.

What does the second connector state read protect against—and what does it not?

It fails open when the connector object cannot be read or its kernel-known state reports disconnected. drmModeGetConnectorCurrent does not actively probe, and the read does not make the scan-to-suppression sequence atomic; a later disappearance is still possible and handled by subsequent hotplug rechecks.

Why are the helper types hidden and compiled directly into the unit test?

They describe an internal DRM classification policy, not Aquamarine’s supported public API. Hidden visibility preserves the shared library interface, while direct compilation lets the pure logic be tested without exporting it.

Why reject a groupId above INT_MAX when Aquamarine stores it in uint32_t?

Because the producer’s domain matters too: Linux DRM models the tile-group ID as int and serializes its canonical text form with signed decimal. A positive canonical ID cannot exceed INT_MAX. The check is defensible even though the destination is unsigned.

Human gate

Ready means you can disagree intelligently.

This guide is an educational map, not upstream prose. Before submitting, personally review every changed line, resolve the open judgments above, author the rationale in your own words, and follow Hyprland’s AI-assistance disclosure and contributor requirements.

  • I can reproduce the old false positive.
  • I can state every fail-open branch.
  • I checked the parser’s accepted domain.
  • I traced the operational effect on outputs and CRTCs.
  • I know which paths lack integration tests.
  • I can defend why aggregation and CTM are separate.
  • I verified the kernel-domain rationale for groupId.
  • I will write the upstream explanation myself.
Read the AI policy PR guidelines Exact patch · SHA-256 verified