diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 0414159f1f..cc1b6e7813 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -311,22 +311,26 @@ namespace dawn_native { } void* BufferBase::GetMappedRange() { - if (GetDevice()->ConsumedError(ValidateGetMappedRange(true))) { - return nullptr; - } - if (mStagingBuffer != nullptr) { - return mStagingBuffer->GetMappedPointer(); - } - return GetMappedPointerImpl(); + return GetMappedRangeInternal(true); } const void* BufferBase::GetConstMappedRange() { - if (GetDevice()->ConsumedError(ValidateGetMappedRange(false))) { + return GetMappedRangeInternal(false); + } + + // TODO(dawn:445): When CreateBufferMapped is removed, make GetMappedRangeInternal also take + // care of the validation of GetMappedRange. + void* BufferBase::GetMappedRangeInternal(bool writable) { + if (!CanGetMappedRange(writable)) { return nullptr; } + if (mStagingBuffer != nullptr) { return mStagingBuffer->GetMappedPointer(); } + if (mSize == 0) { + return reinterpret_cast(intptr_t(0xCAFED00D)); + } return GetMappedPointerImpl(); } @@ -431,24 +435,29 @@ namespace dawn_native { return {}; } - MaybeError BufferBase::ValidateGetMappedRange(bool writable) const { - DAWN_TRY(GetDevice()->ValidateIsAlive()); - DAWN_TRY(GetDevice()->ValidateObject(this)); + bool BufferBase::CanGetMappedRange(bool writable) const { + // Note that: + // + // - We don't check that the device is alive because the application can ask for the + // mapped pointer before it knows, and even Dawn knows, that the device was lost, and + // still needs to work properly. + // - We don't check that the object is alive because we need to return mapped pointers + // for error buffers too. switch (mState) { // Writeable Buffer::GetMappedRange is always allowed when mapped at creation. case BufferState::MappedAtCreation: - return {}; + return true; case BufferState::Mapped: - if (writable && !(mUsage & wgpu::BufferUsage::MapWrite)) { - return DAWN_VALIDATION_ERROR("GetMappedRange requires the MapWrite usage"); - } - return {}; + ASSERT(bool(mUsage & wgpu::BufferUsage::MapRead) ^ + bool(mUsage & wgpu::BufferUsage::MapWrite)); + return !writable || (mUsage & wgpu::BufferUsage::MapWrite); case BufferState::Unmapped: case BufferState::Destroyed: - return DAWN_VALIDATION_ERROR("Buffer is not mapped"); + return false; + default: UNREACHABLE(); } @@ -493,7 +502,7 @@ namespace dawn_native { } void BufferBase::OnMapCommandSerialFinished(uint32_t mapSerial, bool isWrite) { - void* data = GetMappedPointerImpl(); + void* data = GetMappedRangeInternal(isWrite); if (isWrite) { CallMapWriteCallback(mapSerial, WGPUBufferMapAsyncStatus_Success, data, GetSize()); } else { diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h index ea67fab4bc..3dfaa2a1ac 100644 --- a/src/dawn_native/Buffer.h +++ b/src/dawn_native/Buffer.h @@ -72,15 +72,6 @@ namespace dawn_native { BufferBase(DeviceBase* device, ObjectBase::ErrorTag tag); ~BufferBase() override; - void CallMapReadCallback(uint32_t serial, - WGPUBufferMapAsyncStatus status, - const void* pointer, - uint64_t dataLength); - void CallMapWriteCallback(uint32_t serial, - WGPUBufferMapAsyncStatus status, - void* pointer, - uint64_t dataLength); - void DestroyInternal(); bool IsMapped() const; @@ -95,12 +86,21 @@ namespace dawn_native { virtual bool IsMapWritable() const = 0; MaybeError CopyFromStagingBuffer(); + void* GetMappedRangeInternal(bool writable); + void CallMapReadCallback(uint32_t serial, + WGPUBufferMapAsyncStatus status, + const void* pointer, + uint64_t dataLength); + void CallMapWriteCallback(uint32_t serial, + WGPUBufferMapAsyncStatus status, + void* pointer, + uint64_t dataLength); MaybeError ValidateMap(wgpu::BufferUsage requiredUsage, WGPUBufferMapAsyncStatus* status) const; - MaybeError ValidateGetMappedRange(bool writable) const; MaybeError ValidateUnmap() const; MaybeError ValidateDestroy() const; + bool CanGetMappedRange(bool writable) const; uint64_t mSize = 0; wgpu::BufferUsage mUsage = wgpu::BufferUsage::None; diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp index 74e84690e2..37758cfb84 100644 --- a/src/dawn_native/vulkan/BufferVk.cpp +++ b/src/dawn_native/vulkan/BufferVk.cpp @@ -176,14 +176,6 @@ namespace dawn_native { namespace vulkan { DestroyInternal(); } - void Buffer::OnMapReadCommandSerialFinished(uint32_t mapSerial, const void* data) { - CallMapReadCallback(mapSerial, WGPUBufferMapAsyncStatus_Success, data, GetSize()); - } - - void Buffer::OnMapWriteCommandSerialFinished(uint32_t mapSerial, void* data) { - CallMapWriteCallback(mapSerial, WGPUBufferMapAsyncStatus_Success, data, GetSize()); - } - VkBuffer Buffer::GetHandle() const { return mHandle; } diff --git a/src/dawn_native/vulkan/BufferVk.h b/src/dawn_native/vulkan/BufferVk.h index ace13a90a6..feb64b44a4 100644 --- a/src/dawn_native/vulkan/BufferVk.h +++ b/src/dawn_native/vulkan/BufferVk.h @@ -30,9 +30,6 @@ namespace dawn_native { namespace vulkan { public: static ResultOrError Create(Device* device, const BufferDescriptor* descriptor); - void OnMapReadCommandSerialFinished(uint32_t mapSerial, const void* data); - void OnMapWriteCommandSerialFinished(uint32_t mapSerial, void* data); - VkBuffer GetHandle() const; // Transitions the buffer to be used as `usage`, recording any necessary barrier in diff --git a/src/dawn_native/vulkan/DeviceVk.cpp b/src/dawn_native/vulkan/DeviceVk.cpp index 5a2a905ff0..da8c5924c2 100644 --- a/src/dawn_native/vulkan/DeviceVk.cpp +++ b/src/dawn_native/vulkan/DeviceVk.cpp @@ -853,6 +853,7 @@ namespace dawn_native { namespace vulkan { // Releasing the uploader enqueues buffers to be released. // Call Tick() again to clear them before releasing the deleter. + mResourceMemoryAllocator->Tick(GetCompletedCommandSerial()); mDeleter->Tick(GetCompletedCommandSerial()); // The VkRenderPasses in the cache can be destroyed immediately since all commands referring diff --git a/src/dawn_wire/client/Buffer.cpp b/src/dawn_wire/client/Buffer.cpp index 993b2b2e9a..eeb5e49b15 100644 --- a/src/dawn_wire/client/Buffer.cpp +++ b/src/dawn_wire/client/Buffer.cpp @@ -367,7 +367,6 @@ namespace dawn_wire { namespace client { void* Buffer::GetMappedRange() { if (!IsMappedForWriting()) { - device->InjectError(WGPUErrorType_Validation, "Buffer needs to be mapped for writing"); return nullptr; } return mMappedData; @@ -375,7 +374,6 @@ namespace dawn_wire { namespace client { const void* Buffer::GetConstMappedRange() { if (!IsMappedForWriting() && !IsMappedForReading()) { - device->InjectError(WGPUErrorType_Validation, "Buffer needs to be mapped"); return nullptr; } return mMappedData; diff --git a/src/tests/end2end/BufferTests.cpp b/src/tests/end2end/BufferTests.cpp index b0ff2b8621..7163051357 100644 --- a/src/tests/end2end/BufferTests.cpp +++ b/src/tests/end2end/BufferTests.cpp @@ -127,7 +127,21 @@ TEST_P(BufferMapReadTests, GetMappedRange) { wgpu::Buffer buffer = device.CreateBuffer(&descriptor); const void* mappedData = MapReadAsyncAndWait(buffer); - ASSERT_EQ(mappedData, buffer.GetConstMappedRange()); + ASSERT_EQ(buffer.GetConstMappedRange(), mappedData); + ASSERT_NE(buffer.GetConstMappedRange(), nullptr); + UnmapBuffer(buffer); +} + +// Test the result of GetMappedRange when mapped for reading for a zero-sized buffer. +TEST_P(BufferMapReadTests, GetMappedRangeZeroSized) { + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst; + wgpu::Buffer buffer = device.CreateBuffer(&descriptor); + + const void* mappedData = MapReadAsyncAndWait(buffer); + ASSERT_EQ(buffer.GetConstMappedRange(), mappedData); + ASSERT_NE(buffer.GetConstMappedRange(), nullptr); UnmapBuffer(buffer); } @@ -273,8 +287,23 @@ TEST_P(BufferMapWriteTests, GetMappedRange) { wgpu::Buffer buffer = device.CreateBuffer(&descriptor); void* mappedData = MapWriteAsyncAndWait(buffer); - ASSERT_EQ(mappedData, buffer.GetMappedRange()); - ASSERT_EQ(mappedData, buffer.GetConstMappedRange()); + ASSERT_EQ(buffer.GetMappedRange(), mappedData); + ASSERT_EQ(buffer.GetMappedRange(), buffer.GetConstMappedRange()); + ASSERT_NE(buffer.GetMappedRange(), nullptr); + UnmapBuffer(buffer); +} + +// Test the result of GetMappedRange when mapped for writing for a zero-sized buffer. +TEST_P(BufferMapWriteTests, GetMappedRangeZeroSized) { + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc; + wgpu::Buffer buffer = device.CreateBuffer(&descriptor); + + void* mappedData = MapWriteAsyncAndWait(buffer); + ASSERT_EQ(buffer.GetMappedRange(), mappedData); + ASSERT_EQ(buffer.GetMappedRange(), buffer.GetConstMappedRange()); + ASSERT_NE(buffer.GetMappedRange(), nullptr); UnmapBuffer(buffer); } @@ -538,8 +567,23 @@ TEST_P(CreateBufferMappedTests, GetMappedRange) { wgpu::CreateBufferMappedResult result; result = device.CreateBufferMapped(&descriptor); - ASSERT_EQ(result.data, result.buffer.GetMappedRange()); - ASSERT_EQ(result.data, result.buffer.GetConstMappedRange()); + ASSERT_EQ(result.buffer.GetMappedRange(), result.data); + ASSERT_EQ(result.buffer.GetMappedRange(), result.buffer.GetConstMappedRange()); + ASSERT_NE(result.buffer.GetMappedRange(), nullptr); + result.buffer.Unmap(); +} + +// Test the result of GetMappedRange when mapped at creation for a zero-sized buffer. +TEST_P(CreateBufferMappedTests, GetMappedRangeZeroSized) { + wgpu::BufferDescriptor descriptor; + descriptor.size = 0; + descriptor.usage = wgpu::BufferUsage::CopyDst; + wgpu::CreateBufferMappedResult result; + result = device.CreateBufferMapped(&descriptor); + + ASSERT_EQ(result.buffer.GetMappedRange(), result.data); + ASSERT_EQ(result.buffer.GetMappedRange(), result.buffer.GetConstMappedRange()); + ASSERT_NE(result.buffer.GetMappedRange(), nullptr); result.buffer.Unmap(); } diff --git a/src/tests/end2end/DeviceLostTests.cpp b/src/tests/end2end/DeviceLostTests.cpp index fbae566264..40d8fe4b6d 100644 --- a/src/tests/end2end/DeviceLostTests.cpp +++ b/src/tests/end2end/DeviceLostTests.cpp @@ -332,6 +332,73 @@ TEST_P(DeviceLostTest, WriteBufferFails) { ASSERT_DEVICE_ERROR(queue.WriteBuffer(buffer, 0, &data, sizeof(data))); } +// Test it's possible to GetMappedRange on a buffer created mapped after device loss +// TODO(cwallez@chromium.org): enable after CreateBufferMapped is implemented in terms of +// mappedAtCreation. +TEST_P(DeviceLostTest, DISABLED_GetMappedRange_CreateBufferMappedAfterLoss) { + SetCallbackAndLoseForTesting(); + + wgpu::BufferDescriptor desc; + desc.size = 4; + desc.usage = wgpu::BufferUsage::CopySrc; + ASSERT_DEVICE_ERROR(wgpu::CreateBufferMappedResult result = device.CreateBufferMapped(&desc)); + + ASSERT_NE(result.buffer.GetMappedRange(), nullptr); + ASSERT_EQ(result.buffer.GetMappedRange(), result.data); +} + +// Test that device loss doesn't change the result of GetMappedRange, createBufferMapped version. +TEST_P(DeviceLostTest, GetMappedRange_CreateBufferMappedBeforeLoss) { + wgpu::BufferDescriptor desc; + desc.size = 4; + desc.usage = wgpu::BufferUsage::CopySrc; + wgpu::CreateBufferMappedResult result = device.CreateBufferMapped(&desc); + + void* rangeBeforeLoss = result.buffer.GetMappedRange(); + SetCallbackAndLoseForTesting(); + + ASSERT_NE(result.buffer.GetMappedRange(), nullptr); + ASSERT_EQ(result.buffer.GetMappedRange(), rangeBeforeLoss); + ASSERT_EQ(result.buffer.GetMappedRange(), result.data); +} + +// Test that device loss doesn't change the result of GetMappedRange, mapReadAsync version. +TEST_P(DeviceLostTest, GetMappedRange_MapReadAsync) { + wgpu::BufferDescriptor desc; + desc.size = 4; + desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst; + wgpu::Buffer buffer = device.CreateBuffer(&desc); + + buffer.MapReadAsync(nullptr, nullptr); + queue.Submit(0, nullptr); + + const void* rangeBeforeLoss = buffer.GetConstMappedRange(); + SetCallbackAndLoseForTesting(); + + ASSERT_NE(buffer.GetConstMappedRange(), nullptr); + ASSERT_EQ(buffer.GetConstMappedRange(), rangeBeforeLoss); +} + +// Test that device loss doesn't change the result of GetMappedRange, mapReadAsync version. +TEST_P(DeviceLostTest, GetMappedRange_MapWriteAsync) { + wgpu::BufferDescriptor desc; + desc.size = 4; + desc.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc; + wgpu::Buffer buffer = device.CreateBuffer(&desc); + + buffer.MapWriteAsync(nullptr, nullptr); + queue.Submit(0, nullptr); + + const void* rangeBeforeLoss = buffer.GetConstMappedRange(); + SetCallbackAndLoseForTesting(); + + ASSERT_NE(buffer.GetConstMappedRange(), nullptr); + ASSERT_EQ(buffer.GetConstMappedRange(), rangeBeforeLoss); +} + +// mapreadasync + resolve + loss getmappedrange != nullptr. +// mapwriteasync + resolve + loss getmappedrange != nullptr. + // Test that Command Encoder Finish fails when device lost TEST_P(DeviceLostTest, CommandEncoderFinishFails) { wgpu::CommandBuffer commands; diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp index d1ececfcb1..eedd18be7b 100644 --- a/src/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/tests/unittests/validation/BufferValidationTests.cpp @@ -672,8 +672,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnUnmappedBuffer) { desc.usage = wgpu::BufferUsage::CopySrc; wgpu::Buffer buf = device.CreateBuffer(&desc); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } // Unmapped after CreateBufferMapped case. @@ -681,8 +681,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnUnmappedBuffer) { wgpu::Buffer buf = CreateBufferMapped(4, wgpu::BufferUsage::CopySrc).buffer; buf.Unmap(); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } // Unmapped after MapReadAsync case. @@ -696,8 +696,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnUnmappedBuffer) { queue.Submit(0, nullptr); buf.Unmap(); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } // Unmapped after MapWriteAsync case. @@ -710,8 +710,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnUnmappedBuffer) { queue.Submit(0, nullptr); buf.Unmap(); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } } @@ -725,8 +725,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnDestroyedBuffer) { wgpu::Buffer buf = device.CreateBuffer(&desc); buf.Destroy(); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } // Destroyed after CreateBufferMapped case. @@ -734,8 +734,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnDestroyedBuffer) { wgpu::Buffer buf = CreateBufferMapped(4, wgpu::BufferUsage::CopySrc).buffer; buf.Destroy(); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } // Destroyed after MapReadAsync case. @@ -749,8 +749,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnDestroyedBuffer) { queue.Submit(0, nullptr); buf.Destroy(); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } // Destroyed after MapWriteAsync case. @@ -763,8 +763,8 @@ TEST_F(BufferValidationTest, GetMappedRangeOnDestroyedBuffer) { queue.Submit(0, nullptr); buf.Destroy(); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetConstMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); + ASSERT_EQ(nullptr, buf.GetConstMappedRange()); } } @@ -778,7 +778,7 @@ TEST_F(BufferValidationTest, GetMappedRangeOnMappedForReading) { .Times(1); queue.Submit(0, nullptr); - ASSERT_DEVICE_ERROR(ASSERT_EQ(nullptr, buf.GetMappedRange())); + ASSERT_EQ(nullptr, buf.GetMappedRange()); } // Test valid cases to call GetMappedRange on a buffer. @@ -825,3 +825,34 @@ TEST_F(BufferValidationTest, GetMappedRangeValidCases) { ASSERT_EQ(buf.GetConstMappedRange(), mappedPointer); } } + +// Test valid cases to call GetMappedRange on an error buffer. +// TODO(cwallez@chromium.org): enable after CreateBufferMapped is implemented in terms of +// mappedAtCreation. +TEST_F(BufferValidationTest, DISABLED_GetMappedRangeOnErrorBuffer) { + wgpu::BufferDescriptor desc; + desc.size = 4; + desc.usage = wgpu::BufferUsage::Storage | wgpu::BufferUsage::MapRead; + + // GetMappedRange after CreateBufferMapped non-OOM returns a non-nullptr. + { + wgpu::CreateBufferMappedResult result; + ASSERT_DEVICE_ERROR(result = CreateBufferMapped( + 4, wgpu::BufferUsage::Storage | wgpu::BufferUsage::MapRead)); + + ASSERT_NE(result.buffer.GetConstMappedRange(), nullptr); + ASSERT_EQ(result.buffer.GetConstMappedRange(), result.buffer.GetMappedRange()); + ASSERT_EQ(result.buffer.GetConstMappedRange(), result.data); + } + + // GetMappedRange after CreateBufferMapped OOM case returns nullptr. + { + wgpu::CreateBufferMappedResult result; + ASSERT_DEVICE_ERROR(result = CreateBufferMapped( + 1 << 31, wgpu::BufferUsage::Storage | wgpu::BufferUsage::MapRead)); + + ASSERT_EQ(result.buffer.GetConstMappedRange(), nullptr); + ASSERT_EQ(result.buffer.GetConstMappedRange(), result.buffer.GetMappedRange()); + ASSERT_EQ(result.buffer.GetConstMappedRange(), result.data); + } +}