[Vulkan] Properly handle combined image samplers
We currently set YCbCr sampler entries as COMBINED_IMAGE_SAMPLER at the
Vulkan level, but we don't actually then handle that within the Vulkan
backend:
* There should be no entry in the VkDescriptorSetLayout for the texture
that will be paired with this sampler
* That texture should be bound in the VkDescriptorSet at the index for
the sampler
* Within the SPIR-V, the texture var should be decorated to point at the
binding for the combined image sampler
This CL fills in those holes. To do so, we add an optional field to
StaticSamplerBindingLayout wherein the client can specify a texture
entry that should be paired with this sampler. If that field is set, we
will create the sampler entry as COMBINED_IMAGE_SAMPLER with all that
that entails.
Followup will add validation and testing. In addition to general testing
and validation, we will validate and test the following with respect to
YCbCr samplers in particular:
* The client *must* pass the index of the texture being sampled by a
YCbCr sampler entry as part of the YCbCr sampler entry itself, in
essence requiring that the client pair YCbCr samplers and sampled
textures. This requirement matches the current Skia BGL structure and
seems like a rational restriction, as both the YCbCr sampler and the
texture being sampled by that sampler need to take their YCbCr info from
the AHB that is backing by the texture.
Change-Id: I1741f4bcf90a461a3636fd37745a80b461365006
Bug: 41488897
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/197475
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
diff --git a/src/dawn/dawn.json b/src/dawn/dawn.json
index 43adfa0..9823f4f 100644
--- a/src/dawn/dawn.json
+++ b/src/dawn/dawn.json
@@ -453,7 +453,8 @@
"chain roots": ["bind group layout entry"],
"tags": ["dawn"],
"members": [
- {"name": "sampler", "type": "sampler"}
+ {"name": "sampler", "type": "sampler"},
+ {"name": "sampled texture binding", "type": "uint32_t", "default": "WGPU_LIMIT_U32_UNDEFINED" }
]
},
"texture sample type": {
diff --git a/src/dawn/native/BindingInfo.cpp b/src/dawn/native/BindingInfo.cpp
index a2e997a..1fb2b05 100644
--- a/src/dawn/native/BindingInfo.cpp
+++ b/src/dawn/native/BindingInfo.cpp
@@ -261,7 +261,9 @@
: type(apiLayout.type) {}
StaticSamplerBindingInfo::StaticSamplerBindingInfo(const StaticSamplerBindingLayout& apiLayout)
- : sampler(apiLayout.sampler) {}
+ : sampler(apiLayout.sampler),
+ sampledTextureBinding(BindingNumber{apiLayout.sampledTextureBinding}),
+ isUsedForSingleTextureBinding(apiLayout.sampledTextureBinding < WGPU_LIMIT_U32_UNDEFINED) {}
InputAttachmentBindingInfo::InputAttachmentBindingInfo() = default;
InputAttachmentBindingInfo::InputAttachmentBindingInfo(wgpu::TextureSampleType sampleType)
diff --git a/src/dawn/native/BindingInfo.h b/src/dawn/native/BindingInfo.h
index a43f4f9..b094f2b 100644
--- a/src/dawn/native/BindingInfo.h
+++ b/src/dawn/native/BindingInfo.h
@@ -117,6 +117,11 @@
// Holds a ref instead of an unowned pointer.
Ref<SamplerBase> sampler;
+ // Holds the BindingNumber of the single texture with which this sampler is
+ // statically paired, if any.
+ BindingNumber sampledTextureBinding;
+ // Whether this instance is statically paired with a single texture.
+ bool isUsedForSingleTextureBinding = false;
};
// A mirror of wgpu::ExternalTextureBindingLayout for use inside dawn::native.
diff --git a/src/dawn/native/stream/Stream.h b/src/dawn/native/stream/Stream.h
index 070e49c..ab2838b 100644
--- a/src/dawn/native/stream/Stream.h
+++ b/src/dawn/native/stream/Stream.h
@@ -33,6 +33,7 @@
#include <functional>
#include <limits>
#include <unordered_map>
+#include <unordered_set>
#include <utility>
#include <vector>
@@ -344,6 +345,31 @@
}
};
+// Stream specialization for std::unordered_set<V> which sorts the entries
+// to provide a stable ordering.
+template <typename V>
+class Stream<std::unordered_set<V>> {
+ public:
+ static void Write(stream::Sink* sink, const std::unordered_set<V>& s) {
+ std::vector<V> ordered(s.begin(), s.end());
+ std::sort(ordered.begin(), ordered.end(), [](const V& a, const V& b) { return a < b; });
+ StreamIn(sink, ordered);
+ }
+ static MaybeError Read(Source* source, std::unordered_set<V>* s) {
+ using SizeT = decltype(std::declval<std::vector<V>>().size());
+ SizeT size;
+ DAWN_TRY(StreamOut(source, &size));
+ *s = {};
+ s->reserve(size);
+ for (SizeT i = 0; i < size; ++i) {
+ V v;
+ DAWN_TRY(StreamOut(source, &v));
+ s->insert(std::move(v));
+ }
+ return {};
+ }
+};
+
// Helper class to contain the begin/end iterators of an iterable.
namespace detail {
template <typename Iterator>
diff --git a/src/dawn/native/vulkan/BindGroupLayoutVk.cpp b/src/dawn/native/vulkan/BindGroupLayoutVk.cpp
index e75a743..42639ee 100644
--- a/src/dawn/native/vulkan/BindGroupLayoutVk.cpp
+++ b/src/dawn/native/vulkan/BindGroupLayoutVk.cpp
@@ -32,6 +32,7 @@
#include "absl/container/flat_hash_map.h"
#include "dawn/common/BitSetIterator.h"
#include "dawn/common/MatchVariant.h"
+#include "dawn/common/Range.h"
#include "dawn/common/ityp_vector.h"
#include "dawn/native/CacheKey.h"
#include "dawn/native/vulkan/DescriptorSetAllocator.h"
@@ -88,11 +89,11 @@
},
[](const SamplerBindingInfo&) { return VK_DESCRIPTOR_TYPE_SAMPLER; },
[](const StaticSamplerBindingInfo& layout) {
- // By the Vulkan spec, YCbCr samplers must have descriptor type
- // COMBINED_IMAGE_SAMPLER:
- // https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkSamplerYcbcrConversionInfo.html
- return (layout.sampler->IsYCbCr()) ? VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER
- : VK_DESCRIPTOR_TYPE_SAMPLER;
+ // Make this entry into a combined image sampler iff the client
+ // specified a single texture binding to be paired with it.
+ return (layout.isUsedForSingleTextureBinding)
+ ? VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER
+ : VK_DESCRIPTOR_TYPE_SAMPLER;
},
[](const TextureBindingInfo&) { return VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE; },
[](const StorageTextureBindingInfo&) { return VK_DESCRIPTOR_TYPE_STORAGE_IMAGE; },
@@ -115,7 +116,33 @@
ityp::vector<BindingIndex, VkDescriptorSetLayoutBinding> bindings;
bindings.reserve(GetBindingCount());
+ // Build the mapping from the indices of textures that will be paired with
+ // static samplers at the Vk level in combined image sampler entries to
+ // their respective sampler indices.
+ for (BindingIndex bindingIndex : Range(GetBindingCount())) {
+ const BindingInfo& bindingInfo = GetBindingInfo(bindingIndex);
+ if (!std::holds_alternative<StaticSamplerBindingInfo>(bindingInfo.bindingLayout)) {
+ continue;
+ }
+
+ auto samplerLayout = std::get<StaticSamplerBindingInfo>(bindingInfo.bindingLayout);
+ if (!samplerLayout.isUsedForSingleTextureBinding) {
+ // The client did not specify that this sampler should be paired
+ // with a single texture binding.
+ continue;
+ }
+
+ mTextureToStaticSamplerIndices[GetBindingIndex(samplerLayout.sampledTextureBinding)] =
+ bindingIndex;
+ }
+
for (const auto& [_, bindingIndex] : GetBindingMap()) {
+ if (mTextureToStaticSamplerIndices.contains(bindingIndex)) {
+ // This texture will be bound into the VkDescriptorSet at the index
+ // for the sampler itself.
+ continue;
+ }
+
const BindingInfo& bindingInfo = GetBindingInfo(bindingIndex);
VkDescriptorSetLayoutBinding vkBinding;
@@ -154,6 +181,12 @@
absl::flat_hash_map<VkDescriptorType, uint32_t> descriptorCountPerType;
for (BindingIndex bindingIndex{0}; bindingIndex < GetBindingCount(); ++bindingIndex) {
+ if (mTextureToStaticSamplerIndices.contains(bindingIndex)) {
+ // This texture will be bound into the VkDescriptorSet at the index
+ // for the sampler itself.
+ continue;
+ }
+
VkDescriptorType vulkanType = VulkanDescriptorType(GetBindingInfo(bindingIndex));
// absl:flat_hash_map::operator[] will return 0 if the key doesn't exist.
@@ -213,6 +246,14 @@
mBindGroupAllocator->Deallocate(bindGroup);
}
+std::optional<BindingIndex> BindGroupLayout::GetStaticSamplerIndexForTexture(
+ BindingIndex textureBinding) const {
+ if (mTextureToStaticSamplerIndices.contains(textureBinding)) {
+ return mTextureToStaticSamplerIndices.at(textureBinding);
+ }
+ return {};
+}
+
void BindGroupLayout::SetLabelImpl() {
SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_BindGroupLayout", GetLabel());
}
diff --git a/src/dawn/native/vulkan/BindGroupLayoutVk.h b/src/dawn/native/vulkan/BindGroupLayoutVk.h
index 71c0dea..0ac799c 100644
--- a/src/dawn/native/vulkan/BindGroupLayoutVk.h
+++ b/src/dawn/native/vulkan/BindGroupLayoutVk.h
@@ -74,6 +74,11 @@
void DeallocateBindGroup(BindGroup* bindGroup,
DescriptorSetAllocation* descriptorSetAllocation);
+ // If the client specified that the texture at `textureBinding` should be
+ // combined with a static sampler, returns the binding index of the static
+ // sampler that is sampling this texture.
+ std::optional<BindingIndex> GetStaticSamplerIndexForTexture(BindingIndex textureBinding) const;
+
private:
~BindGroupLayout() override;
MaybeError Initialize();
@@ -82,6 +87,10 @@
// Dawn API
void SetLabelImpl() override;
+ // Maps from indices of texture entries that are paired with static samplers
+ // to indices of the entries of their respective samplers.
+ absl::flat_hash_map<BindingIndex, BindingIndex> mTextureToStaticSamplerIndices;
+
VkDescriptorSetLayout mHandle = VK_NULL_HANDLE;
MutexProtected<SlabAllocator<BindGroup>> mBindGroupAllocator;
diff --git a/src/dawn/native/vulkan/BindGroupVk.cpp b/src/dawn/native/vulkan/BindGroupVk.cpp
index 84ff03a..f2be2a6 100644
--- a/src/dawn/native/vulkan/BindGroupVk.cpp
+++ b/src/dawn/native/vulkan/BindGroupVk.cpp
@@ -120,6 +120,20 @@
// resources.
return false;
}
+
+ // TODO(crbug.com/41488897: Add GetVkDescriptorSet{Index,
+ // Type}(BindingIndex) functions to BindGroupLayoutVk that
+ // access vectors holding entries for all BGL entries and
+ // eliminate this special-case code in favor of calling those
+ // functions to assign `dstBinding` and `descriptorType` above.
+ if (auto samplerIndex =
+ ToBackend(GetLayout())->GetStaticSamplerIndexForTexture(bindingIndex)) {
+ // Write the info of the texture at the binding index for the
+ // sampler.
+ write.dstBinding = static_cast<uint32_t>(samplerIndex.value());
+ write.descriptorType = VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER;
+ }
+
writeImageInfo[numWrites].imageView = handle;
writeImageInfo[numWrites].imageLayout = VulkanImageLayout(
view->GetTexture()->GetFormat(), wgpu::TextureUsage::TextureBinding);
diff --git a/src/dawn/native/vulkan/ShaderModuleVk.cpp b/src/dawn/native/vulkan/ShaderModuleVk.cpp
index 2aa94bd..746115a 100644
--- a/src/dawn/native/vulkan/ShaderModuleVk.cpp
+++ b/src/dawn/native/vulkan/ShaderModuleVk.cpp
@@ -30,6 +30,7 @@
#include <cstdint>
#include <map>
#include <string>
+#include <unordered_set>
#include <vector>
#include "absl/container/flat_hash_map.h"
@@ -240,6 +241,7 @@
// Creation of module and spirv is deferred to this point when using tint generator
tint::spirv::writer::Bindings bindings;
+ std::unordered_set<tint::BindingPoint> statically_paired_texture_binding_points;
const BindingInfoArray& moduleBindingInfo =
GetEntryPoint(programmableStage.entryPoint.c_str()).bindings;
@@ -288,6 +290,11 @@
dstBindingPoint.group, dstBindingPoint.binding});
},
[&](const TextureBindingInfo& bindingInfo) {
+ if (auto samplerIndex = bgl->GetStaticSamplerIndexForTexture(
+ BindingIndex{dstBindingPoint.binding})) {
+ dstBindingPoint.binding = static_cast<uint32_t>(samplerIndex.value());
+ statically_paired_texture_binding_points.insert(srcBindingPoint);
+ }
bindings.texture.emplace(srcBindingPoint,
tint::spirv::writer::binding::Texture{
dstBindingPoint.group, dstBindingPoint.binding});
@@ -341,7 +348,8 @@
req.platform = UnsafeUnkeyedValue(GetDevice()->GetPlatform());
req.substituteOverrideConfig = std::move(substituteOverrideConfig);
req.maxSubgroupSizeForFullSubgroups = maxSubgroupSizeForFullSubgroups;
-
+ req.tintOptions.statically_paired_texture_binding_points =
+ std::move(statically_paired_texture_binding_points);
req.tintOptions.clamp_frag_depth = clampFragDepth;
req.tintOptions.disable_robustness = !GetDevice()->IsRobustnessEnabled();
req.tintOptions.emit_vertex_point_size = emitPointSize;
diff --git a/src/tint/lang/spirv/writer/common/option_helper.cc b/src/tint/lang/spirv/writer/common/option_helper.cc
index bfc2a6d..c3ffcc2 100644
--- a/src/tint/lang/spirv/writer/common/option_helper.cc
+++ b/src/tint/lang/spirv/writer/common/option_helper.cc
@@ -56,10 +56,14 @@
return false;
};
- auto spirv_seen = [&diagnostics, &seen_spirv_bindings](const binding::BindingInfo& src,
- const tint::BindingPoint& dst) -> bool {
+ const auto& statically_paired_texture_binding_points =
+ options.statically_paired_texture_binding_points;
+ auto spirv_seen = [&diagnostics, &seen_spirv_bindings,
+ &statically_paired_texture_binding_points](
+ const binding::BindingInfo& src, const tint::BindingPoint& dst) -> bool {
if (auto binding = seen_spirv_bindings.Get(src)) {
- if (*binding != dst) {
+ if (*binding != dst && !statically_paired_texture_binding_points.count(*binding) &&
+ !statically_paired_texture_binding_points.count(dst)) {
diagnostics.AddError(Source{})
<< "found duplicate SPIR-V binding point: [group: " << src.group
<< ", binding: " << src.binding << "]";
diff --git a/src/tint/lang/spirv/writer/common/options.h b/src/tint/lang/spirv/writer/common/options.h
index 616e28e..9af79ab 100644
--- a/src/tint/lang/spirv/writer/common/options.h
+++ b/src/tint/lang/spirv/writer/common/options.h
@@ -29,6 +29,7 @@
#define SRC_TINT_LANG_SPIRV_WRITER_COMMON_OPTIONS_H_
#include <unordered_map>
+#include <unordered_set>
#include "src/tint/api/common/binding_point.h"
#include "src/tint/utils/reflection/reflection.h"
@@ -131,6 +132,12 @@
/// The bindings
Bindings bindings;
+ // BindingPoints for textures that are paired with static samplers in the
+ // BGL. These BindingPoints are the only ones that are allowed to map to
+ // duplicate spir-v bindings, since they must map to the spir-v bindings of
+ // the samplers with which they are paired.
+ std::unordered_set<BindingPoint> statically_paired_texture_binding_points = {};
+
/// Set to `true` to disable software robustness that prevents out-of-bounds accesses.
bool disable_robustness = false;
@@ -174,6 +181,7 @@
/// Reflect the fields of this class so that it can be used by tint::ForeachField()
TINT_REFLECT(Options,
bindings,
+ statically_paired_texture_binding_points,
disable_robustness,
disable_image_robustness,
disable_runtime_sized_array_index_clamping,
