Corrected and Documented CommandBuffer State Management and Checking.

+ Unified return types of begin/end/reset function
+ fixed compiler errors

Author: Erfan-Ahmadi

include/nbl/asset/ICommandBuffer.h

@@ -118,6 +118,23 @@ class ICommandBuffer
         ERF_RELEASE_RESOURCES_BIT = 0x01
     };
 
+    /*
+    CommandBuffer Lifecycle Tracking in Nabla:
+        * We say a command buffer is "resettable" If it was allocated from a command pool which was created with `ECF_RESET_COMMAND_BUFFER_BIT` flag.
+        - ES_INITIAL 
+            -> When a command buffer is allocated, it is in the ES_INITIAL state.
+            -> If a command buffer is "resettable", Calling `reset()` on a command buffer will change it's state to ES_INITIAL If it's not PENDING
+        - ES_RECORDING
+            -> Calling `begin()` on a command buffer will change it's state to `ES_RECORDING` If It's not already RECORDING, and should be INITIAL for non-resettable command buffers.
+        - ES_EXECUTABLE
+            -> Calling `end()` on a command buffer will change it's state to `ES_EXECUTABLE` If it's RECORDING
+            -> After submission for non-resettable command buffers.
+        - ES_PENDING
+            * ES_PENDING Is impossible to track correctly without a fence. So `ES_PENDING` actually means the engine is in the process of SUBMITTING and It will be changed to either `ES_EXECUTABLE` or `ES_INVALID` after SUBMISSION.
+            * So the convention here is different than Vulkan's command buffer lifecycle and therefore contains false negatives (It is not PENDING but actually is PENDING and working on GPU) 
+        - ES_INVALID
+            -> After submission for resettable command buffers.
+    */
     enum E_STATE : uint32_t
     {
         ES_INITIAL,
@@ -193,27 +210,34 @@ class ICommandBuffer
     // hm now i think having begin(), reset() and end() as command buffer API is a little weird
 
     //! `_flags` takes bits from E_USAGE
-    virtual void begin(uint32_t _flags)
+    virtual bool begin(uint32_t _flags)
     {
-        assert(m_state != ES_PENDING);
+        if(m_state == ES_RECORDING)
+        {
+            assert(false);
+            return false;
+        }
         m_state = ES_RECORDING;
         m_recordingFlags = _flags;
+        return true;
     }
-
+   
     //! `_flags` takes bits from E_RESET_FLAGS
     virtual bool reset(uint32_t _flags)
     {
-        if (m_state==ES_PENDING);
-            return false;
         m_state = ES_INITIAL;
-
         return true;
     }
 
-    virtual void end()
+    virtual bool end()
     {
-        assert(m_state!=ES_PENDING);
+        if(m_state!=ES_RECORDING)
+        {
+            assert(false);
+            return false;
+        }
         m_state = ES_EXECUTABLE;
+        return true;
     }
 
     virtual bool bindIndexBuffer(const buffer_t* buffer, size_t offset, E_INDEX_TYPE indexType) = 0;

include/nbl/video/IGPUCommandBuffer.h

@@ -54,14 +54,47 @@ class IGPUCommandBuffer :
     >;
 
 public:
+
+    inline bool isResettable() const
+    {
+        return m_cmdpool->getCreationFlags().hasValue(IGPUCommandPool::ECF_RESET_COMMAND_BUFFER_BIT);
+    }
+
+    inline bool canReset() const
+    {
+        if(isResettable())
+            return m_state != ES_PENDING;
+        return false;
+    }
+
     virtual bool begin(uint32_t _flags, const SInheritanceInfo* inheritanceInfo = nullptr)
     {
-        base_t::begin(_flags);
-        if ((m_cmdpool->getCreationFlags()&IGPUCommandPool::ECF_RESET_COMMAND_BUFFER_BIT)==0u)
+        if (!isResettable())
         {
-            assert(m_state != ES_INITIAL);
+            if(m_state != ES_INITIAL)
+            {
+                assert(false);
+                return false;
+            }
         }
-        return true;
+
+        if(m_state == ES_PENDING)
+        {
+            assert(false);
+            return false;
+        }
+
+        return base_t::begin(_flags);
+    }
+
+    virtual bool reset(uint32_t _flags)
+    {
+        if (!canReset())
+        {
+            assert(false);
+            return false;
+        }
+        return base_t::reset(_flags);
     }
 
     uint32_t getQueueFamilyIndex() const { return m_cmdpool->getQueueFamilyIndex(); }
@@ -135,6 +168,8 @@ class IGPUCommandBuffer :
     }
 
 protected:
+    friend class IGPUQueue;
+
     IGPUCommandBuffer(core::smart_refctd_ptr<const ILogicalDevice>&& dev, E_LEVEL lvl, IGPUCommandPool* _cmdpool) : base_t(lvl), IBackendObject(std::move(dev)), m_cmdpool(_cmdpool)
     {
     }

include/nbl/video/IGPUCommandPool.h

@@ -3,6 +3,7 @@
 
 
 #include "nbl/core/IReferenceCounted.h"
+#include "nbl/core/util/bitflag.h"
 
 #include "nbl/video/decl/IBackendObject.h"
 
@@ -21,15 +22,15 @@ class IGPUCommandPool : public core::IReferenceCounted, public IBackendObject
             ECF_PROTECTED_BIT = 0x04
         };
 
-        IGPUCommandPool(core::smart_refctd_ptr<const ILogicalDevice>&& dev, E_CREATE_FLAGS _flags, uint32_t _familyIx) : IBackendObject(std::move(dev)), m_flags(_flags), m_familyIx(_familyIx) {}
+        IGPUCommandPool(core::smart_refctd_ptr<const ILogicalDevice>&& dev, core::bitflag<E_CREATE_FLAGS> _flags, uint32_t _familyIx) : IBackendObject(std::move(dev)), m_flags(_flags), m_familyIx(_familyIx) {}
 
-        E_CREATE_FLAGS getCreationFlags() const { return m_flags; }
+        core::bitflag<E_CREATE_FLAGS> getCreationFlags() const { return m_flags; }
         uint32_t getQueueFamilyIndex() const { return m_familyIx; }
 
     protected:
         virtual ~IGPUCommandPool() = default;
 
-        E_CREATE_FLAGS m_flags;
+        core::bitflag<E_CREATE_FLAGS> m_flags;
         uint32_t m_familyIx;
 };
 

include/nbl/video/IGPUQueue.h

@@ -63,6 +63,47 @@ class IGPUQueue : public core::Interface, public core::Unmovable
 
         inline constexpr static float DEFAULT_QUEUE_PRIORITY = 1.f;
 
+    protected:
+    
+    inline bool markCommandBuffersAsPending(uint32_t _count, const SSubmitInfo* _submits)
+    {
+        if(_submits == nullptr)
+            return false;
+        for (uint32_t i = 0u; i < _count; ++i)
+        {
+            auto& submit = _submits[i];
+            for (uint32_t j = 0u; j < submit.commandBufferCount; ++j)
+            {
+                auto& cmdbuf = submit.commandBuffers[j];
+                if(cmdbuf == nullptr)
+                    return false;
+                submit.commandBuffers[j]->setState(IGPUCommandBuffer::ES_PENDING);
+            }
+        }
+    }
+    
+    inline bool markCommandBuffersAsDone(uint32_t _count, const SSubmitInfo* _submits)
+    {
+        if(_submits == nullptr)
+            return false;
+        for (uint32_t i = 0u; i < _count; ++i)
+        {
+            auto& submit = _submits[i];
+            for (uint32_t j = 0u; j < submit.commandBufferCount; ++j)
+            {
+                auto& cmdbuf = submit.commandBuffers[j];
+                if(cmdbuf == nullptr)
+                    return false;
+
+                if(cmdbuf->isResettable())
+                    cmdbuf->setState(IGPUCommandBuffer::ES_INVALID);
+                else
+                    cmdbuf->setState(IGPUCommandBuffer::ES_EXECUTABLE);
+            }
+        }
+        return true;
+    }
+
     protected:
         const uint32_t m_familyIndex;
         const E_CREATE_FLAGS m_flags;
