Command buffer queue substitution#1584
Conversation
…cosmetic corrections added: KhronosGroup#1369
|
|
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/basic_command_buffer.h
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/basic_command_buffer.h
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/basic_command_buffer.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/basic_command_buffer.h
Outdated
Show resolved
Hide resolved
…ommand queue with properties: KhronosGroup#1369
|
@aharon-abramson Could you try this out on your implementation |
|
Yes, I'll give it a try.
From: Ewan Crawford ***@***.***>
Sent: Wednesday, November 30, 2022 1:16 PM
To: KhronosGroup/OpenCL-CTS ***@***.***>
Cc: Aharon Abramson ***@***.***>; Mention ***@***.***>
Subject: Re: [KhronosGroup/OpenCL-CTS] Command buffer queue substitution (PR #1584)
EXTERNAL EMAIL: Do not click any links or open any attachments unless you trust the sender and know the content is safe.
@aharon-abramson<https://github.com/aharon-abramson> Could you try this out on your implementation
—
Reply to this email directly, view it on GitHub<#1584 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AW54MILQRTDEPJWDW6UBVE3WK4ZQPANCNFSM6AAAAAASNG7TGI>.
You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>
|
bashbaug
left a comment
There was a problem hiding this comment.
FYI, I tried these new tests with my command buffer emulation layer and they all passed, pretty cool!
queue_substitution...
queue_substitution passed
properties_queue_substitution...
Queue property CL_QUEUE_OUT_OF_ORDER_EXEC_MODE_ENABLE supported. Testing ...
Queue property CL_QUEUE_OUT_OF_ORDER_EXEC_MODE_ENABLE supported. Testing ...
properties_queue_substitution passed
simultaneous_queue_substitution...
simultaneous_queue_substitution passed
PASSED sub-test.
PASSED 9 of 9 tests.
test_conformance/extensions/cl_khr_command_buffer/basic_command_buffer.cpp
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Outdated
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Show resolved
Hide resolved
test_conformance/extensions/cl_khr_command_buffer/basic_command_buffer.cpp
…aced cl_command_queue with related wrapper (KhronosGroup#1369, p.1.4)
| error = clSetKernelArg(kernel, 2, sizeof(off_mem), &off_mem); | ||
| test_error(error, "clSetKernelArg failed"); | ||
|
|
||
| if (simultaneous_use_support) |
There was a problem hiding this comment.
It's problematic that you try to create a command buffer before checking CL_DEVICE_COMMAND_BUFFER_REQUIRED_QUEUE_PROPERTIES_KHR in the Skip() method. It should be the other way around.
There was a problem hiding this comment.
Looks like a good point to me. If queue will not meet the minimum properties specified by CL_DEVICE_COMMAND_BUFFER_REQUIRED_QUEUE_PROPERTIES_KHR then clCreateCommandBufferKHR will likely fail (CL_INCOMPATIBLE_COMMAND_QUEUE_KHR) instead of skipping the test.
My proposition is to move all clGetDeviceInfo queries to BasicCommandBufferTest::Skip method and change the order of Skip and SetUp calls in MakeAndRunTest function. I verified that approach with previous and new tests introduced in this PR - it needs some additional refactoring (and reviewing all remaining cl_khr_command_buffer related PRs) but it looks promising.
Please let me know if you have other propositions on your mind, thanks.
There was a problem hiding this comment.
No, this approach looks good to me.
EwanC
left a comment
There was a problem hiding this comment.
One very minor comment, but the change so that BasicCommandBufferTest::Skip happens before BasicCommandBufferTest::SetUp makes sense to me.
test_conformance/extensions/cl_khr_command_buffer/command_buffer_queue_substitution.cpp
Show resolved
Hide resolved
|
Looks OK to me |
|
Merging as discussed in the January 24th teleconference. |
Introduce series of queue substitution tests for cl_khr_command_buffer extension according to point 1.3 of issue description.