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.
Thanks,
Alex
On 21 July 2017 at 02:20, Grazvydas Ignotas <nota...@gmail.com> wrote:
> For whatever reason this patch is breaking DOOM.
>
> Gražvydas
>
> On Wed, Jul 12, 2017 at 12:29 PM, Alex Smith
> <asm...@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.
> >
> > v2: Generate storage descriptors for images with TRANSFER_DST, since
> > those may be used as storage images internally.
> >
> > Signed-off-by: Alex Smith <asm...@feralinteractive.com>
> > Reviewed-by: Bas Nieuwenhuizen <b...@basnieuwenhuizen.nl>
> > ---
> > 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
> >
> > _______________________________________________
> > 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
