From 7cc3f046e0d6354747338de578fcaef883690e73 Mon Sep 17 00:00:00 2001 From: Chad Versace Date: Mon, 23 May 2022 16:53:23 -0700 Subject: [PATCH] venus: Remove cryptic pointer arithmetic on present attachments Each time some code wanted to access in vn_render_pass the present acquire attachments or the present release attachments, it needed to know the memory layout of vn_render_pass and do pointer arithmetic. It's fragile to handroll this layout knowledge and pointer arithmetic throughout the code, so add a pointer in vn_render_pass for each attachment type. The new pointers are: - present_attachments: all present attachments - present_acquire_attachments: a subslice of present_attachments - present_release_attachments: a subslice of present_attachments Signed-off-by: Chad Versace Reviewed-by: Yiwei Zhang Part-of: --- src/virtio/vulkan/vn_command_buffer.c | 25 ++++----- src/virtio/vulkan/vn_render_pass.c | 76 +++++++++++++++++---------- src/virtio/vulkan/vn_render_pass.h | 24 ++++++--- 3 files changed, 78 insertions(+), 47 deletions(-) diff --git a/src/virtio/vulkan/vn_command_buffer.c b/src/virtio/vulkan/vn_command_buffer.c index 2d89fbadbef..bab69feae52 100644 --- a/src/virtio/vulkan/vn_command_buffer.c +++ b/src/virtio/vulkan/vn_command_buffer.c @@ -366,7 +366,7 @@ vn_cmd_begin_render_pass(struct vn_command_buffer *cmd, cmd->builder.render_pass = pass; cmd->builder.framebuffer = fb; - if (!pass->present_src_count || + if (!pass->present_count || cmd->level == VK_COMMAND_BUFFER_LEVEL_SECONDARY) return; @@ -386,22 +386,23 @@ vn_cmd_begin_render_pass(struct vn_command_buffer *cmd, } const struct vn_image **images = - vk_alloc(&cmd->allocator, sizeof(*images) * pass->present_src_count, + vk_alloc(&cmd->allocator, sizeof(*images) * pass->present_count, VN_DEFAULT_ALIGN, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); if (!images) { cmd->state = VN_COMMAND_BUFFER_STATE_INVALID; return; } - for (uint32_t i = 0; i < pass->present_src_count; i++) { - const uint32_t index = pass->present_src_attachments[i].index; + for (uint32_t i = 0; i < pass->present_count; i++) { + const uint32_t index = pass->present_attachments[i].index; assert(index < view_count); images[i] = vn_image_view_from_handle(views[index])->image; } - if (pass->acquire_count) { - vn_cmd_transfer_present_src_images( - cmd, images, pass->present_src_attachments, pass->acquire_count); + if (pass->present_acquire_count) { + vn_cmd_transfer_present_src_images(cmd, images, + pass->present_acquire_attachments, + pass->present_acquire_count); } cmd->builder.present_src_images = images; @@ -415,17 +416,17 @@ vn_cmd_end_render_pass(struct vn_command_buffer *cmd) cmd->builder.render_pass = NULL; cmd->builder.framebuffer = NULL; - if (!pass->present_src_count || !cmd->builder.present_src_images) + if (!pass->present_count || !cmd->builder.present_src_images) return; const struct vn_image **images = cmd->builder.present_src_images; cmd->builder.present_src_images = NULL; - if (pass->release_count) { + if (pass->present_release_count) { vn_cmd_transfer_present_src_images( - cmd, images + pass->acquire_count, - pass->present_src_attachments + pass->acquire_count, - pass->release_count); + cmd, images + pass->present_acquire_count, + pass->present_release_attachments, + pass->present_release_count); } vk_free(&cmd->allocator, images); diff --git a/src/virtio/vulkan/vn_render_pass.c b/src/virtio/vulkan/vn_render_pass.c index 777b893112a..c3bfd11de93 100644 --- a/src/virtio/vulkan/vn_render_pass.c +++ b/src/virtio/vulkan/vn_render_pass.c @@ -31,9 +31,9 @@ #define REPLACE_PRESENT_SRC(pass, atts, att_count, out_atts) \ do { \ struct vn_present_src_attachment *_acquire_atts = \ - pass->present_src_attachments; \ + pass->present_acquire_attachments; \ struct vn_present_src_attachment *_release_atts = \ - _acquire_atts + pass->acquire_count; \ + pass->present_release_attachments; \ \ memcpy(out_atts, atts, sizeof(*atts) * att_count); \ for (uint32_t i = 0; i < att_count; i++) { \ @@ -92,46 +92,64 @@ static void vn_render_pass_setup_present_src_barriers(struct vn_render_pass *pass) { /* TODO parse VkSubpassDependency for more accurate barriers */ - for (uint32_t i = 0; i < pass->present_src_count; i++) { + + for (uint32_t i = 0; i < pass->present_acquire_count; i++) { struct vn_present_src_attachment *att = - &pass->present_src_attachments[i]; + &pass->present_acquire_attachments[i]; - if (att->acquire) { - att->src_stage_mask = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT; - att->src_access_mask = 0; + att->src_stage_mask = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT; + att->src_access_mask = 0; + att->dst_stage_mask = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT; + att->dst_access_mask = + VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_MEMORY_WRITE_BIT; + } - att->dst_stage_mask = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT; - att->dst_access_mask = - VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_MEMORY_WRITE_BIT; - } else { - att->src_stage_mask = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT; - att->src_access_mask = VK_ACCESS_MEMORY_WRITE_BIT; + for (uint32_t i = 0; i < pass->present_release_count; i++) { + struct vn_present_src_attachment *att = + &pass->present_release_attachments[i]; - att->dst_stage_mask = VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT; - att->dst_access_mask = 0; - } + att->src_stage_mask = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT; + att->src_access_mask = VK_ACCESS_MEMORY_WRITE_BIT; + att->dst_stage_mask = VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT; + att->dst_access_mask = 0; } } static struct vn_render_pass * vn_render_pass_create(struct vn_device *dev, - uint32_t acquire_count, - uint32_t release_count, + uint32_t present_acquire_count, + uint32_t present_release_count, const VkAllocationCallbacks *alloc) { - const uint32_t total_count = acquire_count + release_count; - struct vn_render_pass *pass = vk_zalloc( - alloc, - sizeof(*pass) + sizeof(pass->present_src_attachments[0]) * total_count, - VN_DEFAULT_ALIGN, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); - if (!pass) + uint32_t present_count = present_acquire_count + present_release_count; + struct vn_render_pass *pass; + struct vn_present_src_attachment *present_atts; + + VK_MULTIALLOC(ma); + vk_multialloc_add(&ma, &pass, __typeof__(*pass), 1); + vk_multialloc_add(&ma, &present_atts, __typeof__(*present_atts), + present_count); + + if (!vk_multialloc_zalloc(&ma, alloc, VK_SYSTEM_ALLOCATION_SCOPE_OBJECT)) return NULL; vn_object_base_init(&pass->base, VK_OBJECT_TYPE_RENDER_PASS, &dev->base); - pass->acquire_count = acquire_count; - pass->release_count = release_count; - pass->present_src_count = total_count; + pass->present_count = present_count; + pass->present_acquire_count = present_acquire_count; + pass->present_release_count = present_release_count; + + /* For each array pointer, set it only if its count != 0. This allows code + * elsewhere to intuitively use either condition, `foo_atts == NULL` or + * `foo_count != 0`. + */ + if (present_count) + pass->present_attachments = present_atts; + if (present_acquire_count) + pass->present_acquire_attachments = present_atts; + if (present_release_count) + pass->present_release_attachments = + present_atts + present_acquire_count; return pass; } @@ -159,7 +177,7 @@ vn_CreateRenderPass(VkDevice device, return vn_error(dev->instance, VK_ERROR_OUT_OF_HOST_MEMORY); VkRenderPassCreateInfo local_pass_info; - if (pass->present_src_count) { + if (pass->present_count) { VkAttachmentDescription *temp_atts = vk_alloc(alloc, sizeof(*temp_atts) * pCreateInfo->attachmentCount, VN_DEFAULT_ALIGN, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); @@ -209,7 +227,7 @@ vn_CreateRenderPass2(VkDevice device, return vn_error(dev->instance, VK_ERROR_OUT_OF_HOST_MEMORY); VkRenderPassCreateInfo2 local_pass_info; - if (pass->present_src_count) { + if (pass->present_count) { VkAttachmentDescription2 *temp_atts = vk_alloc(alloc, sizeof(*temp_atts) * pCreateInfo->attachmentCount, VN_DEFAULT_ALIGN, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); diff --git a/src/virtio/vulkan/vn_render_pass.h b/src/virtio/vulkan/vn_render_pass.h index f96ae502346..db8b285a7f3 100644 --- a/src/virtio/vulkan/vn_render_pass.h +++ b/src/virtio/vulkan/vn_render_pass.h @@ -29,14 +29,26 @@ struct vn_render_pass { VkExtent2D granularity; - /* track attachments that have PRESENT_SRC as their initialLayout or - * finalLayout + uint32_t present_count; + uint32_t present_acquire_count; + uint32_t present_release_count; + + /* Attachments where initialLayout or finalLayout was + * VK_IMAGE_LAYOUT_PRESENT_SRC_KHR. */ - uint32_t acquire_count; - uint32_t release_count; - uint32_t present_src_count; - struct vn_present_src_attachment present_src_attachments[]; + struct vn_present_src_attachment *present_attachments; + + /* Slice of present_attachments where initialLayout was + * VK_IMAGE_LAYOUT_PRESENT_SRC_KHR. + */ + struct vn_present_src_attachment *present_acquire_attachments; + + /* Slice of present_attachments where finalLayout was + * VK_IMAGE_LAYOUT_PRESENT_SRC_KHR. + */ + struct vn_present_src_attachment *present_release_attachments; }; + VK_DEFINE_NONDISP_HANDLE_CASTS(vn_render_pass, base.base, VkRenderPass,