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

video: remove *_BUFFER_DYNAMIC descriptors and make bindings always have the UPDATE_AFTER_BIND_BIT flag #736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions include/nbl/asset/IDescriptor.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ class IDescriptor : public virtual core::IReferenceCounted
ET_STORAGE_TEXEL_BUFFER,
ET_UNIFORM_BUFFER,
ET_STORAGE_BUFFER,
ET_UNIFORM_BUFFER_DYNAMIC,
ET_STORAGE_BUFFER_DYNAMIC,
ET_INPUT_ATTACHMENT,
// Provided by VK_KHR_acceleration_structure
ET_ACCELERATION_STRUCTURE,
Expand Down Expand Up @@ -63,8 +61,6 @@ class IDescriptor : public virtual core::IReferenceCounted
return EC_BUFFER_VIEW;
case E_TYPE::ET_UNIFORM_BUFFER:
case E_TYPE::ET_STORAGE_BUFFER:
case E_TYPE::ET_UNIFORM_BUFFER_DYNAMIC:
case E_TYPE::ET_STORAGE_BUFFER_DYNAMIC:
return EC_BUFFER;
case E_TYPE::ET_ACCELERATION_STRUCTURE:
return EC_ACCELERATION_STRUCTURE;
Expand Down
9 changes: 0 additions & 9 deletions include/nbl/video/CVulkanCommon.h
Original file line number Diff line number Diff line change
Expand Up @@ -1013,10 +1013,6 @@ inline constexpr VkDescriptorType getVkDescriptorTypeFromDescriptorType(const as
return VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
case asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER:
return VK_DESCRIPTOR_TYPE_STORAGE_BUFFER;
case asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER_DYNAMIC:
return VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER_DYNAMIC;
case asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER_DYNAMIC:
return VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC;
case asset::IDescriptor::E_TYPE::ET_INPUT_ATTACHMENT:
return VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT;
case asset::IDescriptor::E_TYPE::ET_ACCELERATION_STRUCTURE:
Expand All @@ -1029,11 +1025,6 @@ inline constexpr VkDescriptorType getVkDescriptorTypeFromDescriptorType(const as

inline VkDescriptorBindingFlagBits getVkDescriptorBindingFlagsFrom(const core::bitflag<IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS> flags)
{
// Wait for C++23
//static_assert(std::to_underlying(IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT)==VK_DESCRIPTOR_BINDING_UPDATE_AFTER_BIND_BIT);
//static_assert(IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT==VK_DESCRIPTOR_BINDING_UPDATE_UNUSED_WHILE_PENDING_BIT);
//static_assert(IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT==VK_DESCRIPTOR_BINDING_PARTIALLY_BOUND_BIT);
//static_assert(IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT==VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT);
return static_cast<VkDescriptorBindingFlagBits>(flags.value);
}

Expand Down
7 changes: 0 additions & 7 deletions include/nbl/video/IDescriptorPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ class NBL_API2 IDescriptorPool : public IBackendObject
{
ECF_NONE = 0x00u,
ECF_FREE_DESCRIPTOR_SET_BIT = 0x01,
ECF_UPDATE_AFTER_BIND_BIT = 0x02,
//ECF_HOST_ONLY_BIT_VALVE = 0x04
};

Expand Down Expand Up @@ -129,12 +128,6 @@ class NBL_API2 IDescriptorPool : public IBackendObject
case asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER:
baseAddress = reinterpret_cast<core::smart_refctd_ptr<asset::IDescriptor>*>(m_UBO_SSBOStorage.get()) + m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER)];
break;
case asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER_DYNAMIC:
baseAddress = reinterpret_cast<core::smart_refctd_ptr<asset::IDescriptor>*>(m_UBO_SSBOStorage.get()) + (m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER)] + m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER)]);
break;
case asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER_DYNAMIC:
baseAddress = reinterpret_cast<core::smart_refctd_ptr<asset::IDescriptor>*>(m_UBO_SSBOStorage.get()) + (m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER)] + m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER)] + m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER_DYNAMIC)]);
break;
case asset::IDescriptor::E_TYPE::ET_INPUT_ATTACHMENT:
baseAddress = reinterpret_cast<core::smart_refctd_ptr<asset::IDescriptor>*>(m_storageImageStorage.get()) + m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_STORAGE_IMAGE)];
break;
Expand Down
13 changes: 1 addition & 12 deletions include/nbl/video/IGPUCommandBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,6 @@ class NBL_API2 IGPUCommandBuffer : public IBackendObject
// Vulkan: const VkCommandBuffer*
virtual const void* getNativeHandle() const = 0;

