From a2538b252020294f2ba04fb95e785929027da219 Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Tue, 7 Jul 2020 08:22:31 +0200 Subject: [PATCH] v3dv: make sure we emit vertex attributes in location order The order in which we emit the attributes is relevant, since GL_SHADER_STATE_ATTRIBUTE_RECORD packets don't include an explicit attribute index. This means that we need to emit them in driver location order, since the compiler uses that location to compute attribute offsets in the VPM. Fixes ~1300 CTS tests in: dEQP-VK.pipeline.vertex_input.multiple_attributes.out_of_order.* Part-of: --- src/broadcom/vulkan/v3dv_cmd_buffer.c | 18 +++++++++++++++--- src/broadcom/vulkan/v3dv_pipeline.c | 10 +++++----- src/broadcom/vulkan/v3dv_private.h | 12 +++--------- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/broadcom/vulkan/v3dv_cmd_buffer.c b/src/broadcom/vulkan/v3dv_cmd_buffer.c index d96df92f2c0..a0da96b648e 100644 --- a/src/broadcom/vulkan/v3dv_cmd_buffer.c +++ b/src/broadcom/vulkan/v3dv_cmd_buffer.c @@ -3443,9 +3443,19 @@ emit_gl_shader_state(struct v3dv_cmd_buffer *cmd_buffer) const uint32_t packet_length = cl_packet_length(GL_SHADER_STATE_ATTRIBUTE_RECORD); - for (uint32_t i = 0; i < pipeline->va_count; i++) { - uint32_t binding = pipeline->va[i].binding; - uint32_t location = pipeline->va[i].driver_location; + uint32_t emitted_va_count = 0; + for (uint32_t i = 0; emitted_va_count < pipeline->va_count; i++) { + assert(i < MAX_VERTEX_ATTRIBS); + + if (pipeline->va[i].vk_format == VK_FORMAT_UNDEFINED) + continue; + + const uint32_t binding = pipeline->va[i].binding; + + /* We store each vertex attribute in the array using its driver location + * as index. + */ + const uint32_t location = i; struct v3dv_vertex_binding *c_vb = &cmd_buffer->state.vertex_bindings[binding]; @@ -3484,6 +3494,8 @@ emit_gl_shader_state(struct v3dv_cmd_buffer *cmd_buffer) attr.maximum_index = 0xffffff; } + + emitted_va_count++; } if (pipeline->va_count == 0) { diff --git a/src/broadcom/vulkan/v3dv_pipeline.c b/src/broadcom/vulkan/v3dv_pipeline.c index aeac572bbb4..df1f244236e 100644 --- a/src/broadcom/vulkan/v3dv_pipeline.c +++ b/src/broadcom/vulkan/v3dv_pipeline.c @@ -2562,12 +2562,12 @@ pipeline_init(struct v3dv_pipeline *pipeline, if (var != NULL) { unsigned driver_location = var->data.driver_location; - pipeline->va[pipeline->va_count].offset = desc->offset; - pipeline->va[pipeline->va_count].binding = desc->binding; - pipeline->va[pipeline->va_count].driver_location = driver_location; - pipeline->va[pipeline->va_count].vk_format = desc->format; + assert(driver_location < MAX_VERTEX_ATTRIBS); + pipeline->va[driver_location].offset = desc->offset; + pipeline->va[driver_location].binding = desc->binding; + pipeline->va[driver_location].vk_format = desc->format; - pack_shader_state_attribute_record(pipeline, pipeline->va_count, desc); + pack_shader_state_attribute_record(pipeline, driver_location, desc); pipeline->va_count++; } diff --git a/src/broadcom/vulkan/v3dv_private.h b/src/broadcom/vulkan/v3dv_private.h index 506aa1d206e..71c25090671 100644 --- a/src/broadcom/vulkan/v3dv_private.h +++ b/src/broadcom/vulkan/v3dv_private.h @@ -1458,19 +1458,13 @@ struct v3dv_pipeline { uint32_t vb_count; /* Note that a lot of info from VkVertexInputAttributeDescription is - * already prepacked, so storing here only those that need recheck later - * - * Note that they are not indexed by the location or nir driver location, - * as we are defining here only the inputs that the shader are really - * using. + * already prepacked, so here we are only storing those that need recheck + * later. The array must be indexed by driver location, since that is the + * order in which we need to emit the attributes. */ struct v3dv_pipeline_vertex_attrib { uint32_t binding; uint32_t offset; - /* We store driver_location instead of location because most v3d structs - * are indexed by driver_location - */ - uint32_t driver_location; VkFormat vk_format; } va[MAX_VERTEX_ATTRIBS]; uint32_t va_count;