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

Enable use of native texture atomics. #2164

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

js6i
Copy link
Collaborator

@js6i js6i commented Feb 23, 2024

This commit conditionally skips the emulated image atomics paths if native texture atomics are available and a configuration option is set.

Apart from unlocking some potential performance benefits from not having to force some textures to be linear, it also makes texture atomics work with argument buffers.

Copy link
Contributor

@billhollings billhollings left a comment

Choose a reason for hiding this comment

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

Thanks for submitting.

Question about need for config parameter, and minor nit on code org.

@js6i js6i force-pushed the native-atomics branch 2 times, most recently from 6b79e9d to 67bed4f Compare February 27, 2024 11:38
This commit conditionally skips the emulated image atomics paths if native
texture atomics are available and a configuration option is set.

Apart from unlocking some potential performance benefits from not having to
force some textures to be linear, it also makes texture atomics work with
argument buffers.
Copy link
Contributor

@billhollings billhollings left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM now.

@billhollings billhollings merged commit b56c152 into KhronosGroup:main Feb 27, 2024
6 checks passed
Comment on lines -938 to +943
_isLinearForAtomics = (_arrayLayers == 1 && _mipLevels == 1 && getImageType() == VK_IMAGE_TYPE_2D && mvkIsAnyFlagEnabled(getCombinedUsage(), VK_IMAGE_USAGE_STORAGE_BIT) &&
((_vkFormat == VK_FORMAT_R32_UINT || _vkFormat == VK_FORMAT_R32_SINT) ||
(_hasMutableFormat && pixFmts->getViewClass(_vkFormat) == MVKMTLViewClass::Color32 &&
(getIsValidViewFormat(VK_FORMAT_R32_UINT) || getIsValidViewFormat(VK_FORMAT_R32_SINT)))));
_shouldSupportAtomics = mvkIsAnyFlagEnabled(getCombinedUsage(), VK_IMAGE_USAGE_STORAGE_BIT) && _mipLevels == 1 &&
((_vkFormat == VK_FORMAT_R32_UINT || _vkFormat == VK_FORMAT_R32_SINT) ||
(_hasMutableFormat && pixFmts->getViewClass(_vkFormat) == MVKMTLViewClass::Color32 && (getIsValidViewFormat(VK_FORMAT_R32_UINT) || getIsValidViewFormat(VK_FORMAT_R32_SINT))));

if (_shouldSupportAtomics && !getPhysicalDevice()->useNativeTextureAtomics())
_isLinearForAtomics = _arrayLayers == 1 && getImageType() == VK_IMAGE_TYPE_2D;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something about this change here appears to have caused #2168.

@js6i
Copy link
Collaborator Author

js6i commented Feb 29, 2024

Apart from #2168, I found a second issue when testing another patch - the validation layer complains about MTLTextureUsageShaderAtomic when the pixel format isn't an integer, which currently triggers for buffer views, and images in the _mutableFormat case. Maybe I was not overly tentative after all.

Perhaps, for image views, we could force the primary pixel format of the texture to be an integer, and then make views on that..

@billhollings

@billhollings
Copy link
Contributor

Apart from #2168, I found a second issue when testing another patch - the validation layer complains about MTLTextureUsageShaderAtomic when the pixel format isn't an integer, which currently triggers for buffer views, and images in the _mutableFormat case. Maybe I was not overly tentative after all.

Perhaps, for image views, we could force the primary pixel format of the texture to be an integer, and then make views on that..

@billhollings

Can you open an issue tracker for this second valuation issue?

@js6i js6i mentioned this pull request Mar 6, 2024
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