Skip to content

Commit ddb0640

Browse files
achalpandeyyachalpandeyy
committed
Make IDescriptorPool hold weak pointers to allocated DS and change the way we destroy DS and the way pool is reset to avoid doing double frees.
1 parent 1cd208b commit ddb0640

11 files changed

+223
-105
lines changed

‎include/nbl/asset/utils/ICPUVirtualTexture.h

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -536,30 +536,35 @@ class ICPUVirtualTexture final : public IVirtualTexture<ICPUImageView, ICPUSampl
536536
if (pgtInfos.empty())
537537
return false;
538538

539-
assert(pgtInfos.size() == 1ull);
539+
if (pgtInfos.size() != 1ull)
540+
return false;
541+
540542
auto& info = pgtInfos.begin()[0];
541543
info.info.image.imageLayout = IImage::EL_UNDEFINED;
542544
info.info.image.sampler = nullptr;
543545
info.desc = core::smart_refctd_ptr<ICPUImageView>(getPageTableView());
544546
}
545547

546-
auto updateSamplersBinding = [&](const uint32_t binding, const auto& views)
548+
auto updateSamplersBinding = [&](const uint32_t binding, const auto& views) -> bool
547549
{
548550
auto infos = _dstSet->getDescriptorInfos(binding, IDescriptor::E_TYPE::ET_COMBINED_IMAGE_SAMPLER);
549551

552+
if (infos.size() < views.size())
553+
return false;
554+
550555
for (uint32_t i = 0; i < infos.size(); ++i)
551556
{
552557
auto& info = infos.begin()[i];
553558

554-
info.info.image.imageLayout = IImage::EL_UNDEFINED;
559+
info.info.image.imageLayout = IImage::EL_SHADER_READ_ONLY_OPTIMAL;
555560
info.info.image.sampler = nullptr;
556561
info.desc = views.begin()[i].view;
557562
}
563+
564+
return true;
558565
};
559566

560-
updateSamplersBinding(_fsamplersBinding, getFloatViews());
561-
updateSamplersBinding(_isamplersBinding, getIntViews());
562-
updateSamplersBinding(_usamplersBinding, getUintViews());
567+
return updateSamplersBinding(_fsamplersBinding, getFloatViews()) && updateSamplersBinding(_isamplersBinding, getIntViews()) && updateSamplersBinding(_usamplersBinding, getUintViews());
563568
}
564569

565570
protected:

‎include/nbl/core/alloc/IteratablePoolAddressAllocator.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,9 @@ template<typename _size_type>
2222
class IteratablePoolAddressAllocator : protected PoolAddressAllocator<_size_type>
2323
{
2424
using Base = PoolAddressAllocator<_size_type>;
25-
protected:
25+
public:
2626
inline _size_type* begin() { return &Base::getFreeStack(Base::freeStackCtr); }
27+
protected:
2728
inline _size_type& getIteratorOffset(_size_type i) {return reinterpret_cast<_size_type*>(Base::reservedSpace)[Base::blockCount+i];}
2829
inline const _size_type& getIteratorOffset(_size_type i) const {return reinterpret_cast<const _size_type*>(Base::reservedSpace)[Base::blockCount+i];}
2930

‎include/nbl/video/IDescriptorPool.h

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,16 @@ class IDescriptorPool : public core::IReferenceCounted, public IBackendObject
5959
return nullptr;
6060
}
6161

62-
bool createDescriptorSets(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, core::smart_refctd_ptr<IGPUDescriptorSet>* output);
62+
uint32_t createDescriptorSets(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, core::smart_refctd_ptr<IGPUDescriptorSet>* output);
6363

6464
bool reset();
6565

6666
inline uint32_t getCapacity() const { return m_creationParameters.maxSets; }
67+
inline bool allowsFreeing() const { return m_creationParameters.flags.hasFlags(ECF_FREE_DESCRIPTOR_SET_BIT); }
6768

