Skip to content

Commit

Permalink
Fix dEQP-EGL.functional.mutable_render_buffer#basic
Browse files Browse the repository at this point in the history
These are vulkan commands submitted between
glClear() and glReadPixels() when the EGL_RENDER_BUFFER
is EGL_SINGLE_BUFFER (ImageLayour is SharedPresent):

```
vkCmdClearColorImage()

vkCmdPipelineBarrier: (
VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,//srcStageMask
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,//dstStageMask
VK_ACCESS_MEMORY_WRITE_BIT,//srcAccessMask
VK_ACCESS_MEMORY_WRITE_BIT|VK_ACCESS_MEMORY_READ_BIT//dstAccessMask
)

vkCmdCopyImageToBuffer()
```

This means that operations at the bottom of pipeline
in vkCmdCopyImageToBuffer() need to wait for operations
at the top of pipeline in vmCmdClearColorImage(), which
translates to vkCmdCopyImageToBuffer() does not have
to wait for vkCmdClearColorImage() to finish.
Even the dstAccessMask ensures that
vkCmdCopyImageToBuffer() will invalidate cache before
copying image, it is possible that it will retrieve the
old Framebuffer color attachment data as the
vkCmdClearColorImage() has not finished.

This CL fixes the bug by making the srcStageMask to
be VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT and the
dstStageMask to be VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
when the ImageLayout is SharedPresent.
This ensures that vkCmdCopyImageToBuffer() waits for
vkCmdClearColorImage() to finish.

This CL also addresses similar issue in some other
rx::vk::ImageLayout items in kImageMemoryBarrierData.

Bug: b/264420030
Change-Id: If47ab071afaf96e396357cb0f50131339fa58509
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4198476
Commit-Queue: Yuxin Hu <[email protected]>
Reviewed-by: Shahbaz Youssefi <[email protected]>
Reviewed-by: Charlie Lao <[email protected]>
  • Loading branch information
