Hi Jason,
On 13 February 2017 at 16:10, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 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_surf
>> ace_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?
>
That makes sense, I'll fix that up.
Thanks for looking at this!
Alex
>
>
>> }
>>
>> 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->inte
>> rface_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