6869
protected:
6970
explicit IDescriptorPool(core::smart_refctd_ptr<const ILogicalDevice>&& dev, SCreateInfo&& createInfo)
70-
: IBackendObject(std::move(dev)), m_creationParameters(std::move(createInfo)), m_version(0u)
71+
: IBackendObject(std::move(dev)), m_creationParameters(std::move(createInfo))
7172
{
7273
for (auto i = 0; i < static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_COUNT); ++i)
7374
m_descriptorAllocators[i] = std::make_unique<allocator_state_t>(m_creationParameters.maxDescriptorCount[i], m_creationParameters.flags.hasFlags(ECF_FREE_DESCRIPTOR_SET_BIT));
@@ -82,11 +83,27 @@ class IDescriptorPool : public core::IReferenceCounted, public IBackendObject
8283
m_UBO_SSBOStorage = std::make_unique<core::StorageTrivializer<core::smart_refctd_ptr<IGPUBuffer>>[]>(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)] + m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER_DYNAMIC)]);
8384
m_UTB_STBStorage = std::make_unique<core::StorageTrivializer<core::smart_refctd_ptr<IGPUBufferView>>[]>(m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_UNIFORM_TEXEL_BUFFER)] + m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_STORAGE_BUFFER_DYNAMIC)]);
8485
m_accelerationStructureStorage = std::make_unique<core::StorageTrivializer<core::smart_refctd_ptr<IGPUAccelerationStructure>>[]>(m_creationParameters.maxDescriptorCount[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_ACCELERATION_STRUCTURE)]);
86+
87+
m_allocatedDescriptorSets = std::make_unique<IGPUDescriptorSet* []>(m_creationParameters.maxSets);
88+
std::fill_n(m_allocatedDescriptorSets.get(), m_creationParameters.maxSets, nullptr);
89+
90+
m_descriptorSetAllocatorReservedSpace = std::make_unique<uint8_t[]>(core::IteratablePoolAddressAllocator<uint32_t>::reserved_size(1, m_creationParameters.maxSets, 1));
91+
m_descriptorSetAllocator = core::IteratablePoolAddressAllocator<uint32_t>(m_descriptorSetAllocatorReservedSpace.get(), 0, 0, 1, m_creationParameters.maxSets, 1);
8592
}
8693

87-
virtual ~IDescriptorPool() {}
94+
virtual ~IDescriptorPool()
95+
{
96+
if (allowsFreeing())
97+
{
98+
assert(m_descriptorSetAllocator.get_allocated_size() == 0);
99+
#ifdef _NBL_DEBUG
100+
for (uint32_t i = 0u; i < m_creationParameters.maxSets; ++i)
101+
assert(m_allocatedDescriptorSets[i] == nullptr);
102+
#endif
103+
}
104+
}
88105

89-
virtual bool createDescriptorSets_impl(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, SDescriptorOffsets* const offsets, core::smart_refctd_ptr<IGPUDescriptorSet>* output) = 0;
106+
virtual bool createDescriptorSets_impl(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, SDescriptorOffsets* const offsets, const uint32_t firstSetOffsetInPool, core::smart_refctd_ptr<IGPUDescriptorSet>* output) = 0;
90107

91108
virtual bool reset_impl() = 0;
92109

@@ -142,16 +159,19 @@ class IDescriptorPool : public core::IReferenceCounted, public IBackendObject
142159
friend class IGPUDescriptorSet;
143160
// Returns the offset into the pool's descriptor storage. These offsets will be combined
144161
// later with base memory addresses to get the actual memory address where we put the core::smart_refctd_ptr<const IDescriptor>.
145-
SDescriptorOffsets allocateDescriptorOffsets(const IGPUDescriptorSetLayout* layout);
162+
bool allocateDescriptorOffsets(SDescriptorOffsets& offsets, const IGPUDescriptorSetLayout* layout);
163+
void freeDescriptorOffsets(SDescriptorOffsets& offsets, const IGPUDescriptorSetLayout* layout);
164+
165+
void deleteSetStorage(IGPUDescriptorSet* set);
146166

