Add tests for external sharing not dependant on semaphores. #1648

pj87 · 2023-02-21T11:01:36Z

Additional external sharing tests that use fences instead of semaphores.

Additional external sharing tests that use fences instead of semaphores. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv · 2023-02-28T15:29:28Z

Will need to merge #1630 before merging any changes to test_vulkan to ensure build sanity.

nikhiljnv · 2023-03-07T16:20:40Z

Can we make VkFence + clFinish a synchronization option to existing tests rather than creating a separate test that uses fence?
That way we can extend existing coverage across both modes (synchronization via external semaphores vs synchronization via VkFence + clFinish)

nikhiljnv · 2023-03-14T14:45:36Z

@pj87
Looks like there will be some conflicts between this PR and part of #1645 due to refactoring of vulkan_wrapper moved into common place / lib.
We can go either ways, but do we have a preference which one we want to merge first? We will need to update other one based on the sequence.
Given that this is still in drafts, should we focus on #1645 first?

Apply changes for review: - Make VkFence + clFinish a synchronization option to existing tests instead of creating a separate test that uses fence. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv

Some early comments.

test_conformance/vulkan/vulkan_interop_common/vulkan_wrapper.cpp

nikhiljnv · 2023-03-21T02:47:07Z

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

- vkQueue.submit(vkCommandBuffer, vkVk2CLSemaphore);
+ if (use_fence)
+ {
+ vkQueue.submit(vkCommandBuffer, fence);


Will need similar changes to other files especially to test_vulkan_interop_image.cpp

So as I aready mentioned at the beginnig of the task, I don't know how it should be done for the image part. I tried changing it in this commit accordingly for bufffer and image but I ended up with deadlocks:
9e83f00#diff-2b3fe9aac0e9f06312676bdb7212bb775c8b1386f633d73db32aebb87b283f3f
Do you have any idea on how it should be implemented?

Will take a look. We can cover image changes later if there are more issues to address.

nikhiljnv · 2023-03-21T16:09:15Z

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

- vkQueue.submit(vkCommandBuffer, vkVk2CLSemaphore);
+ if (use_fence)
+ {
+ vkQueue.submit(vkCommandBuffer, fence);


Will take a look. We can cover image changes later if there are more issues to address.

nikhiljnv · 2023-03-21T16:15:40Z

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

+ else
+ {
+ if (use_fence)
+ {


Fence needs to be reset before reusing after previous Wait.

I've added it but now there are deadlocks there.

The sequence should be "Reset fence, use it in some submit operation where it gets signaled, wait on it" or "use fence in some submit operation where it gets signaled, wait on it, Reset fence". Resetting just before wait but after signal would override effect of signalling at the end of completion of work submitted earlier and wait would not wait for the same.

After this change there is an issue with:

err = clEnqueueReadBuffer(cmd_queue1, error_1, CL_TRUE, 0, sizeof(uint8_t), error_3, 0, NULL, NULL);

it produces the following output:

log_error("&&&& vulkan_opencl_buffer test FAILED\n");

Yet I am not sure what the reason is.

Discussed May 9th. @nikhiljnv will try to reproduce these failures.

We tried repro of the issue and found that synchronizations are misplaced in some scenarios.
Have requested changes which should fix data mismatches seen here.

The reason for the else branch when iter > 0 was to wait on the semaphore dispatched at the end of the last frame. We either need to wait on the last fence here, or wait on a fence at the end of the frame, and delete the if(iter==0) behavior.

Fixed following fence issues: - Add missing link to command buffer - Add fence reset before wait Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

Changes made: - wait for fence with clFinish - queue submit with wait for fence Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

pj87 · 2023-05-22T11:02:54Z

After the changes requested by @nikhiljnv I am still seeing undeterministic messages:

log_error("&&&& vulkan_opencl_buffer test FAILED\n");

It mostly doesn't reproduce, but it seems that there still may occur from time to time.

nikhiljnv · 2023-05-19T12:17:04Z

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

@@ -275,7 +265,7 @@ int run_test_with_two_queue(cl_context &context, cl_command_queue &cmd_queue1,

 if (use_fence)
 {
- vkWaitForFences(vkDevice, 1, &fence, VK_TRUE, UINT64_MAX);
+ clFinish(cmd_queue1);


Fence wait is correct operation here.
Just to clarify - The places where execution moves from Vulkan to OpenCL (if blocks corresponding to else containing clVk2CLExternalSemaphore->wait) need to have fence waits.
The places where execution moves from OpenCL to Vulkan ((if blocks corresponding to else containing clCl2VkExternalSemaphore->signal)

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

Replaced clFinish with vkWaitForFences in Vulkan exectution context. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

Replaced remaining clFinish with vkWaitForFences in Vulkan exectution context. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv

LGTM.

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

joshqti · 2023-06-05T22:21:03Z

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

+ else
+ {
+ if (use_fence)
+ {


The reason for the else branch when iter > 0 was to wait on the semaphore dispatched at the end of the last frame. We either need to wait on the last fence here, or wait on a fence at the end of the frame, and delete the if(iter==0) behavior.

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

pj87 · 2023-06-22T12:38:14Z

@joshqti could you take a look if everything is OK now?

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

… tests. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

bcalidas · 2023-06-27T23:15:51Z

The latest commit looks good. This PR can be merged.

Thanks,
Balaji Calidas,
Qualcomm

nikhiljnv

LGTM.

Add tests for external sharing not dependant on semaphores.

714b54c

Additional external sharing tests that use fences instead of semaphores. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

pj87 marked this pull request as draft February 21, 2023 11:05

Fix clang-format

443167e

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

pj87 mentioned this pull request Feb 21, 2023

Improvements to the External Memory and Semaphores conformance tests #1588

Closed

Merge branch 'KhronosGroup:main' into external_sharing_fences

8ee959c

pj87 added 3 commits March 17, 2023 14:05

Apply changes for review.

5faf2dc

Apply changes for review: - Make VkFence + clFinish a synchronization option to existing tests instead of creating a separate test that uses fence. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Fix build break.

f2466e4

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Fix resource release conditions.

92b6e1c

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

pj87 marked this pull request as ready for review March 17, 2023 17:29

nikhiljnv requested changes Mar 21, 2023

View reviewed changes

nikhiljnv reviewed Mar 21, 2023

View reviewed changes

pj87 added 4 commits March 22, 2023 14:47

Merge remote-tracking branch 'origin/main' into external_sharing_fences

7f5c889

Fix fence usage.

3e4f49b

Fixed following fence issues: - Add missing link to command buffer - Add fence reset before wait Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Add Vulkan wrapper for fence.

c5e6dd9

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Rework fence reset.

8612f4a

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv requested changes May 16, 2023

View reviewed changes

Change synchronisation mechanisms.

51cc730

Changes made: - wait for fence with clFinish - queue submit with wait for fence Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv requested changes May 22, 2023

View reviewed changes

Replace clFinish with vkWaitForFences.

eb69cec

Replaced clFinish with vkWaitForFences in Vulkan exectution context. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv requested changes May 23, 2023

View reviewed changes

test_conformance/vulkan/test_vulkan_interop_buffer.cpp Outdated Show resolved Hide resolved

test_conformance/vulkan/test_vulkan_interop_buffer.cpp Outdated Show resolved Hide resolved

Replace remaining clFinish with vkWaitForFences.

24dd489

Replaced remaining clFinish with vkWaitForFences in Vulkan exectution context. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv previously approved these changes May 29, 2023

View reviewed changes

bashbaug added the focused review label May 30, 2023

joshqti reviewed Jun 5, 2023

View reviewed changes

test_conformance/vulkan/test_vulkan_interop_buffer.cpp Outdated Show resolved Hide resolved

joshqti reviewed Jun 5, 2023

View reviewed changes

Fix review comments for synchoronisation simplification.

a5f1df7

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

pj87 dismissed nikhiljnv’s stale review via a5f1df7 June 21, 2023 14:09

joshqti reviewed Jun 22, 2023

View reviewed changes

test_conformance/vulkan/test_vulkan_interop_buffer.cpp Show resolved Hide resolved

pj87 added 2 commits June 23, 2023 13:29

Fix review comments for synchoronisation simplification for remaining…

6436ec7

… tests. Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

Fix condition check.

3e9f0a7

Signed-off-by: Paweł Jastrzębski <p.k.jastrzebski@gmail.com>

nikhiljnv approved these changes Jul 11, 2023

View reviewed changes

neiltrevett merged commit 1ab4b26 into KhronosGroup:main Jul 11, 2023
6 checks passed

pj87 deleted the external_sharing_fences branch November 10, 2023 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for external sharing not dependant on semaphores. #1648

Add tests for external sharing not dependant on semaphores. #1648

pj87 commented Feb 21, 2023

nikhiljnv commented Feb 28, 2023 •

edited

Loading

nikhiljnv commented Mar 7, 2023

nikhiljnv commented Mar 14, 2023

nikhiljnv left a comment

nikhiljnv Mar 21, 2023

pj87 Mar 21, 2023

nikhiljnv Mar 21, 2023

nikhiljnv Mar 21, 2023

nikhiljnv Mar 21, 2023

pj87 Mar 22, 2023

nikhiljnv Apr 4, 2023

pj87 Apr 17, 2023

bashbaug May 9, 2023

nikhiljnv May 16, 2023

joshqti Jun 5, 2023

pj87 commented May 22, 2023

nikhiljnv May 19, 2023

nikhiljnv left a comment

joshqti Jun 5, 2023

pj87 commented Jun 22, 2023

bcalidas commented Jun 27, 2023

nikhiljnv left a comment

Add tests for external sharing not dependant on semaphores. #1648

Add tests for external sharing not dependant on semaphores. #1648

Conversation

pj87 commented Feb 21, 2023

nikhiljnv commented Feb 28, 2023 • edited Loading

nikhiljnv commented Mar 7, 2023

nikhiljnv commented Mar 14, 2023

nikhiljnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pj87 commented May 22, 2023

Choose a reason for hiding this comment

nikhiljnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pj87 commented Jun 22, 2023

bcalidas commented Jun 27, 2023

nikhiljnv left a comment

Choose a reason for hiding this comment

nikhiljnv commented Feb 28, 2023 •

edited

Loading