KhronosGroup / KhronosGroup/OpenCL-CTS
rethink test_cl_khr_command_buffer command_buffer_wait_for_sec_command_buffer
- Dominant language
- C++
- Stars
- 232
- Forks
- 235
- Avg merge
- 8d 7h
- Merged PRs (30d)
- 18
Description
We should rethink how to test:
rethink test_cl_khr_command_buffer command_buffer_wait_for_sec_command_buffer
The current test has no data dependency between the two command-buffers, so I don't believe the current test is testing what it is intending to test.
If we want to have a command-buffer in one command-queue depend on a command-buffer in a different queue then we probably need a data dependency between the commands in each command-buffer. For example, in the first command-buffer we can copy from buffer A to buffer B, and then the second command buffer can copy from buffer B to buffer C, and we can check the results of buffer C. Or, perhaps the first command-buffer can modify buffer A in place (say, by adding a value to each element of buffer A), and then the second command-buffer can also modify buffer A in place (say, by adding a different value to each element of buffer A), and we can check that both updates have been applied. There are lots of options.
Here was the original PR comment that prompted this issue:
_Originally posted by @bashbaug in https://github.com/KhronosGroup/OpenCL-CTS/pull/1840#discussion_r1781917261_
I don't think this is necessarily wrong, but I'm having a tough time figuring out what this test is doing now.
As far as I can tell, for setup:
1. There are two queues (`queue` and `queue_sec`), two command buffers (`command_buffer` and `command_buffer_sec`), two kernels (`kernel` and `kernel_sec`), and two sets of buffers used for kernel inputs and outputs (`in_mem`, `out_mem`, `off_mem`, `in_mem_sec`, `out_mem_sec`, and `off_mem_sec`).
* Note: `queue_sec` is exclusively in-order. `queue` may be out-of-order.
2. `in_mem`, `out_mem`, and `off_mem` are set as kernel arguments for `kernel` and recorded into `command_buffer`.
3. `in_mem_sec`, `out_mem_sec`, and `off_mem_sec` are set as kernel arguments for `kernel_sec` and recorded into `command_buffer_sec`.
4. `command_buffer` records `kernel`, and `command_buffer_sec` records `kernel_sec`.
Then, when the test executes:
5. In `queue_sec`, we initialize `in_mem_sec`, then enqueue `command_buffer_sec`. We save the event for `command_buffer_sec`.
* Remember: `command_buffer_sec` recorded `kernel_sec`, which reads `in_mem_sec` and writes `out_mem_sec`.
6. In `queue`, we initialize `in_mem`, then enqueue `command_buffer`, with an event dependency on the initialization and the execution of `command_buffer_sec` in `queue_sec`. We save the event for `command_buffer`.
* Remember: `command_buffer` recorded `kernel`, which reads `in_mem` and writes `out_mem`.
To verify the test output:
7. We enqueue a read from `out_mem` with an event dependency on the execution of `command_buffer`.
8. We flush `queue`, then finish `queue_sec`, then finish `queue`.
9. We check the results that we read.
This is all nominally fine, but we never check the results of `out_mem_sec` or anything that executes in `command_buffer_sec`, really. Is that what is intended?
Another possible solution that wouldn't require a second kernel and a second set of memory objects is to have the initialization of `in_mem` in step (6) depend on the execution of the command buffer in step (5). This ensures that `in_mem` will not be overwritten while the command buffer is executing. I'm not sure if that's any better, though...
Contributor guide
No contributing guide indexed for this repository
Research direction
Locate the test_cl_khr_command_buffer command_buffer_wait_for_sec_command_buffer entry point and read its queue, command-buffer, and result-checking flow. Run the test to establish current behavior, then revise it so both command-buffer executions have a checked data dependency and the resulting data verifies both operations.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- testing-qa
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100