portable raytracing: Add VulkanAddressReplacer #1879

fabian-lunarg · 2024-11-21T10:02:20Z

Add decode::VulkanAddressReplacer class
Adds (compute-)shader-code used internally by VulkanAddressReplacer
used to sanitize inputs to vkCmdTraceRays | vkCmdBuildAccelerationStructuresKHR
injected calls are marked as such, using decode::Begin/EndInjectedCommands
rebind-allocator: use VMA_ALLOCATOR_CREATE_BUFFER_DEVICE_ADDRESS_BIT

this PR solves issues around portable replay wrt. shader-binding-table (SBT) handling.
it only partially solves remaining issues around VkAccelerationStructure, which will be addressed in a follow-up

belongs to #1526

ci-tester-lunarg · 2024-11-21T10:02:24Z

CI gfxreconstruct build queued with queue ID 307164.

ci-tester-lunarg · 2024-11-21T10:04:33Z

CI gfxreconstruct build queued with queue ID 307178.

ci-tester-lunarg · 2024-11-21T10:12:57Z

CI gfxreconstruct build # 5356 running.

ci-tester-lunarg · 2024-11-21T10:20:36Z

CI gfxreconstruct build queued with queue ID 307197.

ci-tester-lunarg · 2024-11-21T10:25:57Z

CI gfxreconstruct build # 5357 running.

ci-tester-lunarg · 2024-11-21T10:28:31Z

CI gfxreconstruct build queued with queue ID 307213.

ci-tester-lunarg · 2024-11-21T10:33:11Z

CI gfxreconstruct build queued with queue ID 307215.

fabian-lunarg · 2024-11-21T10:54:03Z

android/layer/build.gradle

