radv: fix the NUM_RECORDS field for vertex bindings on GFX6/GFX7

Submitted by Samuel Pitoiset on March 13, 2019, 4:09 p.m.

Details

Message ID 20190313160944.25474-1-samuel.pitoiset@gmail.com
State New
Headers show
Series "radv: fix the NUM_RECORDS field for vertex bindings on GFX6/GFX7" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset March 13, 2019, 4:09 p.m.
Since the driver now uses typed buffer loads, we don't have to
account for the format.

This fixes few CTS regressions on SI.

Fixes: a66b186bebf ("radv: use typed buffer loads for vertex input fetches")
Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 src/amd/vulkan/radv_cmd_buffer.c |  3 +--
 src/amd/vulkan/radv_pipeline.c   | 12 ------------
 src/amd/vulkan/radv_private.h    |  6 ------
 3 files changed, 1 insertion(+), 20 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
index 06806ed6fce..d14bb1093c5 100644
--- a/src/amd/vulkan/radv_cmd_buffer.c
+++ b/src/amd/vulkan/radv_cmd_buffer.c
@@ -1990,7 +1990,6 @@  radv_flush_vertex_descriptors(struct radv_cmd_buffer *cmd_buffer,
 	    (cmd_buffer->state.dirty & RADV_CMD_DIRTY_VERTEX_BUFFER)) &&
 	    cmd_buffer->state.pipeline->num_vertex_bindings &&
 	    radv_get_shader(cmd_buffer->state.pipeline, MESA_SHADER_VERTEX)->info.info.vs.has_vertex_buffers) {
-		struct radv_vertex_elements_info *velems = &cmd_buffer->state.pipeline->vertex_elements;
 		unsigned vb_offset;
 		void *vb_ptr;
 		uint32_t i = 0;
@@ -2018,7 +2017,7 @@  radv_flush_vertex_descriptors(struct radv_cmd_buffer *cmd_buffer,
 			desc[0] = va;
 			desc[1] = S_008F04_BASE_ADDRESS_HI(va >> 32) | S_008F04_STRIDE(stride);
 			if (cmd_buffer->device->physical_device->rad_info.chip_class <= CIK && stride)
-				desc[2] = (buffer->size - offset - velems->format_size[i]) / stride + 1;
+				desc[2] = (buffer->size - offset) / stride + 1;
 			else
 				desc[2] = buffer->size - offset;
 			desc[3] = S_008F0C_DST_SEL_X(V_008F0C_SQ_SEL_X) |
diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c
index 7f2f96c540a..793508d15d6 100644
--- a/src/amd/vulkan/radv_pipeline.c
+++ b/src/amd/vulkan/radv_pipeline.c
@@ -3531,18 +3531,6 @@  radv_compute_vertex_input_state(struct radv_pipeline *pipeline,
 {
 	const VkPipelineVertexInputStateCreateInfo *vi_info =
 		pCreateInfo->pVertexInputState;
-	struct radv_vertex_elements_info *velems = &pipeline->vertex_elements;
-
-	for (uint32_t i = 0; i < vi_info->vertexAttributeDescriptionCount; i++) {
-		const VkVertexInputAttributeDescription *desc =
-			&vi_info->pVertexAttributeDescriptions[i];
-		unsigned loc = desc->location;
-		const struct vk_format_description *format_desc;
-
-		format_desc = vk_format_description(desc->format);
-
-		velems->format_size[loc] = format_desc->block.bits / 8;
-	}
 
 	for (uint32_t i = 0; i < vi_info->vertexBindingDescriptionCount; i++) {
 		const VkVertexInputBindingDescription *desc =
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index 39fa6110fde..5c6258a2952 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -1341,10 +1341,6 @@  struct radv_prim_vertex_count {
 	uint8_t incr;
 };
 
-struct radv_vertex_elements_info {
-	uint32_t format_size[MAX_VERTEX_ATTRIBS];
-};
-
 struct radv_ia_multi_vgt_param_helpers {
 	uint32_t base;
 	bool partial_es_wave;
@@ -1371,8 +1367,6 @@  struct radv_pipeline {
 	uint32_t                                  ctx_cs_hash;
 	struct radeon_cmdbuf                      ctx_cs;
 
-	struct radv_vertex_elements_info             vertex_elements;
-
 	uint32_t                                     binding_stride[MAX_VBS];
 	uint8_t                                      num_vertex_bindings;
 

Comments

I think this needs to be modified to get the vulkan out of bounds behavior.

In particular whether a VS input is out of bounds or not can differ
between attributes from the same buffer with the same index, and that
is something not handled here.

I guess we could fix by only using the offset in the shader and
calculating the offset by adding index * stride to it, but that is
extra instructions. Is doing the typed loads still a win.

(Aside, don't we need to do the NUM_RECORDS logic for Vega too?)

On Wed, Mar 13, 2019 at 5:09 PM Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>
> Since the driver now uses typed buffer loads, we don't have to
> account for the format.
>
> This fixes few CTS regressions on SI.
>
> Fixes: a66b186bebf ("radv: use typed buffer loads for vertex input fetches")
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  src/amd/vulkan/radv_cmd_buffer.c |  3 +--
>  src/amd/vulkan/radv_pipeline.c   | 12 ------------
>  src/amd/vulkan/radv_private.h    |  6 ------
>  3 files changed, 1 insertion(+), 20 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
> index 06806ed6fce..d14bb1093c5 100644
> --- a/src/amd/vulkan/radv_cmd_buffer.c
> +++ b/src/amd/vulkan/radv_cmd_buffer.c
> @@ -1990,7 +1990,6 @@ radv_flush_vertex_descriptors(struct radv_cmd_buffer *cmd_buffer,
>             (cmd_buffer->state.dirty & RADV_CMD_DIRTY_VERTEX_BUFFER)) &&
>             cmd_buffer->state.pipeline->num_vertex_bindings &&
>             radv_get_shader(cmd_buffer->state.pipeline, MESA_SHADER_VERTEX)->info.info.vs.has_vertex_buffers) {
> -               struct radv_vertex_elements_info *velems = &cmd_buffer->state.pipeline->vertex_elements;
>                 unsigned vb_offset;
>                 void *vb_ptr;
>                 uint32_t i = 0;
> @@ -2018,7 +2017,7 @@ radv_flush_vertex_descriptors(struct radv_cmd_buffer *cmd_buffer,
>                         desc[0] = va;
>                         desc[1] = S_008F04_BASE_ADDRESS_HI(va >> 32) | S_008F04_STRIDE(stride);
>                         if (cmd_buffer->device->physical_device->rad_info.chip_class <= CIK && stride)
> -                               desc[2] = (buffer->size - offset - velems->format_size[i]) / stride + 1;
> +                               desc[2] = (buffer->size - offset) / stride + 1;
>                         else
>                                 desc[2] = buffer->size - offset;
>                         desc[3] = S_008F0C_DST_SEL_X(V_008F0C_SQ_SEL_X) |
> diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c
> index 7f2f96c540a..793508d15d6 100644
> --- a/src/amd/vulkan/radv_pipeline.c
> +++ b/src/amd/vulkan/radv_pipeline.c
> @@ -3531,18 +3531,6 @@ radv_compute_vertex_input_state(struct radv_pipeline *pipeline,
>  {
>         const VkPipelineVertexInputStateCreateInfo *vi_info =
>                 pCreateInfo->pVertexInputState;
> -       struct radv_vertex_elements_info *velems = &pipeline->vertex_elements;
> -
> -       for (uint32_t i = 0; i < vi_info->vertexAttributeDescriptionCount; i++) {
> -               const VkVertexInputAttributeDescription *desc =
> -                       &vi_info->pVertexAttributeDescriptions[i];
> -               unsigned loc = desc->location;
> -               const struct vk_format_description *format_desc;
> -
> -               format_desc = vk_format_description(desc->format);
> -
> -               velems->format_size[loc] = format_desc->block.bits / 8;
> -       }
>
>         for (uint32_t i = 0; i < vi_info->vertexBindingDescriptionCount; i++) {
>                 const VkVertexInputBindingDescription *desc =
> diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
> index 39fa6110fde..5c6258a2952 100644
> --- a/src/amd/vulkan/radv_private.h
> +++ b/src/amd/vulkan/radv_private.h
> @@ -1341,10 +1341,6 @@ struct radv_prim_vertex_count {
>         uint8_t incr;
>  };
>
> -struct radv_vertex_elements_info {
> -       uint32_t format_size[MAX_VERTEX_ATTRIBS];
> -};
> -
>  struct radv_ia_multi_vgt_param_helpers {
>         uint32_t base;
>         bool partial_es_wave;
> @@ -1371,8 +1367,6 @@ struct radv_pipeline {
>         uint32_t                                  ctx_cs_hash;
>         struct radeon_cmdbuf                      ctx_cs;
>
> -       struct radv_vertex_elements_info             vertex_elements;
> -
>         uint32_t                                     binding_stride[MAX_VBS];
>         uint8_t                                      num_vertex_bindings;
>
> --
> 2.21.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev