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

Add spec for VK_QNX_external_memory_screen_buffer #2138

Merged
merged 6 commits into from
Jun 15, 2023

Conversation

aruby-blackberry
Copy link
Contributor

Following the general format of VK_ANDROID_external_memory_android_hardware_buffer, this extension is to enable an application to import QNX Screen buffer objects created outside of the Vulkan device into Vulkan memory objects.

  • VUIDs: it's my understanding from the docs that these will be added by the maintainer in the internal repo? So I did not add any "[[VUID-" .... blocks to the ".Valid Usage" sections that I added, I'm assuming this is alright.

  • VulkanSC: We need to add this extension to VulkanSC-Docs as well. What are the steps required to push the equivalent changes to VulkanSC-Docs? I assume I'm going to have to add changes manually and create a separate Pull Request? Is there any internal synchronization between the XMLs?

Testing:

  • ./makeSpec -clean -spec all -v alldocs

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2023

CLA assistant check
All committers have signed the CLA.

@oddhack
Copy link
Contributor

oddhack commented Jun 3, 2023

Following the general format of VK_ANDROID_external_memory_android_hardware_buffer, this extension is to enable an application to import QNX Screen buffer objects created outside of the Vulkan device into Vulkan memory objects.

  • VUIDs: it's my understanding from the docs that these will be added by the maintainer in the internal repo? So I did not add any "[[VUID-" .... blocks to the ".Valid Usage" sections that I added, I'm assuming this is alright.

That's right.

  • VulkanSC: We need to add this extension to VulkanSC-Docs as well. What are the steps required to push the equivalent changes to VulkanSC-Docs? I assume I'm going to have to add changes manually and create a separate Pull Request? Is there any internal synchronization between the XMLs?

We keep the repos loosely synchronized (and are trying to get a little tighter). @dgkoch is the best person to help you out with this.

Testing:

  • ./makeSpec -clean -spec all -v alldocs

I enabled CI for this branch, which runs some markup tests and several different builds. There will be some issues to work through there which I'll comment on further once CI completes.

@oddhack
Copy link
Contributor

oddhack commented Jun 3, 2023

Skimming the CI output, it looks like there's a missing license statement on one new file, and an API which should be documented, but isn't. Suggest fixing those (scroll down to the end of each failing CI task to get a little more information) and trying again.

FYI the way vendor extensions are normally handled, once they are structurally good and passing CI, is to bring them up on the internal maintenance call in case anyone else wants to review or is possibly interested in promoting the extension to EXT status (which probably not in this case, since it's tied to a QNX primitive). Once we've done that, there should be no further obstacles to merging and publishing.

@dgkoch dgkoch self-requested a review June 5, 2023 13:23
@aruby-blackberry
Copy link
Contributor Author

Thanks for the help, @oddhack !

Skimming the CI output, it looks like there's a missing license statement on one new file, and an API which should be documented, but isn't. Suggest fixing those (scroll down to the end of each failing CI task to get a little more information) and trying again.

Attempting fix. Will let CI run.

FYI the way vendor extensions are normally handled, once they are structurally good and passing CI, is to bring them up on the internal maintenance call in case anyone else wants to review or is possibly interested in promoting the extension to EXT status (which probably not in this case, since it's tied to a QNX primitive). Once we've done that, there should be no further obstacles to merging and publishing.

Thank you for the additional information. Sounds good, hoping to get this merged and published as soon as possible for next spec releases (also for VulkanSC).

@dgkoch
Copy link
Contributor

dgkoch commented Jun 5, 2023

For the Vulkan SC version, the most expedient way is to just include it here (now that we've merged the Vulkan SC sources into this repo as of last week), and make sure both VK and VKSC versions build and look correct (I can help with that) and we'll pick it up in the next sync (which we are about to initiate). However, if you need it published in an official VKSC spec sooner rather that later - we might need to discuss that offline.

@aruby-blackberry
Copy link
Contributor Author

For the Vulkan SC version, the most expedient way is to just include it here (now that we've merged the Vulkan SC sources into this repo as of last week), and make sure both VK and VKSC versions build and look correct (I can help with that) and we'll pick it up in the next sync (which we are about to initiate). However, if you need it published in an official VKSC spec sooner rather that later - we might need to discuss that offline.

Thanks @dgkoch . We would like to target the next VKSC spec. So, I think doing it in this repo with the next sync will work fine. Can you help me out on how to build SC spec from this repo?

@aruby-blackberry
Copy link
Contributor Author

@oddhack , do I need to worry about the errors in the Vulkan-CTS or ash-rs workflows?

@dgkoch
Copy link
Contributor

dgkoch commented Jun 5, 2023

Can you help me out on how to build SC spec from this repo?

Sure - see https://github.com/KhronosGroup/Vulkan-Docs/blob/main/BUILD.adoc#building-vulkansc
Let me know if you have any issues.

@aruby-blackberry
Copy link
Contributor Author

Sure - see https://github.com/KhronosGroup/Vulkan-Docs/blob/main/BUILD.adoc#building-vulkansc
Let me know if you have any issues.

Thanks @dgkoch . With some updates, I was able to build vulkansc spec with new extension included, and verified the the docs / spec looks correct.

@oddhack
Copy link
Contributor

oddhack commented Jun 6, 2023

@oddhack , do I need to worry about the errors in the Vulkan-CTS or ash-rs workflows?

For ash-rs I would ask @MarijnS95 to take a look at it (possibly by raising an issue on that repo). We pulled that test in to try and detect problems early, but I literally know nothing about the Rust bindings.

The CTS one may be due to a change they made recently; I think I've seen the same error in other branches in internal gitlab and will check on that.

From a spec POV they aren't worrisome, but it can cause problems to release a new extension spec and find it's breaking immediately downstream consumers like CTS or language bindings, so we like to get those cleared up first.

@MarijnS95
Copy link
Contributor

MarijnS95 commented Jun 6, 2023

For ash-rs I would ask @MarijnS95 to take a look at it (possibly by raising an issue on that repo). We pulled that test in to try and detect problems early, but I literally know nothing about the Rust bindings.

We have to hardcode a (likely *mut c_void or c_void to match the other _screen_{context,window} types) type for _screen_buffer, which is a new "native" type added in this PR (<type requires="screen/screen.h" name="_screen_buffer"/>) but relying on external C headers to provide or mock it. We don't generate those but define them by hand.

I typically add those when landing the PR upgrading to a newer Vulkan spec version, so it should be fine to ignore the CI errors for now (they have not been made required for now) until that happens, but I could also retroactively push it to main now if that helps your case.

@oddhack
Copy link
Contributor

oddhack commented Jun 6, 2023

@oddhack , do I need to worry about the errors in the Vulkan-CTS or ash-rs workflows?

The CTS stage is failing in many other branches in our internal gitlab as well - something changed in that repo recently which is breaking the CI check. So don't worry about that.

@aruby-blackberry
Copy link
Contributor Author

Thanks @MarijnS95 and @oddhack . Sounds like it will be fine to ignore both CTS failures and ash-rs CI runs for now. This Pull Request stands as is, then.

Testing:
VULKAN_API="vulkan" ./makeAllExts -v html
VULKAN_API="vulkansc" ./makeAllExts -version sc1.0 html
@oddhack
Copy link
Contributor

oddhack commented Jun 8, 2023

We brought this extension up briefly on the maintenance call yesterday and asked potentially interested parties to review, but as expected nobody else is going to implement it and there may not be any internal reviews. So when you've addressed the outstanding feedback and are happy with this, you can let me know and I'll include it in the next spec update.

appendices/glossary.adoc Show resolved Hide resolved
chapters/capabilities.adoc Show resolved Hide resolved
chapters/resources.adoc Show resolved Hide resolved
chapters/resources.adoc Show resolved Hide resolved
chapters/resources.adoc Outdated Show resolved Hide resolved
xml/vk.xml Show resolved Hide resolved
@enpengxu
Copy link

changes LGTM, thanks

xml/vk.xml Show resolved Hide resolved
xml/vk.xml Show resolved Hide resolved
to the VK_QNX_external_memory_screen_buffer extension spec additions
Copy link
Contributor

@dgkoch dgkoch left a comment

Choose a reason for hiding this comment

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

LGTM for Vulkan and Vulkan SC.

@aruby-blackberry
Copy link
Contributor Author

Thanks @dgkoch . @oddhack I think we are good to go, all outstanding feedback has been addressed. Thank you!

@oddhack oddhack added this to the Signed-off to Merge milestone Jun 14, 2023
@oddhack oddhack merged commit b5543e9 into KhronosGroup:main Jun 15, 2023
@MarijnS95
Copy link
Contributor

Pushed the new type to ash-rs/ash#760 now that spec 1.3.254 is has been released. Will merge this to master ASAP so that the CI turns green again everywhere.

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.

7 participants