anv: Add support for shaderStorageImageWriteWithoutFormat

This allows shaders to write to storage images declared with unknown
format if they are decorated with NonReadable ("writeonly" in GLSL).

Previously an image view would always use a lowered format for its
surface state, however when a shader declares a write-only image, we
should use the real format. Since we don't know at view creation time
whether it will be used with only write-only images in shaders, create
two surface states using both the original format and the lowered
format. When emitting the binding table, choose between the states
based on whether the image is declared write-only in the shader.

Tested on both Sascha Willems' computeshader sample (with the original
shaders and ones modified to declare images writeonly and omit their
format qualifiers) and on our own shaders for which we need support
for this.

Signed-off-by: Alex Smith <asmith@feralinteractive.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 91ee67f053d5174ae739b3fea16a6e4518230dc4..46b83a3617ee697d91074ee0e1e452db0b2e4d27 100644 (file)
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -484,7 +484,7 @@ void anv_GetPhysicalDeviceFeatures(
       .shaderStorageImageExtendedFormats        = true,
       .shaderStorageImageMultisample            = false,
       .shaderStorageImageReadWithoutFormat      = false,
-      .shaderStorageImageWriteWithoutFormat     = false,
+      .shaderStorageImageWriteWithoutFormat     = true,
       .shaderUniformBufferArrayDynamicIndexing  = true,
       .shaderSampledImageArrayDynamicIndexing   = true,
       .shaderStorageBufferArrayDynamicIndexing  = true,

diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
index e59ef4dbb6759ebe5d7786bc7b5e282e5f5bbc31..cf4304a0cc3b2b7bf9b1694b30857a021331d5d7 100644 (file)
--- a/src/intel/vulkan/anv_image.c
+++ b/src/intel/vulkan/anv_image.c
@@ -583,13 +583,31 @@ anv_CreateImageView(VkDevice _device,
    /* NOTE: This one needs to go last since it may stomp isl_view.format */
    if (image->usage & VK_IMAGE_USAGE_STORAGE_BIT) {
       iview->storage_surface_state = alloc_surface_state(device);
+      iview->writeonly_storage_surface_state = alloc_surface_state(device);
+
+      struct isl_view view = iview->isl;
+      view.usage |= ISL_SURF_USAGE_STORAGE_BIT;
+
+      /* Write-only accesses always used a typed write instruction and should
+       * therefore use the real format.
+       */
+      isl_surf_fill_state(&device->isl_dev,
+                          iview->writeonly_storage_surface_state.map,
+                          .surf = &surface->isl,
+                          .view = &view,
+                          .aux_surf = &image->aux_surface.isl,
+                          .aux_usage = surf_usage,
+                          .mocs = device->default_mocs);
 
       if (isl_has_matching_typed_storage_image_format(&device->info,
                                                       format.isl_format)) {
-         struct isl_view view = iview->isl;
-         view.usage |= ISL_SURF_USAGE_STORAGE_BIT;
+         /* Typed surface reads support a very limited subset of the shader
+          * image formats.  Translate it into the closest format the hardware
+          * supports.
+          */
          view.format = isl_lower_storage_image_format(&device->info,
                                                       format.isl_format);
+
          isl_surf_fill_state(&device->isl_dev,
                              iview->storage_surface_state.map,
                              .surf = &surface->isl,
@@ -608,10 +626,13 @@ anv_CreateImageView(VkDevice _device,
                                 &iview->storage_image_param,
                                 &surface->isl, &iview->isl);
 
-      if (!device->info.has_llc)
+      if (!device->info.has_llc) {
          anv_state_clflush(iview->storage_surface_state);
+         anv_state_clflush(iview->writeonly_storage_surface_state);
+      }
    } else {
       iview->storage_surface_state.alloc_size = 0;
+      iview->writeonly_storage_surface_state.alloc_size = 0;
    }
 
    *pView = anv_image_view_to_handle(iview);
@@ -639,6 +660,11 @@ anv_DestroyImageView(VkDevice _device, VkImageView _iview,
                           iview->storage_surface_state);
    }
 
+   if (iview->writeonly_storage_surface_state.alloc_size > 0) {
+      anv_state_pool_free(&device->surface_state_pool,
+                          iview->writeonly_storage_surface_state);
+   }
+
    vk_free2(&device->alloc, pAllocator, iview);
 }
 
@@ -682,6 +708,7 @@ anv_CreateBufferView(VkDevice _device,
 
    if (buffer->usage & VK_BUFFER_USAGE_STORAGE_TEXEL_BUFFER_BIT) {
       view->storage_surface_state = alloc_surface_state(device);
+      view->writeonly_storage_surface_state = alloc_surface_state(device);
 
       enum isl_format storage_format =
          isl_has_matching_typed_storage_image_format(&device->info,
@@ -695,11 +722,18 @@ anv_CreateBufferView(VkDevice _device,
                                     (storage_format == ISL_FORMAT_RAW ? 1 :
                                      isl_format_get_layout(storage_format)->bpb / 8));
 
+      /* Write-only accesses should use the original format. */
+      anv_fill_buffer_surface_state(device, view->writeonly_storage_surface_state,
+                                    view->format,
+                                    view->offset, view->range,
+                                    isl_format_get_layout(view->format)->bpb / 8);
+
       isl_buffer_fill_image_param(&device->isl_dev,
                                   &view->storage_image_param,
                                   view->format, view->range);
    } else {
       view->storage_surface_state = (struct anv_state){ 0 };
+      view->writeonly_storage_surface_state = (struct anv_state){ 0 };
    }
 
    *pView = anv_buffer_view_to_handle(view);
@@ -725,6 +759,10 @@ anv_DestroyBufferView(VkDevice _device, VkBufferView bufferView,
       anv_state_pool_free(&device->surface_state_pool,
                           view->storage_surface_state);
 
+   if (view->writeonly_storage_surface_state.alloc_size > 0)
+      anv_state_pool_free(&device->surface_state_pool,
+                          view->writeonly_storage_surface_state);
+
    vk_free2(&device->alloc, pAllocator, view);
 }
 

diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
index d6b85611419cd094d978e328a69f29fe8432b7bf..67bcf5e29ef1dd6bc4ae15730cc1bd2d45323085 100644 (file)
--- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
+++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
@@ -351,9 +351,6 @@ anv_nir_apply_pipeline_layout(struct anv_pipeline *pipeline,
          continue;
 
       enum glsl_sampler_dim dim = glsl_get_sampler_dim(var->interface_type);
-      if (dim != GLSL_SAMPLER_DIM_SUBPASS &&
-          dim != GLSL_SAMPLER_DIM_SUBPASS_MS)
-         continue;
 
       const uint32_t set = var->data.descriptor_set;
       const uint32_t binding = var->data.binding;
@@ -369,7 +366,12 @@ anv_nir_apply_pipeline_layout(struct anv_pipeline *pipeline,
          assert(pipe_binding[i].set == set);
          assert(pipe_binding[i].binding == binding);
          assert(pipe_binding[i].index == i);
-         pipe_binding[i].input_attachment_index = var->data.index + i;
+
+         if (dim == GLSL_SAMPLER_DIM_SUBPASS ||
+             dim == GLSL_SAMPLER_DIM_SUBPASS_MS)
+            pipe_binding[i].input_attachment_index = var->data.index + i;
+
+         pipe_binding[i].write_only = var->data.image.write_only;
       }
    }
 
@@ -398,18 +400,6 @@ anv_nir_apply_pipeline_layout(struct anv_pipeline *pipeline,
             unsigned binding = var->data.binding;
             unsigned image_index = state.set[set].image_offsets[binding];
 
-            /* We have a very tight coupling between back-end compiler and
-             * state setup which requires us to fill the image surface state
-             * out differently if and only if the image is declared write-only.
-             * Right now, our state setup code sets up all images as if they
-             * are read-write.  This means that the compiler needs to see
-             * read-only as well.
-             *
-             * Whenever we implement shaderStorageImageWriteWithoutFormat, we
-             * need to delete this.
-             */
-            var->data.image.write_only = false;
-
             var->data.driver_location = shader->num_uniforms +
                                         image_index * BRW_IMAGE_PARAM_SIZE * 4;
          }

diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c
index ca3823c2b6637443ca5c2cdfff7fafa8ebfa7c53..44101038357829d29fe2761c0f59414a1c23c8e6 100644 (file)
--- a/src/intel/vulkan/anv_pipeline.c
+++ b/src/intel/vulkan/anv_pipeline.c
@@ -128,6 +128,7 @@ anv_shader_compile_to_nir(struct anv_device *device,
       .float64 = device->instance->physicalDevice.info.gen >= 8,
       .tessellation = true,
       .draw_parameters = true,
+      .image_write_without_format = true,
    };
 
    nir_function *entry_point =

diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 51e85c74bd15d4ac9cc9cbb8b1d54bb1eb54995d..ec791a42871f93b63e1347488d249e72b8275775 100644 (file)
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -953,6 +953,9 @@ struct anv_pipeline_binding {
 
    /* Input attachment index (relative to the subpass) */
    uint8_t input_attachment_index;
+
+   /* For a storage image, whether it is write-only */
+   bool write_only;
 };
 
 struct anv_pipeline_layout {
@@ -1683,8 +1686,13 @@ struct anv_image_view {
    /** RENDER_SURFACE_STATE when using image as a sampler surface. */
    struct anv_state sampler_surface_state;
 
-   /** RENDER_SURFACE_STATE when using image as a storage image. */
+   /**
+    * RENDER_SURFACE_STATE when using image as a storage image. Separate states
+    * for write-only and readable, using the real format for write-only and the
+    * lowered format for readable.
+    */
    struct anv_state storage_surface_state;
+   struct anv_state writeonly_storage_surface_state;
 
    struct brw_image_param storage_image_param;
 };
@@ -1715,6 +1723,7 @@ struct anv_buffer_view {
 
    struct anv_state surface_state;
    struct anv_state storage_surface_state;
+   struct anv_state writeonly_storage_surface_state;
 
    struct brw_image_param storage_image_param;
 };

diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c
index d07fe78b52f7b8118d922646f4f060b8ef01eb7e..14338b22ecea3364aa21dfe10ec2b1196d6bfe43 100644 (file)
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -1211,7 +1211,9 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,
          break;
 
       case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE: {
-         surface_state = desc->image_view->storage_surface_state;
+         surface_state = (binding->write_only)
+            ? desc->image_view->writeonly_storage_surface_state
+            : desc->image_view->storage_surface_state;
          assert(surface_state.alloc_size);
          add_image_view_relocs(cmd_buffer, desc->image_view,
                                desc->image_view->image->aux_usage,
@@ -1238,7 +1240,9 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,
          break;
 
       case VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER:
-         surface_state = desc->buffer_view->storage_surface_state;
+         surface_state = (binding->write_only)
+            ? desc->buffer_view->writeonly_storage_surface_state
+            : desc->buffer_view->storage_surface_state;
          assert(surface_state.alloc_size);
          add_surface_state_reloc(cmd_buffer, surface_state,
                                  desc->buffer_view->bo,