Rewrite Buffer CPU-mapping API to lift vertex buffer creation into the common API layer#1113
Conversation
| // Maps the buffer's memory for host access. Only valid for CpuToGpu and | ||
| // GpuToCpu buffers; returns an error for GpuOnly. Each successful map() must | ||
| // be paired with a call to unmap(). | ||
| virtual llvm::Expected<void *> map() = 0; | ||
| virtual llvm::Error unmap() = 0; |
There was a problem hiding this comment.
I think this is a fine change for now, as the framework already maps and unmaps buffers. However, the modern APIs allow for persistently mapped resources and I think we should embrace those as well. Although I think that's better for a follow-up PR once the abstraction layer is in a more complete state.
EmilioLaiso
changed the title
EmilioLaiso
force-pushed
the
render-backend-api-vertex-buffer
branch
from
9090b1b to
a4c884c
Compare
MarijnS95
left a comment
MarijnS95
left a comment
There was a problem hiding this comment.
Not a fan -at all- of the CpuToGpu and related enums with UMA drastically increasing in popularity, but that's out of context for this PR even if affecting it significantly.
Secondly I don't think the title does this PR justice, you're factoring out the (somewhat unsafe) (un)map() APIs to be able to copy in and out of buffers on the CPU, and only as a small consequence of that are able to able to generalize vertex buffer creation which maps the buffer to write into it. This PR doesn't seem to update functions like readBackData() to use the new mapping API (and demonstrate its usefulness)?
| // Managed storage (CpuToGpu) requires an explicit didModifyRange to | ||
| // propagate CPU-side writes to the GPU. Shared storage (GpuToCpu) is | ||
| // coherent and needs no action. | ||
| if (Desc.Location == MemoryLocation::CpuToGpu) | ||
| Buf->didModifyRange(NS::Range::Make(0, SizeInBytes)); |
There was a problem hiding this comment.
This is painful, it means CpuToGpu is not host coherent on Metal but it is mapped as such on Vulkan.
There was a problem hiding this comment.
I'm also curious if Metal forces/resets this managed mode request back to shared, because that's not available outside of Intel-based Macs: https://developer.apple.com/documentation/metal/mtlstoragemode/managed?language=objc#discussion
MarijnS95
changed the title
Buffer CPU-mapping API to lift vertex buffer creation into the common API layer
Add `VertexBuffers` parsing to pipeline yaml use new parsed vtx buffers in backends rebase fixups fixup vertex buffer definition in new sampletexture2d test migrate new tests to new VB yaml definition createVertexBuffer in api-agnostic device rebase fixup: `getFormatSize` -> `getFormatSizeInBytes` rename fmt const remove VertexBuffer abstraction. Rely on ParsedVertexBuffer for input layout and buffer metadata revert test VB definition changes
EmilioLaiso
force-pushed
the
render-backend-api-vertex-buffer
branch
from
ae341ed to
d620b0c
Compare
4 participants
Lift vertex buffer creation into the common API layer and unifies it across the DX, Vulkan, and Metal backends.
BufferAPIgetSizeInBytes(),map(),unmap()toBuffer.BufferUsage { Storage, VertexBuffer }onBufferCreateDesc.createVertexBufferFromCPUBuffer(Device&, const CPUBuffer&)DirectX
HEAP_TYPE_DEFAULTgetsALLOW_UNORDERED_ACCESS