Update semaphore synchronisation used in Vulkan interop tests #2243
Conversation
Remove the `CREATE_OPENCL_SEMAPHORE` macro and use derived class instantiations of the `clExternalSemaphore` class, rather than base pointers to derived class objects. Remove the default argument for `queryParamName` in `check_external_semaphore_handle_type()`. Move `check_external_semaphore_handle_type()` checks to constructors of `clExternalImportableSemaphore` and `clExternalExportableSemaphore`, rather than manually making the check before creating an external semaphore. Signed-off-by: Gorazd Sumkovski <[email protected]>
Since sync_fd semaphores have copy transference, `clExternalExportableSemaphore::signal()` exports the payload after enqueuing a signal. However the queue must be finished to execute the signal before the export. Also fix an event dependency deadlock in `buffer_multiple_queue` by adding a finish on `cmd_queue1` to resolve the dependencies. Signed-off-by: Gorazd Sumkovski <[email protected]>
| @@ -1146,6 +1152,14 @@ int clExternalExportableSemaphore::signal(cl_command_queue cmd_queue) | |||
|
|
|||
| if (m_externalHandleType == VULKAN_EXTERNAL_SEMAPHORE_HANDLE_TYPE_SYNC_FD) | |||
| { | |||
| // Finish the queue to signal the semaphore | |||
| err = clFinish(cmd_queue); | |||
There was a problem hiding this comment.
Can you explain why this is needed?
There was a problem hiding this comment.
This shouldn't be needed. The OpenCL spec states that after clEnqueueSignalSemaphoreKHR returns, it is safe to wait. The implication is that it is also safe to import into vulkan, the finish is not necessary here, and defeats the purpose of semaphores.
| // cmd_queue1 needs to be finished now because | ||
| // cmd_queue2 depends on first_launch which is | ||
| // completed in cmd_queue1. | ||
| err = clFinish(cmd_queue1); |
There was a problem hiding this comment.
I do not think this finish is required, semaphores can operate asynchronously. I could be convinced a flush may be needed, but I don't think finish is correct.
This change adds a lot of clFinishes. I think this makes the test synchronous and incorrect.
|
Discussed in 2025/02/25: Qcom like the refactoring but are questioning the addition of many calls to |
No description provided.