Add tests for cl_ext_immutable_memory_objects #2286
Conversation
Signed-off-by: Michael Rizkalla <[email protected]>
This change does the following: 1. Create `negative_enqueue_map_image` with tests for mapping images created with `CL_MEM_IMMUTABLE_EXT` 2. Update `enqueue_map_*` with more variants of `cl_mem_flags` Signed-off-by: Michael Rizkalla <[email protected]>
This change updates `cl_khr_command_buffer` test binary to include behaviour with `CL_MEM_IMMUTABLE_EXT` memory flag. Added the following tests: 1. negative_copy_to_immutable_buffer 2. negative_copy_image_to_immutable_buffer 3. negative_copy_to_immutable_buffer_rect 4. negative_copy_to_immutable_image 5. negative_copy_buffer_to_immutable_image 6. negative_fill_immutable_image 7. negative_fill_immutable_buffer Signed-off-by: Michael Rizkalla <[email protected]>
This change adds `negative_set_immutable_memory_to_writeable_kernel_arg` to verify setting memory arguments marked with `CL_MEM_IMMUTABLE_EXT`. Signed-off-by: Michael Rizkalla <[email protected]>
|
Looks like this is another one where we'll need header changes before we can merge. Would it be possible to get a draft headers PR up with these changes so this won't become a bottleneck once the tests have been reviewed? |
There was a problem hiding this comment.
I don't have a device to test these changes, and I can't even build without the header changes, but the test changes look like a good start.
The command buffer negative testing looks pretty comprehensive. Are there plans to have negative tests for command-queue fills and copies also?
Are there any plans to extend positive testing to confirm that we can read from immutable buffers and images?
| #define PASSIVE_REQUIRE_IMMUTABLE_MEMORY_OBJECTS(device) \ | ||
| if (!is_extension_available(device, "cl_ext_immutable_memory_objects")) \ | ||
| { \ | ||
| log_info("\n\tNote: device does not support " \ | ||
| "'cl_ext_immutable_memory_objects'. Skipping test...\n"); \ | ||
| return TEST_SKIPPED_ITSELF; \ | ||
| } |
There was a problem hiding this comment.
Minor: do we need to add a new macro here? I think REQUIRE_EXTENSION should be sufficient and preferred.
| { | ||
| constexpr size_t image_dim = 32; | ||
|
|
||
| if (is_extension_available(device, "cl_ext_immutable_memory_objects")) |
There was a problem hiding this comment.
Minor: I think it would be preferable to skip this test if immutable memory objects are not supported. Can we use REQUIRE_EXTENSION to do this?
| #define test_object_failure_ret(object, errCode, expectedErrCode, msg, \ | ||
| retValue) \ | ||
| { \ | ||
| test_assert_error_ret(object == nullptr, msg, retValue); \ | ||
| test_failure_error_ret(errCode, expectedErrCode, msg, retValue); \ | ||
| } |
There was a problem hiding this comment.
I feel like we already have too many of these error macros, and this one seems like it will be used only in a few narrow cases (it's only used twice in this PR). Could we just writetest_assert_error_ret and test_failure_error_ret instead?
Note that "object" is really just a "pointer" in actual use, so if we decide to keep it, I'd prefer to give it a different name.
| test_failure_error_ret(error, CL_INVALID_ARG_VALUE, | ||
| "clSetKernelArg is supposed to fail " | ||
| "with CL_INVALID_ARG_VALUE when an image is " | ||
| "created with CL_MEM_IMMUTABLE_EXT is " | ||
| "passed to a read_only kernel argument", | ||
| TEST_FAIL); |
There was a problem hiding this comment.
I think this should be write_only instead of read_only?
| test_failure_error_ret(error, CL_INVALID_ARG_VALUE, | |
| "clSetKernelArg is supposed to fail " | |
| "with CL_INVALID_ARG_VALUE when an image is " | |
| "created with CL_MEM_IMMUTABLE_EXT is " | |
| "passed to a read_only kernel argument", | |
| TEST_FAIL); | |
| test_failure_error_ret(error, CL_INVALID_ARG_VALUE, | |
| "clSetKernelArg is supposed to fail " | |
| "with CL_INVALID_ARG_VALUE when an image is " | |
| "created with CL_MEM_IMMUTABLE_EXT is " | |
| "passed to a write_only kernel argument", | |
| TEST_FAIL); |
This change provides partial test coverage for KhronosGroup/OpenCL-Docs#1280
Adding CTS tests for:
The bulk of the tests is to make sure that the CL driver does not allow writing to a memory object that is created with
CL_MEM_IMMUTABLE_EXTflag when used with the above APIs.