@@ -72,11 +113,17 @@ class IGPUQueue : public core::Interface, public core::Unmovable
 
 inline bool IGPUQueue::submit(uint32_t _count, const SSubmitInfo* _submits, IGPUFence* _fence)
 {
+    if(_submits == nullptr)
+        return false;
+
     for (uint32_t i = 0u; i < _count; ++i)
     {
         auto& submit = _submits[i];
         for (uint32_t j = 0u; j < submit.commandBufferCount; ++j)
         {
+            if(submit.commandBuffers[j] == nullptr)
+                return false;
+
             assert(submit.commandBuffers[j]->getLevel() == IGPUCommandBuffer::EL_PRIMARY);
             assert(submit.commandBuffers[j]->getState() == IGPUCommandBuffer::ES_EXECUTABLE);
 

include/nbl/video/utilities/IUtilities.h

@@ -295,7 +295,7 @@ namespace nbl::video
             const uint32_t alignment = static_cast<uint32_t>(limits.nonCoherentAtomSize);
 
             auto* cmdpool = cmdbuf->getPool();
-            assert(cmdpool->getCreationFlags() & IGPUCommandPool::ECF_RESET_COMMAND_BUFFER_BIT);
+            assert(cmdbuf->isResettable());
             assert(cmdpool->getQueueFamilyIndex() == queue->getFamilyIndex());
 
             // no pipeline barriers necessary because write and optional flush happens before submit, and memory allocation is reclaimed after fence signal

src/nbl/video/COpenGLCommandBuffer.cpp

@@ -112,14 +112,12 @@ COpenGLCommandBuffer::~COpenGLCommandBuffer()
 
     bool COpenGLCommandBuffer::reset(uint32_t _flags)
     {
-        if (!(m_cmdpool->getCreationFlags() & IGPUCommandPool::ECF_RESET_COMMAND_BUFFER_BIT))
+        if (!IGPUCommandBuffer::canReset())
             return false;
 
         freeSpaceInCmdPool();
         m_commands.clear();
-        IGPUCommandBuffer::reset(_flags);
-
-        return true;
+        return IGPUCommandBuffer::reset(_flags);
     }
 
     void COpenGLCommandBuffer::copyBufferToImage(const SCmd<impl::ECT_COPY_BUFFER_TO_IMAGE>& c, IOpenGL_FunctionTable* gl, SOpenGLContextLocalCache* ctxlocal, uint32_t ctxid)

src/nbl/video/COpenGLCommandBuffer.h

@@ -555,11 +555,12 @@ class COpenGLCommandBuffer final : public IGPUCommandBuffer
 
     COpenGLCommandBuffer(core::smart_refctd_ptr<const ILogicalDevice>&& dev, E_LEVEL lvl, IGPUCommandPool* _cmdpool, system::logger_opt_smart_ptr&& logger, const COpenGLFeatureMap* _features);
 
-    inline void begin(uint32_t _flags) override final
+    inline bool begin(uint32_t _flags) override final
     {
-        IGPUCommandBuffer::begin(_flags);
         reset(_flags);
+        return IGPUCommandBuffer::begin(_flags);
     }
+
     bool reset(uint32_t _flags) override final;
 
 

src/nbl/video/COpenGL_Queue.h

@@ -321,6 +321,8 @@ class COpenGL_Queue final : public IGPUQueue
         {
             if (!IGPUQueue::submit(_count, _submits, _fence))
                 return false;
+            if(!IGPUQueue::markCommandBuffersAsPending(_count, _submits))
+                return false;
 
             core::smart_refctd_ptr<COpenGLSync> sync;
             for (uint32_t i = 0u; i < _count; ++i)
@@ -383,7 +385,9 @@ class COpenGL_Queue final : public IGPUQueue
                 COpenGLFence* glfence = static_cast<COpenGLFence*>(_fence);
                 glfence->associateGLSync(std::move(sync)); // associate sync used for signal semaphores in last submit
             }
-
+            
+            if(!IGPUQueue::markCommandBuffersAsDone(_count, _submits))
+                return false;
             return true;
         }
 

src/nbl/video/CVulkanCommandBuffer.h

@@ -76,32 +76,52 @@ class CVulkanCommandBuffer : public IGPUCommandBuffer
 
         const auto* vk = static_cast<const CVulkanLogicalDevice*>(getOriginDevice())->getFunctionTable();
         VkResult retval = vk->vk.vkBeginCommandBuffer(m_cmdbuf, &beginInfo);
-        assert(retval == VK_SUCCESS);
-        IGPUCommandBuffer::begin(recordingFlags);
+        if(retval == VK_SUCCESS)
+        {
+            return IGPUCommandBuffer::begin(recordingFlags);
+        }
+        else
+        {
+            assert(false);
+            return false;
+        }
+        
     }
 
     // API needs to changed, vkEndCommandBuffer can fail
-    void end() override
+    bool end() override
     {
         const auto* vk = static_cast<const CVulkanLogicalDevice*>(getOriginDevice())->getFunctionTable();
         VkResult retval = vk->vk.vkEndCommandBuffer(m_cmdbuf);
-        assert(retval == VK_SUCCESS);
-        IGPUCommandBuffer::end();
+        if(retval == VK_SUCCESS)
+        {
+            return IGPUCommandBuffer::end();
+        }
+        else
+        {
+            assert(false);
+            return false;
+        }
     }
 
     bool reset(uint32_t _flags) override
     {
+        if(!IGPUCommandBuffer::canReset())
+            return false;
+
         freeSpaceInCmdPool();
 
         const auto* vk = static_cast<const CVulkanLogicalDevice*>(getOriginDevice())->getFunctionTable();
 
         if (vk->vk.vkResetCommandBuffer(m_cmdbuf, static_cast<VkCommandBufferResetFlags>(_flags)) == VK_SUCCESS)
         {
-            IGPUCommandBuffer::reset(_flags);
-            return true;
+            return IGPUCommandBuffer::reset(_flags);
         }
         else
+        {
+            assert(false);
             return false;
+        }
     }
 
     virtual bool bindIndexBuffer(const buffer_t* buffer, size_t offset, asset::E_INDEX_TYPE indexType) override

src/nbl/video/CVulkanQueue.cpp

@@ -10,6 +10,12 @@ namespace nbl::video
 
 bool CVulkanQueue::submit(uint32_t _count, const SSubmitInfo* _submits, IGPUFence* _fence)
 {
+    if (!IGPUQueue::submit(_count, _submits, _fence))
+        return false;
+
+    if(!IGPUQueue::markCommandBuffersAsPending(_count, _submits))
+        return false;
+
     auto* vk = static_cast<const CVulkanLogicalDevice*>(m_originDevice)->getFunctionTable();
 
     uint32_t waitSemCnt = 0u;
@@ -98,7 +104,11 @@ bool CVulkanQueue::submit(uint32_t _count, const SSubmitInfo* _submits, IGPUFenc
 
     VkFence fence = _fence ? static_cast<CVulkanFence*>(_fence)->getInternalObject() : VK_NULL_HANDLE;
     if (vk->vk.vkQueueSubmit(m_vkQueue, _count, submits, fence) == VK_SUCCESS)
+    {
+        if(!IGPUQueue::markCommandBuffersAsDone(_count, _submits))
+            return false;
         return true;
+    }
 
     return false;
 }