Skip to content

Commit 912ed7a

Browse files
deprilula28deprilula28
committed
PR reviews & nullifying descriptors
1 parent bc5b22d commit 912ed7a

File tree

6 files changed

+111
-21
lines changed

6 files changed

+111
-21
lines changed

‎include/nbl/video/ILogicalDevice.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -633,7 +633,7 @@ class NBL_API2 ILogicalDevice : public core::IReferenceCounted, public IDeviceMe
633633
}
634634

635635
// should this be joined together with the existing updateDescriptorSets?
636-
void nullifyDescriptors(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors);
636+
bool nullifyDescriptors(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors, asset::IDescriptor::E_TYPE descriptorType);
637637

638638
//! Renderpasses and Framebuffers
639639
core::smart_refctd_ptr<IGPURenderpass> createRenderpass(const IGPURenderpass::SCreationParams& params);
@@ -853,7 +853,7 @@ class NBL_API2 ILogicalDevice : public core::IReferenceCounted, public IDeviceMe
853853

854854
// Drops refcounted references of the descriptors in these indices for the descriptor lifetime tracking
855855
// If the nullDescriptor device feature is enabled, this would also write a null descriptor to the descriptor set
856-
virtual void nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors) = 0;
856+
virtual void nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors, asset::IDescriptor::E_TYPE descriptorType) = 0;
857857

858858
virtual core::smart_refctd_ptr<IGPURenderpass> createRenderpass_impl(const IGPURenderpass::SCreationParams& params, IGPURenderpass::SCreationParamValidationResult&& validation) = 0;
859859
virtual core::smart_refctd_ptr<IGPUFramebuffer> createFramebuffer_impl(IGPUFramebuffer::SCreationParams&& params) = 0;

‎include/nbl/video/alloc/SubAllocatedDescriptorSet.h‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,12 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
6666
// TODO: Find a workaround for this
6767
inline void operator()()
6868
{
69-
core::vector<IGPUDescriptorSet::SDropDescriptorSet> nulls(m_addresses->size());
70-
auto ptr = nulls.data();
71-
operator()(ptr);
72-
auto size = ptr - nulls.data();
73-
m_composed->m_logicalDevice->nullifyDescriptors({nulls.data(),size_type(size)});
69+
assert(false); // This should not be called, timeline needs to be drained before destructor
70+
// core::vector<IGPUDescriptorSet::SDropDescriptorSet> nulls(m_addresses->size());
71+
// auto ptr = nulls.data();
72+
// operator()(ptr);
73+
// auto size = ptr - nulls.data();
74+
// m_composed->m_logicalDevice->nullifyDescriptors({nulls.data(),size_type(size)});
7475
}
7576

7677
// Takes count of allocations we want to free up as reference, true is returned if
@@ -102,20 +103,31 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
102103
std::unique_ptr<AddressAllocator> addressAllocator = nullptr;
103104
std::unique_ptr<ReservedAllocator> reservedAllocator = nullptr;
104105
size_t reservedSize = 0;
106+
asset::IDescriptor::E_TYPE descriptorType = asset::IDescriptor::E_TYPE::ET_COUNT;
105107

106108
SubAllocDescriptorSetRange(
107109
std::unique_ptr<AddressAllocator>&& inAddressAllocator,
108110
std::unique_ptr<ReservedAllocator>&& inReservedAllocator,
109-
size_t inReservedSize) :
111+
size_t inReservedSize,
112+
asset::IDescriptor::E_TYPE inDescriptorType) :
110113
eventHandler({}), addressAllocator(std::move(inAddressAllocator)),
111-
reservedAllocator(std::move(inReservedAllocator)), reservedSize(inReservedSize) {}
114+
reservedAllocator(std::move(inReservedAllocator)),
115+
reservedSize(inReservedSize),
116+
descriptorType(inDescriptorType) {}
112117
SubAllocDescriptorSetRange() {}
113118

114119
SubAllocDescriptorSetRange& operator=(SubAllocDescriptorSetRange&& other)
115120
{
116121
addressAllocator = std::move(other.addressAllocator);
117122
reservedAllocator = std::move(other.reservedAllocator);
118123
reservedSize = other.reservedSize;
124+
descriptorType = other.descriptorType;
125+
126+
// Nullify other
127+
other.addressAllocator = nullptr;
128+
other.reservedAllocator = nullptr;
129+
other.reservedSize = 0u;
130+
other.descriptorType = asset::IDescriptor::E_TYPE::ET_COUNT;
119131
return *this;
120132
}
121133
};
@@ -164,7 +176,7 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
164176
MinDescriptorSetAllocationSize
165177
));
166178

