WIP: Add partial VK_EXT_image_2d_view_of_3d support #2332

ncesario-lunarg · 2024-09-10T00:53:17Z

Given a 3D texture that is backed by a placement heap in MoltenVK, the
approach taken here is to create a 2D texture that is backed by memory
pointing into a 3D texture's memory.

While ideal compared to alternative implementation solutions for this
extension, this approach is sensitive to how Apple lays out the memory
for 3D textures. The solution here uses
heapTextureSizeAndAlignWithDescriptor to determine the overall size of a
given 3D texture and index into the beginning of each "slice" of the 3D
texture. So far this is good enough for storage images in CTS.

FYI @kocdemir.

billhollings

Thanks for getting this started!

In addition to the inline change requests, I recognize this is just to solve one immediate extension issue, but if we can do what we can to simplify a future move to using MTLHeaps, we should try as best we can.

billhollings · 2024-09-12T18:47:43Z

MoltenVK/MoltenVK/GPUObjects/MVKDeviceMemory.mm

@@ -188,10 +188,6 @@
 	// Can't create MTLHeaps of zero size.
 	if (_allocationSize == 0) { return true; }

-#if MVK_MACOS
-	// MTLHeaps on macOS must use private storage for now.
-	if (_mtlStorageMode != MTLStorageModePrivate) { return true; }


Instead of being removed, does this need to be modified to include a test for non-Apple-Silicon devices?

See comment below.

billhollings · 2024-09-12T18:50:35Z

MoltenVK/MoltenVK/GPUObjects/MVKDeviceMemory.mm

-#if MVK_MACOS
-	// MTLHeaps on macOS must use private storage for now.
-	if (_mtlStorageMode != MTLStorageModePrivate) { return true; }
-#endif
 #if MVK_IOS


Should both this section and the one above, be replaced with a single test for MVKMTLDeviceCapabilities.isAppleGPU?

TBH I'm not sure based on the docs, but I'll add that to be on the safe side. It looked like the usage of heaps in MoltenVK was mainly dependent on placement heaps, but my knowledge is still pretty sparse on the details here.

billhollings · 2024-09-12T18:52:53Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.h

