On Mon, Feb 13, 2017 at 9:40 AM, Alex Smith <asm...@feralinteractive.com>
wrote:
> Hi Lionel,
>
> On 13 February 2017 at 17:28, Lionel Landwerlin <
> lionel.g.landwer...@intel.com> wrote:
>
>> On 09/02/17 16:06, Alex Smith 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_surf
>>> ace_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);
>>> }
>>> 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)
>>>
>>
>> This condition is incorrect, now that you moved it here, it should be
>>
>> if (dim == GLSL_SAMPLER_DIM_SUBPASS ||
>> dim == GLSL_SAMPLER_DIM_SUBPASS_MS)
>>
>> With that fixed, this patch is :
>>
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwer...@intel.com>
>
>
> Oops, thanks for that, I'll fix that as well.
>
It would be good to run the CTS with at least dEQP-VK.image.load_store.* to
make sure you didn't break anything before you re-send. If you're not
familiar with running dEQP, it's just
./deqp-vk -n 'dEQP-VK.image.load_store.*'
The quotes matter because dEQP needs to see the "*" not your shell.
--Jason
> Alex
>
>
>>
>>
>> + 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,
>>>
>>
>>
>>
>
> _______________________________________________
> 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