147167
struct allocator_state_t
148168
{
149-
allocator_state_t(const uint32_t maxDescriptorCount, const bool allowsFreeing)
169+
allocator_state_t(const uint32_t maxDescriptorCount, const bool useGeneralAllocator)
150170
{
151171
if (maxDescriptorCount == 0)
152172
return;
153173

154-
if (allowsFreeing)
174+
if (useGeneralAllocator)
155175
{
156176
generalAllocatorReservedSpace = std::make_unique<uint8_t[]>(core::GeneralpurposeAddressAllocator<uint32_t>::reserved_size(1u, maxDescriptorCount, 1u));
157177
generalAllocator = core::GeneralpurposeAddressAllocator<uint32_t>(generalAllocatorReservedSpace.get(), 0u, 0u, 1u, maxDescriptorCount, 1u);
@@ -164,17 +184,12 @@ class IDescriptorPool : public core::IReferenceCounted, public IBackendObject
164184

165185
~allocator_state_t() {}
166186

167-
inline uint32_t allocate(const uint32_t count, const bool allowsFreeing)
187+
inline uint32_t allocate(const uint32_t count)
168188
{
169-
if (allowsFreeing)
170-
{
171-
assert(generalAllocatorReservedSpace);
189+
if (generalAllocatorReservedSpace)
172190
return generalAllocator.alloc_addr(count, 1u);
173-
}
174191
else
175-
{
176192
return linearAllocator.alloc_addr(count, 1u);
177-
}
178193
}
179194

180195
inline void free(const uint32_t allocatedOffset, const uint32_t count)
@@ -183,20 +198,20 @@ class IDescriptorPool : public core::IReferenceCounted, public IBackendObject
183198
generalAllocator.free_addr(allocatedOffset, count);
184199
}
185200

186-
inline void reset(const bool allowsFreeing)
201+
inline void reset()
187202
{
188-
if (!allowsFreeing)
189-
linearAllocator.reset();
190-
else
203+
if (generalAllocatorReservedSpace)
191204
generalAllocator.reset();
205+
else
206+
linearAllocator.reset();
192207
}
193208

194-
inline uint32_t getAllocatedDescriptorCount(const bool allowsFreeing) const
209+
inline uint32_t getAllocatedDescriptorCount() const
195210
{
196-
if (!allowsFreeing)
197-
return linearAllocator.get_allocated_size();
198-
else
211+
if (generalAllocatorReservedSpace)
199212
return generalAllocator.get_allocated_size();
213+
else
214+
return linearAllocator.get_allocated_size();
200215
}
201216

202217
union
@@ -209,7 +224,10 @@ class IDescriptorPool : public core::IReferenceCounted, public IBackendObject
209224
std::unique_ptr<allocator_state_t> m_descriptorAllocators[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_COUNT) + 1];
210225

211226
const SCreateInfo m_creationParameters;
212-
std::atomic_uint32_t m_version;
227+
228+
core::IteratablePoolAddressAllocator<uint32_t> m_descriptorSetAllocator;
229+
std::unique_ptr<uint8_t[]> m_descriptorSetAllocatorReservedSpace = nullptr;
230+
std::unique_ptr<IGPUDescriptorSet* []> m_allocatedDescriptorSets = nullptr; // This array might be sparse.
213231

214232
std::unique_ptr<core::StorageTrivializer<core::smart_refctd_ptr<video::IGPUImageView>>[]> m_textureStorage;
215233
std::unique_ptr<core::StorageTrivializer<core::smart_refctd_ptr<video::IGPUSampler>>[]> m_mutableSamplerStorage;

‎include/nbl/video/IGPUDescriptorSet.h

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,18 +52,15 @@ class IGPUDescriptorSet : public asset::IDescriptorSet<const IGPUDescriptorSetLa
5252
uint32_t count;
5353
};
5454

55-
IGPUDescriptorSet(core::smart_refctd_ptr<const IGPUDescriptorSetLayout>&& _layout, core::smart_refctd_ptr<IDescriptorPool>&& pool, IDescriptorPool::SDescriptorOffsets&& offsets);
55+
IGPUDescriptorSet(core::smart_refctd_ptr<const IGPUDescriptorSetLayout>&& _layout, core::smart_refctd_ptr<IDescriptorPool>&& pool, const uint32_t poolOffset, IDescriptorPool::SDescriptorOffsets&& offsets);
5656

