From 321f0b85f2e5d539144aa3dd91d80289cce3c2a0 Mon Sep 17 00:00:00 2001 From: Jason Ekstrand Date: Thu, 7 Apr 2022 10:17:26 -0500 Subject: [PATCH] v3dv: Always wait on last_job_syncs if job->serialize Even if we're the first job on some queue, there may be no wait semaphores but we still need to ensure things happen in-order. (See the "Implicit Synchronization Guarantees" section of the Vulkan spec.) The client can submit back-to-back command buffers with no semaphores between them and it needs to adt the same as if there were a semaphore. If job->serialize is set because of a barrier or something, we still need to synchronize across HW queues by waiting on last_job_syncs. Reviewed-by: Iago Toral Quiroga Part-of: --- src/broadcom/vulkan/v3dv_queue.c | 53 ++++++++++++++++---------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/broadcom/vulkan/v3dv_queue.c b/src/broadcom/vulkan/v3dv_queue.c index 51d2ae8cf56..d5501371388 100644 --- a/src/broadcom/vulkan/v3dv_queue.c +++ b/src/broadcom/vulkan/v3dv_queue.c @@ -744,11 +744,10 @@ set_in_syncs(struct v3dv_device *device, if (device->last_job_syncs.first[queue]) n_sems = sems_info->wait_sem_count; - /* If we don't need to wait on wait semaphores but the serialize flag is - * set, this job waits for completion of all GPU jobs submitted in any - * queue V3DV_QUEUE_(CL/TFU/CSD) before running. + /* If the serialize flag is set, this job waits for completion of all GPU + * jobs submitted in any queue V3DV_QUEUE_(CL/TFU/CSD) before running. */ - *count = n_sems == 0 && job->serialize ? 3 : n_sems; + *count = n_sems + (job->serialize ? 3 : 0); if (!*count) return NULL; @@ -760,30 +759,30 @@ set_in_syncs(struct v3dv_device *device, if (!syncs) return NULL; - if (n_sems) { - for (int i = 0; i < *count; i++) { - struct v3dv_semaphore *sem = - v3dv_semaphore_from_handle(sems_info->wait_sems[i]); - syncs[i].handle = semaphore_get_sync(sem); + for (int i = 0; i < n_sems; i++) { + struct v3dv_semaphore *sem = + v3dv_semaphore_from_handle(sems_info->wait_sems[i]); + syncs[i].handle = semaphore_get_sync(sem); - /* From the Vulkan 1.0 spec: - * - * "If the import is temporary, the implementation must restore - * the semaphore to its prior permanent state after submitting - * the next semaphore wait operation." - * - * We can't destroy the temporary sync until the kernel is done - * with it, this is why we need to have this 'has_temp' flag instead - * of checking temp_sync for 0 to know if we have a temporary - * payload. The temporary sync will be destroyed if we import into - * the semaphore again or if the semaphore is destroyed by the - * client. - */ - sem->has_temp = false; - } - } else { - for (int i = 0; i < *count; i++) - syncs[i].handle = device->last_job_syncs.syncs[i]; + /* From the Vulkan 1.0 spec: + * + * "If the import is temporary, the implementation must restore + * the semaphore to its prior permanent state after submitting + * the next semaphore wait operation." + * + * We can't destroy the temporary sync until the kernel is done + * with it, this is why we need to have this 'has_temp' flag instead + * of checking temp_sync for 0 to know if we have a temporary + * payload. The temporary sync will be destroyed if we import into + * the semaphore again or if the semaphore is destroyed by the + * client. + */ + sem->has_temp = false; + } + + if (job->serialize) { + for (int i = 0; i < 3; i++) + syncs[n_sems + i].handle = device->last_job_syncs.syncs[i]; } return syncs;