Skip to content

Commit 13c1d10

Browse files
rphilliSkCQ
authored and
SkCQ
committed
[graphite] Fix Pipeline label for when resolve-loads are relevant
While analyzing Pipeline labels from Chrome I kept seeing identical Pipeline labels that were representing different Pipelines (i.e., had different UniqueKeys). Bug: b/394827799 Change-Id: I78fdd48290f9e20ccc73844bba9daf2fc950f65a Reviewed-on: https://skia-review.googlesource.com/c/skia/+/961516 Reviewed-by: Michael Ludwig <michaelludwig@google.com> Commit-Queue: Robert Phillips <robertphillips@google.com>
1 parent 163d52f commit 13c1d10

File tree

4 files changed

+16
-6
lines changed

4 files changed

+16
-6
lines changed

src/gpu/graphite/RenderPassDesc.cpp

+11-2
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,17 @@ SkString RenderPassDesc::toPipelineLabel() const {
132132
// We include the load op of the color attachment when there is a resolve attachment because
133133
// the load may trigger a different renderpass description.
134134
const char* colorLoadStr = "";
135-
if (fColorAttachment.fLoadOp == LoadOp::kLoad &&
136-
(fColorResolveAttachment.fTextureInfo.isValid() || fSampleCount > 1)) {
135+
136+
const bool loadMsaaFromResolve =
137+
fColorResolveAttachment.fTextureInfo.isValid() &&
138+
fColorResolveAttachment.fLoadOp == LoadOp::kLoad;
139+
140+
// This should, technically, check Caps::loadOpAffectsMSAAPipelines before adding the extra
141+
// string. Only the Metal backend doesn't set that flag, however, so we just assume it is set
142+
// to reduce plumbing. Since the Metal backend doesn't differentiate its UniqueKeys wrt
143+
// resolve-loads, this can lead to instances where two Metal Pipeline labels will map to the
144+
// same UniqueKey (i.e., one with "w/ msaa load" and one without it).
145+
if (loadMsaaFromResolve /* && Caps::loadOpAffectsMSAAPipelines() */) {
137146
colorLoadStr = " w/ msaa load";
138147
}
139148

src/gpu/graphite/dawn/DawnCaps.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -1016,7 +1016,7 @@ uint32_t DawnCaps::getRenderPassDescKeyForPipeline(const RenderPassDesc& renderP
10161016
// So we need to include a bit flag to differentiate the two kinds of pipelines.
10171017
// Also avoid returning a cached pipeline that is not compatible with the render pass using
10181018
// ExpandResolveTexture load op and vice versa.
1019-
const bool shouldIncludeLoadResolveAttachmentBit = this->resolveTextureLoadOp().has_value();
1019+
const bool shouldIncludeLoadResolveAttachmentBit = this->loadOpAffectsMSAAPipelines();
10201020
uint32_t loadResolveAttachmentKey = 0;
10211021
if (shouldIncludeLoadResolveAttachmentBit &&
10221022
renderPassDesc.fColorResolveAttachment.fTextureInfo.isValid() &&
@@ -1111,7 +1111,7 @@ bool DawnCaps::extractGraphicsDescs(const UniqueKey& key,
11111111
LoadOp loadOp = LoadOp::kClear;
11121112
if (renderpassDescBits & kResolveMask) {
11131113
// This bit should only be set if Dawn supports ExpandResolveTexture load op
1114-
SkASSERT(this->resolveTextureLoadOp().has_value());
1114+
SkASSERT(this->loadOpAffectsMSAAPipelines());
11151115
loadOp = LoadOp::kLoad;
11161116
}
11171117

src/gpu/graphite/dawn/DawnGraphicsPipeline.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ sk_sp<DawnGraphicsPipeline> DawnGraphicsPipeline::Make(
439439
// Special case: a render pass loading resolve texture requires additional settings for the
440440
// pipeline to make it compatible.
441441
wgpu::ColorTargetStateExpandResolveTextureDawn pipelineMSAALoadResolveTextureDesc;
442-
if (loadMsaaFromResolve && sharedContext->dawnCaps()->resolveTextureLoadOp().has_value()) {
442+
if (loadMsaaFromResolve && sharedContext->dawnCaps()->loadOpAffectsMSAAPipelines()) {
443443
SkASSERT(device.HasFeature(wgpu::FeatureName::DawnLoadResolveTexture));
444444
colorTarget.nextInChain = &pipelineMSAALoadResolveTextureDesc;
445445
pipelineMSAALoadResolveTextureDesc.enabled = true;

src/gpu/graphite/precompile/SerializationUtils.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,8 @@ bool DataToPipelineDesc(const Caps* caps,
350350
}
351351

352352
#if defined(GPU_TEST_UTILS)
353-
void DumpPipelineDesc(const char* label, ShaderCodeDictionary* shaderCodeDictionary,
353+
void DumpPipelineDesc(const char* label,
354+
ShaderCodeDictionary* shaderCodeDictionary,
354355
const GraphicsPipelineDesc& pipelineDesc,
355356
const RenderPassDesc& renderPassDesc) {
356357
SkString pipelineStr = pipelineDesc.toString(shaderCodeDictionary);

0 commit comments

Comments
 (0)