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

vvl: Rework accel structs mem aliasing validation #9492

Closed
wants to merge 2 commits into from
Closed

vvl: Rework accel structs mem aliasing validation #9492

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
903121a
vvl: Rework accel structs mem aliasing validation
arno-lunarg Feb 19, 2025
d0effb9
vvl: Debug logs (temporary)
arno-lunarg Feb 20, 2025
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
274 changes: 220 additions & 54 deletions layers/core_checks/cc_ray_tracing.cpp
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions layers/core_checks/core_validation.h
View file
Open in desktop
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
Open in desktop
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>(api_version, 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
Open in desktop
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(APIVersion api_version, 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(api_version, device, as, ci, std::move(buf_state)),
desc_heap(desc_heap_),
id(desc_heap.NextId(VulkanTypedHandle(as, kVulkanObjectTypeAccelerationStructureKHR))) {}

Expand Down
5 changes: 3 additions & 2 deletions layers/gpuav/resources/gpuav_state_trackers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,9 @@ class Sampler : public vvl::Sampler {

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

void Destroy() final;
void NotifyInvalidate(const NodeList &invalid_nodes, bool unlink) final;
Expand Down
40 changes: 37 additions & 3 deletions layers/state_tracker/ray_tracing_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,27 @@ class AccelerationStructureNV : public Bindable {

class AccelerationStructureKHR : public StateObject {
public:
AccelerationStructureKHR(VkAccelerationStructureKHR handle, const VkAccelerationStructureCreateInfoKHR *pCreateInfo,
std::shared_ptr<Buffer> &&buf_state)
AccelerationStructureKHR(APIVersion api_version, 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) {
// If the buffer's device address has not been queried,
// get it here. Since it is used for the purpose of
// validation, do not try to update buffer_state, since
// it only tracks application state.
if (buffer_state && buffer_state->deviceAddress == 0) {
VkBufferDeviceAddressInfo address_info = vku::InitStructHelper();
address_info.buffer = buffer_state->VkHandle();

if (api_version >= VK_API_VERSION_1_2) {
buffer_device_address = DispatchGetBufferDeviceAddress(device, &address_info);
} else {
buffer_device_address = DispatchGetBufferDeviceAddressKHR(device, &address_info);
}
}
}
AccelerationStructureKHR(const AccelerationStructureKHR &rh_obj) = delete;

virtual ~AccelerationStructureKHR() {
Expand Down Expand Up @@ -120,11 +135,30 @@ class AccelerationStructureKHR : public StateObject {
}
}

// Returns the device address range effectively occupied by the acceleration structure,
// as defined by its creation info.
// It does NOT take into account the acceleration structure address as returned by
// vkGetAccelerationStructureDeviceAddress, this address may be at an offset
// of the buffer range backing the acceleration structure
vvl::range<VkDeviceAddress> GetDeviceAddressRange() const {
if (!buffer_state) {
return {};
}
if (buffer_state->deviceAddress != 0) {
return {buffer_state->deviceAddress + safe_create_info.offset,
buffer_state->deviceAddress + safe_create_info.offset + safe_create_info.size};
}
return {buffer_device_address + safe_create_info.offset,
buffer_device_address + safe_create_info.offset + safe_create_info.size};
}

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

uint64_t opaque_handle = 0;
std::shared_ptr<vvl::Buffer> buffer_state{};
VkDeviceAddress buffer_device_address =
0; // Used in case buffer_state->deviceAddress is 0 (happens if app never queried address)
std::optional<vku::safe_VkAccelerationStructureBuildGeometryInfoKHR> build_info_khr{};
std::vector<VkAccelerationStructureBuildRangeInfoKHR> build_range_infos{};
// You can't have is_built == false and a build_info_khr, but you can have is_built == true and no build_info_khr,
Expand Down
17 changes: 16 additions & 1 deletion layers/state_tracker/state_tracker.cpp
View file
Open in desktop
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is playing a bit with fire, and there is a chance this might cause a false positive if we have now altered what the drive reports back and the user will see a different result with and without validation

Another option/idea is if they don't have the VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT flag, we just don't check it.

I guess personally want to be on the side of "no false positive" to trying to validate everything, but at the same time, not sure if this will "save many mistakes" that otherwise would be missed if we don't enforce it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is a compromise, for now I deem it well worth it. VVL will still report an error when application forgot to set this flag, I don't expect major problems around here, but we will see.
It is impossible to not have shader device address when using acceleration structure, at least because you need them for the scratch buffer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this would not be the first time a debug/validation tool modifies the behavior of the targeted app, just look at GPU-AV, we do things far more horrible!

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but this is not GPU-AV and we have had issues where pipeline creation was setting ignored fields to null and caused lots of headaches

basically people don't expect core validation to alter things, just a read only validation... again, should be fine, but will keep our eyes on the lookout for trouble

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I will keep this change in mind should ray tracing weirdness come

// 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>(api_version, 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
Open in desktop
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
3 changes: 1 addition & 2 deletions tests/icd/test_icd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1696,8 +1696,7 @@ static VKAPI_ATTR void VKAPI_CALL GetShaderModuleIdentifierEXT(VkDevice device,

static VKAPI_ATTR VkDeviceAddress VKAPI_CALL
GetAccelerationStructureDeviceAddressKHR(VkDevice device, const VkAccelerationStructureDeviceAddressInfoKHR* pInfo) {
// arbitrary - need to be aligned to 256 bytes
return 0x262144;
return VkDeviceAddress(pInfo->accelerationStructure) << 8u;
}

static VKAPI_ATTR void VKAPI_CALL GetAccelerationStructureBuildSizesKHR(
Expand Down
62 changes: 31 additions & 31 deletions tests/unit/ray_tracing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1834,6 +1834,11 @@ TEST_F(NegativeRayTracing, AccelerationStructuresOverlappingMemory) {
RETURN_IF_SKIP(InitFrameworkForRayTracingTest());
RETURN_IF_SKIP(InitState());

if (IsPlatformMockICD()) {
GTEST_SKIP()
<< "Test needs acceleration structures addresses to be related to the buffer backing them, mock ICD does not do that";
}

constexpr size_t build_info_count = 3;

// All buffers used to back source/destination acceleration structures will be bound to this memory chunk
Expand Down Expand Up @@ -1861,11 +1866,11 @@ 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", 2);
m_command_buffer.Begin();
vkt::as::BuildAccelerationStructuresKHR(m_command_buffer.handle(), build_infos);
m_command_buffer.End();
Expand Down Expand Up @@ -1910,12 +1915,13 @@ 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", 2);
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-dstAccelerationStructure-03702", 2);

for (size_t i = 0; i < build_info_count; ++i) {
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-pInfos-03668");
}
Expand Down Expand Up @@ -1976,9 +1982,9 @@ 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", 2);
m_command_buffer.Begin();
vkt::as::BuildAccelerationStructuresKHR(m_command_buffer.handle(), build_infos);
m_command_buffer.End();
Expand Down Expand Up @@ -2044,15 +2050,12 @@ 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", 3);
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03704", 2);
m_command_buffer.Begin();
vkt::as::BuildAccelerationStructuresKHR(m_command_buffer.handle(), blas_vec);
m_command_buffer.End();
Expand Down Expand Up @@ -2134,15 +2137,12 @@ 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", 2);
m_errorMonitor->SetDesiredError("VUID-vkCmdBuildAccelerationStructuresKHR-scratchData-03705", 3);

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