Skip to content
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

Vulkan: Use linear tiling #1559

Closed

Conversation

lakshmih
Copy link
Contributor

@lakshmih lakshmih commented Nov 3, 2022

The external memory sharing spec does not require
OpenCL to deduce if a Vulkan image is tiled or linear. Use tiled images.

Additionally, when selecting a queue family, the Vulkan implementation is not required to report the transfer bit for compute or graphics queues, it is implied.

@lakshmih lakshmih force-pushed the external_sharing_use_linear_tiling branch from 3491ae3 to a9449ac Compare November 3, 2022 04:53
@nikhiljnv
Copy link
Contributor

#1530 has disabled these tests in CMake by default.
So, passing CI builds does not guarantee successful builds with the changes.
While the changes themselves are small and unlikely to cause any build breaks, I would encourage that we enable these tests in CMake temporarily for future PRs to ensure we don't break the builds and turn off it again in CMake before merging the change. Alternatively, we can update presubmit script to have VULKAN_IS_SUPPORTED option enabled by default so that CI builds don't skip these tests. I prefer latter.

@bcalidas
Copy link

bcalidas commented Nov 9, 2022

HI Nikhil,

We do want to build and run these tests. Our recommendation is that supporting interop of Linear tiled Vulkan images with OpenCL is sufficient for an implementation to be conformant.

Thanks,
Balaji

@nikhiljnv
Copy link
Contributor

HI Nikhil,

We do want to build and run these tests. Our recommendation is that supporting interop of Linear tiled Vulkan images with OpenCL is sufficient for an implementation to be conformant.

Thanks, Balaji

Hi Balaji,
I am fine to keep conformance requirements minimal - Passing the test only for required (linear) tiling support should be sufficient.
However, not having test coverage for other tiling cases will prevent us not covering the underlying issues. In near future, we may want to address any special requirements for these cases in the spec. And to get there, it is important to have test coverage for different Vulkan formats and settings.
Any concerns keeping all the tiling types in the test and have an option to control which types to exercise with default being only linear tiling. Can think of similar needs for different memory flags. (Think of image sampler tests where certain addressing/ filtering modes are not required for conformance, but conformance tests cover the other non-mandatory modes as well that can be optionally run).

@bcalidas
Copy link

Hi Nikhil,

We are ok with adding an option for testing optimally tiled images as long as the default is linear. Should we update the PR with that option?

Thanks,
Balaji

@nikhiljnv
Copy link
Contributor

Hi Nikhil,

We are ok with adding an option for testing optimally tiled images as long as the default is linear. Should we update the PR with that option?

Thanks, Balaji

If it is a small change, prefer to do it as part of this PR. Otherwise, we can file a separate issue and/or fix it in a subsequent PR.

@bashbaug
Copy link
Contributor

We don't say anything about tiling requirements in the external memory extension specification. Should we?

@nikhiljnv
Copy link
Contributor

We don't say anything about tiling requirements in the external memory extension specification. Should we?

I have filed KhronosGroup/OpenCL-Docs#861 to track the spec discussions and required changes to the spec.

The external memory sharing spec does not require
OpenCL to deduce if a Vulkan image is tiled or linear.
Use linear images.

Additionally, when selecting a queue family, the Vulkan
implementation is not required to report the transfer bit
for compute or graphics queues, it is implied.
The external memory sharing spec does not require
OpenCL to deduce if a Vulkan image is tiled or linear.
Use linear images by default and add a parameter
to test optimal images.

Additionally, when selecting a queue family, the Vulkan
implementation is not required to report the transfer bit
for compute or graphics queues, it is implied.
@bashbaug
Copy link
Contributor

bashbaug commented Feb 7, 2023

Removing "focused review" while we figure out KhronosGroup/OpenCL-Docs#861.

@lakshmih
Copy link
Contributor Author

We will update this test as per final resolution of KhronosGroup/OpenCL-Docs#861

@nikhiljnv
Copy link
Contributor

@lakshmih
Is this already taken care by #1676 ? Or do we still need to update this after latest spec changes?

@nikhiljnv
Copy link
Contributor

Ping @lakshmih !! Is this still needed? If not, can we close?

@lakshmih lakshmih closed this Nov 14, 2023
@lakshmih lakshmih deleted the external_sharing_use_linear_tiling branch November 14, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants