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

Jump to bottom

vulkan: fix subpass validation #6980

Merged
merged 3 commits into from
Jul 25, 2023
Merged

vulkan: fix subpass validation #6980

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
ceaa88a
vulkan: fix subpass validation
poweifeng Jul 21, 2023
23c48f7
Merge branch 'main' into pf/fix-subpass-validation
poweifeng Jul 24, 2023
2de0fd5
Merge branch 'main' into pf/fix-subpass-validation
poweifeng Jul 25, 2023
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: 2 additions & 2 deletions filament/backend/src/vulkan/VulkanDriver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,7 +1265,7 @@ void VulkanDriver::endRenderPass(int) {
}

if (mCurrentRenderPass.currentSubpass > 0) {
for (uint32_t i = 0; i < VulkanPipelineCache::TARGET_BINDING_COUNT; i++) {
for (uint32_t i = 0; i < VulkanPipelineCache::INPUT_ATTACHMENT_COUNT; i++) {
mPipelineCache.bindInputAttachment(i, {});
}
mCurrentRenderPass.currentSubpass = 0;
Expand All @@ -1287,7 +1287,7 @@ void VulkanDriver::nextSubpass(int) {
mPipelineCache.bindRenderPass(mCurrentRenderPass.renderPass,
++mCurrentRenderPass.currentSubpass);

for (uint32_t i = 0; i < VulkanPipelineCache::TARGET_BINDING_COUNT; i++) {
for (uint32_t i = 0; i < VulkanPipelineCache::INPUT_ATTACHMENT_COUNT; i++) {
if ((1 << i) & mCurrentRenderPass.params.subpassMask) {
VulkanAttachment subpassInput = renderTarget->getColor(i);
VkDescriptorImageInfo info = {
Expand Down
30 changes: 14 additions & 16 deletions filament/backend/src/vulkan/VulkanPipelineCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,9 @@ VulkanPipelineCache::DescriptorCacheEntry* VulkanPipelineCache::createDescriptor
// Rewrite every binding in the new descriptor sets.
VkDescriptorBufferInfo descriptorBuffers[UBUFFER_BINDING_COUNT];
VkDescriptorImageInfo descriptorSamplers[SAMPLER_BINDING_COUNT];
VkDescriptorImageInfo descriptorInputAttachments[TARGET_BINDING_COUNT];
VkDescriptorImageInfo descriptorInputAttachments[INPUT_ATTACHMENT_COUNT];
VkWriteDescriptorSet descriptorWrites[UBUFFER_BINDING_COUNT + SAMPLER_BINDING_COUNT +
TARGET_BINDING_COUNT];
INPUT_ATTACHMENT_COUNT];
uint32_t nwrites = 0;
VkWriteDescriptorSet* writes = descriptorWrites;
nwrites = 0;
Expand Down Expand Up @@ -286,9 +286,9 @@ VulkanPipelineCache::DescriptorCacheEntry* VulkanPipelineCache::createDescriptor
writeInfo.dstBinding = binding;
}
}
for (uint32_t binding = 0; binding < TARGET_BINDING_COUNT; binding++) {
VkWriteDescriptorSet& writeInfo = writes[nwrites++];
for (uint32_t binding = 0; binding < INPUT_ATTACHMENT_COUNT; binding++) {
if (mDescriptorRequirements.inputAttachments[binding].imageView) {
VkWriteDescriptorSet& writeInfo = writes[nwrites++];
VkDescriptorImageInfo& imageInfo = descriptorInputAttachments[binding];
imageInfo = mDescriptorRequirements.inputAttachments[binding];
writeInfo.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
Expand All @@ -299,13 +299,11 @@ VulkanPipelineCache::DescriptorCacheEntry* VulkanPipelineCache::createDescriptor
writeInfo.pImageInfo = &imageInfo;
writeInfo.pBufferInfo = nullptr;
writeInfo.pTexelBufferView = nullptr;
} else {
writeInfo = mDummyTargetWriteInfo;
assert_invariant(mDummyTargetInfo.imageView);
writeInfo.dstSet = descriptorCacheEntry.handles[2];
writeInfo.dstBinding = binding;
}
writeInfo.dstSet = descriptorCacheEntry.handles[2];
writeInfo.dstBinding = binding;
}

vkUpdateDescriptorSets(mDevice, nwrites, writes, 0, nullptr);

return &mDescriptorSets.emplace(mDescriptorRequirements, descriptorCacheEntry).first.value();
Expand Down Expand Up @@ -518,14 +516,14 @@ VulkanPipelineCache::PipelineLayoutCacheEntry* VulkanPipelineCache::getOrCreateP
vkCreateDescriptorSetLayout(mDevice, &dlinfo, VKALLOC, &cacheEntry.descriptorSetLayouts[1]);

// Next create the descriptor set layout for input attachments.
VkDescriptorSetLayoutBinding tbindings[TARGET_BINDING_COUNT];
VkDescriptorSetLayoutBinding tbindings[INPUT_ATTACHMENT_COUNT];
binding.descriptorType = VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT;
binding.stageFlags = VK_SHADER_STAGE_FRAGMENT_BIT;
for (uint32_t i = 0; i < TARGET_BINDING_COUNT; i++) {
for (uint32_t i = 0; i < INPUT_ATTACHMENT_COUNT; i++) {
binding.binding = i;
tbindings[i] = binding;
}
dlinfo.bindingCount = TARGET_BINDING_COUNT;
dlinfo.bindingCount = INPUT_ATTACHMENT_COUNT;
dlinfo.pBindings = tbindings;
vkCreateDescriptorSetLayout(mDevice, &dlinfo, VKALLOC, &cacheEntry.descriptorSetLayouts[2]);

Expand Down Expand Up @@ -633,9 +631,9 @@ void VulkanPipelineCache::bindSamplers(VkDescriptorImageInfo samplers[SAMPLER_BI

void VulkanPipelineCache::bindInputAttachment(uint32_t bindingIndex,
VkDescriptorImageInfo targetInfo) noexcept {
ASSERT_POSTCONDITION(bindingIndex < TARGET_BINDING_COUNT,
ASSERT_POSTCONDITION(bindingIndex < INPUT_ATTACHMENT_COUNT,
"Input attachment bindings overflow: index = %d, capacity = %d.",
bindingIndex, TARGET_BINDING_COUNT);
bindingIndex, INPUT_ATTACHMENT_COUNT);
mDescriptorRequirements.inputAttachments[bindingIndex] = targetInfo;
}

Expand Down Expand Up @@ -757,7 +755,7 @@ VkDescriptorPool VulkanPipelineCache::createDescriptorPool(uint32_t size) const
poolSizes[1].type = VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER;
poolSizes[1].descriptorCount = poolInfo.maxSets * SAMPLER_BINDING_COUNT;
poolSizes[2].type = VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT;
poolSizes[2].descriptorCount = poolInfo.maxSets * TARGET_BINDING_COUNT;
poolSizes[2].descriptorCount = poolInfo.maxSets * INPUT_ATTACHMENT_COUNT;

VkDescriptorPool pool;
const UTILS_UNUSED VkResult result = vkCreateDescriptorPool(mDevice, &poolInfo, VKALLOC, &pool);
Expand Down Expand Up @@ -864,7 +862,7 @@ bool VulkanPipelineCache::DescEqual::operator()(const DescriptorKey& k1,
return false;
}
}
for (uint32_t i = 0; i < TARGET_BINDING_COUNT; i++) {
for (uint32_t i = 0; i < INPUT_ATTACHMENT_COUNT; i++) {
if (k1.inputAttachments[i].imageView != k2.inputAttachments[i].imageView ||
k1.inputAttachments[i].imageLayout != k2.inputAttachments[i].imageLayout) {
return false;
Expand Down
22 changes: 13 additions & 9 deletions filament/backend/src/vulkan/VulkanPipelineCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,11 @@ class VulkanPipelineCache : public CommandBufferObserver {

static constexpr uint32_t UBUFFER_BINDING_COUNT = Program::UNIFORM_BINDING_COUNT;
static constexpr uint32_t SAMPLER_BINDING_COUNT = MAX_SAMPLER_COUNT;
static constexpr uint32_t TARGET_BINDING_COUNT = MRT::MAX_SUPPORTED_RENDER_TARGET_COUNT;

// We assume only one possible input attachment between two subpasses. See also the subpasses
// definition in VulkanFboCache.
static constexpr uint32_t INPUT_ATTACHMENT_COUNT = 1;

Comment on lines +62 to +65
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would need to change so that we break that assumption? I suppose the material knows how many inputs it has, so that should be passed to the renderpass or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the really general case, we'd need to know how many subpasses, and the attachment -> input mapping between consecutive subpasses at the start of the renderpass.

right now we assume we have at most one additional subpass, and the sole input attachment to the second subpass is always the first color attachment.

static constexpr uint32_t SHADER_MODULE_COUNT = 2;
static constexpr uint32_t VERTEX_ATTRIBUTE_COUNT = MAX_VERTEX_ATTRIBUTE_COUNT;

Expand Down Expand Up @@ -298,17 +302,17 @@ class VulkanPipelineCache : public CommandBufferObserver {

// Represents all the Vulkan state that comprises a bound descriptor set.
struct DescriptorKey {
VkBuffer uniformBuffers[UBUFFER_BINDING_COUNT]; // 80 0
DescriptorImageInfo samplers[SAMPLER_BINDING_COUNT]; // 1488 80
DescriptorImageInfo inputAttachments[TARGET_BINDING_COUNT]; // 192 1568
uint32_t uniformBufferOffsets[UBUFFER_BINDING_COUNT]; // 40 1760
uint32_t uniformBufferSizes[UBUFFER_BINDING_COUNT]; // 40 1080
VkBuffer uniformBuffers[UBUFFER_BINDING_COUNT]; // 80 0
DescriptorImageInfo samplers[SAMPLER_BINDING_COUNT]; // 1488 80
DescriptorImageInfo inputAttachments[INPUT_ATTACHMENT_COUNT]; // 24 1568
uint32_t uniformBufferOffsets[UBUFFER_BINDING_COUNT]; // 40 1592
uint32_t uniformBufferSizes[UBUFFER_BINDING_COUNT]; // 40 1632
};
static_assert(offsetof(DescriptorKey, samplers) == 80);
static_assert(offsetof(DescriptorKey, inputAttachments) == 1568);
static_assert(offsetof(DescriptorKey, uniformBufferOffsets) == 1760);
static_assert(offsetof(DescriptorKey, uniformBufferSizes) == 1800);
static_assert(sizeof(DescriptorKey) == 1840, "DescriptorKey must not have implicit padding.");
static_assert(offsetof(DescriptorKey, uniformBufferOffsets) == 1592);
static_assert(offsetof(DescriptorKey, uniformBufferSizes) == 1632);
static_assert(sizeof(DescriptorKey) == 1672, "DescriptorKey must not have implicit padding.");

using DescHashFn = utils::hash::MurmurHashFn<DescriptorKey>;

Expand Down
Loading