Merge into development-branch : rfc-vulkan-submission-modes : Code : Mir

Status:

Merged

Approved by:

Cemil Azizoglu on 2017-03-08

Approved revision:

no longer in the source branch.

Merged at revision:

4075

Proposed branch:

lp://staging/~kdub/mir/rfc-vulkan-submission-modes

Merge into:

lp://staging/mir

Diff against target:

131 lines (+65/-1)

5 files modified

include/client/mir_toolkit/client_types.h (+9/-0)
include/client/mir_toolkit/rs/mir_render_surface.h (+22/-1)
src/client/mir_presentation_chain_api.cpp (+19/-0)
src/client/symbols.map (+2/-0)
tests/acceptance-tests/test_presentation_chain.cpp (+13/-0)

To merge this branch:

bzr merge lp://staging/~kdub/mir/rfc-vulkan-submission-modes

Wishlist

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Cemil Azizoglu (community)		2017-02-24	Approve on 2017-03-07
Mir CI Bot	continuous-integration		Approve on 2017-03-06
Review via email: mp+318257@code.staging.launchpad.net

Commit message

client: add different mode selection to MirPresentationChains. Better prepares for supporting more than one vulkan submission mode. (we currently support the required mode, fifo)

Description of the change

RFC, description of reasoning:

mir_connection_present_mode_supported:
A vulkan user can select from the 1st 4 modes, and the modes are negotiated with the server. (with FIFO being a required mode). This fn accommodates the negotiation for the client user, in the future this should be told by, or ask, the server about the support.

Different modes:
The 4 modes are the vulkan modes, so their inclusion is hopefully noncontroversial. The timeout-based framedropping policy only applies to MirBufferStream objects.

VK_PRESENT_MODE_FIFO_KHR and VK_PRESENT_MODE_MAILBOX_KHR roughly map to what we're doing now with swapinterval 1 and swapinterval zero, with the difference being that policy-based timeout frame dropping for MirBufferStreams. With MirPresentationChain, the timeout frame dropping doesn't happen.

VK_PRESENT_MODE_IMMEDIATE_KHR and VK_PRESENT_MODE_FIFO_RELAXED_KHR allow for tearing, so those will take some internal reworking to support. wgl swapinterval -1 roughly maps to FIFO_RELAXED. IMMEDIATE roughly maps to single buffer rendering.

mir_presentation_chain_set_mode:
AFAIK, you have to reconstruct the VkSwapChainKHR to change the mode, so vulkan doesnt need to set modes after construction. The reason to be able to change modes is so that our mesa egl driver can support egl swapinterval 0/1 properly (which does allow for post-construction mode setting).

chain create/destroy:
I think this maps better to vulkan's SwapChain management, where we could recreate a PresentationChain without having to renegotiate the RenderSurface. It also was easy to do, and let us add a parameter to construction without a name collision.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-02-24:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3062/rebuild

FAILED: Continuous integration, rev:4052
https://mir-jenkins.ubuntu.com/job/mir-ci/3062/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4094/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4181
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4171
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4171
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4171
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4121
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4121/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4121/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4121
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4121/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4121
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4121/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4121
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4121/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4121
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4121/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3062/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-02-24:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3065/rebuild

PASSED: Continuous integration, rev:4053
https://mir-jenkins.ubuntu.com/job/mir-ci/3065/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4097
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4184
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4174
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4174
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4174
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4124
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4124/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4124
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4124/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4124
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4124/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4124
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4124/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4124
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4124/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4124
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4124/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3065/rebuild

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2017-02-28:

#

1. As you pointed out, we are in no obligation to support all Vulkan submission modes, so we should not list modes that we don't yet support in our headers. We may never support some of them. We should also have mir_present_mode_num_modes at the end.

2. mir_connection_present_mode_supported()
Actually, the mode will depend on properties of the buffers in a swap chain. So this function should get this info from the platform (though I am not sure if we can assume that the app can always obtain the buffers with the exact properties it asks for, e.g. GBM_BO_USE_SCANOUT, GBM_BO_USE_LINEAR, etc).

3. I think the destroy function should not exist. We can fix the name collision with the create during Mir 1.0 break by adding a parameter to the get function. The destroy (which is only destroying the client side) can be implicit, i.e. get would be allowed to be called multiple times (with a different mode), and it would destroy the old PC and recreate the new one.

Sigh, all that said our client API is still not very intuitive. We should think about getting rid of BSs and only have PCs (which should be wrapped in a nice RS api). I think things should be cleaner. But that's probably a discussion to be had face-to-face during a sprint.

review: Needs Fixing

Revision history for this message

Kevin DuBois (kdub) wrote on 2017-03-06:

#

