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

Alex Smith asmith at feralinteractive.com
Mon Feb 13 17:39:26 UTC 2017


Hi Jason,

On 13 February 2017 at 16:10, Jason Ekstrand <jason at 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 <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_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 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/ae879760/attachment-0001.html>


More information about the mesa-dev mailing list