Repository navigation
test(agc): enable VK_KHR_portability_subset in the tests that create their own device - #10920
maximilianfv wants to merge 1 commit into
Conversation
…their own device Thirteen AGC tests create their own Vulkan device and never enable VK_KHR_portability_subset, which the spec requires when the physical device advertises it, so on MoltenVK every one of them logs VUID-VkDeviceCreateInfo-pProperties-04451. Enabling the extension alone turns every subset feature off, so the features the device reports must also be chained into VkDeviceCreateInfo. A shared test helper enumerates the device extensions, and when the subset is advertised enables it and chains the reported VkPhysicalDevicePortabilitySubsetFeaturesKHR; on other devices it only sets the extension list. The beta header is enabled for these thirteen sources only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
One device-selection path appears to miss the macOS portability-enumeration setup. In |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the changed device-creation paths and ran the repository convention check and diff whitespace validation. One macOS device-enumeration gap is described in the review comment below; I am holding approval until it is addressed or explained.
|
@MarcZX10 That fixture already enables portability enumeration on macOS. It just isn't in this diff, because #ifdef __APPLE__
const char* portability = VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME;
info.flags = VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR;
info.enabledExtensionCount = 1;
info.ppEnabledExtensionNames = &portability;
#endifThis PR only adds the device-side |
|
Thanks for pointing me to the unchanged setup in |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the exact head e04a7ecf48b09d18278073319e67770c64486f58 and the portability-subset feature-chain change. The changed fixtures add VkPhysicalDevicePortabilitySubsetFeaturesKHR to the device-create pNext chain before vkCreateDevice; PortabilitySubset::Enable checks the extension and supported feature bits before adding the extension to the create info. I initially flagged NativeMultisampleDepth.cpp, then verified its macOS instance portability-enumeration extension and flag already exist on main and withdrew that point. git diff --check and python3 tools/check_conventions.py --base upstream/main --head upstream/pr-10920 pass. The available hosted eligibility, conventions, intake, labels, and report checks pass; product test jobs are skipped by path filters, so I am not claiming a runtime test from CI. I found no remaining blocking issue in this focused review.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I rechecked exact head e04a7ecf48b09d18278073319e67770c64486f58 after your reply. The existing NativeMultisampleDepth.cpp setup already enables the instance portability-enumeration flag and extension; your correction is accurate, and my earlier comment on that file was mistaken.
I also reran git diff --check upstream/main...upstream/pr-10920 and python3 tools/check_conventions.py --base upstream/main --head upstream/pr-10920; both pass. The focused test change is still awaiting a runnable Vulkan-host validation on a portability-subset device. The added helper appends the reported extension and queries/enables its feature struct; most call sites chain that struct ahead of the previous feature chain. In Recorder.cpp, however, the preexisting feature chain includes BDA/8-bit-storage and optionally dynamic-rendering, GPL, descriptor-indexing and robustness2 structs. Please confirm on an advertised-subset device that the final chain retains all of those entries and the selected feature values, since Enable() inserts the portability struct by rewriting its own pNext and device.pNext.
No product suite was run locally here. Hosted build/product paths are skipped for this test-only change, so I am leaving this as a comment pending that portability-device validation rather than approving from the policy checks alone.
|
@MarcZX10 thanks for rechecking. The validation on a portability-subset device is the For |
|
Thanks for the detailed exact-device report. I verified the helper links |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head e04a7ecf48b09d18278073319e67770c64486f58. The portability-subset helper preserves the existing pNext chain and enables the extension only when advertised. The author's M1 Pro/MoltenVK validation compares all 103 tests run by the affected binaries with and without the change; the recorder's 69 tests drop from 64 portability VUIDs to zero, with no new VUIDs. My earlier NativeMultisampleDepth setup concern was incorrect because the unchanged base already enables instance portability enumeration. git diff --check and check_conventions.py pass. Product CI jobs are skipped by path filters; the GPU validation above is author-reported, not independently rerun here.
|
Repeated your run on an M5 Pro, macOS 27.2 (26B5086k), x86_64 Release under Rosetta 2, MoltenVK and the validation layer from the LunarG SDK 1.4.363, same env ( Base is
AI-assisted: yes |
|
A Windows run of this head on three GPUs: Windows 11 Home 10.0.26200, WinLibs GCC 15.2.0 r7 (MinGW-w64 UCRT), Release. The test set is the 124 tests whose executables compile the changed test sources. One GPU at a time with
No negative control: this PR changes only tests. AI-assisted: yes (Claude Code) |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Build report on Apple silicon at head e04a7ecf48: on current main the nine tests/execution/ sources this PR changes no longer compile, because of the shared precompiled header that #10164 added after this branch's last push.
Machine: Apple M2 Max, macOS 27.2, Apple clang 21 (Xcode 27), x86_64 Release under Rosetta 2, Ninja, BUILD_TESTING=ON.
Tree: main 27ad7244d2 with the open macOS build PRs merged on top (#10366, #7631, #10659, #10660, #11249, #11358, #11047, #11048, #10669, #9263, #8477, #10876, #10552, #7804, #10540, #10676, #11447, #11437, #11554, #11119, #11049), plus this PR, which merges cleanly. That isn't plain main, but main alone doesn't build libkernel on macOS, and none of those PRs touches the execution tests or the PCH.
All nine changed execution tests fail the same way (DepthPlaneGuestLayers, DepthStorage, DepthSurfaceLastWriter, NativeMultisampleDepth, OversizedDescriptorSets, PartiallyResidentSnapshot, PooledBufferRange, SampledCacheBudget, SparseBufferHoles):
tests/execution/../PortabilitySubset.hpp:14:5: error: unknown type name 'VkPhysicalDevicePortabilitySubsetFeaturesKHR'
tests/execution/../PortabilitySubset.hpp:14:59: error: use of undeclared identifier 'VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_PORTABILITY_SUBSET_FEATURES_KHR'
tests/execution/../PortabilitySubset.hpp:23:92: error: use of undeclared identifier 'VK_KHR_PORTABILITY_SUBSET_EXTENSION_NAME'
The compile line does have -DVK_ENABLE_BETA_EXTENSIONS. Since 3fe7aaf3b4, though, every execution test reuses one PCH (REUSE_FROM the first execution target), and that PCH includes VulkanTestDevice.hpp, so <vulkan/vulkan.h>, without the define. By the time the source is compiled, the Vulkan headers have already been included without the beta part, and their include guards keep it out. Clang uses the PCH and doesn't warn that the macro differs. I haven't checked how GCC handles it.
Including <vulkan/vulkan_beta.h> in PortabilitySubset.hpp isn't enough: it supplies the struct and the extension name, but the VkStructureType value is in vulkan_core.h under the same define. What did work: defining it for every execution test, so the shared PCH is built with it too. I added it in the foreach loop that creates the execution tests (the per-source property can then drop the nine execution sources):
target_compile_definitions(${agcExecutionTest} PRIVATE VK_ENABLE_BETA_EXTENSIONS)With that, the nine sources and the whole tree build again; the only remaining failures are two core/libs/tests objects that this PR doesn't touch. The four non-execution sources (BdaDevice.cpp, PerformanceOverlay.cpp, PresentationPass.cpp, Recorder.cpp) don't use the shared PCH, so their per-source property still works.
I haven't run the changed tests on the GPU in this tree yet.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
@maximilianfv this needs a merge of Could you merge |
What
Thirteen AGC tests create their own Vulkan device with
vkCreateDevice:BdaDevice.cpp,PerformanceOverlay.cpp,PresentationPass.cpp,Recorder.cppand, undertests/execution/,DepthPlaneGuestLayers.cpp,DepthStorage.cpp,DepthSurfaceLastWriter.cpp,NativeMultisampleDepth.cpp,OversizedDescriptorSets.cpp,PartiallyResidentSnapshot.cpp,PooledBufferRange.cpp,SampledCacheBudget.cppandSparseBufferHoles.cpp. None of them enablesVK_KHR_portability_subset, which the spec requires whenever the physical device advertises it, so on MoltenVK each of these devices logsVUID-VkDeviceCreateInfo-pProperties-04451. Four of them (BdaDevice.cpp,Recorder.cpp,OversizedDescriptorSets.cpp,SampledCacheBudget.cpp) do not yet request portability enumeration at instance creation onmain, so there the loader does not show them MoltenVK and the helper does nothing until they do; mugurc's macOS series adds that instance flag.Enabling the extension alone is not enough: every subset feature then counts as off, so
VkPhysicalDevicePortabilitySubsetFeaturesKHRwith the values the device reports must be chained intoVkDeviceCreateInfoas well.A new test header,
tests/PortabilitySubset.hpp, enumerates the device extensions. When the subset is advertised, it adds the extension, queries the subset features withvkGetPhysicalDeviceFeatures2(all thirteen instances are Vulkan 1.1) and chains them intoVkDeviceCreateInfo. In every case it sets the extension count and names from the test's extension vector. On a device without the subset it only does that. The subset struct and itssTypeare guarded byVK_ENABLE_BETA_EXTENSIONSin the Vulkan headers, so the CMake change defines it for these thirteen sources only. InDepthPlaneGuestLayers.cppandPartiallyResidentSnapshot.cpp, theconst std::arrayextension list became astd::vectorso the helper can append to it. InPerformanceOverlay.cppthe optional memory-budget extension is now pushed into a vector instead of being passed through a pointer. The tests that enabled no extensions now pass an empty vector.VulkanDevice.cppis not touched. Onmainit does not enable the subset. mugurc's macOS series enables it there without chaining the features (20ee39d, "feat(agc): create the Vulkan device on MoltenVK"), and that part is left to the series.Prior work and credit:
imageViewFormatSwizzle-04465andimageViewFormatReinterpretation-04466instead, and chaining the queried features cleared them in the two fixtures tried. He noted that this needsVK_ENABLE_BETA_EXTENSIONSand left it out of test(agc): load the Vulkan loader with portability enumeration on macOS #7886, which used instance-level portability enumeration for the same nine fixtures and is the precedent for handling portability in each test: test(agc): load the Vulkan loader with portability enumeration on macOS #7886 (comment)VUID-VkSamplerCreateInfo-samplerMipLodBias-04467, and that with the features chained neither error appears. He offered the tests part to whoever takes it: fix(agc): build the frame recorder and the resident heap mirror test on macOS #10540 (comment) . I took it here: fix(agc): build the frame recorder and the resident heap mirror test on macOS #10540 (comment)VulkanDevice.cpponly, also without chaining the features. It changes other tests, but not the device creation in these thirteen.No code was ported from any of these.
Tested
Apple M1 Pro, macOS 27.0.1, x86_64 build under Rosetta 2, Vulkan through MoltenVK from the LunarG Vulkan SDK 1.4.363.
mainalone does not build on macOS, so the tree is mugurc's macOS series (ci/macos-series-2, 27e9689) merged withmainat 2742004, plus other open macOS fixes. Only this change was toggled. Both runs usedANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationandctest -Von the 103 tests that these thirteen binaries run.Validation ErrorlinespProperties-04451imageViewFormatSwizzle-04465The skipped test is
agc_driver_oversized_descriptor_sets(the device binds fewer than 4097 storage buffers per stage or set). It still creates the changed device before it skips, and its 04451 is gone with the change.agc_performance_overlay: the process's private memory can't be read on macOS (open fix(agc): read the process's private memory from task_info on macOS #9971).agc_driver_pooled_buffer_range: a MoltenVK robust-access difference.agc_driver_recover_srgb_allocation.04465errors come from the same four tests in both runs:agc_driver_native_multisample_depth,agc_driver_native_depth_retirement,agc_driver_native_depth_clearsandagc_driver_native_depth_clear_refusals. Without this change no test device enables the subset, so these errors come from the driver's own device in the macOS series, not from the devices this PR changes.was not found but was requested by env var VK_INSTANCE_LAYERSline appears in either log, and the change adds no newVUID-.I could not run the same tests on
mainbecausemaindoes not build on macOS. Linux and Windows were not tested. Native drivers there don't advertise the portability subset, so the change only rebuilds the extension list; a device that does advertise it gets the same handling as MoltenVK.uv run tools/check_conventions.py --base origin/mainpasses.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code