Mir

include/renderers/gl/mir/renderer/gl/texture_source.h (+10/-0)
src/common/graphics/android/CMakeLists.txt (+1/-0)
src/common/graphics/android/android_native_buffer.cpp (+15/-1)
src/gl/recently_used_cache.cpp (+8/-7)
src/include/common/mir/graphics/android/android_native_buffer.h (+6/-0)
src/include/common/mir/graphics/android/native_buffer.h (+3/-0)
src/platform/symbols.map (+14/-0)
src/platforms/android/client/CMakeLists.txt (+2/-0)
src/platforms/android/client/gralloc_registrar.cpp (+3/-1)
src/platforms/android/server/CMakeLists.txt (+1/-0)
src/platforms/android/server/android_alloc_adaptor.cpp (+12/-5)
src/platforms/android/server/android_alloc_adaptor.h (+6/-2)
src/platforms/android/server/android_buffer_allocator.cpp (+10/-4)
src/platforms/android/server/android_graphic_buffer_allocator.h (+5/-1)
src/platforms/android/server/buffer.cpp (+23/-4)
src/platforms/android/server/buffer.h (+6/-0)
src/platforms/android/server/cmdstream_sync_factory.h (+48/-0)
src/platforms/android/server/display_component_factory.h (+2/-0)
src/platforms/android/server/egl_sync_factory.cpp (+34/-0)
src/platforms/android/server/hal_component_factory.cpp (+15/-0)
src/platforms/android/server/hal_component_factory.h (+3/-1)
src/platforms/android/server/ipc_operations.cpp (+1/-0)
src/platforms/android/server/platform.cpp (+10/-4)
src/platforms/android/server/platform.h (+3/-0)
src/platforms/mesa/server/common/gbm_buffer.cpp (+9/-0)
src/platforms/mesa/server/common/gbm_buffer.h (+2/-0)
src/platforms/mesa/server/common/shm_buffer.cpp (+9/-0)
src/platforms/mesa/server/common/shm_buffer.h (+2/-0)
tests/include/mir/test/doubles/mock_android_native_buffer.h (+2/-0)
tests/include/mir/test/doubles/mock_buffer.h (+1/-0)
tests/include/mir/test/doubles/mock_gl_buffer.h (+2/-0)
tests/include/mir/test/doubles/stub_android_native_buffer.h (+3/-0)
tests/include/mir/test/doubles/stub_cmdstream_sync_factory.h (+41/-0)
tests/include/mir/test/doubles/stub_display_builder.h (+5/-0)
tests/include/mir/test/doubles/stub_gl_buffer.h (+2/-0)
tests/integration-tests/graphics/mesa/test_buffer_integration.cpp (+3/-0)
tests/unit-tests/gl/test_gl_texture_cache.cpp (+3/-1)
tests/unit-tests/gl/test_program_factory.cpp (+1/-1)
tests/unit-tests/graphics/android/test_android_alloc_adaptor.cpp (+5/-2)
tests/unit-tests/graphics/android/test_android_buffer_allocator.cpp (+6/-2)
tests/unit-tests/graphics/android/test_buffer_tex_bind.cpp (+13/-1)
tests/unit-tests/graphics/android/test_display_hotplug.cpp (+5/-0)
tests/unit-tests/graphics/android/test_native_buffer.cpp (+50/-7)
tests/unit-tests/graphics/android/test_platform.cpp (+15/-4)

To merge this branch:

bzr merge lp://qastaging/~kdub/mir/egl-sync-fences

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-12-02
Alan Griffiths		2015-11-20	Approve on 2015-12-02
Cemil Azizoglu (community)			Approve on 2015-12-02
Andreas Pokorny (community)			Approve on 2015-11-30
Review via email: mp+278181@code.qastaging.launchpad.net

Commit message

android: use the EGL_fence_sync extensions to synchronize client software buffers.

fixes: lp: #1517205

Description of the change

android: use the EGL_fence_sync extensions to synchronize client software buffers.

fixes: lp: #1517205

tested on n7, krillin, mx4. (krillin and mx4 had additional problem that will be fixed later. This MP did not make their situation worse)

Summary of Changes:
1) When the texture is used in the GLContext, raise the egl sync fence.
2) When the buffer is sent back to the client, make sure to clear the sync fence before sending it across IPC.
3) Make sure that we actually eglSwapBuffers in mir::renderer::gl::RenderTarget::swap_buffers()

Helpful Details:
Why do we have to shift swapbuffers?
-- If we wait on the EGL sync fences before swapbuffers, then the wait will timeout, as the commands haven't been flushed, and the sync points won't signal. Android currently has the problem that the swapbuffers is delayed until mg::DisplayGroup::post. We shift swapbuffers so it is actually called before any waits on the sync fences can happen.

Why was swapbuffers in post?
-- HWC 1.0 has the unfortunate design that we're not allowed to call swapbuffers. HWC 1.1 and later, as well as the legacy FB module, can all call swapbuffers. We tried to bury this bad design choice, but this particular problem forces us to take different actions here. The problem we're seeing here is really why the HWC 1.1 api designers gave swapbuffers back to the server.

What was the problem?
glEGLTargetTexture2DOES will add the command to load the texture into the gpu command stream, but leaves it to us to hold the buffer until its done being used. eglSwapBuffers doesn't guarantee that we can release the buffer yet (as the gpu might need additional time to process after eglSwapBuffers returns); we have to synchronize using this fence extension.

