Rework how combined aspects are handled in TextureVk

At the moment the computations to decide whether aspects should be
combined are executed on every call related to aspect in TextureVk.
These computations never change and can be computed once at the creation
of TextureVk and reused at runtime.

This is meant to be a noop change as a slight rework prior to fixing
depth-stencil T2T copies no using the combined aspects.

Bug: dawn:1514

Change-Id: I1177cdcf42d072bb2bc2c3a2f149dc480fe79f2f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/103420
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

src/dawn/native/vulkan/TextureVk.cpp

@@ -216,6 +216,31 @@ void FillVulkanCreateInfoSizesAndType(const Texture& texture, VkImageCreateInfo*
     }
 }
 
+Aspect ComputeCombinedAspect(Device* device, const Format& format) {
+    // In early Vulkan versions it is not possible to transition depth and stencil separetely so
+    // textures with Depth|Stencil will be promoted to a single CombinedDepthStencil aspect
+    // internally.
+    if (format.aspects == (Aspect::Depth | Aspect::Stencil)) {
+        return Aspect::CombinedDepthStencil;
+    }
+    // Same thing for Stencil8 if it is emulated with a depth-stencil format and not directly S8.
+    if (format.format == wgpu::TextureFormat::Stencil8 &&
+        !device->IsToggleEnabled(Toggle::VulkanUseS8)) {
+        return Aspect::CombinedDepthStencil;
+    }
+
+    // Some multiplanar images cannot have planes transitioned separately and instead Vulkan
+    // requires that the "Color" aspect be used for barriers, so Plane0|Plane1 is promoted to just
+    // Color. The Vulkan spec requires: "If image has a single-plane color format or is not
+    // disjoint, then the aspectMask member of subresourceRange must be VK_IMAGE_ASPECT_COLOR_BIT.".
+    if (format.aspects == (Aspect::Plane0 | Aspect::Plane1)) {
+        return Aspect::Color;
+    }
+
+    // No need to combine aspects.
+    return Aspect::None;
+}
+
 }  // namespace
 
 // Converts Dawn texture format to Vulkan formats.
