From bdf2a470d3b5a710a585f39ecb5639add96c6275 Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Tue, 2 Apr 2024 10:56:46 +0200 Subject: [PATCH] v3dv: fix job suspend with command buffer simultaneous use flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the simultaneous use flag we can reuse the same command buffer multiple times. That means, for example, that we can have an instance of a job running in the GPU while we are submitting another one for execution to a queue. This scenario is problematic with dynamic rendering and job suspension because suspended jobs need to be patched with the resume address at queue submit time, and thus, if we have another instance of the same job currently executing in the GPU we could stomp its resume address, which could be different. To fix this, at queue submission time, when we detect a suspending job in a command buffer with the simultaneous use flag, we clone the job and create its own copy of the BCL so we can patch the resume address into it safely without conflicting with any other instance of the job that may be running. We need to flag these clones as having their own BCL since we would have to free it when the job is destroyed, unlike other clones that don't own any resources of their own. Also, because this job is created at queue submit time, it won't be in the execution list of the command buffer, so it won't be automatically destroyed with it, so we need to add it to the command buffer as a private object. Reviewed-by: Alejandro Piñeiro Part-of: --- src/broadcom/vulkan/v3dv_cmd_buffer.c | 37 +++++++-- src/broadcom/vulkan/v3dv_private.h | 8 +- src/broadcom/vulkan/v3dv_queue.c | 12 +-- src/broadcom/vulkan/v3dvx_cmd_buffer.c | 108 +++++++++++++++++++++++++ src/broadcom/vulkan/v3dvx_private.h | 3 + 5 files changed, 155 insertions(+), 13 deletions(-) diff --git a/src/broadcom/vulkan/v3dv_cmd_buffer.c b/src/broadcom/vulkan/v3dv_cmd_buffer.c index 6d9e198b05f..7b49a7aadec 100644 --- a/src/broadcom/vulkan/v3dv_cmd_buffer.c +++ b/src/broadcom/vulkan/v3dv_cmd_buffer.c @@ -149,9 +149,21 @@ job_destroy_cloned_gpu_cl_resources(struct v3dv_job *job) { assert(job->type == V3DV_JOB_TYPE_GPU_CL); - list_for_each_entry_safe(struct v3dv_bo, bo, &job->bcl.bo_list, list_link) { - list_del(&bo->list_link); - vk_free(&job->device->vk.alloc, bo); + struct v3dv_cmd_buffer *cmd_buffer = job->cmd_buffer; + if (job->clone_owns_bcl) { + /* For suspending jobs in command buffers with the simultaneous use flag + * we allocate a real copy of the BCL. + */ + assert(job->suspending && + cmd_buffer && + (cmd_buffer->usage_flags & + VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT)); + v3dv_cl_destroy(&job->bcl); + } else { + list_for_each_entry_safe(struct v3dv_bo, bo, &job->bcl.bo_list, list_link) { + list_del(&bo->list_link); + vk_free(&job->device->vk.alloc, bo); + } } list_for_each_entry_safe(struct v3dv_bo, bo, &job->rcl.bo_list, list_link) { @@ -1923,7 +1935,7 @@ clone_bo_list(struct v3dv_device *device, } struct v3dv_job * -v3dv_job_clone(struct v3dv_job *job) +v3dv_job_clone(struct v3dv_job *job, bool skip_bcl) { struct v3dv_job *clone = vk_alloc(&job->device->vk.alloc, sizeof(struct v3dv_job), 8, @@ -1931,7 +1943,16 @@ v3dv_job_clone(struct v3dv_job *job) if (!clone) return NULL; - /* Cloned jobs don't duplicate resources! */ + /* Cloned jobs don't duplicate resources, they share their CLs with the + * oringinal job, since they are typically read-only. The exception to this + * is dynamic rendering suspension paired with + * VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT, since in that case we need + * to patch the BCL with the resume address and for that we need to create a + * copy of the job so we avoid rewriting the resume address for another copy + * of the same job that may be running in the GPU. When we create a job for + * this use case skip_bcl is set to True and the caller will be responsible + * for creating the BCL. + */ *clone = *job; clone->is_clone = true; clone->cmd_buffer = NULL; @@ -1947,8 +1968,10 @@ v3dv_job_clone(struct v3dv_job *job) clone->rcl.job = clone; clone->indirect.job = clone; - if (!clone_bo_list(device, &clone->bcl.bo_list, &job->bcl.bo_list)) + if (!skip_bcl && + !clone_bo_list(device, &clone->bcl.bo_list, &job->bcl.bo_list)) { return NULL; + } if (!clone_bo_list(device, &clone->rcl.bo_list, &job->rcl.bo_list)) return NULL; if (!clone_bo_list(device, &clone->indirect.bo_list, &job->indirect.bo_list)) @@ -1968,7 +1991,7 @@ struct v3dv_job * v3dv_job_clone_in_cmd_buffer(struct v3dv_job *job, struct v3dv_cmd_buffer *cmd_buffer) { - struct v3dv_job *clone = v3dv_job_clone(job); + struct v3dv_job *clone = v3dv_job_clone(job, false); if (!clone) { v3dv_flag_oom(cmd_buffer, NULL); return NULL; diff --git a/src/broadcom/vulkan/v3dv_private.h b/src/broadcom/vulkan/v3dv_private.h index fe2611ff40c..cab80f7e172 100644 --- a/src/broadcom/vulkan/v3dv_private.h +++ b/src/broadcom/vulkan/v3dv_private.h @@ -1235,6 +1235,12 @@ struct v3dv_job { */ bool is_clone; + /* If this is a cloned job, if it has its own BCL resource. This happens + * when we suspend jobs with in command buffers with the + * VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT flag. + */ + bool clone_owns_bcl; + /* VK_KHR_dynamic_rendering */ bool suspending; bool resuming; @@ -1398,7 +1404,7 @@ void v3dv_job_start_frame(struct v3dv_job *job, bool v3dv_job_type_is_gpu(struct v3dv_job *job); struct v3dv_job * -v3dv_job_clone(struct v3dv_job *job); +v3dv_job_clone(struct v3dv_job *job, bool skip_bcl); struct v3dv_job * v3dv_job_clone_in_cmd_buffer(struct v3dv_job *job, diff --git a/src/broadcom/vulkan/v3dv_queue.c b/src/broadcom/vulkan/v3dv_queue.c index 41277f04059..ac981984c4f 100644 --- a/src/broadcom/vulkan/v3dv_queue.c +++ b/src/broadcom/vulkan/v3dv_queue.c @@ -1228,11 +1228,6 @@ v3dv_queue_driver_submit(struct vk_queue *vk_queue, for (int i = 0; i < V3DV_QUEUE_COUNT; i++) queue->last_job_syncs.first[i] = true; - /* FIXME: if suspend/resume chains are recorded into command buffers with - * usage flag VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT, then this won't - * work and we would need to "clone" the jobs instead so we never patch a - * job that may still be executing. - */ struct v3dv_job *first_suspend_job = NULL; struct v3dv_job *current_suspend_job = NULL; for (uint32_t i = 0; i < submit->command_buffer_count; i++) { @@ -1240,6 +1235,13 @@ v3dv_queue_driver_submit(struct vk_queue *vk_queue, container_of(submit->command_buffers[i], struct v3dv_cmd_buffer, vk); list_for_each_entry_safe(struct v3dv_job, job, &cmd_buffer->jobs, list_link) { + if (job->suspending) { + job = v3dv_X(job->device, + cmd_buffer_prepare_suspend_job_for_submit)(job); + if (!job) + return VK_ERROR_OUT_OF_DEVICE_MEMORY; + } + if (job->suspending && !job->resuming) { assert(!first_suspend_job); assert(!current_suspend_job); diff --git a/src/broadcom/vulkan/v3dvx_cmd_buffer.c b/src/broadcom/vulkan/v3dvx_cmd_buffer.c index fa64646ff35..e15343b442a 100644 --- a/src/broadcom/vulkan/v3dvx_cmd_buffer.c +++ b/src/broadcom/vulkan/v3dvx_cmd_buffer.c @@ -2776,3 +2776,111 @@ v3dX(job_patch_resume_address)(struct v3dv_job *first_suspend, first_suspend->suspended_bcl_end = resume->bcl.bo->offset + v3dv_cl_offset(&resume->bcl); } + +static void +job_destroy_cb(VkDevice device, uint64_t pobj, VkAllocationCallbacks *allocb) +{ + struct v3dv_job *clone = (struct v3dv_job *) (uintptr_t) pobj; + v3dv_job_destroy(clone); +} + +/** + * This checks if the command buffer has been created with + * VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT, in which case we won't be + * able to safely patch the resume address into the job (since we could have + * another instance of this job running in the GPU, potentially resuming in a + * different address). In that case, we clone the job and make the clone have + * its own BCL copied from the original job so we can later patch the resume + * address into it safely. + */ +struct v3dv_job * +v3dX(cmd_buffer_prepare_suspend_job_for_submit)(struct v3dv_job *job) +{ + assert(job->suspending); + assert(job->cmd_buffer); + assert(job->type == V3DV_JOB_TYPE_GPU_CL); + + if (!(job->cmd_buffer->usage_flags & VK_COMMAND_BUFFER_USAGE_SIMULTANEOUS_USE_BIT)) + return job; + + /* Create the clone job, but skip the BCL since we are going to create + * our own below. + */ + struct v3dv_job *clone = v3dv_job_clone(job, true); + if (!clone) + return NULL; + + /* Compute total size of BCL we need to copy */ + uint32_t bcl_size = 0; + list_for_each_entry(struct v3dv_bo, bo, &job->bcl.bo_list, list_link) + bcl_size += bo->size; + + /* Prepare the BCL for the cloned job. For this we go over the BOs in the + * BCL of the original job and we copy their contents into the single BO + * in the BCL of the cloned job. + */ + clone->clone_owns_bcl = true; + v3dv_cl_init(clone, &clone->bcl); + v3dv_cl_ensure_space(&clone->bcl, bcl_size, 4); + if (!clone->bcl.bo) + return NULL; + + assert(clone->bcl.base); + assert(clone->bcl.base == clone->bcl.next); + + /* Unlink this job from the command buffer's execution list */ + list_inithead(&clone->list_link); + + /* Copy the contents of each BO in the original job's BCL into the single + * BO we have in the clone's BCL. + * + * If the BO is the last in the BCL (which we can tell because it wouldn't + * have emitted a BRANCH instruction to link to another BO) we need to copy + * up to the current BCL offset, otherwise we need to copy up to the BRANCH + * instruction (excluded, since we are putting everything together into a + * single BO here). + */ + list_for_each_entry(struct v3dv_bo, bo, &job->bcl.bo_list, list_link) { + assert(bo->map); + uint32_t copy_size; + if (bo->cl_branch_offset == 0xffffffff) { /* Last BO in BCL */ + assert(bo == list_last_entry(&job->bcl.bo_list, struct v3dv_bo, list_link)); + copy_size = v3dv_cl_offset(&job->bcl); + } else { + assert(bo->cl_branch_offset >= cl_packet_length(BRANCH)); + copy_size = bo->cl_branch_offset - cl_packet_length(BRANCH); + } + + assert(v3dv_cl_offset(&job->bcl) + copy_size < bcl_size); + memcpy(cl_start(&clone->bcl), bo->map, copy_size); + cl_advance_and_end(&clone->bcl, copy_size); + } + + /* Now we need to fixup the pointer to the suspend BRANCH instruction at the + * end of the BCL so it points to the address in the new BCL. We know that + * to suspend a command buffer we always emit a BRANCH+NOP combo, so we just + * need to go back that many bytes in to the BCL to find the instruction. + */ + uint32_t suspend_terminator_size = + cl_packet_length(BRANCH) + cl_packet_length(NOP); + clone->suspend_branch_inst_ptr = (struct v3dv_cl_out *) + (((uint8_t *)cl_start(&clone->bcl)) - suspend_terminator_size); + assert(*(((uint8_t *)clone->suspend_branch_inst_ptr)) == V3DX(BRANCH_opcode)); + + /* This job is not in the execution list of the command buffer so it + * won't be destroyed with it; add it as a private object to get it freed. + * + * FIXME: every time this job is submitted we clone the job and we only + * destroy it when the command buffer is destroyed. If the user keeps the + * command buffer for the entire lifetime of the application, this command + * buffer could grow significantly, so maybe we want to do something smarter + * like having a syncobj bound to these jobs and every time we submit the + * command buffer again we first check these sncobjs to see if we can free + * some of these clones so we avoid blowing up memory. + */ + v3dv_cmd_buffer_add_private_obj( + job->cmd_buffer, (uintptr_t)clone, + (v3dv_cmd_buffer_private_obj_destroy_cb)job_destroy_cb); + + return clone; +} diff --git a/src/broadcom/vulkan/v3dvx_private.h b/src/broadcom/vulkan/v3dvx_private.h index adaa2b6dcf7..061a52ea6a2 100644 --- a/src/broadcom/vulkan/v3dvx_private.h +++ b/src/broadcom/vulkan/v3dvx_private.h @@ -130,6 +130,9 @@ v3dX(cmd_buffer_emit_indexed_indirect)(struct v3dv_cmd_buffer *cmd_buffer, void v3dX(cmd_buffer_suspend)(struct v3dv_cmd_buffer *cmd_buffer); +struct v3dv_job * +v3dX(cmd_buffer_prepare_suspend_job_for_submit)(struct v3dv_job *job); + void v3dX(get_hw_clear_color)(const VkClearColorValue *color, uint32_t internal_type,