MapAsync update: keep read/write handle through lifetime of mappable buffers

Change dawn read/write handle for buffer mapping to be created at buffer
creation time instead of at mapAsync time. Update related buffer mapping
tests and wire tests.

Bug: dawn:773
Change-Id: I7dd423c94e1bc15cfe561ea33ec9e348ddf2bfe0
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/51164
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Shrek Shao <shrekshao@google.com>

src/dawn_wire/server/ServerBuffer.cpp

@@ -25,9 +25,13 @@ namespace dawn_wire { namespace server {
         auto* buffer = BufferObjects().Get(cmd.selfId);
         DAWN_ASSERT(buffer != nullptr);
 
-        // The buffer was unmapped. Clear the Read/WriteHandle.
-        buffer->readHandle = nullptr;
-        buffer->writeHandle = nullptr;
+        if (buffer->mappedAtCreation && !(buffer->usage & WGPUMapMode_Write)) {
+            // This indicates the writeHandle is for mappedAtCreation only. Destroy on unmap
+            // writeHandle could have possibly been deleted if buffer is already destroyed so we
+            // don't assert it's non-null
+            buffer->writeHandle = nullptr;
+        }
+
         buffer->mapWriteState = BufferMapWriteState::Unmapped;
 
         return true;
@@ -47,12 +51,10 @@ namespace dawn_wire { namespace server {
     }
 
     bool Server::DoBufferMapAsync(ObjectId bufferId,
-                                  uint32_t requestSerial,
+                                  uint64_t requestSerial,
                                   WGPUMapModeFlags mode,
                                   uint64_t offset64,
-                                  uint64_t size64,
-                                  uint64_t handleCreateInfoLength,
-                                  const uint8_t* handleCreateInfo) {
+                                  uint64_t size64) {
         // These requests are just forwarded to the buffer, with userdata containing what the
         // client will require in the return command.
 
@@ -66,13 +68,6 @@ namespace dawn_wire { namespace server {
             return false;
         }
 
-        // The server only knows how to deal with write XOR read. Validate that.
-        bool isReadMode = mode & WGPUMapMode_Read;
-        bool isWriteMode = mode & WGPUMapMode_Write;
-        if (!(isReadMode ^ isWriteMode)) {
-            return false;
-        }
-
         std::unique_ptr<MapUserdata> userdata = MakeUserdata<MapUserdata>();
         userdata->buffer = ObjectHandle{bufferId, buffer->generation};
         userdata->bufferObj = buffer->handle;
@@ -80,8 +75,7 @@ namespace dawn_wire { namespace server {
         userdata->mode = mode;
 
         if (offset64 > std::numeric_limits<size_t>::max() ||
-            size64 > std::numeric_limits<size_t>::max() ||
-            handleCreateInfoLength > std::numeric_limits<size_t>::max()) {
+            size64 > std::numeric_limits<size_t>::max()) {
             OnBufferMapAsyncCallback(WGPUBufferMapAsyncStatus_Error, userdata.get());
             return true;
         }
@@ -92,32 +86,6 @@ namespace dawn_wire { namespace server {
         userdata->offset = offset;
         userdata->size = size;
 
-        // The handle will point to the mapped memory or staging memory for the mapping.
-        // Store it on the map request.
-        if (isWriteMode) {
-            // Deserialize metadata produced from the client to create a companion server handle.
-            MemoryTransferService::WriteHandle* writeHandle = nullptr;
-            if (!mMemoryTransferService->DeserializeWriteHandle(
-                    handleCreateInfo, static_cast<size_t>(handleCreateInfoLength), &writeHandle)) {
-                return false;
-            }
-            ASSERT(writeHandle != nullptr);
-
-            userdata->writeHandle =
-                std::unique_ptr<MemoryTransferService::WriteHandle>(writeHandle);
-        } else {
-            ASSERT(isReadMode);
-            // Deserialize metadata produced from the client to create a companion server handle.
-            MemoryTransferService::ReadHandle* readHandle = nullptr;
-            if (!mMemoryTransferService->DeserializeReadHandle(
-                    handleCreateInfo, static_cast<size_t>(handleCreateInfoLength), &readHandle)) {
-                return false;
-            }
-            ASSERT(readHandle != nullptr);
-
-            userdata->readHandle = std::unique_ptr<MemoryTransferService::ReadHandle>(readHandle);
-        }
-
         mProcs.bufferMapAsync(
             buffer->handle, mode, offset, size,
             ForwardToServer<decltype(
@@ -130,8 +98,10 @@ namespace dawn_wire { namespace server {
     bool Server::DoDeviceCreateBuffer(ObjectId deviceId,
                                       const WGPUBufferDescriptor* descriptor,
                                       ObjectHandle bufferResult,
-                                      uint64_t handleCreateInfoLength,
-                                      const uint8_t* handleCreateInfo) {
+                                      uint64_t readHandleCreateInfoLength,
+                                      const uint8_t* readHandleCreateInfo,
+                                      uint64_t writeHandleCreateInfoLength,
+                                      const uint8_t* writeHandleCreateInfo) {
         auto* device = DeviceObjects().Get(deviceId);
         if (device == nullptr) {
             return false;
@@ -145,56 +115,84 @@ namespace dawn_wire { namespace server {
         resultData->generation = bufferResult.generation;
         resultData->handle = mProcs.deviceCreateBuffer(device->handle, descriptor);
         resultData->deviceInfo = device->info.get();
+        resultData->usage = descriptor->usage;
+        resultData->mappedAtCreation = descriptor->mappedAtCreation;
         if (!TrackDeviceChild(resultData->deviceInfo, ObjectType::Buffer, bufferResult.id)) {
             return false;
         }
 
-        // If the buffer isn't mapped at creation, we are done.
-        if (!descriptor->mappedAtCreation) {
-            return handleCreateInfoLength == 0;
-        }
+        // isReadMode and isWriteMode could be true at the same time if usage contains
+        // WGPUMapMode_Read and buffer is mappedAtCreation
+        bool isReadMode = descriptor->usage & WGPUMapMode_Read;
+        bool isWriteMode = descriptor->usage & WGPUMapMode_Write || descriptor->mappedAtCreation;
 
-        // This is the size of data deserialized from the command stream to create the write handle,
-        // which must be CPU-addressable.
-        if (handleCreateInfoLength > std::numeric_limits<size_t>::max()) {
+        // This is the size of data deserialized from the command stream to create the read/write
+        // handle, which must be CPU-addressable.
+        if (readHandleCreateInfoLength > std::numeric_limits<size_t>::max() ||
+            writeHandleCreateInfoLength > std::numeric_limits<size_t>::max() ||
+            readHandleCreateInfoLength >
+                std::numeric_limits<size_t>::max() - writeHandleCreateInfoLength) {
             return false;
         }
 
-        void* mapping = mProcs.bufferGetMappedRange(resultData->handle, 0, descriptor->size);
-        if (mapping == nullptr) {
-            // A zero mapping is used to indicate an allocation error of an error buffer. This is a
-            // valid case and isn't fatal. Remember the buffer is an error so as to skip subsequent
-            // mapping operations.
-            resultData->mapWriteState = BufferMapWriteState::MapError;
-            return true;
+        if (isWriteMode) {
+            MemoryTransferService::WriteHandle* writeHandle = nullptr;
+            // Deserialize metadata produced from the client to create a companion server handle.
+            if (!mMemoryTransferService->DeserializeWriteHandle(
+                    writeHandleCreateInfo, static_cast<size_t>(writeHandleCreateInfoLength),
+                    &writeHandle)) {
+                return false;
+            }
+            ASSERT(writeHandle != nullptr);
+            resultData->writeHandle.reset(writeHandle);
+            writeHandle->SetDataLength(descriptor->size);
+
+            if (descriptor->mappedAtCreation) {
+                void* mapping =
+                    mProcs.bufferGetMappedRange(resultData->handle, 0, descriptor->size);
+                if (mapping == nullptr) {
+                    // A zero mapping is used to indicate an allocation error of an error buffer.
+                    // This is a valid case and isn't fatal. Remember the buffer is an error so as
+                    // to skip subsequent mapping operations.
+                    resultData->mapWriteState = BufferMapWriteState::MapError;
+                    return true;
+                }
+                ASSERT(mapping != nullptr);
+                writeHandle->SetTarget(mapping);
+
+                resultData->mapWriteState = BufferMapWriteState::Mapped;
+            }
         }
 
-        // Deserialize metadata produced from the client to create a companion server handle.
-        MemoryTransferService::WriteHandle* writeHandle = nullptr;
-        if (!mMemoryTransferService->DeserializeWriteHandle(
-                handleCreateInfo, static_cast<size_t>(handleCreateInfoLength), &writeHandle)) {
-            return false;
+        if (isReadMode) {
+            MemoryTransferService::ReadHandle* readHandle = nullptr;
+            // Deserialize metadata produced from the client to create a companion server handle.
+            if (!mMemoryTransferService->DeserializeReadHandle(
+                    readHandleCreateInfo, static_cast<size_t>(readHandleCreateInfoLength),
+                    &readHandle)) {
+                return false;
+            }
+            ASSERT(readHandle != nullptr);
+
+            resultData->readHandle.reset(readHandle);
         }
 
-        // Set the target of the WriteHandle to the mapped GPU memory.
-        ASSERT(writeHandle != nullptr);
-        writeHandle->SetTarget(mapping, descriptor->size);
-
-        resultData->mapWriteState = BufferMapWriteState::Mapped;
-        resultData->writeHandle.reset(writeHandle);
-
         return true;
     }
 
     bool Server::DoBufferUpdateMappedData(ObjectId bufferId,
-                                          uint64_t writeFlushInfoLength,
-                                          const uint8_t* writeFlushInfo) {
+                                          uint64_t writeDataUpdateInfoLength,
+                                          const uint8_t* writeDataUpdateInfo,
+                                          uint64_t offset,
+                                          uint64_t size) {
         // The null object isn't valid as `self`
         if (bufferId == 0) {
             return false;
         }
 
-        if (writeFlushInfoLength > std::numeric_limits<size_t>::max()) {
+        if (writeDataUpdateInfoLength > std::numeric_limits<size_t>::max() ||
+            offset > std::numeric_limits<size_t>::max() ||
+            size > std::numeric_limits<size_t>::max()) {
             return false;
         }
 
@@ -220,8 +218,9 @@ namespace dawn_wire { namespace server {
 
         // Deserialize the flush info and flush updated data from the handle into the target
         // of the handle. The target is set via WriteHandle::SetTarget.
-        return buffer->writeHandle->DeserializeFlush(writeFlushInfo,
-                                                     static_cast<size_t>(writeFlushInfoLength));
+        return buffer->writeHandle->DeserializeDataUpdate(
+            writeDataUpdateInfo, static_cast<size_t>(writeDataUpdateInfoLength),
+            static_cast<size_t>(offset), static_cast<size_t>(size));
     }
 
     void Server::OnBufferMapAsyncCallback(WGPUBufferMapAsyncStatus status, MapUserdata* data) {
@@ -238,39 +237,41 @@ namespace dawn_wire { namespace server {
         cmd.buffer = data->buffer;
         cmd.requestSerial = data->requestSerial;
         cmd.status = status;
-        cmd.readInitialDataInfoLength = 0;
-        cmd.readInitialDataInfo = nullptr;
+        cmd.readDataUpdateInfoLength = 0;
+        cmd.readDataUpdateInfo = nullptr;
 
         const void* readData = nullptr;
-        if (isSuccess && isRead) {
-            // Get the serialization size of the message to initialize ReadHandle data.
-            readData = mProcs.bufferGetConstMappedRange(data->bufferObj, data->offset, data->size);
-            cmd.readInitialDataInfoLength =
-                data->readHandle->SerializeInitialDataSize(readData, data->size);
+        if (isSuccess) {
+            if (isRead) {
+                // Get the serialization size of the message to initialize ReadHandle data.
+                readData =
+                    mProcs.bufferGetConstMappedRange(data->bufferObj, data->offset, data->size);
+                cmd.readDataUpdateInfoLength =
+                    bufferData->readHandle->SizeOfSerializeDataUpdate(data->offset, data->size);
+            } else {
+                ASSERT(data->mode & WGPUMapMode_Write);
+                // The in-flight map request returned successfully.
+                bufferData->mapWriteState = BufferMapWriteState::Mapped;
+                // Set the target of the WriteHandle to the mapped buffer data.
+                // writeHandle Target always refers to the buffer base address.
+                // but we call getMappedRange exactly with the range of data that is potentially
+                // modified (i.e. we don't want getMappedRange(0, wholeBufferSize) if only a
+                // subset of the buffer is actually mapped) in case the implementation does some
+                // range tracking.
+                bufferData->writeHandle->SetTarget(
+                    static_cast<uint8_t*>(
+                        mProcs.bufferGetMappedRange(data->bufferObj, data->offset, data->size)) -
+                    data->offset);
+            }
         }
 
-        SerializeCommand(cmd, cmd.readInitialDataInfoLength, [&](SerializeBuffer* serializeBuffer) {
-            if (isSuccess) {
-                if (isRead) {
-                    char* readHandleBuffer;
-                    WIRE_TRY(
-                        serializeBuffer->NextN(cmd.readInitialDataInfoLength, &readHandleBuffer));
-
-                    // Serialize the initialization message into the space after the command.
-                    data->readHandle->SerializeInitialData(readData, data->size, readHandleBuffer);
-                    // The in-flight map request returned successfully.
-                    // Move the ReadHandle so it is owned by the buffer.
-                    bufferData->readHandle = std::move(data->readHandle);
-                } else {
-                    // The in-flight map request returned successfully.
-                    // Move the WriteHandle so it is owned by the buffer.
-                    bufferData->writeHandle = std::move(data->writeHandle);
-                    bufferData->mapWriteState = BufferMapWriteState::Mapped;
-                    // Set the target of the WriteHandle to the mapped buffer data.
-                    bufferData->writeHandle->SetTarget(
-                        mProcs.bufferGetMappedRange(data->bufferObj, data->offset, data->size),
-                        data->size);
-                }
+        SerializeCommand(cmd, cmd.readDataUpdateInfoLength, [&](SerializeBuffer* serializeBuffer) {
+            if (isSuccess && isRead) {
+                char* readHandleBuffer;
+                WIRE_TRY(serializeBuffer->NextN(cmd.readDataUpdateInfoLength, &readHandleBuffer));
+                // The in-flight map request returned successfully.
+                bufferData->readHandle->SerializeDataUpdate(readData, data->offset, data->size,
+                                                            readHandleBuffer);
             }
             return WireResult::Success;
         });