167-
m_allocatableRanges[binding.data] = SubAllocDescriptorSetRange(std::move(addressAllocator), std::move(reservedAllocator), reservedSize);
179+
m_allocatableRanges[binding.data] = SubAllocDescriptorSetRange(std::move(addressAllocator), std::move(reservedAllocator), reservedSize, descType);
168180
}
169181
}
170182
}
@@ -261,7 +273,7 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
261273
nulls.resize(m_totalDeferredFrees);
262274
auto outNulls = nulls.data();
263275
eventHandler.wait(maxWaitPoint, unallocatedSize, outNulls);
264-
m_logicalDevice->nullifyDescriptors({nulls.data(),outNulls});
276+
m_logicalDevice->nullifyDescriptors({ nulls.data(),outNulls }, range->second.descriptorType);
265277

266278
// always call with the same parameters, otherwise this turns into a mess with the non invalid_address gaps
267279
unallocatedSize = try_multi_allocate(binding,count,outAddresses);
@@ -325,7 +337,9 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
325337
{
326338
core::vector<IGPUDescriptorSet::SDropDescriptorSet> nulls(count);
327339
auto actualEnd = multi_deallocate(nulls.data(), binding, count, addr);
328-
m_logicalDevice->nullifyDescriptors({nulls.data(),actualEnd});
340+
// This is checked to be valid above
341+
auto range = m_allocatableRanges.find(binding);
342+
m_logicalDevice->nullifyDescriptors({nulls.data(),actualEnd}, range->second.descriptorType);
329343
}
330344
}
331345

@@ -340,8 +354,9 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
340354
{
341355
auto& it = m_allocatableRanges[i];
342356
frees += it.eventHandler.poll(outNulls).eventsLeft;
357+
// TODO: this could be optimized to be put outside the loop
358+
m_logicalDevice->nullifyDescriptors({nulls.data(),outNulls}, it.descriptorType);
343359
}
344-
m_logicalDevice->nullifyDescriptors({nulls.data(),outNulls});
345360
return frees;
346361
}
347362
};

‎src/nbl/video/CVulkanLogicalDevice.cpp‎

Lines changed: 64 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -727,11 +727,73 @@ void CVulkanLogicalDevice::updateDescriptorSets_impl(const SUpdateDescriptorSets
727727
m_devf.vk.vkUpdateDescriptorSets(m_vkdev,vk_writeDescriptorSets.size(),vk_writeDescriptorSets.data(),vk_copyDescriptorSets.size(),vk_copyDescriptorSets.data());
728728
}
729729

730-
void CVulkanLogicalDevice::nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors)
730+
void CVulkanLogicalDevice::nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> drops, asset::IDescriptor::E_TYPE descriptorType)
731731
{
732732
if (getEnabledFeatures().nullDescriptor)
733733
{
734-
// TODO: Write null descriptors here
734+
core::vector<VkWriteDescriptorSet> vk_writeDescriptorSets(drops.size(),{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET,nullptr});
735+
core::vector<VkWriteDescriptorSetAccelerationStructureKHR> vk_writeDescriptorSetAS(69u,{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_ACCELERATION_STRUCTURE_KHR,nullptr});
736+
737+
size_t maxSize = 0;
738+
for (auto i = 0; i < drops.size(); i++)
739+
{
740+
const auto& write = drops[i];
741+
maxSize = core::max(maxSize, write.count);
742+
}
743+
size_t descriptorSize;
744+
switch (asset::IDescriptor::GetTypeCategory(descriptorType))
745+
{
746+
case asset::IDescriptor::EC_BUFFER:
747+
descriptorSize = sizeof(VkDescriptorBufferInfo);
748+
break;
749+
case asset::IDescriptor::EC_IMAGE:
750+
descriptorSize = sizeof(VkDescriptorImageInfo);
751+
break;
752+
case asset::IDescriptor::EC_BUFFER_VIEW:
753+
descriptorSize = sizeof(VkBufferView);
754+
break;
755+
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
756+
descriptorSize = sizeof(VkAccelerationStructureKHR);
757+
break;
758+
}
759+
760+
core::vector<uint8_t> nullDescriptors(maxSize * descriptorSize, 0u);
761+
762+
{
763+
auto outWrite = vk_writeDescriptorSets.data();
764+
auto outWriteAS = vk_writeDescriptorSetAS.data();
765+
766+
for (auto i=0; i<drops.size(); i++)
767+
{
768+
const auto& write = drops[i];
769+
770+
outWrite->dstSet = static_cast<const CVulkanDescriptorSet*>(write.dstSet)->getInternalObject();
771+
outWrite->dstBinding = write.binding;
772+
outWrite->dstArrayElement = write.arrayElement;
773+
outWrite->descriptorType = getVkDescriptorTypeFromDescriptorType(descriptorType);
774+
outWrite->descriptorCount = write.count;
775+
switch (asset::IDescriptor::GetTypeCategory(descriptorType))
776+
{
777+
case asset::IDescriptor::EC_BUFFER:
778+
outWrite->pBufferInfo = reinterpret_cast<VkDescriptorBufferInfo*>(nullDescriptors.data());
779+
break;
780+
case asset::IDescriptor::EC_IMAGE:
781+
outWrite->pImageInfo = reinterpret_cast<VkDescriptorImageInfo*>(nullDescriptors.data());
782+
break;
783+
case asset::IDescriptor::EC_BUFFER_VIEW:
784+
outWrite->pTexelBufferView = reinterpret_cast<VkBufferView*>(nullDescriptors.data());
785+
break;
786+
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
787+
outWriteAS->accelerationStructureCount = write.count;
788+
outWriteAS->pAccelerationStructures = reinterpret_cast<VkAccelerationStructureKHR*>(nullDescriptors.data());
789+
break;
790+
default:
791+
assert(!"Invalid code path.");
792+
}
793+
outWrite++;
794+
}
795+
}
796+
m_devf.vk.vkUpdateDescriptorSets(m_vkdev,vk_writeDescriptorSets.size(),vk_writeDescriptorSets.data(),0,nullptr);
735797
}
736798
}
737799