5757
inline uint64_t getVersion() const { return m_version.load(); }
5858

5959
inline IDescriptorPool* getPool() const { return m_pool.get(); }
6060

6161
inline bool isZombie() const
6262
{
63-
if (m_pool->m_version.load() > m_parentPoolVersion)
64-
return true;
65-
else
66-
return false;
63+
return (m_pool.get() == nullptr);
6764
}
6865

6966
protected:
@@ -171,8 +168,9 @@ class IGPUDescriptorSet : public asset::IDescriptorSet<const IGPUDescriptorSetLa
171168
inline uint32_t getMutableSamplerStorageOffset() const { return m_descriptorStorageOffsets.data[static_cast<uint32_t>(asset::IDescriptor::E_TYPE::ET_COUNT)]; }
172169

173170
std::atomic_uint64_t m_version;
174-
const uint32_t m_parentPoolVersion;
171+
friend class IDescriptorPool;
175172
core::smart_refctd_ptr<IDescriptorPool> m_pool;
173+
uint32_t m_poolOffset;
176174
IDescriptorPool::SDescriptorOffsets m_descriptorStorageOffsets;
177175
};
178176

‎src/nbl/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,7 @@ set(NBL_VIDEO_SOURCES
309309
${NBL_ROOT_PATH}/src/nbl/video/CVulkanPipelineCache.cpp
310310
${NBL_ROOT_PATH}/src/nbl/video/CVulkanComputePipeline.cpp
311311
${NBL_ROOT_PATH}/src/nbl/video/CVulkanDescriptorPool.cpp
312+
${NBL_ROOT_PATH}/src/nbl/video/CVulkanDescriptorSet.cpp
312313
${NBL_ROOT_PATH}/src/nbl/video/CVulkanMemoryAllocation.cpp
313314
${NBL_ROOT_PATH}/src/nbl/video/CVulkanBufferView.cpp
314315
${NBL_ROOT_PATH}/src/nbl/video/CVulkanLogicalDevice.cpp

‎src/nbl/video/CVulkanDescriptorPool.cpp

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ void CVulkanDescriptorPool::setObjectDebugName(const char* label) const
2626
vkSetDebugUtilsObjectNameEXT(vulkanDevice->getInternalObject(), &nameInfo);
2727
}
2828

