[Mesa-dev] [PATCH 1/7] anv/spirv: SSBO/UBO buffers needs padding size is not multiple of 32-bits

[Jose Maria Casanova Crespo]

The surfaces that backup the GPU buffers have a boundary check that
considers that access to partial dwords are considered out-of-bounds.
For example is basic 16-bit cases of buffers with size 2 or 6 where the
last two bytes will always be read as 0 or its writting ignored.

The introduction of 16-bit types implies that we need to align the size
to 4-bytes multiples so that partial dwords could be read/written.
Adding an inconditional +2 size to buffers not being multiple of 2
solves this issue for the general cases of UBO or SSBO.

But, when unsized_arrays of 16-bit elements are used it is not possible
to know if the size was padded or not. To solve this issue the
implementation of SSBO calculates the needed size of the surface,
as suggested by Jason:

surface_size = 2 * aling_u64(buffer_size, 4)  - buffer_size

So when we calculate backwards the SpvOpArrayLenght with a nir expresion
when the array stride is not multiple of 4.

array_size = (surface_size & ~3) - (surface_size & 3)

It is also exposed this buffer requirements when robust buffer access
is enabled so these buffer sizes recommend being multiple of 4.
---

I have some doubts if vtn_variables.c is the best place to include
this specific to calculate the real buffer size as this is new
calculus seems to be quite HW dependent and maybe other drivers different
to ANV don't need this kind of solution.

 src/compiler/spirv/vtn_variables.c    | 14 ++++++++++++++
 src/intel/vulkan/anv_descriptor_set.c | 16 ++++++++++++++++
 src/intel/vulkan/anv_device.c         | 11 +++++++++++
 3 files changed, 41 insertions(+)

diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c
index 9eb85c24e9..78adab3ed2 100644
--- a/src/compiler/spirv/vtn_variables.c
+++ b/src/compiler/spirv/vtn_variables.c
@@ -2113,6 +2113,20 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode,
       nir_builder_instr_insert(&b->nb, &instr->instr);
       nir_ssa_def *buf_size = &instr->dest.ssa;
 
+      /* Calculate real length if padding was done to align the buffer
+       * to 32-bits. This only could happen is stride is not multiple
+       * of 4. Introduced to support 16-bit type unsized arrays in anv.
+       */
+      if (stride % 4) {
+         buf_size = nir_isub(&b->nb,
+                             nir_iand(&b->nb,
+                                      buf_size,
+                                      nir_imm_int(&b->nb, ~3)),
+                             nir_iand (&b->nb,
+                                       buf_size,
+                                       nir_imm_int(&b->nb, 3)));
+      }
+
       /* array_length = max(buffer_size - offset, 0) / stride */
       nir_ssa_def *array_length =
          nir_idiv(&b->nb,
diff --git a/src/intel/vulkan/anv_descriptor_set.c b/src/intel/vulkan/anv_descriptor_set.c
index edb829601e..a97f2f37dc 100644
--- a/src/intel/vulkan/anv_descriptor_set.c
+++ b/src/intel/vulkan/anv_descriptor_set.c
@@ -704,6 +704,22 @@ anv_descriptor_set_write_buffer(struct anv_descriptor_set *set,
       bview->offset = buffer->offset + offset;
       bview->range = anv_buffer_get_range(buffer, offset, range);
 
+      /* Uniform and Storage buffers need to have surface size
+       * not less that the aligned 32-bit size of the buffer.
+       * To calculate the array lenght on unsized arrays
+       * in StorageBuffer the last 2 bits store the padding size
+       * added to the surface, so we can calculate latter the original
+       * buffer size to know the number of elements.
+       *
+       *  surface_size = 2 * aling_u64(buffer_size, 4)  - buffer_size
+       *
+       *  array_size = (surface_size & ~3) - (surface_size & 3)
+       */
+      if (type == VK_DESCRIPTOR_TYPE_STORAGE_BUFFER)
+         bview->range = 2 * align_u64(bview->range, 4) - bview->range;
+      else if (type == VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER)
+         bview->range = align_u64(bview->range, 4);
+
       /* If we're writing descriptors through a push command, we need to
        * allocate the surface state from the command buffer. Otherwise it will
        * be allocated by the descriptor pool when calling
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index a83b7a39f6..cedeed5621 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -2103,6 +2103,17 @@ void anv_GetBufferMemoryRequirements(
 
    pMemoryRequirements->size = buffer->size;
    pMemoryRequirements->alignment = alignment;
+
+   /* Storage and Uniform buffers should have their size aligned to
+    * 32-bits to avoid boundary checks when last DWord is not complete.
+    * This would ensure that not internal padding would be needed for
+    * 16-bit types.
+    */
+   if (device->robust_buffer_access &&
+       (buffer->usage & VK_BUFFER_USAGE_UNIFORM_BUFFER_BIT ||
+        buffer->usage & VK_BUFFER_USAGE_STORAGE_BUFFER_BIT))
+      pMemoryRequirements->size = align_u64(buffer->size, 4);
+
    pMemoryRequirements->memoryTypeBits = memory_types;
 }
 
-- 
2.14.3