From 31274c7c1c10e7de60577270640c7243e5f58444 Mon Sep 17 00:00:00 2001 From: baldurk Date: Sun, 11 Oct 2015 12:01:17 +0200 Subject: [PATCH] Make sure going between WRITING_IDLE and WRITING_CAPFRAME is atomic * This fixes a bug we had before, where we'd go into CAPFRAME then do a bunch of work before clearing the frame record. * The threading around this probably needs more careful thought - e.g. the snapshotting and chunk insertion on end capture. --- renderdoc/driver/vulkan/vk_core.cpp | 2 - renderdoc/driver/vulkan/vk_core.h | 2 + .../vulkan/wrappers/vk_descriptor_funcs.cpp | 19 ++- .../driver/vulkan/wrappers/vk_queue_funcs.cpp | 130 ++++++++++-------- .../vulkan/wrappers/vk_resource_funcs.cpp | 30 ++-- .../driver/vulkan/wrappers/vk_wsi_funcs.cpp | 27 ++-- 6 files changed, 128 insertions(+), 82 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_core.cpp b/renderdoc/driver/vulkan/vk_core.cpp index 6b143e0f5..7b9a5657e 100644 --- a/renderdoc/driver/vulkan/vk_core.cpp +++ b/renderdoc/driver/vulkan/vk_core.cpp @@ -535,8 +535,6 @@ void WrappedVulkan::EndCaptureFrame(VkImage presentImage) void WrappedVulkan::AttemptCapture() { - m_State = WRITING_CAPFRAME; - { RDCDEBUG("Attempting capture"); diff --git a/renderdoc/driver/vulkan/vk_core.h b/renderdoc/driver/vulkan/vk_core.h index 0c63ee5ac..b6598aff5 100644 --- a/renderdoc/driver/vulkan/vk_core.h +++ b/renderdoc/driver/vulkan/vk_core.h @@ -145,6 +145,8 @@ private: vector m_CmdBufferRecords; VulkanResourceManager *m_ResourceManager; + + Threading::CriticalSection m_CapTransitionLock; uint32_t m_FrameCounter; diff --git a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp index 564dfdca7..975a128fe 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp @@ -273,10 +273,13 @@ VkResult WrappedVulkan::vkAllocDescriptorSets( record->AddParent(GetResourceManager()->GetResourceRecord(layoutID)); // just always treat descriptor sets as dirty - if(m_State != WRITING_CAPFRAME) - GetResourceManager()->MarkDirtyResource(id); - else - GetResourceManager()->MarkPendingDirty(id); + { + SCOPED_LOCK(m_CapTransitionLock); + if(m_State != WRITING_CAPFRAME) + GetResourceManager()->MarkDirtyResource(id); + else + GetResourceManager()->MarkPendingDirty(id); + } record->layout = layoutID; m_CreationInfo.m_DescSetLayout[layoutID].CreateBindingsArray(record->descBindings); @@ -473,8 +476,14 @@ void WrappedVulkan::vkUpdateDescriptorSets( ObjDisp(device)->UpdateDescriptorSets(Unwrap(device), writeCount, unwrappedWrites, copyCount, unwrappedCopies); } + + bool capframe = false; + { + SCOPED_LOCK(m_CapTransitionLock); + capframe = (m_State == WRITING_CAPFRAME); + } - if(m_State == WRITING_CAPFRAME) + if(capframe) { // don't have to mark referenced any of the resources pointed to by the descriptor set - that's handled // on queue submission by marking ref'd all the current bindings of the sets referenced by the cmd buffer diff --git a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp index c0a156dbe..7690b4bb8 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp @@ -330,10 +330,72 @@ VkResult WrappedVulkan::vkQueueSubmit( VkResult ret = ObjDisp(queue)->QueueSubmit(Unwrap(queue), cmdBufferCount, unwrapped, Unwrap(fence)); - // VKTODOHIGH when maps are intercepted with local buffers, this will have to be - // done when not in capframe :(. - if(m_State == WRITING_CAPFRAME) + bool capframe = false; + + for(uint32_t i=0; i < cmdBufferCount; i++) { + ResourceId cmd = GetResID(pCmdBuffers[i]); + GetResourceManager()->ApplyTransitions(m_CmdBufferInfo[cmd].imgtransitions, m_ImageInfo); + + VkResourceRecord *record = GetRecord(pCmdBuffers[i]); + + // need to lock the whole section of code, not just the check on + // m_State, as we also need to make sure we don't check the state, + // start marking dirty resources then while we're doing so the + // state becomes capframe. + // the next sections where we mark resources referenced and add + // the submit chunk to the frame record don't have to be protected. + // Only the decision of whether we're inframe or not, and marking + // dirty. + { + SCOPED_LOCK(m_CapTransitionLock); + if(m_State == WRITING_CAPFRAME) + { + for(auto it = record->bakedCommands->dirtied.begin(); it != record->bakedCommands->dirtied.end(); ++it) + GetResourceManager()->MarkPendingDirty(*it); + + capframe = true; + } + else + { + for(auto it = record->bakedCommands->dirtied.begin(); it != record->bakedCommands->dirtied.end(); ++it) + GetResourceManager()->MarkDirtyResource(*it); + } + } + + if(capframe) + { + // for each bound descriptor set, mark it referenced as well as all resources currently bound to it + for(auto it = record->bakedCommands->boundDescSets.begin(); it != record->bakedCommands->boundDescSets.end(); ++it) + { + GetResourceManager()->MarkResourceFrameReferenced(GetResID(*it), eFrameRef_Read); + + VkResourceRecord *setrecord = GetRecord(*it); + + for(auto refit = setrecord->bindFrameRefs.begin(); refit != setrecord->bindFrameRefs.end(); ++refit) + GetResourceManager()->MarkResourceFrameReferenced(refit->first, refit->second.second); + } + + // pull in frame refs from this baked command buffer + record->bakedCommands->AddResourceReferences(GetResourceManager()); + + // ref the parent command buffer by itself, this will pull in the cmd buffer pool + GetResourceManager()->MarkResourceFrameReferenced(record->GetResourceID(), eFrameRef_Read); + + if(fence != VK_NULL_HANDLE) + GetResourceManager()->MarkResourceFrameReferenced(GetResID(fence), eFrameRef_Read); + + m_CmdBufferRecords.push_back(record->bakedCommands); + record->bakedCommands->AddRef(); + } + + record->dirtied.clear(); + } + + if(capframe) + { + // VKTODOHIGH when maps are intercepted with local buffers, this will have to be + // done when not in capframe :(. for(auto it = m_MemoryInfo.begin(); it != m_MemoryInfo.end(); ++it) { // potential persistent map, force a full flush @@ -364,65 +426,17 @@ VkResult WrappedVulkan::vkQueueSubmit( } } } - } - if(m_State == WRITING_CAPFRAME) - { - CACHE_THREAD_SERIALISER(); - - SCOPED_SERIALISE_CONTEXT(QUEUE_SUBMIT); - Serialise_vkQueueSubmit(localSerialiser, queue, cmdBufferCount, pCmdBuffers, fence); - - m_FrameCaptureRecord->AddChunk(scope.Get()); - } - - for(uint32_t i=0; i < cmdBufferCount; i++) - { - ResourceId cmd = GetResID(pCmdBuffers[i]); - GetResourceManager()->ApplyTransitions(m_CmdBufferInfo[cmd].imgtransitions, m_ImageInfo); - - VkResourceRecord *record = GetRecord(pCmdBuffers[i]); - - if(m_State == WRITING_CAPFRAME) { - for(auto it = record->bakedCommands->dirtied.begin(); it != record->bakedCommands->dirtied.end(); ++it) - GetResourceManager()->MarkPendingDirty(*it); + CACHE_THREAD_SERIALISER(); + + SCOPED_SERIALISE_CONTEXT(QUEUE_SUBMIT); + Serialise_vkQueueSubmit(localSerialiser, queue, cmdBufferCount, pCmdBuffers, fence); + + m_FrameCaptureRecord->AddChunk(scope.Get()); } - else - { - for(auto it = record->bakedCommands->dirtied.begin(); it != record->bakedCommands->dirtied.end(); ++it) - GetResourceManager()->MarkDirtyResource(*it); - } - - if(m_State == WRITING_CAPFRAME) - { - // for each bound descriptor set, mark it referenced as well as all resources currently bound to it - for(auto it = record->bakedCommands->boundDescSets.begin(); it != record->bakedCommands->boundDescSets.end(); ++it) - { - GetResourceManager()->MarkResourceFrameReferenced(GetResID(*it), eFrameRef_Read); - - VkResourceRecord *setrecord = GetRecord(*it); - - for(auto refit = setrecord->bindFrameRefs.begin(); refit != setrecord->bindFrameRefs.end(); ++refit) - GetResourceManager()->MarkResourceFrameReferenced(refit->first, refit->second.second); - } - - // pull in frame refs from this baked command buffer - record->bakedCommands->AddResourceReferences(GetResourceManager()); - - // ref the parent command buffer by itself, this will pull in the cmd buffer pool - GetResourceManager()->MarkResourceFrameReferenced(record->GetResourceID(), eFrameRef_Read); - - if(fence != VK_NULL_HANDLE) - GetResourceManager()->MarkResourceFrameReferenced(GetResID(fence), eFrameRef_Read); - - m_CmdBufferRecords.push_back(record->bakedCommands); - record->bakedCommands->AddRef(); - } - - record->dirtied.clear(); } - + return ret; } diff --git a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp index f0d3fd440..8e581b0fc 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp @@ -94,10 +94,13 @@ VkResult WrappedVulkan::vkAllocMemory( // VKTODOMED always treat memory as dirty for now, so its initial state // is guaranteed to be prepared - if(m_State != WRITING_CAPFRAME) - GetResourceManager()->MarkDirtyResource(id); - else - GetResourceManager()->MarkPendingDirty(id); + { + SCOPED_LOCK(m_CapTransitionLock); + if(m_State != WRITING_CAPFRAME) + GetResourceManager()->MarkDirtyResource(id); + else + GetResourceManager()->MarkPendingDirty(id); + } } else { @@ -235,7 +238,20 @@ void WrappedVulkan::vkUnmapMemory( } else { - if(m_State >= WRITING_CAPFRAME) + // decide atomically if this chunk should be in-frame or not + // so that we're not in the else branch but haven't marked + // dirty when capframe starts, then we mark dirty while in-frame + + bool capframe = false; + { + SCOPED_LOCK(m_CapTransitionLock); + capframe = (m_State == WRITING_CAPFRAME); + + if(!capframe) + GetResourceManager()->MarkDirtyResource(GetResID(mem)); + } + + if(capframe) { if(!it->second.mapFlushed) { @@ -262,10 +278,6 @@ void WrappedVulkan::vkUnmapMemory( // this is true for all non-coherent memory types. } } - else - { - GetResourceManager()->MarkDirtyResource(GetResID(mem)); - } it->second.mappedPtr = NULL; SAFE_DELETE_ARRAY(it->second.refData); diff --git a/renderdoc/driver/vulkan/wrappers/vk_wsi_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_wsi_funcs.cpp index d37b868d1..274fff343 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_wsi_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_wsi_funcs.cpp @@ -619,8 +619,12 @@ VkResult WrappedVulkan::vkQueuePresentKHR( GetResourceManager()->MarkResourceFrameReferenced(swapid, eFrameRef_Read); - EndCaptureFrame(backbuffer); - FinishCapture(); + // transition back to IDLE atomically + { + SCOPED_LOCK(m_CapTransitionLock); + EndCaptureFrame(backbuffer); + FinishCapture(); + } byte *thpixels = NULL; uint32_t thwidth = 0; @@ -943,8 +947,6 @@ VkResult WrappedVulkan::vkQueuePresentKHR( if(RenderDoc::Inst().ShouldTriggerCapture(m_FrameCounter) && m_State == WRITING_IDLE && m_FrameRecord.empty()) { - m_State = WRITING_CAPFRAME; - FetchFrameRecord record; record.frameInfo.frameNumber = m_FrameCounter+1; record.frameInfo.captureTime = Timing::GetUnixTimestamp(); @@ -953,10 +955,19 @@ VkResult WrappedVulkan::vkQueuePresentKHR( GetResourceManager()->ClearReferencedResources(); GetResourceManager()->MarkResourceFrameReferenced(m_InstanceRecord->GetResourceID(), eFrameRef_Read); - GetResourceManager()->PrepareInitialContents(); - - AttemptCapture(); - BeginCaptureFrame(); + + // need to do all this atomically so that no other commands + // will check to see if they need to markdirty or markpendingdirty + // and go into the frame record. + { + SCOPED_LOCK(m_CapTransitionLock); + GetResourceManager()->PrepareInitialContents(); + + AttemptCapture(); + BeginCaptureFrame(); + + m_State = WRITING_CAPFRAME; + } RDCLOG("Starting capture, frame %u", m_FrameCounter); }