Skip to content

Commit

Permalink
Relax the behaviour of the Vulkan API around binding sets and registe…
Browse files Browse the repository at this point in the history 
…r spaces to better match the DX12 API
  • Loading branch information
@Oli-Wright @apanteleev
Oli-Wright authored and apanteleev committed Sep 18, 2024
1 parent c11134a commit a4c6f97
Show file tree
Hide file tree
Showing 10 changed files with 313 additions and 229 deletions.
20 changes: 13 additions & 7 deletions include/nvrhi/nvrhi.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1635,17 +1635,23 @@ namespace nvrhi
struct BindingLayoutDesc
{
ShaderType visibility = ShaderType::None;

// In DX12, this controls the register space of the bindings
// In Vulkan, DXC maps register spaces to descriptor sets by default, so this can be used to
// determine the descriptor set index for the binding layout.
// In order to use this behaviour, you must set `registerSpaceIsDescriptorSet` to true. See below.
uint32_t registerSpace = 0;

// This flag controls the validation behavior for pipelines that use multiple binding layouts.
// This flag controls the behavior for pipelines that use multiple binding layouts.
// It must be set to the same value for _all_ of the binding layouts in a pipeline.
// - When it's set to `false`, the `registerSpace` parameter only affects the DX12 implementation,
// and the validation layer will report an error when non-zero `registerSpace` is used with other APIs.
// - When it's set to `true`, the `registerSpace` parameter is assumed to be the same as the descriptor set
// index on Vulkan. Since binding layouts and binding sets map to Vulkan descriptor sets 1:1,
// that means if a pipeline is using multiple binding layouts, layout 0 must have `registerSpace = 0`,
// layout 1 must have `registerSpace = 1` and so on. NVRHI validation layer will verify that and
// report errors on pipeline creation when register spaces don't match layout indices.
// The motivation for such validation is that DXC maps register spaces to Vulkan descriptor sets by default.
// - When it's set to `true` the parameter also affects the Vulkan implementation, allowing any
// layout to occupy any register space or descriptor set, regardless of their order in the pipeline.
// However, a consequence of DXC mapping the descriptor set index to register space is that you may
// not have more than one `BindingLayout` using the same `registerSpace` value in the same pipeline.
// - When it's set to different values for the layouts in a pipeline, the validation layer will report
// an error.
bool registerSpaceIsDescriptorSet = false;

BindingLayoutItemArray bindings;
Expand Down
48 changes: 45 additions & 3 deletions src/validation/validation-device.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,15 @@ namespace nvrhi::validation

int pushConstantCount = 0;
uint32_t pushConstantSize = 0;
enum class RegisterSpaceIsDescriptorSet
{
False,
True,
Undetermined,
Mixed
} registerSpaceIsDescriptorSet = RegisterSpaceIsDescriptorSet::Undetermined;
static_vector<int, c_MaxBindingLayouts> registerSpaceToLayoutIdx(c_MaxBindingLayouts);
registerSpaceToLayoutIdx.fill(-1);

for (int layoutIndex = 0; layoutIndex < numBindingLayouts; layoutIndex++)
{
Expand All @@ -939,17 +948,50 @@ namespace nvrhi::validation

if (layoutDesc->registerSpaceIsDescriptorSet)
{
if (uint32_t(layoutIndex) != layoutDesc->registerSpace)
if (layoutDesc->registerSpace >= c_MaxBindingLayouts)
{
std::stringstream errorStream;
errorStream << "Binding layout at index " << layoutIndex << " has registerSpace = " << layoutDesc->registerSpace
<< ". Largest supported registerSpace index is " << (c_MaxBindingLayouts-1);
error(errorStream.str());
anyErrors = true;
}
else if (registerSpaceToLayoutIdx[layoutDesc->registerSpace] != -1)
{
std::stringstream errorStream;
errorStream << "Binding layout at index " << layoutIndex << " has registerSpace = " << layoutDesc->registerSpace
<< ". When BindingLayoutDesc.registerSpaceIsDescriptorSet = true, the layout index in the pipeline must match its registerSpace.";
errorStream << "Binding layout at index " << layoutIndex << " has registerSpace = " << layoutDesc->registerSpace
<< ". That register space has already been used in layout index " << registerSpaceToLayoutIdx[layoutDesc->registerSpace];
error(errorStream.str());
anyErrors = true;
}
registerSpaceToLayoutIdx[layoutDesc->registerSpace] = layoutIndex;
if (registerSpaceIsDescriptorSet == RegisterSpaceIsDescriptorSet::Undetermined)
{
registerSpaceIsDescriptorSet = RegisterSpaceIsDescriptorSet::True;
}
else if (registerSpaceIsDescriptorSet == RegisterSpaceIsDescriptorSet::False)
{
registerSpaceIsDescriptorSet = RegisterSpaceIsDescriptorSet::Mixed;
}
}
else
{
if (registerSpaceIsDescriptorSet == RegisterSpaceIsDescriptorSet::Undetermined)
{
registerSpaceIsDescriptorSet = RegisterSpaceIsDescriptorSet::False;
}
else if (registerSpaceIsDescriptorSet == RegisterSpaceIsDescriptorSet::True)
{
registerSpaceIsDescriptorSet = RegisterSpaceIsDescriptorSet::Mixed;
}
}
}
}
if (registerSpaceIsDescriptorSet == RegisterSpaceIsDescriptorSet::Mixed)
{
error("Pipeline contains Binding layouts with differing values of `registerSpaceIsDescriptorSet`");
anyErrors = true;
}

