Closed 0xcaff closed 2 months ago
The idea with non-exhaustive structs is that we can add fields without it being a breaking change.
Can you please fix the string literals spanning more than 100 columns? You can see examples of how to do it in the same file.
I've updated the interface to no longer use DeviceSize::MAX
I've updated the PR but seems like the windows actions has crashed and its blocking the check from completing.
ok, I've incorporated all the suggestions
My last gripe is that there are two code paths that both call map_memory_khr
. Reason being that when new extensions to MemoryMapInfoKHR
get added, this is going to get out of hand quick. I suggest instead doing this unconditionally:
if offset + size == self.allocation_size() {
size = ash::vk::WHOLE_SIZE;
}
As this cannot change the behavior in any case. Then the two code paths will become unnecessary. Notice also the use of the constant instead of the magic value.
Also, preferably with a comment like:
// HACK: ext_map_memory_placed only accepts `VK_WHOLE_SIZE` unlike the rest of the API.
// See https://github.com/KhronosGroup/Vulkan-Docs/issues/2350
I agree, not a fan of the branch https://github.com/vulkano-rs/vulkano/blob/5b7618fc483b3b4150c6e3eba5a488c57092b71c/vulkano/src/memory/device_memory.rs#L480-L526 either.
That said, wouldn't what you proposed here https://github.com/vulkano-rs/vulkano/pull/2514#issuecomment-2052267894 change the behavior of callers which pass currently pass allocation size? I'm concerned this might break compatibility for something which relies on the alternative way (allows allocation size but not VK_WHOLE_SIZE
). Are you sure this is never the case?
If we're not sure, maybe its prudent to go down the route of having a separate map_placed function for now where we only do this remapping.
Yes I am sure, either VK_WHOLE_SIZE
or the rest of the allocation size is permitted in all other cases. You can keep the condition with !device.enabled_features().map_memory_range_placed
too if you prefer.
I guess that is also more self-documenting, true that.
Updated to remove the extra branch. Kept the feature gate.
I did do
if size == self.allocation_size() {
size = ash::vk::WHOLE_SIZE;
}
instead of
if offset + size == self.allocation_size() {
size = ash::vk::WHOLE_SIZE;
}
as a non-zero offset
is invalid when features.memory_map_range_placed
is not available.
I'm afraid that that condition is incorrect, because those sets of features can be en/disabled without using MemoryMapFlags::PLACED
. So either it needs to be what I suggested, or you have to check for the flag instead of those features.
Err, more specifically, you would have to check that memory_map_range_placed
is disabled and MemoryMapFlags::PLACED
is enabled. (You can't enable the flag without memory_map_placed
anyway.)
Yes, I believe this is correct. Previously, placed_address implied the flag and memory_map_placed
, now the flag implies memory_map_placed
and placed_address
implies nothing. The feature is a global thing and the flag is call specific.
That made me realize, VK_WHOLE_SIZE
is the only acceptable value both when map_memory_range_placed
is enabled and disabled. So really the check needs to be what I sent initially because of VUID-VkMemoryMapInfoKHR-flags-09574:
flags.contains(MemoryMapFlags::PLACED) && offset + size == self.allocation_size()
What if map_memory_range_placed
is enabled and you want to pass allocation_size
instead of VK_WHOLE_SIZE
? I think its a good idea to avoid transforming this value unless it is invalid. allocation_size
might be sometimes valid for map_memory_range_placed
but is never valid for map_memory_range_placed
disabled.
allocation_size
is never valid regardless of whether memory_map_range_placed
is enabled or not (except by chance). That's why we need to translate the rest of the size to VK_WHOLE_SIZE
in all cases.
In case you are talking about the case when allocation_size
is valid by chance when map_memory_range_placed
is enabled, it still wouldn't change functionality. Let's not overcomplicate this any more than it already is. You don't even need to check for the flag like I mentioned because this is always a valid transformation to make, at least currently.
Ultimately I've concluded this decision is an opinion and I care more about merging than having a debate. Updated to exactly what you asked for.
If there are further minor changes, the branch has maintainer edits on, please feel free to edit directly.
Really curious what the docs ppl end up concluding.
Yes, I can't wait to see what will come of it. My gut tells me that they'll make it consistent. Worst case is they refuse and we have to expose VK_WHOLE_SIZE
including the inconsistencies.
I updated the PR in light of the latest updates to the spec and fixed the last remaining unresolved comment since you seemed a bit defeated. I opted for the additional map_placed
function as much as I hate it because we overlooked the most obvious: it is (very) unsafe.
Ah hey yeah @marc0246 no worries, honestly I tried the copy way you suggested and was finding the performance to be acceptable. I decided to focus on correctness for now in my emulation project and worry about performance later. I was planning to come back and update this eventually but it kept getting pushed below the cutline.
If this is important to you I'm happy for you to take it over. If its not, I'll get to it eventually.
Could you merge in the changes from the latest master? I'm encountering a bug that you fixed in another PR, and that's preventing me from testing this currently.
Thank you! Sorry it took so long.
No worries! Thank you!
Overview
Implements the memoryMapPlaced and memoryMapRangePlaced features of the
VK_EXT_map_memory_placed
extension (docs, proposal).Adds a field,
MemoryMapInfo.placed_address
for use withDeviceMemory::map
andDeviceMemory::map_unchecked
specifying an address.Testing
As
VK_EXT_map_memory_placed
is currently not yet released in any implementation, I've tested this using a trunk revision of mesa (https://gitlab.freedesktop.org/mesa/mesa/-/commit/6b383ca810bb853f11c127d9e1d284dc4dcb0992). Mesa 24.1 coming out in the beginning of May is scheduled to include this extension.Here are the instructions I used to run the tests locally. These will not make global system changes.
Questions
gfx_dev_and_queue
will early return, silently passing if a feature or extension requested is not available.Would adding a field to a struct (in this caseThe non-exhaustive allows for non-breaking change.MemoryMapInfo.placed_address
) be considered a breaking change? Wasn't sure so I erred on the conservative side and am assuming yes.Am I using the right vuids? See the inline comments for specifics.Discussing in inline comments.Changelog