GL: Avoid calling glBindFragDatalocationIndexed on Qualcomm

Track if the output location and index came from a layout qualifier
or from a call to glBindFragDataLocation[Indexed] and only call
glBindFragDataLocationIndexed in the latter case. Re-binding a
location that was already specified in the shader is not allowed.

Qualcomm fails to bind any location that is not specified with a
layout qualifier. Skip tests that do this behaviour and log warnings
that the driver is unable to handle this case.

Assign the ProgramOutput::pod::index field when doing output assignment
to mirror how the location is assigned.

Bug: angleproject:42267082
Change-Id: Icdf83bb93f63a6375b5a6062690e53905c9ffe71
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/5415796
Reviewed-by: Shahbaz Youssefi <[email protected]>
Commit-Queue: Geoff Lang <[email protected]>
Reviewed-by: Alexey Knyazev <[email protected]>

include/platform/autogen/FeaturesGL_autogen.h

@@ -739,6 +739,13 @@ struct FeaturesGL : FeatureSetBase
  &members, "https://crbug.com/40279678"
  };
 
+ FeatureInfo avoidBindFragDataLocation = {
+ "avoidBindFragDataLocation",
+ FeatureCategory::OpenGLWorkarounds,
+ "Qualcomm drivers fail to link after binding fragment data locations.",
+ &members, "https://anglebug.com/8646"
+ };
+
 };
 
 inline FeaturesGL::FeaturesGL() = default;

include/platform/gl_features.json

@@ -801,6 +801,14 @@
  "Some drivers lose context when repeatedly generating mipmaps on textures that were used as framebuffers."
  ],
  "issue": "https://crbug.com/40279678"
+ },
+ {
+ "name": "avoid_bind_frag_data_location",
+ "category": "Workarounds",
+ "description": [
+ "Qualcomm drivers fail to link after binding fragment data locations."
+ ],
+ "issue": "https://anglebug.com/8646"
  }
  ]
 }

src/libANGLE/ProgramExecutable.cpp

@@ -109,6 +109,7 @@ void AssignOutputLocations(std::vector<VariableLocation> &outputLocations,
  unsigned int elementCount,
  const std::vector<VariableLocation> &reservedLocations,
  unsigned int variableIndex,
+ bool locationAssignedByApi,
  ProgramOutput &outputVariable)
 {
  if (baseLocation + elementCount > outputLocations.size())
@@ -126,6 +127,7 @@ void AssignOutputLocations(std::vector<VariableLocation> &outputLocations,
  outputLocations[location] = locationInfo;
  }
  }
