From 0a93c0364c3bfcfee9ddea2d0ffdd989a32b274e Mon Sep 17 00:00:00 2001 From: Jason Ekstrand Date: Thu, 29 Jul 2021 16:41:14 -0500 Subject: [PATCH] anv/image: Rework YCbCr image aspects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Vulkan 1.2.184 spec says: "When creating a VkImageView, if sampler Y′CBCR conversion is enabled in the sampler, the aspectMask of a subresourceRange used by the VkImageView must be VK_IMAGE_ASPECT_COLOR_BIT. When creating a VkImageView, if sampler Y′CBCR conversion is not enabled in the sampler and the image format is multi-planar, the image must have been created with VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT, and the aspectMask of the VkImageView’s subresourceRange must be VK_IMAGE_ASPECT_PLANE_0_BIT, VK_IMAGE_ASPECT_PLANE_1_BIT or VK_IMAGE_ASPECT_PLANE_2_BIT." Previously, for YCbCr images, we were flipping this around. For single- plane views where VK_IMAGE_ASPECT_PLANE_N_BIT would be passed in by the app, we would store VK_IMAGE_ASPECT_COLOR_BIT. For multi-plane views where the client says VK_IMAGE_ASPECT_COLOR_BIT, we would store a all of the planes. (There was also an extra bit of remapping that would compact the planes in the non-existent case of a format with a non- contiguous set of planes.) The idea behind this was that for things like rendering or single-plane sampling, storage, or compute, we want it to look as much like a single-plane image as possible but we wanted the multi-plane case to be the awkward one. This commit changes it around so that iview->aspects is always exactly the subset of image->vk.aspects represented by the view. This is identical to how aspects work for depth/stencil so it gains us some consistency. This commit also changes anv_image_view::aspect_mask to aspects to force a full audit of the field. As can be seen, there are only a few uses of this field and they're all mostly fine: - A bunch of them are used to check for depth/stencil. That hasn't changed. - Most of the checks for color already used ANY_COLOR_BIT, only one needed fixing. - There's a check that both src/depth are color for MSAA resolves. However, we don't support MSAA on YCbCr so there's no point in checking for ANY_COLOR_BIT. There is a hidden usage of planes in anv_descriptor_set_write_image_view that's not as obvious. However, this function simply looks at anv_image_view::n_planes and blindly fills out the descriptor accordingly. As long as image views with a single plane continue to claim n_planes == 1, this will be fine. Reviewed-by: Lionel Landwerlin Part-of: --- src/intel/vulkan/anv_cmd_buffer.c | 4 +-- src/intel/vulkan/anv_image.c | 42 +++++++----------------------- src/intel/vulkan/anv_private.h | 19 ++++++++++---- src/intel/vulkan/genX_cmd_buffer.c | 10 +++---- 4 files changed, 30 insertions(+), 45 deletions(-) diff --git a/src/intel/vulkan/anv_cmd_buffer.c b/src/intel/vulkan/anv_cmd_buffer.c index 5b1a354a7f8..08cfad79eab 100644 --- a/src/intel/vulkan/anv_cmd_buffer.c +++ b/src/intel/vulkan/anv_cmd_buffer.c @@ -1332,8 +1332,8 @@ anv_cmd_buffer_get_depth_stencil_view(const struct anv_cmd_buffer *cmd_buffer) const struct anv_image_view *iview = cmd_buffer->state.attachments[subpass->depth_stencil_attachment->attachment].image_view; - assert(iview->aspect_mask & (VK_IMAGE_ASPECT_DEPTH_BIT | - VK_IMAGE_ASPECT_STENCIL_BIT)); + assert(iview->aspects & (VK_IMAGE_ASPECT_DEPTH_BIT | + VK_IMAGE_ASPECT_STENCIL_BIT)); return iview; } diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c index 5c2c682d02c..e08e7297d1f 100644 --- a/src/intel/vulkan/anv_image.c +++ b/src/intel/vulkan/anv_image.c @@ -2641,22 +2641,6 @@ anv_image_fill_surface_state(struct anv_device *device, } } -static VkImageAspectFlags -remap_aspect_flags(VkImageAspectFlags view_aspects) -{ - if (view_aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) { - if (util_bitcount(view_aspects) == 1) - return VK_IMAGE_ASPECT_COLOR_BIT; - - VkImageAspectFlags color_aspects = 0; - for (uint32_t i = 0; i < util_bitcount(view_aspects); i++) - color_aspects |= VK_IMAGE_ASPECT_PLANE_0_BIT << i; - return color_aspects; - } - /* No special remapping needed for depth & stencil aspects. */ - return view_aspects; -} - static uint32_t anv_image_aspect_get_planes(VkImageAspectFlags aspect_mask) { @@ -2773,24 +2757,16 @@ anv_CreateImageView(VkDevice _device, break; } + iview->image = image; + /* First expand aspects to the image's ones (for example * VK_IMAGE_ASPECT_COLOR_BIT will be converted to * VK_IMAGE_ASPECT_PLANE_0_BIT | VK_IMAGE_ASPECT_PLANE_1_BIT | * VK_IMAGE_ASPECT_PLANE_2_BIT for an image of format * VK_FORMAT_G8_B8_R8_3PLANE_420_UNORM. */ - VkImageAspectFlags expanded_aspects = - anv_image_expand_aspects(image, range->aspectMask); - - iview->image = image; - - /* Remap the expanded aspects for the image view. For example if only - * VK_IMAGE_ASPECT_PLANE_1_BIT was given in range->aspectMask, we will - * convert it to VK_IMAGE_ASPECT_COLOR_BIT since from the point of view of - * the image view, it only has a single plane. - */ - iview->aspect_mask = remap_aspect_flags(expanded_aspects); - iview->n_planes = anv_image_aspect_get_planes(iview->aspect_mask); + iview->aspects = anv_image_expand_aspects(image, range->aspectMask); + iview->n_planes = anv_image_aspect_get_planes(iview->aspects); iview->vk_format = pCreateInfo->format; /* "If image has an external format, format must be VK_FORMAT_UNDEFINED." */ @@ -2809,14 +2785,14 @@ anv_CreateImageView(VkDevice _device, .depth = anv_minify(image->extent.depth , range->baseMipLevel), }; - /* Now go through the underlying image selected planes (computed in - * expanded_aspects) and map them to planes in the image view. + /* Now go through the underlying image selected planes and map them to + * planes in the image view. */ - anv_foreach_image_aspect_bit(iaspect_bit, image, expanded_aspects) { + anv_foreach_image_aspect_bit(iaspect_bit, image, iview->aspects) { const uint32_t iplane = anv_image_aspect_to_plane(image->aspects, 1UL << iaspect_bit); const uint32_t vplane = - anv_image_aspect_to_plane(expanded_aspects, 1UL << iaspect_bit); + anv_image_aspect_to_plane(iview->aspects, 1UL << iaspect_bit); struct anv_format_plane format; if (image->aspects & (VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT)) { @@ -2883,7 +2859,7 @@ anv_CreateImageView(VkDevice _device, if (view_usage & VK_IMAGE_USAGE_SAMPLED_BIT || (view_usage & VK_IMAGE_USAGE_INPUT_ATTACHMENT_BIT && - !(iview->aspect_mask & VK_IMAGE_ASPECT_COLOR_BIT))) { + !(iview->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV))) { iview->planes[vplane].optimal_sampler_surface_state.state = alloc_surface_state(device); iview->planes[vplane].general_sampler_surface_state.state = alloc_surface_state(device); diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index e89bdd55b52..e293bbbb67a 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -3781,22 +3781,31 @@ struct anv_format { }; /** + * Return the aspect's plane relative to all_aspects. For an image, for + * instance, all_aspects would be the set of aspects in the image. For + * an image view, all_aspects would be the subset of aspects represented + * by that particular view. + * * Return the aspect's _format_ plane, not its _memory_ plane (using the * vocabulary of VK_EXT_image_drm_format_modifier). As a consequence, \a * aspect_mask may contain VK_IMAGE_ASPECT_PLANE_*, but must not contain * VK_IMAGE_ASPECT_MEMORY_PLANE_* . */ static inline uint32_t -anv_image_aspect_to_plane(VkImageAspectFlags image_aspects, - VkImageAspectFlags aspect_mask) +anv_image_aspect_to_plane(VkImageAspectFlags all_aspects, + VkImageAspectFlagBits aspect) { - switch (aspect_mask) { + assert(util_bitcount(aspect) == 1); + if (util_bitcount(all_aspects) == 1) + return 0; + + switch (aspect) { case VK_IMAGE_ASPECT_COLOR_BIT: case VK_IMAGE_ASPECT_DEPTH_BIT: case VK_IMAGE_ASPECT_PLANE_0_BIT: return 0; case VK_IMAGE_ASPECT_STENCIL_BIT: - if ((image_aspects & VK_IMAGE_ASPECT_DEPTH_BIT) == 0) + if ((all_aspects & VK_IMAGE_ASPECT_DEPTH_BIT) == 0) return 0; FALLTHROUGH; case VK_IMAGE_ASPECT_PLANE_1_BIT: @@ -4373,7 +4382,7 @@ struct anv_image_view { const struct anv_image *image; /**< VkImageViewCreateInfo::image */ - VkImageAspectFlags aspect_mask; + VkImageAspectFlags aspects; VkFormat vk_format; VkExtent3D extent; /**< Extent of VkImageViewCreateInfo::baseMipLevel. */ diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c index 4ad14393848..5913265abf2 100644 --- a/src/intel/vulkan/genX_cmd_buffer.c +++ b/src/intel/vulkan/genX_cmd_buffer.c @@ -2723,7 +2723,7 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer, case VK_DESCRIPTOR_TYPE_INPUT_ATTACHMENT: assert(shader->stage == MESA_SHADER_FRAGMENT); assert(desc->image_view != NULL); - if ((desc->image_view->aspect_mask & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) == 0) { + if ((desc->image_view->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) == 0) { /* For depth and stencil input attachments, we treat it like any * old texture that a user may have bound. */ @@ -6342,7 +6342,7 @@ cmd_buffer_end_subpass(struct anv_cmd_buffer *cmd_buffer) * with depth. */ const struct isl_view *ds_view = &iview->planes[0].isl; - if (iview->aspect_mask & VK_IMAGE_ASPECT_DEPTH_BIT) { + if (iview->aspects & VK_IMAGE_ASPECT_DEPTH_BIT) { genX(cmd_buffer_mark_image_written)(cmd_buffer, iview->image, VK_IMAGE_ASPECT_DEPTH_BIT, att_state->aux_usage, @@ -6350,7 +6350,7 @@ cmd_buffer_end_subpass(struct anv_cmd_buffer *cmd_buffer) ds_view->base_array_layer, fb->layers); } - if (iview->aspect_mask & VK_IMAGE_ASPECT_STENCIL_BIT) { + if (iview->aspects & VK_IMAGE_ASPECT_STENCIL_BIT) { /* Even though stencil may be plane 1, it always shares a * base_level with depth. */ @@ -6405,8 +6405,8 @@ cmd_buffer_end_subpass(struct anv_cmd_buffer *cmd_buffer) enum isl_aux_usage dst_aux_usage = cmd_buffer->state.attachments[dst_att].aux_usage; - assert(src_iview->aspect_mask == VK_IMAGE_ASPECT_COLOR_BIT && - dst_iview->aspect_mask == VK_IMAGE_ASPECT_COLOR_BIT); + assert(src_iview->aspects == VK_IMAGE_ASPECT_COLOR_BIT && + dst_iview->aspects == VK_IMAGE_ASPECT_COLOR_BIT); anv_image_msaa_resolve(cmd_buffer, src_iview->image, src_aux_usage,