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

Support for VK_EXT_image_drm_format_modifier #1194

Open
szihs opened this issue Dec 4, 2018 · 24 comments
Open

Support for VK_EXT_image_drm_format_modifier #1194

szihs opened this issue Dec 4, 2018 · 24 comments
Labels
Feature An improvement or feature Unresolved Waiting for a fix or implementation

Comments

@szihs
Copy link

szihs commented Dec 4, 2018

Hi

Is there any plan to add support for VK_EXT_image_drm_format_modifier and VK_KHR_sampler_ycbcr_conversion extensions.

Regards,
Harsh

@baldurk
Copy link
Owner

baldurk commented Dec 4, 2018

Generally for extensions KHR are highly likely to be supported at some point, and EXT extensions are generally planned but priority may vary. Knowing which extensions are actually used and desired helps though.

VK_KHR_sampler_ycbcr_conversion is already supported as it's part of core 1.1 Vulkan - I assume you meant specifically that you want the physical device feature to be enabled?

For these extensions in particular it would greatly help to have a simple test case that exercises them in a non-trivial way, as implementing extensions 'blind' can easily lead to inadvertent bugs.

@baldurk baldurk added Feature An improvement or feature Unresolved Waiting for a fix or implementation labels Dec 4, 2018
@szihs
Copy link
Author

szihs commented Dec 4, 2018

Background: Inspecting some issue where application is using both these extensions.

in renderdoc/driver/vulkan/vk_core.cpp
Currently I just appended to supportedExtensions[] below change at proper order

{
      VK_EXT_IMAGE_DRM_FORMAT_MODIFIER_EXTENSION_NAME,    VK_EXT_IMAGE_DRM_FORMAT_MODIFIER_SPEC_VERSION,
  },

And compiled renderdoc locally.

