Skip to content

Commit

Permalink
Metal: Handle incompatible attachments
Browse files Browse the repository at this point in the history 
Metal runtime fails if the attachment pixel
formats are not compatible with the program
outputs or if the corresponding render pass
and pipeline pixel formats do not match.

Added Metal-specific state tracking and forced
draw framebuffer syncronization for such cases.

Cleaned up and reduced Framebuffer::setAttachmentImpl.

Fixed: angleproject:5233
Change-Id: I4ee01889debe0e3cce54635e6cba62dbfdc02722
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/5377813
Reviewed-by: Shahbaz Youssefi <[email protected]>
Reviewed-by: Quyen Le <[email protected]>
Commit-Queue: Alexey Knyazev <[email protected]>
  • Loading branch information
@lexaknyazev
lexaknyazev authored and Angle LUCI CQ committed Apr 1, 2024
1 parent b8374d4 commit 18797bf
Show file tree
Hide file tree
Showing 8 changed files with 264 additions and 19 deletions.
25 changes: 14 additions & 11 deletions src/libANGLE/Framebuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2134,13 +2134,11 @@ void Framebuffer::setAttachmentImpl(const Context *context,

default:
{
size_t colorIndex = binding - GL_COLOR_ATTACHMENT0;
const size_t colorIndex = binding - GL_COLOR_ATTACHMENT0;
ASSERT(colorIndex < mState.mColorAttachments.size());
size_t dirtyBit = DIRTY_BIT_COLOR_ATTACHMENT_0 + colorIndex;
updateAttachment(context, &mState.mColorAttachments[colorIndex], dirtyBit,
&mDirtyColorAttachmentBindings[colorIndex], type, binding,
textureIndex, resource, numViews, baseViewIndex, isMultiview, samples);

// Caches must be updated before notifying the observers.
ComponentType componentType = ComponentType::NoType;
if (!resource)
{
mFloat32ColorAttachmentBits.reset(colorIndex);
Expand All @@ -2149,15 +2147,20 @@ void Framebuffer::setAttachmentImpl(const Context *context,
}
else
{
updateFloat32AndSharedExponentColorAttachmentBits(
colorIndex, resource->getAttachmentFormat(binding, textureIndex).info);
const InternalFormat *formatInfo =
resource->getAttachmentFormat(binding, textureIndex).info;
componentType = GetAttachmentComponentType(formatInfo->componentType);
updateFloat32AndSharedExponentColorAttachmentBits(colorIndex, formatInfo);
mState.mColorAttachmentsMask.set(colorIndex);
}

bool enabled = (type != GL_NONE && getDrawBufferState(colorIndex) != GL_NONE);
const bool enabled = (type != GL_NONE && getDrawBufferState(colorIndex) != GL_NONE);
mState.mEnabledDrawBuffers.set(colorIndex, enabled);
SetComponentTypeMask(getDrawbufferWriteType(colorIndex), colorIndex,
&mState.mDrawBufferTypeMask);
SetComponentTypeMask(componentType, colorIndex, &mState.mDrawBufferTypeMask);

const size_t dirtyBit = DIRTY_BIT_COLOR_ATTACHMENT_0 + colorIndex;
updateAttachment(context, &mState.mColorAttachments[colorIndex], dirtyBit,
&mDirtyColorAttachmentBindings[colorIndex], type, binding,
textureIndex, resource, numViews, baseViewIndex, isMultiview, samples);
}
break;
}
Expand Down
7 changes: 7 additions & 0 deletions src/libANGLE/angletypes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,13 @@ ANGLE_INLINE ComponentTypeMask GetActiveComponentTypeMask(gl::AttributesMask act
return ComponentTypeMask(activeAttribs << kMaxComponentTypeMaskIndex | activeAttribs);
}

ANGLE_INLINE DrawBufferMask GetComponentTypeMaskDiff(ComponentTypeMask mask1,
ComponentTypeMask mask2)
{
const uint32_t diff = static_cast<uint32_t>((mask1 ^ mask2).bits());
return DrawBufferMask(static_cast<uint8_t>(diff | (diff >> gl::kMaxComponentTypeMaskIndex)));
}

bool ValidateComponentTypeMasks(unsigned long outputTypes,
unsigned long inputTypes,
unsigned long outputMask,
Expand Down
10 changes: 10 additions & 0 deletions src/libANGLE/renderer/metal/ContextMtl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ class ContextMtl : public ContextImpl, public mtl::Context
void invalidateDriverUniforms();
void invalidateRenderPipeline();

void updateIncompatibleAttachments(const gl::State &glState);

// Call this to notify ContextMtl whenever FramebufferMtl's state changed
void onDrawFrameBufferChangedState(const gl::Context *context,
FramebufferMtl *framebuffer,
Expand Down Expand Up @@ -404,9 +406,13 @@ class ContextMtl : public ContextImpl, public mtl::Context

const angle::ImageLoadContext &getImageLoadContext() const { return mImageLoadContext; }

bool getForceResyncDrawFramebuffer() const { return mForceResyncDrawFramebuffer; }
gl::DrawBufferMask getIncompatibleAttachments() const { return mIncompatibleAttachments; }

private:
void ensureCommandBufferReady();
void endBlitAndComputeEncoding();
angle::Result resyncDrawFramebufferIfNeeded(const gl::Context *context);
angle::Result setupDraw(const gl::Context *context,
gl::PrimitiveMode mode,
GLint firstVertex,
Expand Down Expand Up @@ -619,6 +625,10 @@ class ContextMtl : public ContextImpl, public mtl::Context
MTLCullMode mCullMode;
bool mCullAllPolygons = false;

// Cached state to handle attachments incompatible with the current program
bool mForceResyncDrawFramebuffer = false;
gl::DrawBufferMask mIncompatibleAttachments;

mtl::BufferManager mBufferManager;

// Lineloop and TriFan index buffer
Expand Down
61 changes: 60 additions & 1 deletion src/libANGLE/renderer/metal/ContextMtl.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,31 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
return angle::Result::Continue;
}

ANGLE_INLINE angle::Result ContextMtl::resyncDrawFramebufferIfNeeded(const gl::Context *context)
{
// Resync the draw framebuffer if
// - it has incompatible attachments; OR
// - it had incompatible attachments during the previous operation.
if (ANGLE_UNLIKELY(mIncompatibleAttachments.any() || mForceResyncDrawFramebuffer))
{
if (mIncompatibleAttachments.any())
{
ANGLE_PERF_WARNING(context->getState().getDebug(), GL_DEBUG_SEVERITY_LOW,
"Resyncing the draw framebuffer because it has active attachments "
"incompatible with the current program outputs.");
}

// Ensure sync on the next operation if the current state has incompatible attachments.
mForceResyncDrawFramebuffer = mIncompatibleAttachments.any();

FramebufferMtl *fbo = mtl::GetImpl(getState().getDrawFramebuffer());
ASSERT(fbo != nullptr);
return fbo->syncState(context, GL_DRAW_FRAMEBUFFER, gl::Framebuffer::DirtyBits(),
gl::Command::Draw);
}
return angle::Result::Continue;
}

// Drawing methods.
angle::Result ContextMtl::drawTriFanArraysWithBaseVertex(const gl::Context *context,
GLint first,
Expand Down Expand Up @@ -508,6 +533,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
GLint first,
GLsizei count)
{
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
return drawArraysImpl(context, mode, first, count, 0, 0);
}

Expand All @@ -520,6 +546,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
// Instanced draw calls with zero instances are skipped in the frontend.
// The drawArraysImpl function would treat them as non-instanced.
ASSERT(instances > 0);
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
return drawArraysImpl(context, mode, first, count, instances, 0);
}

Expand All @@ -533,6 +560,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
// Instanced draw calls with zero instances are skipped in the frontend.
// The drawArraysImpl function would treat them as non-instanced.
ASSERT(instanceCount > 0);
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
return drawArraysImpl(context, mode, first, count, instanceCount, baseInstance);
}

Expand Down Expand Up @@ -858,6 +886,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
gl::DrawElementsType type,
const void *indices)
{
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
return drawElementsImpl(context, mode, count, type, indices, 0, 0, 0);
}