inline const core::unordered_map<const IGPUDescriptorSet*, uint64_t>& getBoundDescriptorSetsRecord() const { return m_boundDescriptorSetsRecord; }

protected:
friend class IQueue;

Expand Down Expand Up @@ -590,8 +588,7 @@ class NBL_API2 IGPUCommandBuffer : public IBackendObject
virtual bool bindGraphicsPipeline_impl(const IGPUGraphicsPipeline* const pipeline) = 0;
virtual bool bindDescriptorSets_impl(
const asset::E_PIPELINE_BIND_POINT pipelineBindPoint, const IGPUPipelineLayout* const layout,
const uint32_t firstSet, const uint32_t descriptorSetCount, const IGPUDescriptorSet* const* const pDescriptorSets,
const uint32_t dynamicOffsetCount = 0u, const uint32_t* const dynamicOffsets = nullptr
const uint32_t firstSet, const uint32_t descriptorSetCount, const IGPUDescriptorSet* const* const pDescriptorSets
) = 0;
virtual bool pushConstants_impl(const IGPUPipelineLayout* const layout, const core::bitflag<IGPUShader::E_SHADER_STAGE> stageFlags, const uint32_t offset, const uint32_t size, const void* const pValues) = 0;
virtual bool bindVertexBuffers_impl(const uint32_t firstBinding, const uint32_t bindingCount, const asset::SBufferBinding<const IGPUBuffer>* const pBindings) = 0;
Expand Down Expand Up @@ -653,8 +650,6 @@ class NBL_API2 IGPUCommandBuffer : public IBackendObject
m_resetCheckedStamp = m_cmdpool->getResetCounter();
m_state = STATE::INITIAL;

m_boundDescriptorSetsRecord.clear();

m_commandList.head = nullptr;
m_commandList.tail = nullptr;

Expand All @@ -666,7 +661,6 @@ class NBL_API2 IGPUCommandBuffer : public IBackendObject
inline void releaseResourcesBackToPool()
{
deleteCommandList();
m_boundDescriptorSetsRecord.clear();
releaseResourcesBackToPool_impl();
}

Expand Down Expand Up @@ -777,11 +771,6 @@ class NBL_API2 IGPUCommandBuffer : public IBackendObject

template<typename IndirectCommand> requires nbl::is_any_of_v<IndirectCommand, hlsl::DrawArraysIndirectCommand_t, hlsl::DrawElementsIndirectCommand_t>
bool invalidDrawIndirectCount(const asset::SBufferBinding<const IGPUBuffer>& indirectBinding, const asset::SBufferBinding<const IGPUBuffer>& countBinding, const uint32_t maxDrawCount, const uint32_t stride);

// This bound descriptor set record doesn't include the descriptor sets whose layout has _any_ one of its bindings
// created with IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT
// or IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_UNUSED_WHILE_PENDING_BIT.
core::unordered_map<const IGPUDescriptorSet*,uint64_t> m_boundDescriptorSetsRecord;

IGPUCommandPool::CCommandSegmentListPool::SCommandSegmentList m_commandList = {};

Expand Down
9 changes: 2 additions & 7 deletions include/nbl/video/IGPUDescriptorSetLayout.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,18 @@ class IGPUDescriptorSetLayout : public asset::IDescriptorSetLayout<IGPUSampler>,
using base_t = asset::IDescriptorSetLayout<IGPUSampler>;

