Skip to content

Commit

Permalink
vvl: Rework accel structs mem aliasing validation
Browse files Browse the repository at this point in the history
  • Loading branch information
@arno-lunarg
arno-lunarg committed Feb 20, 2025
1 parent 087c22a commit 3856539
Show file tree
Hide file tree
Showing 9 changed files with 279 additions and 96 deletions.
260 changes: 206 additions & 54 deletions layers/core_checks/cc_ray_tracing.cpp
View file

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions layers/core_checks/core_validation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1689,9 +1689,9 @@ class CoreChecks : public vvl::Device {
bool ValidateAccelerationStructuresMemoryAlisasing(const LogObjectList& objlist, uint32_t infoCount,
const VkAccelerationStructureBuildGeometryInfoKHR* pInfos, uint32_t info_i,
const ErrorObject& error_obj) const;
bool ValidateAccelerationStructuresDeviceScratchBufferMemoryAlisasing(
const LogObjectList& objlist, uint32_t info_count, const VkAccelerationStructureBuildGeometryInfoKHR* p_infos,
uint32_t info_i, const VkAccelerationStructureBuildRangeInfoKHR* const* pp_range_infos, const ErrorObject& error_obj) const;
bool ValidateAccelerationStructuresDeviceScratchBufferMemoryAliasing(
VkCommandBuffer cmd_buffer, uint32_t info_count, const VkAccelerationStructureBuildGeometryInfoKHR* p_infos,
const VkAccelerationStructureBuildRangeInfoKHR* const* pp_range_infos, const ErrorObject& error_obj) const;
bool PreCallValidateCmdBuildAccelerationStructuresKHR(VkCommandBuffer commandBuffer, uint32_t infoCount,
const VkAccelerationStructureBuildGeometryInfoKHR* pInfos,
const VkAccelerationStructureBuildRangeInfoKHR* const* ppBuildRangeInfos,
Expand Down
2 changes: 1 addition & 1 deletion layers/gpuav/core/gpuav_setup.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ std::shared_ptr<vvl::Sampler> Validator::CreateSamplerState(VkSampler handle, co
std::shared_ptr<vvl::AccelerationStructureKHR> Validator::CreateAccelerationStructureState(
VkAccelerationStructureKHR handle, const VkAccelerationStructureCreateInfoKHR *create_info,
std::shared_ptr<vvl::Buffer> &&buf_state) {
return std::make_shared<AccelerationStructureKHR>(handle, create_info, std::move(buf_state), *desc_heap_);
return std::make_shared<AccelerationStructureKHR>(device, handle, create_info, std::move(buf_state), *desc_heap_);
}

std::shared_ptr<vvl::DescriptorSet> Validator::CreateDescriptorSet(VkDescriptorSet handle, vvl::DescriptorPool *pool,
Expand Down
5 changes: 3 additions & 2 deletions layers/gpuav/resources/gpuav_state_trackers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,10 @@ void Sampler::NotifyInvalidate(const NodeList &invalid_nodes, bool unlink) {
vvl::Sampler::NotifyInvalidate(invalid_nodes, unlink);
}

AccelerationStructureKHR::AccelerationStructureKHR(VkAccelerationStructureKHR as, const VkAccelerationStructureCreateInfoKHR *ci,
AccelerationStructureKHR::AccelerationStructureKHR(VkDevice device, VkAccelerationStructureKHR as,
const VkAccelerationStructureCreateInfoKHR *ci,
std::shared_ptr<vvl::Buffer> &&buf_state, DescriptorHeap &desc_heap_)
: vvl::AccelerationStructureKHR(as, ci, std::move(buf_state)),
: vvl::AccelerationStructureKHR(device, as, ci, std::move(buf_state)),
desc_heap(desc_heap_),
id(desc_heap.NextId(VulkanTypedHandle(as, kVulkanObjectTypeAccelerationStructureKHR))) {}

Expand Down
2 changes: 1 addition & 1 deletion layers/gpuav/resources/gpuav_state_trackers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ class Sampler : public vvl::Sampler {

class AccelerationStructureKHR : public vvl::AccelerationStructureKHR {
public:
AccelerationStructureKHR(VkAccelerationStructureKHR as, const VkAccelerationStructureCreateInfoKHR *ci,
AccelerationStructureKHR(VkDevice device, VkAccelerationStructureKHR as, const VkAccelerationStructureCreateInfoKHR *ci,
std::shared_ptr<vvl::Buffer> &&buf_state, DescriptorHeap &desc_heap_);

void Destroy() final;
Expand Down
13 changes: 10 additions & 3 deletions layers/state_tracker/ray_tracing_state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,16 @@ class AccelerationStructureNV : public Bindable {

class AccelerationStructureKHR : public StateObject {
public:
AccelerationStructureKHR(VkAccelerationStructureKHR handle, const VkAccelerationStructureCreateInfoKHR *pCreateInfo,
std::shared_ptr<Buffer> &&buf_state)
AccelerationStructureKHR(VkDevice device, VkAccelerationStructureKHR handle,
const VkAccelerationStructureCreateInfoKHR *pCreateInfo, std::shared_ptr<Buffer> &&buf_state)
: StateObject(handle, kVulkanObjectTypeAccelerationStructureKHR),
safe_create_info(pCreateInfo),
create_info(*safe_create_info.ptr()),
buffer_state(buf_state) {}
buffer_state(buf_state) {
VkAccelerationStructureDeviceAddressInfoKHR device_addr_info = vku::InitStructHelper();
device_addr_info.accelerationStructure = handle;
device_address = DispatchGetAccelerationStructureDeviceAddressKHR(device, &device_addr_info);
}
AccelerationStructureKHR(const AccelerationStructureKHR &rh_obj) = delete;

virtual ~AccelerationStructureKHR() {
Expand Down Expand Up @@ -120,10 +124,13 @@ class AccelerationStructureKHR : public StateObject {
}
}

vvl::range<VkDeviceAddress> GetDeviceAddressRange() const { return {device_address, device_address + safe_create_info.size}; }

const vku::safe_VkAccelerationStructureCreateInfoKHR safe_create_info;
const VkAccelerationStructureCreateInfoKHR &create_info;

uint64_t opaque_handle = 0;
VkDeviceAddress device_address = 0;
std::shared_ptr<vvl::Buffer> buffer_state{};
std::optional<vku::safe_VkAccelerationStructureBuildGeometryInfoKHR> build_info_khr{};
std::vector<VkAccelerationStructureBuildRangeInfoKHR> build_range_infos{};
Expand Down
17 changes: 16 additions & 1 deletion layers/state_tracker/state_tracker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,21 @@ std::shared_ptr<Buffer> Device::CreateBufferState(VkBuffer handle, const VkBuffe
return std::make_shared<Buffer>(*this, handle, create_info);
}

void Device::PreCallRecordCreateBuffer(VkDevice device, const VkBufferCreateInfo *pCreateInfo,
const VkAllocationCallbacks *pAllocator, VkBuffer *pBuffer, const RecordObject &record_obj,
chassis::CreateBuffer &chassis_state) {
if (pCreateInfo->usage & VK_BUFFER_USAGE_ACCELERATION_STRUCTURE_STORAGE_BIT_KHR) {
// When it comes to validation acceleration memory overlaps, it is much faster to
// work on device address ranges directly, but for that to be possible,
// buffers used to back acceleration structures must have been created with the
// VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT usage flag
// => Enforce it.
// Doing so will not modify VVL state tracking, and if the application forgot to set
// this flag, it will still be detected.
chassis_state.modified_create_info.usage |= VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT;
}
}

void Device::PostCallRecordCreateBuffer(VkDevice device, const VkBufferCreateInfo *pCreateInfo,
const VkAllocationCallbacks *pAllocator, VkBuffer *pBuffer,
const RecordObject &record_obj) {
Expand Down Expand Up @@ -2233,7 +2248,7 @@ void Device::PostCallRecordCreateAccelerationStructureNV(VkDevice device, const
std::shared_ptr<AccelerationStructureKHR> Device::CreateAccelerationStructureState(
VkAccelerationStructureKHR handle, const VkAccelerationStructureCreateInfoKHR *create_info,
std::shared_ptr<Buffer> &&buf_state) {
return std::make_shared<AccelerationStructureKHR>(handle, create_info, std::move(buf_state));
return std::make_shared<AccelerationStructureKHR>(device, handle, create_info, std::move(buf_state));
}

void Device::PostCallRecordCreateAccelerationStructureKHR(VkDevice device, const VkAccelerationStructureCreateInfoKHR *pCreateInfo,
Expand Down
3 changes: 3 additions & 0 deletions layers/state_tracker/state_tracker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,9 @@ class Device : public vvl::base::Device {
const RecordObject& record_obj) override;

virtual std::shared_ptr<vvl::Buffer> CreateBufferState(VkBuffer handle, const VkBufferCreateInfo* create_info);
void PreCallRecordCreateBuffer(VkDevice device, const VkBufferCreateInfo* pCreateInfo, const VkAllocationCallbacks* pAllocator,
VkBuffer* pBuffer, const RecordObject& record_obj,
chassis::CreateBuffer& chassis_state) override;
void PostCallRecordCreateBuffer(VkDevice device, const VkBufferCreateInfo* pCreateInfo, const VkAllocationCallbacks* pAllocator,
VkBuffer* pBuffer, const RecordObject& record_obj) override;
void PreCallRecordDestroyBuffer(VkDevice device, VkBuffer buffer, const VkAllocationCallbacks* pAllocator,
Expand Down
67 changes: 36 additions & 31 deletions tests/unit/ray_tracing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1861,11 +1861,12 @@ TEST_F(NegativeRayTracing, AccelerationStructuresOverlappingMemory) {
build_infos.emplace_back(std::move(blas));
}

// Since all the destination acceleration structures are bound to the same memory, 03702 will be triggered for each pair of
// elements in `build_infos`
for (size_t i = 0; i < binom<size_t>(build_info_count, 2); ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702");
}
// Since all the destination acceleration structures are bound to the same memory, 03702 should be triggered for each pair
// of elements in `build_infos`
// => due to validation code optimisations, not *all* overlaps will be detected,
// but if there is *at least one*, it will *always+ be detected.
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702");
m_command_buffer.Begin();
vkt::as::BuildAccelerationStructuresKHR(m_command_buffer.handle(), build_infos);
m_command_buffer.End();
Expand Down Expand Up @@ -1910,12 +1911,15 @@ TEST_F(NegativeRayTracing, AccelerationStructuresOverlappingMemory) {
blas_vec.emplace_back(std::move(blas));
}

// Since all the source and destination acceleration structures are bound to the same memory, 03701 and 03702 will be
// Since all the source and destination acceleration structures are bound to the same memory, 03701 and 03702 should be
// triggered for each pair of elements in `build_infos`, and 03668 for each element
for (size_t i = 0; i < binom<size_t>(build_info_count, 2); ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03701");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702");
}
// => due to validation code optimisations, not *all* overlaps described by 03701 and 03702 will be detected,
// but if there is *at least one*, it will *always+ be detected.
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03701");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03701");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702");

for (size_t i = 0; i < build_info_count; ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-pInfos-03668");
}
Expand Down Expand Up @@ -1976,9 +1980,10 @@ TEST_F(NegativeRayTracing, AccelerationStructuresOverlappingMemory2) {

// Since all the scratch buffers are bound to the same memory, 03704 will be triggered for each pair of elements in
// `build_infos`
for (size_t i = 0; i < binom<size_t>(build_info_count, 2); ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
}
// => due to validation code optimisations, not *all* overlaps will be detected,
// but if there is *at least one*, it will *always+ be detected.
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
m_command_buffer.Begin();
vkt::as::BuildAccelerationStructuresKHR(m_command_buffer.handle(), build_infos);
m_command_buffer.End();
Expand Down Expand Up @@ -2044,15 +2049,15 @@ TEST_F(NegativeRayTracing, AccelerationStructuresOverlappingMemory3) {
}

// Since all the destination acceleration structures and scratch buffers are bound to the same memory, 03702, 03703 and
// 03704 will be triggered for each pair of elements in `build_infos`. 03703 will also be triggered for individual elements.
for (size_t i = 0; i < binom<size_t>(build_info_count, 2); ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03703");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
}
for (size_t i = 0; i < build_info_count; ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03703");
}
// 03704 *should* be triggered for each pair of elements in `build_infos`. 03703 *should* also be triggered for individual
// elements.
// => due to validation code optimisations, not *all* overlaps will be detected,
// but if there is *at least one*, it will *always+ be detected.
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03703");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03703");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03703");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
m_command_buffer.Begin();
vkt::as::BuildAccelerationStructuresKHR(m_command_buffer.handle(), blas_vec);
m_command_buffer.End();
Expand Down Expand Up @@ -2134,15 +2139,15 @@ TEST_F(NegativeRayTracing, AccelerationStructuresOverlappingMemory4) {
blas_vec.emplace_back(std::move(blas));
}

// Since all the source and destination acceleration structures are bound to the same memory, 03704 and 03705 will be
// triggered for each pair of elements in `build_infos`. 03705 will also be triggered for individual elements.
for (size_t i = 0; i < binom<size_t>(build_info_count, 2); ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03705");
}
for (size_t i = 0; i < build_info_count; ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03705");
}
// Since all the source and destination acceleration structures are bound to the same memory, 03704 and 03705 *should* be
// triggered for each pair of elements in `build_infos`. 03705 *should* also be triggered for individual elements.
// => due to validation code optimisations, not *all* overlaps will be detected,
// but if there is *at least one*, it will *always+ be detected.
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03705");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03705");
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03705");

m_command_buffer.Begin();
vkt::as::BuildAccelerationStructuresKHR(m_command_buffer.handle(), blas_vec);
Expand Down

0 comments on commit 3856539

Please sign in to comment.