From cce8e60a310f032aeccf4139d87f2ea3514b273b Mon Sep 17 00:00:00 2001 From: Konstantin Seurer Date: Fri, 19 Aug 2022 15:28:04 +0200 Subject: [PATCH] radv: Proper handling for inactive instance nodes We only skipped the `accelerationStructureReference == 0` case which could lead to reading uninitialized memory. This patch introduces a NULL_NODE_ID constant and uses it to handle inactive nodes properly. I do not think, that this case is common in practice. Signed-off-by: Konstantin Seurer Reviewed-by: Friedrich Vock Part-of: --- src/amd/vulkan/bvh/build_helpers.h | 2 ++ src/amd/vulkan/bvh/internal.comp | 15 ++++++++++---- src/amd/vulkan/bvh/leaf.comp | 15 ++++++++------ src/amd/vulkan/bvh/morton.comp | 32 ++++++++++++++++++++---------- 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/amd/vulkan/bvh/build_helpers.h b/src/amd/vulkan/bvh/build_helpers.h index 5147a455dd7..3c82e6d608f 100644 --- a/src/amd/vulkan/bvh/build_helpers.h +++ b/src/amd/vulkan/bvh/build_helpers.h @@ -259,6 +259,8 @@ pack_node_id(uint32_t offset, uint32_t type) return (offset >> 3) | type; } +#define NULL_NODE_ID 0xFFFFFFFF + AABB calculate_node_bounds(VOID_REF bvh, uint32_t id) { diff --git a/src/amd/vulkan/bvh/internal.comp b/src/amd/vulkan/bvh/internal.comp index f9d9375fe57..8d54bfe556e 100644 --- a/src/amd/vulkan/bvh/internal.comp +++ b/src/amd/vulkan/bvh/internal.comp @@ -64,6 +64,7 @@ main(void) total_bounds.min = vec3(INFINITY); total_bounds.max = vec3(-INFINITY); + bool is_active = false; for (uint32_t i = 0; i < 4; i++) { AABB bounds; bounds.min = vec3(NAN); @@ -71,14 +72,17 @@ main(void) uint32_t child_id = DEREF(INDEX(key_id_pair, args.src_ids, src_index + i)).id; - if (i < child_count) { - DEREF(dst_node).children[i] = child_id; - + if (i < child_count && child_id != NULL_NODE_ID) { bounds = calculate_node_bounds(args.bvh, child_id); total_bounds.min = min(total_bounds.min, bounds.min); total_bounds.max = max(total_bounds.max, bounds.max); + is_active = true; + } else { + child_id = NULL_NODE_ID; } + DEREF(dst_node).children[i] = child_id; + DEREF(dst_node).coords[i][0][0] = bounds.min.x; DEREF(dst_node).coords[i][0][1] = bounds.min.y; DEREF(dst_node).coords[i][0][2] = bounds.min.z; @@ -88,7 +92,10 @@ main(void) } uint32_t node_id = pack_node_id(dst_offset, radv_bvh_node_internal); - DEREF(INDEX(key_id_pair, args.dst_ids, global_id)).id = node_id; + /* An internal node is considered inactive if it has no children. Set the resulting scratch node + * id to NULL_NODE_ID for more internal nodes to become inactive. + */ + DEREF(INDEX(key_id_pair, args.dst_ids, global_id)).id = is_active ? node_id : NULL_NODE_ID; if (fill_header) { REF(radv_accel_struct_header) header = REF(radv_accel_struct_header)(args.bvh); diff --git a/src/amd/vulkan/bvh/leaf.comp b/src/amd/vulkan/bvh/leaf.comp index 4c5930bc6a9..3badc1bd1ae 100644 --- a/src/amd/vulkan/bvh/leaf.comp +++ b/src/amd/vulkan/bvh/leaf.comp @@ -198,14 +198,15 @@ struct AccelerationStructureInstance { }; TYPE(AccelerationStructureInstance, 64); -void +/* Returns whether the instance is active. */ +bool build_instance(inout AABB bounds, VOID_REF src_ptr, VOID_REF dst_ptr, uint32_t global_id) { REF(radv_bvh_instance_node) node = REF(radv_bvh_instance_node)(dst_ptr); AccelerationStructureInstance instance = DEREF(REF(AccelerationStructureInstance)(src_ptr)); if (instance.accelerationStructureReference == 0) - return; + return false; mat4 transform = mat4(1.0); for (uint32_t col = 0; col < 4; col++) @@ -251,6 +252,8 @@ build_instance(inout AABB bounds, VOID_REF src_ptr, VOID_REF dst_ptr, uint32_t g DEREF(node).aabb[1][0] = bounds.max.x; DEREF(node).aabb[1][1] = bounds.max.y; DEREF(node).aabb[1][2] = bounds.max.z; + + return true; } void @@ -275,12 +278,10 @@ main(void) } uint32_t dst_offset = args.dst_offset + global_id * dst_stride; - - DEREF(id_ptr).id = pack_node_id(dst_offset, node_type); - VOID_REF dst_ptr = OFFSET(args.bvh, dst_offset); AABB bounds; + bool is_active = true; if (args.geometry_type == VK_GEOMETRY_TYPE_TRIANGLES_KHR) { triangle_indices indices = load_indices(args.indices, args.index_format, global_id); @@ -339,9 +340,11 @@ main(void) src_ptr = DEREF(REF(VOID_REF)(src_ptr)); } - build_instance(bounds, src_ptr, dst_ptr, global_id); + is_active = build_instance(bounds, src_ptr, dst_ptr, global_id); } + DEREF(id_ptr).id = is_active ? pack_node_id(dst_offset, node_type) : NULL_NODE_ID; + min_float_emulated(INDEX(int32_t, args.bounds, 0), bounds.min.x); min_float_emulated(INDEX(int32_t, args.bounds, 1), bounds.min.y); min_float_emulated(INDEX(int32_t, args.bounds, 2), bounds.min.z); diff --git a/src/amd/vulkan/bvh/morton.comp b/src/amd/vulkan/bvh/morton.comp index b68a44c6ad7..1f8c1c36609 100644 --- a/src/amd/vulkan/bvh/morton.comp +++ b/src/amd/vulkan/bvh/morton.comp @@ -72,18 +72,28 @@ main(void) REF(key_id_pair) key_id = INDEX(key_id_pair, args.ids, global_id); uint32_t id = DEREF(key_id).id; - AABB bounds = calculate_node_bounds(args.bvh, id); - vec3 center = (bounds.min + bounds.max) * 0.5; - AABB bvh_bounds; - bvh_bounds.min.x = load_minmax_float_emulated(VOID_REF(args.bounds)); - bvh_bounds.min.y = load_minmax_float_emulated(OFFSET(args.bounds, 4)); - bvh_bounds.min.z = load_minmax_float_emulated(OFFSET(args.bounds, 8)); - bvh_bounds.max.x = load_minmax_float_emulated(OFFSET(args.bounds, 12)); - bvh_bounds.max.y = load_minmax_float_emulated(OFFSET(args.bounds, 16)); - bvh_bounds.max.z = load_minmax_float_emulated(OFFSET(args.bounds, 20)); + uint32_t key; + if (id != NULL_NODE_ID) { + AABB bounds = calculate_node_bounds(args.bvh, id); + vec3 center = (bounds.min + bounds.max) * 0.5; - vec3 normalized_center = (center - bvh_bounds.min) / (bvh_bounds.max - bvh_bounds.min); + AABB bvh_bounds; + bvh_bounds.min.x = load_minmax_float_emulated(VOID_REF(args.bounds)); + bvh_bounds.min.y = load_minmax_float_emulated(OFFSET(args.bounds, 4)); + bvh_bounds.min.z = load_minmax_float_emulated(OFFSET(args.bounds, 8)); + bvh_bounds.max.x = load_minmax_float_emulated(OFFSET(args.bounds, 12)); + bvh_bounds.max.y = load_minmax_float_emulated(OFFSET(args.bounds, 16)); + bvh_bounds.max.z = load_minmax_float_emulated(OFFSET(args.bounds, 20)); - DEREF(key_id).key = lbvh_key(normalized_center.x, normalized_center.y, normalized_center.z); + vec3 normalized_center = (center - bvh_bounds.min) / (bvh_bounds.max - bvh_bounds.min); + + key = lbvh_key(normalized_center.x, normalized_center.y, normalized_center.z); + } else { + /* Move null instances to the end to avoid mixing null instances with active instances. This + * way, we can skip early during traversal. + */ + key = 0xFFFFFFFF; + } + DEREF(key_id).key = key; }