NVIDIAGameWorks / nvrhi

MIT License
846 stars 97 forks source link

Shader resource arrays #29

Open h-mathias opened 1 year ago

h-mathias commented 1 year ago

It seems that currently arrays of resources are not possible. E.g mipmapgen_cs from donut causes an validation error (Vulkan): Shader expects at least 13 descriptors for binding 0.384 but only 1 provided The Vulkan spec states: layout must be consistent with the layout of the compute shader specified in stage.
While it works on both Vulkan and DX it might be then driver related (RTX 3080). I am not too familiar with the DX implementation but it could be correct as it groups same resources? For Vulkan this might be hard to support without providing the concept of arrays in layout/bindingset items.

apanteleev commented 1 year ago

Thanks for pointing this out... it's a difficult topic. Descriptor arrays are unique to Vulkan and, as you noted, currently not supported in regular binding sets.

It is possible to make a descriptor array using IBindlessLayout and IDescriptorTable but that a) is ugly, b) requires manual resource liveness and state tracking, c) requires a separate code path on DX11 at least.

I'm not sure why using a descriptor array in the shader but separate bindings on the host side works, but it does work on both NV and AMD hardware. It's possible to silence the validation layer by using an unsized array in the shader, which is what I've just done to the mipmapgen pass. Looks like nobody has tried to use that pass on Vulkan before, because there were more bugs in the implementation.

Another possible fix for this pass specifically would be to use -fspv-flatten-resource-arrays argument for DXC, but that's generally not a good idea for new shaders, and NVRHI's shader compiler doesn't support specifying custom compiler arguments per-shader and per-target.

Ideally, of course, descriptor arrays should be supported by NVRHI, but that is not trivial to implement. Issues start with there being not enough unused bits in BindingSetItem to specify array index, so the struct would have to be expanded and the nice alignment would break. Then the NVRHI validation layer needs to be extended to support these arrays to make sure that all array items are populated by a binding set. DX11 and DX12 do not support arrays, so I guess the implementation for these platforms would be a trivial reject, and the Vulkan implementation is pretty straightforward.

Alternatively, maybe introduce a DescriptorArray type resource that would just contain an array of BindingSetItems, and then point to that resource from a top-level BindingSetItem when creating the binding set? This avoids the need to extend BindingSetItem and makes validation a bit easier, I think.

h-mathias commented 1 year ago

The unsized array for Vulkan seems to be a good workaround but in long term the DescriptorArray type would be very helpful. Did not find a lot of shaders which use this feature so the priority is not that high in my opinion.