@@ -631,14 +656,14 @@ Ref<Texture> Texture::CreateForSwapChain(Device* device,
 
 Texture::Texture(Device* device, const TextureDescriptor* descriptor, TextureState state)
     : TextureBase(device, descriptor, state),
+      mCombinedAspect(ComputeCombinedAspect(device, GetFormat())),
       // A usage of none will make sure the texture is transitioned before its first use as
       // required by the Vulkan spec.
-      mSubresourceLastUsages(std::make_unique<SubresourceStorage<wgpu::TextureUsage>>(
+      mSubresourceLastUsages(
-          (ShouldCombineDepthStencilBarriers() ? Aspect::CombinedDepthStencil
+          mCombinedAspect != Aspect::None ? mCombinedAspect : GetFormat().aspects,
-                                               : GetFormat().aspects),
           GetArrayLayers(),
           GetNumMipLevels(),
-          wgpu::TextureUsage::None)) {}
+          wgpu::TextureUsage::None) {}
 
 MaybeError Texture::InitializeAsInternalTexture(VkImageUsageFlags extraUsages) {
     Device* device = ToBackend(GetDevice());
@@ -720,16 +745,17 @@ MaybeError Texture::InitializeFromExternal(const ExternalImageDescriptorVk* desc
     Device* device = ToBackend(GetDevice());
     VkFormat format = VulkanImageFormat(device, GetFormat().format);
     VkImageUsageFlags usage = VulkanImageUsage(GetInternalUsage(), GetFormat());
-    DAWN_INVALID_IF(!externalMemoryService->SupportsCreateImage(descriptor, format, usage,
+
-                                                                &mSupportsDisjointVkImage),
+    bool supportsDisjoint;
+    DAWN_INVALID_IF(
+        !externalMemoryService->SupportsCreateImage(descriptor, format, usage, &supportsDisjoint),
         "Creating an image from external memory is not supported.");
-    // mSubresourceLastUsages was initialized with Plane0/Plane1 in the constructor for
+    // The creation of mSubresourceLastUsage assumes that multi-planar are always disjoint and sets
-    // multiplanar formats, so we need to correct it to Color here.
+    // the combined aspect without checking for disjoint support.
-    if (ShouldCombineMultiPlaneBarriers()) {
+    // TODO(dawn:1548): Support multi-planar images with the DISJOINT feature and potentially allow
-        mSubresourceLastUsages = std::make_unique<SubresourceStorage<wgpu::TextureUsage>>(
+    // acting on planes individually? Always using Color is valid even for disjoint images.
-            ComputeAspectsForSubresourceStorage(), GetArrayLayers(), GetNumMipLevels(),
+    DAWN_UNUSED(supportsDisjoint);
-            wgpu::TextureUsage::None);
+    ASSERT(!GetFormat().IsMultiPlanar() || mCombinedAspect == Aspect::Color);
-    }
 
     mExternalState = ExternalState::PendingAcquire;
     mExportQueueFamilyIndex = externalMemoryService->GetQueueFamilyIndex();
@@ -805,11 +831,11 @@ MaybeError Texture::BindExternalMemory(const ExternalImageDescriptorVk* descript
 void Texture::TransitionEagerlyForExport(CommandRecordingContext* recordingContext) {
     mExternalState = ExternalState::EagerlyTransitioned;
 
-    Aspect aspects = ComputeAspectsForSubresourceStorage();
+    // Get any usage, ideally the last one to do nothing
     ASSERT(GetNumMipLevels() == 1 && GetArrayLayers() == 1);
-    SubresourceRange range = {aspects, {0, 1}, {0, 1}};
+    SubresourceRange range = {GetAllVulkanAspects(), {0, 1}, {0, 1}};
 
-    wgpu::TextureUsage usage = mSubresourceLastUsages->Get(aspects, 0, 0);
+    wgpu::TextureUsage usage = mSubresourceLastUsages.Get(range.aspects, 0, 0);
 
     std::vector<VkImageMemoryBarrier> barriers;
     VkPipelineStageFlags srcStages = 0;
@@ -855,9 +881,8 @@ MaybeError Texture::ExportExternalTexture(VkImageLayout desiredLayout,
     // Release the texture
     mExternalState = ExternalState::Released;
 
-    Aspect aspects = ComputeAspectsForSubresourceStorage();
     ASSERT(GetNumMipLevels() == 1 && GetArrayLayers() == 1);
-    wgpu::TextureUsage usage = mSubresourceLastUsages->Get(aspects, 0, 0);
+    wgpu::TextureUsage usage = mSubresourceLastUsages.Get(GetAllVulkanAspects(), 0, 0);
 
     VkImageLayout layout = VulkanImageLayout(this, usage);
 
@@ -946,7 +971,7 @@ void Texture::TweakTransitionForExternalUsage(CommandRecordingContext* recording
         if (barriers->size() == transitionBarrierStart) {
             barriers->push_back(BuildMemoryBarrier(
                 this, wgpu::TextureUsage::None, wgpu::TextureUsage::None,
-                SubresourceRange::SingleMipAndLayer(0, 0, ComputeAspectsForSubresourceStorage())));
+                SubresourceRange::SingleMipAndLayer(0, 0, GetAllVulkanAspects())));
         }
 
         VkImageMemoryBarrier* barrier = &(*barriers)[transitionBarrierStart];
@@ -1029,53 +1054,17 @@ bool Texture::CanReuseWithoutBarrier(wgpu::TextureUsage lastUsage, wgpu::Texture
     return false;
 }
 
-// Base Vulkan doesn't support transitioning depth and stencil separately. We work around
-// this limitation by combining the usages in the two planes of `textureUsages` into a
-// single plane in a new SubresourceStorage<TextureUsage>. The barriers will be produced
-// for DEPTH | STENCIL since the SubresourceRange uses Aspect::CombinedDepthStencil.
-bool Texture::ShouldCombineDepthStencilBarriers() const {
-    // If the Stencil8 format is being emulated then memory barriers also need to include
-    // the depth aspect. (See: crbug.com/dawn/1331)
-    if (GetFormat().format == wgpu::TextureFormat::Stencil8 &&
-        !GetDevice()->IsToggleEnabled(Toggle::VulkanUseS8)) {
-        return true;
-    }
-    return GetFormat().aspects == (Aspect::Depth | Aspect::Stencil);
-}
-
-// The Vulkan spec requires:
-// "If image has a single-plane color format or is not disjoint, then the aspectMask member of
-// subresourceRange must be VK_IMAGE_ASPECT_COLOR_BIT.".
-// For multi-planar formats, we currently only support import them in non-disjoint way.
-bool Texture::ShouldCombineMultiPlaneBarriers() const {
-    // TODO(chromium:1258986): Figure out how to support disjoint vkImage.
-    ASSERT(!mSupportsDisjointVkImage);
-    return GetFormat().aspects == (Aspect::Plane0 | Aspect::Plane1);
-}
-
-Aspect Texture::ComputeAspectsForSubresourceStorage() const {
-    if (ShouldCombineDepthStencilBarriers()) {
-        return Aspect::CombinedDepthStencil;
-    }
-    // Force to use Aspect::Color for Aspect::Plane0/1.
-    if (ShouldCombineMultiPlaneBarriers()) {
-        return Aspect::Color;
-    }
-    return GetFormat().aspects;
-}
-
 void Texture::TransitionUsageForPass(CommandRecordingContext* recordingContext,
                                      const TextureSubresourceUsage& textureUsages,
                                      std::vector<VkImageMemoryBarrier>* imageBarriers,
                                      VkPipelineStageFlags* srcStages,
                                      VkPipelineStageFlags* dstStages) {
-    if (ShouldCombineBarriers()) {
+    if (UseCombinedAspects()) {
-        Aspect combinedAspect = ComputeAspectsForSubresourceStorage();
+        SubresourceStorage<wgpu::TextureUsage> combinedUsages(mCombinedAspect, GetArrayLayers(),
-        SubresourceStorage<wgpu::TextureUsage> combinedUsages(combinedAspect, GetArrayLayers(),
                                                               GetNumMipLevels());
         textureUsages.Iterate([&](const SubresourceRange& range, wgpu::TextureUsage usage) {
             SubresourceRange updateRange = range;
-            updateRange.aspects = combinedAspect;
+            updateRange.aspects = mCombinedAspect;
 
             combinedUsages.Update(updateRange,
                                   [&](const SubresourceRange&, wgpu::TextureUsage* combinedUsage) {
@@ -1103,7 +1092,7 @@ void Texture::TransitionUsageForPassImpl(
     wgpu::TextureUsage allUsages = wgpu::TextureUsage::None;
     wgpu::TextureUsage allLastUsages = wgpu::TextureUsage::None;
 
-    mSubresourceLastUsages->Merge(subresourceUsages, [&](const SubresourceRange& range,
+    mSubresourceLastUsages.Merge(subresourceUsages, [&](const SubresourceRange& range,
                                                         wgpu::TextureUsage* lastUsage,
                                                         const wgpu::TextureUsage& newUsage) {
         if (newUsage == wgpu::TextureUsage::None || CanReuseWithoutBarrier(*lastUsage, newUsage)) {
@@ -1153,9 +1142,9 @@ void Texture::TransitionUsageAndGetResourceBarrier(wgpu::TextureUsage usage,
                                                    std::vector<VkImageMemoryBarrier>* imageBarriers,
                                                    VkPipelineStageFlags* srcStages,
                                                    VkPipelineStageFlags* dstStages) {
-    if (ShouldCombineBarriers()) {
+    if (UseCombinedAspects()) {
         SubresourceRange updatedRange = range;
-        updatedRange.aspects = ComputeAspectsForSubresourceStorage();
+        updatedRange.aspects = mCombinedAspect;
         TransitionUsageAndGetResourceBarrierImpl(usage, updatedRange, imageBarriers, srcStages,
                                                  dstStages);
     } else {
@@ -1173,7 +1162,7 @@ void Texture::TransitionUsageAndGetResourceBarrierImpl(
     const Format& format = GetFormat();
 
     wgpu::TextureUsage allLastUsages = wgpu::TextureUsage::None;
-    mSubresourceLastUsages->Update(
+    mSubresourceLastUsages.Update(
         range, [&](const SubresourceRange& range, wgpu::TextureUsage* lastUsage) {
             if (CanReuseWithoutBarrier(*lastUsage, usage)) {
                 return;
@@ -1352,7 +1341,18 @@ void Texture::UpdateExternalSemaphoreHandle(ExternalSemaphoreHandle handle) {
 
 VkImageLayout Texture::GetCurrentLayoutForSwapChain() const {
     ASSERT(GetFormat().aspects == Aspect::Color);
-    return VulkanImageLayout(this, mSubresourceLastUsages->Get(Aspect::Color, 0, 0));
+    return VulkanImageLayout(this, mSubresourceLastUsages.Get(Aspect::Color, 0, 0));
+}
+
+bool Texture::UseCombinedAspects() const {
+    return mCombinedAspect != Aspect::None;
+}
+
+Aspect Texture::GetAllVulkanAspects() const {
+    if (UseCombinedAspects()) {
+        return mCombinedAspect;
+    }
+    return GetFormat().aspects;
 }
 
 // static

src/dawn/native/vulkan/TextureVk.h

@@ -138,24 +138,6 @@ class Texture final : public TextureBase {
                                          size_t transitionBarrierStart);
     bool CanReuseWithoutBarrier(wgpu::TextureUsage lastUsage, wgpu::TextureUsage usage);
 
-    // In base Vulkan, Depth and stencil can only be transitioned together. This function
-    // indicates whether we should combine depth and stencil barriers to accommodate this
-    // limitation.
-    bool ShouldCombineDepthStencilBarriers() const;
-
-    // This indicates whether the VK_IMAGE_ASPECT_COLOR_BIT instead of
-    // VK_IMAGE_ASPECT_PLANE_n_BIT must be used.
-    bool ShouldCombineMultiPlaneBarriers() const;
-
-    bool ShouldCombineBarriers() const {
-        return ShouldCombineDepthStencilBarriers() || ShouldCombineMultiPlaneBarriers();
-    }
-
-    // Compute the Aspects of the SubresourceStoage for this texture depending on whether we're
-    // doing the workaround for combined depth and stencil barriers, or combining multi-plane
-    // barriers.
-    Aspect ComputeAspectsForSubresourceStorage() const;
-
     VkImage mHandle = VK_NULL_HANDLE;
     ResourceMemoryAllocation mMemoryAllocation;
     VkDeviceMemory mExternalAllocation = VK_NULL_HANDLE;
@@ -187,12 +169,21 @@ class Texture final : public TextureBase {
 
     std::vector<VkSemaphore> mWaitRequirements;
 
-    // Note that in early Vulkan versions it is not possible to transition depth and stencil
+    // Sometimes the WebGPU aspects don't directly map to Vulkan aspects:
-    // separately so textures with Depth|Stencil aspects will have a single Depth aspect in the
+    //
-    // storage.
+    //  - In early Vulkan versions it is not possible to transition depth and stencil separetely so
-    std::unique_ptr<SubresourceStorage<wgpu::TextureUsage>> mSubresourceLastUsages;
+    //    textures with Depth|Stencil will be promoted to a single CombinedDepthStencil aspect
+    //    internally.
+    //  - Some multiplanar images cannot have planes transitioned separately and instead Vulkan
+    //    requires that the "Color" aspect be used for barriers, so Plane0|Plane1 is promoted to
+    //    just Color.
+    //
+    // This variable, if not Aspect::None, is the combined aspect to use for all transitions.
+    const Aspect mCombinedAspect;
+    SubresourceStorage<wgpu::TextureUsage> mSubresourceLastUsages;
 
-    bool mSupportsDisjointVkImage = false;
+    bool UseCombinedAspects() const;
+    Aspect GetAllVulkanAspects() const;
 };
 
 class TextureView final : public TextureViewBase {