Merge pull request #3382 from erikaharrison-adsk/adsk/bugfix/incorrect-HgiVulkanBlitCmds-CopyBufferCpuToGpu

Autodesk: Vulkan Bugfix - HgiVulkanBlitCmds offset

(Internal change: 2347311)

Author: pixar-oss

pxr/imaging/hgiVulkan/blitCmds.cpp

@@ -283,23 +283,27 @@ void HgiVulkanBlitCmds::CopyBufferCpuToGpu(
     if (!buffer->IsCPUStagingAddress(copyOp.cpuSourceBuffer) ||
         copyOp.sourceByteOffset != copyOp.destinationByteOffset) {
 
-        uint8_t* dst = static_cast<uint8_t*>(buffer->GetCPUStagingAddress());
-        size_t dstOffset = copyOp.destinationByteOffset;
+        // Offset into the src buffer.
+        const uint8_t* const src =
+            static_cast<const uint8_t*>(copyOp.cpuSourceBuffer) +
+                copyOp.sourceByteOffset;
 
-        // Offset into the src buffer
-        uint8_t* src = ((uint8_t*) copyOp.cpuSourceBuffer) +
-            copyOp.sourceByteOffset;
+        // Offset into the dst buffer.
+        uint8_t* const dst =
+            static_cast<uint8_t*>(buffer->GetCPUStagingAddress()) +
+                copyOp.destinationByteOffset;
 
-        // Offset into the dst buffer
-        memcpy(dst + dstOffset, src, copyOp.byteSize);
+        memcpy(dst, src, copyOp.byteSize);
     }
 
     // Schedule copy data from staging buffer to device-local buffer.
     HgiVulkanBuffer* stagingBuffer = buffer->GetStagingBuffer();
 
     if (TF_VERIFY(stagingBuffer)) {
         VkBufferCopy copyRegion = {};
-        copyRegion.srcOffset = copyOp.sourceByteOffset;
+        // Note we use the destinationByteOffset as the srcOffset here. The staging buffer
+        // should be prepared with the same data layout of the destination buffer.
+        copyRegion.srcOffset = copyOp.destinationByteOffset;
         copyRegion.dstOffset = copyOp.destinationByteOffset;
         copyRegion.size = copyOp.byteSize;
 

pxr/imaging/hgiVulkan/testenv/testHgiVulkan.cpp

@@ -588,6 +588,46 @@ TestVulkanBuffer(HgiVulkan& hgiVulkan)
         return false;
     }
 
+    // Test another CPU to GPU copy, this time with different src and dst
+    // offsets.
+
+    // Write new data into CPU staging area
+    stagingBlob[8] = 789;
+    stagingBlob[9] = 789;
+    stagingBlob[10] = 789;
+    stagingBlob[11] = 789;
+    memcpy(cpuAddress, stagingBlob.data() + 4, 8 * sizeof(blob[0]));
+
+    // Schedule copy from staging area to GPU device-local buffer.
+    HgiBufferCpuToGpuOp transferOp2;
+    transferOp2.byteSize = 8 * sizeof(blob[0]);
+    transferOp2.cpuSourceBuffer = cpuAddress;
+    transferOp2.sourceByteOffset = 0;
+    transferOp2.destinationByteOffset = 4 * sizeof(blob[0]);
+    transferOp2.gpuDestinationBuffer = buffer;
+
+    HgiBlitCmdsUniquePtr blitCmds4 = hgiVulkan.CreateBlitCmds();
+    blitCmds4->CopyBufferCpuToGpu(transferOp2);
+    hgiVulkan.SubmitCmds(blitCmds4.get(), HgiSubmitWaitTypeWaitUntilCompleted);
+
+    // Read back the transfer to confirm it worked
+    HgiBufferGpuToCpuOp copyOp3;
+    copyOp3.byteSize = desc.byteSize;
+    copyOp3.cpuDestinationBuffer = &transferBackBlob[0];
+    copyOp3.destinationByteOffset = 0;
+    copyOp3.gpuSourceBuffer = buffer;
+    copyOp3.sourceByteOffset = 0;
+
+    HgiBlitCmdsUniquePtr blitCmds5 = hgiVulkan.CreateBlitCmds();
+    blitCmds5->CopyBufferGpuToCpu(copyOp3);
+    hgiVulkan.SubmitCmds(blitCmds5.get(), HgiSubmitWaitTypeWaitUntilCompleted);
+
+    std::cout << std::endl;
+    if (transferBackBlob != stagingBlob) {
+        TF_CODING_ERROR("Transfer readback failed");
+        return false;
+    }
+
     // Put buffer in garbage collector
     device->WaitForIdle();
     hgiVulkan.DestroyBuffer(&buffer);