+ outputVariable.pod.hasApiAssignedLocation = locationAssignedByApi;
 }
 
 int GetOutputLocationForLink(const ProgramAliasedBindings &fragmentOutputLocations,
@@ -143,24 +145,29 @@ int GetOutputLocationForLink(const ProgramAliasedBindings &fragmentOutputLocatio
  return -1;
 }
 
-bool IsOutputSecondaryForLink(const ProgramAliasedBindings &fragmentOutputIndexes,
-  const ProgramOutput &outputVariable)
+void AssignOutputIndex(const ProgramAliasedBindings &fragmentOutputIndexes,
+ ProgramOutput &outputVariable)
 {
- if (outputVariable.pod.index != -1)
+ if (outputVariable.pod.hasShaderAssignedLocation)
  {
+ // Already assigned through a layout qualifier
  ASSERT(outputVariable.pod.index == 0 || outputVariable.pod.index == 1);
- return (outputVariable.pod.index == 1);
+ return;
  }
+
  int apiIndex = fragmentOutputIndexes.getBinding(outputVariable);
  if (apiIndex != -1)
  {
  // Index layout qualifier from the shader takes precedence, so the index from the API is
  // checked only if the index was not set in the shader. This is not specified in the EXT
  // spec, but is specified in desktop OpenGL specs.
- return (apiIndex == 1);
+ ASSERT(apiIndex == 0 || apiIndex == 1);
+ outputVariable.pod.index = apiIndex;
+ return;
  }
+
  // EXT_blend_func_extended: Outputs get index 0 by default.
- return false;
+ outputVariable.pod.index = 0;
 }
 
 RangeUI AddUniforms(const ShaderMap<SharedProgramExecutable> &executables,
@@ -720,7 +727,15 @@ ProgramOutput::ProgramOutput(const sh::ShaderVariable &var)
  SetBitField(pod.isBuiltIn, IsBuiltInName(var.name));
  SetBitField(pod.isArray, var.isArray());
  SetBitField(pod.hasImplicitLocation, var.hasImplicitLocation);
+ SetBitField(pod.hasShaderAssignedLocation, var.location != -1);
+ SetBitField(pod.hasApiAssignedLocation, false);
  SetBitField(pod.pad, 0);
+
+ if (pod.hasShaderAssignedLocation && pod.index == -1)
+ {
+ // Location was assigned but index was not. Equivalent to setting index to 0.
+ pod.index = 0;
+ }
 }
 
 // ProgramExecutable implementation.
@@ -1610,7 +1625,7 @@ bool ProgramExecutable::linkValidateOutputVariables(
  for (unsigned int outputVariableIndex = 0; outputVariableIndex < mOutputVariables.size();
  outputVariableIndex++)
  {
- const ProgramOutput &outputVariable = mOutputVariables[outputVariableIndex];
+ ProgramOutput &outputVariable = mOutputVariables[outputVariableIndex];
 
  // Don't store outputs for gl_FragDepth, gl_FragColor, etc.
  if (outputVariable.isBuiltIn())
@@ -1624,10 +1639,10 @@ bool ProgramExecutable::linkValidateOutputVariables(
  }
  unsigned int baseLocation = static_cast<unsigned int>(fixedLocation);
 
+ AssignOutputIndex(fragmentOutputIndices, outputVariable);
+ ASSERT(outputVariable.pod.index == 0 || outputVariable.pod.index == 1);
  std::vector<VariableLocation> &outputLocations =
- IsOutputSecondaryForLink(fragmentOutputIndices, outputVariable)
- ? mSecondaryOutputLocations
- : mOutputLocations;
+ outputVariable.pod.index == 0 ? mOutputLocations : mSecondaryOutputLocations;
 
  // GLSL ES 3.10 section 4.3.6: Output variables cannot be arrays of arrays or arrays of
  // structures, so we may use getBasicTypeElementCount().
@@ -1639,8 +1654,10 @@ bool ProgramExecutable::linkValidateOutputVariables(
  << " conflicts with another variable.";
  return false;
  }
+ bool hasApiAssignedLocation = !outputVariable.pod.hasShaderAssignedLocation &&
+ (fragmentOutputLocations.getBinding(outputVariable) != -1);
  AssignOutputLocations(outputLocations, baseLocation, elementCount, reservedLocations,
- outputVariableIndex, mOutputVariables[outputVariableIndex]);
+ outputVariableIndex, hasApiAssignedLocation, outputVariable);
  }
 
  // Here we assign locations for the output variables that don't yet have them. Note that we're
@@ -1659,17 +1676,18 @@ bool ProgramExecutable::linkValidateOutputVariables(
  for (unsigned int outputVariableIndex = 0; outputVariableIndex < mOutputVariables.size();
  outputVariableIndex++)
  {
- const ProgramOutput &outputVariable = mOutputVariables[outputVariableIndex];
+ ProgramOutput &outputVariable = mOutputVariables[outputVariableIndex];
 
  // Don't store outputs for gl_FragDepth, gl_FragColor, etc.
  if (outputVariable.isBuiltIn())
  continue;
 
- int fixedLocation = GetOutputLocationForLink(fragmentOutputLocations, outputVariable);
+ AssignOutputIndex(fragmentOutputIndices, outputVariable);
+ ASSERT(outputVariable.pod.index == 0 || outputVariable.pod.index == 1);
  std::vector<VariableLocation> &outputLocations =
- IsOutputSecondaryForLink(fragmentOutputIndices, outputVariable)
- ? mSecondaryOutputLocations
-  : mOutputLocations;
+ outputVariable.pod.index == 0 ? mOutputLocations : mSecondaryOutputLocations;
+
+ int fixedLocation = GetOutputLocationForLink(fragmentOutputLocations, outputVariable);
  unsigned int baseLocation = 0;
  unsigned int elementCount = outputVariable.pod.basicTypeElementCount;
  if (fixedLocation != -1)
@@ -1689,7 +1707,7 @@ bool ProgramExecutable::linkValidateOutputVariables(
  baseLocation++;
  }
  AssignOutputLocations(outputLocations, baseLocation, elementCount, reservedLocations,
- outputVariableIndex, mOutputVariables[outputVariableIndex]);
+ outputVariableIndex, false, outputVariable);
  }
 
  // Check for any elements assigned above the max location that are actually used.

src/libANGLE/ProgramExecutable.h

@@ -175,7 +175,9 @@ struct ProgramOutput
  uint32_t isBuiltIn : 1;
  uint32_t isArray : 1;
  uint32_t hasImplicitLocation : 1;
- uint32_t pad : 27;
+ uint32_t hasShaderAssignedLocation : 1;
+ uint32_t hasApiAssignedLocation : 1;
+ uint32_t pad : 25;
  } pod;
 };
 ANGLE_DISABLE_STRUCT_PADDING_WARNINGS

