From d428187d35197f275f3d257b841836f4cd6b9fad Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Wed, 8 Jun 2022 14:46:41 +0000 Subject: [PATCH] Add reflection APIs for wgpu::Buffer. Changes dawn::native procs to correctly convert C++ enum and bitmask returns values. Bug: dawn:1451 Change-Id: I39a8d218f76e25b178a83eeb99d653222d39d040 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/92440 Kokoro: Kokoro Reviewed-by: Loko Kung Commit-Queue: Austin Eng Reviewed-by: Austin Eng --- dawn.json | 8 +++ dawn_wire.json | 2 + generator/templates/dawn/native/ProcTable.cpp | 4 +- .../templates/dawn/native/dawn_platform.h | 10 +++ src/dawn/native/Buffer.cpp | 14 +++- src/dawn/native/Buffer.h | 7 +- .../validation/BufferValidationTests.cpp | 71 +++++++++++++++++++ src/dawn/wire/client/Buffer.cpp | 17 ++++- src/dawn/wire/client/Buffer.h | 9 ++- src/dawn/wire/client/Device.cpp | 3 +- 10 files changed, 132 insertions(+), 13 deletions(-) diff --git a/dawn.json b/dawn.json index 0a85dcffd4..a5b0dcc9e0 100644 --- a/dawn.json +++ b/dawn.json @@ -433,6 +433,14 @@ {"name": "label", "type": "char", "annotation": "const*", "length": "strlen"} ] }, + { + "name": "get usage", + "returns": "buffer usage" + }, + { + "name": "get size", + "returns": "uint64_t" + }, { "name": "unmap" }, diff --git a/dawn_wire.json b/dawn_wire.json index 2e2318efb1..8ce3e90bb0 100644 --- a/dawn_wire.json +++ b/dawn_wire.json @@ -189,6 +189,8 @@ "BufferMapAsync", "BufferGetConstMappedRange", "BufferGetMappedRange", + "BufferGetSize", + "BufferGetUsage", "DeviceCreateBuffer", "DeviceCreateComputePipelineAsync", "DeviceCreateRenderPipelineAsync", diff --git a/generator/templates/dawn/native/ProcTable.cpp b/generator/templates/dawn/native/ProcTable.cpp index cd829e3031..f9980ffc61 100644 --- a/generator/templates/dawn/native/ProcTable.cpp +++ b/generator/templates/dawn/native/ProcTable.cpp @@ -66,7 +66,7 @@ namespace {{native_namespace}} { {%- endfor -%} ); {% if method.return_type.name.canonical_case() != "void" %} - {% if method.return_type.category == "object" %} + {% if method.return_type.category in ["object", "enum", "bitmask"] %} return ToAPI(result); {% else %} return result; @@ -104,7 +104,7 @@ namespace {{native_namespace}} { {%- endfor -%} ); {% if function.return_type.name.canonical_case() != "void" %} - {% if function.return_type.category == "object" %} + {% if function.return_type.category in ["object", "enum", "bitmask"] %} return ToAPI(result); {% else %} return result; diff --git a/generator/templates/dawn/native/dawn_platform.h b/generator/templates/dawn/native/dawn_platform.h index e3f1c91a33..1bb5426991 100644 --- a/generator/templates/dawn/native/dawn_platform.h +++ b/generator/templates/dawn/native/dawn_platform.h @@ -77,6 +77,16 @@ namespace {{native_namespace}} { static constexpr uint32_t value = {{len(e.values)}}; }; {% endfor %} + + {% for type in by_category["enum"] + by_category["bitmask"] %} + inline {{as_cType(type.name)}} ToAPI({{namespace}}::{{as_cppType(type.name)}} rhs) { + return static_cast<{{as_cType(type.name)}}>(rhs); + } + + inline {{namespace}}::{{as_cppType(type.name)}} FromAPI({{as_cType(type.name)}} rhs) { + return static_cast<{{namespace}}::{{as_cppType(type.name)}}>(rhs); + } + {% endfor %} } #endif // {{NATIVE_DIR}}_{{PREFIX}}_PLATFORM_AUTOGEN_H_ diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index 80e905b00f..b9b2e3dbb0 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp @@ -158,7 +158,10 @@ BufferBase::BufferBase(DeviceBase* device, const BufferDescriptor* descriptor) BufferBase::BufferBase(DeviceBase* device, const BufferDescriptor* descriptor, ObjectBase::ErrorTag tag) - : ApiObjectBase(device, tag), mSize(descriptor->size), mState(BufferState::Unmapped) { + : ApiObjectBase(device, tag), + mSize(descriptor->size), + mUsage(descriptor->usage), + mState(BufferState::Unmapped) { if (descriptor->mappedAtCreation) { mState = BufferState::MappedAtCreation; mMapOffset = 0; @@ -215,9 +218,14 @@ wgpu::BufferUsage BufferBase::GetUsage() const { } wgpu::BufferUsage BufferBase::GetUsageExternalOnly() const { + ASSERT(!IsError()); return GetUsage() & ~kAllInternalBufferUsages; } +wgpu::BufferUsage BufferBase::APIGetUsage() const { + return mUsage & ~kAllInternalBufferUsages; +} + MaybeError BufferBase::MapAtCreation() { DAWN_TRY(MapAtCreationInternal()); @@ -378,6 +386,10 @@ void BufferBase::APIDestroy() { Destroy(); } +uint64_t BufferBase::APIGetSize() const { + return mSize; +} + MaybeError BufferBase::CopyFromStagingBuffer() { ASSERT(mStagingBuffer); if (mSize == 0) { diff --git a/src/dawn/native/Buffer.h b/src/dawn/native/Buffer.h index e5c6150584..6bb1503340 100644 --- a/src/dawn/native/Buffer.h +++ b/src/dawn/native/Buffer.h @@ -48,8 +48,6 @@ class BufferBase : public ApiObjectBase { MappedAtCreation, Destroyed, }; - BufferBase(DeviceBase* device, const BufferDescriptor* descriptor); - static BufferBase* MakeError(DeviceBase* device, const BufferDescriptor* descriptor); ObjectType GetType() const override; @@ -86,12 +84,15 @@ class BufferBase : public ApiObjectBase { const void* APIGetConstMappedRange(size_t offset, size_t size); void APIUnmap(); void APIDestroy(); + wgpu::BufferUsage APIGetUsage() const; + uint64_t APIGetSize() const; protected: + BufferBase(DeviceBase* device, const BufferDescriptor* descriptor); BufferBase(DeviceBase* device, const BufferDescriptor* descriptor, ObjectBase::ErrorTag tag); - // Constructor used only for mocking and testing. BufferBase(DeviceBase* device, BufferState state); + void DestroyImpl() override; ~BufferBase() override; diff --git a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp index a063c4c971..11b42be275 100644 --- a/src/dawn/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/BufferValidationTests.cpp @@ -940,3 +940,74 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { EXPECT_EQ(buffer.GetMappedRange(0, 0), nullptr); } } + +// Test that the buffer creation parameters are correctly reflected for succesfully created buffers. +TEST_F(BufferValidationTest, CreationParameterReflectionForValidBuffer) { + // Test reflection on two succesfully created but different buffers. The reflected data should + // be different! + { + wgpu::BufferDescriptor desc; + desc.size = 16; + desc.usage = wgpu::BufferUsage::Uniform; + wgpu::Buffer buf = device.CreateBuffer(&desc); + + EXPECT_EQ(wgpu::BufferUsage::Uniform, buf.GetUsage()); + EXPECT_EQ(16u, buf.GetSize()); + } + { + wgpu::BufferDescriptor desc; + desc.size = 32; + desc.usage = wgpu::BufferUsage::Storage; + wgpu::Buffer buf = device.CreateBuffer(&desc); + + EXPECT_EQ(wgpu::BufferUsage::Storage, buf.GetUsage()); + EXPECT_EQ(32u, buf.GetSize()); + } +} + +// Test that the buffer creation parameters are correctly reflected for buffers invalid because of +// validation errors. +TEST_F(BufferValidationTest, CreationParameterReflectionForErrorBuffer) { + wgpu::BufferDescriptor desc; + desc.usage = wgpu::BufferUsage::Uniform; + desc.size = 19; + desc.mappedAtCreation = true; + + // Error! MappedAtCreation requires size % 4 == 0. + wgpu::Buffer buf; + ASSERT_DEVICE_ERROR(buf = device.CreateBuffer(&desc)); + + // Reflection data is still exactly what was in the descriptor. + EXPECT_EQ(wgpu::BufferUsage::Uniform, buf.GetUsage()); + EXPECT_EQ(19u, buf.GetSize()); +} + +// Test that the buffer creation parameters are correctly reflected for buffers invalid because of +// OOM. +TEST_F(BufferValidationTest, CreationParameterReflectionForOOMBuffer) { + constexpr uint64_t kAmazinglyLargeSize = 0x1234'5678'90AB'CDEF; + wgpu::BufferDescriptor desc; + desc.usage = wgpu::BufferUsage::Storage; + desc.size = kAmazinglyLargeSize; + + // OOM! + wgpu::Buffer buf; + ASSERT_DEVICE_ERROR(buf = device.CreateBuffer(&desc)); + + // Reflection data is still exactly what was in the descriptor. + EXPECT_EQ(wgpu::BufferUsage::Storage, buf.GetUsage()); + EXPECT_EQ(kAmazinglyLargeSize, buf.GetSize()); +} + +// Test that buffer reflection doesn't show internal usages +TEST_F(BufferValidationTest, CreationParameterReflectionNoInternalUsage) { + wgpu::BufferDescriptor desc; + desc.size = 16; + // QueryResolve also adds kInternalStorageBuffer for processing of queries. + desc.usage = wgpu::BufferUsage::QueryResolve; + wgpu::Buffer buf = device.CreateBuffer(&desc); + + // The reflection shouldn't show kInternalStorageBuffer + EXPECT_EQ(wgpu::BufferUsage::QueryResolve, buf.GetUsage()); + EXPECT_EQ(16u, buf.GetSize()); +} diff --git a/src/dawn/wire/client/Buffer.cpp b/src/dawn/wire/client/Buffer.cpp index 850d822997..eb9fa617d0 100644 --- a/src/dawn/wire/client/Buffer.cpp +++ b/src/dawn/wire/client/Buffer.cpp @@ -54,7 +54,7 @@ WGPUBuffer Buffer::Create(Device* device, const WGPUBufferDescriptor* descriptor wireClient->GetMemoryTransferService()->CreateReadHandle(descriptor->size)); if (readHandle == nullptr) { device->InjectError(WGPUErrorType_OutOfMemory, "Failed to create buffer mapping"); - return device->CreateErrorBuffer(); + return CreateError(device, descriptor); } cmd.readHandleCreateInfoLength = readHandle->SerializeCreateSize(); } @@ -65,7 +65,7 @@ WGPUBuffer Buffer::Create(Device* device, const WGPUBufferDescriptor* descriptor wireClient->GetMemoryTransferService()->CreateWriteHandle(descriptor->size)); if (writeHandle == nullptr) { device->InjectError(WGPUErrorType_OutOfMemory, "Failed to create buffer mapping"); - return device->CreateErrorBuffer(); + return CreateError(device, descriptor); } cmd.writeHandleCreateInfoLength = writeHandle->SerializeCreateSize(); } @@ -79,6 +79,7 @@ WGPUBuffer Buffer::Create(Device* device, const WGPUBufferDescriptor* descriptor buffer->mDevice = device; buffer->mDeviceIsAlive = device->GetAliveWeakPtr(); buffer->mSize = descriptor->size; + buffer->mUsage = static_cast(descriptor->usage); buffer->mDestructWriteHandleOnUnmap = false; if (descriptor->mappedAtCreation) { @@ -124,10 +125,12 @@ WGPUBuffer Buffer::Create(Device* device, const WGPUBufferDescriptor* descriptor } // static -WGPUBuffer Buffer::CreateError(Device* device) { +WGPUBuffer Buffer::CreateError(Device* device, const WGPUBufferDescriptor* descriptor) { auto* allocation = device->client->BufferAllocator().New(device->client); allocation->object->mDevice = device; allocation->object->mDeviceIsAlive = device->GetAliveWeakPtr(); + allocation->object->mSize = descriptor->size; + allocation->object->mUsage = static_cast(descriptor->usage); DeviceCreateErrorBufferCmd cmd; cmd.self = ToAPI(device); @@ -365,6 +368,14 @@ void Buffer::Destroy() { client->SerializeCommand(cmd); } +WGPUBufferUsage Buffer::GetUsage() const { + return mUsage; +} + +uint64_t Buffer::GetSize() const { + return mSize; +} + bool Buffer::IsMappedForReading() const { return mMapState == MapState::MappedForRead; } diff --git a/src/dawn/wire/client/Buffer.h b/src/dawn/wire/client/Buffer.h index 21ec8a6804..2644a4fe74 100644 --- a/src/dawn/wire/client/Buffer.h +++ b/src/dawn/wire/client/Buffer.h @@ -28,10 +28,8 @@ class Device; class Buffer final : public ObjectBase { public: - using ObjectBase::ObjectBase; - static WGPUBuffer Create(Device* device, const WGPUBufferDescriptor* descriptor); - static WGPUBuffer CreateError(Device* device); + static WGPUBuffer CreateError(Device* device, const WGPUBufferDescriptor* descriptor); Buffer(Client* client, uint32_t refcount, uint32_t id); ~Buffer(); @@ -51,6 +49,10 @@ class Buffer final : public ObjectBase { void Destroy(); + // Note that these values can be arbitrary since they aren't validated in the wire client. + WGPUBufferUsage GetUsage() const; + uint64_t GetSize() const; + private: void CancelCallbacksForDisconnect() override; void ClearAllCallbacks(WGPUBufferMapAsyncStatus status); @@ -90,6 +92,7 @@ class Buffer final : public ObjectBase { }; RequestTracker mRequests; uint64_t mSize = 0; + WGPUBufferUsage mUsage; // Only one mapped pointer can be active at a time because Unmap clears all the in-flight // requests. diff --git a/src/dawn/wire/client/Device.cpp b/src/dawn/wire/client/Device.cpp index 6e4d932643..f4b24601b9 100644 --- a/src/dawn/wire/client/Device.cpp +++ b/src/dawn/wire/client/Device.cpp @@ -200,7 +200,8 @@ WGPUBuffer Device::CreateBuffer(const WGPUBufferDescriptor* descriptor) { } WGPUBuffer Device::CreateErrorBuffer() { - return Buffer::CreateError(this); + WGPUBufferDescriptor fakeDescriptor = {}; + return Buffer::CreateError(this, &fakeDescriptor); } WGPUQueue Device::GetQueue() {