Expand All @@ -879,6 +908,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
const void *indices,
GLsizei instanceCount)
{
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
// Instanced draw calls with zero instances are skipped in the frontend.
// The drawElementsImpl function would treat them as non-instanced.
ASSERT(instanceCount > 0);
Expand All @@ -893,6 +923,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
GLsizei instanceCount,
GLint baseVertex)
{
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
// Instanced draw calls with zero instances are skipped in the frontend.
// The drawElementsImpl function would treat them as non-instanced.
ASSERT(instanceCount > 0);
Expand All @@ -908,6 +939,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
GLint baseVertex,
GLuint baseInstance)
{
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
// Instanced draw calls with zero instances are skipped in the frontend.
// The drawElementsImpl function would treat them as non-instanced.
ASSERT(instances > 0);
Expand All @@ -923,6 +955,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
gl::DrawElementsType type,
const void *indices)
{
ANGLE_TRY(resyncDrawFramebufferIfNeeded(context));
return drawElementsImpl(context, mode, count, type, indices, 0, 0, 0);
}

Expand Down Expand Up @@ -1086,6 +1119,23 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
return angle::Result::Continue;
}

void ContextMtl::updateIncompatibleAttachments(const gl::State &glState)
{
const gl::ProgramExecutable *programExecutable = glState.getProgramExecutable();
gl::Framebuffer *drawFramebuffer = glState.getDrawFramebuffer();
if (programExecutable == nullptr || drawFramebuffer == nullptr)
{
mIncompatibleAttachments.reset();
return;
}

// Cache a mask of incompatible attachments ignoring unused outputs and disabled draw buffers.
mIncompatibleAttachments =
gl::GetComponentTypeMaskDiff(drawFramebuffer->getDrawBufferTypeMask(),
programExecutable->getFragmentOutputsTypeMask()) &
drawFramebuffer->getDrawBufferMask() & programExecutable->getActiveOutputVariablesMask();
}

