gpu: Check index buffer contents in pre-draw validation #8374
Conversation
|
CI Vulkan-ValidationLayers build queued with queue ID 232681. |
|
CI Vulkan-ValidationLayers build # 17215 running. |
|
This has many TODOs but works well enough I want a backup just in case my computer gets hit with an EMP |
|
CI Vulkan-ValidationLayers build # 17215 failed. |
jeremyg-lunarg
force-pushed
the
jeremyg-issue-2492
branch
from
c229447
to
366871b
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 232715. |
|
CI Vulkan-ValidationLayers build # 17216 running. |
|
CI Vulkan-ValidationLayers build # 17216 failed. |
This involves splitting up draw.vert into separate shaders for non-indexed indirect, indexed (both indirect and direct) and mesh draw calls. Fixes KhronosGroup#2492
jeremyg-lunarg
force-pushed
the
jeremyg-issue-2492
branch
from
366871b
to
378e450
Compare
|
CI Vulkan-ValidationLayers build queued with queue ID 235968. |
|
CI Vulkan-ValidationLayers build # 17242 running. |
| max_index = (size - offset - attrib.offset - attrib.size) / stride | ||
| */ | ||
| #if 0 | ||
| static uint32_t MinVertexBufferLen(const vvl::CommandBuffer &cb_state, const vvl::Pipeline &pipeline) { |
There was a problem hiding this comment.
This was my WIP attempt to figure out the max vertex buffer length
There was a problem hiding this comment.
Just thought about the case where there is an index buffer but no vertex buffer. I think given how you set min_vertex_count to UINT32_MAX, it is properly handled? Ideally in this case we should not even bother doing validation
There was a problem hiding this comment.
Then you can just turn off kPreDrawSelectIndexBuffer for indirect and skip the diagnostic draw for vkCmdDrawIndexed()
|
CI Vulkan-ValidationLayers build # 17242 failed. |
| gpuav.InternalError(cb_state.Handle(), loc, "Unsupported indexType"); | ||
| return; | ||
| } | ||
| push_constants.vertex_buffer_size = 3; // TODO vertex size calc |
There was a problem hiding this comment.
This is the important TODO, actually send down the max vertex buffer size that will work across all bindings
There was a problem hiding this comment.
If I am not mistaken, multiple vertex buffers can be bound, each being an input to a different location in the shader, so we have to account for that?
There was a problem hiding this comment.
Ah but it seems your WIP is taking care of that!
There was a problem hiding this comment.
Yes but the WIP is handling VK_VERTEX_INPUT_RATE_INSTANCE and the divisor. Those could be split out to a separate value. I think these can be checked on the CPU for direct calls but not for indirect.
| m_commandBuffer->begin(&begin_info); | ||
| m_commandBuffer->BeginRenderPass(m_renderPassBeginInfo); | ||
| vk::CmdBindPipeline(m_commandBuffer->handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, pipe.Handle()); | ||
| const uint32_t kNumVertices = 12; |
There was a problem hiding this comment.
This test makes sure the TODO gets fixed.
|
@arno-lunarg you may want to take a look at the latest push. I fixed up @spencer-lunarg's previous review comments |
| return false; | ||
| } | ||
|
|
||
| for (uint j = 0; j < ic; j++) { |
There was a problem hiding this comment.
I think having one GPU thread check all indices will make things really slow, I am thinking about moving the index check to a dedicated validation vertex shader, and dispatch a draw on it with a number of vertices specified on the CPU for non indirect draws, and computed on the GPU for indirect ones
There was a problem hiding this comment.
Good point. You could just map the index buffer as a vertex buffer and have a separate thread check each index. And have the other checking only happen in thread 0. Or are you thinking about having 2 shaders?
| @@ -38,13 +41,14 @@ struct SharedDrawValidationResources final { | |||
| VkDevice device = VK_NULL_HANDLE; | |||
|
|
|||
| SharedDrawValidationResources(Validator &gpuav, VkDescriptorSetLayout error_output_desc_set_layout, bool use_shader_objects, | |||
| const Location &loc) | |||
| const Location &loc, const uint32_t *shader, uint32_t shader_size) | |||
There was a problem hiding this comment.
I should have added comments, but this will not work as expected x_x
Only one SharedDrawValidationResources object is created, it is then cached and retrieved when needed. So the first call to gpuav.shared_resources_manager.Get<SharedDrawValidationResources>( /*...*/, shader, shader_size); will set the validation shader, and calling it again with a different shader will be a no-op and just retrieve the cached entry......
There was a problem hiding this comment.
The key in the cache is only based on the type of the stored object, ideally I would also use the passed in parameters to form the key, but I don't even know if that is possible
There was a problem hiding this comment.
It doesn't look like it is possible as written. If it was up to me, I'd ditch the typeid magic and just make a key enum.
| if (gl_VertexIndex == 0) { | ||
| uint draw_count = 0; | ||
|
|
||
| if ((push_data.flags & kPreDrawSelectCountBuffer) != 0) { |
There was a problem hiding this comment.
Validation code is now duplicated, I don't think this is ideal?
I really like how you broke up the logic in gpuav_draw.cpp, but here, the old Uber shader was not that bad I think
There was a problem hiding this comment.
The answer to this will depend on how much additional validation needs to happen in each draw type. Also how many push constants become necessary and when that starts to exceeds device limits. It seems to me that at least mesh shaders would benefit from being separate.
There was a problem hiding this comment.
The other reason I split up the shaders was that there are fields with the same name but different offsets in VkDrawIndirectCommand and VkDrawIndexedIndirectCommand. And then VkDrawMeshTasksIndirectCommandEXT is completely different. The old shader code got down the offset of the firstInstance member in a push constant for whatever draw was being validated. But... I wanted to access multiple fields and not deal with the mess of managing different offsets in the same shader.
This involves splitting up draw.vert into separate shaders for non-indexed indirect, indexed (both indirect and direct) and mesh draw calls.
Fixes #2492