-
Notifications
You must be signed in to change notification settings - Fork 467
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 a new extension: VK_EXT_map_memory_placed #1906
Conversation
617c8de
to
57c719a
Compare
92b94ac
to
6a880b8
Compare
@jekstrand can you add a proposal document for this extension? |
Will be handled internally by the Development call - assigning to Tom as a placeholder for that. |
6a880b8
to
7d89dc2
Compare
I've updated the PR as well as all the driver/test branches with the following changes:
|
7d89dc2
to
c3a24fc
Compare
78c80d2
to
5f95682
Compare
5f95682
to
858f08c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I feel like you'll anticipate what I'm going to say here but, can you write a proposal doc for map memory 2 as well please 😅
It seems like a straightforward extension, but it's not clear why we need it without the motivation written down.
I suppose you could also combine the two extensions with a single proposal document if you'd prefer, but that might be more work.
4a07b8a
to
3ff75cf
Compare
84e5dc1
to
945f3c1
Compare
While dealing with the comments from Tobias, I realized I didn't specify interactions with host pointer import. I've added a couple VUs saying that you can't do a placed maps or unmap with reserve on something imported from a host pointer import. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this is my most thorough review yet, so I'm fairly sure once all of this is addressed you can finally have my approval :P
945f3c1
to
90e0777
Compare
Now that VK_KHR_map_memory2 is ratified and released, I've rebased this branch and fixed up a bunch of stuff. The ANV branch has also been updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still says WIP, but this LGTM.
Dropped the WIP. We're currently waiting on someone to write some CTS tests. I put together a test plan but have yet to find someone to write the actual tests. |
56e52dc
to
f769f72
Compare
There is an initial implementation of this extension in our NVIDIA Vulkan Beta drivers here: https://developer.nvidia.com/vulkan-driver |
f769f72
to
e8ff0cd
Compare
@oddhack, I just rebased the branch today and we agreed on the working group call to skip some of the usual process and go ahead and ship this. Do you want to merge it from here or would you like me to create an internal MR and merge it that way? Also, this is pretty short notice so it's okay if we miss this week's spec update. |
And... we now have an NVK implementation: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27599 |
@oddhack To be clear, this is good to ship as soon as you're ready. There's no need to delay until next week. I just didn't want to throw a wrench in your process if you already had other extensions queued up to go in. |
@gfxstrand This is the actual NVK implementation: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27599 (the link you posted is just some non-Vulkan CTS tests for the extension) |
Did I do that here, too? It's been a day... 😂 |
It's fine to merge from here, though I'm glad you're bringing it up early as it's easier to handle the github PRs first and then the internal ones. Should respond to @Tobski's suggestion but I'll sign this off and it will go out this week. |
e8ff0cd
to
5f08140
Compare
I've dealt with the comments from @Tobski and @spencer-lunarg. I also fixed a dependency warning. |
5f08140
to
a44b02d
Compare
Unfortunately, my fix for the dependency warning caused a new CI issue. It's complaining that I put the requires for I've moved it back to VK_KHR_map_memory2 |
a44b02d
to
0539443
Compare
It looks like you squashed away the offending commit? If you happen to recall the diff I'd like to take a look at it for future extensions but it appears this is working OK now. |
@gfxstrand this is saying something about @Tobski requested changes though I can't figure out what was requested that you haven't already addressed. Maybe git is confused? Also there's the suggestion from @DadSchoorse above. So I will hold off on merging until tomorrow and we can check back on the call. |
I'm pretty sure I just asked for a proposal, which has now been done. Will review again and clear this up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nitpicky editorial things that I'd like to see fixed but it's nothing I feel needs to be resolved before merging.
concept of when a memory object is or is not mapped. | ||
|
||
3) Should a placed memory map replace existing maps in the specified | ||
range or fail if a map exists? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've generally been dropping them from the appendix rather than the proposal - reason being the questions usually come up when you read the proposal, not the appendix. Suggest dropping them here? Not going to gate merging on this but it is weird to have them duped...
0539443
to
6165f6f
Compare
This extension adds support for requesting that a
VkDeviceMemory
object be mapped at a particular location in the client's address space. This is similar to theaddr
parameter to themmap()
ioctl. In order for this to succeed, the client may have to first reserve the address range via a platform-specific mechanism.Fixes: #1832
External links