-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Command buffer wait_for_sec_queue_event subtest, call clFinish in the correct order. #1758
Conversation
…h in the correct order. queue has a command that depends on a command that resides in queue_sec, calling clFinish(queue) before clFinish(queue_sec) causes the test to hang as the queue_sec command never got a chance to finish.
It says build macos-latest failed, but I am unsure the reason is my change. |
That should be fixed by #1756 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
test_conformance/extensions/cl_khr_command_buffer/command_buffer_event_sync.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't bad but I do wonder if it is defeating the purpose of the test. Would it be better to at least call clFlush(queue)
first, before calling clFinish(queue_sec)
? So we'd have:
error = clFlush(queue);
test_error(error, "clFlush failed");
error = clFinish(queue_sec);
test_error(error, "clFinish failed");
error = clFinish(queue);
test_error(error, "clFinish failed");
Yes, that works too as clFlush is not blocking and it also goes more "in line" with what the test was doing. Should I modify the commit with your suggestion then? |
Looks good. |
I'd also prefer the patch was updated to this suggestion |
Updated PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change works for me - thanks!
Do we need any further discussion or spec clarifications to document whether or why the previous code was incorrect?
Merging as discussed in the September 5th teleconference. |
queue has a command that depends on a command that resides in queue_sec, calling clFinish(queue) before clFinish(queue_sec) causes the test to hang as the queue_sec command never got a chance to finish.