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

gpu: Check index buffer contents in pre-draw validation #8489

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

gpu: Check index buffer contents in pre-draw validation #8489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84b847a
gpu: Check index buffer contents in pre-draw validation
jeremyg-lunarg Aug 2, 2024
e566158
gpu: Fixes
arno-lunarg Sep 10, 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
14 changes: 12 additions & 2 deletions BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,18 @@ vvl_sources = [
"layers/vulkan/generated/cmd_validation_copy_buffer_to_image_comp.h",
"layers/vulkan/generated/cmd_validation_dispatch_comp.cpp",
"layers/vulkan/generated/cmd_validation_dispatch_comp.h",
"layers/vulkan/generated/cmd_validation_draw_vert.cpp",
"layers/vulkan/generated/cmd_validation_draw_vert.h",
"layers/vulkan/generated/cmd_validation_count_buffer_vert.h",
"layers/vulkan/generated/cmd_validation_count_buffer_vert.cpp",
"layers/vulkan/generated/cmd_validation_first_instance_vert.h",
"layers/vulkan/generated/cmd_validation_first_instance_vert.cpp",
"layers/vulkan/generated/cmd_validation_draw_indexed_vert.h",
"layers/vulkan/generated/cmd_validation_draw_indexed_vert.cpp",
"layers/vulkan/generated/cmd_validation_draw_indexed_indirect_index_buffer_vert.h",
"layers/vulkan/generated/cmd_validation_draw_indexed_indirect_index_buffer_vert.cpp",
"layers/vulkan/generated/cmd_validation_draw_indexed_indirect_vertex_buffer_vert.h",
"layers/vulkan/generated/cmd_validation_draw_indexed_indirect_vertex_buffer_vert.cpp",
"layers/vulkan/generated/cmd_validation_draw_mesh_indirect_vert.h",
"layers/vulkan/generated/cmd_validation_draw_mesh_indirect_vert.cpp",
"layers/vulkan/generated/cmd_validation_trace_rays_rgen.cpp",
"layers/vulkan/generated/cmd_validation_trace_rays_rgen.h",
"layers/vulkan/generated/command_validation.cpp",
Expand Down
14 changes: 12 additions & 2 deletions layers/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,18 @@ target_sources(vvl PRIVATE
${API_TYPE}/generated/cmd_validation_copy_buffer_to_image_comp.cpp
${API_TYPE}/generated/cmd_validation_dispatch_comp.h
${API_TYPE}/generated/cmd_validation_dispatch_comp.cpp
${API_TYPE}/generated/cmd_validation_draw_vert.h
${API_TYPE}/generated/cmd_validation_draw_vert.cpp
${API_TYPE}/generated/cmd_validation_count_buffer_vert.h
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
${API_TYPE}/generated/cmd_validation_count_buffer_vert.h
${API_TYPE}/generated/cmd_validation_count_buffer_vert.h

Copy link
Contributor

Choose a reason for hiding this comment

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

@arno-lunarg still need to fix this

${API_TYPE}/generated/cmd_validation_count_buffer_vert.cpp
${API_TYPE}/generated/cmd_validation_first_instance_vert.h
${API_TYPE}/generated/cmd_validation_first_instance_vert.cpp
${API_TYPE}/generated/cmd_validation_draw_indexed_vert.h
${API_TYPE}/generated/cmd_validation_draw_indexed_vert.cpp
${API_TYPE}/generated/cmd_validation_draw_indexed_indirect_index_buffer_vert.h
${API_TYPE}/generated/cmd_validation_draw_indexed_indirect_index_buffer_vert.cpp
${API_TYPE}/generated/cmd_validation_draw_indexed_indirect_vertex_buffer_vert.h
${API_TYPE}/generated/cmd_validation_draw_indexed_indirect_vertex_buffer_vert.cpp
${API_TYPE}/generated/cmd_validation_draw_mesh_indirect_vert.h
${API_TYPE}/generated/cmd_validation_draw_mesh_indirect_vert.cpp
${API_TYPE}/generated/cmd_validation_trace_rays_rgen.h
${API_TYPE}/generated/cmd_validation_trace_rays_rgen.cpp
gpu/core/gpu_settings.h
Expand Down
20 changes: 20 additions & 0 deletions layers/VkLayer_khronos_validation.json.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,26 @@
}
]
}
},
{
"key": "gpuav_index_buffers",
"label": "Index buffers",
"type": "BOOL",
"default": true,
"description": "Validate that indexed draws do not fetch indices outside of the bounds of the index buffer. Also validates that those indices are not out of the bounds of the fetched vertex buffers.",
"platforms": [
"WINDOWS",
"LINUX"
],
"dependence": {
"mode": "ALL",
"settings": [
{
"key": "gpuav_buffers_validation",
"value": true
}
]
}
}
]
},
Expand Down
47 changes: 35 additions & 12 deletions layers/gpu/cmd_validation/gpuav_cmd_validation_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@
#include "gpu/shaders/gpu_shaders_constants.h"