> 1. As you pointed out, we are in no obligation to support all Vulkan
> submission modes, so we should not list modes that we don't yet support in our
> headers. We may never support some of them. We should also have
> mir_present_mode_num_modes at the end.-

I think having all 4 modes in there at the start is good (I think we should support all the modes, we have had previous attempts to support something like IMMEDIATE (which was called nbuffers=1 at the time Daniel was experimenting with it)). Don't mind removing the unused ones, but would prefer to leave in.

>
> 2. mir_connection_present_mode_supported()
> Actually, the mode will depend on properties of the buffers in a swap chain.
> So this function should get this info from the platform (though I am not sure
> if we can assume that the app can always obtain the buffers with the exact
> properties it asks for, e.g. GBM_BO_USE_SCANOUT, GBM_BO_USE_LINEAR, etc).
>

Right, we'll probably need some further input from the platforms, but I think the signature will work for that as is? (MirConnection has access to platform-specific info)

> 3. I think the destroy function should not exist. We can fix the name
> collision with the create during Mir 1.0 break by adding a parameter to the
> get function. The destroy (which is only destroying the client side) can be
> implicit, i.e. get would be allowed to be called multiple times (with a
> different mode), and it would destroy the old PC and recreate the new one.
>
> Sigh, all that said our client API is still not very intuitive. We should
> think about getting rid of BSs and only have PCs (which should be wrapped in a
> nice RS api). I think things should be cleaner. But that's probably a
> discussion to be had face-to-face during a sprint.

Sure, although I think the new stuff by itself makes sense. BufferStream is legacy, and imo, that's where the confusion comes from. Its also been isolated (only SW MirBufferStreams can be created), and I'd be all for removing that support as well.

The current model, we have a chain or BS created from a RS, and that object has the same lifetime as the RS. We also cannot 'get' a chain/BS from a RS once a chain/BS has been made before. So this makes object creation and resource management a bit fiddly/confusing.

I think create/destroy gets rid of the re-creation and construction gotchas, but that can be addressed separately. (iirc, we decided not to address before 1.0 in the name of time)

> 1. As you pointed out, we are in no obligation to support all Vulkan
> submission modes, so we should not list modes that we don't yet support in our
> headers. We may never support some of them. We should also have
> mir_present_mode_num_modes at the end.-

I think having all 4 modes in there at the start is good (I think we should support all the modes, we have had previous attempts to support something like IMMEDIATE (which was called nbuffers=1 at the time Daniel was experimenting with it)). Don't mind removing the unused ones, but would prefer to leave in.

> 
> 2. mir_connection_present_mode_supported()
> Actually, the mode will depend on properties of the buffers in a swap chain.
> So this function should get this info from the platform (though I am not sure
> if we can assume that the app can always obtain   the buffers with the exact
> properties it asks for, e.g. GBM_BO_USE_SCANOUT, GBM_BO_USE_LINEAR, etc).
>

Right, we'll probably need some further input from the platforms, but I think the signature will work for that as is? (MirConnection has access to platform-specific info)

> 3. I think the destroy function should not exist. We can fix the name
> collision with the create during Mir 1.0 break by adding a parameter to the
> get function. The destroy (which is only destroying the client side) can be
> implicit, i.e. get would be allowed to be called multiple times (with a
> different mode), and it would destroy the old PC and recreate the new one.
> 
> Sigh, all that said our client API is still not very intuitive. We should
> think about getting rid of BSs and only have PCs (which should be wrapped in a
> nice RS api). I think things should be cleaner. But that's probably a
> discussion to be had face-to-face during a sprint.

Sure, although I think the new stuff by itself makes sense. BufferStream is legacy, and imo, that's where the confusion comes from. Its also been isolated (only SW MirBufferStreams can be created), and I'd be all for removing that support as well.

The current model, we have a chain or BS created from a RS, and that object has the same lifetime as the RS. We also cannot 'get' a chain/BS from a RS once a chain/BS has been made before. So this makes object creation and resource management a bit fiddly/confusing.

I think create/destroy gets rid of the re-creation and construction gotchas, but that can be addressed separately. (iirc, we decided not to address before 1.0 in the name of time)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-03-06:

#

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3106/rebuild

PASSED: Continuous integration, rev:4058
https://mir-jenkins.ubuntu.com/job/mir-ci/3106/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4164
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4251
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4241
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4241
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4241
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4191
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4191/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4191
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4191/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4191
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4191/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4191
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4191/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4191
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4191/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4191
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4191/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3106/rebuild

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2017-03-07:

#

Okay lgtm

review: Approve

Mir

Merge lp://staging/~kdub/mir/rfc-vulkan-submission-modes into lp://staging/mir

Commit message

Description of the change

Preview Diff

Subscribers