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.

Sign up for GitHub

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

Suballocated Descriptor Set PR Review Fixes #667

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Suballocated Descriptor Set PR Review Fixes #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2c3a33
PR reviews
deprilula28 Mar 17, 2024
bfd3be3
PR reviews
deprilula28 Mar 17, 2024
b0b2e30
Didn't go in the last commit for some reason
deprilula28 Mar 17, 2024
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
1 change: 0 additions & 1 deletion include/nbl/video/IGPUDescriptorSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ class IGPUDescriptorSet : public asset::IDescriptorSet<const IGPUDescriptorSetLa
inline IDescriptorPool* getPool() const { return m_pool.get(); }
inline bool isZombie() const { return (m_pool.get() == nullptr); }

// why is this private? nothing it uses is private
// small utility
inline asset::IDescriptor::E_TYPE getBindingType(const uint32_t binding) const
{
Expand Down
14 changes: 11 additions & 3 deletions include/nbl/video/ILogicalDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,12 +865,20 @@ class NBL_API2 ILogicalDevice : public core::IReferenceCounted, public IDeviceMe
uint32_t bufferViewCount = 0u;
uint32_t imageCount = 0u;
uint32_t accelerationStructureCount = 0u;
uint32_t accelerationStructureWriteCount = 0u;
};
virtual void updateDescriptorSets_impl(const SUpdateDescriptorSetsParams& params) = 0;

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

virtual core::smart_refctd_ptr<IGPURenderpass> createRenderpass_impl(const IGPURenderpass::SCreationParams& params, IGPURenderpass::SCreationParamValidationResult&& validation) = 0;
virtual core::smart_refctd_ptr<IGPUFramebuffer> createFramebuffer_impl(IGPUFramebuffer::SCreationParams&& params) = 0;
Expand Down
8 changes: 8 additions & 0 deletions include/nbl/video/alloc/SubAllocatedDescriptorSet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
m_composed = other.m_composed;
m_addresses = other.m_addresses;
m_binding = other.m_binding;

// Nullifying other
other.m_composed = nullptr;
other.m_addresses = nullptr;
other.m_binding = 0;
return *this;
}

Expand Down Expand Up @@ -117,6 +122,9 @@ class SubAllocatedDescriptorSet : public core::IReferenceCounted
descriptorType(inDescriptorType) {}
SubAllocDescriptorSetRange() {}

SubAllocDescriptorSetRange(const SubAllocDescriptorSetRange& other) = delete;
SubAllocDescriptorSetRange& operator=(const SubAllocDescriptorSetRange& other) = delete;

