Skip to content

Commit 70d77c1

Browse files
devshgraphicsprogrammingdevshgraphicsprogramming
authored
Merge pull request #667 from Devsh-Graphics-Programming/suballocdescriptors-pr-reviews
Suballocated Descriptor Set PR Review Fixes
2 parents 77dd4d7 + b0b2e30 commit 70d77c1

File tree

6 files changed

+54
-33
lines changed

6 files changed

+54
-33
lines changed

‎include/nbl/video/IGPUDescriptorSet.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ class IGPUDescriptorSet : public asset::IDescriptorSet<const IGPUDescriptorSetLa
5757
inline IDescriptorPool* getPool() const { return m_pool.get(); }
5858
inline bool isZombie() const { return (m_pool.get() == nullptr); }
5959

60-
// why is this private? nothing it uses is private
6160
// small utility
6261
inline asset::IDescriptor::E_TYPE getBindingType(const uint32_t binding) const
6362
{

‎include/nbl/video/ILogicalDevice.h

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -865,12 +865,20 @@ class NBL_API2 ILogicalDevice : public core::IReferenceCounted, public IDeviceMe
865865
uint32_t bufferViewCount = 0u;
866866
uint32_t imageCount = 0u;
867867
uint32_t accelerationStructureCount = 0u;
868+
uint32_t accelerationStructureWriteCount = 0u;
868869
};
869870
virtual void updateDescriptorSets_impl(const SUpdateDescriptorSetsParams& params) = 0;
870871

871-
// Drops refcounted references of the descriptors in these indices for the descriptor lifetime tracking
872-
// If the nullDescriptor device feature is enabled, this would also write a null descriptor to the descriptor set
873-
virtual void nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors) = 0;
872+
struct SDropDescriptorSetsParams
873+
{
874+
std::span<const IGPUDescriptorSet::SDropDescriptorSet> drops;
875+
uint32_t bufferCount = 0u;
876+
uint32_t bufferViewCount = 0u;
877+
uint32_t imageCount = 0u;
878+
uint32_t accelerationStructureCount = 0u;
879+
uint32_t accelerationStructureWriteCount = 0u;
880+
};
881+
virtual void nullifyDescriptors_impl(const SDropDescriptorSetsParams& params) = 0;
874882

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

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
5858
m_composed = other.m_composed;
5959
m_addresses = other.m_addresses;
6060
m_binding = other.m_binding;
61+
62+
// Nullifying other
63+
other.m_composed = nullptr;
64+
other.m_addresses = nullptr;
65+
other.m_binding = 0;
6166
return *this;
6267
}
6368

@@ -117,6 +122,9 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
117122
descriptorType(inDescriptorType) {}
118123
SubAllocDescriptorSetRange() {}
119124

