[Mesa-dev] radv: Fix descriptors for cube images with VK_IMAGE_USAGE_STORAGE_BIT

Submitted by Alex Smith on June 30, 2017, 10:18 a.m.

Details

Message ID 20170630101832.14491-1-asmith@feralinteractive.com
State New
Headers show
Series "radv: Fix descriptors for cube images with VK_IMAGE_USAGE_STORAGE_BIT" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alex Smith June 30, 2017, 10:18 a.m.
If a cube image has VK_IMAGE_USAGE_STORAGE_BIT set, the type in an image
view's descriptor was set to a 2D array (and a few other fields adjusted
accordingly). This is correct when the image view is actually bound as a
storage image, but not when bound as a sampled image. In that case the
type should be set as a cube.

Fix by generating 2 sets of descriptors at view creation time for both
storage and non-storage usage, and then choose between them based on
descriptor type when writing descriptor sets.

Signed-off-by: Alex Smith <asmith@feralinteractive.com>
---
 src/amd/vulkan/radv_descriptor_set.c | 18 +++++++--
 src/amd/vulkan/radv_image.c          | 77 ++++++++++++++++++++++++------------
 src/amd/vulkan/radv_private.h        |  6 +++
 3 files changed, 72 insertions(+), 29 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/amd/vulkan/radv_descriptor_set.c b/src/amd/vulkan/radv_descriptor_set.c
