<div dir="ltr"><div>Hey Alex, </div>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. :-) <div class="gmail_extra"> <div class="gmail_quote">On Thu, Feb 9, 2017 at 8:06 AM, Alex Smith <<a href="mailto:asmith@feralinteractive.com" target="_blank">asmith@feralinteractive.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <<a href="mailto:asmith@feralinteractive.com">asmith@feralinteractive.com</a>> --- 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); </blockquote><div> </div><div>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? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> } 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>