public:
inline bool needUpdateAfterBindPool() const {return m_hasUpdateAfterBindBindings;}

inline bool versionChangeInvalidatesCommandBuffer() const { return m_versionChangeInvalidatesCommandBuffer; }

static inline bool writeIncrementsVersion(const core::bitflag<SBinding::E_CREATE_FLAGS> bindingCreateFlags) {
return not (bindingCreateFlags.hasFlags(SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT) or bindingCreateFlags.hasFlags(SBinding::E_CREATE_FLAGS::ECF_UPDATE_UNUSED_WHILE_PENDING_BIT));
return !bindingCreateFlags.hasFlags(SBinding::E_CREATE_FLAGS::ECF_UPDATE_UNUSED_WHILE_PENDING_BIT);
}

protected:
inline IGPUDescriptorSetLayout(core::smart_refctd_ptr<const ILogicalDevice>&& dev, const std::span<const SBinding> _bindings) : base_t(_bindings), IBackendObject(std::move(dev))
{
for (const auto& binding : _bindings)
{
if (binding.createFlags.hasFlags(SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT))
m_hasUpdateAfterBindBindings = true;
if (not (binding.createFlags.hasFlags(SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT) or binding.createFlags.hasFlags(SBinding::E_CREATE_FLAGS::ECF_UPDATE_UNUSED_WHILE_PENDING_BIT)))
if (!binding.createFlags.hasFlags(SBinding::E_CREATE_FLAGS::ECF_UPDATE_UNUSED_WHILE_PENDING_BIT))
{
m_versionChangeInvalidatesCommandBuffer = true;
break;
Expand All @@ -49,7 +45,6 @@ class IGPUDescriptorSetLayout : public asset::IDescriptorSetLayout<IGPUSampler>,

virtual ~IGPUDescriptorSetLayout() = default;

bool m_hasUpdateAfterBindBindings = false;
bool m_isPushDescLayout = false;
bool m_versionChangeInvalidatesCommandBuffer = false;
};
Expand Down
3 changes: 1 addition & 2 deletions include/nbl/video/alloc/SubAllocatedDescriptorSet.h
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
auto flags = redirect.getCreateFlags(storageIndex);

// Only bindings with these flags will be allocatable
if (flags.hasFlags(core::bitflag(IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT)
| IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_UNUSED_WHILE_PENDING_BIT
if (flags.hasFlags(IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_UNUSED_WHILE_PENDING_BIT
| IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_PARTIALLY_BOUND_BIT))
{
auto reservedSize = AddressAllocator::reserved_size(MaxDescriptorSetAllocationAlignment, static_cast<size_type>(count), MinDescriptorSetAllocationSize);
Expand Down
4 changes: 2 additions & 2 deletions include/nbl/video/utilities/IGPUObjectFromAssetConverter.h
Original file line number Diff line number Diff line change
Expand Up @@ -534,9 +534,9 @@ inline created_gpu_object_array<asset::ICPUDescriptorSet> IGPUObjectFromAssetCon
if (isBufferDesc(type))
{
auto cpuBuf = static_cast<asset::ICPUBuffer*>(descriptor);
if (type == asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER || type == asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER_DYNAMIC)
if (type == asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER)
cpuBuf->addUsageFlags(asset::IBuffer::EUF_UNIFORM_BUFFER_BIT);
else if (type == asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER || type == asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER_DYNAMIC)
else if (type == asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER)
cpuBuf->addUsageFlags(asset::IBuffer::EUF_STORAGE_BUFFER_BIT);
cpuBuffers.push_back(cpuBuf);
}
Expand Down
17 changes: 2 additions & 15 deletions src/nbl/video/CVulkanCommandBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -397,27 +397,19 @@ bool CVulkanCommandBuffer::bindGraphicsPipeline_impl(const IGPUGraphicsPipeline*
return true;
}

bool CVulkanCommandBuffer::bindDescriptorSets_impl(const asset::E_PIPELINE_BIND_POINT pipelineBindPoint, const IGPUPipelineLayout* const layout, const uint32_t firstSet, const uint32_t descriptorSetCount, const IGPUDescriptorSet* const* const pDescriptorSets, const uint32_t dynamicOffsetCount, const uint32_t* const dynamicOffsets)
bool CVulkanCommandBuffer::bindDescriptorSets_impl(const asset::E_PIPELINE_BIND_POINT pipelineBindPoint, const IGPUPipelineLayout* const layout, const uint32_t firstSet, const uint32_t descriptorSetCount, const IGPUDescriptorSet* const* const pDescriptorSets)
{
VkDescriptorSet vk_descriptorSets[IGPUPipelineLayout::DESCRIPTOR_SET_COUNT] = {};
uint32_t dynamicOffsetCountPerSet[IGPUPipelineLayout::DESCRIPTOR_SET_COUNT] = { 0u };

// We allow null descriptor sets in our bind function to skip a certain set number we don't use
// Will bind [first, last) with one call
for (uint32_t i = 0u; i<descriptorSetCount; ++i)
if (pDescriptorSets[i])
{
vk_descriptorSets[i] = static_cast<const CVulkanDescriptorSet*>(pDescriptorSets[i])->getInternalObject();
// count dynamic offsets per set, if there are any
if (dynamicOffsets)
{
dynamicOffsetCountPerSet[i] += pDescriptorSets[i]->getLayout()->getDescriptorRedirect(asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER_DYNAMIC).getTotalCount();
dynamicOffsetCountPerSet[i] += pDescriptorSets[i]->getLayout()->getDescriptorRedirect(asset::IDescriptor::E_TYPE::ET_UNIFORM_BUFFER_DYNAMIC).getTotalCount();
}
}

uint32_t bindCallsCount = 0u;
uint32_t dynamicOffsetsBindOffset = 0u;
uint32_t first = ~0u;
uint32_t last = ~0u;
const VkPipelineLayout vk_pipelineLayout = static_cast<const CVulkanPipelineLayout*>(layout)->getInternalObject();
Expand All @@ -432,16 +424,11 @@ bool CVulkanCommandBuffer::bindDescriptorSets_impl(const asset::E_PIPELINE_BIND_
const auto next = i+1;
if (next>=descriptorSetCount || !pDescriptorSets[next])
{
uint32_t dynamicOffsetCount = 0u;
for (auto setIndex=first; setIndex<last; ++setIndex)
dynamicOffsetCount += dynamicOffsetCountPerSet[setIndex];

getFunctionTable().vkCmdBindDescriptorSets(
m_cmdbuf,static_cast<VkPipelineBindPoint>(pipelineBindPoint),vk_pipelineLayout,
firstSet+first, last-first, vk_descriptorSets+first,
dynamicOffsetCount, dynamicOffsets+dynamicOffsetsBindOffset
0, nullptr
);
dynamicOffsetsBindOffset += dynamicOffsetCount;

first = last;
++bindCallsCount;
Expand Down
2 changes: 1 addition & 1 deletion src/nbl/video/CVulkanCommandBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ class CVulkanCommandBuffer final : public IGPUCommandBuffer

bool bindComputePipeline_impl(const IGPUComputePipeline* const pipeline) override;
bool bindGraphicsPipeline_impl(const IGPUGraphicsPipeline* const pipeline) override;
bool bindDescriptorSets_impl(const asset::E_PIPELINE_BIND_POINT pipelineBindPoint, const IGPUPipelineLayout* const layout, const uint32_t firstSet, const uint32_t descriptorSetCount, const IGPUDescriptorSet* const* const pDescriptorSets, const uint32_t dynamicOffsetCount = 0u, const uint32_t* const dynamicOffsets = nullptr) override;
bool bindDescriptorSets_impl(const asset::E_PIPELINE_BIND_POINT pipelineBindPoint, const IGPUPipelineLayout* const layout, const uint32_t firstSet, const uint32_t descriptorSetCount, const IGPUDescriptorSet* const* const pDescriptorSets) override;
bool pushConstants_impl(const IGPUPipelineLayout* const layout, const core::bitflag<IGPUShader::E_SHADER_STAGE> stageFlags, const uint32_t offset, const uint32_t size, const void* const pValues) override;
bool bindVertexBuffers_impl(const uint32_t firstBinding, const uint32_t bindingCount, const asset::SBufferBinding<const IGPUBuffer>* const pBindings) override;
bool bindIndexBuffer_impl(const asset::SBufferBinding<const IGPUBuffer>& binding, const asset::E_INDEX_TYPE indexType) override;
Expand Down
9 changes: 2 additions & 7 deletions src/nbl/video/CVulkanLogicalDevice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,6 @@ core::smart_refctd_ptr<IGPUDescriptorSetLayout> CVulkanLogicalDevice::createDesc
vk_dsLayoutBindings.reserve(bindings.size());
vk_bindingFlags.reserve(bindings.size());

bool updateAfterBindFound = false;
for (const auto& binding : bindings)
{
auto& vkDescSetLayoutBinding = vk_dsLayoutBindings.emplace_back();
Expand All @@ -564,18 +563,14 @@ core::smart_refctd_ptr<IGPUDescriptorSetLayout> CVulkanLogicalDevice::createDesc
vkDescSetLayoutBinding.pImmutableSamplers = vk_samplers.data()+samplerOffset;
}

if (binding.createFlags.hasFlags(IGPUDescriptorSetLayout::SBinding::E_CREATE_FLAGS::ECF_UPDATE_AFTER_BIND_BIT))
updateAfterBindFound = true;
vk_bindingFlags.emplace_back() = getVkDescriptorBindingFlagsFrom(binding.createFlags);
}

VkDescriptorSetLayoutBindingFlagsCreateInfo vk_bindingFlagsInfo = { VK_STRUCTURE_TYPE_DESCRIPTOR_SET_LAYOUT_BINDING_FLAGS_CREATE_INFO, nullptr };
VkDescriptorSetLayoutCreateInfo vk_createInfo = { VK_STRUCTURE_TYPE_DESCRIPTOR_SET_LAYOUT_CREATE_INFO, &vk_bindingFlagsInfo };
// Todo(achal): I would need to create a IDescriptorSetLayout::SCreationParams for this
// Answer: We don't actually support any extensions/features that would necessitate exposing any other flag than update_after_bind
vk_createInfo.flags = 0;
if (updateAfterBindFound)
vk_createInfo.flags |= VK_DESCRIPTOR_SET_LAYOUT_CREATE_UPDATE_AFTER_BIND_POOL_BIT;
vk_createInfo.flags = VK_DESCRIPTOR_SET_LAYOUT_CREATE_UPDATE_AFTER_BIND_POOL_BIT;
vk_createInfo.bindingCount = vk_bindingFlagsInfo.bindingCount = vk_dsLayoutBindings.size();
vk_createInfo.pBindings = vk_dsLayoutBindings.data();
vk_bindingFlagsInfo.pBindingFlags = vk_bindingFlags.data();
Expand Down Expand Up @@ -647,7 +642,7 @@ core::smart_refctd_ptr<IDescriptorPool> CVulkanLogicalDevice::createDescriptorPo

VkDescriptorPoolCreateInfo vk_createInfo = { VK_STRUCTURE_TYPE_DESCRIPTOR_POOL_CREATE_INFO };
vk_createInfo.pNext = nullptr; // no pNext of interest so far
vk_createInfo.flags = static_cast<VkDescriptorPoolCreateFlags>(createInfo.flags.value);
vk_createInfo.flags = VK_DESCRIPTOR_POOL_CREATE_UPDATE_AFTER_BIND_BIT | static_cast<VkDescriptorPoolCreateFlags>(createInfo.flags.value);
vk_createInfo.maxSets = createInfo.maxSets;
vk_createInfo.poolSizeCount = poolSizeCount;
vk_createInfo.pPoolSizes = poolSizes;
Expand Down
Loading