src/libANGLE/renderer/gl/ProgramGL.cpp

@@ -115,10 +115,10 @@ class ProgramGL::LinkTaskGL final : public LinkTask
  ASSERT(linkSubTasksOut && linkSubTasksOut->empty());
  ASSERT(postLinkSubTasksOut && postLinkSubTasksOut->empty());
 
- mProgram->linkJobImpl(mExtensions);
+ mResult = mProgram->linkJobImpl(mExtensions);
 
  // If there is no native parallel compile, do the post-link right away.
- if (!mHasNativeParallelCompile)
+ if (mResult == angle::Result::Continue && !mHasNativeParallelCompile)
  {
  mResult = mProgram->postLinkJobImpl(resources);
  }
@@ -132,7 +132,7 @@ class ProgramGL::LinkTaskGL final : public LinkTask
  {
  ANGLE_TRACE_EVENT0("gpu.angle", "LinkTaskGL::getResult");
 
- if (mHasNativeParallelCompile)
+ if (mResult == angle::Result::Continue && mHasNativeParallelCompile)
  {
  mResult = mProgram->postLinkJobImpl(*mResources);
  }
@@ -293,7 +293,7 @@ angle::Result ProgramGL::link(const gl::Context *context, std::shared_ptr<LinkTa
  return angle::Result::Continue;
 }
 
-void ProgramGL::linkJobImpl(const gl::Extensions &extensions)
+angle::Result ProgramGL::linkJobImpl(const gl::Extensions &extensions)
 {
  ANGLE_TRACE_EVENT0("gpu.angle", "ProgramGL::linkJobImpl");
  const gl::ProgramExecutable &executable = mState.getExecutable();
@@ -374,6 +374,8 @@ void ProgramGL::linkJobImpl(const gl::Extensions &extensions)
  if (fragmentShader && fragmentShader->shaderVersion == 100 &&
  mFunctions->standard == STANDARD_GL_DESKTOP)
  {
+ ASSERT(!mFeatures.avoidBindFragDataLocation.enabled);
+
  const auto &shaderOutputs = fragmentShader->activeOutputVariables;
  for (const auto &output : shaderOutputs)
  {
@@ -418,59 +420,61 @@ void ProgramGL::linkJobImpl(const gl::Extensions &extensions)
  else if (fragmentShader && fragmentShader->shaderVersion >= 300)
  {
  // ESSL 3.00 and up.
- const auto &outputLocations = executable.getOutputLocations();
- const auto &secondaryOutputLocations = executable.getSecondaryOutputLocations();
- for (size_t outputLocationIndex = 0u; outputLocationIndex < outputLocations.size();
- ++outputLocationIndex)
- {
- const gl::VariableLocation &outputLocation =
- outputLocations[outputLocationIndex];
- if (outputLocation.arrayIndex == 0 && outputLocation.used() &&
- !outputLocation.ignored)
+ auto assignOutputLocations = [this](const std::vector<gl::VariableLocation>
+ &locations) {
+ const gl::ProgramExecutable &executable = mState.getExecutable();
+ for (size_t outputLocationIndex = 0u; outputLocationIndex < locations.size();
+ ++outputLocationIndex)
  {
- const gl::ProgramOutput &outputVar =
-  executable.getOutputVariables()[outputLocation.index];
- if (outputVar.pod.location == -1 || outputVar.pod.index == -1)
+ const gl::VariableLocation &outputLocation = locations[outputLocationIndex];
+ if (outputLocation.arrayIndex != 0 || !outputLocation.used() ||
+  outputLocation.ignored)
  {
- // We only need to assign the location and index via the API in case the
- // variable doesn't have a shader-assigned location and index. If a
- // variable doesn't have its location set in the shader it doesn't have
- // the index set either.
- ASSERT(outputVar.pod.index == -1);
- mFunctions->bindFragDataLocationIndexed(
- mProgramID, static_cast<int>(outputLocationIndex), 0,
- outputVar.mappedName.c_str());
+ continue;
  }
- }
- }
- for (size_t outputLocationIndex = 0u;
- outputLocationIndex < secondaryOutputLocations.size(); ++outputLocationIndex)
- {
- const gl::VariableLocation &outputLocation =
- secondaryOutputLocations[outputLocationIndex];
- if (outputLocation.arrayIndex == 0 && outputLocation.used() &&
- !outputLocation.ignored)
- {
+
  const gl::ProgramOutput &outputVar =
  executable.getOutputVariables()[outputLocation.index];
- if (outputVar.pod.location == -1 || outputVar.pod.index == -1)
+ if (outputVar.pod.hasShaderAssignedLocation)
+ {
+ continue;
+ }
+
+ // We only need to assign the location and index via the API if the variable
+ // doesn't have a shader-assigned location.
+ ASSERT(outputVar.pod.index != -1);
+
+ // Skip assignment due to Qualcomm driver issues.
+ if (mFeatures.avoidBindFragDataLocation.enabled)
  {
- // We only need to assign the location and index via the API in case the
- // variable doesn't have a shader-assigned location and index. If a
- // variable doesn't have its location set in the shader it doesn't have
- // the index set either.
- ASSERT(outputVar.pod.index == -1);
- mFunctions->bindFragDataLocationIndexed(
- mProgramID, static_cast<int>(outputLocationIndex), 1,
- outputVar.mappedName.c_str());
+ // Warn the user that they had attempted an explicit API assignment that
+ // cannot be honored
+ if (outputVar.pod.hasApiAssignedLocation)
+ {
+ executable.getInfoLog()
+ << "Unable to assign fragment data location for output "
+ "variable "
+ << outputVar.mappedName
+ << " due to driver issues. See http:https://anglebug.com/42267082";
+ }
+
+ continue;
  }
+
+ mFunctions->bindFragDataLocationIndexed(
+ mProgramID, static_cast<int>(outputLocationIndex), outputVar.pod.index,
+ outputVar.mappedName.c_str());
  }
- }
+ };
+
+ assignOutputLocations(executable.getOutputLocations());
+ assignOutputLocations(executable.getSecondaryOutputLocations());
  }
  }
  }
 
  mFunctions->linkProgram(mProgramID);
+ return angle::Result::Continue;
 }
 
 angle::Result ProgramGL::postLinkJobImpl(const gl::ProgramLinkedResources &resources)

src/libANGLE/renderer/gl/ProgramGL.h

@@ -76,7 +76,7 @@ class ProgramGL : public ProgramImpl
  friend class LinkTaskGL;
  friend class PostLinkGL;
 
- void linkJobImpl(const gl::Extensions &extensions);
+ angle::Result linkJobImpl(const gl::Extensions &extensions);
  angle::Result postLinkJobImpl(const gl::ProgramLinkedResources &resources);
 
  bool checkLinkStatus();

src/libANGLE/renderer/gl/renderergl_utils.cpp

@@ -2372,11 +2372,11 @@ void InitializeFeatures(const FunctionsGL *functions, angle::FeaturesGL *feature
  ANGLE_FEATURE_CONDITION(features, dontUseLoopsToInitializeVariables,
  (!isMesa && isQualcomm) || (isIntel && IsApple()));
 
- // Adreno drivers do not support glBindFragDataLocation* with MRT
  // Intel macOS condition ported from gpu_driver_bug_list.json (#327)
  ANGLE_FEATURE_CONDITION(features, disableBlendFuncExtended,
- (!isMesa && isQualcomm) ||
- (IsApple() && isIntel && GetMacOSVersion() < OSVersion(10, 14, 0)));
+ IsApple() && isIntel && GetMacOSVersion() < OSVersion(10, 14, 0));
+
+ ANGLE_FEATURE_CONDITION(features, avoidBindFragDataLocation, !isMesa && isQualcomm);
 
  ANGLE_FEATURE_CONDITION(features, unsizedSRGBReadPixelsDoesntTransform, !isMesa && isQualcomm);
 

src/tests/angle_end2end_tests_expectations.txt

@@ -490,6 +490,14 @@ b/273271471 WIN INTEL VULKAN : ShaderAlgorithmTest.rgb_to_hsl_vertex_shader/* =
 42266774 PIXEL4ORXL GLES : MultithreadingTestES3.SimultaneousUploadAndDraw/* = SKIP
 42266774 PIXEL4ORXL VULKAN : MultithreadingTestES3.SimultaneousUploadAndDraw/* = SKIP
 
+42267082 PIXEL4ORXL GLES : EXTBlendFuncExtendedDrawTestES3.ES3GettersArray/* = SKIP
+42267082 PIXEL4ORXL GLES : EXTBlendFuncExtendedDrawTestES3.ESSL3BindSimpleVarAsArrayNoBind/* = SKIP
+42267082 PIXEL4ORXL GLES : EXTBlendFuncExtendedDrawTestES3.FragmentArrayOutputLocationsAPI/* = SKIP
+42267082 PIXEL4ORXL GLES : EXTBlendFuncExtendedDrawTestES3.FragmentOutputLocationsAPI/* = SKIP
+42267082 PIXEL4ORXL GLES : EXTBlendFuncExtendedDrawTestES3.FragmentOutputLocationsAPIAndAutomatic/* = SKIP
+42267082 PIXEL4ORXL GLES : EXTBlendFuncExtendedDrawTestES3.MultipleDrawBuffersAPI/* = SKIP
+42267082 PIXEL4ORXL GLES : EXTBlendFuncExtendedTestES3.FragmentOutputLocationsPartiallyAutomatic/* = SKIP
+
 42262087 PIXEL4ORXL VULKAN : ClearTestES3.MaskedClearBufferBug/* = SKIP
 42262087 PIXEL4ORXL VULKAN : DrawBuffersWebGL2Test.TwoProgramsWithDifferentOutputsAndClear/* = SKIP
 42264480 PIXEL4ORXL VULKAN : TransformFeedbackTestES32.PrimitivesWrittenAndGenerated/* = SKIP
@@ -512,6 +520,7 @@ b/273271471 WIN INTEL VULKAN : ShaderAlgorithmTest.rgb_to_hsl_vertex_shader/* =
 42265338 PIXEL4ORXL VULKAN : ImageTest.SourceYUVAHBTargetExternalRGBSampleInitData/* = SKIP
 42265338 PIXEL4ORXL VULKAN : ImageTestES3.SourceYUVAHBTargetExternalYUVSampleLinearFiltering/* = SKIP
 42265581 PIXEL4ORXL VULKAN : ImageTestES3.SourceAHBMipTarget2DMip/* = SKIP
+
 // eglCreateImageKHR: AHardwareBuffer format 23 does not support enough features to use as a texture.
 298037344 PIXEL4ORXL VULKAN : ImageTestES3.RGBAHBUploadDataColorspace/* = SKIP
 

util/autogen/angle_features_autogen.cpp

@@ -51,6 +51,7 @@ constexpr PackedEnumMap<Feature, const char *> kFeatureNames = {{
  {Feature::AsyncCommandBufferReset, "asyncCommandBufferReset"},
  {Feature::AsyncCommandQueue, "asyncCommandQueue"},
  {Feature::Avoid1BitAlphaTextureFormats, "avoid1BitAlphaTextureFormats"},
+ {Feature::AvoidBindFragDataLocation, "avoidBindFragDataLocation"},
  {Feature::AvoidOpSelectWithMismatchingRelaxedPrecision, "avoidOpSelectWithMismatchingRelaxedPrecision"},
  {Feature::AvoidStencilTextureSwizzle, "avoidStencilTextureSwizzle"},
  {Feature::BindCompleteFramebufferForTimerQueries, "bindCompleteFramebufferForTimerQueries"},

util/autogen/angle_features_autogen.h

@@ -51,6 +51,7 @@ enum class Feature
  AsyncCommandBufferReset,
  AsyncCommandQueue,
  Avoid1BitAlphaTextureFormats,
+ AvoidBindFragDataLocation,
  AvoidOpSelectWithMismatchingRelaxedPrecision,
  AvoidStencilTextureSwizzle,
  BindCompleteFramebufferForTimerQueries,