On 20 Apr 2021, at 19:25, Henri Verbeet hverbeet@gmail.com wrote:
On Tue, 20 Apr 2021 at 15:08, Jan Sikorski jsikorski@codeweavers.com wrote:
@@ -2071,6 +2072,46 @@ static bool wined3d_context_vk_update_graphics_pipeline_key(struct wined3d_conte } }
if (attribute_count < signature->element_count){I don't think that check is safe. Specifically, there may be multiple input signature elements for the same input register. (E.g., mapping v0.xy to "COLOUR0", and v0.zw to "COLOUR1".)
for (i = 0; i < signature->element_count; ++i){struct wined3d_shader_signature_element *element = &signature->elements[i];uint32_t location = element->register_idx;VkVertexInputAttributeDescription *a;if ((stream_info.use_map & (1 << location)))continue;a = &key->attributes[attribute_count++];a->location = location;a->binding = null_binding;a->format = VK_FORMAT_R32G32B32A32_UINT;a->offset = 0;}Similarly, this may end up creating multiple attribute descriptions for the same input location if we don't keep track of which ones we've already processed here. We should probably skip elements with "sysval_semantic" set, although I wouldn't expect anything bad to happen if we don't.
Somewhat similar to my comment to patch 4/5, we could alternatively use the "shader->reg_maps.input_registers" bitmap by doing something like "uint32_t map = vs->reg_maps.input_registers & ~stream_info.use_map;", and then iterating over that map.
Right, thanks for pointing that out. Another thing I noticed is that I should set the attribute format according to component type, or else the validation layer complains about mismatch. Luckily that’s straightforward and I don’t think I need to match number of components. There’s also an issue that wined3d_context_vk_bind_vertex_buffers() only gets called if STATE_STREAMSRC is dirty, while now a pipeline change might necessitate adding the null binding. Calling it each time the pipeline changes would be wasteful. It also smells fishy to invalidate STATE_STREAMSRC inside wined3d_context_vk_update_graphics_pipeline_key for that case, although I guess it would work. I can think of a few ways to resolve it, my favourite is to directly bind the null buffer next to vkCmdBindPipeline() if we need one (and perhaps if STATE_STREAMSRC is clean). We’d probably still need to do the thing in wined3d_context_vk_bind_vertex_buffers() in case someone binds a NULL vertex buffer. Although looks like now if STATE_STREAMSRC is dirty we always go to vkCmdBindPipeline(), so maybe not, but that's a bit brittle.
- Jan