From 87a1724368e9eeec6a69737e7ef337ccf1a9e636 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Wed, 14 Dec 2022 23:44:11 +0000 Subject: [PATCH] Improve draw-time bind group validation messages Makes it clear that the index being reported by these messages is the group index and not the binding number. This was a point of confusion in on the bug. Additionally, adds more information to the error message regarding buffer sizes being too small for the current pipeline. Now includes the pipeline name and buffer size as well as the minimum required size. Also includes a note explaining that uniform buffer bindings must be a multiple of 16. (This recently changed and cause several existing samples to break for non-obvious reasons.) The error message still does not contain the buffer or binding number, which would be helpful. This is because we currently lack a way to look up the binding index from the packed index that this error is generated with. Bug: dawn:1604 Change-Id: Ibb2b44bc9e1583ddef34d703e83bcf64ed7a3aa2 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/113602 Commit-Queue: Brandon Jones Auto-Submit: Brandon Jones Reviewed-by: Kai Ninomiya Kokoro: Kokoro --- src/dawn/native/CommandBufferStateTracker.cpp | 54 +++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/src/dawn/native/CommandBufferStateTracker.cpp b/src/dawn/native/CommandBufferStateTracker.cpp index 6677398ef2..872196d418 100644 --- a/src/dawn/native/CommandBufferStateTracker.cpp +++ b/src/dawn/native/CommandBufferStateTracker.cpp @@ -14,6 +14,8 @@ #include "dawn/native/CommandBufferStateTracker.h" +#include + #include "dawn/common/Assert.h" #include "dawn/common/BitSetIterator.h" #include "dawn/native/BindGroup.h" @@ -31,17 +33,21 @@ namespace dawn::native { namespace { -bool BufferSizesAtLeastAsBig(const ityp::span unverifiedBufferSizes, - const std::vector& pipelineMinBufferSizes) { +// Returns nullopt if all buffers in unverifiedBufferSizes are at least large enough to satisfy the +// minimum listed in pipelineMinBufferSizes, or the index of the first detected failing buffer +// otherwise. +std::optional FindFirstUndersizedBuffer( + const ityp::span unverifiedBufferSizes, + const std::vector& pipelineMinBufferSizes) { ASSERT(unverifiedBufferSizes.size() == pipelineMinBufferSizes.size()); for (uint32_t i = 0; i < unverifiedBufferSizes.size(); ++i) { if (unverifiedBufferSizes[i] < pipelineMinBufferSizes[i]) { - return false; + return i; } } - return true; + return std::nullopt; } } // namespace @@ -234,8 +240,9 @@ void CommandBufferStateTracker::RecomputeLazyAspects(ValidationAspects aspects) for (BindGroupIndex i : IterateBitSet(mLastPipelineLayout->GetBindGroupLayoutsMask())) { if (mBindgroups[i] == nullptr || mLastPipelineLayout->GetBindGroupLayout(i) != mBindgroups[i]->GetLayout() || - !BufferSizesAtLeastAsBig(mBindgroups[i]->GetUnverifiedBufferSizes(), - (*mMinBufferSizes)[i])) { + FindFirstUndersizedBuffer(mBindgroups[i]->GetUnverifiedBufferSizes(), + (*mMinBufferSizes)[i]) + .has_value()) { matches = false; break; } @@ -315,7 +322,7 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe for (BindGroupIndex i : IterateBitSet(mLastPipelineLayout->GetBindGroupLayoutsMask())) { ASSERT(HasPipeline()); - DAWN_INVALID_IF(mBindgroups[i] == nullptr, "No bind group set at index %u.", + DAWN_INVALID_IF(mBindgroups[i] == nullptr, "No bind group set at group index %u.", static_cast(i)); BindGroupLayoutBase* requiredBGL = mLastPipelineLayout->GetBindGroupLayout(i); @@ -326,8 +333,8 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe currentBGL->GetPipelineCompatibilityToken() != requiredBGL->GetPipelineCompatibilityToken(), "The current pipeline (%s) was created with a default layout, and is not " - "compatible with the %s at index %u which uses a %s that was not created by " - "the pipeline. Either use the bind group layout returned by calling " + "compatible with the %s set at group index %u which uses a %s that was not " + "created by the pipeline. Either use the bind group layout returned by calling " "getBindGroupLayout(%u) on the pipeline when creating the bind group, or " "provide an explicit pipeline layout when creating the pipeline.", mLastPipeline, mBindgroups[i], static_cast(i), currentBGL, @@ -336,8 +343,9 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe DAWN_INVALID_IF( requiredBGL->GetPipelineCompatibilityToken() == PipelineCompatibilityToken(0) && currentBGL->GetPipelineCompatibilityToken() != PipelineCompatibilityToken(0), - "%s at index %u uses a %s which was created as part of the default layout for " - "a different pipeline than the current one (%s), and as a result is not " + "%s set at group index %u uses a %s which was created as part of the default " + "layout " + "for a different pipeline than the current one (%s), and as a result is not " "compatible. Use an explicit bind group layout when creating bind groups and " "an explicit pipeline layout when creating pipelines to share bind groups " "between pipelines.", @@ -346,15 +354,27 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe DAWN_INVALID_IF( mLastPipelineLayout->GetBindGroupLayout(i) != mBindgroups[i]->GetLayout(), "Bind group layout %s of pipeline layout %s does not match layout %s of bind " - "group %s at index %u.", + "group %s set at group index %u.", requiredBGL, mLastPipelineLayout, currentBGL, mBindgroups[i], static_cast(i)); - // TODO(dawn:563): Report the binding sizes and which ones are failing. - DAWN_INVALID_IF(!BufferSizesAtLeastAsBig(mBindgroups[i]->GetUnverifiedBufferSizes(), - (*mMinBufferSizes)[i]), - "Binding sizes are too small for bind group %s at index %u", - mBindgroups[i], static_cast(i)); + // TODO(dawn:563): Report which buffer bindings are failing. This requires the ability + // to look up the binding index from the packed index. + std::optional packedIndex = FindFirstUndersizedBuffer( + mBindgroups[i]->GetUnverifiedBufferSizes(), (*mMinBufferSizes)[i]); + if (packedIndex.has_value()) { + uint64_t bufferSize = + mBindgroups[i]->GetUnverifiedBufferSizes()[packedIndex.value()]; + uint64_t minBufferSize = (*mMinBufferSizes)[i][packedIndex.value()]; + return DAWN_VALIDATION_ERROR( + "Binding sizes are too small for %s set at group index %u. A bound buffer " + "contained %u bytes, but the current pipeline (%s) requires a buffer which is " + "at least %u bytes. (Note that uniform buffer bindings must be a multiple of " + "16 bytes, and as a result may be larger than the associated data in the " + "shader source.)", + mBindgroups[i], static_cast(i), bufferSize, mLastPipeline, + minBufferSize); + } } // The chunk of code above should be similar to the one in |RecomputeLazyAspects|.