Skip to content

feat(vulkan): qualify surface adapters with present queues - #267

Merged
kolkov merged 5 commits into
gogpu:mainfrom
besmpl:besmpl/surface-qualified-adapter
Jul 22, 2026
Merged

feat(vulkan): qualify surface adapters with present queues#267
kolkov merged 5 commits into
gogpu:mainfrom
besmpl:besmpl/surface-qualified-adapter

Conversation

@besmpl

@besmpl besmpl commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Qualify Vulkan adapters against the requested surface before opening a device:

  • query every queue family with checked vkGetPhysicalDeviceSurfaceSupportKHR results
  • initially require one queue family that supports both graphics and presentation
  • query and validate the surface capability/format/present-mode snapshot, preserving exact Vulkan format/color-space pairs
  • return a request-local adapter wrapper carrying the selected queue family into Open
  • keep cached surface-independent adapters immutable
  • serialize surface-aware requests around one-shot deferred GLES enumeration
  • release unselected request-local adapters immediately and release the selected ID with its public adapter, bounding repeated surface/window requests and selection failures

This 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 .
  • current golangci-lint v2: zero issues introduced relative to upstream/main

Failure-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 at 5843aab345b024e5d5675376631d963264ee49cf. 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.

@besmpl

besmpl commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Stacked consumer preview: #268 at exact head b11d2671e80081a191705c41c4e77e8b61472017 incorporates this PR's head a3e839f94a12edce98e2496d96e5bd8d3cdd2fc3. The latest request-lifetime commit has a matching stable patch ID; earlier Vulkan overlap is conflict-resolved against Android-specific surface hardening.

The preview remains explicitly draft/non-mergeable and will be rebased to Android-only commits after this prerequisite lands.

@besmpl
besmpl force-pushed the besmpl/surface-qualified-adapter branch from a3e839f to bdc33a1 Compare July 19, 2026 07:14
@codecov

codecov Bot commented Jul 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.17476% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
core/instance.go 95.69% 2 Missing and 2 partials ⚠️
instance_native.go 71.42% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@besmpl
besmpl force-pushed the besmpl/surface-qualified-adapter branch from 6d82f42 to 63efce7 Compare July 19, 2026 08:36

@kolkov kolkov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Request-local qualified adapters — correct separation between cached physical adapters and surface-bound wrappers. Prevents surface state from leaking into ordinary adapter enumeration.
  2. surfaceRequestMu serialization — correctly serializes around one-shot GLES deferred enumeration. Without this, concurrent requests could misclassify each other's surface-bound adapters.
  3. Vulkan queue family iteration — ahead of Rust wgpu which hardcodes queue_family_index = 0 with a TODO. Your presentGraphicsQueueFamily checks all families — this is the correct Vulkan pattern.
  4. Lifetime bounds — unselected qualified adapters released immediately, selected adapter released through Adapter.Release()ReleaseSurfaceAdapter. Instance.Destroy cleans up any remaining.

Minor suggestions (non-blocking)

  • qualifiedCore := adapter shallow-copies halCapabilities pointer — consider nil-ing it on the wrapper or cloning, since the qualified adapter gets capabilities through the wrapper's SurfaceCapabilities(). Currently safe (capabilities are read-only after creation), but a latent aliasing hazard.
  • SurfaceCapabilities on base Adapter removed vkSurface.handle == 0 guard — querySurfaceSnapshot re-checks this internally, but consider keeping it at the outer layer for defense-in-depth.
  • Old vkPresentModeToHAL and vkCompositeAlphaToHAL appear to become dead code after this PR since SurfaceCapabilities is rewritten through makeSurfaceSnapshot. 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.

@besmpl

besmpl commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@kolkov Thank you for the approval and the useful follow-up suggestions. I implemented all three in 4736f85:

  • request-local qualified adapters now clear the shallow-copied halCapabilities, while the cached base adapter remains unchanged;
  • restored the outer Vulkan surface-handle guard;
  • removed the obsolete unchecked present-mode/composite-alpha conversion helpers and their tests.

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.

@kolkov
kolkov merged commit e96d480 into gogpu:main Jul 22, 2026
11 checks passed
kolkov added a commit that referenced this pull request Jul 22, 2026
* docs: update CONTRIBUTING.md — Smart Coding framework, AI-assisted policy, current project structure

* docs: CHANGELOG for merged prerequisite PRs (#264, #265, #266, #267, #269)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants