Fix Vulkan deferred operation handling errors #1192

mizhen · 2023-07-11T14:28:28Z

The problem
when using deferred operation object in command vkcreateraytracingpipelines, there are two cases with the driver. One is driver proceed all workload in current thread without deferring this command, another case is deferring this command to other threads. The second case is not covered by existing handling. It cause some
title playback failure because reading wrong pipeline data and capture/playback shader group handle, it also cause trim playback failure because ray tracing pipeline creation failure.

The solution
The pull request is the fix for these errors. It added needed handling for deferred command and fixed related errors on pipeline reading, capture/playback shader group handle handling and trim deferred operation object handling.

Result
All tests passed for the target title and other applications include trim trace and full trace capture/playback, these test results show the pull-request fixed the target title playback issue caused by these deferred operation handling errors.

ci-tester-lunarg · 2023-07-11T14:28:30Z

CI gfxreconstruct build queued with queue ID 4468.

ci-tester-lunarg · 2023-07-11T14:30:01Z

CI gfxreconstruct build # 2958 running.

ci-tester-lunarg · 2023-07-11T15:15:35Z

CI gfxreconstruct build # 2958 failed.

ci-tester-lunarg · 2023-07-18T17:55:37Z

CI gfxreconstruct build queued with queue ID 8587.

ci-tester-lunarg · 2023-07-18T17:56:20Z

CI gfxreconstruct build # 2993 running.

ci-tester-lunarg · 2023-07-18T19:42:18Z

CI gfxreconstruct build # 2993 passed.

ci-tester-lunarg · 2023-08-02T13:50:07Z

CI gfxreconstruct build queued with queue ID 16590.

ci-tester-lunarg · 2023-08-02T16:58:30Z

CI gfxreconstruct build # 3050 running.

ci-tester-lunarg · 2023-08-02T17:44:51Z

CI gfxreconstruct build # 3050 passed.

locke-lunarg · 2023-08-20T06:06:13Z

framework/decode/vulkan_replay_consumer_base.cpp

@@ -6155,7 +6161,33 @@ VkResult VulkanReplayConsumerBase::OverrideCreateRayTracingPipelinesKHR(
                                                            createInfoCount,
                                                            modified_create_infos.data(),
                                                            in_pAllocator,
-                                                            out_pPipelines);
+                                                            deferred_operation_info->replayPipelines.data());


deferred_operation_info->replayPipelines is resized when if (deferred_operation_info). It means if it doesn't use deferred operation, it will cause an issue.

Thanks for pointing that out. I'll follow up with a commit.

ci-tester-lunarg · 2023-08-25T19:44:17Z

CI gfxreconstruct build queued with queue ID 30721.

ci-tester-lunarg · 2023-08-25T20:20:26Z

CI gfxreconstruct build # 3137 running.

ci-tester-lunarg · 2023-08-25T21:12:16Z

CI gfxreconstruct build # 3137 passed.

framework/encode/custom_vulkan_api_call_encoders.h

andrew-lunarg · 2023-10-23T14:24:56Z

framework/decode/vulkan_export_json_consumer_base.cpp

+void VulkanExportJsonConsumerBase::Process_vkCreateRayTracingPipelinesKHR(
+    const ApiCallInfo&                                               call_info,
+    VkResult                                                         returnValue,
+    format::HandleId                                                 device,
+    format::HandleId                                                 deferredOperation,
+    format::HandleId                                                 pipelineCache,
+    uint32_t                                                         createInfoCount,
+    StructPointerDecoder<Decoded_VkRayTracingPipelineCreateInfoKHR>* pCreateInfos,
+    StructPointerDecoder<Decoded_VkAllocationCallbacks>*             pAllocator,
+    HandlePointerDecoder<VkPipeline>*                                pPipelines)
+{
+    nlohmann::ordered_json& jdata = WriteApiCallStart(call_info, "vkCreateRayTracingPipelinesKHR");
+    FieldToJson(jdata[NameReturn()], returnValue, json_options_);
+    auto& args = jdata[NameArgs()];
+    HandleToJson(args["device"], device, json_options_);
+    HandleToJson(args["deferredOperation"], deferredOperation, json_options_);
+    HandleToJson(args["pipelineCache"], pipelineCache, json_options_);
+    FieldToJson(args["createInfoCount"], createInfoCount, json_options_);
+    FieldToJson(args["pCreateInfos"], pCreateInfos, json_options_);
+    FieldToJson(args["pAllocator"], pAllocator, json_options_);
+    HandleToJson(args["pPipelines"], pPipelines, json_options_);
+    WriteBlockEnd();
+}


