Repository navigation
honeykrisp: map placed memory at the allocation size - #8
joshuaswarren wants to merge 20 commits into
Conversation
A VK_EXT_map_memory_placed mapping covered the whole BO. The BO cache can return a BO that is larger than the allocation, for example 0x104000 bytes for a 0x100000 byte request. The mapping then covered the page after the allocation in application memory. The conformance test writes guard bytes there and reads them back, and it failed with a crash. Record the length of the live mapping in the BO and use it to unmap. Map a placed mapping with the allocation size, not the BO size. Report VK_ERROR_MEMORY_MAP_FAILED when the map fails, in place of an assert that is compiled out in release builds. A placed mapping can also stay on a BO that goes back to the cache. The next placed map of that BO then returned the old address and made no new mapping. Unmap a placed mapping when its memory is freed, and drop any old mapping before a new placed map. The virtio path takes the new size argument. It was not run.
Review follow-up for the placed map change. Return VK_ERROR_MEMORY_MAP_FAILED if a placed map ends at another address than the one the application asked for, and drop that mapping. The old assert for this was compiled out in release builds. A placed map without a range covers the whole allocation from offset 0. Return an error for any other offset or size. Refuse a placed map when the BO is shared through an import and already mapped, so a mapping that another memory object holds is not torn down. Use one helper for the length of a live mapping in the unmap and the reserve paths. A reserve unmap of a BO with no mapping does nothing.
The placed length now comes from the allocation size. Return an error if it ever exceeds the BO, so mmap cannot map past the GEM object.
|
GREEN at ddfb4cf, with two small notes. I read the full diff against main at 7cda785. I ran no build. Checked:
Notes, neither blocking:
Not covered, as the body says. The shared-BO guard and the offset and size rejection path have no test. Virtio is not run. The Gallium driver is not built. |
|
Both notes are open items. I did not change the branch for either.
|
|
Maintainer routing request for PR 8. This is a request for a merge contact, not a claim that review is finished. It fixes the crash of Question: who can merge into this repository's |
|
Second routing request for PR 8, about 14 hours after the first. It is still a request for a merge contact, not a claim that review is finished. Nothing on the head has changed since my first note above. The review notes and the list of what was run are in the description and the two review comments. Question: who can merge into this repository's main branch? If there is a different place to ask, a link is enough. My account has pull access only, so I cannot merge or mark anything ready. |
|
Status, 25 Oct: no review since I asked for a merge contact. This is the base of the whole-suite run of #6: without it the suite crashes in |
|
Status, 25 Oct, for the landing order (#10 first, then #8, then #9, then #6). Head MEASURED: it is in the control arm of the whole-suite CTS of the #6 stack on a 16-inch M1 Max and a 13-inch M1 (main plus #8, #9 and #10 against that plus #6). The memory_model and map_placed groups ran without a Pass to Fail change from the control arm to the candidate arm. NOT measured: a CTS run with only this change against main. |
|
Batch 1 for main: #10, then #8, then #9, in that order. All three apply to main at
Stack check, one boot on a 13-inch M1, These are targeted checks, not the full conformance suite. The CTS binary's source commit is not independently verified. #6 is not part of this batch. Its run over the touched test groups at its current head has not finished, and three The author account on these PRs has pull-only access, so a maintainer has to merge. Tell us if you want a different order or a different check first. |
|
@maralcbr could you review this current head for landing after #10? The exact-head test evidence and remaining scope limits are in the PR body. The requested landing order is #10, then #8, then #9. Our account cannot assign formal reviewers or merge here; please route this to a main-branch maintainer if needed. |
chriskearney
left a comment
chriskearney
left a comment
There was a problem hiding this comment.
A placed map can invalidate a live image's internal host-copy pointer at ddfb4cf.
A non-sparse image bind stores plane->map = agx_bo_map(mem->bo) + offset in hk_image.c:1334. That internal mapping does not set mem->map. A subsequent application vkMapMemory2KHR with VK_MEMORY_MAP_PLACED_BIT_EXT can therefore reach this call while the image is still bound. The new unconditional agx_bo_unmap(bo) removes that internal mapping and replaces bo->_map, but the image keeps its old pointer. The host-image-copy paths later dereference planes[plane].map (for example hk_image.c:1444 and :1523), which can now be unmapped or reused memory.
The refcount check does not cover this case: binding the image keeps the memory object alive through the application's lifetime rules; it does not create another BO reference. This is distinct from main's existing wrong-address behavior when a placed map finds a cached mapping. Replacing the mapping fixes that address result while introducing a dangling pointer for this valid lifetime combination.
Please keep the internal image mapping independent of the application mapping, or otherwise preserve/reestablish every retained pointer, and add host-image-copy coverage with a placed map after image binding and after unmap. The placed-map contract does not invalidate a bound image when an application maps its memory.
This finding is from source review, not a new device crash or a CTS result. Independent runtime reproduction and the shared regression benchmarks are still pending; their results will be kept separate from this source finding.
| agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr, size_t size) | ||
| { | ||
| if (fixed_addr) | ||
| agx_bo_unmap(bo); |
There was a problem hiding this comment.
This unmap can invalidate hk_image_plane.map, retained when the same allocation was bound to an image. mem->map is still NULL for that internal map and the BO refcount can still be 1, so neither guard above excludes the sequence. Please preserve an independent internal mapping or repair the retained image pointers.
|
Confirmed from the source path: the BO mapping can be retained by a bound image even when mem->map is NULL, so the refcount guard is insufficient. I am withdrawing the READY status and adding a regression for image binding followed by placed map, host copy and unmap before changing the shared mapping path. No runtime reproduction or device result is claimed yet. |
A bound image retains an internal host-copy mapping (hk_image_plane_bind, plane->map = agx_bo_map(mem->bo) + offset) with no mem->map and no BO reference, so a later application placed map reaches agx_bo_map_placed, drops the BO mapping, and leaves the retained plane pointers dangling for the host-copy paths. The new libasahi_placed_map_test drives the real agx_device.h mapping code with the exact bind, placed-map, unmap and reserve expressions from hk_image.c and hk_device_memory.c. The only mock is the kernel mmap, which hands back anonymous pages. Cases: placed map then plain unmap, placed map then VK_MEMORY_UNMAP_RESERVE_BIT_EXT, and two images aliased on one memory object. Fails at ddfb4cf; passes once the internal mapping is independent of the application mapping.
Review follow-up on the CPU fixture: back the BO with one tmpfile (ftruncate to the BO size) and hand out MAP_SHARED mappings, so the internal image mapping, the application mapping and the placed mapping are separate mappings of the same BO contents. The fixture now checks coherence in both directions (retained vs earlier application mapping, placed mapping vs bind-offset contents) in addition to lifetime after unmap. The placed target keeps its PROT_NONE reservation mapped until the MAP_FIXED placed map replaces it. Every resource failure now FAILs the case; the SKIP paths are gone. The shim tracks and cleans up all mappings and the backing file per case. A map-before-bind control case covers the application-map-then-bind order that the proposed fix's stash logic must handle. Case bodies collapse into one data-driven runner; no new framework.
A bound image retains a BO mapping independently of VkDeviceMemory.map. Replacing that mapping for a placed application map left host-copy pointers dangling even with one BO reference. Give internal and application mappings separate slots. Return mmap results from the native and virtio primitives instead of swapping shared state; release both mappings with the BO and preserve placed-map size/error paths. The four CPU regression cases failed on fixture head 9c3850f and now pass: plain unmap, reserve unmap, aliased images, and application-map-before-bind. They use real mapping helpers with shared file-backed kernel mmap emulation. Vulkan entrypoint/device coverage and full driver compilation remain separate qualification gates; no hardware readiness claim.
 [Medium impact] No blocking code findings remain from this review; the PR's stated device checks remain separate merge gates. SummaryApplication mappings now belong to each memory object, while images and tracing keep the BO's stable mapping.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (mem->map && mem->placed_map) | ||
| agx_bo_unmap(mem->bo); |
There was a problem hiding this comment.
hk_FreeMemory can now unmap memory still used by another imported memory object. Export and reimport a BO, placed-map the first object while _map is empty, then normally map the second object. The shared-BO check allows this order, and the normal map returns the same _map.
Freeing the first object now unmaps that address even though the second object remains mapped and holds a BO reference. Later reads or writes through the second object's pointer can crash. Give each memory object its own application mapping, or prevent this sharing before returning the pointer.
There was a problem hiding this comment.
Confirmed in the source: after A creates a placed application map, B can normally map the same BO and receive A's cached pointer; freeing A then unmaps B's live mapping. This head remains NOT READY. I am adding a failing-first two-object/free-one regression and removing BO ownership of application mappings: each VkDeviceMemory will own its app pointer and length, while the BO keeps its stable driver/decoder mapping. No device reproduction is claimed, and no re-review request is being sent before the regression and fix are verified.
|
The follow-up tracing and imported-memory findings expose a remaining ownership error in this head: a BO can be shared by multiple VkDeviceMemory objects, so it cannot own their replaceable application mapping. I am keeping this NOT READY and moving application pointer/length ownership to each memory object, while restoring the stable BO mapping used by driver and decoder callers. New trace and two-importer lifetime regressions will run failing-first. The earlier four CPU cases pass and the full local driver compiles, but neither covers these follow-up cases or replaces device qualification. |
chriskearney
left a comment
chriskearney
left a comment
There was a problem hiding this comment.
At c70e537, trace mode crashes at the first queue submission. This confirms at runtime the tracing finding above, which your 23:25 note already plans to fix.
How it was run: an x86_64 host with Mesa's asahi drm-shim (G13G profile; no kernel, no GPU), and Honeykrisp built from this exact head (release, NDEBUG). A small Vulkan program records a transform feedback draw and calls vkQueueSubmit with ASAHI_MESA_DEBUG=trace.
| Driver | Result |
|---|---|
main 7cda7850ed |
decode completes; 25,311-byte agxdecode dump |
previous head ddfb4cf114 |
decode completes; same dump size |
this head c70e5372 |
SIGSEGV in the first vkQueueSubmit; the dump stops at 78 bytes |
Backtrace: __agxdecode_fetch_gpu_mem (decode.c:136) ← agxdecode_stateful ← agxdecode_drm_cmdbuf ← queue_submit ← vkQueueSubmit.
Cause:
- With tracing on,
agx_bo_createcallsagx_bo_map(), thenagxdecode_track_alloc()stores a copy of the BO (agx_bo.c:397-399). agx_bo_map()now fills only_imap, so the stored copy has_map == NULL.- decode.c:102, :136 and :976 still read
mem->_map/cmdbuf->_map.
Control: a local scratch change that reads _imap before _map at those three lines removes the crash, and the dump is byte-identical to main's. The scratch change only tests the cause; it is not a proposed final shape. The program's transform feedback data check fails under drm-shim on every driver, because nothing executes without a GPU; that check is not part of this result.
The crash is the same in a local merge of main with #10, this head and #9 (d38e71924527392fca5b09c23b9aa6017c52bc2b).
Separately, in the same host setup this head fixes the image-bind/placed-map case from the earlier review. It also fixes host copy after vkUnmapMemory2 and after VK_MEMORY_UNMAP_RESERVE_BIT_EXT, which main fails too. All 6 cases pass, with the image contents read back. These are host CPU-path results, not device or CTS results. Approval still needs the trace path fixed and the two-importer case the bot raised.
…ced-map regression Follow-up findings on the independent-slots change: decode.c reads bo->_map directly (agxdecode_grab_mapped, fetch_gpu_mem, cmdstream) and wrap.c populates ._map at BO creation, so the stable BO mapping must keep existing and stay pointer-stable across application maps; and two memory objects sharing one imported BO must map independently, since an ordinary map currently returns another importer's placed mapping and that importer's unmap destroys it. Two new cases on the same tmpfile-backed shared kernel seam: the decoder-style stable mapping must match agx_bo_map, stay coherent with a placed application mapping and survive placed map + unmap with an unchanged pointer; and two importers (refcnt 2) must get distinct mappings with mutual coherence, where the first importer's unmap leaves the second's mapping valid. Application-map call sites of these cases adapt to the per-memory mapping shape in the production fix commit; assertions do not change. All resource failures FAIL; no SKIP paths.
|
Observational benchmark of the current heads on two M1 Macs. This is not an approval. At What was compared
Workload: the Vulkan benchmark helper from omarchy-m-test GPU suite 1.
Results: median change of the control against the baseline. These are latencies, so lower is better. † marks a case where the min–max ranges of the two arms don't overlap.
Limits
|
…regression The user-required free scenario: two memory objects share one imported BO and one is freed. The new case runs the BO-relevant hk_FreeMemory sequence for the freed object - the placed-unmap condition of hk_device_memory.c:283 and the real agx_bo_unreference refcount drop - and requires the survivor's mapping to stay valid and coherent, with the shared BO left at refcount 1. The heap accounting, ext-BO tracking and vk_device_memory_destroy tail of hk_FreeMemory need a live vk_device and stay behind the admitted device gate.
The free-path case calls the real agx_bo_unreference, which lives in agx_bo.c in libasahi_lib; the test target never linked the library, so the parent's red run failed at link with an undefined reference instead of exercising the refcount. Mirror the libasahi_tests dependency pattern: link_with libasahi_lib, which propagates its own dependency closure (dep_libdrm, idep_nir, libvdrm, libasahi_decode, idep_libagx, idep_libpoly) to the consumer. No test or assertion changed.
Parent-observed red at e14884a59caa (pr8-owner-red3.log): the stable BO mapping was absent because agx_bo_map populated only _imap, and two memory objects sharing an imported BO shared one replaceable mapping - importer B's ordinary map returned importer A's placed mapping, so freeing or unmapping A destroyed B's pointer. Restore bo->_map as the single stable driver-internal mapping and delete the _imap slot: agx_bo_map creates and returns _map (whole BO), decode.c and wrap.c keep reading and populating it directly, the BO cache keeps it as before, and agx_bo_free releases it. Application mappings leave the BO entirely: hk_MapMemory2KHR obtains a fresh mapping per memory object from the returning bo_mmap primitive (placed maps at the requested fixed address, ordinary maps whole-BO at a kernel-chosen address) into the existing mem->map plus a new mem->map_size; hk_UnmapMemory2KHR plain and reserve paths, the placed-address mismatch error path and vkFreeMemory release exactly that object's mapping. The shared-BO guard is removed rather than layered: with no replaceable mapping on the BO there is nothing left for it to protect, and two importers now map independently with coherence coming from the shared pages. Offset/size placed contract, mmap failure handling, the already-mapped check and error results are unchanged. The regression keeps its assertions and adapts only the application call expressions to the per-memory shape. Red lineage cited in the test header (8af285bd30c, e14884a59ca). No device run: hardware, CTS and precision gates hold.
The stable BO mapping always covers the whole BO. Its length is already bo->size. Application mapping lengths belong to each memory object, and the old placed-map flag has no readers after the ownership fix.
|
Independent follow-up at 4e269c5. The source correction addresses the mapping-ownership findings: the BO retains its stable internal/decoder mapping, while each VkDeviceMemory owns its application pointer and length. This is a source/host validation update, not device qualification or merge approval. On an x86_64 address-honouring drm-shim, using the same preserved repro programs and controls:
Separately, the exact clean author union of #10 b87e745, this #8 head, and #9 a8960b7 (local tip 9460a17b877, tree c0404e39) built natively for aarch64 with all 49 registered Meson tests passing, including both Asahi regression targets. No build overlay was needed: #9 now supplies its own idep_nir dependency. Production and test package payloads are byte-identical, independently checked. Still untested on this head: actual Mac/GPU execution, CTS, and the complete heap-accounting/external-BO/Vulkan teardown path for shared imported memory. The earlier observational benchmark stays bound to the old stack and does not qualify this successor. The PR's current device-qualification hold remains. |
Move the map, unmap and reserve-unmap operations into shared helpers so the regression executes the same ownership paths as the Vulkan callers. Register the test with the Vulkan dependency bundle instead of copying its transitive headers. Proof: eight CPU mapping-lifetime cases pass and the full local Vulkan driver compiles. The earlier missing libagx header build failure is retained. No device qualification or merge approval is claimed.
|
Source review at 8eec298: no source defect found. This is a source verdict only. It is not device qualification and not a merge GREEN; the device and maintainer holds named on this PR stay in force. What I checked at this exact head:
Findings, none blocking a source verdict:
Not checked: any device or CTS run (heap accounting, external BO tracking, the teardown tail, placed map after bind on hardware), a full Vulkan driver build (the driver compile is reported on the PR; I built only the test and its library), and the maintainer holds. |
maralcbr
left a comment
maralcbr
left a comment
There was a problem hiding this comment.
Reviewed at 8eec298f16b5ec7e8aa116e12dc83e78950d2159. Requesting changes: the fix is right, but please retarget it to integration/aurora.
The code looks correct on its base. I found no defects.
- Each
VkDeviceMemorynow owns its application mapping (hk_memory_map_from_bo,hk_memory_unmap,hk_memory_unmap_reserve). This fixes aliasing between two imports of one BO. bo->_mapbecomes a stable internal mapping that onlyagx_bo_freedrops. That keeps two retained pointers valid: the image host-copyplane->map = agx_bo_map(mem->bo) + offset, and the trace decoder's copy of the BO taken inagxdecode_track_alloc.- A placed map is sized to the allocation, and
vkFreeMemoryunmaps it before the BO goes back to the cache. - The virtio backends return NULL (virtgpu) or
MAP_FAILED(vpipe), and both are handled. A placed map that lands at a different address fails cleanly and no longer hits an assert.
Blocking: retarget to integration/aurora.
mainmirrors stock Mesa, per the fork policy inAGENTS.mdthere.- Aurora still has the hazard this PR fixes. For non-shared BOs, its
hk_MapMemory2KHRmaps throughagx_bo_map_placed, which munmaps and relocatesbo->_mapfor a placed map (agx_device.h~368–375). Itshk_UnmapMemory2KHRmunmapsbo->_map(hk_device_memory.c~541–553). Meanwhilehk_image.c~2169 keepsplane->map = agx_bo_map(mem->bo) + off. So on the branch we actually ship, image bind followed by any map or unmap, then a host image copy, can use a dangling pointer. Trace mode has the same problem. - Aurora already has
agx_bo_map_owned/bo_mmap_independentfor shared BOs. The port should fold that into this per-memory ownership model and keep Aurora's backend abstractions, not run a second scheme beside it. - Upstream Mesa
mainstill has the oldagx_bo_map_placedpath, so please send this upstream as an MR as well.
Non-blocking:
- The header comment in
tests/test-placed-map.cis review history: GitHub URLs, review-thread ids,file:linereferences, fixture SHAs. Please cut it to what the test checks. Lines 92–93 describe a "stash" fix that the PR no longer uses. - An ordinary
vkMapMemorynow makes a second whole-BO mmap even whenbo->_mapexists. That's fine, but worth a line in the commit message. agx_bostats (mapped_B) no longer count application mappings.
Chris's earlier change requests were against ddfb4cf1 and c70e5372. His 2026-10-30 follow-up says the ownership findings are fixed at 4e269c5, so he may want to update his review state.
|
Correction to my review above: drop the "please send this upstream as an MR as well" point. Upstream Mesa's The rest of the review stands: retarget to |
Every VkDeviceMemory application map takes the backend-owned mapping path, ordinary as well as placed, even when a stable internal BO map already exists; image/decoder users keep that stable map untouched. Map, unmap, reserve and free operate only on the per-memory mapping. No mapping-count statistics exist in the backends, so app maps cannot skew them; command-execution stats are unaffected. Add a CPU regression asserting an ordinary app map leaves bo->_map empty before bind and the later internal map has a different address, alongside the placed-map and shared-importer lifetime cases.
hk_MapMemory2KHR's three-argument call is the contract; the regression called the same helper with a stray device pointer. Remove the parameter and its (void) silencer from the header and update all eight test callsites, so the header, driver and test agree on one signature.
Declare the regression after hk_deps exists and link the Asahi layout and compiler libraries required by libasahi_lib. Guard the stable-map coherence read so the pre-fix control reports the dangling retained map as a lifetime failure instead of crashing in memcmp.
The tested Aurora ownership port supersedes the older main-based backend changes. Keep both histories without reapplying the obsolete implementation. This merge intentionally retains the exact tested tree from 2f8ebd0; it does not merge the pull request into the integration branch.
This comment has been minimized.
This comment has been minimized.
agx_virtio_bo_mmap ignored the length argument and always asked the host for the whole cached BO. A placed map on a BO larger than the VkDeviceMemory then maps past the address the application reserved and records the wrong _map_size, unlike the native backend's length ?: bo->size. Ask vdrm_bo_map for exactly the requested length and fall back to the BO size only for the length-0 ordinary map; the MAP_FAILED cleanup is unchanged. Expose the primitive so the placed-map regression drives the real function against a recorded vdrm stub: placed must request exactly the allocation, length 0 the whole BO, and MAP_FAILED must leave the BO slot cleared. The case is red against the pre-fix length logic and green with it.
Include the production header instead of maintaining a second prototype. The registered nine-case mapping test passes after rebuilding with that declaration.
|
CHANGES at 4ca6985. The virtio length finding is fixed in the shared map function. It passes the requested length to the host and records that same size. Zero length keeps the whole-BO fallback. Failure cleanup is unchanged. All nine CPU cases pass. The new case calls the real function with a recording host stub. It checks placed length, zero length and MAP_FAILED cleanup. The fix owner observed a size assertion fail with the old length logic, not merely a build failure. This does not prove real virtio or GPU behavior. Exact-head review, CI and existing device gates remain open. No merge approval is claimed. |
The real virtgpu mapper returns NULL on ioctl or mmap failure. Normalize both failure sentinels so no failed BO retains a map length. The nine-case CPU regression fails on the new NULL case before this fix and passes afterward; no VM or GPU qualification is claimed.
|
GREEN at 093714a. Source review only. This is not merge approval. An independent review covered the full six-file diff against The native check on 2026-11-01 passed all 13 placed-map CTS cases. The candidate provider trace contains exactly two initialization observations. All five candidate fixture modes passed. Control lifetime was clean; the other four control cases met their original defect checks. The strict image-bind sequence checks passed on those same recorded events. There was no second GPU run of that sequence. The candidate driver SHA-256 was This closes the recorded native scope and this source review only. Real virtio execution remains unproved. Broad matrix, heap and timing qualification are not claimed. Existing CI and review requirements remain in force. The prior failed native attempt is preserved. |
|
Status, 3 Nov 2026 (UTC): no change to head At this head: the base is Not applied: the non-blocking request to trim the review-history header in As on 1 Nov, this is a source review and the recorded native scope only, not merge approval. Real virtio execution remains unproved. |
The header of test-placed-map.c carried review history: a GitHub discussion URL, a thread id, fixture head ids, and file:line references into the driver. It now says what the test checks and how, and names the functions instead of line numbers. The same file:line references in two code comments are gone. Only comments change, plus one failure message that quoted the thread id. Compiling the file with comments stripped gives the same code except that string. hk_placed_map_lifetime passes (1/1).
|
Status, 3 Nov 2026 (UTC): pushed |
|
GREEN on d4fd1a4 Source review of the delta only: Reviewer identity: routed by Main to Sonnet (Claude); the model id is unverifiable from inside the session, and Main's routing notice is the source. The implementer is a different party; implementer model unverified. I did not edit the PR, and I did not build or run the test. Checked at the exact head (fetched
Not checked: that |
|
GREEN at d4fd1a4 Source review of the single commit over 093714a (reviewed by Claude Opus; the commit's author is a different agent). It touches only src/asahi/vulkan/tests/test-placed-map.c. The changes are header and comment trims plus one CASE_FAIL label string ("placed map + unmap" no longer quotes the thread id). No test logic or driver source changed, and the new header lists the map-before-bind case already in the file. CI at this head passes. The 1 Nov device evidence stays bound to 093714a, as the PR comment says. |
|
@maralcbr: this is ready for your re-review at What changed against your review at Your change request and Chris's two are on older heads ( |
|
Note 🤖 GPT-6.1 Terra responding on behalf of joshuaswarren This PR now targets |
|
[Joshua's agent] @maralcbr: the retarget you asked for is done. This PR now targets integration/aurora. Current head: d4fd1a4 (merge state CLEAN, 20 commits). Your CHANGES_REQUESTED is attached to 8eec298, which predates the retarget. Could you re-review at d4fd1a4, or dismiss the old request if the retarget covers it? Your earlier note said the code was correct on its base, so the open point was the base only. |
|
State response at the current head (9 commits since the 4e269c5 follow-up above), covering what changed against that validation point. Since 4e269c5 the branch adds, grouped by the open items in that follow-up:
What this comment does not claim: no new Mac/GPU execution, no CTS, and no new runtime numbers. The open items from the follow-up stand unchanged: actual device execution, CTS coverage, and the complete shared-import heap accounting and teardown path, which drm-shim cannot exercise because it lacks the GEM PRIME handle and fd operations. The device qualification hold on this PR stays until those are covered. On the trace path: the interim shape that produced the first-submit SIGSEGV (a separate internal mapping field that agxdecode copies as NULL) is no longer in the tree at this head; agx_bo_map fills the single stable mapping at allocation, so agxdecode_track_alloc stores a copy with a live mapping. The follow-up's finding that the crash is gone at 4e269c5 is consistent with that design; no separate decode fix is carried on this branch. |
Current status: not ready to merge
Head
d4fd1a4cb06f2f1cfbec79c07afa4b681e492988. Target:integration/auroraata19691114759e2c0507f3502cf6166ef2c6e2e33.Evidence binding (3 Nov 2026): the native, CTS and fixture results recorded on 1 Nov are for commit
093714aec24529ae3b127f3b0439b4f3126923c3. The head above is that commit plus one commit,d4fd1a4cb06f, which trims the review history from the header oftests/test-placed-map.cas the maintainer asked. It changes comments and one failure message in that CPU test; no driver source file changed. With comments stripped, the file differs from093714aec245only in that message.hk_placed_map_lifetimepasses at the new head in a local x86 build. No new device or CTS run was made ford4fd1a4cb06f, so the 1 Nov device evidence stays bound to093714aec245.Two behaviors the review noted: an ordinary
vkMapMemorynow makes its own whole-BO mapping even when the BO's internal map exists, and theagx_bostatistics (mapped_B, fromaccount_bo) count only the internal map, so application mappings are no longer in it.Each app map belongs to one memory object. It no longer moves or drops the BO map used by an image or the trace code. This uses Aurora's map helpers. The heap rules do not change.
The virtio map now honors the requested length. A zero length still maps the whole BO. A failed host map still clears the pointer and size. The regression calls the real map function with a recording host stub. It checks placed length, ordinary mapping and failure cleanup. All nine CPU cases pass. The test includes the production header.
The fix owner observed the new case fail with the old length logic: the map covered 0x104000 bytes instead of the requested 0x100000. That control kept the function linkable. An earlier link failure is not regression evidence. These are CPU checks, not a VM or GPU run.
The real virtgpu host mapper returns NULL on failure. The BO map now clears its size for NULL as well as MAP_FAILED. The added regression failed before the fix: a NULL map kept a size of 0x104000. All nine CPU cases pass after the fix. This checks the real map function with a recording host stub, not a VM.
The original eight-case ownership control failed seven cases and passed the no-op case. It used an adapted shim, not the old tree as shipped. The ownership port's independent source review predates this virtio correction. It is not an exact-head review or merge GREEN for this update.
History is preserved by a forward update. No force push is used. PR10 is closed as already included, not merged. PR8 remains next; PR9 remains held. Current-head CI, exact-head review and GPU or real virtio checks remain required. Old device results below do not qualify this port.
Historical main-based description and results
Current status: READY after #10 (30 October)
Head
8eec298f16b5ec7e8aa116e12dc83e78950d2159.The device check passed on an M1 Apple GPU on 30 October at 13:26 UTC. The exact driver passed all 13 native placed-map CTS cases. The loader trace confirms it loaded the candidate driver.
Four Vulkan checks passed on this head and failed on the control at
7cda7850edd103ace21aac37d416d2fdf7a282e1. Two imported memory objects kept distinct, coherent maps. Freeing one left the other readable and writable. Host copies worked before and after plain unmap, reserve unmap and ordinary unmap of an unaligned allocation. The reserve address could also be mapped again.The control reused the other importer's mapping. Its three host-copy checks each faulted after release, after a working baseline. These were failures inside the cases, not missing features or setup errors.
Placed maps used aligned allocation sizes. The unaligned check used an ordinary map. See VUID-VkMemoryMapInfo-flags-09651.
The eight CPU cases and full local driver build also passed. An independent source review found no defect at this head.
At 14:37 UTC, a separate device check passed the sequence from Chris's finding: image bind, placed map, host copy, plain unmap, then host copy again. A working host-copy baseline was established before the placed map. All four sequence checks passed. The control aborted during the placed map, with the mapping-address assertion in
hk_MapMemory2KHR. Both runs loaded their bound drivers. The packet checks remained clean.Limits: virtio partial-length mapping remains a stated gap for a separate VM test. This is not full CTS.
Landing order stays #10, then #8, then #9. No merge is claimed.
Earlier description and evidence
Status on 29 October (withdrawn, superseded by the status above)
Head
ddfb4cf114a6e72b690581ff6fb2fe8447d11ffa: READY status withdrawn following Chris's source-review finding. A placed application mapping can unmap the BO mapping still retained in a bound image's host-copy pointer;mem->map == NULLand a single BO reference do not exclude this lifetime.Required next steps: reproduce the image-bind → placed-map → host-copy → unmap sequence, preserve the internal mapping independently or repair every retained pointer, and rerun the relevant regressions and exact-head review. The finding is source evidence, not a measured device crash. Earlier targeted results below do not cover this sequence.
Landing order remains #10 first, then corrected/qualified #8, then #9. #9 must wait for #8's correction and qualification.
Historical evidence and original PR description
READY TO MERGE after #10.
Head:
ddfb4cf114a6e72b690581ff6fb2fe8447d11ffa.GitHub reports MERGEABLE against
main: no text conflicts. This is not a test pass.The exact-head tests and GREEN source review below cover this commit. They are targeted checks, not full CTS. Merge it after #10 lands. Preserve this unchanged head; no new run or rebase is needed just to update its status. An authorized maintainer can land it next in the agreed order.
Earlier review, source and test notes are kept below with their original bindings. They do not change this status.
What this fixes
The conformance test
dEQP-VK.memory.map_placed.unmap_reserve.size_1048576crashes the process when it runs after many other memory tests. Four cases in the neighbouringexact_sizegroup fail in that run.omacom/mesamain at7cda7850edhas this crash.The cause is in the placed map path of
VK_EXT_map_memory_placed:vkFreeMemory.What this changes
vkFreeMemoryunmaps a placed mapping that its own memory object holds, before the BO goes back to the cache.VK_ERROR_MEMORY_MAP_FAILED. The old assert for this is compiled out in release builds. The same error covers a placed map at a different address than the one requested, a placed map with an offset other than 0 or a size other than the allocation size, and a placed map of a BO that an import shares and that is already mapped. The driver reportsmemoryMapRangePlacedas false, soVUID-VkMemoryMapInfo-flags-09571andVUID-VkMemoryMapInfo-flags-09572make offset 0 and sizeVK_WHOLE_SIZEor the allocation size the only valid placed map.Test results
The chunk runs use the library bytes of the built package, one run per arm. Control is the package of main at
7cda7850ed, built with the same recipe as the candidate. Candidate is the package of this branch atddfb4cf114.The first 1000 cases of the memory group (chunk 0 of its 7 chunks), the chunk where the control crashes. The 16-inch M1 Max and the 13-inch M1 give the same table.
Exactly 5 of the 1000 rows differ on both chips. No other row differs.
map_placed.exact_size.size_4096map_placed.exact_size.size_8192map_placed.exact_size.size_65536map_placed.exact_size.size_1048576map_placed.unmap_reserve.size_1048576All 13
map_placedcases pass on the candidate.A small test program, run on the 16-inch M1 Max, one boot, with a native build of this branch, one test per process. It covers an ordinary map, an ordinary map then a reserve unmap, a cached larger BO with a placed map (plain and reserve unmap), a placed map after a free without unmap, and an application page at a freed placed address. The control crashes on the cached-BO case with a 0x104000 byte placed map, returns the wrong pointer after a free without unmap, and leaves the placed range mapped after
vkFreeMemory. The branch passes all six tests with placed lengths of 0x100000 and 0x10000.What this does not show
The conformance test expects exact page boundaries here (see
vktMemoryMapPlacedTests.cpp). I found no sentence in the extension text that requires it.Observational two-M1 comparison (not approval)
Chris's comparison binds to main
7cda7850edversus the old control stackd38e7192452(#10b87e7456, #8c70e5372, #911ae82d8) plus build-only overlay645169e3901adding #9's missingidep_nir. It is not evidence for a future ownership-repair head.The 13-inch M1 and M1 Air each ran seven common-path workloads, one warm-up and three measured rounds, plus clean/corrupted compute controls: 30 valid runs per arm, all four arms 30/30. Driver attribution, clean environment, no new kernel fault lines and drained process groups were checked per run. Same aarch64 recipes passed 47/47 registered tests on main and 49/49 on the control. Overall lab jobs still failed a separate known provider-attribution check; those failed verdicts are not being relabelled as passes.
Median latency change of control versus baseline (negative is lower):
These are descriptive n=3 measurements without CPU frequency/affinity control, not statistical significance or equivalence. The source comment identifies which min/max ranges did not overlap. No placed maps, host image copies, capture/replay buffers or transform feedback were exercised, so this qualifies none of the three fixes and does not close the tracing or imported-object defects. The external-display path was not tested. Both Macs were restored afterward. Post-fix correctness and any new-head observations must retain their own source bindings.