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

fix support for read of 3D images with unnormalised sampler #614

Merged
merged 2 commits into from
Oct 29, 2023

Conversation

rjodinchr
Copy link
Contributor

This PR depends on google/clspv#1234

We parse the new NormalizedSamplerMaskPushConstant non-semantic instruction from KhronosGroup/SPIRV-Headers#377.

Set the sampler mask in the push constant, but also create a new sampler with normalised coordinate if not already the case or already created.

@rjodinchr rjodinchr force-pushed the dev/sampler-arg branch 2 times, most recently from 5ecd83c to f07fe59 Compare September 20, 2023 12:22
@rjodinchr
Copy link
Contributor Author

Ref google/clspv#1202

Copy link
Owner

@kpet kpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with the approach. A couple of (suggested) name tweaks would make the code much easier to read and maintain.

src/kernel.cpp Show resolved Hide resolved
@@ -237,7 +245,8 @@ struct cvk_kernel_argument_values {
}

if (m_entry_point->has_pod_arguments() ||
m_entry_point->has_image_metadata()) {
m_entry_point->has_image_metadata() ||
m_entry_point->has_sampler_metadata()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really time we refactor push constant management to make it more readable. In a lot of places, they're still named "POD". Best done as a standalone changes.

src/memory.cpp Outdated Show resolved Hide resolved
src/memory.hpp Outdated Show resolved Hide resolved
src/memory.hpp Show resolved Hide resolved
sizeof(uint32_t) -
push_constant_range.offset;
push_constant_range.size =
offset + sizeof(uint32_t) - push_constant_range.offset;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is now screaming "refactor me" :). To be done as a standalone change.

src/kernel.cpp Outdated Show resolved Hide resolved
Set the sampler mask in the push constant, but also create a new
sampler with normalised coordinate if not already the case or already
created.
@rjodinchr
Copy link
Contributor Author

This PR will need to have clspv and spirv-headers updated to go it

@rjodinchr rjodinchr requested review from kpet and alan-baker October 13, 2023 06:17
Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Owner

@kpet kpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. To be able to update clspv, we are going to need to get #609 in first.

@kpet kpet merged commit b65b365 into kpet:main Oct 29, 2023
1 of 9 checks passed
@rjodinchr rjodinchr deleted the dev/sampler-arg branch October 30, 2023 13:29
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.

3 participants