From c1ea3d9b8e292f734a96bb32b52e13da0046a84b Mon Sep 17 00:00:00 2001 From: Greg Daniel Date: Wed, 27 Jan 2021 10:21:21 -0500 Subject: [PATCH] Add debug checks to enforce gpu buffer access patterns. Specifically make sure it is true that we only write to static and stream access buffers once. Bug: skia:11226 Change-Id: I566a095093e884075bc4bee07ba1101e772e3332 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/360476 Reviewed-by: Jim Van Verth Commit-Queue: Greg Daniel --- src/gpu/GrGpuBuffer.cpp | 13 ++++++++++++- src/gpu/GrGpuBuffer.h | 6 ++++++ src/gpu/vk/GrVkBuffer.cpp | 3 ++- src/gpu/vk/GrVkGpu.cpp | 4 ++-- src/gpu/vk/GrVkTransferBuffer.cpp | 11 +++++++---- src/gpu/vk/GrVkTransferBuffer.h | 5 +++-- 6 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/gpu/GrGpuBuffer.cpp b/src/gpu/GrGpuBuffer.cpp index 64e1167723..71efe88688 100644 --- a/src/gpu/GrGpuBuffer.cpp +++ b/src/gpu/GrGpuBuffer.cpp @@ -21,6 +21,7 @@ void* GrGpuBuffer::map() { if (this->wasDestroyed()) { return nullptr; } + SkASSERT(!fHasWrittenToBuffer || fAccessPattern == kDynamic_GrAccessPattern); if (!fMapPtr) { this->onMap(); } @@ -34,17 +35,27 @@ void GrGpuBuffer::unmap() { SkASSERT(fMapPtr); this->onUnmap(); fMapPtr = nullptr; +#ifdef SK_DEBUG + fHasWrittenToBuffer = true; +#endif } bool GrGpuBuffer::isMapped() const { return SkToBool(fMapPtr); } bool GrGpuBuffer::updateData(const void* src, size_t srcSizeInBytes) { + SkASSERT(!fHasWrittenToBuffer || fAccessPattern == kDynamic_GrAccessPattern); SkASSERT(!this->isMapped()); SkASSERT(srcSizeInBytes <= fSizeInBytes); if (this->intendedType() == GrGpuBufferType::kXferGpuToCpu) { return false; } - return this->onUpdateData(src, srcSizeInBytes); + bool result = this->onUpdateData(src, srcSizeInBytes); +#ifdef SK_DEBUG + if (result) { + fHasWrittenToBuffer = true; + } +#endif + return result; } void GrGpuBuffer::ComputeScratchKeyForDynamicVBO(size_t size, GrGpuBufferType intendedType, diff --git a/src/gpu/GrGpuBuffer.h b/src/gpu/GrGpuBuffer.h index 0e9a8f7369..73cb31d23f 100644 --- a/src/gpu/GrGpuBuffer.h +++ b/src/gpu/GrGpuBuffer.h @@ -98,6 +98,12 @@ private: size_t fSizeInBytes; GrAccessPattern fAccessPattern; GrGpuBufferType fIntendedType; + +#ifdef SK_DEBUG + // Static and stream access buffers are only ever written to once. This is used to track that + // and assert it is true. + bool fHasWrittenToBuffer = false; +#endif }; #endif diff --git a/src/gpu/vk/GrVkBuffer.cpp b/src/gpu/vk/GrVkBuffer.cpp index b623071011..d8b43621d5 100644 --- a/src/gpu/vk/GrVkBuffer.cpp +++ b/src/gpu/vk/GrVkBuffer.cpp @@ -197,7 +197,8 @@ void GrVkBuffer::copyCpuDataToGpuBuffer(GrVkGpu* gpu, const void* src, size_t si gpu->updateBuffer(this, src, this->offset(), size); } else { sk_sp transferBuffer = - GrVkTransferBuffer::Make(gpu, size, GrVkBuffer::kCopyRead_Type); + GrVkTransferBuffer::Make(gpu, size, GrVkBuffer::kCopyRead_Type, + kStream_GrAccessPattern); if (!transferBuffer) { return; } diff --git a/src/gpu/vk/GrVkGpu.cpp b/src/gpu/vk/GrVkGpu.cpp index 08e512d92f..11497fa00b 100644 --- a/src/gpu/vk/GrVkGpu.cpp +++ b/src/gpu/vk/GrVkGpu.cpp @@ -423,12 +423,12 @@ sk_sp GrVkGpu::onCreateBuffer(size_t size, GrGpuBufferType type, case GrGpuBufferType::kXferCpuToGpu: SkASSERT(kDynamic_GrAccessPattern == accessPattern || kStream_GrAccessPattern == accessPattern); - buff = GrVkTransferBuffer::Make(this, size, GrVkBuffer::kCopyRead_Type); + buff = GrVkTransferBuffer::Make(this, size, GrVkBuffer::kCopyRead_Type, accessPattern); break; case GrGpuBufferType::kXferGpuToCpu: SkASSERT(kDynamic_GrAccessPattern == accessPattern || kStream_GrAccessPattern == accessPattern); - buff = GrVkTransferBuffer::Make(this, size, GrVkBuffer::kCopyWrite_Type); + buff = GrVkTransferBuffer::Make(this, size, GrVkBuffer::kCopyWrite_Type, accessPattern); break; default: SK_ABORT("Unknown buffer type."); diff --git a/src/gpu/vk/GrVkTransferBuffer.cpp b/src/gpu/vk/GrVkTransferBuffer.cpp index d5401d08e4..3e3025835c 100644 --- a/src/gpu/vk/GrVkTransferBuffer.cpp +++ b/src/gpu/vk/GrVkTransferBuffer.cpp @@ -10,7 +10,8 @@ #include "src/gpu/vk/GrVkTransferBuffer.h" sk_sp GrVkTransferBuffer::Make(GrVkGpu* gpu, size_t size, - GrVkBuffer::Type type) { + GrVkBuffer::Type type, + GrAccessPattern access) { GrVkBuffer::Desc desc; desc.fDynamic = true; SkASSERT(GrVkBuffer::kCopyRead_Type == type || GrVkBuffer::kCopyWrite_Type == type); @@ -22,19 +23,21 @@ sk_sp GrVkTransferBuffer::Make(GrVkGpu* gpu, size_t size, return nullptr; } - GrVkTransferBuffer* buffer = new GrVkTransferBuffer(gpu, desc, bufferResource); + GrVkTransferBuffer* buffer = new GrVkTransferBuffer(gpu, desc, access, bufferResource); if (!buffer) { bufferResource->unref(); } return sk_sp(buffer); } -GrVkTransferBuffer::GrVkTransferBuffer(GrVkGpu* gpu, const GrVkBuffer::Desc& desc, +GrVkTransferBuffer::GrVkTransferBuffer(GrVkGpu* gpu, + const GrVkBuffer::Desc& desc, + GrAccessPattern access, const GrVkBuffer::Resource* bufferResource) : INHERITED(gpu, desc.fSizeInBytes, kCopyRead_Type == desc.fType ? GrGpuBufferType::kXferCpuToGpu : GrGpuBufferType::kXferGpuToCpu, - kStream_GrAccessPattern) + access) , GrVkBuffer(desc, bufferResource) { this->registerWithCache(SkBudgeted::kYes); } diff --git a/src/gpu/vk/GrVkTransferBuffer.h b/src/gpu/vk/GrVkTransferBuffer.h index c1c25a6d06..d3c61c385a 100644 --- a/src/gpu/vk/GrVkTransferBuffer.h +++ b/src/gpu/vk/GrVkTransferBuffer.h @@ -16,14 +16,15 @@ class GrVkGpu; class GrVkTransferBuffer : public GrGpuBuffer, public GrVkBuffer { public: - static sk_sp Make(GrVkGpu* gpu, size_t size, GrVkBuffer::Type type); + static sk_sp Make(GrVkGpu* gpu, size_t size, GrVkBuffer::Type type, + GrAccessPattern access); protected: void onAbandon() override; void onRelease() override; private: - GrVkTransferBuffer(GrVkGpu* gpu, const GrVkBuffer::Desc& desc, + GrVkTransferBuffer(GrVkGpu* gpu, const GrVkBuffer::Desc& desc, GrAccessPattern access, const GrVkBuffer::Resource* resource); void setMemoryBacking(SkTraceMemoryDump* traceMemoryDump, const SkString& dumpName) const override;