Revert "GL: Avoid calling glBindFragDatalocationIndexed on Qualcomm"

This reverts commit e768aed.

Reason for revert: Breaks WebGL tests.

Original change's description:
> 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]>

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

include/platform/autogen/FeaturesGL_autogen.h

@@ -739,13 +739,6 @@ 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"
- };
-
  FeatureInfo srgbBlendingBroken = {
  "srgbBlendingBroken",
  FeatureCategory::OpenGLWorkarounds,

include/platform/gl_features.json

@@ -802,14 +802,6 @@
  ],
  "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"
- },
  {
  "name": "srgb_blending_broken",
  "category": "Workarounds",

src/libANGLE/ProgramExecutable.cpp

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

src/libANGLE/ProgramExecutable.h

@@ -175,9 +175,7 @@ struct ProgramOutput
  uint32_t isBuiltIn : 1;
  uint32_t isArray : 1;
  uint32_t hasImplicitLocation : 1;
- uint32_t hasShaderAssignedLocation : 1;
- uint32_t hasApiAssignedLocation : 1;
- uint32_t pad : 25;
+ uint32_t pad : 27;
  } 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());
 
- mResult = mProgram->linkJobImpl(mExtensions);
+ mProgram->linkJobImpl(mExtensions);
 
  // If there is no native parallel compile, do the post-link right away.
- if (mResult == angle::Result::Continue && !mHasNativeParallelCompile)
+ if (!mHasNativeParallelCompile)
  {
  mResult = mProgram->postLinkJobImpl(resources);
  }
@@ -132,7 +132,7 @@ class ProgramGL::LinkTaskGL final : public LinkTask
  {
  ANGLE_TRACE_EVENT0("gpu.angle", "LinkTaskGL::getResult");
 
- if (mResult == angle::Result::Continue && mHasNativeParallelCompile)
+ if (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;
 }
 
-angle::Result ProgramGL::linkJobImpl(const gl::Extensions &extensions)
+void ProgramGL::linkJobImpl(const gl::Extensions &extensions)
 {
  ANGLE_TRACE_EVENT0("gpu.angle", "ProgramGL::linkJobImpl");
  const gl::ProgramExecutable &executable = mState.getExecutable();
@@ -374,8 +374,6 @@ angle::Result 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)
  {
@@ -420,61 +418,59 @@ angle::Result ProgramGL::linkJobImpl(const gl::Extensions &extensions)
  else if (fragmentShader && fragmentShader->shaderVersion >= 300)
  {
  // ESSL 3.00 and up.
- 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 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)
  {
- const gl::VariableLocation &outputLocation = locations[outputLocationIndex];
- if (outputLocation.arrayIndex != 0 || !outputLocation.used() ||
- outputLocation.ignored)
- {
- continue;
- }
-
  const gl::ProgramOutput &outputVar =
  executable.getOutputVariables()[outputLocation.index];
- if (outputVar.pod.hasShaderAssignedLocation)
+ if (outputVar.pod.location == -1 || outputVar.pod.index == -1)
  {
- continue;
+ // 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());
  }
-
- // 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)
+ }
+ }
+ 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)
  {
- // 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;
+ // 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());
  }
-
- 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;
 
- angle::Result linkJobImpl(const gl::Extensions &extensions);
+ void 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,
- IsApple() && isIntel && GetMacOSVersion() < OSVersion(10, 14, 0));
-
- ANGLE_FEATURE_CONDITION(features, avoidBindFragDataLocation, !isMesa && isQualcomm);
+ (!isMesa && isQualcomm) ||
+ (IsApple() && isIntel && GetMacOSVersion() < OSVersion(10, 14, 0)));
 
  ANGLE_FEATURE_CONDITION(features, unsizedSRGBReadPixelsDoesntTransform, !isMesa && isQualcomm);
 

src/tests/angle_end2end_tests_expectations.txt

@@ -490,14 +490,6 @@ 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
@@ -520,7 +512,6 @@ 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,7 +51,6 @@ 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::BgraTexImageFormatsBroken, "bgraTexImageFormatsBroken"},

util/autogen/angle_features_autogen.h

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