[Mesa-dev] [PATCH 2/2] anv: Add support for shaderStorageImageWriteWithoutFormat

Jason Ekstrand jason at jlekstrand.net
Mon Feb 13 16:10:27 UTC 2017


Hey Alex,

Thanks for the patch!  I've made a couple of comments below but otherwise
this looks fantastic!  I've thought a few times about how we would
implement this and what you've come up with is almost exactly what I would
have done. :-)

On Thu, Feb 9, 2017 at 8:06 AM, Alex Smith <asmith at feralinteractive.com>
wrote:

> 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 at feralinteractive.com>
> ---
>  src/intel/vulkan/anv_device.c                    |  2 +-
>  src/intel/vulkan/anv_image.c                     | 31
> +++++++++++++++++++++++-
>  src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 10 +++++---
>  src/intel/vulkan/anv_pipeline.c                  |  1 +
>  src/intel/vulkan/anv_private.h                   | 10 +++++++-
>  src/intel/vulkan/genX_cmd_buffer.c               |  4 ++-
>  6 files changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 91ee67f..46b83a3 100644
> --- 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 e59ef4d..1aeec44 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -583,13 +583,29 @@ 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);
>
>        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;
> +
> +         /* Write-only accesses should 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);
> +
> +         /* 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,
> @@ -602,16 +618,24 @@ anv_CreateImageView(VkDevice _device,
>                                         ISL_FORMAT_RAW,
>                                         iview->offset,
>                                         iview->bo->size - iview->offset,
> 1);
> +         anv_fill_buffer_surface_state(device,
> +                                       iview->writeonly_storage_
> surface_state,
> +                                       ISL_FORMAT_RAW,
> +                                       iview->offset,
> +                                       iview->bo->size - iview->offset,
> 1);
>

This isn't quite right.  The isl_has_matching_typed_storage_image_format
function decides whether it's lowered to a load/store with a different type
or if it's lowered to, effectively, an SSBO and all of the tiling
calculations are done in the shader.  With write-only, we always use a
typed write instruction so that should never get the buffer treatment.
What we really need to do is fill out the write-only surface state outside
of the if statement.  Make sense?


>        }
>
>        isl_surf_fill_image_param(&device->isl_dev,
>                                  &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 +663,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);
>  }
>
> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> index 8846c2e..296fd05 100644
> --- 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;
>        }
>     }
>
> diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_
> pipeline.c
> index ca3823c..4410103 100644
> --- 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 51e85c7..8db851a 100644
> --- 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;
>  };
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index 8db26e9..b80159b 100644
> --- 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,
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170213/34fd7f19/attachment-0001.html>


More information about the mesa-dev mailing list