From 65ee6497d620110546181c1dfbd0cc3064f9f7f9 Mon Sep 17 00:00:00 2001
From: Austin Eng <enga@chromium.org>
Date: Fri, 22 May 2020 00:23:39 +0000
Subject: [PATCH] Fix
 VulkanImageWrappingUsageTests.ClearImageAcrossDevicesAliased

In Vulkan, importing memory by file descriptor takes ownership of
the file descriptor. It is necessary to dup it in
ClearImageAcrossDevicesAliased because the texture is imported
twice. This fixes these tests on SwiftShader.

Bug: dawn:417
Change-Id: I08b6464c4b8bd31f738037678e29fd6d066e7888
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/22020
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
---
 src/include/dawn_native/VulkanBackend.h       |  5 ++++-
 .../VulkanImageWrappingTestsOpaqueFD.cpp      | 21 +++++++++++--------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/include/dawn_native/VulkanBackend.h b/src/include/dawn_native/VulkanBackend.h
index b144e4ece7..0965871941 100644
--- a/src/include/dawn_native/VulkanBackend.h
+++ b/src/include/dawn_native/VulkanBackend.h
@@ -34,7 +34,10 @@ namespace dawn_native { namespace vulkan {
 
 // Can't use DAWN_PLATFORM_LINUX since header included in both dawn and chrome
 #ifdef __linux__
-        // Common properties of external images represented by FDs
+        // Common properties of external images represented by FDs. On successful import the file
+        // descriptor's ownership is transferred to the Dawn implementation and they shouldn't be
+        // used outside of Dawn again. TODO(enga): Also transfer ownership in the error case so the
+        // caller can assume the FD is always consumed.
         struct DAWN_NATIVE_EXPORT ExternalImageDescriptorFD : ExternalImageDescriptor {
           public:
             int memoryFD;  // A file descriptor from an export of the memory of the image
diff --git a/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp b/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp
index 8e3dc8a259..3fc37cf788 100644
--- a/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp
+++ b/src/tests/white_box/VulkanImageWrappingTestsOpaqueFD.cpp
@@ -34,7 +34,6 @@ namespace dawn_native { namespace vulkan {
             void SetUp() override {
                 DawnTest::SetUp();
                 DAWN_SKIP_TEST_IF(UsesWire());
-                DAWN_SKIP_TEST_IF(IsSwiftshader());
 
                 deviceVk = reinterpret_cast<dawn_native::vulkan::Device*>(device.Get());
             }
@@ -202,7 +201,7 @@ namespace dawn_native { namespace vulkan {
       public:
         void SetUp() override {
             VulkanImageWrappingTestBase::SetUp();
-            if (UsesWire() || IsSwiftshader()) {
+            if (UsesWire()) {
                 return;
             }
 
@@ -220,7 +219,7 @@ namespace dawn_native { namespace vulkan {
         }
 
         void TearDown() override {
-            if (UsesWire() || IsSwiftshader()) {
+            if (UsesWire()) {
                 VulkanImageWrappingTestBase::TearDown();
                 return;
             }
@@ -354,7 +353,7 @@ namespace dawn_native { namespace vulkan {
       public:
         void SetUp() override {
             VulkanImageWrappingTestBase::SetUp();
-            if (UsesWire() || IsSwiftshader()) {
+            if (UsesWire()) {
                 return;
             }
 
@@ -382,7 +381,7 @@ namespace dawn_native { namespace vulkan {
         }
 
         void TearDown() override {
-            if (UsesWire() || IsSwiftshader()) {
+            if (UsesWire()) {
                 VulkanImageWrappingTestBase::TearDown();
                 return;
             }
@@ -488,13 +487,17 @@ namespace dawn_native { namespace vulkan {
     // alias the same memory
     TEST_P(VulkanImageWrappingUsageTests, ClearImageAcrossDevicesAliased) {
         DAWN_SKIP_TEST_IF(UsesWire());
+
+        // WrapVulkanImage consumes the file descriptor so we can't import defaultFd twice.
+        // Duplicate the file descriptor so we can import it twice.
+        int defaultFdCopy = dup(defaultFd);
+        ASSERT(defaultFdCopy != -1);
+
         // Import the image on |device
         wgpu::Texture wrappedTextureAlias =
-            WrapVulkanImage(device, &defaultDescriptor, defaultFd, defaultAllocationSize,
+            WrapVulkanImage(device, &defaultDescriptor, defaultFdCopy, defaultAllocationSize,
                             defaultMemoryTypeIndex, {});
 
-        int memoryFd = GetMemoryFd(deviceVk, defaultAllocation);
-
         // Import the image on |secondDevice|
         wgpu::Texture wrappedTexture =
             WrapVulkanImage(secondDevice, &defaultDescriptor, defaultFd, defaultAllocationSize,
@@ -507,7 +510,7 @@ namespace dawn_native { namespace vulkan {
                                                                           wrappedTexture.Get());
 
         // Import the image to |device|, making sure we wait on signalFd
-        memoryFd = GetMemoryFd(deviceVk, defaultAllocation);
+        int memoryFd = GetMemoryFd(deviceVk, defaultAllocation);
         wgpu::Texture nextWrappedTexture =
             WrapVulkanImage(device, &defaultDescriptor, memoryFd, defaultAllocationSize,
                             defaultMemoryTypeIndex, {signalFd});