SubAllocDescriptorSetRange& operator=(SubAllocDescriptorSetRange&& other)
{
eventHandler = std::move(other.eventHandler);
Expand Down
34 changes: 8 additions & 26 deletions src/nbl/video/CVulkanLogicalDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ void CVulkanLogicalDevice::updateDescriptorSets_impl(const SUpdateDescriptorSets
// 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
// VkWriteDescriptorSetAccelerationStructureKHR, VkWriteDescriptorSetAccelerationStructureNV, or VkWriteDescriptorSetInlineUniformBlockEXT
core::vector<VkWriteDescriptorSet> vk_writeDescriptorSets(params.writes.size(),{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET,nullptr});
core::vector<VkWriteDescriptorSetAccelerationStructureKHR> vk_writeDescriptorSetAS(MaxDescriptorSetAsWrites,{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_ACCELERATION_STRUCTURE_KHR,nullptr});
core::vector<VkWriteDescriptorSetAccelerationStructureKHR> vk_writeDescriptorSetAS(params.accelerationStructureWriteCount,{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET_ACCELERATION_STRUCTURE_KHR,nullptr});

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

void CVulkanLogicalDevice::nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> drops)
void CVulkanLogicalDevice::nullifyDescriptors_impl(const SDropDescriptorSetsParams& params)
{
const auto& drops = params.drops;
if (getEnabledFeatures().nullDescriptor)
{
return;
}

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

size_t maxSize = 0;
for (auto i = 0; i < drops.size(); i++)
{
const auto& write = drops[i];
auto descriptorType = write.dstSet->getBindingType(write.binding);
size_t descriptorSize;
switch (asset::IDescriptor::GetTypeCategory(descriptorType))
{
case asset::IDescriptor::EC_BUFFER:
descriptorSize = sizeof(VkDescriptorBufferInfo);
break;
case asset::IDescriptor::EC_IMAGE:
descriptorSize = sizeof(VkDescriptorImageInfo);
break;
case asset::IDescriptor::EC_BUFFER_VIEW:
descriptorSize = sizeof(VkBufferView);
break;
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
descriptorSize = sizeof(VkAccelerationStructureKHR);
break;
}
maxSize = core::max(maxSize, write.count * descriptorSize);
}
size_t maxSize = core::max(
core::max(params.bufferCount * sizeof(VkDescriptorBufferInfo), params.imageCount * sizeof(VkDescriptorImageInfo)),
core::max(params.bufferViewCount * sizeof(VkBufferView), params.accelerationStructureCount * sizeof(VkAccelerationStructureKHR))
);

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

Expand Down
2 changes: 1 addition & 1 deletion src/nbl/video/CVulkanLogicalDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ class CVulkanLogicalDevice final : public ILogicalDevice
// descriptor sets
core::smart_refctd_ptr<IDescriptorPool> createDescriptorPool_impl(const IDescriptorPool::SCreateInfo& createInfo) override;
void updateDescriptorSets_impl(const SUpdateDescriptorSetsParams& params) override;
void nullifyDescriptors_impl(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors) override;
void nullifyDescriptors_impl(const SDropDescriptorSetsParams& params) override;

// renderpasses and framebuffers
core::smart_refctd_ptr<IGPURenderpass> createRenderpass_impl(const IGPURenderpass::SCreationParams& params, IGPURenderpass::SCreationParamValidationResult&& validation) override;
Expand Down
28 changes: 26 additions & 2 deletions src/nbl/video/ILogicalDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ bool ILogicalDevice::updateDescriptorSets(const std::span<const IGPUDescriptorSe
break;
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
params.accelerationStructureCount += writeCount;
params.accelerationStructureWriteCount++;
break;
default: // validation failed
return false;
Expand Down Expand Up @@ -457,13 +458,36 @@ bool ILogicalDevice::updateDescriptorSets(const std::span<const IGPUDescriptorSe

bool ILogicalDevice::nullifyDescriptors(const std::span<const IGPUDescriptorSet::SDropDescriptorSet> dropDescriptors)
{
SDropDescriptorSetsParams params = {.drops=dropDescriptors};
for (const auto& drop : dropDescriptors)
{
auto ds = drop.dstSet;
if (!ds || !ds->wasCreatedBy(this))
return false;

auto bindingType = ds->getBindingType(drop.binding);
auto writeCount = drop.count;
switch (asset::IDescriptor::GetTypeCategory(bindingType))
{
case asset::IDescriptor::EC_BUFFER:
params.bufferCount += writeCount;
break;
case asset::IDescriptor::EC_IMAGE:
params.imageCount += writeCount;
break;
case asset::IDescriptor::EC_BUFFER_VIEW:
params.bufferViewCount += writeCount;
break;
case asset::IDescriptor::EC_ACCELERATION_STRUCTURE:
params.accelerationStructureCount += writeCount;
params.accelerationStructureWriteCount++;
break;
default: // validation failed
return false;
}

// (no binding)
if (ds->getBindingType(drop.binding) == asset::IDescriptor::E_TYPE::ET_COUNT)
if (bindingType == asset::IDescriptor::E_TYPE::ET_COUNT)
return false;
}

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

nullifyDescriptors_impl(dropDescriptors);
nullifyDescriptors_impl(params);
return true;
}

Expand Down