This way I could capture frame the renderdoc. (Otherwise application would exit because of renderdoc not exposing the extension. I do think we need to do changes at more place to serialize and save the specifics,

After successful capture, On replaying I am getting crash with vkCreateImage() being passed invalid sizes.

I am not sure if it is because of my renderdoc change , unsupported extensions by rdoc / application issue.

I may try to add support in my local copy if you can guide me.
Any feedback will help.

@baldurk
Copy link
Owner

baldurk commented Dec 4, 2018

Yes the reason for the supportedExtensions array is because any extensions not in that array will likely either crash on capture, crash on replay, or if you're lucky then work but possibly display wrongly on replay. Adding the extension to that array doesn't add support for the extension.

To support an extension RenderDoc needs to handle all function calls, structures etc that are part of that extension, both on capture and replay. What is needed to support an extension varies depending on what the extension actually provides.

@szihs
Copy link
Author

szihs commented Dec 4, 2018

I will check codes how to implement it then.

By the way, does that mean i cannot use YCbCr samplers ?

  if(ycbcr)
  {
    RDCWARN("Forcibly disabling support for YCbCr Conversion");
    ycbcr->samplerYcbcrConversion = VK_FALSE;
  }

Is VK_KHR_sampler_ycbcr_conversion atleast supported then ?

@baldurk
Copy link
Owner

baldurk commented Dec 4, 2018

Yes the extension is supported as it's part of Vulkan 1.1 core, but the physical device feature is marked as not available meaning you can't use ycbcr samplers. This is the same as it would look if a device driver reported the extension but didn't report ycbcr sampler support.

@szihs
Copy link
Author

szihs commented Dec 4, 2018

What would be the behaviour if application still use it when running in renderdoc context (ignore the physical device feature)
Considering the actual device driver does support it.

So the renderdoc will not pass through the information to underlying actual device driver ?
Or will it work normally ?

Thanks for your reply.

@baldurk
Copy link
Owner

baldurk commented Dec 4, 2018

I don't know, the application is then breaking the vulkan spec by using features that are reported as unavailable and haven't been enabled. As I mentioned above this may crash during capture, during replay, it may replay incorrectly, or it might work fine for 99% of cases and break in 1%.

A correct vulkan application should make sure it enumerates the available extensions and features. Your code must not use features that aren't reported, and then it will work correctly with RenderDoc.

@szihs
Copy link
Author

szihs commented Dec 5, 2018

I understand. Thanks for the explanation

@baldurk
Copy link
Owner

baldurk commented Dec 14, 2018

I've pushed support for ycbcr conversion samplers and displaying YUV textures (in D3D as well as Vulkan), which addresses part of what you were asking for.

I don't know when I'll get to VK_EXT_image_drm_format_modifier. As I mentioned before, having a non-trivial example program would greatly help.

@szihs
Copy link
Author

szihs commented Dec 18, 2018

Thank you for your prompt reply.
I do have one query. Since VK_EXT_image_drm_format_modifier would include importing external memory, one would have to create the same external memory while replaying.

Is there any similar code in the tree which i can refer to start basic implementation to make it work.
Where external dma-buf/opaque-fd type image is used.

Thanks.

@baldurk
Copy link
Owner

baldurk commented Dec 18, 2018

External memory itself is already supported, but on replay it just creates an equivalent image without external memory properties so for the most part the only support needed is to handle processing & serialising the external memory structs, which are then discarded on replay. You can look for e.g. VkExternalMemoryImageCreateInfo it's primarily mentioned in vk_next_chains.cpp for unwrapping/processing and vk_serialise.cpp to serialise the structs.

I haven't looked at the VK_EXT_image_drm_format_modifier extension but if it relates to external memory I'd expect to handle it similarly as it's not possible to also serialise any things outside of Vulkan needed to initialise external memory.

@szihs
Copy link
Author

szihs commented Dec 28, 2018

Hello I was able to add partial support for VK_EXT_image_drm_format_modifier locally.

It is able to capture the rdc file, however while replay on the same device I get error

RDOC 006886: [18:26:47] vk_resource_funcs.cpp( 231) - Error   - Trying to bind Image 124934 to memory 124935 which is type 0, but at offset 0x0 the reported size of 0x29580 won't fit the 0x1c000 bytes of memory.
This is most likely caused by incompatible hardware or drivers between capture and replay, causing a change in memory requirements.

I guess, it maybe because the external image sent to vkCreateImage has multiple planes.
However I am not sure if WrappedVulkan::vkCreateImage handles that appropriately.

Kindly help.

@baldurk
Copy link
Owner

baldurk commented Dec 28, 2018

As I mentioned before I haven't looked at the extension so I don't know what's involved with supporting it, if there's special handling needed in vkCreateImage then you will need to add it, there is no support in the current codebase for this extension.

@szihs
Copy link
Author

szihs commented Dec 31, 2018

Hello,
I debugged the issue and it looks like there is issue when Serialize_vkBindImageMemory2 bindInfoCount=2 The vulkan image VkImage `passed for both planes are same, however it should check sum total of both planes memory requirements.

diff --git a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp
index 7fdd2fb..21cce71 100644
--- a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp
+++ b/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp
@@ -1903,6 +1903,7 @@ bool WrappedVulkan::Serialise_vkBindImageMemory2(SerialiserType &ser, VkDevice d
       ResourceId resOrigId = GetResourceManager()->GetOriginalID(GetResID(bindInfo.image));
       ResourceId memOrigId = GetResourceManager()->GetOriginalID(GetResID(bindInfo.memory));
 
+#if 0
       VkMemoryRequirements mrq = {};
       ObjDisp(device)->GetImageMemoryRequirements(Unwrap(device), Unwrap(bindInfo.image), &mrq);
 
@@ -1911,6 +1912,7 @@ bool WrappedVulkan::Serialise_vkBindImageMemory2(SerialiserType &ser, VkDevice d
 
       if(!ok)
         return false;
+#endif
 
       GetReplay()->GetResourceDesc(memOrigId).derivedResources.push_back(resOrigId);
       GetReplay()->GetResourceDesc(resOrigId).parentResources.push_back(memOrigId);

I commented the debug check and it replay it well.

I have one further question, For external memory which is to be used as a sampler and has texture,
does renderdoc copy the buffer contents, so that replay is corrrect ?

Also during application running in capture context do have one issue, when i capture YUV application, drawing Red,Blue,Green,White boxes, the output is not correct, it looks the sampler / memory is not correct. ( I see 8 boxes, instead of 4)
I guess this log might be reason.

RDOC 022155: [14:25:15] vk_resource_funcs.cpp(1581) - Debug - External image requires 3110400 bytes at 16 alignment, in 3 memory types
RDOC 022155: [14:25:15] vk_resource_funcs.cpp(1584) - Debug - Non-external version requires 3110400 bytes at 64 alignment, in 3 memory types

Regards,
Harsh

@baldurk
Copy link
Owner

baldurk commented Dec 31, 2018

Yes external memory images should get their contents saved as normal at the start of the frame. Any changes to the image within the frame won't be reflected as there's not a good way to know when the image has been modified outside of the application. If you have a repro where this isn't happening and doesn't use VK_EXT_image_drm_format_modifier, please share it.

The messages are normal, some implementations have stricter memory requirements for non-external images, so I take the worse of the two and return those as the requirements during capture to ensure memory allocated will work on replay.

@szihs
Copy link
Author

szihs commented Jan 2, 2019

Hello, Thanks for your reply.
I exported the RDC file to zip file to view the associated resource from external yuv-viewer application.

However its just Renderoc 2D Image viewer window doesn't show any contents.

@baldurk
Copy link
Owner

baldurk commented Jan 2, 2019

As I said if you have a repro where the YUV texture isn't displayed and it's not using VK_EXT_image_drm_format_modifier then please share it so I can debug it. I have a test program in util/test/ which uses various YUV textures on vulkan and that works OK for me, but it's pretty simplistic and I hadn't seen any other more complex use-cases to test.

@szihs
Copy link
Author

szihs commented Jan 4, 2019

Sorry, it is not easy for me to share code sample due to my employers restrictions / NDA.

Appreciate your help. Renderdoc help me understand lot of issues 👍

I checked your test-program in util/test directory
It looks like multi-planar format (NV21) require multiple that multiple VkImageViews, Correct me if i am wrong here.

In VK_EXT_image_drm_format_modifier sample, it just create a single ImageView.
I am able to add support for the extension and it works well for packed NV21 (2planes)
Capture (on target) and replay (on texture-veiwer) both output correct colors.

  1. Just that 2DImage (Input) doesn't show correctly. Which is okay, sine i can view with external YUV viewer

For NV12 format, 2plane, non-packed (2 buffer objects for each plane).
The capture time, the output works well on target.

However on replay, it tries to bind 2 VkDeviceMemory to vkImage, it fail

RDOC 004604: [05:35:06] vk_resource_funcs.cpp( 231) - Error   - Trying to bind Image 116 to memory 117 which is type 1, but at offset 0x0 the reported size of 0x2f7600 won't fit the 0x1fa400 bytes of memory.
This is most likely caused by incompatible hardware or drivers between capture and replay, causing a change in memory requirements.
RDOC 004604: [05:35:06] vk_resource_funcs.cpp( 231) - Error   - Trying to bind Image 116 to memory 118 which is type 1, but at offset 0x0 the reported size of 0x2f7600 won't fit the 0xfd200 bytes of memory.
This is most likely caused by incompatible hardware or drivers between capture and replay, causing a change in memory requirements.

Above 3110400 is size of 1920x1080 YUV NV12 image, each buffer object individually sized ( plane[0] : 2073600 bytes - plane[1] 1036800 bytes)
Plane 0 + Plane 1 => 3110400 bytes

I am checking how to add support because maybe some information pNext is not serialzed.

I hope it is feasible,
Or is it that we need mulitple vkImageView in this case, to be able to replay non-external memory. ?
Or maybe on replay, change to create vkCreateImage with VK_IMAGE_CREATE_DISJOINT_BIT.

I am not sure yet.

Thanks.

@baldurk
Copy link
Owner

baldurk commented Jan 4, 2019

In my test program yes I create separate image views for planar formats to texelFetch directly, though I do create a couple of samplers with ycbcr conversion enabled to test single-view conversions of planar/packed images.

I think the image views is unrelated to what you're talking about as that's separate from memory binding, the problem seems to be:

2plane, non-packed (2 buffer objects for each plane).
However on replay, it tries to bind 2 VkDeviceMemory to vkImage, it fail

RenderDoc removes the VK_FORMAT_FEATURE_DISJOINT_BIT from the results of vkGetPhysicalDeviceFormatProperties or vkGetPhysicalDeviceFormatProperties2, indicating that the implementation does not support disjoint memory binding. If an application is still trying to use disjoint binding then it is invalid and breaking the spec, so the problem is with the application.

@szihs
Copy link
Author

szihs commented Jan 4, 2019

I was not clear about case when 2 plane , 2 buffer object are present.

In this case VkImageCreateInfo is called with VkExternalMemoryImageCreateInfoKHR structure in pNext. The struct VkExternalMemoryImageCreateInfoKHR pNext internally contains VkImageDrmFormatModifierExplicitCreateInfoEXT having information about number of planes and layout for each plane.

vkCreateImage returned VkImage handle would have this 2 plane, information about two buffer obj.
vkAllocateMemory is called twice with for those 2 buffer obj, and vkBindImageMemory2() defines the backing memory for those 2 buffer obj.

However, when replay, we only use non-external memory ( right ? ).
The replayed version of vkCreateImage would not have passed planar information to ICD.

I guess it is removed from below codes

WrappedVulkan::vkCreateImage
removed |= RemoveNextStruct(&createInfo_adjusted,
                                 VK_STRUCTURE_TYPE_EXTERNAL_MEMORY_IMAGE_CREATE_INFO);

So created handle would not know about how memory is being bound in this case.

When vkBindImageMemory2 is replayed, renderdoc check sizes of image by calling GetRequirement(), however vkImage is not created with external image / plane information, so it just return total size.

The check inside BindImageMemory2() fail with below error. (Trying to bind buffer 1 and buffer 2 to same image. with offset, size info from VkBindImagePlaneMemoryInfoKHR)

RDOC 004604: [05:35:06] vk_resource_funcs.cpp( 231) - Error   - Trying to bind Image 116 to memory 117 which is type 1, but at offset 0x0 the reported size of 0x2f7600 won't fit the 0x1fa400 bytes of memory.
This is most likely caused by incompatible hardware or drivers between capture and replay, causing a change in memory requirements.
RDOC 004604: [05:35:06] vk_resource_funcs.cpp( 231) - Error   - Trying to bind Image 116 to memory 118 which is type 1, but at offset 0x0 the reported size of 0x2f7600 won't fit the 0xfd200 bytes of memory.
This is most likely caused by incompatible hardware or drivers between capture and replay, causing a change in memory requirements.

The replay would have succeded if created VkImage had plane information somehow!

The only way i think to be albe to replay, without using external memory, is to hijack replay to call vkCreateImage with VK_FORMAT_FEATURE_DISJOINT_BIT in this case.

Application never create VkImage with VK_FORMAT_FEATURE_DISJOINT_BIT.
But how to bind correctly 2 buffer object to a single mulitiplanar VkImage,
(which would then have a corresponding single VkImageView and a corresponding YCbCr sampler.)

Please let me know your view.

@baldurk
Copy link
Owner

baldurk commented Jan 4, 2019

I double-checked for clarification, nothing about the VK_EXT_image_drm_format_modifier requires disjoint memory binding. The memory planes you mentioned are just the planar data in the image, it doesn't mean they must be bound separately.

With the extension it's the same as without the extension and with plain base ycbcr support: if you want to bind DISJOINT, you must check for VK_FORMAT_FEATURE_DISJOINT_BIT support and then specify VK_IMAGE_CREATE_DISJOINT_BIT when creating the image. Creating an image without that bit and then trying to bind multiple memory objects is invalid and the validation layers should catch this.

With RenderDoc active, the disjoint feature will be disabled even if the driver supports it, so the application must do a single memory bind.

@szihs
Copy link
Author

szihs commented Jan 4, 2019

I think you misunderstood.
Application source doesn't call VK_FORMAT_FEATURE_DISJOINT_BIT and i don't intend to use application with that bit.

However my query is:
Actual case, application can pass planar information using VK_EXT_image_drm_format_modifier . while creating a VkImage.

No mention of DISJOINT.
It call vkBindImageMemory2 to bind multiple buffer objects, 2 in my case to the same single created VkImage.

This all work fine, The capture is done, with application running with RenderDoc_Capture layer enabled.

When I replay context is the issue, now the application is renderdoccmd.
While under replay, it tries to bind 2 memory to a vkImage returned from vkCreateImage, sequence from rdc file. except planar info is note present while creating in replay context.
This image is however devoid of any planar info,

The replayer would replay vkBindImageMemory2 with 2 buffer object, and with single vkImage
but will however fail because of

@@ -1909,8 +1917,10 @@ bool WrappedVulkan::Serialise_vkBindImageMemory2(SerialiserType &ser, VkDevice d
       bool ok = CheckMemoryRequirements(StringFormat::Fmt("Image %llu", resOrigId).c_str(),
                                         GetResID(bindInfo.memory), bindInfo.memoryOffset, mrq);
 

       if(!ok)
         return false;

DOC 004604: [05:35:06] vk_resource_funcs.cpp( 231) - Error   - Trying to bind **Image 116** to memory 117 which is type 1, but at offset 0x0 the reported size of 0x2f7600 won't fit the 0x1fa400 bytes of memory.
This is most likely caused by incompatible hardware or drivers between capture and replay, causing a change in memory requirements.
RDOC 004604: [05:35:06] vk_resource_funcs.cpp( 231) - Error   - Trying to bind **Image 116** to memory 118 which is type 1, but at offset 0x0 the reported size of 0x2f7600 won't fit the 0xfd200 bytes of memory.
This is most likely caused by incompatible hardware or drivers between capture and replay, causing a change in memory requirements.

How to make replay success is my query ?

@baldurk
Copy link
Owner

baldurk commented Jan 4, 2019

I don't think I've misunderstood, I see the problem you're running into but I'm saying that it's caused by the application behaving in an invalid way.

The spec says that if you are going to bind a single image to multiple memory objects it must be a disjoint image. From the spec Valid Usage on VkBindImageMemoryInfo:

If the pNext chain includes an instance of the VkBindImagePlaneMemoryInfo structure, image must have been created with the VK_IMAGE_CREATE_DISJOINT_BIT bit set.

To create an image with that bit you must verify that the format you're creating supports VK_FORMAT_FEATURE_DISJOINT_BIT. Thus with RenderDoc active the format feature is not available, so you can't create an image as disjoint, so you can't bind multiple memory objects. From what you're describing the application is doing this anyway without properly checking which is invalid behaviour. It's then expected that things will break.

None of that is implicit or required by the drm modifier extension, it's perfectly OK to use the drm modifier with a non-disjoint image with only one memory binding.

@szihs
Copy link
Author

szihs commented Jan 4, 2019

Oh I see, i disabled validation layer for specifically vkCreateImage function specifically to avoid some crash,
for it didn't support the extension well.
KhronosGroup/Vulkan-ValidationLayers#456

Okay, so i understand that VK_FORMAT_FEATURE_DISJOINT_BIT for any format is not available in renderdoc context,
So i can only use single planar and modify application code 👍

Thank you so much for your informative answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature An improvement or feature Unresolved Waiting for a fix or implementation
Projects
None yet
Development

No branches or pull requests

2 participants