Please remove this, it is the same as the default generated version. We don't want custom code where generated code can work. You probably added this because the default implementation disappeared when you added vkCreateRayTracingPipelinesKHR to vulkan_generators/blacklists.json. Each generator can have its own blocklisk.blacklist JSON file: they don't have to share this one.

Either give the JSON consumer its own blocklist or remove this function from the default blocklist in the JSON consumer header and body generators with a line like self.APICALL_BLACKLIST.remove('vkCreateRayTracingPipelinesKHR') (untested), probably at the botton of __init__() or the top of generate_feature().

I've updated it accordingly, thanks.

andrew-lunarg · 2023-10-23T14:26:40Z

framework/decode/vulkan_export_json_consumer_base.h

+        StructPointerDecoder<Decoded_VkRayTracingPipelineCreateInfoKHR>* pCreateInfos,
+        StructPointerDecoder<Decoded_VkAllocationCallbacks>*             pAllocator,
+        HandlePointerDecoder<VkPipeline>*                                pPipelines) override;
+


Please remove this copy of the generated version. See other comment on body of same module.

I've updated it accordingly, thanks.

ci-tester-lunarg · 2023-10-27T18:49:51Z

CI gfxreconstruct build queued with queue ID 72512.

ci-tester-lunarg · 2023-10-27T18:50:01Z

CI gfxreconstruct build # 3436 running.

ci-tester-lunarg · 2023-10-27T19:42:20Z

CI gfxreconstruct build # 3436 failed.

bradgrantham-lunarg · 2023-10-27T21:58:00Z

CI gfxreconstruct build # 3436 failed.

This failure is just a replay failure for an unknown struct in a vulkaninfo capture in our CI tests. The failure occurred because the PR branch needs to be rebased on dev to bring in support for the upgraded Vulkan structs that were added in recent SDKs.

mizhen · 2023-10-30T13:02:04Z

CI gfxreconstruct build # 3436 failed.

This failure is just a replay failure for an unknown struct in a vulkaninfo capture in our CI tests. The failure occurred because the PR branch needs to be rebased on dev to bring in support for the upgraded Vulkan structs that were added in recent SDKs.

I'll rebase, thank you.

ci-tester-lunarg · 2023-10-31T17:19:24Z

CI gfxreconstruct build queued with queue ID 74963.

mizhen · 2023-11-02T13:44:44Z

While trying to test this, I made a simple case based on Sascha Willem's raytracingbasic example that creates a VkDeferredOperationKHR, uses it with vkCreateRayTracingPipelinesKHR, joins the operation until complete, and then destroys the operation. Please see https://github.com/bradgrantham-lunarg/Vulkan/tree/brad-sascha-raytracingbasic-deferred-crtp.

Is this use of deferredoperation valid, in your opinion? If not, would you let me know why?

With this branch, GFXR capture at c46523a (dev at this moment) can capture and replay raytracingbasic without a problem, but your PR branch outputs this warning on capture:
[gfxrecon] WARNING - GetWrappedId() couldn't find Handle: 14829735431805717965's wrapper. It might have been destroyed
And then gfxrecon-replay from your PR branch fails with this assertion failure:
Assertion failed: (device_info != nullptr) && (pipeline_info != nullptr) && (pData != nullptr) && (pData->GetOutputPointer() != nullptr), file F:\gfxreconstruct\framework\decode\vulkan_replay_consumer_base.cpp, line 6598
Can you please address what might be happening there? Thank you!

