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 <asm...@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 <asm...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