HuYuxin authored and Angle LUCI CQ committed Feb 15, 2023
1 parent 79b0d80 commit 629da7f
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 4 deletions.
8 changes: 4 additions & 4 deletions src/libANGLE/renderer/vulkan/vk_helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
ImageMemoryBarrierData{
"Undefined",
VK_IMAGE_LAYOUT_UNDEFINED,
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,
VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,
// Transition to: we don't expect to transition into Undefined.
0,
// Transition from: there's no data in the image to care about.
Expand Down Expand Up @@ -371,8 +371,8 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
ImageMemoryBarrierData{
"Present",
VK_IMAGE_LAYOUT_PRESENT_SRC_KHR,
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,
VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,
// transition to: vkQueuePresentKHR automatically performs the appropriate memory barriers:
//
// > Any writes to memory backing the images referenced by the pImageIndices and
Expand All @@ -391,8 +391,8 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
ImageMemoryBarrierData{
"SharedPresent",
VK_IMAGE_LAYOUT_SHARED_PRESENT_KHR,
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,
VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,
// Transition to: all reads and writes must happen after barrier.
VK_ACCESS_MEMORY_READ_BIT | VK_ACCESS_MEMORY_WRITE_BIT,
// Transition from: all writes must finish before barrier.
Expand All @@ -415,7 +415,7 @@ constexpr angle::PackedEnumMap<ImageLayout, ImageMemoryBarrierData> kImageMemory
// layout. If the content is not already defined, the first operation may not be a
// glWaitSemaphore, but in this case undefined layout is appropriate.
VK_IMAGE_LAYOUT_UNDEFINED,
VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT,
VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT,
VK_PIPELINE_STAGE_HOST_BIT | VK_PIPELINE_STAGE_ALL_COMMANDS_BIT,
// Transition to: we don't expect to transition into PreInitialized.
0,
Expand Down
114 changes: 114 additions & 0 deletions src/tests/egl_tests/EGLSurfaceTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,8 @@ class EGLSingleBufferTest : public ANGLETest<>
return result;
}

uint32_t drawAndSwap(EGLSurface &surface, EGLDisplay &display, uint32_t color, bool flush);

EGLDisplay mDisplay = EGL_NO_DISPLAY;
EGLint mMajorVersion = 0;
const EGLint kWidth = 32;
Expand Down Expand Up @@ -2003,6 +2005,118 @@ TEST_P(EGLSingleBufferTest, OnSetSurfaceAttrib)
context = EGL_NO_CONTEXT;
}

uint32_t EGLSingleBufferTest::drawAndSwap(EGLSurface &surface,
EGLDisplay &display,
uint32_t color,
bool flush)
{
ASSERT(color < 256);

glClearColor((float)color / 255.f, (float)color / 255.f, (float)color / 255.f,
(float)color / 255.f);
glClear(GL_COLOR_BUFFER_BIT);

if (flush)
{
glFlush();
}
else
{
eglSwapBuffers(display, surface);
}

return (color | color << 8 | color << 16 | color << 24);
}

// Replicate dEQP-EGL.functional.mutable_render_buffer#basic
TEST_P(EGLSingleBufferTest, MutableRenderBuffer)
{
ANGLE_SKIP_TEST_IF(!IsEGLDisplayExtensionEnabled(mDisplay, "EGL_KHR_mutable_render_buffer"));

EGLConfig config = EGL_NO_CONFIG_KHR;
const EGLint attribs[] = {EGL_RED_SIZE,
8,
EGL_GREEN_SIZE,
8,
EGL_BLUE_SIZE,
8,
EGL_ALPHA_SIZE,
8,
EGL_SURFACE_TYPE,
EGL_WINDOW_BIT | EGL_MUTABLE_RENDER_BUFFER_BIT_KHR,
EGL_RENDERABLE_TYPE,
EGL_OPENGL_ES2_BIT,
EGL_NONE};
EGLint count = 0;
ANGLE_SKIP_TEST_IF(!eglChooseConfig(mDisplay, attribs, &config, 1, &count));

EGLContext context = EGL_NO_CONTEXT;
EXPECT_EGL_TRUE(createContext(config, &context));
ASSERT_EGL_SUCCESS() << "eglCreateContext failed.";

EGLSurface surface = EGL_NO_SURFACE;
OSWindow *osWindow = OSWindow::New();
osWindow->initialize("EGLSingleBufferTest", kWidth, kHeight);
EXPECT_EGL_TRUE(
createWindowSurface(config, osWindow->getNativeWindow(), &surface, EGL_BACK_BUFFER));
ASSERT_EGL_SUCCESS() << "eglCreateWindowSurface failed.";

EXPECT_EGL_TRUE(eglMakeCurrent(mDisplay, surface, surface, context));
ASSERT_EGL_SUCCESS() << "eglMakeCurrent failed.";

int frameNumber = 1;

// run a few back-buffered frames
for (; frameNumber < 5; frameNumber++)
{
drawAndSwap(surface, mDisplay, frameNumber, false);
}

if (eglSurfaceAttrib(mDisplay, surface, EGL_RENDER_BUFFER, EGL_SINGLE_BUFFER))
{
drawAndSwap(surface, mDisplay, frameNumber, false);
frameNumber++;

// test a few single-buffered frames
for (; frameNumber < 10; frameNumber++)
{
uint32_t backBufferPixel = 0xFFFFFFFF;
uint32_t frontBufferPixel = drawAndSwap(surface, mDisplay, frameNumber, true);
glReadPixels(0, 0, 1, 1, GL_RGBA, GL_UNSIGNED_BYTE, &backBufferPixel);
EXPECT_EQ(backBufferPixel, frontBufferPixel);
}
}

else
{
std::cout << "EGL_SINGLE_BUFFER mode is not supported." << std::endl;
}

// switch back to back-buffer rendering
if (eglSurfaceAttrib(mDisplay, surface, EGL_RENDER_BUFFER, EGL_BACK_BUFFER))
{
for (; frameNumber < 14; frameNumber++)
{
drawAndSwap(surface, mDisplay, frameNumber, false);
}
}
else
{
std::cout << "EGL_BACK_BUFFER mode is not supported." << std::endl;
}

EXPECT_EGL_TRUE(eglMakeCurrent(mDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, context));
ASSERT_EGL_SUCCESS() << "eglMakeCurrent - uncurrent failed.";

eglDestroySurface(mDisplay, surface);
surface = EGL_NO_SURFACE;
osWindow->destroy();
OSWindow::Delete(&osWindow);

eglDestroyContext(mDisplay, context);
context = EGL_NO_CONTEXT;
}

// Test that setting a surface to EGL_SINGLE_BUFFER after enabling
// EGL_FRONT_BUFFER_AUTO_REFRESH_ANDROID does not disable auto refresh
TEST_P(EGLAndroidAutoRefreshTest, Basic)
Expand Down

0 comments on commit 629da7f

Please sign in to comment.