feat(vulkan): qualify surface adapters with present queues - #267
Conversation
|
Stacked consumer preview: #268 at exact head The preview remains explicitly draft/non-mergeable and will be rebased to Android-only commits after this prerequisite lands. |
a3e839f to
bdc33a1
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
bdc33a1 to
6d82f42
Compare
6d82f42 to
63efce7
Compare
kolkov
left a comment
There was a problem hiding this comment.
Thorough review of the full diff and comparison with Rust wgpu-core request_adapter (instance.rs:428-560) and wgpu-hal Vulkan swapchain (native.rs:37-110).
This is a high-quality architectural improvement. The key design decisions are sound:
- Request-local qualified adapters — correct separation between cached physical adapters and surface-bound wrappers. Prevents surface state from leaking into ordinary adapter enumeration.
surfaceRequestMuserialization — correctly serializes around one-shot GLES deferred enumeration. Without this, concurrent requests could misclassify each other's surface-bound adapters.- Vulkan queue family iteration — ahead of Rust wgpu which hardcodes
queue_family_index = 0with a TODO. YourpresentGraphicsQueueFamilychecks all families — this is the correct Vulkan pattern. - Lifetime bounds — unselected qualified adapters released immediately, selected adapter released through
Adapter.Release()→ReleaseSurfaceAdapter.Instance.Destroycleans up any remaining.
Minor suggestions (non-blocking)
qualifiedCore := adaptershallow-copieshalCapabilitiespointer — consider nil-ing it on the wrapper or cloning, since the qualified adapter gets capabilities through the wrapper'sSurfaceCapabilities(). Currently safe (capabilities are read-only after creation), but a latent aliasing hazard.SurfaceCapabilitieson baseAdapterremovedvkSurface.handle == 0guard —querySurfaceSnapshotre-checks this internally, but consider keeping it at the outer layer for defense-in-depth.- Old
vkPresentModeToHALandvkCompositeAlphaToHALappear to become dead code after this PR sinceSurfaceCapabilitiesis rewritten throughmakeSurfaceSnapshot. Consider removing or marking for removal to avoid divergence between checked/unchecked paths.
Test coverage is comprehensive: 487 new test lines covering cleanup, concurrency serialization, mock preservation, incompatible rejection, VK_INCOMPLETE retry, format/color-space preservation, and unknown mapping rejection.
LGTM.
|
@kolkov Thank you for the approval and the useful follow-up suggestions. I implemented all three in
The additions have focused regression coverage; the branch remains mergeable and all 10 repository Actions checks are green. A quick re-review would be appreciated in case the new commit dismissed the prior approval. |
Summary
Qualify Vulkan adapters against the requested surface before opening a device:
vkGetPhysicalDeviceSurfaceSupportKHRresultsOpenThis follows the Rust wgpu shape: adapter selection for a compatible surface proves the presentation queue that device creation will actually use.
Why
The previous path could enumerate an adapter without proving surface support, then open queue family zero (or another graphics-only family). Capability-query failures could also collapse into an apparently usable adapter.
Request-local wrappers must have request-local lifetime as well; otherwise repeated surface replacement, multi-GPU selection, and failed fallback selection retain qualified adapter and surface state until the whole instance is destroyed. Serializing the snapshot with deferred GLES enumeration also prevents concurrent requests from misclassifying another request's surface-bound adapter as their own.
Validation
Go 1.25.12 and Go 1.26.5:
go test ./core ./hal/vulkan .go test -race ./core ./hal/vulkan .go vet ./core ./hal/vulkan .upstream/mainFailure-injection tests cover split queues, failed Vulkan queries, request-local ownership and cleanup, selection errors, repeated requests, and cached-adapter immutability.
Split graphics/present queues remain deliberately unsupported in this first correction.
Android integration relationship
This Android-independent change is one of five prerequisites replayed in #268. Its canonical candidate head is
63efce782d5cf41dacab761d202da0e3d68ed2da; the corresponding replay in #268 ends at5843aab345b024e5d5675376631d963264ee49cf. The #268 description records the current exact integration head and full replay map; it drops this replay after this PR merges. Rust-v29 typed surface parity remains separate in #273. Android runtime and release claims remain in #268.