Mir

Merge lp://staging/~kdub/mir/egl-sync-khr into lp://staging/mir

egl-sync-khr
Merge into development-branch

Proposed by Kevin DuBois on 2015-11-16

Status:

Merged

Approved by:

Alan Griffiths on 2015-11-27

Approved revision:

no longer in the source branch.

Merged at revision:

3137

Proposed branch:

lp://staging/~kdub/mir/egl-sync-khr

Merge into:

lp://staging/mir

Diff against target:

581 lines (+427/-20)

10 files modified

include/test/mir/test/doubles/mock_egl.h (+4/-0)
src/include/platform/mir/graphics/egl_extensions.h (+9/-0)
src/include/platform/mir/graphics/egl_sync_fence.h (+82/-0)
src/platform/graphics/CMakeLists.txt (+1/-0)
src/platform/graphics/egl_extensions.cpp (+19/-10)
src/platform/graphics/egl_sync_fence.cpp (+93/-0)
tests/mir_test_doubles/mock_egl.cpp (+27/-0)
tests/unit-tests/graphics/CMakeLists.txt (+1/-0)
tests/unit-tests/graphics/test_egl_extensions.cpp (+40/-10)
tests/unit-tests/graphics/test_egl_sync_fence.cpp (+151/-0)

To merge this branch:

bzr merge lp://staging/~kdub/mir/egl-sync-khr

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Approve on 2015-11-26
Chris Halse Rogers			Approve on 2015-11-26
PS Jenkins bot (community)	continuous-integration		Approve on 2015-11-25
Alberto Aguirre (community)		2015-11-16	Approve on 2015-11-20
Review via email: mp+277578@code.staging.launchpad.net

Commit message

graphics: add objects for supporting the EGL_KHR_fence_sync extensions

Description of the change

graphics: add objects for supporting the EGL_KHR_fence_sync extensions*

*corrected from original description to make note of correct extension.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-11-16:

Looks ok, although I would prefer to remove raise and put that chunk of code in in the constructor instead.

Any reason why we need to create one object and call raise repeatedly on it?

Why not create EGLSync objects as needed and then just have a wait and wait_for api?

review: Needs Information

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-11-17:

Yeah, this doesn't currently use the “reusable” bit of EGL_KHR_reusable_sync.

It would seem to be clearer to either have this be an *actual* EGLSyncKHR proxy¹ or make it more obviously single-shot².

¹: so, take an EGLDisplay in the constructor, construct the EGLSyncKHR, and then the sync can be repeatedly raised/waited on without being destroyed. This would seem to eliminate the need for the locking, too.

²: raise() in the constructor, permanently invalidate after waiting-on.

review: Needs Information

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-11-18:

I mistyped in the description. This is support for KHR_fence_sync, not KHR_reusable_sync (the latter of which isn't on some of our devices)

Having one object that we're not creating and recreating is preferable, just because I intend to just have one instance of this placed into mga::NativeFence. A singular instance sync fence instance in mga::NativeFence helps resolve some subtle problems with some versions of HWC (1.0).

Its also struck me as unintuitive that creating (ie, make_shared<>) the object implicitly inserts a sync point to the GL command stream at the time of creation. IMO, Its a bit more clear in that less comments have to be read if we have to call raise() explicitly.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-11-18:

I would prefer just "wait_for" instead of clear_or_timeout_after

And have wait_for throw if the fence has been reset or it has already been waited on - at least so we can detect programming errors in which a wait_for call is made before raise.

review: Needs Information

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-11-19:

@Alberto,
Changed to suggested name.

I'd rather have wait_for() just return as cleared if the fence hasn't been raised. This lets the waiting bit of code have less communication with the code that's raising the fence.

Eg: If we fixed https://bugs.launchpad.net/mir/+bug/1395421 by shoving the buffer sending code into its own thread, we'd need less communication between the threads; we'd just call wait_for() in the ipc thread.

Even in the current setup, we could drop some buffers without raising a fence on them. As the code is, wait_for() will just return.

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-11-20:

> Changed to suggested name.
but forgot to push until today/friday

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-11-20:

OK, I guess since EGLSyncFence is attached to a buffer, starting in signaled state can make sense (i.e. resource is available). And raise pulses that availability. I suppose the ordering of the raise/wait_for calls is externally enforced through the BufferQueue/BufferStream mechanisms already.

review: Approve

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/5611/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

FAILED: Continuous integration, rev:3113
http://jenkins.qa.ubuntu.com/job/mir-ci/5662/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5022
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3928
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4969
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/925/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1814/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/922
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/922/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4969
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4969/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7516
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25494
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/926/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5662/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/5669/rebuild

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-11-26:

+TEST_F(EglSyncFence, raise_sets_sync_point)
+{
+ Sequence seq;
+ EXPECT_CALL(mock_egl, eglGetCurrentDisplay());
+ EXPECT_CALL(mock_egl, eglCreateSyncKHR(mock_egl.fake_egl_display, EGL_SYNC_FENCE_KHR, nullptr))
+ .WillOnce(Return(fake_fence0));
+ EXPECT_CALL(mock_egl, eglDestroySyncKHR(mock_egl.fake_egl_display, fake_fence0))
+ .InSequence(seq); //destructor should call this

You don't need the Sequence here, as there's only one thing in the sequence?

Otherwise looks good.

review: Approve

Revision history for this message

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

> +TEST_F(EglSyncFence, raise_sets_sync_point)
> +{
> + Sequence seq;
> + EXPECT_CALL(mock_egl, eglGetCurrentDisplay());
> + EXPECT_CALL(mock_egl, eglCreateSyncKHR(mock_egl.fake_egl_display,
> EGL_SYNC_FENCE_KHR, nullptr))
> + .WillOnce(Return(fake_fence0));
> + EXPECT_CALL(mock_egl, eglDestroySyncKHR(mock_egl.fake_egl_display,
> fake_fence0))
> + .InSequence(seq); //destructor should call this
>
> You don't need the Sequence here, as there's only one thing in the sequence?

In the cited case (and others) ".InSequence(seq)" call is actually redundant and misleading as, by default, the sequence will be applied to any expectations set while "seq" is in scope.

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-11-26:

I believe that you're thinking of the InSequence type, rather than
Sequence? InSequence is the scope based auto-sequence; Sequence needs to be
explicitly used in a .InSequence() expectation.

Revision history for this message

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

> I believe that you're thinking of the InSequence type, rather than
> Sequence? InSequence is the scope based auto-sequence; Sequence needs to be
> explicitly used in a .InSequence() expectation.

right