Device memory mapping issues

[philiptaylor]

Section 10.2.1 (Host Access to Device Memory Objects) says:

[... vkMapMemory ...]

• offset must be less than the size of memory
• If size is not equal to VK_WHOLE_SIZE, the sum of offset and size must be less than or equal to the size of the memory

There's nothing that says size must be greater than zero, so e.g. { size = 0, offset = size_of_memory - 1 } is perfectly fine. But { size = 0, offset = size_of_memory } fails the first condition, yet passes the second condition.

Is that intentional?

Most Vulkan commands seem to forbid sizes of zero, e.g. vkAllocateMemory requires allocationSize > 0. mmap() also fails if len is zero (http://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html).

If size is allowed to be zero, the restriction on offset seems weird to me. It's inconsistent with e.g. pointers in C, which are permitted to point to the element just after the end of the array (as long as you don't dereference more than 0 bytes of them).

If size isn't allowed to be zero, the "offset must be less than the size of memory" requirement seems entirely redundant with the next requirement.

(I'm assuming that "sum of offset and size" means a proper mathematical sum without any integer overflow. If it didn't mean that, and instead meant a VkDeviceSize-sized C addition with overflow, then { offset = 10, size = (VkDeviceSize)-10 } would be accepted, which would be silly, so presumably it's not that.)

the application must round down the start of the sub-range to the previous multiple of VkPhysicalDeviceLimits::nonCoherentAtomSize

If nonCoherentAtomSize is 64, and I call vkMapAllocate with offset=0, this is saying I have to round down the sub-range offset to -64 (since that's the "previous multiple" of 64), which is nonsensical.

I think this should say "round down ... to the nearest multiple of nonCoherentAtomSize" (similarly to how it says "round the end of the range up to the nearest multiple") (with the implicit understanding that it means "nearest in the direction of rounding").

sub-range

The rest of the section talks about a "range", not a "sub-range". If there is no technical difference then they should use the same term.

[... vkFlushMappedMemoryRanges ...]

• The memory ranges specified by pMemoryRanges must all currently be mapped

This doesn't account for the rounding to nonCoherentAtomSize that was previously described. I believe the memory between round_down(vkMapMemory.offset, nonCoherentAtomSize) and vkMapMemory.offset does not count as mapped, so this requirement forbids it being passed to vkFlushMappedMemoryRanges.

• The memory ranges specified by pMemoryRanges must all currently be mapped

If I map a 64KB range, then want to flush the first 4KB of it, is that allowed? In one interpretation, I never mapped a 4KB memory range so I'm not allowed to flush a 4KB memory range. In another interpretation, that section of the memory object is mapped (as part of a larger section) so I am allowed to flush it.

I don't think the spec is clear about whether a "memory range" is a discrete thing with an offset/size, with a direct correspondence to VkMappedMemoryRange, or is a sequence of bytes that might happen to share the property of being mapped.

(If it's a discrete thing, and you can only flush exactly the range mapped by vkMapMemory, then it's impossible to flush a range that's not aligned to nonCoherentAtomSize.)

[... VkMappedMemoryRange ...]

• offset is the zero-based byte offset from the beginning of the memory object.
• [...]
• offset must be less than the size of the currently mapped range of memory
• If size is not equal to VK_WHOLE_SIZE, the sum of offset and size must be less than or equal to the size of the currently mapped range of memory

That means that if I map with { size = 64, offset = 4096 }, it's impossible to ever flush/invalidate that range. I'd have to set VkMappedMemoryRange.offset = 4096 to reach the mapped range, but 4096 > 64 so it violates the first of those requirements (and also violates the second unless I flush VK_WHOLE_SIZE).

[szdarkhack]

I think there should also be some explicit mention of whether vkUnmapMemory performs an implicit flush or not, there is nothing in the specification clarifying it (I've looked throughout the entire 10.2.1 section as well as the description of the VkMemoryPropertyFlagBits flags in section 10.2, and text-searching "unmap" doesn't bring up any other hits).

On the other hand, this (obviously outdated, but look at the description for VK_MEMORY_PROPERTY_HOST_NON_COHERENT_BIT) seems to imply that unmapping does flush non coherent memory.

I think a clarification should be added in the description of vkUnmapMemory.

[jeffbolznv]

We haven't done a thorough job of disallowing size=0, but I think forbidding it is the right thing to do in most cases. It removes a corner case that isn't useful, and more often than not would indicate an app bug. While the comparison to C pointer arithmetic is interesting, I don't think we need to follow that convention because we're not talking about doing arithmetic on the offset.

IMO the rules should be interpreted to be defined using VkDeviceSize-sized (or whatever type is appropriate for the command) integer math, so each rule can be directly translated into code. There are obviously some holes here (as you also point out in #31), but the intent is that offset must be less than the size of the memory/object, and the range must be less than or equal to size of the memory/object minus the offset.

For the VkMappedMemoryRange questions, the intent is that a VkMappedMemoryRange structure describes a subset of the currently mapped portion of the memory object, not that it necessarily describes the entire mapped portion. The validity language is really broken here - it twice implies that the offset is relative to the mapping when the spec clearly states it is relative to the beginning of the memory object. The first case is as you describe (offset + size <= size of mapped range), the second is the requirement that offset < size of mapped range.

[philiptaylor]

Consistently disallowing zero-sized things sounds okay to me. (I don't really care about the specific behaviour, I just get bothered by inconsistency :-) )

the intent is that offset must be less than the size of the memory/object, and the range must be less than or equal to size of the memory/object minus the offset.

If it's phrased in that way then integer overflow won't matter (I think), so I'd be happy with that.

[oddhack]

Done in the 1.0.5 spec update.