From fa1b10f36d413448e69cd2a5e29b1f51a152af67 Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Fri, 18 Mar 2022 08:04:22 +0100 Subject: [PATCH] v3dv: lock around noop job submits Any thread we create may end up creating/submitting at least a noop job, which is a shared object. Before multisync, this was an issue only for the creation of the job itself, but with multisync we can also modify parameters of the noop job every time it is used (for signaling and serialization configuration). This change adds a noop mutex that all threads (main, wait and master) take before submitting a noop job to ensure concurrent access is not an issue. Fixes flakyness observed with multisync with the following test: dEQP-VK.api.command_buffers.secondary_execute_twice Reviewed-by: Melissa Wen Part-of: --- src/broadcom/vulkan/v3dv_device.c | 2 ++ src/broadcom/vulkan/v3dv_private.h | 3 +++ src/broadcom/vulkan/v3dv_queue.c | 16 ++++++++++++---- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/broadcom/vulkan/v3dv_device.c b/src/broadcom/vulkan/v3dv_device.c index bc83215c787..c0f598d38af 100644 --- a/src/broadcom/vulkan/v3dv_device.c +++ b/src/broadcom/vulkan/v3dv_device.c @@ -1827,6 +1827,7 @@ queue_init(struct v3dv_device *device, struct v3dv_queue *queue, queue->noop_job = NULL; list_inithead(&queue->submit_wait_list); pthread_mutex_init(&queue->mutex, NULL); + pthread_mutex_init(&queue->noop_mutex, NULL); return VK_SUCCESS; } @@ -1838,6 +1839,7 @@ queue_finish(struct v3dv_queue *queue) if (queue->noop_job) v3dv_job_destroy(queue->noop_job); pthread_mutex_destroy(&queue->mutex); + pthread_mutex_destroy(&queue->noop_mutex); } static void diff --git a/src/broadcom/vulkan/v3dv_private.h b/src/broadcom/vulkan/v3dv_private.h index 056f89dcec5..3002d21fe80 100644 --- a/src/broadcom/vulkan/v3dv_private.h +++ b/src/broadcom/vulkan/v3dv_private.h @@ -260,6 +260,9 @@ struct v3dv_queue { /* A mutex to prevent concurrent access to the list of wait threads */ mtx_t mutex; + /* A mutex to prevent concurrent noop job submissions */ + mtx_t noop_mutex; + struct v3dv_job *noop_job; }; diff --git a/src/broadcom/vulkan/v3dv_queue.c b/src/broadcom/vulkan/v3dv_queue.c index 30d5e0d74f2..75d4172e791 100644 --- a/src/broadcom/vulkan/v3dv_queue.c +++ b/src/broadcom/vulkan/v3dv_queue.c @@ -1228,18 +1228,26 @@ queue_submit_noop_job(struct v3dv_queue *queue, if (!do_sem_signal && !serialize && !sems_info->wait_sem_count) return VK_SUCCESS; - /* VkQueue host access is externally synchronized so we don't need to lock - * here for the static variable. + /* We need to protect noop_job against concurrent access. While + * the client must externally synchronize queue submissions, we + * may spawn threads that can submit noop jobs themselves. */ + mtx_lock(&queue->noop_mutex); if (!queue->noop_job) { VkResult result = queue_create_noop_job(queue); - if (result != VK_SUCCESS) + if (result != VK_SUCCESS) { + mtx_unlock(&queue->noop_mutex); return result; + } } queue->noop_job->do_sem_signal = do_sem_signal; queue->noop_job->serialize = serialize; - return queue_submit_job(queue, queue->noop_job, sems_info, NULL); + VkResult result = + queue_submit_job(queue, queue->noop_job, sems_info, NULL); + + mtx_unlock(&queue->noop_mutex); + return result; } /* This function takes a job type and returns True if we have