#include "state_tracker/descriptor_sets.h"
#include "state_tracker/render_pass_state.h"
#include "state_tracker/shader_object_state.h"

namespace gpuav {

void BindValidationCmdsCommonDescSet(Validator &gpuav, CommandBuffer &cb_state, VkPipelineBindPoint bind_point,
VkPipelineLayout pipeline_layout, uint32_t cmd_index, uint32_t error_logger_index) {
void BindErrorLoggingDescriptorSet(Validator &gpuav, CommandBuffer &cb_state, VkPipelineBindPoint bind_point,
VkPipelineLayout pipeline_layout, uint32_t cmd_index, uint32_t error_logger_index) {
assert(cmd_index < cst::indices_count);
assert(error_logger_index < cst::indices_count);
std::array<uint32_t, 2> dynamic_offsets = {
Expand All @@ -38,8 +39,7 @@ void BindValidationCmdsCommonDescSet(Validator &gpuav, CommandBuffer &cb_state,
dynamic_offsets.data());
}

void RestorablePipelineState::Create(vvl::CommandBuffer &cb_state, VkPipelineBindPoint bind_point) {
cmd_buffer_ = cb_state.VkHandle();
void RestorablePipelineState::Create(CommandBuffer &cb_state, VkPipelineBindPoint bind_point) {
pipeline_bind_point_ = bind_point;
const auto lv_bind_point = ConvertToLvlBindPoint(bind_point);

Expand Down Expand Up @@ -78,11 +78,29 @@ void RestorablePipelineState::Create(vvl::CommandBuffer &cb_state, VkPipelineBin
if (last_bound.push_descriptor_set) {
push_descriptor_set_writes_ = last_bound.push_descriptor_set->GetWrites();
}

// Do not handle cb_state.activeRenderPass->use_dynamic_rendering_inherited for now
if (bind_point == VK_PIPELINE_BIND_POINT_GRAPHICS && cb_state.activeRenderPass->use_dynamic_rendering) {
rendering_info_ = &cb_state.activeRenderPass->dynamic_rendering_begin_rendering_info;
DispatchCmdEndRendering(cb_state.VkHandle());

VkRenderingInfo rendering_info = vku::InitStructHelper();
rendering_info.renderArea = {{0, 0}, {1, 1}};
rendering_info.layerCount = 1;
rendering_info.viewMask = 0;
rendering_info.colorAttachmentCount = 0;
DispatchCmdBeginRendering(cb_state.VkHandle(), &rendering_info);
}
}

void RestorablePipelineState::Restore() const {
if (rendering_info_) {
DispatchCmdEndRendering(cb_state_.VkHandle());
Copy link
Contributor

Choose a reason for hiding this comment

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

discussing with driver people

  1. This will trash runtime speed on some drivers if we are going
vkCmdBeginRendering();
vkCmdDrawIndexed();
vkCmdDrawIndexed();
vkCmdDrawIndexed();
vkCmdDrawIndexed();
vkCmdDrawIndexed();
vkCmdEndRendering():

too

vkCmdBeginRendering();
vkCmdDrawIndexed();
vkCmdEndRendering():
vkCmdBeginRendering();
vkCmdDrawIndexed();
vkCmdEndRendering():
vkCmdBeginRendering();
vkCmdDrawIndexed();
vkCmdEndRendering():
vkCmdBeginRendering();
vkCmdDrawIndexed();
vkCmdEndRendering():
vkCmdBeginRendering();
vkCmdDrawIndexed();
vkCmdEndRendering():
  1. There might be dependency (via VK_KHR_dynamic_rendering_local_read) that are going to just blow up if we don't chain it for them

DispatchCmdBeginRendering(cb_state_.VkHandle(), rendering_info_->ptr());
}

if (pipeline_ != VK_NULL_HANDLE) {
DispatchCmdBindPipeline(cmd_buffer_, pipeline_bind_point_, pipeline_);
DispatchCmdBindPipeline(cb_state_.VkHandle(), pipeline_bind_point_, pipeline_);
}
if (!shader_objects_.empty()) {
std::vector<VkShaderStageFlagBits> stages;
Expand All @@ -91,29 +109,34 @@ void RestorablePipelineState::Restore() const {
stages.emplace_back(shader_obj->create_info.stage);
shaders.emplace_back(shader_obj->VkHandle());
}
DispatchCmdBindShadersEXT(cmd_buffer_, static_cast<uint32_t>(shader_objects_.size()), stages.data(), shaders.data());
DispatchCmdBindShadersEXT(cb_state_.VkHandle(), static_cast<uint32_t>(shader_objects_.size()), stages.data(),
shaders.data());
}

for (std::size_t i = 0; i < descriptor_sets_.size(); i++) {
VkDescriptorSet descriptor_set = descriptor_sets_[i].first;
if (descriptor_set != VK_NULL_HANDLE) {
DispatchCmdBindDescriptorSets(cmd_buffer_, pipeline_bind_point_, desc_set_pipeline_layout_, descriptor_sets_[i].second,
1, &descriptor_set, static_cast<uint32_t>(dynamic_offsets_[i].size()),
dynamic_offsets_[i].data());
DispatchCmdBindDescriptorSets(cb_state_.VkHandle(), pipeline_bind_point_, desc_set_pipeline_layout_,
descriptor_sets_[i].second, 1, &descriptor_set,
static_cast<uint32_t>(dynamic_offsets_[i].size()), dynamic_offsets_[i].data());
}
}

if (!push_descriptor_set_writes_.empty()) {
DispatchCmdPushDescriptorSetKHR(cmd_buffer_, pipeline_bind_point_, desc_set_pipeline_layout_, push_descriptor_set_index_,
static_cast<uint32_t>(push_descriptor_set_writes_.size()),
DispatchCmdPushDescriptorSetKHR(cb_state_.VkHandle(), pipeline_bind_point_, desc_set_pipeline_layout_,
push_descriptor_set_index_, static_cast<uint32_t>(push_descriptor_set_writes_.size()),
reinterpret_cast<const VkWriteDescriptorSet *>(push_descriptor_set_writes_.data()));
}

for (const auto &push_constant_range : push_constants_data_) {
DispatchCmdPushConstants(cmd_buffer_, push_constant_range.layout, push_constant_range.stage_flags,
DispatchCmdPushConstants(cb_state_.VkHandle(), push_constant_range.layout, push_constant_range.stage_flags,
push_constant_range.offset, static_cast<uint32_t>(push_constant_range.values.size()),
push_constant_range.values.data());
}

if (pipeline_bind_point_ == VK_PIPELINE_BIND_POINT_GRAPHICS) {
cb_state_.RestoreDynamicStates(gpuav_);
}
}

} // namespace gpuav
15 changes: 10 additions & 5 deletions layers/gpu/cmd_validation/gpuav_cmd_validation_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,19 @@ class Validator;

class RestorablePipelineState {
public:
RestorablePipelineState(vvl::CommandBuffer& cb_state, VkPipelineBindPoint bind_point) { Create(cb_state, bind_point); }
RestorablePipelineState(const Validator& gpuav, CommandBuffer& cb_state, VkPipelineBindPoint bind_point)
: gpuav_(gpuav), cb_state_(cb_state) {
Create(cb_state, bind_point);
}
~RestorablePipelineState() { Restore(); }

private:
void Create(vvl::CommandBuffer& cb_state, VkPipelineBindPoint bind_point);
void Create(CommandBuffer& cb_state, VkPipelineBindPoint bind_point);
void Restore() const;

VkCommandBuffer cmd_buffer_;
const Validator& gpuav_;
CommandBuffer& cb_state_;
const vku::safe_VkRenderingInfo* rendering_info_ = nullptr;
VkPipelineBindPoint pipeline_bind_point_ = VK_PIPELINE_BIND_POINT_MAX_ENUM;
VkPipeline pipeline_ = VK_NULL_HANDLE;
VkPipelineLayout desc_set_pipeline_layout_ = VK_NULL_HANDLE;
Expand All @@ -46,7 +51,7 @@ class RestorablePipelineState {
std::vector<vvl::ShaderObject*> shader_objects_;
};

void BindValidationCmdsCommonDescSet(Validator& gpuav, CommandBuffer& cb_state, VkPipelineBindPoint bind_point,
VkPipelineLayout pipeline_layout, uint32_t cmd_index, uint32_t error_logger_index);
void BindErrorLoggingDescriptorSet(Validator& gpuav, CommandBuffer& cb_state, VkPipelineBindPoint bind_point,
VkPipelineLayout pipeline_layout, uint32_t cmd_index, uint32_t error_logger_index);

} // namespace gpuav
7 changes: 3 additions & 4 deletions layers/gpu/cmd_validation/gpuav_copy_buffer_to_image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,14 +307,13 @@ void InsertCopyBufferToImageValidation(Validator &gpuav, const Location &loc, Co
DispatchUpdateDescriptorSets(gpuav.device, static_cast<uint32_t>(desc_writes.size()), desc_writes.data(), 0, nullptr);
}
// Save current graphics pipeline state
RestorablePipelineState restorable_state(cb_state, VK_PIPELINE_BIND_POINT_COMPUTE);
RestorablePipelineState restorable_state(gpuav, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE);

// Insert diagnostic dispatch
DispatchCmdBindPipeline(cb_state.VkHandle(), VK_PIPELINE_BIND_POINT_COMPUTE, shared_copy_validation_resources.pipeline);

BindValidationCmdsCommonDescSet(gpuav, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE,
shared_copy_validation_resources.pipeline_layout, 0,
static_cast<uint32_t>(cb_state.per_command_error_loggers.size()));
BindErrorLoggingDescriptorSet(gpuav, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE, shared_copy_validation_resources.pipeline_layout,
0, static_cast<uint32_t>(cb_state.per_command_error_loggers.size()));
DispatchCmdBindDescriptorSets(cb_state.VkHandle(), VK_PIPELINE_BIND_POINT_COMPUTE,
shared_copy_validation_resources.pipeline_layout, glsl::kDiagPerCmdDescriptorSet, 1,
&validation_desc_set, 0, nullptr);
Expand Down
6 changes: 3 additions & 3 deletions layers/gpu/cmd_validation/gpuav_dispatch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ void InsertIndirectDispatchValidation(Validator &gpuav, const Location &loc, Com
DispatchUpdateDescriptorSets(gpuav.device, 1, &desc_write, 0, nullptr);

// Save current graphics pipeline state
RestorablePipelineState restorable_state(cb_state, VK_PIPELINE_BIND_POINT_COMPUTE);
RestorablePipelineState restorable_state(gpuav, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE);

// Insert diagnostic dispatch
if (use_shader_objects) {
Expand All @@ -205,8 +205,8 @@ void InsertIndirectDispatchValidation(Validator &gpuav, const Location &loc, Com
push_constants[3] = static_cast<uint32_t>((indirect_offset / sizeof(uint32_t)));
DispatchCmdPushConstants(cb_state.VkHandle(), shared_dispatch_resources.pipeline_layout, VK_SHADER_STAGE_COMPUTE_BIT, 0,
sizeof(push_constants), push_constants);
BindValidationCmdsCommonDescSet(gpuav, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE, shared_dispatch_resources.pipeline_layout,
cb_state.compute_index, static_cast<uint32_t>(cb_state.per_command_error_loggers.size()));
BindErrorLoggingDescriptorSet(gpuav, cb_state, VK_PIPELINE_BIND_POINT_COMPUTE, shared_dispatch_resources.pipeline_layout,
cb_state.compute_index, static_cast<uint32_t>(cb_state.per_command_error_loggers.size()));
DispatchCmdBindDescriptorSets(cb_state.VkHandle(), VK_PIPELINE_BIND_POINT_COMPUTE, shared_dispatch_resources.pipeline_layout,
glsl::kDiagPerCmdDescriptorSet, 1, &indirect_buffer_desc_set, 0, nullptr);
DispatchCmdDispatch(cb_state.VkHandle(), 1, 1, 1);
Expand Down
Loading