What's still wrong with krillin/mx4?
https://bugs.launchpad.net/mir/+bug/1270245 (Which i'll hopefully track down and fix in a subsequent MP). The corruption caused by this bug looks visually the same as the corruption fixed by this MP.

note: this should land at the same time as the stacked on branch:
https://code.launchpad.net/~kdub/mir/android-shift-swapbuffers/+merge/278176
I split the MP up for the benefit of reviewing (esp as we have to shift swapping behavior a bit)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5612/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-20:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5617/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5653/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5668/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-30:

+ //must be called if using this buffer after initial texture upload
+ virtual void used_as_texture() = 0;

The comment suggests this is an easy-to-use-wrongly API and the name suggest a query, not a property setter.

I'm not very familiar with this code, but could gl_bind_to_texture() be made idempotent and called in both branches (which might then be merged into one)?

Alternatively, as the underlying action ("native_buffer->used_by_gpu()") is a side-effect of gl_bind_to_texture() in one branch shouldn't the above function be called "gl_reuse_texture()".

~~~~

+ void used_by_gpu();
+ void ensure_not_used_by_gpu();

Again, this naming isn't great. Maybe "lock_for_gpu()"/"wait_for_unlock_by_gpu()"?

review: Needs Information

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-11-30:

I agree with Alans comments, better naming is needed.
Under the premise of those being resolved - the solution looks good and solves the bug even on n10

review: Approve

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-12-01:

I got a failure on PD silo (which merges this branch) and fixed it using the following patch :

=== modified file 'tests/unit-tests/graphics/android/test_android_alloc_adaptor.cpp'
--- tests/unit-tests/graphics/android/test_android_alloc_adaptor.cpp 2015-08-07 15:55:22 +0000
+++ tests/unit-tests/graphics/android/test_android_alloc_adaptor.cpp 2015-12-01 03:51:15 +0000
@@ -18,11 +18,14 @@

#include "src/platforms/android/server/android_alloc_adaptor.h"
#include "src/platforms/android/server/device_quirks.h"
+#include "src/platforms/android/server/cmdstream_sync_factory.h"
#include "mir/graphics/android/native_buffer.h"

#include "mir/test/doubles/mock_android_alloc_device.h"
#include "mir/test/doubles/mock_alloc_adaptor.h"

+#include "mir/test/doubles/mock_egl.h"
+
#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <stdexcept>
@@ -47,6 +50,7 @@
using namespace testing;
mock_alloc_device = std::make_shared<NiceMock<mtd::MockAllocDevice>>();

+ auto sync_factory = std::make_shared<mga::EGLSyncFactory>();
auto quirks = std::make_shared<mga::DeviceQuirks>(mga::PropertiesOps{});
alloc_adaptor = std::make_shared<mga::AndroidAllocAdaptor>(mock_alloc_device, quirks);

@@ -57,6 +61,7 @@

std::shared_ptr<mtd::MockAllocDevice> mock_alloc_device;
std::shared_ptr<mga::AndroidAllocAdaptor> alloc_adaptor;
+ ::testing::NiceMock<mtd::MockEGL> mock_egl;

MirPixelFormat pf;
geom::Size size;

review: Needs Fixing

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-12-01:

Just realized the patch above was taken w.r.t. the trunk not this branch, so it will not apply cleanly. So just stick the added lines to your branch instead.

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-12-01:

Will try to improve the naming, although something that is nicer to use might take more invasive structural changes.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5735/rebuild

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-12-02:

> I'm not very familiar with this code, but could gl_bind_to_texture() be made idempotent and called
> in both branches (which might then be merged into one)?

gl_bind_to_texture possibly uploads the texture, so calling it twice when not needed will waste texture uploads. We need the second function that adds the sync point without uploading, mostly for texture cache usage.

Ideally, I'd change it to:
//uploads
gl_bind_to_texture()
//adds sync points
secure_for_render()

but this doesn't fix the bugs for downstreams.

I think I'll go with:
//deprecated/legacy, calls bind(), then secure_for_render()
gl_bind_to_texture()
//uploads
bind()
//adds sync points
secure_for_render()

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-12-02:

> > I'm not very familiar with this code, but could gl_bind_to_texture() be
> made idempotent and called
> > in both branches (which might then be merged into one)?
>
> gl_bind_to_texture possibly uploads the texture, so calling it twice when not
> needed will waste texture uploads. We need the second function that adds the
> sync point without uploading, mostly for texture cache usage.

I was wondering if gl_bind_to_texture() had enough context to "know" of the upload was unnecessary.

>
> Ideally, I'd change it to:
> //uploads
> gl_bind_to_texture()
> //adds sync points
> secure_for_render()
>
> but this doesn't fix the bugs for downstreams.

Its also an easy to misuse API as it requires a specific order of calls. It is almost as though secure_for_render() should return an object (a future?) whose destructor does the wait for the gpu release.

> I think I'll go with:
> //deprecated/legacy, calls bind(), then secure_for_render()
> gl_bind_to_texture()
> //uploads
> bind()
> //adds sync points
> secure_for_render()

OK, I don't have strong feelings against this, it is better than the current proposal.

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-12-02:

LGTM ...and works.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-12-02:

This naming isn't actively misleading, and I'll accept the remaining ugliness as largely preexisting.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-02: