[graphite] Consolidate upload code around UploadSource The two UploadInstance factories were nearly identical, accounting for some assumptions around subrects vs. full rects and adding an identity TextureFormatXferFn that can run over compressed data. This makes UploadInstance created from an UploadSource, and makes UploadSource the arbiter of whether or not a transfer is valid (no more passing around duplicate parameters). UploadSource is also now the sole handler of the upload-on-host code path, and it no longer magically happens when appending to any UploadList. There are distinct times when upload-on-host makes sense and others where we really do want the task to be in the graph and the Recording. This also patches up a race condition that could have happened in Graphite-Vulkan by requiring that the TextureProxy be uniquely held and shareability of the resource won't have it aliased to other proxies. Technically this was low risk anyways since we require that the image layout of the texture be unknown, which really only happens if we haven't used the texture yet for anything. Bug: b/333909822 Change-Id: I4a32c7034775ad96fd43cc4dc2839e0a59c03bab Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1221678 Commit-Queue: Michael Ludwig <michaelludwig@google.com> Reviewed-by: Nicolette Prevost <nicolettep@google.com> Reviewed-by: Robert Phillips <robertphillips@google.com>
diff --git a/src/gpu/graphite/Device.cpp b/src/gpu/graphite/Device.cpp index 9aaa508..b56265c 100644 --- a/src/gpu/graphite/Device.cpp +++ b/src/gpu/graphite/Device.cpp
@@ -784,16 +784,7 @@ this->imageInfo().colorInfo(), levels, dstRect); - if (!uploadSource.isValid()) { - return false; - } - return fDC->recordUpload(fRecorder, - fDC->target(), - src.info().colorInfo(), - this->imageInfo().colorInfo(), - uploadSource, - dstRect, - nullptr); + return fDC->recordUpload(fRecorder, uploadSource); }
diff --git a/src/gpu/graphite/DrawAtlas.cpp b/src/gpu/graphite/DrawAtlas.cpp index 5287de6..d8e13b2 100644 --- a/src/gpu/graphite/DrawAtlas.cpp +++ b/src/gpu/graphite/DrawAtlas.cpp
@@ -189,16 +189,7 @@ const UploadSource uploadSource = UploadSource::Make( recorder->priv().caps(), view, colorInfo, colorInfo, levels, dstRect); - if (!uploadSource.isValid()) { - return false; - } - if (!dc->recordUpload(recorder, - view, - colorInfo, - colorInfo, - uploadSource, - dstRect, - /*ConditionalUploadContext=*/nullptr)) { + if (!dc->recordUpload(recorder, uploadSource)) { return false; } }
diff --git a/src/gpu/graphite/DrawContext.cpp b/src/gpu/graphite/DrawContext.cpp index a4740a1..bdafa91 100644 --- a/src/gpu/graphite/DrawContext.cpp +++ b/src/gpu/graphite/DrawContext.cpp
@@ -188,23 +188,11 @@ } bool DrawContext::recordUpload(Recorder* recorder, - const TextureProxyView& dstView, - const SkColorInfo& srcColorInfo, - const SkColorInfo& dstColorInfo, const UploadSource& source, - const SkIRect& dstRect, std::unique_ptr<ConditionalUploadContext> condContext) { - // Our caller should have clipped to the bounds of the surface already. - SkASSERT(dstView.proxy()->isFullyLazy() || - SkIRect::MakeSize(dstView.dimensions()).contains(dstRect)); - SkASSERT(source.isValid()); - return fPendingUploads->recordUpload(recorder, - std::move(dstView), - srcColorInfo, - dstColorInfo, - source, - dstRect, - std::move(condContext)); + // Since this upload is inline with the DrawContext's tasks, we do not attempt to upload it + // directly via the host, we want to keep it as a repeatable task. + return fPendingUploads->recordUpload(recorder, source, std::move(condContext)); } void DrawContext::recordDependency(sk_sp<Task> task) {
diff --git a/src/gpu/graphite/DrawContext.h b/src/gpu/graphite/DrawContext.h index b4227e3..0e0d7bb 100644 --- a/src/gpu/graphite/DrawContext.h +++ b/src/gpu/graphite/DrawContext.h
@@ -17,6 +17,7 @@ #include "src/gpu/graphite/ResourceTypes.h" #include "src/gpu/graphite/TextureProxy.h" #include "src/gpu/graphite/TextureProxyView.h" +#include "src/gpu/graphite/task/UploadTask.h" #include <array> #include <memory> @@ -41,8 +42,6 @@ class StrokeStyle; class Task; class Transform; -class UploadList; -class UploadSource; /** * DrawContext records draw commands into a specific Surface, via a general task graph @@ -88,12 +87,8 @@ const Insertion& latestInsertion); bool recordUpload(Recorder* recorder, - const TextureProxyView& dstView, - const SkColorInfo& srcColorInfo, - const SkColorInfo& dstColorInfo, const UploadSource& source, - const SkIRect& dstRect, - std::unique_ptr<ConditionalUploadContext>); + std::unique_ptr<ConditionalUploadContext> = nullptr); // Add a Task that will be executed *before* any of the pending draws and uploads are // executed as part of the next flush().
diff --git a/src/gpu/graphite/Recorder.cpp b/src/gpu/graphite/Recorder.cpp index bcba421..a03e8f7 100644 --- a/src/gpu/graphite/Recorder.cpp +++ b/src/gpu/graphite/Recorder.cpp
@@ -422,24 +422,19 @@ ReadSwizzleForColorType(srcData[0].info().colorType(), format)}; const SkIRect dimensions = SkIRect::MakeSize(backendTex.dimensions()); UploadSource uploadSource = UploadSource::Make( - this->priv().caps(), view, colorInfo, colorInfo, mipLevels, dimensions); + this->priv().caps(), std::move(view), colorInfo, colorInfo, mipLevels, dimensions); if (!uploadSource.isValid()) { SKGPU_LOG_E("Recorder::updateBackendTexture: Could not create UploadSource"); return false; } - // Attempt to update the texture directly on the host if possible. - if (uploadSource.canUploadOnHost()) { - return view.proxy()->texture()->uploadDataOnHost(uploadSource, dimensions); + if (uploadSource.attemptUploadOnhost()) { + return true; } // Add UploadTask to Recorder UploadInstance upload = UploadInstance::Make(this, - view, - colorInfo, - colorInfo, uploadSource, - dimensions, std::make_unique<ImageUploadContext>()); if (!upload.isValid()) { SKGPU_LOG_E("Recorder::updateBackendTexture: Could not create UploadInstance"); @@ -482,20 +477,20 @@ sk_sp<TextureProxy> proxy = TextureProxy::Wrap(std::move(texture)); UploadSource uploadSource = - UploadSource::MakeCompressed(this->priv().caps(), *proxy, data, dataSize); + UploadSource::MakeCompressed(this->priv().caps(), std::move(proxy), data, dataSize); if (!uploadSource.isValid()) { SKGPU_LOG_E("Recorder::updateBackendTexture: Could not create compressed UploadSource"); return false; } - // Attempt to update the texture directly on the host if possible. - if (uploadSource.canUploadOnHost()) { - return proxy->texture()->uploadDataOnHost(uploadSource, - SkIRect::MakeSize(proxy->dimensions())); + if (uploadSource.attemptUploadOnhost()) { + return true; } // Add UploadTask to Recorder - UploadInstance upload = UploadInstance::MakeCompressed(this, std::move(proxy), uploadSource); + UploadInstance upload = UploadInstance::Make(this, + uploadSource, + std::make_unique<ImageUploadContext>()); if (!upload.isValid()) { SKGPU_LOG_E("Recorder::updateBackendTexture: Could not create compressed UploadInstance"); return false;
diff --git a/src/gpu/graphite/Resource.h b/src/gpu/graphite/Resource.h index bad09e9..d056b53 100644 --- a/src/gpu/graphite/Resource.h +++ b/src/gpu/graphite/Resource.h
@@ -228,11 +228,7 @@ // Whether the resource is currently in use by the GPU: any resource that is used in a command // buffer is considered in use by the GPU. - // - // NOTE: This is currently only correct for textures, hence the name. Once the rest of the - // resources use the command buffer ref instead of usage ref appropriately, this can be made - // more generaic. - bool isTextureBusyOnGPU() const { + bool isBusyOnGPU() const { return (fRefs.load(std::memory_order_acquire) & RefMask(RefType::kCommandBuffer)) != 0; }
diff --git a/src/gpu/graphite/Texture.cpp b/src/gpu/graphite/Texture.cpp index 551ac9c..734a259 100644 --- a/src/gpu/graphite/Texture.cpp +++ b/src/gpu/graphite/Texture.cpp
@@ -44,7 +44,7 @@ } } -bool Texture::uploadDataOnHost(const UploadSource& source, const SkIRect& dstRect) { +bool Texture::uploadDataOnHost(const UploadSource& source) { SK_ABORT("Not implemented"); }
diff --git a/src/gpu/graphite/Texture.h b/src/gpu/graphite/Texture.h index 5e6d46f..5a7d3df 100644 --- a/src/gpu/graphite/Texture.h +++ b/src/gpu/graphite/Texture.h
@@ -43,9 +43,9 @@ virtual bool canUploadOnHost() const { return false; } - // With the assumption that source.canUploadOnHost() is true, attempts to write to the + // With the assumption that the source data can upload on the host, attempts to write to the // texture on the host directly. Returns `false` only if driver calls fail. - virtual bool uploadDataOnHost(const UploadSource& source, const SkIRect& dstRect); + virtual bool uploadDataOnHost(const UploadSource& source); protected: Texture(const SharedContext*,
diff --git a/src/gpu/graphite/TextureFormatXferFn.cpp b/src/gpu/graphite/TextureFormatXferFn.cpp index 285e0fc..9289cc0 100644 --- a/src/gpu/graphite/TextureFormatXferFn.cpp +++ b/src/gpu/graphite/TextureFormatXferFn.cpp
@@ -294,6 +294,17 @@ return TextureFormatXferFn(srcFormat, preOps, std::move(rp), /*postOps=*/0); } +std::optional<TextureFormatXferFn> TextureFormatXferFn::MakeIdentity(TextureFormat format) { + auto [baseCT, xferOps] = TextureFormatColorTypeInfo(format); + if (xferOps & FormatXferOp::kDisabled) { + if (TextureFormatCompressionType(format) == SkTextureCompressionType::kNone) { + return std::nullopt; + } // else allow compressed formats through for identity conversion uploads + } + + return TextureFormatXferFn(format, /*preOps=*/0, /*rp=*/nullptr, /*postOps=*/0); +} + template <typename... RPModifiers> sk_sp<TextureFormatXferFn::RPOps> TextureFormatXferFn::RPOps::Make( SkColorType srcColorType,
diff --git a/src/gpu/graphite/TextureFormatXferFn.h b/src/gpu/graphite/TextureFormatXferFn.h index 9814bce..492a1ba 100644 --- a/src/gpu/graphite/TextureFormatXferFn.h +++ b/src/gpu/graphite/TextureFormatXferFn.h
@@ -43,6 +43,11 @@ const SkColorSpaceXformSteps& csSteps, SkColorType dstCT); + // Builds a transfer function that moves data, assuming it is already exactly in the provided + // TextureFormat. Unlike the above two color-type/space converting functions, this also + // can support compressed textures (assuming the data is already compressed). + static std::optional<TextureFormatXferFn> MakeIdentity(TextureFormat format); + TextureFormatXferFn(const TextureFormatXferFn&) = default; TextureFormatXferFn(TextureFormatXferFn&&) = default; @@ -59,7 +64,8 @@ // respective buffers by `srcRowBytes` and `dstRowBytes` respectively. // // It is assumed that the row bytes and data contents match the formats and color types provided - // when the TextureFormatXferFn was created. + // when the TextureFormatXferFn was created. If transferring data for compressed formats, the + // width and height should be in units of blocks. void run(int width, int height, const void* src, size_t srcRowBytes, void* dst, size_t dstRowBytes) const;
diff --git a/src/gpu/graphite/TextureUtils.cpp b/src/gpu/graphite/TextureUtils.cpp index f19b6ed..a2c419f 100644 --- a/src/gpu/graphite/TextureUtils.cpp +++ b/src/gpu/graphite/TextureUtils.cpp
@@ -329,24 +329,27 @@ // no need to coordinate resource sharing. It is better to then group them into a single task // at the start of the Recording. const SkIRect dimensions = SkIRect::MakeSize(bitmap.dimensions()); + // Move the view into `uploadSource` so that it is the unique holder of the proxy UploadSource uploadSource = UploadSource::Make( - recorder->priv().caps(), view, colorInfo, colorInfo, texels, dimensions); + recorder->priv().caps(), std::move(view), colorInfo, colorInfo, texels, dimensions); if (!uploadSource.isValid()) { SKGPU_LOG_E("MakeBitmapProxyView: Could not create UploadSource"); return {}; } + + if (uploadSource.attemptUploadOnhost()) { + return uploadSource.view(); + } + // else it failed or was unavailable, so use a task on the root upload list + if (!recorder->priv().rootUploadList()->recordUpload(recorder, - view, - colorInfo, - colorInfo, uploadSource, - dimensions, std::make_unique<ImageUploadContext>())) { SKGPU_LOG_E("MakeBitmapProxyView: Could not create UploadInstance"); return {}; } - return view; + return uploadSource.view(); } sk_sp<TextureProxy> MakePromiseImageLazyProxy(
diff --git a/src/gpu/graphite/task/UploadTask.cpp b/src/gpu/graphite/task/UploadTask.cpp index 0c6e2f4..f972e27 100644 --- a/src/gpu/graphite/task/UploadTask.cpp +++ b/src/gpu/graphite/task/UploadTask.cpp
@@ -52,14 +52,16 @@ // the aligned destination rowBytes for each level. std::pair<size_t, size_t> compute_combined_buffer_size( const Caps* caps, + TextureFormat format, int mipLevelCount, - size_t bytesPerBlock, const SkISize& baseDimensions, - SkTextureCompressionType compressionType, TArray<std::pair<size_t, size_t>>* levelOffsetsAndRowBytes) { SkASSERT(levelOffsetsAndRowBytes && levelOffsetsAndRowBytes->empty()); SkASSERT(mipLevelCount >= 1); + const size_t bytesPerBlock = TextureFormatBytesPerBlock(format); + const SkTextureCompressionType compressionType = TextureFormatCompressionType(format); + SkISize compressedBlockDimensions = CompressedDimensionsInBlocks(compressionType, baseDimensions); @@ -92,7 +94,7 @@ return {combinedBufferSize, minTransferBufferAlignment}; } -UploadSource::UploadSource() : fCompression(SkTextureCompressionType::kNone) {} +UploadSource::UploadSource(TextureProxyView view) : fView(std::move(view)) {} UploadSource::UploadSource(UploadSource&&) = default; UploadSource& UploadSource::operator=(UploadSource&&) = default; UploadSource::~UploadSource() = default; @@ -103,36 +105,27 @@ const SkColorInfo& dstColorInfo, SkSpan<const MipLevel> levels, const SkIRect& dstRect) { - SkDEBUGCODE(const TextureInfo& texInfo = dstView.proxy()->textureInfo()); - - SkASSERT(caps->isTexturable(texInfo)); - SkASSERT(AreColorTypeAndFormatCompatible(dstColorInfo.colorType(), dstView.proxy()->format())); - - unsigned int mipLevelCount = levels.size(); - // The assumption is either that we have no mipmaps, or that our rect is the entire texture - SkASSERT(mipLevelCount == 1 || dstRect == SkIRect::MakeSize(dstView.dimensions())); - - // We assume that if the texture has mip levels, we either upload to all the levels or just the - // first. -#ifdef SK_DEBUG - unsigned int numExpectedLevels = 1; - if (texInfo.mipmapped() == Mipmapped::kYes) { - numExpectedLevels = SkMipmap::ComputeLevelCount(dstView.dimensions()) + 1; - } - SkASSERT(mipLevelCount == 1 || mipLevelCount == numExpectedLevels); -#endif - + // No data to upload if (dstRect.isEmpty()) { return Invalid(); } + // Ensure data would fit into the texture + if (!dstView.proxy()->isFullyLazy() && + !SkIRect::MakeSize(dstView.dimensions()).contains(dstRect)) { + return Invalid(); + } - UploadSource source; - for (unsigned int i = 0; i < mipLevelCount; ++i) { - // We do not allow any gaps in the mip data - if (!levels[i].fPixels) { - return Invalid(); - } - source.fLevels.push_back(levels[i]); + unsigned int mipLevelCount = levels.size(); + // The assumption is either that we have no mipmaps, or that our rect is the entire texture + if (mipLevelCount != 1 && dstRect != SkIRect::MakeSize(dstView.dimensions())) { + return Invalid(); + } + + // We assume that if the texture has mips, we either upload to all the levels or just the first. + unsigned int numExpectedLevels = dstView.mipmapped() == Mipmapped::kYes ? + SkMipmap::ComputeLevelCount(dstView.dimensions()) + 1 : 1; + if (numExpectedLevels != mipLevelCount) { + return Invalid(); } SkColorSpaceXformSteps csSteps{srcColorInfo.colorSpace(), srcColorInfo.alphaType(), @@ -145,160 +138,115 @@ return Invalid(); } - source.fXferFn = xferFn; - source.fBytesPerPixel = TextureFormatBytesPerBlock(dstView.proxy()->format()); - // Don't upload on the host if we need to perform conversions that could be done directly into - // a mapped GPU buffer. - source.fCanUploadOnHost = xferFn->isIdentity() && - dstView.proxy()->isInstantiated() && - dstView.proxy()->texture()->canUploadOnHost(); + UploadSource source{std::move(dstView)}; + for (unsigned int i = 0; i < mipLevelCount; ++i) { + // We do not allow any gaps in the mip data + if (!levels[i].fPixels) { + return Invalid(); + } + source.fLevels.push_back(levels[i]); + } + source.fDstRect = dstRect; + source.fXferFn = xferFn; return source; } UploadSource UploadSource::MakeCompressed(const Caps* caps, - const TextureProxy& textureProxy, + sk_sp<TextureProxy> textureProxy, const void* data, size_t dataSize) { if (!data) { return Invalid(); // no data to upload } - - const TextureInfo& texInfo = textureProxy.textureInfo(); - SkASSERT(caps->isTexturable(texInfo)); - - SkTextureCompressionType compression = TextureFormatCompressionType(textureProxy.format()); + SkTextureCompressionType compression = TextureFormatCompressionType(textureProxy->format()); if (compression == SkTextureCompressionType::kNone) { return Invalid(); } - // Create a transfer buffer and fill with data. - const SkISize dimensions = textureProxy.dimensions(); - skia_private::STArray<16, size_t> srcMipOffsets; - SkDEBUGCODE(size_t computedSize =) SkCompressedDataSize( - compression, dimensions, &srcMipOffsets, texInfo.mipmapped() == Mipmapped::kYes); - SkASSERT(computedSize == dataSize); + const SkISize dimensions = textureProxy->dimensions(); + STArray<16, size_t> srcMipOffsets; + size_t computedSize = SkCompressedDataSize( + compression, dimensions, &srcMipOffsets, textureProxy->mipmapped() == Mipmapped::kYes); + if (computedSize != dataSize) { + return Invalid(); + } + + auto xferFn = TextureFormatXferFn::MakeIdentity(textureProxy->format()); + SkASSERT(xferFn.has_value()); const unsigned int mipLevelCount = srcMipOffsets.size(); - UploadSource source; + UploadSource source{TextureProxyView(textureProxy)}; source.fLevels.resize(mipLevelCount); + int currentWidth = textureProxy->dimensions().width(); for (unsigned int i = 0; i < mipLevelCount; ++i) { source.fLevels[i].fPixels = SkTAddOffset<const void>(data, srcMipOffsets[i]); - source.fLevels[i].fRowBytes = 0; // Tightly packed + // Assume the source data is tightly packed. + source.fLevels[i].fRowBytes = CompressedRowBytes(compression, currentWidth); + currentWidth = std::max(1, currentWidth / 2); } - source.fCompression = compression; - source.fBytesPerPixel = SkCompressedBlockSize(compression); - source.fCanUploadOnHost = textureProxy.isInstantiated() && - textureProxy.texture()->canUploadOnHost(); + source.fDstRect = SkIRect::MakeSize(dimensions); + source.fXferFn = xferFn; return source; } +bool UploadSource::attemptUploadOnhost() const { + SkASSERT(this->isValid()); + + // Don't upload on the host if we need to perform conversions that could be done directly into + // a mapped GPU buffer. + if (!fXferFn->isIdentity() || + !fView.proxy()->isInstantiated() || + !fView.proxy()->texture()->canUploadOnHost()) { + return false; + } + + // Don't upload on the host if the UploadSource doesn't have a unique hold on the TextureProxy + // (otherwise some other thread could trigger GPU work while this thread was modifying the + // underlying Texture resource). + if (!fView.proxy()->unique()) { + return false; + } + return fView.proxy()->texture()->uploadDataOnHost(*this); +} + UploadInstance::UploadInstance() = default; UploadInstance::UploadInstance(UploadInstance&&) = default; UploadInstance& UploadInstance::operator=(UploadInstance&&) = default; UploadInstance::~UploadInstance() = default; UploadInstance::UploadInstance(const Buffer* buffer, - size_t bytesPerPixel, sk_sp<TextureProxy> textureProxy, std::unique_ptr<ConditionalUploadContext> condContext) : fBuffer(buffer) - , fBytesPerPixel(bytesPerPixel) , fTextureProxy(textureProxy) , fConditionalContext(std::move(condContext)) {} UploadInstance UploadInstance::Make(Recorder* recorder, - const TextureProxyView& dstView, - const SkColorInfo& srcColorInfo, - const SkColorInfo& dstColorInfo, const UploadSource& source, - const SkIRect& dstRect, std::unique_ptr<ConditionalUploadContext> condContext) { + sk_sp<TextureProxy> textureProxy = source.view().refProxy(); + const SkIRect dstRect = source.dstRect(); + const SkTextureCompressionType compression = + TextureFormatCompressionType(textureProxy->format()); + const Caps* caps = recorder->priv().caps(); SkSpan<const MipLevel> levels = source.levels(); uint32_t mipLevelCount = static_cast<uint32_t>(levels.size()); - TArray<std::pair<size_t, size_t>> levelOffsetsAndRowBytes(mipLevelCount); - + STArray<16, std::pair<size_t, size_t>> levelOffsetsAndRowBytes(mipLevelCount); auto [combinedBufferSize, minAlignment] = compute_combined_buffer_size(caps, + textureProxy->format(), mipLevelCount, - source.bytesPerPixel(), dstRect.size(), - source.compression(), &levelOffsetsAndRowBytes); SkASSERT(combinedBufferSize); UploadBufferManager* bufferMgr = recorder->priv().uploadBufferManager(); auto [writer, bufferInfo] = bufferMgr->getTextureUploadWriter(combinedBufferSize, minAlignment); - if (!writer) { - SKGPU_LOG_W("Failed to get write-mapped buffer for texture upload of size %zu", - combinedBufferSize); - return Invalid(); - } - SkASSERT(source.formatXferFn().has_value()); // UploadSource verified this - - UploadInstance upload{bufferInfo.fBuffer, - source.bytesPerPixel(), - dstView.refProxy(), - std::move(condContext)}; - - // Fill in copy data - int32_t currentWidth = dstRect.width(); - int32_t currentHeight = dstRect.height(); - for (uint32_t currentMipLevel = 0; currentMipLevel < mipLevelCount; currentMipLevel++) { - const size_t srcRowBytes = levels[currentMipLevel].fRowBytes; - const auto [mipOffset, dstRowBytes] = levelOffsetsAndRowBytes[currentMipLevel]; - - // copy data into the buffer, skipping any trailing bytes - const char* src = (const char*)levels[currentMipLevel].fPixels; - writer.convert(mipOffset, currentWidth, currentHeight, - src, srcRowBytes, *source.formatXferFn(), dstRowBytes); - - // For mipped data, the dstRect is always the full texture so we don't need to worry about - // modifying the TL coord as it will always be 0,0,for all levels. - upload.fCopyData.push_back({ - /*fBufferOffset=*/bufferInfo.fOffset + mipOffset, - /*fBufferRowBytes=*/dstRowBytes, - /*fRect=*/SkIRect::MakeXYWH(dstRect.left(), dstRect.top(), currentWidth, currentHeight), - /*fMipLevel=*/currentMipLevel - }); - - currentWidth = std::max(1, currentWidth / 2); - currentHeight = std::max(1, currentHeight / 2); - } - - ATRACE_ANDROID_FRAMEWORK("Upload %sTexture [%dx%d]", - mipLevelCount > 1 ? "MipMap " : "", - dstRect.width(), dstRect.height()); - - return upload; -} - -UploadInstance UploadInstance::MakeCompressed(Recorder* recorder, - sk_sp<TextureProxy> textureProxy, - const UploadSource& source) { - const Caps* caps = recorder->priv().caps(); - const SkISize dimensions = textureProxy->dimensions(); - SkSpan<const MipLevel> levels = source.levels(); - uint32_t mipLevelCount = static_cast<uint32_t>(levels.size()); - - TArray<std::pair<size_t, size_t>> levelOffsetsAndRowBytes(mipLevelCount); - auto [combinedBufferSize, minAlignment] = - compute_combined_buffer_size(caps, - mipLevelCount, - source.bytesPerPixel(), - dimensions, - source.compression(), - &levelOffsetsAndRowBytes); - SkASSERT(combinedBufferSize); - - UploadBufferManager* bufferMgr = recorder->priv().uploadBufferManager(); - auto [writer, bufferInfo] = bufferMgr->getTextureUploadWriter(combinedBufferSize, minAlignment); - - std::vector<BufferTextureCopyData> copyData(mipLevelCount); if (!bufferInfo.fBuffer) { SKGPU_LOG_W("Failed to get write-mapped buffer for texture upload of size %zu", @@ -306,37 +254,48 @@ return Invalid(); } - UploadInstance upload{bufferInfo.fBuffer, source.bytesPerPixel(), std::move(textureProxy)}; + ATRACE_ANDROID_FRAMEWORK("Upload %s %sTexture [%dx%d]", + TextureFormatName(textureProxy->format()), + mipLevelCount > 1 ? "MipMap " : "", + dstRect.width(), + dstRect.height()); + + UploadInstance upload{bufferInfo.fBuffer, std::move(textureProxy), std::move(condContext)}; // Fill in copy data - int32_t currentWidth = dimensions.width(); - int32_t currentHeight = dimensions.height(); + int32_t currentWidth = dstRect.width(); + int32_t currentHeight = dstRect.height(); for (uint32_t currentMipLevel = 0; currentMipLevel < mipLevelCount; currentMipLevel++) { + // NOTE: When not compressed, this function automatically returns currentWidth and height, + // e.g. uncompressed blocks are the same as texels. SkISize blockDimensions = - CompressedDimensionsInBlocks(source.compression(), {currentWidth, currentHeight}); - int32_t blockHeight = blockDimensions.height(); + CompressedDimensionsInBlocks(compression, {currentWidth, currentHeight}); - const size_t trimRowBytes = CompressedRowBytes(source.compression(), currentWidth); - const size_t srcRowBytes = trimRowBytes; + const size_t srcRowBytes = levels[currentMipLevel].fRowBytes; const auto [dstMipOffset, dstRowBytes] = levelOffsetsAndRowBytes[currentMipLevel]; // copy data into the buffer, skipping any trailing bytes const void* src = levels[currentMipLevel].fPixels; - - writer.write(dstMipOffset, src, srcRowBytes, dstRowBytes, trimRowBytes, blockHeight); + writer.convert(dstMipOffset, blockDimensions.width(), blockDimensions.height(), + src, srcRowBytes, source.formatXferFn(), dstRowBytes); int32_t copyWidth = currentWidth; int32_t copyHeight = currentHeight; - if (caps->fullCompressedUploadSizeMustAlignToBlockDims()) { - SkISize oneBlockDims = CompressedDimensions(source.compression(), {1, 1}); + if (compression != SkTextureCompressionType::kNone && + caps->fullCompressedUploadSizeMustAlignToBlockDims()) { + SkISize oneBlockDims = CompressedDimensions(compression, {1, 1}); copyWidth = SkAlignTo(copyWidth, oneBlockDims.fWidth); copyHeight = SkAlignTo(copyHeight, oneBlockDims.fHeight); } + // For compressed and mipped data, the dstRect is always the full texture so we don't need + // to worry about modifying the TL coord as it will always be 0,0,for all levels. + SkASSERT((dstRect.left() == 0 && dstRect.top() == 0) || + (mipLevelCount == 1 && compression == SkTextureCompressionType::kNone)); upload.fCopyData.push_back({ /*fBufferOffset=*/bufferInfo.fOffset + dstMipOffset, /*fBufferRowBytes=*/dstRowBytes, - /*fRect=*/SkIRect::MakeXYWH(0, 0, copyWidth, copyHeight), + /*fRect=*/SkIRect::MakeXYWH(dstRect.left(), dstRect.top(), copyWidth, copyHeight), /*fMipLevel=*/currentMipLevel }); @@ -344,11 +303,6 @@ currentHeight = std::max(1, currentHeight / 2); } - ATRACE_ANDROID_FRAMEWORK("Upload Compressed %sTexture [%dx%d]", - mipLevelCount > 1 ? "MipMap " : "", - dimensions.width(), - dimensions.height()); - return upload; } @@ -414,10 +368,11 @@ return Status::kSuccess; } + const int bpp = TextureFormatBytesPerBlock(fTextureProxy->format()); BufferTextureCopyData transformedCopyData = copyData; transformedCopyData.fBufferOffset += (croppedDstRect.y() - dstRect.y()) * copyData.fBufferRowBytes + - (croppedDstRect.x() - dstRect.x()) * fBytesPerPixel; + (croppedDstRect.x() - dstRect.x()) * bpp; transformedCopyData.fRect = croppedDstRect; if (!commandBuffer->copyBufferToTexture(fBuffer, @@ -439,23 +394,10 @@ //--------------------------------------------------------------------------- bool UploadList::recordUpload(Recorder* recorder, - const TextureProxyView& dstView, - const SkColorInfo& srcColorInfo, - const SkColorInfo& dstColorInfo, const UploadSource& source, - const SkIRect& dstRect, std::unique_ptr<ConditionalUploadContext> condContext) { - // If possible, upload the data directly on host. - if (source.canUploadOnHost()) { - return dstView.proxy()->texture()->uploadDataOnHost(source, dstRect); - } - UploadInstance instance = UploadInstance::Make(recorder, - std::move(dstView), - srcColorInfo, - dstColorInfo, source, - dstRect, std::move(condContext)); if (!instance.isValid()) { return false;
diff --git a/src/gpu/graphite/task/UploadTask.h b/src/gpu/graphite/task/UploadTask.h index ca17c7b..aa2e123 100644 --- a/src/gpu/graphite/task/UploadTask.h +++ b/src/gpu/graphite/task/UploadTask.h
@@ -15,6 +15,7 @@ #include "include/private/base/SkTArray.h" #include "src/gpu/graphite/CommandTypes.h" #include "src/gpu/graphite/TextureFormatXferFn.h" +#include "src/gpu/graphite/TextureProxyView.h" #include <cstddef> #include <cstdint> @@ -36,7 +37,6 @@ class RuntimeEffectDictionary; class ScratchResourceManager; class TextureProxy; -class TextureProxyView; struct MipLevel { const void* fPixels = nullptr; @@ -92,7 +92,7 @@ SkSpan<const MipLevel> levels, const SkIRect& dstRect); static UploadSource MakeCompressed(const Caps*, - const TextureProxy& textureProxy, + sk_sp<TextureProxy> textureProxy, const void* data, size_t dataSize); @@ -102,26 +102,36 @@ bool isValid() const { return !fLevels.empty(); } + const TextureProxyView& view() const { return fView; } + SkSpan<const MipLevel> levels() const { return fLevels; } - bool canUploadOnHost() const { return fCanUploadOnHost; } - SkTextureCompressionType compression() const { return fCompression; } - size_t bytesPerPixel() const { return fBytesPerPixel; } - const std::optional<TextureFormatXferFn>& formatXferFn() const { return fXferFn; } + const SkIRect& dstRect() const { return fDstRect; } + const TextureFormatXferFn& formatXferFn() const { return *fXferFn; } + + // This uploads the data to the texture directly without going through any command buffer. + // This is a) not always desired (e.g. for repeated uploads in a task graph), and b) not + // always possible (if it's in use by the GPU and the backend doesn't synchronize a host copy). + // + // This returns true if the upload succeeded, otherwise false, in which case a regular + // upload should be attempted with `UploadInstance`. When uploading on a host is desired, the + // target proxy should be moved into the UploadSource in order to track that the target is + // uniquely held by the current thread (required to ensure no simultaneous GPU use starts + // using the texture). + bool attemptUploadOnhost() const; private: - static UploadSource Invalid() { return {}; } + static UploadSource Invalid() { return {{}}; } - UploadSource(); + UploadSource(TextureProxyView); + // Technically after we've created the TextureFormatXferFn, we don't need the view's swizzle + // anymore, but hold on to the view for convenience since moving the original view/proxy into + // the UploadSource for host uploads is encouraged. + TextureProxyView fView; + SkIRect fDstRect; skia_private::STArray<16, MipLevel> fLevels; - // Whether the texture supports uploads directly from host memory. - bool fCanUploadOnHost = false; - // Compression type, if any. - SkTextureCompressionType fCompression; - // Bytes per pixel or block (if compressed) - size_t fBytesPerPixel = 0; - // Not present for compressed formats + // All valid UploadSources will have a transfer function. std::optional<TextureFormatXferFn> fXferFn; }; @@ -132,15 +142,8 @@ class UploadInstance { public: static UploadInstance Make(Recorder*, - const TextureProxyView& dst, - const SkColorInfo& srcColorInfo, - const SkColorInfo& dstColorInfo, const UploadSource& source, - const SkIRect& dstRect, std::unique_ptr<ConditionalUploadContext>); - static UploadInstance MakeCompressed(Recorder*, - sk_sp<TextureProxy> textureProxy, - const UploadSource& source); static UploadInstance Invalid() { return {}; } @@ -162,12 +165,10 @@ UploadInstance(); // Copy data is appended directly after the object is created UploadInstance(const Buffer*, - size_t bytesPerPixel, sk_sp<TextureProxy>, std::unique_ptr<ConditionalUploadContext> = nullptr); const Buffer* fBuffer; - size_t fBytesPerPixel; sk_sp<TextureProxy> fTextureProxy; skia_private::STArray<1, BufferTextureCopyData> fCopyData; std::unique_ptr<ConditionalUploadContext> fConditionalContext; @@ -185,12 +186,8 @@ class UploadList { public: bool recordUpload(Recorder*, - const TextureProxyView& dst, - const SkColorInfo& srcColorInfo, - const SkColorInfo& dstColorInfo, const UploadSource& source, - const SkIRect& dstRect, - std::unique_ptr<ConditionalUploadContext>); + std::unique_ptr<ConditionalUploadContext> = nullptr); int size() { return fInstances.size(); }
diff --git a/src/gpu/graphite/vk/VulkanTexture.cpp b/src/gpu/graphite/vk/VulkanTexture.cpp index 6820443..6e3151f 100644 --- a/src/gpu/graphite/vk/VulkanTexture.cpp +++ b/src/gpu/graphite/vk/VulkanTexture.cpp
@@ -15,6 +15,7 @@ #include "src/gpu/DataUtils.h" #include "src/gpu/graphite/Log.h" #include "src/gpu/graphite/Sampler.h" +#include "src/gpu/graphite/TextureProxy.h" #include "src/gpu/graphite/task/UploadTask.h" #include "src/gpu/graphite/vk/VulkanCaps.h" #include "src/gpu/graphite/vk/VulkanCommandBuffer.h" @@ -499,8 +500,14 @@ return false; } - // Can't use host-image-copy if the image is busy on the GPU. - if (this->isTextureBusyOnGPU()) { + // Can't use host-image-copy if the image is busy on the GPU. While UploadSource was responsible + // for ensuring the TextureProxy was uniquely held (so other threads can't add work for that + // proxy), we have to ensure the host upload won't conflict with any other use of the texture: + // - The texture could be used on the GPU by some now-deleted proxy, so we'll have to fall + // back to a regular upload via command buffer submission. + // - The texture needs to be non-shareable, otherwise multiple proxies could be instantiated + // with the same proxy. + if (this->shareable() != Shareable::kNo || this->isBusyOnGPU()) { return false; } @@ -516,7 +523,17 @@ return true; } -bool VulkanTexture::uploadDataOnHost(const UploadSource& source, const SkIRect& dstRect) { +bool VulkanTexture::uploadDataOnHost(const UploadSource& source) { + // Should not have changed since canUploadOnHost() returned true given the usage possibilities + // implied by these constraints (current thread is the only possible mutator). + SkASSERT(source.formatXferFn().isIdentity()); + SkASSERT(source.view().proxy()->unique()); + SkASSERT(source.view().proxy()->texture() == this); + SkASSERT(this->shareable() == Shareable::kNo); + SkASSERT(!this->isBusyOnGPU()); + SkASSERT(this->vulkanTextureInfo().fImageUsageFlags & VK_IMAGE_USAGE_HOST_TRANSFER_BIT); + SkASSERT(this->currentLayout() == VK_IMAGE_LAYOUT_UNDEFINED); + auto sharedContext = static_cast<const VulkanSharedContext*>(this->sharedContext()); SkSpan<const MipLevel> levels = source.levels(); const unsigned int mipLevelCount = levels.size(); @@ -544,7 +561,9 @@ TArray<VkMemoryToImageCopy> copyRegions(mipLevelCount); - // The assumption is either that we have no mipmaps, or that our rect is the entire texture + // The assumption is either that we have no mipmaps, or that our rect is the entire texture, + // which is guaranteed by UploadSource's creation. + const SkIRect& dstRect = source.dstRect(); SkASSERT(mipLevelCount == 1 || dstRect == SkIRect::MakeSize(this->dimensions())); // Copy data mip by mip. @@ -553,16 +572,12 @@ int32_t currentWidth = dstRect.width(); int32_t currentHeight = dstRect.height(); + const int bpp = TextureFormatBytesPerBlock(TextureInfoPriv::ViewFormat(this->textureInfo())); for (unsigned int currentMipLevel = 0; currentMipLevel < mipLevelCount; currentMipLevel++) { - // Upload data for compressed formats are fully packed. If this changes, the division by - // bytes-per-pixel should be adjusted for compressed formats. - SkASSERT(source.compression() == SkTextureCompressionType::kNone || - levels[currentMipLevel].fRowBytes == 0); - VkMemoryToImageCopy copyRegion = {}; copyRegion.sType = VK_STRUCTURE_TYPE_MEMORY_TO_IMAGE_COPY; copyRegion.pHostPointer = levels[currentMipLevel].fPixels; - copyRegion.memoryRowLength = levels[currentMipLevel].fRowBytes / source.bytesPerPixel(); + copyRegion.memoryRowLength = levels[currentMipLevel].fRowBytes / bpp; copyRegion.memoryImageHeight = 0; // Tightly packed copyRegion.imageSubresource.aspectMask = aspectFlags; copyRegion.imageSubresource.mipLevel = currentMipLevel;
diff --git a/src/gpu/graphite/vk/VulkanTexture.h b/src/gpu/graphite/vk/VulkanTexture.h index 1127cbf..bd3e4d9 100644 --- a/src/gpu/graphite/vk/VulkanTexture.h +++ b/src/gpu/graphite/vk/VulkanTexture.h
@@ -106,7 +106,7 @@ bool canUploadOnHost() const override; // Once upload is finished, the image will be in the VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL // layout. - bool uploadDataOnHost(const UploadSource& source, const SkIRect& dstRect) override; + bool uploadDataOnHost(const UploadSource& source) override; private: VulkanTexture(const VulkanSharedContext* sharedContext,
diff --git a/tests/graphite/ComputeTest.cpp b/tests/graphite/ComputeTest.cpp index 482f6c4..1545502 100644 --- a/tests/graphite/ComputeTest.cpp +++ b/tests/graphite/ComputeTest.cpp
@@ -960,11 +960,7 @@ return; } UploadInstance upload = UploadInstance::Make(recorder.get(), - TextureProxyView(srcProxy), - srcPixels.info().colorInfo(), - srcPixels.info().colorInfo(), uploadSource, - SkIRect::MakeWH(kDim, kDim), std::make_unique<ImageUploadContext>()); if (!upload.isValid()) { ERRORF(reporter, "Could not create UploadInstance");