Added comparability verification for GL associated devices query #2231
Conversation
|
I've tested this PR with linux and windows, currently I am not able to launch the test on MacOS - I will appreciate confirmation from this platform. GLES version does not work for me, neither on ubuntu nor on windows. Is this sample operational ? |
test_common/gl/setup_osx.cpp
Outdated
| cl_context_properties properties[] = { | ||
| CL_CONTEXT_PROPERTY_USE_CGL_SHAREGROUP_APPLE, | ||
| (cl_context_properties)mShareGroup, 0 | ||
| }; |
There was a problem hiding this comment.
Could we instead call GetContextProps here?
As-written, if any of the context properties change, we need to make a code change in two places, whereas if we call GetContextProps instead we only need to change one place.
Same goes for win32 and x11 also.
There was a problem hiding this comment.
GetContextProps removed due to the discussion below. List extended with CL_CONTEXT_PLATFORM.
There is still one small confusion worth mentioning I think. Please note that code to setup interoperability context uses first platform on the list of clGetPlatformIDs. I think we could get platform from harness, right?
test_conformance/gl/test_queries.cpp
Outdated
| cl_int init() | ||
| { | ||
| cl_uint num_platforms = 0; | ||
| cl_int error = clGetPlatformIDs(16, nullptr, &num_platforms); |
There was a problem hiding this comment.
Let's just pass zero for num_entries here if platforms is nullptr (I'm surprised this isn't an error):
| cl_int error = clGetPlatformIDs(16, nullptr, &num_platforms); | |
| cl_int error = clGetPlatformIDs(0, nullptr, &num_platforms); |
There was a problem hiding this comment.
Corrected, it was borrowed from other test. I will create separate PR
test_conformance/gl/test_queries.cpp
Outdated
| return CL_SUCCESS; | ||
| } | ||
|
|
||
| cl_int find(const cl_device_id &id) |
There was a problem hiding this comment.
There's no need to pass the device ID by reference. Additionally, can we return true/false here, vs. assigning some meaning to an "int"?
| cl_int find(const cl_device_id &id) | |
| bool find(const cl_device_id id) |
test_conformance/gl/test_queries.cpp
Outdated
| num_devices, devices.data(), nullptr); | ||
| test_error(error, "clGetDeviceIDs failed"); | ||
|
|
||
| // try to find invalid device |
There was a problem hiding this comment.
Comment is confusing - I think this is trying to find the passed-in device, not necessarily an "invalid device"?
test_conformance/gl/test_queries.cpp
Outdated
|
|
||
| // get GL context info function pointer | ||
| size_t dev_size = 0; | ||
| clGetGLContextInfoKHR_fn GetGLContextInfo = |
There was a problem hiding this comment.
Let's use the name of the function as the function pointer name to avoid confusion.
| clGetGLContextInfoKHR_fn GetGLContextInfo = | |
| clGetGLContextInfoKHR_fn clGetGLContextInfoKHR = |
test_conformance/gl/test_queries.cpp
Outdated
| if (GetGLContextInfo == NULL) | ||
| { | ||
| log_error("ERROR: clGetGLContextInfoKHR failed! (%s:%d)\n", __FILE__, | ||
| __LINE__); | ||
| return TEST_FAIL; | ||
| } |
There was a problem hiding this comment.
We should be able to use test_assert_error here, which already prints the right __FILE__ and __LINE__ information.
Note, the error message should also say something about being unable to get the function pointer for clGetGLContextInfoKHR, not that clGetGLContextInfoKHR failed.
test_conformance/gl/test_queries.cpp
Outdated
| // Create the environment for the test. | ||
| GLEnvironment *glEnv = GLEnvironment::Instance(); | ||
|
|
||
| // get GL environment props | ||
| std::vector<cl_context_properties> props; | ||
| props.push_back(CL_CONTEXT_PLATFORM); | ||
| props.push_back((cl_context_properties)platform); |
There was a problem hiding this comment.
Could we query the context properties from the passed-in context from the harness (clGetContextInfo(CL_CONTEXT_PROPERTIES)) vs. trying to re-create the test environment here? This seems a lot simpler, and then we wouldn't need any changes to the setup code, but I'm not sure if that would hurt the test for some reason.
There was a problem hiding this comment.
Correction applied as suggested.
| } | ||
|
|
||
| // comparability test for CL_CURRENT_DEVICE_FOR_GL_CONTEXT_KHR | ||
| test_assert_error(device == devices[0], |
There was a problem hiding this comment.
I don't have easy access to a setup to test this, but if I'm reading the code right, there's no guarantee that device will be equal to the queried devices[0] for a multi-device context, particularly in the X11 case. This is because the context contains all of the devices that support cl_khr_gl_sharing in the platform in the X11 case. The Win32 case is probably OK, but only because the context only contains the "current device for GL context" rather than all devices.
Should the X11 case follow the same logic as the Win32 case, in which case this check would probably be OK?
There was a problem hiding this comment.
This is because the
contextcontains all of the devices that supportcl_khr_gl_sharingin the platform in the X11 case. The Win32 case is probably OK, but only because thecontextonly contains the "current device for GL context" rather than all devices.
Please note that final comparability test is performed agains CL_CURRENT_DEVICE_FOR_GL_CONTEXT_KHR query result, only one (currently associated with context) device ID should be returned, right ?
|
@bashbaug thanks for your feedback. The confusion around
which means your suggestion should work. Now if I extend list of properties for creation of a context with |
|
I think this question is in reference to #2231 (comment) - is that correct? If so, note that replying to review comments tends to thread better, and then we can resolve the discussion once it's closed. This seems like a problem:
When I don't have an OSX system setup to test with. If no other working group members do either, I think we should just do our best, which probably means including the |
Fixes #1485 according to work plan from issue description.