‎src/nbl/video/CVulkanLogicalDevice.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ class CVulkanLogicalDevice final : public ILogicalDevice
282282
// descriptor sets
283283
core::smart_refctd_ptr<IDescriptorPool> createDescriptorPool_impl(const IDescriptorPool::SCreateInfo& createInfo) override;
284284
void updateDescriptorSets_impl(const SUpdateDescriptorSetsParams& params) override;
285-
void nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors) override;
285+
void nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors, asset::IDescriptor::E_TYPE descriptorType) override;
286286

287287
// renderpasses and framebuffers
288288
core::smart_refctd_ptr<IGPURenderpass> createRenderpass_impl(const IGPURenderpass::SCreationParams& params, IGPURenderpass::SCreationParamValidationResult&& validation) override;

‎src/nbl/video/IGPUDescriptorSet.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,8 @@ void IGPUDescriptorSet::dropDescriptors(const IGPUDescriptorSet::SDropDescriptor
134134

135135
auto* dstDescriptors = drop.dstSet->getDescriptors(type, drop.binding);
136136
auto* dstSamplers = drop.dstSet->getMutableSamplers(drop.binding);
137+
assert(dstDescriptors);
138+
assert(dstSamplers);
137139

138140
if (dstDescriptors)
139141
for (uint32_t i = 0; i < drop.count; i++)
@@ -143,8 +145,12 @@ void IGPUDescriptorSet::dropDescriptors(const IGPUDescriptorSet::SDropDescriptor
143145
for (uint32_t i = 0; i < drop.count; i++)
144146
dstSamplers[drop.arrayElement + i] = nullptr;
145147
}
146-
147-
incrementVersion();
148+
// we only increment the version to detect UPDATE-AFTER-BIND and automagically invalidate descriptor sets
149+
// so, only if we do the path that writes descriptors, do we want to increment version
150+
if (getOriginDevice()->getEnabledFeatures().nullDescriptor)
151+
{
152+
incrementVersion();
153+
}
148154
}
149155

150156
bool IGPUDescriptorSet::validateCopy(const IGPUDescriptorSet::SCopyDescriptorSet& copy) const

‎src/nbl/video/ILogicalDevice.cpp‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -444,12 +444,19 @@ bool ILogicalDevice::updateDescriptorSets(const std::span<const IGPUDescriptorSe
444444
return true;
445445
}
446446

447-
void ILogicalDevice::nullifyDescriptors(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors)
447+
bool ILogicalDevice::nullifyDescriptors(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors, asset::IDescriptor::E_TYPE descriptorType)
448448
{
449449
for (const auto& drop : dropDescriptors)
450-
drop.dstSet->dropDescriptors(drop);
450+
{
451+
auto ds = drop.dstSet;
452+
if (!ds || !ds->wasCreatedBy(this))
453+
return false;
451454

452-
nullifyDescriptors_impl(dropDescriptors);
455+
ds->dropDescriptors(drop);
456+
}
457+
458+
nullifyDescriptors_impl(dropDescriptors, descriptorType);
459+
return true;
453460
}
454461

455462
core::smart_refctd_ptr<IGPURenderpass> ILogicalDevice::createRenderpass(const IGPURenderpass::SCreationParams& params)

0 commit comments

Comments
 (0)