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 trimming that some current layout of image isn't tracked #1842

Closed
wants to merge 1 commit into from

Conversation

zongdu-arm
Copy link
Contributor

@zongdu-arm zongdu-arm commented Oct 29, 2024

Images with layout undefined will skip to dump snapshot during trimming, but not all the image layouts are set by VkImageBarrier or Renderpass, and CopyImage, BlitImage and ResolveImage can also be set.

Images with layout undefined will skip to dump snapshot during trimming, but not all the image layouts are set by VkImageBarrier or Renderpass, and CopyImage, BlitImage and ResolveImage.
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 290077.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5226 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5226 passed.

@zongdu-arm
Copy link
Contributor Author

Hi @panos-lunarg, @bradgrantham-lunarg,
Could you review and merge this commit?

@panos-lunarg
Copy link
Contributor

panos-lunarg commented Nov 12, 2024

I am not sure I understand when layout transitioning can be missed when tracking. None of the vulkan commands that are tracked and touched in this PR perform a layout transition.
Can you please give an example of when it can happen and this change will fix it?

@zongdu-arm
Copy link
Contributor Author

zongdu-arm commented Nov 13, 2024

The example api call, image 382 is just a sample image. Image can be specify to this dest layout VK_IMAGE_LAYOUT_GENERAL from vkCmdCopyBufferToImage, does not use image barrier, or attachment in renderpass.
So image 382 can not be trimmed, the trimmed gfxr renders error.

{"index":367229,"function":{"name":"vkCreateImage","thread":1,"return":"VK_SUCCESS","args":{"device":5,"pCreateInfo":{"sType":"VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO","flags":"0x00000000","imageType":"VK_IMAGE_TYPE_2D","format":"VK_FORMAT_R8G8B8A8_UNORM","extent":{"width":1,"height":1,"depth":1},"mipLevels":1,"arrayLayers":1,"samples":"VK_SAMPLE_COUNT_1_BIT","tiling":"VK_IMAGE_TILING_OPTIMAL","usage":"0x00000007","sharingMode":"VK_SHARING_MODE_EXCLUSIVE","queueFamilyIndexCount":0,"pQueueFamilyIndices":null,"initialLayout":"VK_IMAGE_LAYOUT_UNDEFINED","pNext":null},"pAllocator":{"pUserData":"0x0","pfnAllocation":"0x74abf69a60","pfnReallocation":"0x74abf69a80","pfnFree":"0x74abf69a70","pfnInternalAllocation":"0x74abf69a8c","pfnInternalFree":"0x74abf69a90"},"pImage":382}}} {"index":367230,"function":{"name":"vkGetImageMemoryRequirements","thread":1,"args":{"device":5,"image":382,"pMemoryRequirements":{"size":1024,"alignment":64,"memoryTypeBits":3}}}} {"index":367231,"function":{"name":"vkBindImageMemory","thread":1,"return":"VK_SUCCESS","args":{"device":5,"image":382,"memory":17,"memoryOffset":2048}}} {"index":367232,"function":{"name":"vkCreateImageView","thread":1,"return":"VK_SUCCESS","args":{"device":5,"pCreateInfo":{"sType":"VK_STRUCTURE_TYPE_IMAGE_VIEW_CREATE_INFO","flags":"0x00000000","image":382,"viewType":"VK_IMAGE_VIEW_TYPE_2D","format":"VK_FORMAT_R8G8B8A8_UNORM","components":{"r":"VK_COMPONENT_SWIZZLE_IDENTITY","g":"VK_COMPONENT_SWIZZLE_IDENTITY","b":"VK_COMPONENT_SWIZZLE_IDENTITY","a":"VK_COMPONENT_SWIZZLE_IDENTITY"},"subresourceRange":{"aspectMask":"0x00000001","baseMipLevel":0,"levelCount":4294967295,"baseArrayLayer":0,"layerCount":1},"pNext":null},"pAllocator":{"pUserData":"0x0","pfnAllocation":"0x74abf69a60","pfnReallocation":"0x74abf69a80","pfnFree":"0x74abf69a70","pfnInternalAllocation":"0x74abf69a8c","pfnInternalFree":"0x74abf69a90"},"pView":383}}} {"index":367233,"function":{"name":"vkCmdCopyBufferToImage","thread":1,"cmd_index":1,"args":{"commandBuffer":337,"srcBuffer":363,"dstImage":382,"dstImageLayout":"VK_IMAGE_LAYOUT_GENERAL","regionCount":1,"pRegions":[{"bufferOffset":0,"bufferRowLength":16,"bufferImageHeight":1,"imageSubresource":{"aspectMask":"0x00000001","mipLevel":0,"baseArrayLayer":0,"layerCount":1},"imageOffset":{"x":0,"y":0,"z":0},"imageExtent":{"width":1,"height":1,"depth":1}}]}}} {"index":367234,"function":{"name":"vkGetImageMemoryRequirements","thread":1,"args":{"device":5,"image":382,"pMemoryRequirements":{"size":1024,"alignment":64,"memoryTypeBits":3}}}} {"index":375999,"function":{"name":"vkUpdateDescriptorSets","thread":7,"args":{"device":5,"descriptorWriteCount":1,"pDescriptorWrites":[{"sType":"VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET","dstSet":381,"dstBinding":294,"dstArrayElement":138,"descriptorCount":1,"descriptorType":"VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE","pImageInfo":[{"sampler":0,"imageView":383,"imageLayout":1}],"pNext":null}],"descriptorCopyCount":0,"pDescriptorCopies":null}}}

@panos-lunarg
Copy link
Contributor

In your example Image 382 is created with "initialLayout": "VK_IMAGE_LAYOUT_UNDEFINED".
Then the vkCmdCopyBufferToImage uses "dstImageLayout": "VK_IMAGE_LAYOUT_GENERAL".
If am reading the spec correctly this is invalid usage, no?

VUID-vkCmdCopyBufferToImage-dstImageLayout-00180
dstImageLayout must specify the layout of the image subresources of dstImage specified in pRegions at the time this command is executed on a VkDevice

@zongdu-arm
Copy link
Contributor Author

Yes, according the spec, vkCmdCopyBufferToImage does not transition image memory layout, the app calls vkCmdCopyBufferToImage without adding an appropriate image memory barrier, it may result in undefined behavior or incorrect data. This is an app bug.

@zongdu-arm zongdu-arm closed this Nov 13, 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