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

Import image from dma_buf following VK_EXT_external_memory_dma_buf #2145

Merged
merged 13 commits into from
Apr 1, 2023

Conversation

spaghetti-stack
Copy link
Contributor

  1. Update documentation to reflect any user-facing changes - in this repository.

  2. Make sure that the changes are covered by unit-tests.

  3. Run cargo fmt on the changes.

  4. Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    file by maintainers right after the Pull Request merge.

    Please remove any items from the template below that are not applicable.

  5. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects:

Implements importing an image into Vulkan from a Linux dma_buf,
according to the following Vulkan extensions:

This functionality is already part of Ash and the Vulkan specification, so not many changes were necessary.
The main change was a new constructor to StorageImage that creates an image from a list of Unix file descriptors, a DRM_FORMAT_MODIFIER and some other parameters. This only adds support for importing single-planar images (those backed by a single file descriptor), despite the specification supporting multiple file descriptors.

Changelog:

### Public dependency updates
None
 
### Breaking changes
None

### Additions
- Implements importing an image into Vulkan from a Linux dma_buf, following the `VK_EXT_external_memory_dma_buf` extension.

### Bugs fixed
None

@Rua
Copy link
Contributor

Rua commented Mar 3, 2023

Can you add the appropriate validation for any VUIDs that rely on the tiling of the image? Both for VkImageCreateInfo and elsewhere.

The Windows build is also currently failing, that should be fixed.

Adds conditional compilation checks to functionality for importing
vulkan images from a Linux dmabuf, as doing this only makes sense on Linux.
@spaghetti-stack
Copy link
Contributor Author

Can you add the appropriate validation for any VUIDs that rely on the tiling of the image? Both for VkImageCreateInfo and elsewhere.

The Windows build is also currently failing, that should be fixed.

I noticed that on the VUIDs validation on ImageCreateInfo, some checks are done with assert!s, and some are done by returning ImageCreateErrors. Which option should I prefer for this?

@Rua
Copy link
Contributor

Rua commented Mar 8, 2023

Using the error is preferred.

@spaghetti-stack
Copy link
Contributor Author

@Rua The Windows check seems to be failing with the following message: This request was automatically failed because there were no enabled runners online to process the request for more than 1 days. I have built vulkano locally on Windows, and it succeeds. Is there something wrong with the workflows or runners?

Also, are there other things I need to address before this can get merged? I added the VUID checking for ImageCreateInfo, except for those that require checking that the pNext chain contains some structs, since I don't think vulkano currently offers a way to do that.

@Rua
Copy link
Contributor

Rua commented Mar 20, 2023

The Windows CI stuff has been intermittent for a while. This isn't caused by your code, so you don't need to worry about it.

There are many other VUIDs throughout the Vulkan specification that reference the tiling of an image. Some examples:

  • VUID-vkGetPhysicalDeviceImageFormatProperties-tiling-02248
  • VUID-VkPhysicalDeviceImageFormatInfo2-tiling-02249
  • VUID-vkGetImageSubresourceLayout-02270

You can get a better idea by searching for VK_IMAGE_TILING_ or VK_EXT_image_drm_format_modifier within the source code of the documentation, which is available at https://github.com/KhronosGroup/Vulkan-Docs.

@spaghetti-stack
Copy link
Contributor Author

How can I validate VUID-vkGetPhysicalDeviceImageFormatProperties-tiling-02248?
If I got it right, I have to add the check to PhysicalDevice::validate_image_format_properties() so that an error is returned when the tiling is VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT, but only when using vkGetPhysicalDeviceImageFormatProperties, and not vkGetPhysicalDeviceImageFormatProperties2.
The problem is that vulkano chooses whether to call vkGetPhysicalDeviceImageFormatProperties or vkGetPhysicalDeviceImageFormatProperties2 after the validation method based on some conditions. I suppose I could just add an if statement in the validation to check the same conditions that decide whether vkGetPhysicalDeviceImageFormatProperties should be called, but this might not be future proof since the conditions could change.

On another note, how can I validate if something is in the vulkan pNext chain?

@Rua
Copy link
Contributor

Rua commented Mar 24, 2023

What you describe is generally how Vulkano validates such things. If the called function is determined by some condition, then that condition is used when validating as well. In this particular case though, I think you can take a shortcut: if image_format_properties_unchecked is called with DrmFormatModifier, and vkGetPhysicalDeviceImageFormatProperties is going to be used, then just return None rather than making the call. Then you don't need to validate it, and the user gets expected results in all cases.

@Rua
Copy link
Contributor

Rua commented Mar 24, 2023

Actually, looking at the Vulkan spec closer, it seems that VUID-vkGetPhysicalDeviceImageFormatProperties-tiling-02248 is related to VUID-VkPhysicalDeviceImageFormatInfo2-tiling-02249. Since the non-2 version of the function has no way to provide pNext structures, it follows that VUID-VkPhysicalDeviceImageFormatInfo2-tiling-02249 can never be fulfilled when calling that function. So there may be no need to validate VUID-vkGetPhysicalDeviceImageFormatProperties-tiling-02248 separately.

Check for
VUID-vkGetPhysicalDeviceImageFormatProperties-tiling-02248, and VUID-VkPhysicalDeviceImageFormatInfo2-tiling-02249
Or explanations of why they cannot yet be added
@spaghetti-stack
Copy link
Contributor Author

