<div dir="ltr">Hi Gražvydas, I don't have Doom available to test. How's it broken? Could you see if removing the usage flags condition on the second call to radv_image_view_make_descriptor makes any difference? Otherwise I'm not sure what's wrong - as far as I could tell this should be making RADV do basically the same as what radeonsi does.<div> </div><div>Thanks,</div><div>Alex</div></div><div class="gmail_extra"> <div class="gmail_quote">On 21 July 2017 at 02:20, Grazvydas Ignotas <<a href="mailto:notasas@gmail.com" target="_blank">notasas@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">For whatever reason this patch is breaking DOOM. Gražvydas <div><div class="h5"> On Wed, Jul 12, 2017 at 12:29 PM, Alex Smith <<a href="mailto:asmith@feralinteractive.com">asmith@feralinteractive.com</a>> 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. > > v2: Generate storage descriptors for images with TRANSFER_DST, since > those may be used as storage images internally. > > Signed-off-by: Alex Smith <<a href="mailto:asmith@feralinteractive.com">asmith@feralinteractive.com</a>> > Reviewed-by: Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl">bas@basnieuwenhuizen.nl</a>> > --- > src/amd/vulkan/radv_descriptor_set.c | 18 ++++++-- > src/amd/vulkan/radv_image.c | 79 ++++++++++++++++++++++++------------ > src/amd/vulkan/radv_private.h | 6 +++ > 3 files changed, 74 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 115e5a5..245bca3 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,15 @@ 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); > + > + /* For transfers we may use the image as a storage image. */ > + if (image->usage & (VK_IMAGE_USAGE_STORAGE_BIT | VK_IMAGE_USAGE_TRANSFER_DST_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 a167409..a8bc5c9 100644 > --- a/src/amd/vulkan/radv_private.h > +++ b/src/amd/vulkan/radv_private.h > @@ -1276,6 +1276,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 > </div></div>> _______________________________________________ > 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>