if (pushConstantCount > 1)
{
Expand Down
16 changes: 15 additions & 1 deletion src/vulkan/vulkan-backend.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ namespace nvrhi::vulkan
std::unique_ptr<rtxmu::VkAccelStructManager> rtxMemUtil;
std::unique_ptr<RtxMuResources> rtxMuResources;
#endif
vk::DescriptorSetLayout emptyDescriptorSetLayout;

void nameVKObject(const void* handle, vk::DebugReportObjectTypeEXT objtype, const char* name) const;
void error(const std::string& message) const;
Expand Down Expand Up @@ -808,13 +809,23 @@ namespace nvrhi::vulkan
template <typename T>
using BindingVector = static_vector<T, c_MaxBindingLayouts>;

// common code when creating shader pipelines to build binding set layouts
vk::Result createPipelineLayout(
vk::PipelineLayout& outPipelineLayout,
BindingVector<RefCountPtr<BindingLayout>>& outBindingLayouts,
vk::ShaderStageFlags& outPushConstantVisibility,
BindingVector<uint32_t>& outStateBindingIdxToPipelineBindingIdx,
VulkanContext const& context,
BindingLayoutVector const& inBindingLayouts);

class GraphicsPipeline : public RefCounter<IGraphicsPipeline>
{
public:
GraphicsPipelineDesc desc;
FramebufferInfo framebufferInfo;
ShaderType shaderMask = ShaderType::None;
BindingVector<RefCountPtr<BindingLayout>> pipelineBindingLayouts;
BindingVector<uint32_t> descriptorSetIdxToBindingIdx;
vk::PipelineLayout pipelineLayout;
vk::Pipeline pipeline;
vk::ShaderStageFlags pushConstantVisibility;
Expand All @@ -839,6 +850,7 @@ namespace nvrhi::vulkan
ComputePipelineDesc desc;

BindingVector<RefCountPtr<BindingLayout>> pipelineBindingLayouts;
BindingVector<uint32_t> descriptorSetIdxToBindingIdx;
vk::PipelineLayout pipelineLayout;
vk::Pipeline pipeline;
vk::ShaderStageFlags pushConstantVisibility;
Expand All @@ -862,6 +874,7 @@ namespace nvrhi::vulkan
FramebufferInfo framebufferInfo;
ShaderType shaderMask = ShaderType::None;
BindingVector<RefCountPtr<BindingLayout>> pipelineBindingLayouts;
BindingVector<uint32_t> descriptorSetIdxToBindingIdx;
vk::PipelineLayout pipelineLayout;
vk::Pipeline pipeline;
vk::ShaderStageFlags pushConstantVisibility;
Expand All @@ -885,6 +898,7 @@ namespace nvrhi::vulkan
public:
rt::PipelineDesc desc;
BindingVector<RefCountPtr<BindingLayout>> pipelineBindingLayouts;
BindingVector<uint32_t> descriptorSetIdxToBindingIdx;
vk::PipelineLayout pipelineLayout;
vk::Pipeline pipeline;
vk::ShaderStageFlags pushConstantVisibility;
Expand Down Expand Up @@ -1270,7 +1284,7 @@ namespace nvrhi::vulkan

void clearTexture(ITexture* texture, TextureSubresourceSet subresources, const vk::ClearColorValue& clearValue);

void bindBindingSets(vk::PipelineBindPoint bindPoint, vk::PipelineLayout pipelineLayout, const BindingSetVector& bindings);
void bindBindingSets(vk::PipelineBindPoint bindPoint, vk::PipelineLayout pipelineLayout, const BindingSetVector& bindings, BindingVector<uint32_t> const& descriptorSetIdxToBindingIdx);

void endRenderPass();

Expand Down
58 changes: 10 additions & 48 deletions src/vulkan/vulkan-compute.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

namespace nvrhi::vulkan
{

ComputePipelineHandle Device::createComputePipeline(const ComputePipelineDesc& desc)
{
vk::Result res;
Expand All @@ -35,50 +34,13 @@ namespace nvrhi::vulkan
ComputePipeline *pso = new ComputePipeline(m_Context);
pso->desc = desc;

for (const BindingLayoutHandle& _layout : desc.bindingLayouts)
{
BindingLayout* layout = checked_cast<BindingLayout*>(_layout.Get());
pso->pipelineBindingLayouts.push_back(layout);
}

BindingVector<vk::DescriptorSetLayout> descriptorSetLayouts;
uint32_t pushConstantSize = 0;
pso->pushConstantVisibility = vk::ShaderStageFlagBits();
for (const BindingLayoutHandle& _layout : desc.bindingLayouts)
{
BindingLayout* layout = checked_cast<BindingLayout*>(_layout.Get());
descriptorSetLayouts.push_back(layout->descriptorSetLayout);

if (!layout->isBindless)
{
for (const BindingLayoutItem& item : layout->desc.bindings)
{
if (item.type == ResourceType::PushConstants)
{
pushConstantSize = item.size;
pso->pushConstantVisibility = convertShaderTypeToShaderStageFlagBits(layout->desc.visibility);
// assume there's only one push constant item in all layouts -- the validation layer makes sure of that
break;
}
}
}
}

auto pushConstantRange = vk::PushConstantRange()
.setOffset(0)
.setSize(pushConstantSize)
.setStageFlags(pso->pushConstantVisibility);

auto pipelineLayoutInfo = vk::PipelineLayoutCreateInfo()
.setSetLayoutCount(uint32_t(descriptorSetLayouts.size()))
.setPSetLayouts(descriptorSetLayouts.data())
.setPushConstantRangeCount(pushConstantSize ? 1 : 0)
.setPPushConstantRanges(&pushConstantRange);

res = m_Context.device.createPipelineLayout(&pipelineLayoutInfo,
m_Context.allocationCallbacks,
&pso->pipelineLayout);

res = createPipelineLayout(
pso->pipelineLayout,
pso->pipelineBindingLayouts,
pso->pushConstantVisibility,
pso->descriptorSetIdxToBindingIdx,
m_Context,
desc.bindingLayouts);
CHECK_VK_FAIL(res)

Shader* CS = checked_cast<Shader*>(desc.CS.Get());
Expand Down Expand Up @@ -159,7 +121,7 @@ namespace nvrhi::vulkan
{
for (size_t i = 0; i < state.bindings.size() && i < pso->desc.bindingLayouts.size(); i++)
{
BindingLayout* layout = pso->pipelineBindingLayouts[i].Get();
BindingLayout* layout = checked_cast<BindingLayout*>(pso->desc.bindingLayouts[i].Get());

if ((layout->desc.visibility & ShaderType::Compute) == 0)
continue;
Expand All @@ -180,7 +142,7 @@ namespace nvrhi::vulkan

if (arraysAreDifferent(m_CurrentComputeState.bindings, state.bindings) || m_AnyVolatileBufferWrites)
{
bindBindingSets(vk::PipelineBindPoint::eCompute, pso->pipelineLayout, state.bindings);
bindBindingSets(vk::PipelineBindPoint::eCompute, pso->pipelineLayout, state.bindings, pso->descriptorSetIdxToBindingIdx);
}

m_CurrentPipelineLayout = pso->pipelineLayout;
Expand Down Expand Up @@ -213,7 +175,7 @@ namespace nvrhi::vulkan
{
ComputePipeline* pso = checked_cast<ComputePipeline*>(m_CurrentComputeState.pipeline);

bindBindingSets(vk::PipelineBindPoint::eCompute, pso->pipelineLayout, m_CurrentComputeState.bindings);
bindBindingSets(vk::PipelineBindPoint::eCompute, pso->pipelineLayout, m_CurrentComputeState.bindings, pso->descriptorSetIdxToBindingIdx);

m_AnyVolatileBufferWrites = false;
}
Expand Down
13 changes: 13 additions & 0 deletions src/vulkan/vulkan-device.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,19 @@ namespace nvrhi::vulkan
{
m_Context.error("Failed to create the pipeline cache");
}

// Create an empty Vk::DescriptorSetLayout
auto descriptorSetLayoutInfo = vk::DescriptorSetLayoutCreateInfo()
.setBindingCount(0)
.setPBindings(nullptr);
res = m_Context.device.createDescriptorSetLayout(&descriptorSetLayoutInfo,
m_Context.allocationCallbacks,
&m_Context.emptyDescriptorSetLayout);

if (res != vk::Result::eSuccess)
{
m_Context.error("Failed to create an empty descriptor set layout");
}
}

Device::~Device()
Expand Down
54 changes: 9 additions & 45 deletions src/vulkan/vulkan-graphics.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,12 +371,6 @@ namespace nvrhi::vulkan
GraphicsPipeline *pso = new GraphicsPipeline(m_Context);
pso->desc = desc;
pso->framebufferInfo = fb->framebufferInfo;

for (const BindingLayoutHandle& _layout : desc.bindingLayouts)
{
BindingLayout* layout = checked_cast<BindingLayout*>(_layout.Get());
pso->pipelineBindingLayouts.push_back(layout);
}

Shader* VS = checked_cast<Shader*>(desc.VS.Get());
Shader* HS = checked_cast<Shader*>(desc.HS.Get());
Expand Down Expand Up @@ -505,43 +499,13 @@ namespace nvrhi::vulkan
.setCombinerOps(combiners)
.setFragmentSize(convertFragmentShadingRate(desc.shadingRateState.shadingRate));

BindingVector<vk::DescriptorSetLayout> descriptorSetLayouts;
uint32_t pushConstantSize = 0;
pso->pushConstantVisibility = vk::ShaderStageFlagBits();
for (const BindingLayoutHandle& _layout : desc.bindingLayouts)
{
BindingLayout* layout = checked_cast<BindingLayout*>(_layout.Get());
descriptorSetLayouts.push_back(layout->descriptorSetLayout);

if (!layout->isBindless)
{
for (const BindingLayoutItem& item : layout->desc.bindings)
{
if (item.type == ResourceType::PushConstants)
{
pushConstantSize = item.size;
pso->pushConstantVisibility = convertShaderTypeToShaderStageFlagBits(layout->desc.visibility);
// assume there's only one push constant item in all layouts -- the validation layer makes sure of that
break;
}
}
}
}

auto pushConstantRange = vk::PushConstantRange()
.setOffset(0)
.setSize(pushConstantSize)
.setStageFlags(pso->pushConstantVisibility);

auto pipelineLayoutInfo = vk::PipelineLayoutCreateInfo()
.setSetLayoutCount(uint32_t(descriptorSetLayouts.size()))
.setPSetLayouts(descriptorSetLayouts.data())
.setPushConstantRangeCount(pushConstantSize ? 1 : 0)
.setPPushConstantRanges(&pushConstantRange);

res = m_Context.device.createPipelineLayout(&pipelineLayoutInfo,
m_Context.allocationCallbacks,
&pso->pipelineLayout);
res = createPipelineLayout(
pso->pipelineLayout,
pso->pipelineBindingLayouts,
pso->pushConstantVisibility,
pso->descriptorSetIdxToBindingIdx,
m_Context,
desc.bindingLayouts);
CHECK_VK_FAIL(res)

attachment_vector<vk::PipelineColorBlendAttachmentState> colorBlendAttachments(fb->desc.colorAttachments.size());
Expand Down Expand Up @@ -710,7 +674,7 @@ namespace nvrhi::vulkan

if (arraysAreDifferent(m_CurrentComputeState.bindings, state.bindings) || m_AnyVolatileBufferWrites)
{
bindBindingSets(vk::PipelineBindPoint::eGraphics, pso->pipelineLayout, state.bindings);
bindBindingSets(vk::PipelineBindPoint::eGraphics, pso->pipelineLayout, state.bindings, pso->descriptorSetIdxToBindingIdx);
}

if (!state.viewport.viewports.empty() && arraysAreDifferent(state.viewport.viewports, m_CurrentGraphicsState.viewport.viewports))
Expand Down Expand Up @@ -803,7 +767,7 @@ namespace nvrhi::vulkan
{
GraphicsPipeline* pso = checked_cast<GraphicsPipeline*>(m_CurrentGraphicsState.pipeline);

bindBindingSets(vk::PipelineBindPoint::eGraphics, pso->pipelineLayout, m_CurrentGraphicsState.bindings);
bindBindingSets(vk::PipelineBindPoint::eGraphics, pso->pipelineLayout, m_CurrentGraphicsState.bindings, pso->descriptorSetIdxToBindingIdx);

m_AnyVolatileBufferWrites = false;
}
Expand Down
Loading

0 comments on commit a4c6f97

Please sign in to comment.