NVK: Misrendering with Civilization 6
Detail
My current focus is on using the Civ6/D3D12 benchmark to better understand interactions between vkd3d and NVK, and have already encountered a bug where not all of the UI elements are consistently drawn (see first screenshot). Every one of the UI elements not drawn has an index > 4096 in the million-element descriptor set used by vkd3d to reference per-object buffers.
After a lot of troubleshooting, I found that the problem lies in the CBUF building code in nvk_nir_lower_descriptors.c
, which is struggling to index the hashmap consistently. This is due to how it builds its keys, using designated initializers without initializing every field. This results in the other fields having indeterminate value (ISO/IEC 9899:2011 §6.7.9 point 9), which triggers UB when the hashmap code attempts to hash them (but in practice, likely just results in many matching keys being incorrectly considered distinct).
There are also some cases where the "designated initializer" syntax is used as part of an assignment, with a cast to coerce the compiler into performing the assignment. I don't believe that this is permitted by C11, and I fear the compiler does not always know the type of the struct being initialized before the cast, resulting in the assignment happening incorrectly.
Applying my patch (see below) that addresses some -- though not all -- of these misuses of designated initializers in nvk_nir_lower_descriptors.c
causes the full UI of Civ6 to be displayed correctly (see second screenshot). There are likely also performance improvements associated with hash keys matching properly as well, but I will have to rerun the full benchmark to determine that.
We should look into fixing the NVK codebase by:
- Ensuring that every designated initializer initializes all fields, explicitly zero-initializing ones that do not need a particular value.
- Removing all instances of
(struct foo) { .field1 = ..., ... }
Files
Patch
diff --git a/src/nouveau/vulkan/nvk_nir_lower_descriptors.c b/src/nouveau/vulkan/nvk_nir_lower_descriptors.c
index a7ba2bf..e916a30 100644
--- a/src/nouveau/vulkan/nvk_nir_lower_descriptors.c
+++ b/src/nouveau/vulkan/nvk_nir_lower_descriptors.c
@@ -71,12 +71,10 @@ record_cbuf_use(const struct nvk_cbuf *key, uint64_t start, uint64_t end,
} else {
struct lower_desc_cbuf *cbuf =
ralloc(ctx->cbufs, struct lower_desc_cbuf);
- *cbuf = (struct lower_desc_cbuf) {
- .key = *key,
- .use_count = 1,
- .start = start,
- .end = end,
- };
+ cbuf->key = *key;
+ cbuf->use_count = 1;
+ cbuf->start = start;
+ cbuf->end = end;
_mesa_hash_table_insert(ctx->cbufs, &cbuf->key, cbuf);
}
}
@@ -104,6 +102,8 @@ record_descriptor_cbuf_use(uint32_t set, uint32_t binding, nir_src *index,
const struct nvk_cbuf key = {
.type = NVK_CBUF_TYPE_DESC_SET,
.desc_set = set,
+ .dynamic_idx = 0,
+ .desc_offset = 0,
};
uint64_t start, end;
@@ -507,6 +507,9 @@ lower_load_constant(nir_builder *b, nir_intrinsic_instr *load,
const struct nvk_cbuf cbuf_key = {
.type = NVK_CBUF_TYPE_SHADER_DATA,
+ .desc_set = 0,
+ .dynamic_idx = 0,
+ .desc_offset = 0,
};
int cbuf_idx = get_mapped_cbuf_idx(&cbuf_key, ctx);
assert(cbuf_idx >= 0);
@@ -633,6 +636,8 @@ load_descriptor(nir_builder *b, unsigned num_components, unsigned bit_size,
const struct nvk_cbuf cbuf_key = {
.type = NVK_CBUF_TYPE_DESC_SET,
.desc_set = set,
+ .dynamic_idx = 0,
+ .desc_offset = 0,
};
int cbuf_idx = get_mapped_cbuf_idx(&cbuf_key, ctx);