@@ -48,6 +48,16 @@ typedef struct {
 class MVKImagePlane : public MVKBaseObject {

 public:
+    struct HeapAllocation {


Should this declaration be moved to MVKDeviceMemory or MVKResource, since heaps can be used by buffers too?

Yep, I'll move it there.

@aitor-lunarg as this is something that may have a more straightforward approach with WIP: Proposal to base VkDeviceMemory on MTLHeap always #2309.

billhollings · 2024-09-12T19:37:43Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

@@ -1066,6 +1079,11 @@ static MTLRegion getMTLRegion(const ImgRgn& imgRgn) {
 	return _memoryBindings[0]->_deviceMemory ? _memoryBindings[0]->_deviceMemory->getMTLCPUCacheMode() : MTLCPUCacheModeDefaultCache;
 }

+MVKImagePlane::HeapAllocation* MVKImage::getHeapAllocation(uint32_t planeIndex) {
+    auto& heapAllocation = _planes[planeIndex]->_heapAllocation;
+    return (heapAllocation) ? &heapAllocation : nullptr;


It surprised me that this was compiling, until I noticed the operator bool() on the struct above.

While clever, we don't use this design pattern anywhere else, so I'm concerned others will balk the same way I did.

Maybe rename operator bool() to bool isValid(), for clarity.

Changed to isValid().

billhollings · 2024-09-12T19:40:44Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

@@ -1251,6 +1269,8 @@ static MTLRegion getMTLRegion(const ImgRgn& imgRgn) {
 	if (pExportInfo && pExportInfo->exportObjectType == VK_EXPORT_METAL_OBJECT_TYPE_METAL_IOSURFACE_BIT_EXT && !_ioSurface) {
 		setConfigurationResult(useIOSurface(nil));
 	}
+
+    _is2DViewCompatible = pCreateInfo->flags & VK_IMAGE_CREATE_2D_VIEW_COMPATIBLE_BIT_EXT;


Use mvkIsAnyFlagEnabled() for consistency.

billhollings · 2024-09-12T19:43:33Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.h

@@ -395,6 +410,7 @@ class MVKImage : public MVKVulkanAPIDeviceObject {
 	bool _hasMutableFormat;
 	bool _shouldSupportAtomics;
 	bool _isLinearForAtomics;
+    bool _is2DViewCompatible = false;


I assume this will apply only to 2D on 3D, and not for all 2D views?

Maybe rename to _is2DViewOn3DImageCompatible?

Yep, makes sense. Done.

billhollings · 2024-09-12T19:58:26Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.h

+        size_t size = 0; // Total size of this allocation
+        size_t align = 0; // Allocation alignment requirement
+
+        operator bool() const {


See my comment below where this is used. Probably best to rename this to something like bool isValid().

billhollings · 2024-09-12T20:10:16Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.h

+
+    // For now, this is only used for VK_EXT_image_2d_view_of_3d. Specifically, when the backing image of this view is 3D and the view is 2D,
+    // this texture will be allocated using a placement heap "on top of" the 3D textures backing memory.
+    id<MTLTexture> _mtlShadowTexture;


Since _mtlTexture end up holding a reference to the same object, does this need to be a separate iVar than _mtlTexture?

If it is needed for some reason, move it up just below the _mtlTexture declaration. We try to organize iVars to minimize gaps in the underlying class structure (ie- keep all 8-byte iVars together).

It also feels dangerous that the MTLTexture is not retained in both _mtlTexture and _mtlShadowTexture, and released from both in the destructor. If in the future, we might possibly have different objects in the two iVars, then we'll end up with a hard to discover memory leak.

Finally, maybe it's me, but when I see _mtlShadowTexture, I keep thinking "shadow map texture". Maybe rename it to _mtlAliasTexture or something.

Nope, there is no need for the extra MTLTexture in MVKImageViewPlane; I will remove it. I added this from a debug session and never reverted it (and also forgot to clean it up in the destructor when I was using it :/).

ncesario-lunarg · 2024-09-13T17:22:06Z

Thanks for the review @billhollings! I've made the changes you've suggested locally, but I'm hitting a crash on dEQP-VK.api.get_memory_commitment.memory_commitment at https://github.com/ncesario-lunarg/MoltenVK/blob/d01e7ecf13ef9bd4d9af3dc61b13ce683299d5ba/MoltenVK/MoltenVK/GPUObjects/MVKDeviceMemory.mm#L205. I'm going to resolve that before updating this PR.

squidbus · 2024-10-09T22:49:32Z

Does this PR impact the imageView2DOn3DImage feature disabled in the portability subset at all?

ncesario-lunarg · 2024-10-17T16:42:12Z

@squidbus yes it does, and I had mistakenly not enabled that, but it should be enabled now.

Given a 3D texture that is backed by a placement heap in MoltenVK, the approach taken here is to create a 2D texture that is backed by memory pointing into a 3D texture's memory. While ideal compared to alternative implementation solutions for this extension, this approach is sensitive to how Apple lays out the memory for 3D textures. The solution here uses heapTextureSizeAndAlignWithDescriptor to determine the overall size of a given 3D texture and index into the beginning of each "slice" of the 3D texture. So far this is good enough for storage images in CTS.

ncesario-lunarg · 2024-10-31T18:10:25Z

MoltenVK/MoltenVK/Utility/MVKEnvironment.cpp

 #include "MVKConfigMembers.def"
-	cfgSize += kMVKConfigurationInternalPaddingByteCount;


@billhollings This seemed to be calculating the incorrect size, but perhaps that meant I added the config member incorrectly? I changed this function to align the expected size to the alignment of MVKConfiguration; please let me know if something is "off" here.

iiuc, this is to make the padding requirement of the structure clearer and it expects modification to kMVKConfigurationInternalPaddingByteCount with the new additions, as the real size of the structure seems to be important for backwards compatibility. I believe with this new version, adding a new item only to the .def file when there is a padding in the structure (or in some cases adding an item to the both sides with different sizes) won't generate an error. Both paths are prone to errors and probably it should use a better protocol, but it might be easier to follow the changes with with an explicit padding value.

Otherwise, I don't see any other functionality of kMVKConfigurationInternalPaddingByteCount variable, and if this is going to be submitted, maybe that value can be removed as well.

Have a look at the description of kMVKConfigurationInternalPaddingByteCount. It's sole purpose is to adjust the calculation of:

static_assert(getExpectedMVKConfigurationSize() == sizeof(MVKConfiguration), "MVKConfigMembers.def does not match the members of MVKConfiguration.");

And the purpose of that static_assert() is to ensure that the members of MVKConfiguration are all accounted for in MVKConfigMembers.def (ie- a dev mod isn't made to MVKConfiguration without adding the corrsponding entry in MVKConfigMembers.def), which would be tough to debug.

The reason kMVKConfigurationInternalPaddingByteCount is needed is to account for alignment padding that the compiler adds to sizeof(MVKConfiguration).

The solution to the issue you have dealt with here is to adjust the value of kMVKConfigurationInternalPaddingByteCount accordingly. In this specific case, the value should be set to 0, instead of removing the padding calculation. The next time an entry is added to MVKConfiguration, it will need to be set to a different value, and the padding calculation needs to be available.

kocdemir · 2024-11-01T16:31:48Z

MoltenVK/MoltenVK/Layers/MVKExtensions.def

@@ -144,6 +144,7 @@ MVK_EXTENSION(EXT_swapchain_maintenance1,             EXT_SWAPCHAIN_MAINTENANCE_
 MVK_EXTENSION(EXT_texel_buffer_alignment,             EXT_TEXEL_BUFFER_ALIGNMENT,             DEVICE,   10.13, 11.0,  1.0)
 MVK_EXTENSION(EXT_texture_compression_astc_hdr,       EXT_TEXTURE_COMPRESSION_ASTC_HDR,       DEVICE,   11.0,  13.0,  1.0)
 MVK_EXTENSION(EXT_vertex_attribute_divisor,           EXT_VERTEX_ATTRIBUTE_DIVISOR,           DEVICE,   10.11,  8.0,  1.0)
+MVK_EXTENSION(EXT_image_2d_view_of_3d,                EXT_IMAGE_2D_VIEW_OF_3D,                DEVICE,   10.15, 13.0,  1.0)


Do you think we can modify mvkIsSupportedOnPlatform check and not advertise support on this when the config flag is not enabled?

Yes, I'll wrap that in an #ifdef.

Wrapping in #ifdef here only works for environment variables, and not VK_EXT_layer_settings.

To properly support allowing this extension to be enabled via settings, use something like:

pWritableExtns->vk_EXT_image_2d_view_of_3d.enabled = getMVKConfig().enable2DViewOf3D;

in MVKPhysicalDevice::initExtensions().

However, see my question above about whether we need to have this configurable at all.

Since the implementation makes some assumptions as to how 3D texture memory is laid out by the metal driver, only advertise VK_EXT_image_2d_view_of_3d if MVK_CONFIG_ENABLE_2DVIEWOF3D is enabled. MVK_CONFIG_ENABLE_2DVIEWOF3D is off by default.

Sets VkPhysicalDeviceImage2DViewOf3DFeaturesEXT::sampler2DViewOf3D to false, as CTS tests involving 2D views of sampled 3D textures are not currently working.

billhollings

Thanks for getting this ready for release!

I've made a few change requests to catch a few potential memory leaks and simplify the configuration option.

billhollings · 2024-11-08T22:23:51Z

MoltenVK/MoltenVK/Layers/MVKExtensions.def

@@ -144,6 +144,7 @@ MVK_EXTENSION(EXT_swapchain_maintenance1,             EXT_SWAPCHAIN_MAINTENANCE_
 MVK_EXTENSION(EXT_texel_buffer_alignment,             EXT_TEXEL_BUFFER_ALIGNMENT,             DEVICE,   10.13, 11.0,  1.0)
 MVK_EXTENSION(EXT_texture_compression_astc_hdr,       EXT_TEXTURE_COMPRESSION_ASTC_HDR,       DEVICE,   11.0,  13.0,  1.0)
 MVK_EXTENSION(EXT_vertex_attribute_divisor,           EXT_VERTEX_ATTRIBUTE_DIVISOR,           DEVICE,   10.11,  8.0,  1.0)
+MVK_EXTENSION(EXT_image_2d_view_of_3d,                EXT_IMAGE_2D_VIEW_OF_3D,                DEVICE,   10.15, 13.0,  1.0)


Wrapping in #ifdef here only works for environment variables, and not VK_EXT_layer_settings.

To properly support allowing this extension to be enabled via settings, use something like:

pWritableExtns->vk_EXT_image_2d_view_of_3d.enabled = getMVKConfig().enable2DViewOf3D;

in MVKPhysicalDevice::initExtensions().

However, see my question above about whether we need to have this configurable at all.

billhollings · 2024-11-08T22:29:50Z

Docs/MoltenVK_Configuration_Parameters.md

@@ -684,3 +684,15 @@ Determines the style used to implement _Vulkan_ semaphore (`VkSemaphore`) functi

 In the special case of `VK_SEMAPHORE_TYPE_TIMELINE` semaphores, **MoltenVK** will always use
 `MTLSharedEvent` if it is available on the platform, regardless of the value of this parameter.
+
+---------------------------------------
+#### MVK_CONFIG_ENABLE_2DVIEWOF3D


Is there a reason to have the app have to opt-in to this extension?

If not, I've run CTS on everything, and it all seems to be working fine, and doesn't seem to be interfering with other behaviour. I think I would be satisfied to remove MVK_CONFIG_ENABLE_2DVIEWOF3D and MVKConfiguration::enable2DViewOf3D, and then where you test for enable2DViewOf3D internally, use useMTLHeap instead.

billhollings · 2024-11-08T22:32:09Z

MoltenVK/MoltenVK/Utility/MVKEnvironment.cpp

 #include "MVKConfigMembers.def"
-	cfgSize += kMVKConfigurationInternalPaddingByteCount;


Have a look at the description of kMVKConfigurationInternalPaddingByteCount. It's sole purpose is to adjust the calculation of:

static_assert(getExpectedMVKConfigurationSize() == sizeof(MVKConfiguration), "MVKConfigMembers.def does not match the members of MVKConfiguration.");

And the purpose of that static_assert() is to ensure that the members of MVKConfiguration are all accounted for in MVKConfigMembers.def (ie- a dev mod isn't made to MVKConfiguration without adding the corrsponding entry in MVKConfigMembers.def), which would be tough to debug.

The reason kMVKConfigurationInternalPaddingByteCount is needed is to account for alignment padding that the compiler adds to sizeof(MVKConfiguration).

The solution to the issue you have dealt with here is to adjust the value of kMVKConfigurationInternalPaddingByteCount accordingly. In this specific case, the value should be set to 0, instead of removing the padding calculation. The next time an entry is added to MVKConfiguration, it will need to be set to a different value, and the padding calculation needs to be available.

billhollings · 2024-11-08T23:53:44Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

@@ -823,6 +832,10 @@ static MTLRegion getMTLRegion(const ImgRgn& imgRgn) {
    imgData.usage = getCombinedUsage();
 }

+MTLTextureDescriptor* MVKImage::getTextureDescriptor(uint32_t planeIndex) {
+    return _planes[planeIndex]->newMTLTextureDescriptor();
+}


getTextureDescriptor() should be renamed to newMTLTextureDescriptor, to clarify that the returned object is retained, and needs to be released by the caller.

Since this function is only used once, it might be better to remove this getTextureDescriptor(), and replace its use with just _planes[planeIndex]->newMTLTextureDescriptor().

billhollings · 2024-11-09T00:01:54Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

+                // TODO (ncesario-lunarg) untested where _imageView->subresourceRange.layerCount > 1, but VK_EXT_image_2d_view_of_3d
+                //      allows for 2D_ARRAY views of 3D textures.
+                const auto relativeSliceOffset = _imageView->_subresourceRange.baseArrayLayer * (heapAllocation->size / image->_extent.depth);
+                MTLTextureDescriptor* mtlTexDesc = image->getTextureDescriptor(_planeIndex);


mtlTexDesc will leak if it isn't released after it is used. See other uses of newMTLTextureDescriptor() for guidance on that.

billhollings · 2024-11-09T00:04:00Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

+            mtlTextureType = MTLTextureType3D;
+            sliceRange = NSMakeRange(0, 1);
+        } else {
+            if (!_mtlTexture) {


This test is unnecessary, since newMTLTexture() is only called if _mtlTexture is nil.

billhollings · 2024-11-09T00:22:08Z

MoltenVK/MoltenVK/GPUObjects/MVKImage.mm

+                // Create a temporary texture that is backed by the 3D texture's memory
+                _mtlTexture = [heapAllocation->heap
+                                     newTextureWithDescriptor: mtlTexDesc
+                                     offset: heapAllocation->offset + relativeSliceOffset];


Two issues here.

This unexpectedly plugs a value into _mtlTexture, which will be overridden by the value returned by this function.

The new MTLTexture created here is retained, and will leak if it is not released at some point.

I suggest:

Declare a id<MTLTexture> aliasTex = nil; at the top of this function.

Assign aliasTex here from the call to newTextureWithDescriptor(), and set mtlTex = aliasTex;.

Replace the two return [mtlTex newTextureView... below with texView = [mtlTex newTextureView....

End the function with:

[aliasTex release]; return texView;

It's important to release aliasTex after texView has been created, and aliasTex will be retained within texView, and will be released and deallocated when texView is released and deallocated.

billhollings requested changes Sep 12, 2024

View reviewed changes

ncesario-lunarg force-pushed the 2dviewof3d-2 branch from d01e7ec to ba83997 Compare September 14, 2024 00:09

ncesario-lunarg force-pushed the 2dviewof3d-2 branch from ba83997 to ce6e1fc Compare October 17, 2024 16:41

ncesario-lunarg force-pushed the 2dviewof3d-2 branch from ce6e1fc to 3329d40 Compare October 31, 2024 18:06

ncesario-lunarg commented Oct 31, 2024

View reviewed changes

ncesario-lunarg force-pushed the 2dviewof3d-2 branch from 3329d40 to 962c94f Compare October 31, 2024 18:15

ncesario-lunarg changed the title ~~[WIP] Add VK_EXT_image_2d_view_of_3d support~~ Add partial VK_EXT_image_2d_view_of_3d support Oct 31, 2024

kocdemir reviewed Nov 1, 2024

View reviewed changes

ncesario-lunarg added 2 commits November 1, 2024 12:04

Disable sampler2DViewOf3D

2a62041

Sets VkPhysicalDeviceImage2DViewOf3DFeaturesEXT::sampler2DViewOf3D to false, as CTS tests involving 2D views of sampled 3D textures are not currently working.

ncesario-lunarg force-pushed the 2dviewof3d-2 branch from 962c94f to 2a62041 Compare November 1, 2024 18:04

billhollings requested changes Nov 9, 2024

View reviewed changes

billhollings changed the title ~~Add partial VK_EXT_image_2d_view_of_3d support~~ WIP: Add partial VK_EXT_image_2d_view_of_3d support Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add partial VK_EXT_image_2d_view_of_3d support #2332

WIP: Add partial VK_EXT_image_2d_view_of_3d support #2332

ncesario-lunarg commented Sep 10, 2024

billhollings left a comment •

edited

Loading

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

billhollings Sep 12, 2024

ncesario-lunarg Sep 12, 2024

ncesario-lunarg commented Sep 13, 2024

squidbus commented Oct 9, 2024

ncesario-lunarg commented Oct 17, 2024

ncesario-lunarg Oct 31, 2024

kocdemir Nov 1, 2024

billhollings Nov 8, 2024

kocdemir Nov 1, 2024

ncesario-lunarg Nov 1, 2024

billhollings Nov 8, 2024

billhollings left a comment

billhollings Nov 8, 2024

billhollings Nov 8, 2024

billhollings Nov 8, 2024

billhollings Nov 8, 2024

billhollings Nov 9, 2024

billhollings Nov 9, 2024

billhollings Nov 9, 2024

		#include "MVKConfigMembers.def"
		cfgSize += kMVKConfigurationInternalPaddingByteCount;

WIP: Add partial VK_EXT_image_2d_view_of_3d support #2332

Are you sure you want to change the base?

WIP: Add partial VK_EXT_image_2d_view_of_3d support #2332

Conversation

ncesario-lunarg commented Sep 10, 2024

billhollings left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncesario-lunarg commented Sep 13, 2024

squidbus commented Oct 9, 2024

ncesario-lunarg commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billhollings left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billhollings left a comment •

edited

Loading