EDIT: I tested on an "NVIDIA GeForce RTX 2070 SUPER"

I'm looking into it, will keep you updated, thanks.

bradgrantham-lunarg · 2023-11-01T17:38:05Z

framework/decode/vulkan_replay_consumer_base.cpp

        deferred_operation_info->record_modified_create_infos.clear();
        deferred_operation_info->record_modified_pgroups.clear();
+        deferred_operation_info->replayPipelines.resize(createInfoCount);
+        deferred_operation_info->capturePipelines.resize(createInfoCount);
+        memcpy(deferred_operation_info->capturePipelines.data(),


Is it possible to use deferred_operation_info->capturePipelines->insert? t allows you to omit the sizeof and is more idiomatic C++. I realize there are uses of memcpy in this file but we'd like to move away from memcpy when possible.

ok, will do.

bradgrantham-lunarg · 2023-11-03T22:24:45Z

framework/encode/custom_vulkan_api_call_encoders.cpp

+    if ((result != VK_OPERATION_DEFERRED_KHR) ||
+        (!device_wrapper->property_feature_info.feature_rayTracingPipelineShaderGroupHandleCaptureReplay))
+    {
+        // If the operation is not deferred by driver. or the system doesn't support


As discussed, this block ends up being written twice by the code in this branch. I don't think that's what we want for this solution. If there has to be a deferred operation read of the output variable, we probably need a MetaData command to write that value, and then replay should handle the case of postponing creation of the VkPipeline handles until that MetaData command is written.

I'm confused about what's happening here. Is the problem that the application creates an RT pipeline and gets VK_OPERATION_DEFERRED_KHR then joins the deferred operation until success, then uses the pipeline handles but GFXR somehow writes the operations out of order so the pipeline handles are used in replay before their creation is complete?

Do you have a test case where the application creates an RT pipeline and gets VK_OPERATION_DEFERRED_KHR then joins the deferred operation until success, then uses the pipeline handles but GFXR somehow writes the operations out of order so the pipeline handles are used in replay before their creation is complete?

If that's what's happening, then I think I need to understand how that could happen and we may need to introduce synchronization around the pipeline handles and anything using those pipeline handles, and not create this block twice.

Can you describe in steps how the problem occurs so I can understand how this branch is expected to the issue?

The modified app is valid.

Yes, I think the hardware doesn't support the feature rayTracingPipelineShaderGroupHandleCaptureReplay, it triggers the issue that the above code block generates one more trace block. I'll add a commit for the issue. It may playback the trace of raytracingbasic as current dev branch. However, for this hardware, it's not guarantee playback always work, the reason is the following:

GFXR current framework requires the feature for capture/replay raytracing titles. It needs the related GPU address/ShaderGroupHandle capture/replay extension to reproduce these capture time GPU address/handle value during replay time. Without the feature (rayTracingPipelineShaderGroupHandleCaptureReplay) support, it's not guarantee for replaying raytracing trace file because shadergroup handle value may have different value during replay time, especially for complicated apps or real game, simple app and full trace file replay sometimes may work because the used GPU addresses/handles way less than real title.

ci-tester-lunarg · 2023-11-20T18:38:43Z

CI gfxreconstruct build queued with queue ID 87408.

ci-tester-lunarg · 2023-11-20T23:37:08Z

CI gfxreconstruct build # 3536 running.

ci-tester-lunarg · 2023-11-21T01:34:04Z

CI gfxreconstruct build # 3536 passed.

locke-lunarg · 2023-12-01T08:40:29Z

Thank you, @mizhen , for fixing it. I didn't realize the TrackRayTracingShaderGroupHandles issue for the deferred case. This branch looks good to me. Could you please add a warning message for delaying writing the block of vkCreateRayTracingPipelinesKHR to the capture file? I think users need to know it.

One idea is maybe we could still write the block of vkCreateRayTracingPipelinesKHR to the capture file at original moment, only delay TrackRayTracingShaderGroupHandles. The pipeline handles that are written could be invalid. We could add a MetaData to write pipelines when the pipelines are ready. But it's complicated. It might not be worth. Delaying writing is good.

mizhen · 2023-12-08T15:02:40Z

Thank you, @mizhen , for fixing it. I didn't realize the TrackRayTracingShaderGroupHandles issue for the deferred case. This branch looks good to me. Could you please add a warning message for delaying writing the block of vkCreateRayTracingPipelinesKHR to the capture file? I think users need to know it.

One idea is maybe we could still write the block of vkCreateRayTracingPipelinesKHR to the capture file at original moment, only delay TrackRayTracingShaderGroupHandles. The pipeline handles that are written could be invalid. We could add a MetaData to write pipelines when the pipelines are ready. But it's complicated. It might not be worth. Delaying writing is good.

Thank you for your review, I will make some changes.

ci-tester-lunarg · 2023-12-08T23:20:59Z

CI gfxreconstruct build queued with queue ID 97935.

mizhen · 2023-12-08T23:21:24Z

vkCreateRayTracingPipelinesKHR

Done.

ci-tester-lunarg · 2023-12-08T23:25:03Z

CI gfxreconstruct build # 3612 running.

ci-tester-lunarg · 2023-12-09T01:04:37Z

CI gfxreconstruct build # 3612 passed.

locke-lunarg · 2023-12-10T07:11:13Z

@mizhen Thank you. Could you please rebase dev to fix conflicts? And then, it will be ready to merge.

mizhen · 2023-12-11T13:25:50Z

@mizhen Thank you. Could you please rebase dev to fix conflicts? And then, it will be ready to merge.
OK, will do.

when using deferred operation object in command vkcreateraytracingpipelines , driver has two choices. One is driver proceed all workload in current thread within this command, another choice is deferring this command to other threads. The commit added handling to the second choice and fixed related errors on pipeline reading and capture/playback shader group handle error. Change-Id: Id4704502714b866211555d007e09dcefa69a4138

The deferred operation object info pointer will be nullptr on the condition and the following access to this pointer will cause access error. The commit fixed the issue. Change-Id: Ia9e729f4cf083360801a7ffbefcef759e00efe79

Remove the condition of checking system feature rayTracingPipelineShaderGroupHandleCaptureReplay which may trigger error for some hardware/driver which support raytracing without the feature.

Add warning message for delaying writing the block of vkCreateRayTracingPipelinesKHR to the capture file.

ci-tester-lunarg · 2023-12-15T20:18:05Z

CI gfxreconstruct build queued with queue ID 101804.

mizhen · 2023-12-15T20:18:41Z

@mizhen Thank you. Could you please rebase dev to fix conflicts? And then, it will be ready to merge.
OK, will do.

Done.

ci-tester-lunarg · 2023-12-15T20:20:01Z

CI gfxreconstruct build # 3626 running.

ci-tester-lunarg · 2023-12-15T22:01:06Z

CI gfxreconstruct build # 3626 failed.

locke-lunarg · 2023-12-18T22:33:48Z

CI gfxreconstruct build # 3629 passed. It's good to merge.

mizhen force-pushed the ming-dev-vk-fix-deferred-operation-handling-errors branch from 3cf98ed to 7b11eaf Compare July 18, 2023 17:55

bradgrantham-lunarg requested a review from locke-lunarg July 18, 2023 20:07

mizhen force-pushed the ming-dev-vk-fix-deferred-operation-handling-errors branch from 7b11eaf to 15e0f69 Compare August 2, 2023 13:50

locke-lunarg reviewed Aug 20, 2023

View reviewed changes

bradgrantham-lunarg self-requested a review August 28, 2023 20:34

bradgrantham-lunarg self-assigned this Aug 28, 2023

bradgrantham-lunarg assigned andrew-lunarg Sep 12, 2023

bradgrantham-lunarg requested a review from andrew-lunarg September 26, 2023 16:55

andrew-lunarg suggested changes Oct 23, 2023

View reviewed changes

andrew-lunarg mentioned this pull request Oct 23, 2023

Codegen Blocklists Separation #1319

Open

bradgrantham-lunarg added the P1 Prevents an important capture from being replayed label Oct 30, 2023

mizhen force-pushed the ming-dev-vk-fix-deferred-operation-handling-errors branch from dfadf84 to 46813fd Compare October 31, 2023 17:19

bradgrantham-lunarg requested changes Nov 3, 2023

View reviewed changes

bradgrantham-lunarg assigned panos-lunarg and locke-lunarg and unassigned bradgrantham-lunarg, andrew-lunarg and panos-lunarg Nov 13, 2023

mizhen added 4 commits December 11, 2023 15:51

Fix replay issue on some hardware

afbbfc8

Remove the condition of checking system feature rayTracingPipelineShaderGroupHandleCaptureReplay which may trigger error for some hardware/driver which support raytracing without the feature.

Address code review comment

adee9fb

Add warning message for delaying writing the block of vkCreateRayTracingPipelinesKHR to the capture file.

mizhen force-pushed the ming-dev-vk-fix-deferred-operation-handling-errors branch from 5c4311c to adee9fb Compare December 15, 2023 20:17

locke-lunarg merged commit af796d3 into LunarG:dev Dec 18, 2023
6 checks passed

locke-lunarg mentioned this pull request Dec 18, 2023

Not delay to write vkCreateRayTracingPipelinesKHR block to capture file for deferred case. #1395

Open

Fix Vulkan deferred operation handling errors #1192

Fix Vulkan deferred operation handling errors #1192

Conversation

mizhen commented Jul 11, 2023

ci-tester-lunarg commented Jul 11, 2023

ci-tester-lunarg commented Jul 11, 2023

ci-tester-lunarg commented Jul 11, 2023

ci-tester-lunarg commented Jul 18, 2023

ci-tester-lunarg commented Jul 18, 2023

ci-tester-lunarg commented Jul 18, 2023

ci-tester-lunarg commented Aug 2, 2023

ci-tester-lunarg commented Aug 2, 2023

ci-tester-lunarg commented Aug 2, 2023

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 Aug 25, 2023

ci-tester-lunarg commented Aug 25, 2023

ci-tester-lunarg commented Aug 25, 2023

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 Oct 27, 2023

ci-tester-lunarg commented Oct 27, 2023

ci-tester-lunarg commented Oct 27, 2023

bradgrantham-lunarg commented Oct 27, 2023

mizhen commented Oct 30, 2023

ci-tester-lunarg commented Oct 31, 2023

mizhen commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradgrantham-lunarg Nov 3, 2023 • 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

ci-tester-lunarg commented Nov 20, 2023

ci-tester-lunarg commented Nov 20, 2023

ci-tester-lunarg commented Nov 21, 2023

locke-lunarg commented Dec 1, 2023 • edited Loading

mizhen commented Dec 8, 2023

ci-tester-lunarg commented Dec 8, 2023

mizhen commented Dec 8, 2023

ci-tester-lunarg commented Dec 8, 2023

ci-tester-lunarg commented Dec 9, 2023

locke-lunarg commented Dec 10, 2023

mizhen commented Dec 11, 2023

ci-tester-lunarg commented Dec 15, 2023

mizhen commented Dec 15, 2023

ci-tester-lunarg commented Dec 15, 2023

ci-tester-lunarg commented Dec 15, 2023

locke-lunarg commented Dec 18, 2023

bradgrantham-lunarg Nov 3, 2023 •

edited

Loading

locke-lunarg commented Dec 1, 2023 •

edited

Loading