Dawn: Allow transient storage attachment and pure storage attachments
Storage attachments should be allowed to be transient at least for the
moment because the Metal backend will start by requiring that there is
no implicit PLS (PLS data not associated with an attachment) but
allocating that data would be very expensive.
Storage attachments should also count as attachments for the validation
that pipelines and render passes have at least one attachment.
Bug: dawn:1704
Change-Id: I41305fa447ba25fbae4d38545886448440a39bfc
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/151420
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Quyen Le <lehoangquyen@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/AttachmentState.cpp b/src/dawn/native/AttachmentState.cpp
index 3628446..7acc4c0 100644
--- a/src/dawn/native/AttachmentState.cpp
+++ b/src/dawn/native/AttachmentState.cpp
@@ -107,6 +107,8 @@
DAWN_ASSERT(mSampleCount == attachmentSampleCount);
}
}
+
+ // Gather the depth-stencil information.
if (descriptor->depthStencilAttachment != nullptr) {
TextureViewBase* attachment = descriptor->depthStencilAttachment->view;
mDepthStencilFormat = attachment->GetFormat().format;
@@ -116,7 +118,6 @@
DAWN_ASSERT(mSampleCount == attachment->GetTexture()->GetSampleCount());
}
}
- DAWN_ASSERT(mSampleCount > 0);
// Gather the PLS information.
const RenderPassPixelLocalStorage* pls = nullptr;
@@ -127,10 +128,18 @@
pls->totalPixelLocalStorageSize / kPLSSlotByteSize, wgpu::TextureFormat::Undefined);
for (size_t i = 0; i < pls->storageAttachmentCount; i++) {
size_t slot = pls->storageAttachments[i].offset / kPLSSlotByteSize;
- mStorageAttachmentSlots[slot] = pls->storageAttachments[i].storage->GetFormat().format;
+ const TextureViewBase* attachment = pls->storageAttachments[i].storage;
+ mStorageAttachmentSlots[slot] = attachment->GetFormat().format;
+
+ if (mSampleCount == 0) {
+ mSampleCount = attachment->GetTexture()->GetSampleCount();
+ } else {
+ DAWN_ASSERT(mSampleCount == attachment->GetTexture()->GetSampleCount());
+ }
}
}
+ DAWN_ASSERT(mSampleCount > 0);
SetContentHash(ComputeContentHash());
}
diff --git a/src/dawn/native/CommandEncoder.cpp b/src/dawn/native/CommandEncoder.cpp
index 50eb74d..ff1f297 100644
--- a/src/dawn/native/CommandEncoder.cpp
+++ b/src/dawn/native/CommandEncoder.cpp
@@ -582,7 +582,7 @@
"Color attachment count (%u) exceeds the maximum number of color attachments (%u).",
descriptor->colorAttachmentCount, maxColorAttachments);
- bool isAllColorAttachmentNull = true;
+ bool anyColorAttachment = false;
ColorAttachmentFormats colorAttachmentFormats;
for (uint32_t i = 0; i < descriptor->colorAttachmentCount; ++i) {
DAWN_TRY_CONTEXT(ValidateRenderPassColorAttachment(
@@ -590,7 +590,7 @@
implicitSampleCount, usageValidationMode),
"validating colorAttachments[%u].", i);
if (descriptor->colorAttachments[i].view) {
- isAllColorAttachmentNull = false;
+ anyColorAttachment = true;
colorAttachmentFormats->push_back(&descriptor->colorAttachments[i].view->GetFormat());
}
}
@@ -602,10 +602,6 @@
device, descriptor->depthStencilAttachment, width, height, sampleCount,
usageValidationMode),
"validating depthStencilAttachment.");
- } else {
- DAWN_INVALID_IF(
- isAllColorAttachmentNull,
- "No color or depthStencil attachments specified. At least one is required.");
}
if (descriptor->occlusionQuerySet != nullptr) {
@@ -665,8 +661,7 @@
*implicitSampleCount, usageValidationMode));
}
- DAWN_INVALID_IF(descriptor->colorAttachmentCount == 0 &&
- descriptor->depthStencilAttachment == nullptr &&
+ DAWN_INVALID_IF(!anyColorAttachment && descriptor->depthStencilAttachment == nullptr &&
storageAttachmentCount == 0,
"Render pass has no attachments.");
diff --git a/src/dawn/native/PipelineLayout.cpp b/src/dawn/native/PipelineLayout.cpp
index 1ef68b7..a15fece 100644
--- a/src/dawn/native/PipelineLayout.cpp
+++ b/src/dawn/native/PipelineLayout.cpp
@@ -419,6 +419,15 @@
return mStorageAttachmentSlots;
}
+bool PipelineLayoutBase::HasAnyStorageAttachments() const {
+ for (auto format : mStorageAttachmentSlots) {
+ if (format != wgpu::TextureFormat::Undefined) {
+ return true;
+ }
+ }
+ return false;
+}
+
BindGroupLayoutMask PipelineLayoutBase::InheritedGroupsMask(const PipelineLayoutBase* other) const {
DAWN_ASSERT(!IsError());
return {(1 << static_cast<uint32_t>(GroupsInheritUpTo(other))) - 1u};
diff --git a/src/dawn/native/PipelineLayout.h b/src/dawn/native/PipelineLayout.h
index 9b005d6..3d56a83 100644
--- a/src/dawn/native/PipelineLayout.h
+++ b/src/dawn/native/PipelineLayout.h
@@ -74,6 +74,7 @@
const BindGroupLayoutMask& GetBindGroupLayoutsMask() const;
bool HasPixelLocalStorage() const;
const std::vector<wgpu::TextureFormat>& GetStorageAttachmentSlots() const;
+ bool HasAnyStorageAttachments() const;
// Utility functions to compute inherited bind groups.
// Returns the inherited bind groups as a mask.
diff --git a/src/dawn/native/RenderPipeline.cpp b/src/dawn/native/RenderPipeline.cpp
index 40e2828..8b2bd76 100644
--- a/src/dawn/native/RenderPipeline.cpp
+++ b/src/dawn/native/RenderPipeline.cpp
@@ -655,8 +655,11 @@
descriptor->multisample.alphaToCoverageEnabled),
"validating fragment state.");
- DAWN_INVALID_IF(descriptor->fragment->targetCount == 0 && !descriptor->depthStencil,
- "Must have at least one color or depthStencil target.");
+ bool hasStorageAttachments =
+ descriptor->layout != nullptr && descriptor->layout->HasAnyStorageAttachments();
+ DAWN_INVALID_IF(descriptor->fragment->targetCount == 0 && !descriptor->depthStencil &&
+ !hasStorageAttachments,
+ "No attachment was specified (color, depth-stencil or other).");
DAWN_TRY(ValidateInterStageMatching(device, descriptor->vertex, *(descriptor->fragment)));
}
diff --git a/src/dawn/native/Texture.cpp b/src/dawn/native/Texture.cpp
index 89aa296..76e7b80 100644
--- a/src/dawn/native/Texture.cpp
+++ b/src/dawn/native/Texture.cpp
@@ -350,12 +350,15 @@
"The texture usage (%s) includes %s, which requires the %s feature to be set", usage,
kTransientAttachment, ToAPI(Feature::TransientAttachments));
- const auto kAllowedTransientUsage =
- kTransientAttachment | wgpu::TextureUsage::RenderAttachment;
- DAWN_INVALID_IF(usage != kAllowedTransientUsage,
- "The texture usage (%s) includes %s, which requires that the texture usage "
- "be exactly %s",
- usage, kTransientAttachment, kAllowedTransientUsage);
+ DAWN_INVALID_IF(
+ usage == kTransientAttachment,
+ "The texture usage is only %s (which always requires another attachment usage).",
+ kTransientAttachment);
+ const auto kAttachmentUsages = kTransientAttachment | wgpu::TextureUsage::RenderAttachment |
+ wgpu::TextureUsage::StorageAttachment;
+ DAWN_INVALID_IF(!IsSubset(usage, kAttachmentUsages),
+ "The texture usage (%s) includes both %s and non-attachment usages (%s).",
+ usage, kTransientAttachment, usage & ~kAttachmentUsages);
}
if (!allowedSharedTextureMemoryUsage) {
diff --git a/src/dawn/tests/unittests/validation/PixelLocalStorageTests.cpp b/src/dawn/tests/unittests/validation/PixelLocalStorageTests.cpp
index b774751..e7191ea 100644
--- a/src/dawn/tests/unittests/validation/PixelLocalStorageTests.cpp
+++ b/src/dawn/tests/unittests/validation/PixelLocalStorageTests.cpp
@@ -1028,11 +1028,154 @@
}
}
+// Check that it is allowed to create render passes with only a storage attachment.
+TEST_F(PixelLocalStorageTest, RenderPassOnlyStorageAttachment) {
+ wgpu::TextureDescriptor tDesc;
+ tDesc.format = wgpu::TextureFormat::R32Uint;
+ tDesc.size = {1, 1};
+ tDesc.usage = wgpu::TextureUsage::StorageAttachment;
+ wgpu::Texture tex = device.CreateTexture(&tDesc);
+
+ wgpu::RenderPassStorageAttachment rpAttachment;
+ rpAttachment.storage = tex.CreateView();
+ rpAttachment.offset = 0;
+ rpAttachment.loadOp = wgpu::LoadOp::Load;
+ rpAttachment.storeOp = wgpu::StoreOp::Store;
+
+ wgpu::RenderPassPixelLocalStorage rpPlsDesc;
+ rpPlsDesc.totalPixelLocalStorageSize = 4;
+ rpPlsDesc.storageAttachmentCount = 1;
+ rpPlsDesc.storageAttachments = &rpAttachment;
+
+ wgpu::RenderPassDescriptor rpDesc;
+ rpDesc.nextInChain = &rpPlsDesc;
+ rpDesc.colorAttachmentCount = 0;
+ rpDesc.depthStencilAttachment = nullptr;
+
+ // Success case: a render pass with just a storage attachment is valid.
+ {
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rpDesc);
+ pass.End();
+ encoder.Finish();
+ }
+
+ // Error case: a render pass with PLS but no attachments is invalid.
+ {
+ rpPlsDesc.storageAttachmentCount = 0;
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rpDesc);
+ pass.End();
+ ASSERT_DEVICE_ERROR(encoder.Finish());
+ }
+}
+
+// Check that it is allowed to create render pipelines with only a storage attachment.
+TEST_F(PixelLocalStorageTest, RenderPipelineOnlyStorageAttachment) {
+ wgpu::ShaderModule module = utils::CreateShaderModule(device, R"(
+ enable chromium_experimental_pixel_local;
+
+ @vertex fn vs() -> @builtin(position) vec4f {
+ return vec4f(0, 0, 0, 0.5);
+ }
+
+ struct PLS {
+ value : u32,
+ };
+ var<pixel_local> pls : PLS;
+ @fragment fn fs() {
+ pls.value = pls.value + 1;
+ }
+ )");
+
+ wgpu::PipelineLayoutStorageAttachment plAttachment;
+ plAttachment.offset = 0;
+ plAttachment.format = wgpu::TextureFormat::R32Uint;
+
+ wgpu::PipelineLayoutPixelLocalStorage plPlsDesc;
+ plPlsDesc.totalPixelLocalStorageSize = 4;
+ plPlsDesc.storageAttachmentCount = 1;
+ plPlsDesc.storageAttachments = &plAttachment;
+
+ wgpu::PipelineLayoutDescriptor plDesc;
+ plDesc.nextInChain = &plPlsDesc;
+ plDesc.bindGroupLayoutCount = 0;
+ wgpu::PipelineLayout pl = device.CreatePipelineLayout(&plDesc);
+
+ utils::ComboRenderPipelineDescriptor pDesc;
+ pDesc.layout = pl;
+ pDesc.vertex.module = module;
+ pDesc.vertex.entryPoint = "vs";
+ pDesc.cFragment.module = module;
+ pDesc.cFragment.entryPoint = "fs";
+ pDesc.cFragment.targetCount = 0;
+
+ // Success case: a render pipeline with just a storage attachment is valid.
+ device.CreateRenderPipeline(&pDesc);
+
+ // Error case: a render pass with PLS but no attachments is invalid.
+ plPlsDesc.storageAttachmentCount = 0;
+ pDesc.layout = device.CreatePipelineLayout(&plDesc);
+ ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&pDesc));
+}
+
+// Check that the size of the render pass is correctly deduced when there is only a storage
+// attachment. Use the SetViewport validation to check this.
+TEST_F(PixelLocalStorageTest, RenderPassSizeDetectionWithOnlyStorageAttachment) {
+ wgpu::TextureDescriptor tDesc;
+ tDesc.format = wgpu::TextureFormat::R32Uint;
+ tDesc.size = {7, 11};
+ tDesc.usage = wgpu::TextureUsage::StorageAttachment;
+ wgpu::Texture tex = device.CreateTexture(&tDesc);
+
+ wgpu::RenderPassStorageAttachment rpAttachment;
+ rpAttachment.storage = tex.CreateView();
+ rpAttachment.offset = 0;
+ rpAttachment.loadOp = wgpu::LoadOp::Load;
+ rpAttachment.storeOp = wgpu::StoreOp::Store;
+
+ wgpu::RenderPassPixelLocalStorage rpPlsDesc;
+ rpPlsDesc.totalPixelLocalStorageSize = 4;
+ rpPlsDesc.storageAttachmentCount = 1;
+ rpPlsDesc.storageAttachments = &rpAttachment;
+
+ wgpu::RenderPassDescriptor rpDesc;
+ rpDesc.nextInChain = &rpPlsDesc;
+ rpDesc.colorAttachmentCount = 0;
+ rpDesc.depthStencilAttachment = nullptr;
+
+ // Success case: viewport is exactly the size of the render pass.
+ {
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rpDesc);
+ pass.SetViewport(0, 0, tDesc.size.width, tDesc.size.height, 0.0, 1.0);
+ pass.End();
+ encoder.Finish();
+ }
+
+ // Error case: viewport width is larger than the render pass's.
+ {
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rpDesc);
+ pass.SetViewport(0, 0, tDesc.size.width + 1, tDesc.size.height, 0.0, 1.0);
+ pass.End();
+ ASSERT_DEVICE_ERROR(encoder.Finish());
+ }
+
+ // Error case: viewport width is larger than the render pass's.
+ {
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rpDesc);
+ pass.SetViewport(0, 0, tDesc.size.width, tDesc.size.height + 1, 0.0, 1.0);
+ pass.End();
+ ASSERT_DEVICE_ERROR(encoder.Finish());
+ }
+}
+
class PixelLocalStorageAndRenderToSingleSampledTest : public PixelLocalStorageTest {
protected:
WGPUDevice CreateTestDevice(native::Adapter dawnAdapter,
wgpu::DeviceDescriptor descriptor) override {
- // TODO(dawn:1704): Do we need to test both extensions?
wgpu::FeatureName requiredFeatures[2] = {wgpu::FeatureName::PixelLocalStorageNonCoherent,
wgpu::FeatureName::MSAARenderToSingleSampled};
descriptor.requiredFeatures = requiredFeatures;
@@ -1056,7 +1199,32 @@
ASSERT_DEVICE_ERROR(RecordRenderPass(&desc.rpDesc));
}
+class PixelLocalStorageAndTransientAttachmentTest : public PixelLocalStorageTest {
+ WGPUDevice CreateTestDevice(native::Adapter dawnAdapter,
+ wgpu::DeviceDescriptor descriptor) override {
+ wgpu::FeatureName requiredFeatures[2] = {wgpu::FeatureName::PixelLocalStorageNonCoherent,
+ wgpu::FeatureName::TransientAttachments};
+ descriptor.requiredFeatures = requiredFeatures;
+ descriptor.requiredFeatureCount = 2;
+ return dawnAdapter.CreateDevice(&descriptor);
+ }
+};
+
+// Check that a transient + storage attachment is allowed.
+TEST_F(PixelLocalStorageAndTransientAttachmentTest, TransientStorageAttachment) {
+ wgpu::TextureDescriptor desc;
+ desc.size = {1, 1};
+ desc.format = wgpu::TextureFormat::R32Uint;
+ desc.usage = wgpu::TextureUsage::StorageAttachment | wgpu::TextureUsage::TransientAttachment;
+ device.CreateTexture(&desc);
+
+ desc.usage |= wgpu::TextureUsage::RenderAttachment;
+ device.CreateTexture(&desc);
+}
+
// TODO(dawn:1704): Add tests for limits
+// TODO(dawn:1704): Add tests for load/store op validation with transient.
+// TODO(dawn:1704): Allow multisampled storage attachments
} // anonymous namespace
} // namespace dawn
