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

m_cleanupImageBarriers in WrappedVulkan does not update its size #3492

Closed
wants to merge 6 commits into from

Conversation

JorenJoestar
Copy link
Contributor

Add missing array resize for ImageBarriereSequence when replaying Vulkan captures, as ImageBarrierSequence::SetMaxQueueFamilyIndex does not update the arrays but just the static index.

Already wrote more informations in #3491, but will post them here as well!

Description

Vulkan crash when opening a capture, using a custom Vulkan renderer that has any queue family index > MaxQueueFamilyIndex.

The crash is happening in WrappedVulkan::ApplyInitialContents(), line 3708:

it->second.LockWrite()->ResetToOldState(m_cleanupImageBarriers, GetImageTransitionInfo());

Basically m_cleanupImageBarriers has the array that is not updated if MaxQueueFamilyIndex is updated.

I've downloaded latest source code this morning, and by debugging I found out that m_cleanupImageBarriers is not resized if the QueueFamilyIndex is > MaxFamilyIndex in the method bool WrappedVulkan::Serialise_vkCreateDevice in vk_device_funcs.cpp.

In line 4059 on:

for(uint32_t i = 0; i < createInfo.queueCreateInfoCount; i++)
{
uint32_t qidx = createInfo.pQueueCreateInfos[i].queueFamilyIndex;
m_ExternalQueues.resize(RDCMAX((uint32_t)m_ExternalQueues.size(), qidx + 1));

// This does NOT resize m_cleanupImageBarriers nor m_setupImageBarriers!!!
ImageBarrierSequence::SetMaxQueueFamilyIndex(qidx);

just adding this (with better interface, probably an utility method in ImageBarrierSequence:

for(uint32_t mbc = 0; mbc < ImageBarrierSequence::MAX_BATCH_COUNT; mbc++)
{
m_setupImageBarriers.batches[mbc].resize_for_index(qidx);
m_cleanupImageBarriers.batches[mbc].resize_for_index(qidx);
}

BEFORE the ImageBarrierSequence::SetMaxQueueFamilyIndex(qidx); call it fixes the problem.

Using a Vulkan renderer that has a QueueFamily > 4 and do an image transition with that.
I am using that with a transfer queue that has FamilyIndex = 5, thus the crash.
Capture a frame, and when opening RenderDoc will crash.

Add missing array resize for ImageBarriereSequence when replaying Vulkan captures, as ImageBarrierSequence::SetMaxQueueFamilyIndex does not update the arrays but just the static index.
Copy link
Owner

@baldurk baldurk left a comment

Choose a reason for hiding this comment

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

This change looks good, I can merge it once CI is green.

For future note, there is no point in creating an issue if you are also immediately going to create a PR fixing the issue. It only creates more noise and extra work without adding any value.

@JorenJoestar
Copy link
Contributor Author

You are totally right Baldur, next time I will just open a pull request! :)

Change is ready and most of all...an heartfelt thank you for creating RenderDoc.
Years I am using it, and it is quintessential to my rendering work, at home and at work!

@baldurk
Copy link
Owner

baldurk commented Nov 26, 2024

Thanks for that, and I'm glad that RenderDoc is useful to you.

As in the CONTRIBUTING documentation, please squash your changes to ensure you do not have any merge commits and that every commit individually is correctly formatted and compiles, to avoid any commits that only do reformatting or compile fixes.

JorenJoestar and others added 3 commits November 27, 2024 22:29
m_cleanupImageBarriers in WrappedVulkan does not update its size

Add missing array resize for ImageBarriereSequence when replaying Vulkan captures, as ImageBarrierSequence::SetMaxQueueFamilyIndex does not update the arrays but just the static index.

Fixed clang formatting

Add NV project on all platforms, only include aftermath stubs

Fix mac

Preliminary Vulkan AS/RQ test

This test renders a small triangle that casts a shadow from a point light, onto a larger triangle.  The test runner then checks that various pixels in the final output are the correct colour.

There is also an arbitrary AS copy in the render loop just to hit more API coverage when manually capturing, but the test runner doesn't check its output.

Core test work originally done by [email protected]

DXIL Control Flow Change FindUniformBlocks to ignore blocks in a loop

DXIL Debugger error if no threads are marked as active

Run all threads which haven't finished

DXIL Control Flow added a simple if/then test case

DXIL Debugger improve the detection of when threads are diverged

Discount threads which have finished which could include helper threads which have called discard

Patch BDA feature in 1.2 promoted feature struct query

* If BDA capture/replay is not supported we patched the feature query for the
  vulkan 1.2 struct, but if an application used the promoted KHR extension
  struct we weren't patching that because it was previously invalid to query on
  the extension itself.

DXIL ControlFlow Optimisations

DXIL Debugger align up to 16-bytes the constant buffer data size

D3D12 Pixel History scale minimum number of fragments with event count

Increased the buffer storage to support a minimum of 1024 fragments

D3D12 Pixel History do not crash when processing an unknown fragment

This could happen with non-deterministic replay

D3D12 Pixel History do not copy depth for non-raster events

This can be invalid i.e. a ClearRenderTarget() no guarantee what/if a depth target is bound
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