add support for image1d_buffer #609
Conversation
Change-Id: I4eef91af393b5487bbc208bdc96d43daa4c5c462
remove 1DBUFFER mention in init_vulkan_image limit CL_DEVICE_IMAGE_MAX_BUFFER_SIZE WITH CL_DEVICE_MAX_MEM_ALLOC_SIZE
rjodinchr
force-pushed
the
dev/1dbuffer
branch
from
f5af614
to
1744028
Compare
|
This PR depends on an update of clspv |
There was a problem hiding this comment.
This change does not introduce CTS regressions at my end. As discussed I'm not a big fan of handling the buffer view special case in the API layer as this breaks encapsulation. In the interest of getting forward progress, I'm with this approach as a first step (and I think it likely makes the code a bit shorter).
| if ((components_sampled != components_storage) && | ||
| (image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I'm a bit confused by this condition. Those swizzles are unused for 1D buffer images. Why is this needed?
There was a problem hiding this comment.
I am not sure, this is coming from your initial commit I believe. Thinking about it, I don't see a reason to have it, maybe we should get rid of it?
There was a problem hiding this comment.
yeah, if we pass the CTS without it, then we should remove it. If we somehow need this to pass the CTS, we should find a clearer expression of the conditions.
There was a problem hiding this comment.
It seems that this test is needed. Otherwise the CTS thinks that we support CL_INTENSITY and CL_LUMINANCE with 1d buffer, which does not work with the actual implementation. As they are not mandatory, it feels ok to remove them with this test. What do you think?
There was a problem hiding this comment.
If they're not supported we should remove them but the condition used to do that is a hack (you said it was part of my draft patch, I'm assuming I'd done that as a quick way of excluding those formats). I've added an entry to #625 so we don't forget to revisit this.
|
@rjodinchr In case you hadn't noticed: it looks like the |
|
The tests are expected to fail one way or another as this PR is not updating clspv. |
|
Ok, makes sense. Can you include the clspv update here (just the one commit we need please) so the CI passes? Thanks! |
| if ((components_sampled != components_storage) && | ||
| (image_type == CL_MEM_OBJECT_IMAGE1D_BUFFER)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
yeah, if we pass the CTS without it, then we should remove it. If we somehow need this to pass the CTS, we should find a clearer expression of the conditions.
| @@ -957,6 +967,7 @@ struct cvk_command_buffer_image_copy final : public cvk_command_batchable { | |||
| size_t m_offset; | |||
| std::array<size_t, 3> m_origin; | |||
| std::array<size_t, 3> m_region; | |||
| cl_command_type m_copy_type; | |||
There was a problem hiding this comment.
Reusing cl_command_type is a bit confusing but it'll do for a first implementation.
|
@rjodinchr I've had a look at the CI failures. The newly added test creates a sub-buffer at an offset that does not satisfy Vulkan constraints. clvk should have rejected the creation of the sub-buffer and texel buffer alignment constraints should be taken into account when calculating |
| VkFormatFeatureFlags format_feature_flags_WO; | ||
| format_feature_flags_WO = VK_FORMAT_FEATURE_STORAGE_IMAGE_BIT; | ||
| if (type == CL_MEM_OBJECT_IMAGE1D_BUFFER) { | ||
| format_feature_flags_WO = VK_FORMAT_FEATURE_STORAGE_TEXEL_BUFFER_BIT; |
There was a problem hiding this comment.
Looking at the latest CI failures, it seems we also need VK_FORMAT_FEATURE_2_STORAGE_WRITE_WITHOUT_FORMAT_BIT_KHR which apparently is only available as a "2" format feature so we're dragging #497 as a dependency.
|
Merging this as is to unblock clspv updates. I will fix the CI with a separate commit on top. |
This implementation has been tested using the OpenCL-CTS patch with KhronosGroup/OpenCL-CTS#1806