Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect dimension arguments of the ExecuteIndirect method shown in the event browser #2970

Closed
tiruns opened this issue Jun 30, 2023 · 2 comments

Comments

@tiruns
Copy link

tiruns commented Jun 30, 2023

Description

The dimension arguments are read from the argBuf resource, which is accessed via the Map method. However, (due to lack of synchronization?), it takes some time for the Map action to take effect, and before that the contents read from the pointer are all zeros. Therefore, the values of dispatchDimension can be all zeros, which lead to incorrect results in the event browser.

Some workarounds that might help:

I didn't find any content related to thread synchronization on the documentation of ID3D12Resource::Map, but the synchronization operations in the D3D12DebugManager::GetBufferData do solve the problem.

Steps to reproduce

I've sent you the capture file and a screenshot via email.

Diff of my modification:

@@ -3637,7 +3637,12 @@ void WrappedID3D12GraphicsCommandList::PatchExecuteIndirect(BakedCmdListInfo &in

   D3D12_RANGE range = {0, D3D12CommandData::m_IndirectSize};
   byte *mapPtr = NULL;
-  m_pDevice->CheckHRESULT(exec.argBuf->Map(0, &range, (void **)&mapPtr));
+  //m_pDevice->CheckHRESULT(exec.argBuf->Map(0, &range, (void **)&mapPtr));
+
+  bytebuf argBufBytes;
+  m_pDevice->GetDebugManager()->GetBufferData(exec.argBuf, range.Begin, range.End - range.Begin,
+                                              argBufBytes);
+  mapPtr = argBufBytes.data();

   if(m_pDevice->HasFatalError())
     return;

Environment

  • RenderDoc version: 1.27
  • Operating System: Windows 11 (22H2)
  • Graphics API: D3D12
  • GPU: NVIDIA GeForce RTX 3080
  • Driver Version: 536.23
@Zorro666
Copy link
Collaborator

Thank you for reporting the problem and suggesting a proposed fix.
I think there is a slightly different fix which you can try if that is helpful for you.

renderdoc/driver/d3d12/d3d12_command_queue_wrap.cpp
@@ -504,8 +504,8 @@ bool WrappedID3D12CommandQueue::Serialise_ExecuteCommandLists(SerialiserType &se

           for(size_t c = 1; c < info.crackedLists.size(); c++)
           {
-            // ensure all work on all queues has finished
-            m_pDevice->GPUSyncAllQueues();
+           // ensure all GPU work has finished
+           m_pDevice->GPUSync();

             if(m_pDevice->HasFatalError())
               return false;

@tiruns
Copy link
Author

tiruns commented Jul 3, 2023

Thank you for your suggestion and I think it's just the solution that I want.
I'll close the issue since my problem is resolved.

@tiruns tiruns closed this as completed Jul 3, 2023
Zorro666 added a commit to Zorro666/renderdoc that referenced this issue Jul 3, 2023
GPUSyncAllQueues() changed behaviour in e33629c which meant the GPU was not being synchronized when patching the execute indirect arguments when loading a capture.

This led to a race between CPU & GPU and incorrect argument data being read back from the GPU to the CPU and then used for ExecuteIndirect replay.

In addition to fixing the bug changed other calls from GPUSyncAllQueues() to GPUSync() where appropriate ie. single flush validate debug mode.
Zorro666 added a commit that referenced this issue Jul 3, 2023
GPUSyncAllQueues() changed behaviour in e33629c which meant the GPU was not being synchronized when patching the execute indirect arguments when loading a capture.

This led to a race between CPU & GPU and incorrect argument data being read back from the GPU to the CPU and then used for ExecuteIndirect replay.

In addition to fixing the bug changed other calls from GPUSyncAllQueues() to GPUSync() where appropriate ie. single flush validate debug mode.
Zorro666 pushed a commit to Zorro666/renderdoc that referenced this issue Jul 20, 2023
Fixes incorrect shader debugging for ImageFetch operation on arrays on buffers (and certain scenarios of arrays of textures).
Zorro666 pushed a commit that referenced this issue Jul 20, 2023
Fixes incorrect shader debugging for ImageFetch operation on arrays on buffers (and certain scenarios of arrays of textures).
thisisjimmyfb pushed a commit to thisisjimmyfb/renderdoc_public that referenced this issue Jul 21, 2023
GPUSyncAllQueues() changed behaviour in e33629c which meant the GPU was not being synchronized when patching the execute indirect arguments when loading a capture.

This led to a race between CPU & GPU and incorrect argument data being read back from the GPU to the CPU and then used for ExecuteIndirect replay.

In addition to fixing the bug changed other calls from GPUSyncAllQueues() to GPUSync() where appropriate ie. single flush validate debug mode.
thisisjimmyfb pushed a commit to thisisjimmyfb/renderdoc_public that referenced this issue Jul 21, 2023
Fixes incorrect shader debugging for ImageFetch operation on arrays on buffers (and certain scenarios of arrays of textures).
CamilliaX pushed a commit to CamilliaX/renderdoc that referenced this issue Aug 5, 2023
Allow a little latitude on expected line numbers in callstacks test

Protect against invalid socket data potentially causing crashes

Add test of Set*Root32BitConstants with 0 constants being set

* This crashes on nvidia, on compute only strangely, if we pass in a NULL
  pointer along with a 0 number of constants.

Expand NULL-protection for Set*Root32BitConstants

Don't allow tooltip to display if context menu is shown in shader viewer

Add "used" attribute to prevent exported symbol dead-stripping

Ideally would use "retain" but that would require upgrading to recent compilers in CI and the project.

__attribute__((retain)) function/variable to prevent linker garbage
collection.

Ensure image layout is up to date mid-command buffer in pixel history

* Previously we were obtaining the image layout from before the current command
  buffer was recorded, but this is out of date if the image layout has changed
  during the command buffer. Worst case it could be e.g. UNDEFINED if the image
  was created and not used until the current command buffer.

Add documentation for return type

Add missing documentation reference

Remove unimplemented function declarations

Add extra pings to reduce problems with unreasonably slow adb commands

Don't sanitise selected paths from remote hosts. Closes baldurk#3006

* If the remote host disconnects during the selection process we will no longer
  have a valid connection, we shouldn't sanitise the resulting path according to
  local filenames.

Tidy up and implement support for self capture on linux

Handle OpExecutionModeId in SPIR-V reflection

Bump version to v1.29

Handle non-square checkboxes : adjust to make them square

Extension of 7452e29

Change RenderDoc style checkboxes to use checks for low contrast themes

Include InidicatorCheckBox style to match style change in 7452e29

Vulkan PostVS ignore primmitive restart indexes when restart is active

Compute primitive restart using VulkanRenderState not pipeline, to account for dynamic state.
Ignore primitive restart indexes, handles primitive restart indexes being used when there are no active vertex buffer bindings

Fix branch specified in CI badge in README

Make gcc happier with escaped \\ in comments

Hide invalid shader resources

Fixed the problem that showing invalid shader resource in pipeline state viewer since v1.24

Revert "Hide invalid shader resources"

This reverts commit e62b6fa.

Hide invalid shader resources on D3D12

Change RenderDoc style checkboxes to use checks for low contrast themes

* Deliberately low contrast themes like the dark theme can have issues showing a
  box that is filled vs just a plain rectangle which is harder to read at a
  glance. Changing the fill to an X icon gives extra readability on the dark
  theme.

Fix capture comments styling of links

Remove explicit styling span in settings dialog, links style themselves

Ensure D3D12 barrier API is kept happy by matching access to layout

Fix disassembly for arrays of pointers having wrong type declaration

Account for arrays of pointers when calculating buffer offsets

Use correct size for basic types when calculating cbuffer sizes

avoid get color_read_* with depth format

Fix Android Vulkan capturing

Add __attribute__((visibility("default"))) to VK_LAYER_EXPORT on Android.
It used to be defined in the Vulkan headers and changed when the update to the latest Vulkan headers was integrated in 5118f08

GL Pixel History small code clean up in CalculateFragmentDepthTests

Move constant out of inner loop.
Remove if test for depth test being enabled and set the depth function to GL_ALWAYS if the depth test is disabled.

GL Pixel History set preMod.depth before call to QueryPostModPerFragment

Copy the postMod depth to next history's preMod depth.
The preMode depth is used to prime the depth buffer in QueryPostModPerFragment.

Use GL_ANY_SAMPLES_PASSED instead of GL_SAMPLES_PASSED Closes baldurk#2972

GL_SAMPLES_PASSED is not supported by GLES.
The pixel history test is already using a 1x1 scissor test, GL_ANY_SAMPLES_PASSED should be the same as GL_SAMPLES_PASSED.
The pixel history logic is only testing for non-zero passing samples, the precise value is not used.

Workaround nVidia crash in SetGraphicsRoot32BitConstants replay

When Num32BitValuesToSet = 0, set valid dummy pointer for pSrcData.
nVidia driver crashes if pSrcData is NULL.

Add Qt:WindowStaysOnTopHint to resource preview Closes baldurk#2971

Qt on Ubuntu 23.04 includes qt/qtbase@f9e4402ffe, which during show() under certain scenarios (for example Qt::ToolTip windows) would destroy and recreate the xcb window.

Adding Qt:WindowStaysOnTopHint to the window flags for ToolTip windows prevents the xcb window from being destroyed and recreated during show().

Vk tests ignore default pipeline in headless mode

Fixes crash trying to launch VK_Compute_Only

Use innertype id to look up image type. Closes baldurk#2970

Fixes incorrect shader debugging for ImageFetch operation on arrays on buffers (and certain scenarios of arrays of textures).

Set name on newly created named metadata. Closes baldurk#2981 (credit Yun)

Add missing ")" R11G11B10 GetBufferFormatString()

"[[packed(r11g11b10]] float3" -> "[[packed(r11g11b10)]] float3"

There was a missing ")"

Test null DSV in D3D12::GetRenderOutputSubresource

Prevents a crash when refreshing the overlay for am unbound Texture resource and the pipeline state does not have a bound DSV.

Related to 742e3de which changed the internal behaviour of FillResourceView().
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

No branches or pull requests

2 participants