125+
SubAllocDescriptorSetRange(const SubAllocDescriptorSetRange& other) = delete;
126+
SubAllocDescriptorSetRange& operator=(const SubAllocDescriptorSetRange& other) = delete;
127+
120128
SubAllocDescriptorSetRange& operator=(SubAllocDescriptorSetRange&& other)
121129
{
122130
eventHandler = std::move(other.eventHandler);

‎src/nbl/video/CVulkanLogicalDevice.cpp

Lines changed: 8 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,7 @@ void CVulkanLogicalDevice::updateDescriptorSets_impl(const SUpdateDescriptorSets
644644
// Each pNext member of any structure (including this one) in the pNext chain must be either NULL or a pointer to a valid instance of
645645
// VkWriteDescriptorSetAccelerationStructureKHR, VkWriteDescriptorSetAccelerationStructureNV, or VkWriteDescriptorSetInlineUniformBlockEXT
646646
core::vector<VkWriteDescriptorSet> vk_writeDescriptorSets(params.writes.size(),{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET,nullptr});
647-
core::vector<VkWriteDescriptorSetAccelerationStructureKHR> vk_writeDescriptorSetAS(MaxDescriptorSetAsWrites,{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_ACCELERATION_STRUCTURE_KHR,nullptr});
647+
core::vector<VkWriteDescriptorSetAccelerationStructureKHR> vk_writeDescriptorSetAS(params.accelerationStructureWriteCount,{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_ACCELERATION_STRUCTURE_KHR,nullptr});
648648

649649
core::vector<VkDescriptorBufferInfo> vk_bufferInfos(params.bufferCount);
650650
core::vector<VkDescriptorImageInfo> vk_imageInfos(params.imageCount);
@@ -731,39 +731,21 @@ void CVulkanLogicalDevice::updateDescriptorSets_impl(const SUpdateDescriptorSets
731731
m_devf.vk.vkUpdateDescriptorSets(m_vkdev,vk_writeDescriptorSets.size(),vk_writeDescriptorSets.data(),vk_copyDescriptorSets.size(),vk_copyDescriptorSets.data());
732732
}
733733

734-
void CVulkanLogicalDevice::nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> drops)
734+
void CVulkanLogicalDevice::nullifyDescriptors_impl(const SDropDescriptorSetsParams& params)
735735
{
736+
const auto& drops = params.drops;
736737
if (getEnabledFeatures().nullDescriptor)
737738
{
738739
return;
739740
}
740741

741742
core::vector<VkWriteDescriptorSet> vk_writeDescriptorSets(drops.size(),{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET,nullptr});
742-
core::vector<VkWriteDescriptorSetAccelerationStructureKHR> vk_writeDescriptorSetAS(MaxDescriptorSetAsWrites,{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_ACCELERATION_STRUCTURE_KHR,nullptr});
743+
core::vector<VkWriteDescriptorSetAccelerationStructureKHR> vk_writeDescriptorSetAS(params.accelerationStructureWriteCount,{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_ACCELERATION_STRUCTURE_KHR,nullptr});
743744

744-
size_t maxSize = 0;
745-
for (auto i = 0; i < drops.size(); i++)
746-
{
747-
const auto& write = drops[i];
748-
auto descriptorType = write.dstSet->getBindingType(write.binding);
749-
size_t descriptorSize;
750-
switch (asset::IDescriptor::GetTypeCategory(descriptorType))
751-
{
752-
case asset::IDescriptor::EC_BUFFER:
753-
descriptorSize = sizeof(VkDescriptorBufferInfo);
754-
break;
755-
case asset::IDescriptor::EC_IMAGE:
756-
descriptorSize = sizeof(VkDescriptorImageInfo);
757-
break;
758-
case asset::IDescriptor::EC_BUFFER_VIEW:
759-
descriptorSize = sizeof(VkBufferView);
760-
break;
761-
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
762-
descriptorSize = sizeof(VkAccelerationStructureKHR);
763-
break;
764-
}
765-
maxSize = core::max(maxSize, write.count * descriptorSize);
766-
}
745+
size_t maxSize = core::max(
746+
core::max(params.bufferCount * sizeof(VkDescriptorBufferInfo), params.imageCount * sizeof(VkDescriptorImageInfo)),
747+
core::max(params.bufferViewCount * sizeof(VkBufferView), params.accelerationStructureCount * sizeof(VkAccelerationStructureKHR))
748+
);
767749

768750
core::vector<uint8_t> nullDescriptors(maxSize, 0u);
769751

‎src/nbl/video/CVulkanLogicalDevice.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,7 @@ class CVulkanLogicalDevice final : public ILogicalDevice
283283
// descriptor sets
284284
core::smart_refctd_ptr<IDescriptorPool> createDescriptorPool_impl(const IDescriptorPool::SCreateInfo& createInfo) override;
285285
void updateDescriptorSets_impl(const SUpdateDescriptorSetsParams& params) override;
286-
void nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors) override;
286+
void nullifyDescriptors_impl(const SDropDescriptorSetsParams& params) override;
287287

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

‎src/nbl/video/ILogicalDevice.cpp

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,7 @@ bool ILogicalDevice::updateDescriptorSets(const std::span<const IGPUDescriptorSe
422422
break;
423423
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
424424
params.accelerationStructureCount += writeCount;
425+
params.accelerationStructureWriteCount++;
425426
break;
426427
default: // validation failed
427428
return false;
@@ -457,13 +458,36 @@ bool ILogicalDevice::updateDescriptorSets(const std::span<const IGPUDescriptorSe
457458

458459
bool ILogicalDevice::nullifyDescriptors(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors)
459460
{
461+
SDropDescriptorSetsParams params = {.drops=dropDescriptors};
460462
for (const auto& drop : dropDescriptors)
461463
{
462464
auto ds = drop.dstSet;
463465
if (!ds || !ds->wasCreatedBy(this))
464466
return false;
467+
468+
auto bindingType = ds->getBindingType(drop.binding);
469+
auto writeCount = drop.count;
470+
switch (asset::IDescriptor::GetTypeCategory(bindingType))
471+
{
472+
case asset::IDescriptor::EC_BUFFER:
473+
params.bufferCount += writeCount;
474+
break;
475+
case asset::IDescriptor::EC_IMAGE:
476+
params.imageCount += writeCount;
477+
break;
478+
case asset::IDescriptor::EC_BUFFER_VIEW:
479+
params.bufferViewCount += writeCount;
480+
break;
481+
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
482+
params.accelerationStructureCount += writeCount;
483+
params.accelerationStructureWriteCount++;
484+
break;
485+
default: // validation failed
486+
return false;
487+
}
488+
465489
// (no binding)
466-
if (ds->getBindingType(drop.binding) == asset::IDescriptor::E_TYPE::ET_COUNT)
490+
if (bindingType == asset::IDescriptor::E_TYPE::ET_COUNT)
467491
return false;
468492
}
469493

@@ -473,7 +497,7 @@ bool ILogicalDevice::nullifyDescriptors(const std::span<const IGPUDescriptorSet:
473497
ds->dropDescriptors(drop);
474498
}
475499

476-
nullifyDescriptors_impl(dropDescriptors);
500+
nullifyDescriptors_impl(params);
477501
return true;
478502
}
479503

0 commit comments

Comments
 (0)