@@ -24,7 +24,7 @@ android {
        }
        externalNativeBuild {
            cmake {
-                cppFlags "-fexceptions", "-std=c++14", "-Wno-nullability-completeness"
+                cppFlags "-fexceptions", "-std=c++17", "-Wno-nullability-completeness"


fyi @bradgrantham - in this PR, I'm adjusting our android c++ dialect to c++17, matching the other configs.

Thanks for doing this. I think it's important for us to try to maintain consistency between the two. Especially as we add C++17 features into GFXR.

ci-tester-lunarg · 2024-11-21T11:06:52Z

CI gfxreconstruct build # 5359 running.

ci-tester-lunarg · 2024-11-21T11:43:42Z

CI gfxreconstruct build # 5359 failed.

ci-tester-lunarg · 2024-11-21T12:38:13Z

CI gfxreconstruct build queued with queue ID 307286.

ci-tester-lunarg · 2024-11-21T12:40:01Z

CI gfxreconstruct build # 5361 running.

ci-tester-lunarg · 2024-11-21T13:08:55Z

CI gfxreconstruct build queued with queue ID 307306.

ci-tester-lunarg · 2024-11-21T13:15:18Z

CI gfxreconstruct build # 5362 running.

ci-tester-lunarg · 2024-11-21T13:44:09Z

CI gfxreconstruct build queued with queue ID 307342.

ci-tester-lunarg · 2024-11-21T13:51:38Z

CI gfxreconstruct build # 5363 running.

framework/decode/vulkan_device_address_tracker.h

framework/decode/vulkan_address_replacer.cpp

android/framework/decode/CMakeLists.txt

framework/decode/vulkan_replay_consumer_base.cpp

framework/decode/vulkan_address_replacer.h

framework/decode/vulkan_address_replacer.cpp

ci-tester-lunarg · 2024-11-21T14:25:27Z

CI gfxreconstruct build queued with queue ID 307367.

ci-tester-lunarg · 2024-11-21T14:28:08Z

CI gfxreconstruct build # 5364 running.

ci-tester-lunarg · 2024-11-21T15:04:28Z

CI gfxreconstruct build # 5364 failed.

ci-tester-lunarg · 2024-11-21T19:05:12Z

CI gfxreconstruct build queued with queue ID 307821.

ci-tester-lunarg · 2024-11-21T19:10:01Z

CI gfxreconstruct build # 5367 running.

ci-tester-lunarg · 2024-11-21T19:34:42Z

CI gfxreconstruct build # 5367 failed.

ci-tester-lunarg · 2024-11-22T10:11:31Z

CI gfxreconstruct build queued with queue ID 308221.

ci-tester-lunarg · 2024-11-22T10:15:01Z

CI gfxreconstruct build # 5370 running.

ci-tester-lunarg · 2024-11-22T10:41:08Z

CI gfxreconstruct build # 5370 passed.

- fixes to shader-group-handle-replay feature-detection (needs both capture&replay)

…capture

…rds compat

…ingle capture, get back to it

…discard

ci-tester-lunarg · 2024-12-03T09:12:02Z

CI gfxreconstruct build queued with queue ID 315511.

ci-tester-lunarg · 2024-12-03T09:37:21Z

CI gfxreconstruct build # 5486 running.

ci-tester-lunarg · 2024-12-03T10:06:51Z

CI gfxreconstruct build # 5486 passed.

…I-calls

ci-tester-lunarg · 2024-12-03T11:31:02Z

CI gfxreconstruct build queued with queue ID 315569.

ci-tester-lunarg · 2024-12-03T11:35:01Z

CI gfxreconstruct build # 5487 running.

ci-tester-lunarg · 2024-12-03T12:08:27Z

CI gfxreconstruct build # 5487 passed.

MarkY-LunarG

Just mostly a few cleanups and questions.

MarkY-LunarG · 2024-12-03T15:08:55Z

android/layer/build.gradle

@@ -24,7 +24,7 @@ android {
        }
        externalNativeBuild {
            cmake {
-                cppFlags "-fexceptions", "-std=c++14", "-Wno-nullability-completeness"
+                cppFlags "-fexceptions", "-std=c++17", "-Wno-nullability-completeness"


Thanks for doing this. I think it's important for us to try to maintain consistency between the two. Especially as we add C++17 features into GFXR.

MarkY-LunarG · 2024-12-03T15:09:55Z

framework/decode/vulkan_address_replacer.cpp

+    return memory_type_index;
+}
+
+//! 16 bytes


What's the point of this comment?

was me, counting bytes during development. I'll remove it :)

framework/decode/vulkan_address_replacer.cpp

MarkY-LunarG · 2024-12-03T15:12:44Z

framework/decode/vulkan_address_replacer.cpp

+    // NOTE: we expect this map to be populated here, but not for older captures (before #1844) using trimming.
+    if (group_handle_map.empty())
+    {
+        // the capture appears to be older and is missing information we require here -> bail out


Maybe log an INFO_ONCE level message here? (Side note, I don't even know if we have an INFO_ONCE, if not, just use INFO).

I'll check. and yes, was also thinking back/forth to use warning or just INFO

we do have a GFXRECON_LOG_INFO_ONCE, using that

MarkY-LunarG · 2024-12-03T15:13:14Z

framework/decode/vulkan_address_replacer.cpp

+        }
+    }
+
+    // TODO: testing only -> remove when closing issue #1526


Nice! Thanks for remembering to associate a TODO with a PR.

MarkY-LunarG · 2024-12-03T15:21:14Z

framework/decode/vulkan_address_replacer.cpp

+            device_table_->CmdBindPipeline(command_buffer_info->handle, VK_PIPELINE_BIND_POINT_COMPUTE, pipeline_bda_);
+
+            // NOTE: using push-constants here requires us to re-establish the previous data, if any
+            device_table_->CmdPushConstants(command_buffer_info->handle,


Be careful of push constants size. Some HW (Arm?) may have no support or very limited support.

glad you're asking :)

"Push constants have a minimum size of 128 bytes" (https://vkguide.dev/docs/chapter-3/push_constants/)

replacer_params_t is only 40 bytes, so plenty of headroom

I was also worried about this and basically chose this all-in approach with using buffer-device-addresses for everything (input/output addresses, hashmap-storage).

MarkY-LunarG · 2024-12-03T15:22:25Z

framework/decode/vulkan_address_replacer.cpp

+    }
+}
+
+void VulkanAddressReplacer::init_pipeline()


For each of these functions, you might want to put a note that they should only be used in a "mark injected" section. This just makes sure others going forward continue to be consistent

alternatively, instead of putting notes to the private methods,
maybe document this ("all vulkan-commands will be wrapped with markers")
as general contract or "expected behavior" inside a class doc-string?

I wrote a class-doc saying:

Important note: all internal Vulkan-API calls performed by this class are expected to be wrapped by calls to: * - decode::BeginInjectedCommands() / decode::EndInjectedCommands()

MarkY-LunarG · 2024-12-03T15:23:03Z

framework/decode/vulkan_address_replacer.cpp

+
+            if (pipeline_bda_ == VK_NULL_HANDLE)
+            {
+                init_pipeline();


If we hit a fatal error inside this, we should abandon and jump out.

done. we check, print an error and bail out in case this goes wrong

MarkY-LunarG · 2024-12-03T15:24:43Z

framework/decode/vulkan_address_replacer.h

+     * @param callable_sbt          ray-callable sbt
+     * @param address_tracker       const reference to a VulkanDeviceAddressTracker, used for mapping device-addresses
+     * @param group_handle_map      a map from capture- to replay-time group-handles
+     */


Great comment

MarkY-LunarG · 2024-12-03T15:26:19Z

framework/decode/vulkan_address_replacer_shaders.h

+GFXRECON_BEGIN_NAMESPACE(gfxrecon)
+GFXRECON_BEGIN_NAMESPACE(decode)
+
+// g_replacer_sbt_comp: original GLSL source


I have to admit, at first I'm was "what is this?", but then I saw the binary blobs below. I like this way of doing it.

I copied this approach from Dustin :)

ci-tester-lunarg · 2024-12-03T16:25:01Z

CI gfxreconstruct build queued with queue ID 315750.

ci-tester-lunarg · 2024-12-03T16:30:01Z

CI gfxreconstruct build # 5490 running.

ci-tester-lunarg · 2024-12-03T16:58:13Z

CI gfxreconstruct build # 5490 passed.

fabian-lunarg requested a review from MarkY-LunarG November 21, 2024 10:02

fabian-lunarg self-assigned this Nov 21, 2024

fabian-lunarg commented Nov 21, 2024

View reviewed changes

fabian-lunarg requested a review from antonio-lunarg November 21, 2024 13:36

antonio-lunarg reviewed Nov 21, 2024

View reviewed changes

fabian-lunarg force-pushed the fabian_vulkan_address_replacer branch from 281a730 to 2fa629d Compare November 28, 2024 13:40

fabian-lunarg added 14 commits December 3, 2024 10:11

Add bda-replacer shader, implement pipeline. looking good here

7036850

rebind-allocator: use VMA_ALLOCATOR_CREATE_BUFFER_DEVICE_ADDRESS_BIT

872dec0

- fixes to shader-group-handle-replay feature-detection (needs both capture&replay)

comment out debug/test-flags

d29d8cf

formatting fix as usual

bb357f6

compilation fixes for android and msvc

0de4dc3

android: missing header

7f0e68c

msvc: avoid c++20 struct-initialization

6be50ac

VulkanAddresReplacer: bail out if group_handle_map is empty -> older …

b666f2d

…capture

guard execution of replacer for vkCmdBuildAccelerationStructuresKHR

3485b80

guard execution of replacer for vkCmdTraceRays, goal is to fix backwa…

abaf34a

…rds compat

work on PR-feedback

bdbd6eb

Roll back changes to vulkan_device_util, was causing weirdness in a s…

764ea7e

…ingle capture, get back to it

work review-comments, add GFXRECON_ASSERT for map-insert, place a no-…

6420f9c

…discard

Log warning once when address-replacement is actually needed/applied

be4eff2

fabian-lunarg force-pushed the fabian_vulkan_address_replacer branch from 2fa629d to be4eff2 Compare December 3, 2024 09:11

Use newly added decode::Begin/EndInjectedCommands to mark injected AP…

d22f957

…I-calls

MarkY-LunarG approved these changes Dec 3, 2024

View reviewed changes

apply review comments

badee69

fabian-lunarg merged commit 342de43 into LunarG:dev Dec 3, 2024
9 checks passed

fabian-lunarg deleted the fabian_vulkan_address_replacer branch December 3, 2024 17:10

fabian-lunarg mentioned this pull request Dec 4, 2024

Fix a regression in dump-resources, related to vkCmdPushConstants #1906

Merged

portable raytracing: Add VulkanAddressReplacer #1879

portable raytracing: Add VulkanAddressReplacer #1879

Conversation

fabian-lunarg commented Nov 21, 2024 • edited Loading

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 21, 2024

ci-tester-lunarg commented Nov 22, 2024

ci-tester-lunarg commented Nov 22, 2024

ci-tester-lunarg commented Nov 22, 2024

ci-tester-lunarg commented Dec 3, 2024

ci-tester-lunarg commented Dec 3, 2024

ci-tester-lunarg commented Dec 3, 2024

ci-tester-lunarg commented Dec 3, 2024

ci-tester-lunarg commented Dec 3, 2024

ci-tester-lunarg commented Dec 3, 2024

MarkY-LunarG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-lunarg Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Dec 3, 2024

ci-tester-lunarg commented Dec 3, 2024

ci-tester-lunarg commented Dec 3, 2024

fabian-lunarg commented Nov 21, 2024 •

edited

Loading

fabian-lunarg Dec 3, 2024 •

edited

Loading