29-
bool CVulkanDescriptorPool::createDescriptorSets_impl(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, SDescriptorOffsets* const offsets, core::smart_refctd_ptr<IGPUDescriptorSet>* output)
29+
bool CVulkanDescriptorPool::createDescriptorSets_impl(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, SDescriptorOffsets* const offsets, const uint32_t firstSetOffsetInPool, core::smart_refctd_ptr<IGPUDescriptorSet>* output)
3030
{
3131
VkDescriptorSetAllocateInfo vk_allocateInfo = { VK_STRUCTURE_TYPE_DESCRIPTOR_SET_ALLOCATE_INFO };
3232
vk_allocateInfo.pNext = nullptr; // pNext must be NULL or a pointer to a valid instance of VkDescriptorSetVariableDescriptorCountAllocateInfo
@@ -37,9 +37,7 @@ bool CVulkanDescriptorPool::createDescriptorSets_impl(uint32_t count, const IGPU
3737
core::vector<VkDescriptorSetLayout> vk_dsLayouts(count);
3838
for (uint32_t i = 0; i < count; ++i)
3939
{
40-
if (layouts[i]->getAPIType() != EAT_VULKAN)
41-
return false;
42-
40+
assert(layouts[i]->getAPIType() == EAT_VULKAN);
4341
vk_dsLayouts[i] = IBackendObject::device_compatibility_cast<const CVulkanDescriptorSetLayout*>(layouts[i], getOriginDevice())->getInternalObject();
4442
}
4543

@@ -52,7 +50,7 @@ bool CVulkanDescriptorPool::createDescriptorSets_impl(uint32_t count, const IGPU
5250
if (vk->vk.vkAllocateDescriptorSets(vulkanDevice->getInternalObject(), &vk_allocateInfo, vk_descriptorSets.data()) == VK_SUCCESS)
5351
{
5452
for (uint32_t i = 0; i < count; ++i)
55-
output[i] = core::make_smart_refctd_ptr<CVulkanDescriptorSet>(core::smart_refctd_ptr<const IGPUDescriptorSetLayout>(layouts[i]), core::smart_refctd_ptr<IDescriptorPool>(this), std::move(offsets[i]), vk_descriptorSets[i]);
53+
output[i] = core::make_smart_refctd_ptr<CVulkanDescriptorSet>(core::smart_refctd_ptr<const IGPUDescriptorSetLayout>(layouts[i]), core::smart_refctd_ptr<IDescriptorPool>(this), firstSetOffsetInPool + i, std::move(offsets[i]), vk_descriptorSets[i]);
5654

5755
return true;
5856
}

‎src/nbl/video/CVulkanDescriptorPool.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ class CVulkanDescriptorPool : public IDescriptorPool
2323
void setObjectDebugName(const char* label) const override;
2424

2525
private:
26-
bool createDescriptorSets_impl(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, SDescriptorOffsets *const offsets, core::smart_refctd_ptr<IGPUDescriptorSet>* output) override;
26+
bool createDescriptorSets_impl(uint32_t count, const IGPUDescriptorSetLayout* const* layouts, SDescriptorOffsets *const offsets, const uint32_t firstSetOffsetInPool, core::smart_refctd_ptr<IGPUDescriptorSet>* output) override;
2727
bool reset_impl() override;
2828

2929
VkDescriptorPool m_descriptorPool;

‎src/nbl/video/CVulkanDescriptorSet.cpp

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
#include "nbl/video/CVulkanDescriptorSet.h"
2+
3+
#include "nbl/video/CVulkanLogicalDevice.h"
4+
5+
namespace nbl::video
6+
{
7+
8+
CVulkanDescriptorSet::~CVulkanDescriptorSet()
9+
{
10+
if (!isZombie() && getPool()->allowsFreeing())
11+
{
12+
const CVulkanLogicalDevice* vulkanDevice = static_cast<const CVulkanLogicalDevice*>(getOriginDevice());
13+
auto* vk = vulkanDevice->getFunctionTable();
14+
15+
const auto* vk_dsPool = IBackendObject::device_compatibility_cast<const CVulkanDescriptorPool*>(getPool(), getOriginDevice());
16+
assert(vk_dsPool);
17+
18+
vk->vk.vkFreeDescriptorSets(vulkanDevice->getInternalObject(), vk_dsPool->getInternalObject(), 1u, &m_descriptorSet);
19+
}
20+
}
21+
22+
}

‎src/nbl/video/CVulkanDescriptorSet.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@ class CVulkanDescriptorPool;
1313
class CVulkanDescriptorSet : public IGPUDescriptorSet
1414
{
1515
public:
16-
CVulkanDescriptorSet(core::smart_refctd_ptr<const IGPUDescriptorSetLayout>&& layout, core::smart_refctd_ptr<IDescriptorPool>&& pool, IDescriptorPool::SDescriptorOffsets offsets, VkDescriptorSet descriptorSet)
17-
: IGPUDescriptorSet(std::move(layout), std::move(pool), std::move(offsets)), m_descriptorSet(descriptorSet)
16+
CVulkanDescriptorSet(core::smart_refctd_ptr<const IGPUDescriptorSetLayout>&& layout, core::smart_refctd_ptr<IDescriptorPool>&& pool, const uint32_t poolOffset, IDescriptorPool::SDescriptorOffsets offsets, VkDescriptorSet descriptorSet)
17+
: IGPUDescriptorSet(std::move(layout), std::move(pool), poolOffset, std::move(offsets)), m_descriptorSet(descriptorSet)
1818
{}
1919

20+
~CVulkanDescriptorSet();
21+
2022
inline VkDescriptorSet getInternalObject() const { return m_descriptorSet; }
2123

2224
private:

0 commit comments

Comments
 (0)