I created a list of all VUIDs by searching for VK_IMAGE_TILING_ or VK_EXT_image_drm_format_modifier.
I added all the ones I could (the ones that have a checkbox), but many are for vulkan calls that vulkano does not yet do anywhere in it's source code. Others also pertain muliplanar images imported using a Linux FD, which is something this PR does not currently implement. I have added a check with a panic! to the method I made, which can be removed when that is implemented.

  • VUID-vkGetPhysicalDeviceImageFormatProperties-tiling-02248
  • VUID-VkPhysicalDeviceImageFormatInfo2-tiling-02249
  • VUID-VkPhysicalDeviceImageFormatInfo2-tiling-02313 (not possible)
  • VUID-VkImageCreateInfo-tiling-02261
  • VUID-VkImageCreateInfo-pNext-02262
  • VUID-VkImageCreateInfo-tiling-02353 (not possible)
  • VUID-vkGetImageDrmFormatModifierPropertiesEXT-image-02272 (not possible)
  • VUID-VkImageMemoryRequirementsInfo2-image-02279
    • Vulkano currently has no validation in get_memory_requirements()
    • No VkImagePlaneMemoryRequirementsInfo in get_memory_requirements
    • Therefore setting VK_IMAGE_CREATE_DISJOINT_BIT would not work, but multiplanar image importing is not supported anyways.
  • VUID-VkImageMemoryRequirementsInfo2-image-02280
    • Same as VUID-VkImageMemoryRequirementsInfo2-image-02279
  • VUID-VkDeviceImageMemoryRequirementsKHR-pCreateInfo-06420
    • VkDeviceImageMemoryRequirements not yet used in vulkano
  • VUID-VkImagePlaneMemoryRequirementsInfo-planeAspect-02282
    • Multiplanar/disjoint
  • VUID-VkBindImagePlaneMemoryInfo-planeAspect-02284
    • Muliplanar/disjoint
  • VUID-VkImageSubresourceLayers-aspectMask-02247 (memory planes)
  • VUID-VkClearAttachment-aspectMask-02246 (memory planes)
  • VUID-vkGetImageSubresourceLayout-tiling-02271
    • Hard to do rn with only single-plane images
  • VUID-vkGetImageSubresourceLayout2EXT-tiling-02271
    • Same as previous
  • VUID-VkInputAttachmentAspectReference-aspectMask-02250
    • Only used once, without any other checks?
  • VUID-VkImageCreateInfo-pNext-06746
    • No VkImageCompressionControlEXT in vulkano yet.
  • VUID-VkImageDrmFormatModifierListCreateInfoEXT-pDrmFormatModifiers-02263
    • VkImageDrmFormatModifierListCreateInfoEXT is not used anywhere, only VkImageDrmFormatModifierExplicitCreateInfoEXT
  • VUID-VkImageDrmFormatModifierExplicitCreateInfoEXT-drmFormatModifier-02264
      • Maybe we can check this?
  • VUID-VkImageDrmFormatModifierExplicitCreateInfoEXT-drmFormatModifierPlaneCount-02265
    • Maybe we can check this?
  • VUID-VkImageDrmFormatModifierExplicitCreateInfoEXT-size-02267
  • VUID-VkImageDrmFormatModifierExplicitCreateInfoEXT-arrayPitch-02268
  • VUID-VkImageDrmFormatModifierExplicitCreateInfoEXT-depthPitch-02269
  • VUID-vkGetImageSubresourceLayout-image-07790
    • Done, same as VUID-vkGetImageSubresourceLayout-image-02270
  • VUID-VkImageMemoryRequirementsInfo2-image-01590
    • No multiplanar support at the moment
  • VUID-VkImageMemoryRequirementsInfo2-image-01591
  • VUID-VkDeviceImageMemoryRequirements-pCreateInfo-06776
  • VUID-VkSubpassDescription2-attachment-04563
    • multiple planes

vulkano/src/image/storage.rs Outdated Show resolved Hide resolved
vulkano/src/image/storage.rs Outdated Show resolved Hide resolved
vulkano/src/image/sys.rs Outdated Show resolved Hide resolved
Use lowercase for error, replace panic! with todo!, and make some
comments show up in documentation.
@spaghetti-stack
Copy link
Contributor Author

Is there anything else that needs to be changed or added?

@Rua
Copy link
Contributor

Rua commented Mar 31, 2023

You mention that there is no multiplanar support yet, but Vulkano does have support for multiplanar images. Do you mean multiplanar images specifically in combination with the extensions you're adding?

@Rua Rua merged commit b7ecee3 into vulkano-rs:master Apr 1, 2023
Rua added a commit that referenced this pull request Apr 1, 2023
Rua added a commit that referenced this pull request Apr 1, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
…ulkano-rs#2145)

* Import image from dma_buf

Implements importing an image into Vulkan from a Linux dma_buf,
according to the following Vulkan extensions:
- https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_external_memory_dma_buf.html
- https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_image_drm_format_modifier.html

* Only compile dmabuf image importing on Linux

Adds conditional compilation checks to functionality for importing
vulkan images from a Linux dmabuf, as doing this only makes sense on Linux.

* Add VUID checking for VkImageCreateInfo

* Avoid Linux-only dependencies on other OSs

* Add missing initializer to StorageImage

* Add more VUID validation

Check for
VUID-vkGetPhysicalDeviceImageFormatProperties-tiling-02248, and VUID-VkPhysicalDeviceImageFormatInfo2-tiling-02249

* Add some more VUIDs

Or explanations of why they cannot yet be added

* Small fix

* Add suggested fixes

Use lowercase for error, replace panic! with todo!, and make some
comments show up in documentation.
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 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