index ec7fd3d..b4a78aa 100644
--- a/src/amd/vulkan/radv_descriptor_set.c
+++ b/src/amd/vulkan/radv_descriptor_set.c
@@ -603,11 +603,18 @@  write_image_descriptor(struct radv_device *device,
 		       struct radv_cmd_buffer *cmd_buffer,
 		       unsigned *dst,
 		       struct radeon_winsys_bo **buffer_list,
+		       VkDescriptorType descriptor_type,
 		       const VkDescriptorImageInfo *image_info)
 {
 	RADV_FROM_HANDLE(radv_image_view, iview, image_info->imageView);
-	memcpy(dst, iview->descriptor, 8 * 4);
-	memcpy(dst + 8, iview->fmask_descriptor, 8 * 4);
+
+	if (descriptor_type == VK_DESCRIPTOR_TYPE_STORAGE_IMAGE) {
+		memcpy(dst, iview->storage_descriptor, 8 * 4);
+		memcpy(dst + 8, iview->storage_fmask_descriptor, 8 * 4);
+	} else {
+		memcpy(dst, iview->descriptor, 8 * 4);
+		memcpy(dst + 8, iview->fmask_descriptor, 8 * 4);
+	}
 
 	if (cmd_buffer)
 		device->ws->cs_add_buffer(cmd_buffer->cs, iview->bo, 7);
@@ -620,12 +627,13 @@  write_combined_image_sampler_descriptor(struct radv_device *device,
 					struct radv_cmd_buffer *cmd_buffer,
 					unsigned *dst,
 					struct radeon_winsys_bo **buffer_list,
+					VkDescriptorType descriptor_type,
 					const VkDescriptorImageInfo *image_info,
 					bool has_sampler)
 {
 	RADV_FROM_HANDLE(radv_sampler, sampler, image_info->sampler);
 
-	write_image_descriptor(device, cmd_buffer, dst, buffer_list, image_info);
+	write_image_descriptor(device, cmd_buffer, dst, buffer_list, descriptor_type, image_info);
 	/* copy over sampler state */
 	if (has_sampler)
 		memcpy(dst + 16, sampler->state, 16);
@@ -696,10 +704,12 @@  void radv_update_descriptor_sets(
 			case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE:
 			case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT:
 				write_image_descriptor(device, cmd_buffer, ptr, buffer_list,
+						       writeset->descriptorType,
 						       writeset->pImageInfo + j);
 				break;
 			case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER:
 				write_combined_image_sampler_descriptor(device, cmd_buffer, ptr, buffer_list,
+									writeset->descriptorType,
 									writeset->pImageInfo + j,
 									!binding_layout->immutable_samplers_offset);
 				if (copy_immutable_samplers) {
@@ -866,10 +876,12 @@  void radv_update_descriptor_set_with_template(struct radv_device *device,
 			case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE:
 			case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT:
 				write_image_descriptor(device, cmd_buffer, pDst, buffer_list,
+						       templ->entry[i].descriptor_type,
 					               (struct VkDescriptorImageInfo *) pSrc);
 				break;
 			case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER:
 				write_combined_image_sampler_descriptor(device, cmd_buffer, pDst, buffer_list,
+									templ->entry[i].descriptor_type,
 									(struct VkDescriptorImageInfo *) pSrc,
 									templ->entry[i].has_sampler);
 				if (templ->entry[i].immutable_samplers)
diff --git a/src/amd/vulkan/radv_image.c b/src/amd/vulkan/radv_image.c
index 147ebed..54e7fc5 100644
--- a/src/amd/vulkan/radv_image.c
+++ b/src/amd/vulkan/radv_image.c
@@ -325,7 +325,7 @@  static unsigned gfx9_border_color_swizzle(const unsigned char swizzle[4])
 static void
 si_make_texture_descriptor(struct radv_device *device,
 			   struct radv_image *image,
-			   bool sampler,
+			   bool is_storage_image,
 			   VkImageViewType view_type,
 			   VkFormat vk_format,
 			   const VkComponentMapping *mapping,
@@ -362,7 +362,7 @@  si_make_texture_descriptor(struct radv_device *device,
 	}
 
 	type = radv_tex_dim(image->type, view_type, image->info.array_size, image->info.samples,
-			    (image->usage & VK_IMAGE_USAGE_STORAGE_BIT));
+			    is_storage_image);
 	if (type == V_008F1C_SQ_RSRC_IMG_1D_ARRAY) {
 	        height = 1;
 		depth = image->info.array_size;
@@ -526,7 +526,7 @@  radv_query_opaque_metadata(struct radv_device *device,
 	md->metadata[1] = si_get_bo_metadata_word1(device);
 
 
-	si_make_texture_descriptor(device, image, true,
+	si_make_texture_descriptor(device, image, false,
 				   (VkImageViewType)image->type, image->vk_format,
 				   &fixedmapping, 0, image->info.levels - 1, 0,
 				   image->info.array_size,
@@ -834,6 +834,50 @@  radv_image_create(VkDevice _device,
 	return VK_SUCCESS;
 }
 
+static void
+radv_image_view_make_descriptor(struct radv_image_view *iview,
+				struct radv_device *device,
+				const VkImageViewCreateInfo* pCreateInfo,
+				bool is_storage_image)
+{
+	RADV_FROM_HANDLE(radv_image, image, pCreateInfo->image);
+	const VkImageSubresourceRange *range = &pCreateInfo->subresourceRange;
+	bool is_stencil = iview->aspect_mask == VK_IMAGE_ASPECT_STENCIL_BIT;
+	uint32_t blk_w;
+	uint32_t *descriptor;
+	uint32_t *fmask_descriptor;
+
+	if (is_storage_image) {
+		descriptor = iview->storage_descriptor;
+		fmask_descriptor = iview->storage_fmask_descriptor;
+	} else {
+		descriptor = iview->descriptor;
+		fmask_descriptor = iview->fmask_descriptor;
+	}
+
+	assert(image->surface.blk_w % vk_format_get_blockwidth(image->vk_format) == 0);
+	blk_w = image->surface.blk_w / vk_format_get_blockwidth(image->vk_format) * vk_format_get_blockwidth(iview->vk_format);
+
+	si_make_texture_descriptor(device, image, is_storage_image,
+				   iview->type,
+				   iview->vk_format,
+				   &pCreateInfo->components,
+				   0, radv_get_levelCount(image, range) - 1,
+				   range->baseArrayLayer,
+				   range->baseArrayLayer + radv_get_layerCount(image, range) - 1,
+				   iview->extent.width,
+				   iview->extent.height,
+				   iview->extent.depth,
+				   descriptor,
+				   fmask_descriptor);
+	si_set_mutable_tex_desc_fields(device, image,
+				       is_stencil ? &image->surface.u.legacy.stencil_level[range->baseMipLevel]
+				                  : &image->surface.u.legacy.level[range->baseMipLevel],
+				       range->baseMipLevel,
+				       range->baseMipLevel,
+				       blk_w, is_stencil, descriptor);
+}
+
 void
 radv_image_view_init(struct radv_image_view *iview,
 		     struct radv_device *device,
@@ -841,8 +885,7 @@  radv_image_view_init(struct radv_image_view *iview,
 {
 	RADV_FROM_HANDLE(radv_image, image, pCreateInfo->image);
 	const VkImageSubresourceRange *range = &pCreateInfo->subresourceRange;
-	uint32_t blk_w;
-	bool is_stencil = false;
+
 	switch (image->type) {
 	case VK_IMAGE_TYPE_1D:
 	case VK_IMAGE_TYPE_2D:
@@ -862,7 +905,6 @@  radv_image_view_init(struct radv_image_view *iview,
 	iview->aspect_mask = pCreateInfo->subresourceRange.aspectMask;
 
 	if (iview->aspect_mask == VK_IMAGE_ASPECT_STENCIL_BIT) {
-		is_stencil = true;
 		iview->vk_format = vk_format_stencil_only(iview->vk_format);
 	} else if (iview->aspect_mask == VK_IMAGE_ASPECT_DEPTH_BIT) {
 		iview->vk_format = vk_format_depth_only(iview->vk_format);
@@ -879,30 +921,13 @@  radv_image_view_init(struct radv_image_view *iview,
 	iview->extent.height = round_up_u32(iview->extent.height * vk_format_get_blockheight(iview->vk_format),
 					    vk_format_get_blockheight(image->vk_format));
 
-	assert(image->surface.blk_w % vk_format_get_blockwidth(image->vk_format) == 0);
-	blk_w = image->surface.blk_w / vk_format_get_blockwidth(image->vk_format) * vk_format_get_blockwidth(iview->vk_format);
 	iview->base_layer = range->baseArrayLayer;
 	iview->layer_count = radv_get_layerCount(image, range);
 	iview->base_mip = range->baseMipLevel;
 
-	si_make_texture_descriptor(device, image, false,
-				   iview->type,
-				   iview->vk_format,
-				   &pCreateInfo->components,
-				   0, radv_get_levelCount(image, range) - 1,
-				   range->baseArrayLayer,
-				   range->baseArrayLayer + radv_get_layerCount(image, range) - 1,
-				   iview->extent.width,
-				   iview->extent.height,
-				   iview->extent.depth,
-				   iview->descriptor,
-				   iview->fmask_descriptor);
-	si_set_mutable_tex_desc_fields(device, image,
-				       is_stencil ? &image->surface.u.legacy.stencil_level[range->baseMipLevel]
-				                  : &image->surface.u.legacy.level[range->baseMipLevel],
-				       range->baseMipLevel,
-				       range->baseMipLevel,
-				       blk_w, is_stencil, iview->descriptor);
+	radv_image_view_make_descriptor(iview, device, pCreateInfo, false);
+	if (image->usage & VK_IMAGE_USAGE_STORAGE_BIT)
+		radv_image_view_make_descriptor(iview, device, pCreateInfo, true);
 }
 
 bool radv_layout_has_htile(const struct radv_image *image,
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index ac89fc1..5d5e609 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -1271,6 +1271,12 @@  struct radv_image_view {
 
 	uint32_t descriptor[8];
 	uint32_t fmask_descriptor[8];
+
+	/* Descriptor for use as a storage image as opposed to a sampled image.
+	 * This has a few differences for cube maps (e.g. type).
+	 */
+	uint32_t storage_descriptor[8];
+	uint32_t storage_fmask_descriptor[8];
 };
 
 struct radv_image_create_info {

Comments

Ping.

On 30 June 2017 at 11:18, Alex Smith <asmith@feralinteractive.com> wrote:
> If a cube image has VK_IMAGE_USAGE_STORAGE_BIT set, the type in an image
> view's descriptor was set to a 2D array (and a few other fields adjusted
> accordingly). This is correct when the image view is actually bound as a
> storage image, but not when bound as a sampled image. In that case the
> type should be set as a cube.
>
> Fix by generating 2 sets of descriptors at view creation time for both
> storage and non-storage usage, and then choose between them based on
> descriptor type when writing descriptor sets.
>
> Signed-off-by: Alex Smith <asmith@feralinteractive.com>
> ---
>  src/amd/vulkan/radv_descriptor_set.c | 18 +++++++--
>  src/amd/vulkan/radv_image.c          | 77 ++++++++++++++++++++++++------------
>  src/amd/vulkan/radv_private.h        |  6 +++
>  3 files changed, 72 insertions(+), 29 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_descriptor_set.c b/src/amd/vulkan/radv_descriptor_set.c
> index ec7fd3d..b4a78aa 100644
> --- a/src/amd/vulkan/radv_descriptor_set.c
> +++ b/src/amd/vulkan/radv_descriptor_set.c
> @@ -603,11 +603,18 @@ write_image_descriptor(struct radv_device *device,
>                        struct radv_cmd_buffer *cmd_buffer,
>                        unsigned *dst,
>                        struct radeon_winsys_bo **buffer_list,
> +                      VkDescriptorType descriptor_type,
>                        const VkDescriptorImageInfo *image_info)
>  {
>         RADV_FROM_HANDLE(radv_image_view, iview, image_info->imageView);
> -       memcpy(dst, iview->descriptor, 8 * 4);
> -       memcpy(dst + 8, iview->fmask_descriptor, 8 * 4);
> +
> +       if (descriptor_type == VK_DESCRIPTOR_TYPE_STORAGE_IMAGE) {
> +               memcpy(dst, iview->storage_descriptor, 8 * 4);
> +               memcpy(dst + 8, iview->storage_fmask_descriptor, 8 * 4);
> +       } else {
> +               memcpy(dst, iview->descriptor, 8 * 4);
> +               memcpy(dst + 8, iview->fmask_descriptor, 8 * 4);
> +       }
>
>         if (cmd_buffer)
>                 device->ws->cs_add_buffer(cmd_buffer->cs, iview->bo, 7);
> @@ -620,12 +627,13 @@ write_combined_image_sampler_descriptor(struct radv_device *device,
>                                         struct radv_cmd_buffer *cmd_buffer,
>                                         unsigned *dst,
>                                         struct radeon_winsys_bo **buffer_list,
> +                                       VkDescriptorType descriptor_type,
>                                         const VkDescriptorImageInfo *image_info,
>                                         bool has_sampler)
>  {
>         RADV_FROM_HANDLE(radv_sampler, sampler, image_info->sampler);
>
> -       write_image_descriptor(device, cmd_buffer, dst, buffer_list, image_info);
> +       write_image_descriptor(device, cmd_buffer, dst, buffer_list, descriptor_type, image_info);
>         /* copy over sampler state */
>         if (has_sampler)
>                 memcpy(dst + 16, sampler->state, 16);
> @@ -696,10 +704,12 @@ void radv_update_descriptor_sets(
>                         case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE:
>                         case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT:
>                                 write_image_descriptor(device, cmd_buffer, ptr, buffer_list,
> +                                                      writeset->descriptorType,
>                                                        writeset->pImageInfo + j);
>                                 break;
>                         case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER:
>                                 write_combined_image_sampler_descriptor(device, cmd_buffer, ptr, buffer_list,
> +                                                                       writeset->descriptorType,
>                                                                         writeset->pImageInfo + j,
>                                                                         !binding_layout->immutable_samplers_offset);
>                                 if (copy_immutable_samplers) {
> @@ -866,10 +876,12 @@ void radv_update_descriptor_set_with_template(struct radv_device *device,
>                         case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE:
>                         case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT:
>                                 write_image_descriptor(device, cmd_buffer, pDst, buffer_list,
> +                                                      templ->entry[i].descriptor_type,
>                                                        (struct VkDescriptorImageInfo *) pSrc);
>                                 break;
>                         case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER:
>                                 write_combined_image_sampler_descriptor(device, cmd_buffer, pDst, buffer_list,
> +                                                                       templ->entry[i].descriptor_type,
>                                                                         (struct VkDescriptorImageInfo *) pSrc,
>                                                                         templ->entry[i].has_sampler);
>                                 if (templ->entry[i].immutable_samplers)
> diff --git a/src/amd/vulkan/radv_image.c b/src/amd/vulkan/radv_image.c
> index 147ebed..54e7fc5 100644
> --- a/src/amd/vulkan/radv_image.c
> +++ b/src/amd/vulkan/radv_image.c
> @@ -325,7 +325,7 @@ static unsigned gfx9_border_color_swizzle(const unsigned char swizzle[4])
>  static void
>  si_make_texture_descriptor(struct radv_device *device,
>                            struct radv_image *image,
> -                          bool sampler,
> +                          bool is_storage_image,
>                            VkImageViewType view_type,
>                            VkFormat vk_format,
>                            const VkComponentMapping *mapping,
> @@ -362,7 +362,7 @@ si_make_texture_descriptor(struct radv_device *device,
>         }
>
>         type = radv_tex_dim(image->type, view_type, image->info.array_size, image->info.samples,
> -                           (image->usage & VK_IMAGE_USAGE_STORAGE_BIT));
> +                           is_storage_image);
>         if (type == V_008F1C_SQ_RSRC_IMG_1D_ARRAY) {
>                 height = 1;
>                 depth = image->info.array_size;
> @@ -526,7 +526,7 @@ radv_query_opaque_metadata(struct radv_device *device,
>         md->metadata[1] = si_get_bo_metadata_word1(device);
>
>
> -       si_make_texture_descriptor(device, image, true,
> +       si_make_texture_descriptor(device, image, false,
>                                    (VkImageViewType)image->type, image->vk_format,
>                                    &fixedmapping, 0, image->info.levels - 1, 0,
>                                    image->info.array_size,
> @@ -834,6 +834,50 @@ radv_image_create(VkDevice _device,
>         return VK_SUCCESS;
>  }
>
> +static void
> +radv_image_view_make_descriptor(struct radv_image_view *iview,
> +                               struct radv_device *device,
> +                               const VkImageViewCreateInfo* pCreateInfo,
> +                               bool is_storage_image)
> +{
> +       RADV_FROM_HANDLE(radv_image, image, pCreateInfo->image);
> +       const VkImageSubresourceRange *range = &pCreateInfo->subresourceRange;
> +       bool is_stencil = iview->aspect_mask == VK_IMAGE_ASPECT_STENCIL_BIT;
> +       uint32_t blk_w;
> +       uint32_t *descriptor;
> +       uint32_t *fmask_descriptor;
> +
> +       if (is_storage_image) {
> +               descriptor = iview->storage_descriptor;
> +               fmask_descriptor = iview->storage_fmask_descriptor;
> +       } else {
> +               descriptor = iview->descriptor;
> +               fmask_descriptor = iview->fmask_descriptor;
> +       }
> +
> +       assert(image->surface.blk_w % vk_format_get_blockwidth(image->vk_format) == 0);
> +       blk_w = image->surface.blk_w / vk_format_get_blockwidth(image->vk_format) * vk_format_get_blockwidth(iview->vk_format);
> +
> +       si_make_texture_descriptor(device, image, is_storage_image,
> +                                  iview->type,
> +                                  iview->vk_format,
> +                                  &pCreateInfo->components,
> +                                  0, radv_get_levelCount(image, range) - 1,
> +                                  range->baseArrayLayer,
> +                                  range->baseArrayLayer + radv_get_layerCount(image, range) - 1,
> +                                  iview->extent.width,
> +                                  iview->extent.height,
> +                                  iview->extent.depth,
> +                                  descriptor,
> +                                  fmask_descriptor);
> +       si_set_mutable_tex_desc_fields(device, image,
> +                                      is_stencil ? &image->surface.u.legacy.stencil_level[range->baseMipLevel]
> +                                                 : &image->surface.u.legacy.level[range->baseMipLevel],
> +                                      range->baseMipLevel,
> +                                      range->baseMipLevel,
> +                                      blk_w, is_stencil, descriptor);
> +}
> +
>  void
>  radv_image_view_init(struct radv_image_view *iview,
>                      struct radv_device *device,
> @@ -841,8 +885,7 @@ radv_image_view_init(struct radv_image_view *iview,
>  {
>         RADV_FROM_HANDLE(radv_image, image, pCreateInfo->image);
>         const VkImageSubresourceRange *range = &pCreateInfo->subresourceRange;
> -       uint32_t blk_w;
> -       bool is_stencil = false;
> +
>         switch (image->type) {
>         case VK_IMAGE_TYPE_1D:
>         case VK_IMAGE_TYPE_2D:
> @@ -862,7 +905,6 @@ radv_image_view_init(struct radv_image_view *iview,
>         iview->aspect_mask = pCreateInfo->subresourceRange.aspectMask;
>
>         if (iview->aspect_mask == VK_IMAGE_ASPECT_STENCIL_BIT) {
> -               is_stencil = true;
>                 iview->vk_format = vk_format_stencil_only(iview->vk_format);
>         } else if (iview->aspect_mask == VK_IMAGE_ASPECT_DEPTH_BIT) {
>                 iview->vk_format = vk_format_depth_only(iview->vk_format);
> @@ -879,30 +921,13 @@ radv_image_view_init(struct radv_image_view *iview,
>         iview->extent.height = round_up_u32(iview->extent.height * vk_format_get_blockheight(iview->vk_format),
>                                             vk_format_get_blockheight(image->vk_format));
>
> -       assert(image->surface.blk_w % vk_format_get_blockwidth(image->vk_format) == 0);
> -       blk_w = image->surface.blk_w / vk_format_get_blockwidth(image->vk_format) * vk_format_get_blockwidth(iview->vk_format);
>         iview->base_layer = range->baseArrayLayer;
>         iview->layer_count = radv_get_layerCount(image, range);
>         iview->base_mip = range->baseMipLevel;
>
> -       si_make_texture_descriptor(device, image, false,
> -                                  iview->type,
> -                                  iview->vk_format,
> -                                  &pCreateInfo->components,
> -                                  0, radv_get_levelCount(image, range) - 1,
> -                                  range->baseArrayLayer,
> -                                  range->baseArrayLayer + radv_get_layerCount(image, range) - 1,
> -                                  iview->extent.width,
> -                                  iview->extent.height,
> -                                  iview->extent.depth,
> -                                  iview->descriptor,
> -                                  iview->fmask_descriptor);
> -       si_set_mutable_tex_desc_fields(device, image,
> -                                      is_stencil ? &image->surface.u.legacy.stencil_level[range->baseMipLevel]
> -                                                 : &image->surface.u.legacy.level[range->baseMipLevel],
> -                                      range->baseMipLevel,
> -                                      range->baseMipLevel,
> -                                      blk_w, is_stencil, iview->descriptor);
> +       radv_image_view_make_descriptor(iview, device, pCreateInfo, false);
> +       if (image->usage & VK_IMAGE_USAGE_STORAGE_BIT)
> +               radv_image_view_make_descriptor(iview, device, pCreateInfo, true);
>  }
>
>  bool radv_layout_has_htile(const struct radv_image *image,
> diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
> index ac89fc1..5d5e609 100644
> --- a/src/amd/vulkan/radv_private.h
> +++ b/src/amd/vulkan/radv_private.h
> @@ -1271,6 +1271,12 @@ struct radv_image_view {
>
>         uint32_t descriptor[8];
>         uint32_t fmask_descriptor[8];
> +
> +       /* Descriptor for use as a storage image as opposed to a sampled image.
> +        * This has a few differences for cube maps (e.g. type).
> +        */
> +       uint32_t storage_descriptor[8];
> +       uint32_t storage_fmask_descriptor[8];
>  };
>
>  struct radv_image_create_info {
> --
> 2.9.4
>
On Fri, Jun 30, 2017 at 12:18 PM, Alex Smith
<asmith@feralinteractive.com> wrote:
> If a cube image has VK_IMAGE_USAGE_STORAGE_BIT set, the type in an image
> view's descriptor was set to a 2D array (and a few other fields adjusted
> accordingly). This is correct when the image view is actually bound as a
> storage image, but not when bound as a sampled image. In that case the
> type should be set as a cube.
>
> Fix by generating 2 sets of descriptors at view creation time for both
> storage and non-storage usage, and then choose between them based on
> descriptor type when writing descriptor sets.
>
> Signed-off-by: Alex Smith <asmith@feralinteractive.com>
> ---
>  src/amd/vulkan/radv_descriptor_set.c | 18 +++++++--
>  src/amd/vulkan/radv_image.c          | 77 ++++++++++++++++++++++++------------
>  src/amd/vulkan/radv_private.h        |  6 +++
>  3 files changed, 72 insertions(+), 29 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_descriptor_set.c b/src/amd/vulkan/radv_descriptor_set.c
> index ec7fd3d..b4a78aa 100644
> --- a/src/amd/vulkan/radv_descriptor_set.c
> +++ b/src/amd/vulkan/radv_descriptor_set.c
> @@ -603,11 +603,18 @@ write_image_descriptor(struct radv_device *device,
>                        struct radv_cmd_buffer *cmd_buffer,
>                        unsigned *dst,
>                        struct radeon_winsys_bo **buffer_list,
> +                      VkDescriptorType descriptor_type,
>                        const VkDescriptorImageInfo *image_info)
>  {
>         RADV_FROM_HANDLE(radv_image_view, iview, image_info->imageView);
> -       memcpy(dst, iview->descriptor, 8 * 4);
> -       memcpy(dst + 8, iview->fmask_descriptor, 8 * 4);
> +
> +       if (descriptor_type == VK_DESCRIPTOR_TYPE_STORAGE_IMAGE) {
> +               memcpy(dst, iview->storage_descriptor, 8 * 4);
> +               memcpy(dst + 8, iview->storage_fmask_descriptor, 8 * 4);
> +       } else {
> +               memcpy(dst, iview->descriptor, 8 * 4);
> +               memcpy(dst + 8, iview->fmask_descriptor, 8 * 4);
> +       }
>
>         if (cmd_buffer)
>                 device->ws->cs_add_buffer(cmd_buffer->cs, iview->bo, 7);
> @@ -620,12 +627,13 @@ write_combined_image_sampler_descriptor(struct radv_device *device,
>                                         struct radv_cmd_buffer *cmd_buffer,
>                                         unsigned *dst,
>                                         struct radeon_winsys_bo **buffer_list,
> +                                       VkDescriptorType descriptor_type,
>                                         const VkDescriptorImageInfo *image_info,
>                                         bool has_sampler)
>  {
>         RADV_FROM_HANDLE(radv_sampler, sampler, image_info->sampler);
>
> -       write_image_descriptor(device, cmd_buffer, dst, buffer_list, image_info);
> +       write_image_descriptor(device, cmd_buffer, dst, buffer_list, descriptor_type, image_info);
>         /* copy over sampler state */
>         if (has_sampler)
>                 memcpy(dst + 16, sampler->state, 16);
> @@ -696,10 +704,12 @@ void radv_update_descriptor_sets(
>                         case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE:
>                         case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT:
>                                 write_image_descriptor(device, cmd_buffer, ptr, buffer_list,
> +                                                      writeset->descriptorType,
>                                                        writeset->pImageInfo + j);
>                                 break;
>                         case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER:
>                                 write_combined_image_sampler_descriptor(device, cmd_buffer, ptr, buffer_list,
> +                                                                       writeset->descriptorType,
>                                                                         writeset->pImageInfo + j,
>                                                                         !binding_layout->immutable_samplers_offset);
>                                 if (copy_immutable_samplers) {
> @@ -866,10 +876,12 @@ void radv_update_descriptor_set_with_template(struct radv_device *device,
>                         case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE:
>                         case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT:
>                                 write_image_descriptor(device, cmd_buffer, pDst, buffer_list,
> +                                                      templ->entry[i].descriptor_type,
>                                                        (struct VkDescriptorImageInfo *) pSrc);
>                                 break;
>                         case VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER:
>                                 write_combined_image_sampler_descriptor(device, cmd_buffer, pDst, buffer_list,
> +                                                                       templ->entry[i].descriptor_type,
>                                                                         (struct VkDescriptorImageInfo *) pSrc,
>                                                                         templ->entry[i].has_sampler);
>                                 if (templ->entry[i].immutable_samplers)
> diff --git a/src/amd/vulkan/radv_image.c b/src/amd/vulkan/radv_image.c
> index 147ebed..54e7fc5 100644
> --- a/src/amd/vulkan/radv_image.c
> +++ b/src/amd/vulkan/radv_image.c
> @@ -325,7 +325,7 @@ static unsigned gfx9_border_color_swizzle(const unsigned char swizzle[4])
>  static void
>  si_make_texture_descriptor(struct radv_device *device,
>                            struct radv_image *image,
> -                          bool sampler,
> +                          bool is_storage_image,
>                            VkImageViewType view_type,
>                            VkFormat vk_format,
>                            const VkComponentMapping *mapping,
> @@ -362,7 +362,7 @@ si_make_texture_descriptor(struct radv_device *device,
>         }
>
>         type = radv_tex_dim(image->type, view_type, image->info.array_size, image->info.samples,
> -                           (image->usage & VK_IMAGE_USAGE_STORAGE_BIT));
> +                           is_storage_image);
>         if (type == V_008F1C_SQ_RSRC_IMG_1D_ARRAY) {
>                 height = 1;
>                 depth = image->info.array_size;
> @@ -526,7 +526,7 @@ radv_query_opaque_metadata(struct radv_device *device,
>         md->metadata[1] = si_get_bo_metadata_word1(device);
>
>
> -       si_make_texture_descriptor(device, image, true,
> +       si_make_texture_descriptor(device, image, false,
>                                    (VkImageViewType)image->type, image->vk_format,
>                                    &fixedmapping, 0, image->info.levels - 1, 0,
>                                    image->info.array_size,
> @@ -834,6 +834,50 @@ radv_image_create(VkDevice _device,
>         return VK_SUCCESS;
>  }
>
> +static void
> +radv_image_view_make_descriptor(struct radv_image_view *iview,
> +                               struct radv_device *device,
> +                               const VkImageViewCreateInfo* pCreateInfo,
> +                               bool is_storage_image)
> +{
> +       RADV_FROM_HANDLE(radv_image, image, pCreateInfo->image);
> +       const VkImageSubresourceRange *range = &pCreateInfo->subresourceRange;
> +       bool is_stencil = iview->aspect_mask == VK_IMAGE_ASPECT_STENCIL_BIT;
> +       uint32_t blk_w;
> +       uint32_t *descriptor;
> +       uint32_t *fmask_descriptor;
> +
> +       if (is_storage_image) {
> +               descriptor = iview->storage_descriptor;
> +               fmask_descriptor = iview->storage_fmask_descriptor;
> +       } else {
> +               descriptor = iview->descriptor;
> +               fmask_descriptor = iview->fmask_descriptor;
> +       }
> +
> +       assert(image->surface.blk_w % vk_format_get_blockwidth(image->vk_format) == 0);
> +       blk_w = image->surface.blk_w / vk_format_get_blockwidth(image->vk_format) * vk_format_get_blockwidth(iview->vk_format);
> +
> +       si_make_texture_descriptor(device, image, is_storage_image,
> +                                  iview->type,
> +                                  iview->vk_format,
> +                                  &pCreateInfo->components,
> +                                  0, radv_get_levelCount(image, range) - 1,
> +                                  range->baseArrayLayer,
> +                                  range->baseArrayLayer + radv_get_layerCount(image, range) - 1,
> +                                  iview->extent.width,
> +                                  iview->extent.height,
> +                                  iview->extent.depth,
> +                                  descriptor,
> +                                  fmask_descriptor);
> +       si_set_mutable_tex_desc_fields(device, image,
> +                                      is_stencil ? &image->surface.u.legacy.stencil_level[range->baseMipLevel]
> +                                                 : &image->surface.u.legacy.level[range->baseMipLevel],
> +                                      range->baseMipLevel,
> +                                      range->baseMipLevel,
> +                                      blk_w, is_stencil, descriptor);
> +}
> +
>  void
>  radv_image_view_init(struct radv_image_view *iview,
>                      struct radv_device *device,
> @@ -841,8 +885,7 @@ radv_image_view_init(struct radv_image_view *iview,
>  {
>         RADV_FROM_HANDLE(radv_image, image, pCreateInfo->image);
>         const VkImageSubresourceRange *range = &pCreateInfo->subresourceRange;
> -       uint32_t blk_w;
> -       bool is_stencil = false;
> +
>         switch (image->type) {
>         case VK_IMAGE_TYPE_1D:
>         case VK_IMAGE_TYPE_2D:
> @@ -862,7 +905,6 @@ radv_image_view_init(struct radv_image_view *iview,
>         iview->aspect_mask = pCreateInfo->subresourceRange.aspectMask;
>
>         if (iview->aspect_mask == VK_IMAGE_ASPECT_STENCIL_BIT) {
> -               is_stencil = true;
>                 iview->vk_format = vk_format_stencil_only(iview->vk_format);
>         } else if (iview->aspect_mask == VK_IMAGE_ASPECT_DEPTH_BIT) {
>                 iview->vk_format = vk_format_depth_only(iview->vk_format);
> @@ -879,30 +921,13 @@ radv_image_view_init(struct radv_image_view *iview,
>         iview->extent.height = round_up_u32(iview->extent.height * vk_format_get_blockheight(iview->vk_format),
>                                             vk_format_get_blockheight(image->vk_format));
>
> -       assert(image->surface.blk_w % vk_format_get_blockwidth(image->vk_format) == 0);
> -       blk_w = image->surface.blk_w / vk_format_get_blockwidth(image->vk_format) * vk_format_get_blockwidth(iview->vk_format);
>         iview->base_layer = range->baseArrayLayer;
>         iview->layer_count = radv_get_layerCount(image, range);
>         iview->base_mip = range->baseMipLevel;
>
> -       si_make_texture_descriptor(device, image, false,
> -                                  iview->type,
> -                                  iview->vk_format,
> -                                  &pCreateInfo->components,
> -                                  0, radv_get_levelCount(image, range) - 1,
> -                                  range->baseArrayLayer,
> -                                  range->baseArrayLayer + radv_get_layerCount(image, range) - 1,
> -                                  iview->extent.width,
> -                                  iview->extent.height,
> -                                  iview->extent.depth,
> -                                  iview->descriptor,
> -                                  iview->fmask_descriptor);
> -       si_set_mutable_tex_desc_fields(device, image,
> -                                      is_stencil ? &image->surface.u.legacy.stencil_level[range->baseMipLevel]
> -                                                 : &image->surface.u.legacy.level[range->baseMipLevel],
> -                                      range->baseMipLevel,
> -                                      range->baseMipLevel,
> -                                      blk_w, is_stencil, iview->descriptor);
> +       radv_image_view_make_descriptor(iview, device, pCreateInfo, false);
> +       if (image->usage & VK_IMAGE_USAGE_STORAGE_BIT)
> +               radv_image_view_make_descriptor(iview, device, pCreateInfo, true);

Also generate these for TRANSFER_DST (since we can use stores
internally), or just generate them unconditionally. With that,

Reviewed-by: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>

>  }
>
>  bool radv_layout_has_htile(const struct radv_image *image,
> diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
> index ac89fc1..5d5e609 100644
> --- a/src/amd/vulkan/radv_private.h
> +++ b/src/amd/vulkan/radv_private.h
> @@ -1271,6 +1271,12 @@ struct radv_image_view {
>
>         uint32_t descriptor[8];
>         uint32_t fmask_descriptor[8];
> +
> +       /* Descriptor for use as a storage image as opposed to a sampled image.
> +        * This has a few differences for cube maps (e.g. type).
> +        */
> +       uint32_t storage_descriptor[8];
> +       uint32_t storage_fmask_descriptor[8];
>  };
>
>  struct radv_image_create_info {
> --
> 2.9.4
>