From f828ecf4bae0df211d8749429e91416385c45e71 Mon Sep 17 00:00:00 2001 From: Jeroen Bakker Date: Mon, 13 Feb 2023 08:34:19 +0100 Subject: [PATCH] GPU: Use same read back API as SSBOs The GPU module has 2 different styles when reading back data from GPU buffers. The SSBOs used a memcpy to copy the data to a pre-allocated buffer. IndexBuf/VertBuf gave back a driver/platform controlled pointer to the memory. Readback is done for test cases returning mapped pointers is not safe. For this reason we settled on using the same approach as the SSBO. Copy the data to a caller pre-allocated buffer. Reason why this API is currently changed is that the Vulkan API is more strict on mapping/unmapping buffers that can lead to potential issues down the road. Pull Request #104571 --- source/blender/gpu/GPU_index_buffer.h | 8 +++----- source/blender/gpu/GPU_vertex_buffer.h | 9 +++------ source/blender/gpu/intern/gpu_index_buffer.cc | 17 ++--------------- .../gpu/intern/gpu_index_buffer_private.hh | 3 +-- source/blender/gpu/intern/gpu_vertex_buffer.cc | 9 ++------- .../gpu/intern/gpu_vertex_buffer_private.hh | 3 +-- source/blender/gpu/metal/mtl_index_buffer.hh | 2 +- source/blender/gpu/metal/mtl_index_buffer.mm | 9 +++------ source/blender/gpu/metal/mtl_vertex_buffer.hh | 3 +-- source/blender/gpu/metal/mtl_vertex_buffer.mm | 15 +++------------ source/blender/gpu/opengl/gl_index_buffer.cc | 8 ++++---- source/blender/gpu/opengl/gl_index_buffer.hh | 2 +- source/blender/gpu/opengl/gl_vertex_buffer.cc | 12 +++--------- source/blender/gpu/opengl/gl_vertex_buffer.hh | 3 +-- source/blender/gpu/tests/gpu_shader_test.cc | 8 ++++---- source/blender/gpu/vulkan/vk_index_buffer.cc | 3 +-- source/blender/gpu/vulkan/vk_index_buffer.hh | 2 +- source/blender/gpu/vulkan/vk_vertex_buffer.cc | 8 +------- source/blender/gpu/vulkan/vk_vertex_buffer.hh | 3 +-- 19 files changed, 37 insertions(+), 90 deletions(-) diff --git a/source/blender/gpu/GPU_index_buffer.h b/source/blender/gpu/GPU_index_buffer.h index e5fefda527d..9d152b01c8b 100644 --- a/source/blender/gpu/GPU_index_buffer.h +++ b/source/blender/gpu/GPU_index_buffer.h @@ -89,13 +89,11 @@ void GPU_indexbuf_create_subrange_in_place(GPUIndexBuf *elem, uint length); /** - * (Download and) return a pointer containing the data of an index buffer. + * (Download and) fill data with the contents of the index buffer. * - * Note that the returned pointer is still owned by the driver. To get an - * local copy, use `GPU_indexbuf_unmap` after calling `GPU_indexbuf_read`. + * NOTE: caller is responsible to reserve enough memory. */ -const uint32_t *GPU_indexbuf_read(GPUIndexBuf *elem); -uint32_t *GPU_indexbuf_unmap(const GPUIndexBuf *elem, const uint32_t *mapped_buffer); +void GPU_indexbuf_read(GPUIndexBuf *elem, uint32_t *data); void GPU_indexbuf_discard(GPUIndexBuf *elem); diff --git a/source/blender/gpu/GPU_vertex_buffer.h b/source/blender/gpu/GPU_vertex_buffer.h index 979b7cc06cf..5fcb97a2a6b 100644 --- a/source/blender/gpu/GPU_vertex_buffer.h +++ b/source/blender/gpu/GPU_vertex_buffer.h @@ -64,13 +64,10 @@ GPUVertBuf *GPU_vertbuf_create_with_format_ex(const GPUVertFormat *, GPUUsageTyp GPU_vertbuf_create_with_format_ex(format, GPU_USAGE_STATIC) /** - * (Download and) return a pointer containing the data of a vertex buffer. - * - * Note that the returned pointer is still owned by the driver. To get an - * local copy, use `GPU_vertbuf_unmap` after calling `GPU_vertbuf_read`. + * (Download and) fill data with the data from the vertex buffer. + * NOTE: caller is responsible to reserve enough memory of the data parameter. */ -const void *GPU_vertbuf_read(GPUVertBuf *verts); -void *GPU_vertbuf_unmap(const GPUVertBuf *verts, const void *mapped_data); +void GPU_vertbuf_read(GPUVertBuf *verts, void *data); /** Same as discard but does not free. */ void GPU_vertbuf_clear(GPUVertBuf *verts); void GPU_vertbuf_discard(GPUVertBuf *); diff --git a/source/blender/gpu/intern/gpu_index_buffer.cc b/source/blender/gpu/intern/gpu_index_buffer.cc index 99e47f5452a..c5fd6ff2206 100644 --- a/source/blender/gpu/intern/gpu_index_buffer.cc +++ b/source/blender/gpu/intern/gpu_index_buffer.cc @@ -399,14 +399,6 @@ void IndexBuf::squeeze_indices_short(uint min_idx, } } -uint32_t *IndexBuf::unmap(const uint32_t *mapped_memory) const -{ - size_t size = size_get(); - uint32_t *result = static_cast(MEM_mallocN(size, __func__)); - memcpy(result, mapped_memory, size); - return result; -} - } // namespace blender::gpu /** \} */ @@ -456,14 +448,9 @@ void GPU_indexbuf_create_subrange_in_place(GPUIndexBuf *elem, unwrap(elem)->init_subrange(unwrap(elem_src), start, length); } -const uint32_t *GPU_indexbuf_read(GPUIndexBuf *elem) +void GPU_indexbuf_read(GPUIndexBuf *elem, uint32_t *data) { - return unwrap(elem)->read(); -} - -uint32_t *GPU_indexbuf_unmap(const GPUIndexBuf *elem, const uint32_t *mapped_buffer) -{ - return unwrap(elem)->unmap(mapped_buffer); + return unwrap(elem)->read(data); } void GPU_indexbuf_discard(GPUIndexBuf *elem) diff --git a/source/blender/gpu/intern/gpu_index_buffer_private.hh b/source/blender/gpu/intern/gpu_index_buffer_private.hh index cb55045d775..8689d805f23 100644 --- a/source/blender/gpu/intern/gpu_index_buffer_private.hh +++ b/source/blender/gpu/intern/gpu_index_buffer_private.hh @@ -98,8 +98,7 @@ class IndexBuf { virtual void bind_as_ssbo(uint binding) = 0; - virtual const uint32_t *read() const = 0; - uint32_t *unmap(const uint32_t *mapped_memory) const; + virtual void read(uint32_t *data) const = 0; virtual void update_sub(uint start, uint len, const void *data) = 0; diff --git a/source/blender/gpu/intern/gpu_vertex_buffer.cc b/source/blender/gpu/intern/gpu_vertex_buffer.cc index abf5d2dc4c0..d24dd6af95b 100644 --- a/source/blender/gpu/intern/gpu_vertex_buffer.cc +++ b/source/blender/gpu/intern/gpu_vertex_buffer.cc @@ -153,14 +153,9 @@ GPUVertBuf *GPU_vertbuf_duplicate(GPUVertBuf *verts_) return wrap(unwrap(verts_)->duplicate()); } -const void *GPU_vertbuf_read(GPUVertBuf *verts) +void GPU_vertbuf_read(GPUVertBuf *verts, void *data) { - return unwrap(verts)->read(); -} - -void *GPU_vertbuf_unmap(const GPUVertBuf *verts, const void *mapped_data) -{ - return unwrap(verts)->unmap(mapped_data); + unwrap(verts)->read(data); } void GPU_vertbuf_clear(GPUVertBuf *verts) diff --git a/source/blender/gpu/intern/gpu_vertex_buffer_private.hh b/source/blender/gpu/intern/gpu_vertex_buffer_private.hh index f20f6caf6de..be8e4f0c185 100644 --- a/source/blender/gpu/intern/gpu_vertex_buffer_private.hh +++ b/source/blender/gpu/intern/gpu_vertex_buffer_private.hh @@ -94,8 +94,7 @@ class VertBuf { } virtual void update_sub(uint start, uint len, const void *data) = 0; - virtual const void *read() const = 0; - virtual void *unmap(const void *mapped_data) const = 0; + virtual void read(void *data) const = 0; protected: virtual void acquire_data() = 0; diff --git a/source/blender/gpu/metal/mtl_index_buffer.hh b/source/blender/gpu/metal/mtl_index_buffer.hh index 702aa7f27d6..dd828598110 100644 --- a/source/blender/gpu/metal/mtl_index_buffer.hh +++ b/source/blender/gpu/metal/mtl_index_buffer.hh @@ -48,7 +48,7 @@ class MTLIndexBuf : public IndexBuf { ~MTLIndexBuf(); void bind_as_ssbo(uint32_t binding) override; - const uint32_t *read() const override; + void read(uint32_t *data) const override; void upload_data() override; void update_sub(uint32_t start, uint32_t len, const void *data) override; diff --git a/source/blender/gpu/metal/mtl_index_buffer.mm b/source/blender/gpu/metal/mtl_index_buffer.mm index cf921e43a4b..6a912983492 100644 --- a/source/blender/gpu/metal/mtl_index_buffer.mm +++ b/source/blender/gpu/metal/mtl_index_buffer.mm @@ -46,16 +46,13 @@ void MTLIndexBuf::bind_as_ssbo(uint32_t binding) MTL_LOG_WARNING("MTLIndexBuf::bind_as_ssbo not yet implemented!\n"); } -const uint32_t *MTLIndexBuf::read() const +void MTLIndexBuf::read(uint32_t *data) const { if (ibo_ != nullptr) { - - /* Return host pointer. */ - void *data = ibo_->get_host_ptr(); - return static_cast(data); + void *host_ptr = ibo_->get_host_ptr(); + memcpy(data, host_ptr, size_get()); } BLI_assert(false && "Index buffer not ready to be read."); - return nullptr; } void MTLIndexBuf::upload_data() diff --git a/source/blender/gpu/metal/mtl_vertex_buffer.hh b/source/blender/gpu/metal/mtl_vertex_buffer.hh index 54d2b7a86a3..056e2062ab1 100644 --- a/source/blender/gpu/metal/mtl_vertex_buffer.hh +++ b/source/blender/gpu/metal/mtl_vertex_buffer.hh @@ -56,8 +56,7 @@ class MTLVertBuf : public VertBuf { void update_sub(uint start, uint len, const void *data) override; - const void *read() const override; - void *unmap(const void *mapped_data) const override; + void read(void *data) const override; void wrap_handle(uint64_t handle) override; diff --git a/source/blender/gpu/metal/mtl_vertex_buffer.mm b/source/blender/gpu/metal/mtl_vertex_buffer.mm index 919eb1e3267..6114e1057c8 100644 --- a/source/blender/gpu/metal/mtl_vertex_buffer.mm +++ b/source/blender/gpu/metal/mtl_vertex_buffer.mm @@ -328,21 +328,12 @@ void MTLVertBuf::bind_as_texture(uint binding) GPU_texture_bind(buffer_texture_, binding); } -const void *MTLVertBuf::read() const +void MTLVertBuf::read(void *data) const { BLI_assert(vbo_ != nullptr); BLI_assert(usage_ != GPU_USAGE_DEVICE_ONLY); - void *return_ptr = vbo_->get_host_ptr(); - BLI_assert(return_ptr != nullptr); - - return return_ptr; -} - -void *MTLVertBuf::unmap(const void *mapped_data) const -{ - void *result = MEM_mallocN(alloc_size_, __func__); - memcpy(result, mapped_data, alloc_size_); - return result; + void *host_ptr = vbo_->get_host_ptr(); + memcpy(data, host_ptr, alloc_size_); } void MTLVertBuf::wrap_handle(uint64_t handle) diff --git a/source/blender/gpu/opengl/gl_index_buffer.cc b/source/blender/gpu/opengl/gl_index_buffer.cc index 566169182e3..2125583cd6b 100644 --- a/source/blender/gpu/opengl/gl_index_buffer.cc +++ b/source/blender/gpu/opengl/gl_index_buffer.cc @@ -46,12 +46,12 @@ void GLIndexBuf::bind_as_ssbo(uint binding) glBindBufferBase(GL_SHADER_STORAGE_BUFFER, binding, ibo_id_); } -const uint32_t *GLIndexBuf::read() const +void GLIndexBuf::read(uint32_t *data) const { BLI_assert(is_active()); - void *data = glMapBuffer(GL_ELEMENT_ARRAY_BUFFER, GL_READ_ONLY); - uint32_t *result = static_cast(data); - return result; + void *buffer = glMapBuffer(GL_ELEMENT_ARRAY_BUFFER, GL_READ_ONLY); + memcpy(data, buffer, size_get()); + glUnmapBuffer(GL_ELEMENT_ARRAY_BUFFER); } bool GLIndexBuf::is_active() const diff --git a/source/blender/gpu/opengl/gl_index_buffer.hh b/source/blender/gpu/opengl/gl_index_buffer.hh index 78159db764c..eeb7aeeca88 100644 --- a/source/blender/gpu/opengl/gl_index_buffer.hh +++ b/source/blender/gpu/opengl/gl_index_buffer.hh @@ -29,7 +29,7 @@ class GLIndexBuf : public IndexBuf { void bind(); void bind_as_ssbo(uint binding) override; - const uint32_t *read() const override; + void read(uint32_t *data) const override; void *offset_ptr(uint additional_vertex_offset) const { diff --git a/source/blender/gpu/opengl/gl_vertex_buffer.cc b/source/blender/gpu/opengl/gl_vertex_buffer.cc index d186c2025b3..3a22480e190 100644 --- a/source/blender/gpu/opengl/gl_vertex_buffer.cc +++ b/source/blender/gpu/opengl/gl_vertex_buffer.cc @@ -125,18 +125,12 @@ void GLVertBuf::bind_as_texture(uint binding) GPU_texture_bind(buffer_texture_, binding); } -const void *GLVertBuf::read() const +void GLVertBuf::read(void *data) const { BLI_assert(is_active()); void *result = glMapBuffer(GL_ARRAY_BUFFER, GL_READ_ONLY); - return result; -} - -void *GLVertBuf::unmap(const void *mapped_data) const -{ - void *result = MEM_mallocN(vbo_size_, __func__); - memcpy(result, mapped_data, vbo_size_); - return result; + memcpy(data, result, size_used_get()); + glUnmapBuffer(GL_ARRAY_BUFFER); } void GLVertBuf::wrap_handle(uint64_t handle) diff --git a/source/blender/gpu/opengl/gl_vertex_buffer.hh b/source/blender/gpu/opengl/gl_vertex_buffer.hh index deb966961f2..5137781309e 100644 --- a/source/blender/gpu/opengl/gl_vertex_buffer.hh +++ b/source/blender/gpu/opengl/gl_vertex_buffer.hh @@ -37,8 +37,7 @@ class GLVertBuf : public VertBuf { void update_sub(uint start, uint len, const void *data) override; - const void *read() const override; - void *unmap(const void *mapped_data) const override; + void read(void *data) const override; void wrap_handle(uint64_t handle) override; diff --git a/source/blender/gpu/tests/gpu_shader_test.cc b/source/blender/gpu/tests/gpu_shader_test.cc index 2125c67108e..c07fda843c7 100644 --- a/source/blender/gpu/tests/gpu_shader_test.cc +++ b/source/blender/gpu/tests/gpu_shader_test.cc @@ -151,8 +151,8 @@ static void test_gpu_shader_compute_vbo() GPU_memory_barrier(GPU_BARRIER_SHADER_STORAGE); /* Download the vertex buffer. */ - const float *data = static_cast(GPU_vertbuf_read(vbo)); - ASSERT_NE(data, nullptr); + float data[SIZE * 4]; + GPU_vertbuf_read(vbo, data); for (int index = 0; index < SIZE; index++) { float expected_value = index; EXPECT_FLOAT_EQ(data[index * 4 + 0], expected_value); @@ -195,8 +195,8 @@ static void test_gpu_shader_compute_ibo() GPU_memory_barrier(GPU_BARRIER_SHADER_STORAGE); /* Download the index buffer. */ - const uint32_t *data = GPU_indexbuf_read(ibo); - ASSERT_NE(data, nullptr); + uint32_t data[SIZE]; + GPU_indexbuf_read(ibo, data); for (int index = 0; index < SIZE; index++) { uint32_t expected = index; EXPECT_EQ(data[index], expected); diff --git a/source/blender/gpu/vulkan/vk_index_buffer.cc b/source/blender/gpu/vulkan/vk_index_buffer.cc index ca3ce48dc64..70387bd0fce 100644 --- a/source/blender/gpu/vulkan/vk_index_buffer.cc +++ b/source/blender/gpu/vulkan/vk_index_buffer.cc @@ -17,9 +17,8 @@ void VKIndexBuffer::bind_as_ssbo(uint /*binding*/) { } -const uint32_t *VKIndexBuffer::read() const +void VKIndexBuffer::read(uint32_t * /*data*/) const { - return 0; } void VKIndexBuffer::update_sub(uint /*start*/, uint /*len*/, const void * /*data*/) diff --git a/source/blender/gpu/vulkan/vk_index_buffer.hh b/source/blender/gpu/vulkan/vk_index_buffer.hh index 94a584086e0..7586b49ff21 100644 --- a/source/blender/gpu/vulkan/vk_index_buffer.hh +++ b/source/blender/gpu/vulkan/vk_index_buffer.hh @@ -17,7 +17,7 @@ class VKIndexBuffer : public IndexBuf { void bind_as_ssbo(uint binding) override; - const uint32_t *read() const override; + void read(uint32_t *data) const override; void update_sub(uint start, uint len, const void *data) override; diff --git a/source/blender/gpu/vulkan/vk_vertex_buffer.cc b/source/blender/gpu/vulkan/vk_vertex_buffer.cc index 3931c927499..f2326ddb134 100644 --- a/source/blender/gpu/vulkan/vk_vertex_buffer.cc +++ b/source/blender/gpu/vulkan/vk_vertex_buffer.cc @@ -32,14 +32,8 @@ void VKVertexBuffer::update_sub(uint /*start*/, uint /*len*/, const void * /*dat { } -const void *VKVertexBuffer::read() const +void VKVertexBuffer::read(void * /*data*/) const { - return nullptr; -} - -void *VKVertexBuffer::unmap(const void * /*mapped_data*/) const -{ - return nullptr; } void VKVertexBuffer::acquire_data() diff --git a/source/blender/gpu/vulkan/vk_vertex_buffer.hh b/source/blender/gpu/vulkan/vk_vertex_buffer.hh index e4240787138..ebc3e105553 100644 --- a/source/blender/gpu/vulkan/vk_vertex_buffer.hh +++ b/source/blender/gpu/vulkan/vk_vertex_buffer.hh @@ -20,8 +20,7 @@ class VKVertexBuffer : public VertBuf { void wrap_handle(uint64_t handle) override; void update_sub(uint start, uint len, const void *data) override; - const void *read() const override; - void *unmap(const void *mapped_data) const override; + void read(void *data) const override; protected: void acquire_data() override;