// State sync with dirty bits.
angle::Result ContextMtl::syncState(const gl::Context *context,
const gl::state::DirtyBits dirtyBits,
Expand Down Expand Up @@ -1267,6 +1317,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
case gl::state::DIRTY_BIT_READ_FRAMEBUFFER_BINDING:
break;
case gl::state::DIRTY_BIT_DRAW_FRAMEBUFFER_BINDING:
updateIncompatibleAttachments(glState);
updateDrawFrameBufferBinding(context);
break;
case gl::state::DIRTY_BIT_RENDERBUFFER_BINDING:
Expand All @@ -1286,6 +1337,7 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
break;
case gl::state::DIRTY_BIT_PROGRAM_EXECUTABLE:
{
updateIncompatibleAttachments(glState);
const gl::ProgramExecutable *executable = mState.getProgramExecutable();
ASSERT(executable);
mExecutable = mtl::GetImpl(executable);
Expand Down Expand Up @@ -2199,7 +2251,14 @@ GLint GetOwnershipIdentity(const egl::AttributeMap &attribs)
{
const gl::State &glState = getState();

mDrawFramebuffer = mtl::GetImpl(glState.getDrawFramebuffer());
FramebufferMtl *newDrawFramebuffer = mtl::GetImpl(glState.getDrawFramebuffer());
if (newDrawFramebuffer != mDrawFramebuffer)
{
// Reset this flag if the framebuffer has changed to not sync it twice
mForceResyncDrawFramebuffer = false;
}

mDrawFramebuffer = newDrawFramebuffer;

mDrawFramebuffer->onStartedDrawingToFrameBuffer(context);

Expand Down
4 changes: 3 additions & 1 deletion src/libANGLE/renderer/metal/FrameBufferMtl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,9 @@ class FramebufferMtl : public FramebufferImpl
const bool forceDepthStencilMultisampleLoad);

// Fill RenderPassDesc with relevant attachment's info from GL front end.
angle::Result prepareRenderPass(const gl::Context *context, mtl::RenderPassDesc *descOut);
angle::Result prepareRenderPass(const gl::Context *context,
mtl::RenderPassDesc *descOut,
gl::Command command);

// Check if a render pass specified by the given RenderPassDesc has started or not, if not this
// method will start the render pass and return its render encoder.
Expand Down
56 changes: 53 additions & 3 deletions src/libANGLE/renderer/metal/FrameBufferMtl.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,12 @@ void OverrideMTLClearColor(const mtl::TextureRef &texture,
{
ContextMtl *contextMtl = mtl::GetImpl(context);

if (ANGLE_UNLIKELY(contextMtl->getForceResyncDrawFramebuffer()))
{
ANGLE_TRY(syncState(context, GL_DRAW_FRAMEBUFFER, gl::Framebuffer::DirtyBits(),
gl::Command::Clear));
}

mtl::ClearRectParams clearOpts;

bool clearColor = IsMaskFlagSet(mask, static_cast<GLbitfield>(GL_COLOR_BUFFER_BIT));
Expand Down Expand Up @@ -206,6 +212,12 @@ void OverrideMTLClearColor(const mtl::TextureRef &texture,
GLint drawbuffer,
const GLfloat *values)
{
if (ANGLE_UNLIKELY(mtl::GetImpl(context)->getForceResyncDrawFramebuffer()))
{
ANGLE_TRY(syncState(context, GL_DRAW_FRAMEBUFFER, gl::Framebuffer::DirtyBits(),
gl::Command::Clear));
}

mtl::ClearRectParams clearOpts;

gl::DrawBufferMask clearColorBuffers;
Expand All @@ -226,6 +238,12 @@ void OverrideMTLClearColor(const mtl::TextureRef &texture,
GLint drawbuffer,
const GLuint *values)
{
if (ANGLE_UNLIKELY(mtl::GetImpl(context)->getForceResyncDrawFramebuffer()))
{
ANGLE_TRY(syncState(context, GL_DRAW_FRAMEBUFFER, gl::Framebuffer::DirtyBits(),
gl::Command::Clear));
}

gl::DrawBufferMask clearColorBuffers;
clearColorBuffers.set(drawbuffer);

Expand All @@ -239,6 +257,12 @@ void OverrideMTLClearColor(const mtl::TextureRef &texture,
GLint drawbuffer,
const GLint *values)
{
if (ANGLE_UNLIKELY(mtl::GetImpl(context)->getForceResyncDrawFramebuffer()))
{
ANGLE_TRY(syncState(context, GL_DRAW_FRAMEBUFFER, gl::Framebuffer::DirtyBits(),
gl::Command::Clear));
}

mtl::ClearRectParams clearOpts;

gl::DrawBufferMask clearColorBuffers;
Expand Down Expand Up @@ -459,6 +483,12 @@ void RoundValueAndAdjustCorrespondingValue(float a,
return angle::Result::Continue;
}

if (ANGLE_UNLIKELY(mtl::GetImpl(context)->getForceResyncDrawFramebuffer()))
{
ANGLE_TRY(syncState(context, GL_DRAW_FRAMEBUFFER, gl::Framebuffer::DirtyBits(),
gl::Command::Blit));
}

const gl::Rectangle srcFramebufferDimensions = srcFrameBuffer->getCompleteRenderArea();
const gl::Rectangle dstFramebufferDimensions = this->getCompleteRenderArea();

Expand Down Expand Up @@ -753,7 +783,21 @@ void RoundValueAndAdjustCorrespondingValue(float a,
}
}

ANGLE_TRY(prepareRenderPass(context, &mRenderPassDesc));
// If attachments have been changed and this is the current draw framebuffer,
// update the Metal context's incompatible attachments cache before preparing a render pass.
static_assert(gl::Framebuffer::DIRTY_BIT_COLOR_ATTACHMENT_0 == 0, "FB dirty bits");
constexpr gl::Framebuffer::DirtyBits kAttachmentsMask =
gl::Framebuffer::DirtyBits::Mask(gl::Framebuffer::DIRTY_BIT_COLOR_ATTACHMENT_MAX);
if (mustNotifyContext || (dirtyBits & kAttachmentsMask).any())
{
const gl::State &glState = context->getState();
if (mtl::GetImpl(glState.getDrawFramebuffer()) == this)
{
contextMtl->updateIncompatibleAttachments(glState);
}
}

ANGLE_TRY(prepareRenderPass(context, &mRenderPassDesc, command));
bool renderPassChanged = !oldRenderPassDesc.equalIgnoreLoadStoreOptions(mRenderPassDesc);

if (mustNotifyContext || renderPassChanged)
Expand Down Expand Up @@ -1015,9 +1059,15 @@ void RoundValueAndAdjustCorrespondingValue(float a,
}

angle::Result FramebufferMtl::prepareRenderPass(const gl::Context *context,
mtl::RenderPassDesc *pDescOut)
mtl::RenderPassDesc *pDescOut,
gl::Command command)
{
const gl::DrawBufferMask enabledDrawBuffers = getState().getEnabledDrawBuffers();
// Skip incompatible attachments for draw ops to avoid triggering Metal runtime failures.
const gl::DrawBufferMask incompatibleAttachments =
(command == gl::Command::Draw) ? mtl::GetImpl(context)->getIncompatibleAttachments()
: gl::DrawBufferMask();
const gl::DrawBufferMask enabledDrawBuffers =
getState().getEnabledDrawBuffers() & ~incompatibleAttachments;

mtl::RenderPassDesc &desc = *pDescOut;

Expand Down
3 changes: 0 additions & 3 deletions src/tests/angle_end2end_tests_expectations.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,8 +336,6 @@ b/273271471 WIN INTEL VULKAN : ShaderAlgorithmTest.rgb_to_hsl_vertex_shader/* =

6456 MAC METAL INTEL : Texture2DTest.TextureSizeCase1/* = SKIP

5233 MAC METAL : WebGL2ValidationStateChangeTest.MultiAttachmentDrawFramebufferNegativeAPI/* = SKIP

7451 MAC METAL INTEL : PointSpritesTest.PointSizeAboveMaxIsClamped/* = SKIP

// New failures on MacBook Pro 2019 macOS 13.2.1
Expand Down Expand Up @@ -834,7 +832,6 @@ b/330697097 PIXEL6 VULKAN : FramebufferTest_ES31.MultisampleResolveBothAttachmen
7994 IOS METAL : UniformBlockWithOneLargeArrayMemberTest.MemberTypeIsStructAndInstancedArray/* = SKIP
7994 IOS METAL : UniformBlockWithOneLargeArrayMemberTest.SharedSameBufferWithOtherOne/* = SKIP
7994 IOS METAL : UniformBlockWithOneLargeArrayMemberTest.TwoUniformBlocksInSameProgram/* = SKIP
7994 IOS METAL : WebGL2ValidationStateChangeTest.MultiAttachmentDrawFramebufferNegativeAPI/* = SKIP
8043 IOS METAL : FramebufferTest_ES3.RenderSharedExponent/* = SKIP
8051 IOS METAL : Texture2DDepthStencilTestES3.TexSampleModes*Swizzled/* = SKIP
8278 IOS METAL : GLSLTest_ES3.LiteralInfinityOutput/* = SKIP
Expand Down
Loading

0 comments on commit 18797bf

Please sign in to comment.