Mir

Merge lp:~kdub/mir/gbm-ext-v2 into lp:mir

Proposed by Kevin DuBois on 2017-02-17
Status: Merged
Approved by: Daniel van Vugt on 2017-03-15
Approved revision: 4050
Merged at revision: 4088
Proposed branch: lp:~kdub/mir/gbm-ext-v2
Merge into: lp:mir
Diff against target: 537 lines (+349/-15)
6 files modified
include/client/mir_toolkit/extensions/gbm_buffer.h (+90/-0)
src/platforms/mesa/client/client_buffer.cpp (+8/-4)
src/platforms/mesa/client/client_platform.cpp (+149/-5)
src/platforms/mesa/client/client_platform.h (+2/-1)
src/platforms/mesa/include/native_buffer.h (+3/-0)
tests/unit-tests/platforms/mesa/client/test_client_platform.cpp (+97/-5)
To merge this branch: bzr merge lp:~kdub/mir/gbm-ext-v2
Reviewer Review Type Date Requested Status
Andreas Pokorny (community) Approve on 2017-03-14
Alan Griffiths Approve on 2017-03-14
Daniel van Vugt Needs Fixing on 2017-03-13
Mir CI Bot continuous-integration Approve on 2017-03-10
Chris Halse Rogers Approve on 2017-02-23
Cemil Azizoglu (community) 2017-02-17 Approve on 2017-02-21
Review via email: mp+317620@code.launchpad.net

Commit message

add MirExtensionGbmBufferV2, which adds functions that help a GBM buffer import in the mesa driver. This prepares mir_buffer_get_buffer_packgae (and MirBufferPackage) for deprecation.

Integrated with the 'rs' platform (in the current naming) Mesa patch on the ML.

Description of the change

add MirExtensionGbmBufferV2, which adds functions that help a GBM buffer import in the mesa driver. This prepares mir_buffer_get_buffer_packgae (and MirBufferPackage) for deprecation.

Integrated with the 'rs' platform (in the current naming) Mesa patch on the ML.

Also, spent some time considering whether the extension should just return a gbm_bo*. This was a bit disadvantageous, because we'd have to link the mesa client plugin to gbm (which it currently doesn't need), and we would need a fair amount of plumbing work to get a gbm_device on the client side.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4039
https://mir-jenkins.ubuntu.com/job/mir-ci/3013/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4023/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4110
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4100
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4100
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4100
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4050
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4050/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/4050/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4050
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4050/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/4050
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4050/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/4050
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4050/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/4050
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4050/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
Cemil Azizoglu (cemil-azizoglu) wrote :

Looks good. Some minor typos.

return comment is wrong.

41 +/** Access the import fd a MirBuffer
42 + * \pre The buffer is suitable for GBM_BO_IMPORT_FD
43 + * \warning The fd is owned by the buffer. Do not close() it.
44 + * \param [in] buffer The buffer
45 + * \return The stride of the buffer
46 + */
47 +typedef int (*MirBufferExtFd)(MirBuffer* buffer);
---------------------------------------------------------------
s/Check/Get

49 +/** Check the stride of a MirBuffer
50 + * \pre The buffer is suitable for GBM_BO_IMPORT_FD
51 + * \param [in] buffer The buffer
52 + * \return The stride of the buffer
53 + */
54 +typedef uint32_t (*MirBufferExtStride)(MirBuffer* buffer);

56 +/** Check the GBM_FORMAT of a MirBuffer
57 + * \pre The buffer is suitable for GBM_BO_IMPORT_FD
58 + * \param [in] buffer The buffer
59 + * \return The GBM_FORMAT of the buffer
60 + */
61 +typedef uint32_t (*MirBufferExtFormat)(MirBuffer* buffer);
-----------------------------------------------------------------------------
s/Check the gbm_bo_flags/Get buffer's age

70 +/** Check the gbm_bo_flags of a MirBuffer
71 + * \pre The buffer is suitable for GBM_BO_IMPORT_FD
72 + * \param [in] buffer The buffer
73 + * \return The age of the buffer
74 + */
75 +typedef uint64_t (*MirBufferExtAge)(MirBuffer* buffer);
76

review: Needs Fixing
lp:~kdub/mir/gbm-ext-v2 updated on 2017-02-20
4040. By Kevin DuBois on 2017-02-20

merge in mir

4041. By Kevin DuBois on 2017-02-20

fix up header documentation

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4041
https://mir-jenkins.ubuntu.com/job/mir-ci/3017/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4029
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4116
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4106
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4106
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4106
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4056
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4056/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/4056
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4056/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4056
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4056/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/4056
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4056/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/4056
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4056/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/4056
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4056/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Cemil Azizoglu (cemil-azizoglu) wrote :

lgtm

review: Approve
Chris Halse Rogers (raof) wrote :

This:
- gbm_buffer{allocate_buffer_gbm},
+ gbm_buffer1{allocate_buffer_gbm},
+ gbm_buffer2{allocate_buffer_gbm, allocate_buffer_gbm_sync,
+ is_gbm_importable, import_fd, buffer_stride, buffer_format, buffer_flags, buffer_age},

is unnecessary; just change the type of gbm_buffer to MirExtensionGbmBufferV2.

Likewise,

     if (!strcmp(extension_name, "mir_extension_gbm_buffer") && (version == 1))
- return &gbm_buffer;
+ return &gbm_buffer1;
+ if (!strcmp(extension_name, "mir_extension_gbm_buffer") && (version == 2))
+ return &gbm_buffer2;

Is unnecessary; just check for version <= 2, and return gbm_buffer.

Otherwise looks good!

review: Needs Fixing
Kevin DuBois (kdub) wrote :

good point, changing code accordingly

lp:~kdub/mir/gbm-ext-v2 updated on 2017-02-22
4042. By Kevin DuBois on 2017-02-22

simplify the extensions internally to just use one struct

4043. By Kevin DuBois on 2017-02-22

merge in mir

Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4043
https://mir-jenkins.ubuntu.com/job/mir-ci/3042/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4066/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4153
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4143
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4143
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4143
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4093
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4093/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/4093
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4093/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4093/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4093
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4093/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/4093
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4093/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/4093
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4093/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
Chris Halse Rogers (raof) wrote :

(Non-blocking)
+}
+catch (...)
+{
+ return INT64_MAX;
+}

Probably (slightly) safer to return 0 here - 0 means the contents are undefined, so the client will have to do more work but is guaranteed to render correctly.

Also, uint64_t? This is almost always going to have a range of 0-3, and cannot possibly go beyond the tens of thousands (buffers will be on the order of megabytes, we're definitely not going to give a client > 10GB of VRAM just for window buffers!)

Is there a reason you picked uint64_t rather than uint32_t?

review: Approve
Daniel van Vugt (vanvugt) wrote :

I agree with the above nit. It's a count of frames so even to exceed uint32 on a 60Hz display will take 2.2 years. A client would have to remember what it rendered 2.2 years ago. So 32-bits is plenty and we also don't need an explicit bit size. Just make it "unsigned int".

review: Needs Fixing
lp:~kdub/mir/gbm-ext-v2 updated on 2017-03-06
4044. By Kevin DuBois on 2017-03-06

merge in mir

4045. By Kevin DuBois on 2017-03-06

use uint32_t and return a 0 in case of exception

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4045
https://mir-jenkins.ubuntu.com/job/mir-ci/3103/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4161
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4248
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4238
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4238
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4238
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4188/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/4188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4188/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4188/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/4188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4188/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/4188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4188/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/4188
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4188/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

Still seems like we're playing fast and loose with integer types:
  * age is uint32 but never needs to be anything larger than a uint.
  * width and height are 'int' but gbm.h uses uint32_t for them.

Please consider:
  (1) Removing the "32" from MirBufferExtAge unless the implementation is explicitly limited to 32-bits.
  (2) Converting width and height from int to uint32_t (since that's what gbm.h uses?).

We've all been guilty of choosing wrong integer types in the past, but if we're in the business of redesigning existing interfaces then at least the new design should be more correct.

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote :

(3) Shouldn't format and flags be uint32 too?

struct gbm_bo *
gbm_bo_create(struct gbm_device *gbm,
              uint32_t width, uint32_t height,
              uint32_t format, uint32_t flags);

Daniel van Vugt (vanvugt) wrote :

(4) Does MirConnectionAllocateBufferGbmSync need to exist? Is that a better design than providing a function that can just import a struct gbm_bo? (using gbm_bo_get_fd to send it to the server?)

Daniel van Vugt (vanvugt) wrote :

Oh you answered (4) in the description, sorry.

Just (1)-(3) then please.

lp:~kdub/mir/gbm-ext-v2 updated on 2017-03-08
4046. By Kevin DuBois on 2017-03-08

merge in mir

4047. By Kevin DuBois on 2017-03-08

correct types for the new extension

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4047
https://mir-jenkins.ubuntu.com/job/mir-ci/3116/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4180
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4267
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4207
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4207/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/4207
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4207/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4207
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4207/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/4207
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4207/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/4207
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4207/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/4207
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4207/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Kevin DuBois (kdub) wrote :

sure, fixed. In fixing the signature of the async buffer allocation function, had to go back to having 2 structs as extensions here.

Daniel van Vugt (vanvugt) wrote :

(5) A bunch of these functions take MirBuffer as an 'in' parameter only so should take const pointers, no?
   typedef uint32_t (*MirBufferExtFormat)(MirBuffer* buffer);

(6) Given the header is GBM-specific, is it a good idea to define callback types that are not GBM-specific in name inside it?

review: Needs Fixing
Kevin DuBois (kdub) wrote :

5) ideally, but not done in other similar situations so I'd rather just leave as-is.
6) sure, (guessing you mean MirBufferExtFd should be MirBufferGbmFd, functions aren't callbacks)

lp:~kdub/mir/gbm-ext-v2 updated on 2017-03-10
4048. By Kevin DuBois on 2017-03-10

merge in mir

4049. By Kevin DuBois on 2017-03-10

make extension fn pointers say Gbm somewhere in them, instead of Ext

4050. By Kevin DuBois on 2017-03-10

correct find-replace mistake

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4049
https://mir-jenkins.ubuntu.com/job/mir-ci/3129/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4201
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4288
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4278
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4278
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4278
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4228
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4228/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/4228
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4228/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4228
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4228/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/4228
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4228/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/4228
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4228/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/4228
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4228/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4050
https://mir-jenkins.ubuntu.com/job/mir-ci/3131/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4203
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4290
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4280
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4280
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4280
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4230
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4230/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/4230
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4230/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4230
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4230/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/4230
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4230/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/4230
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4230/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/4230
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4230/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

(5) I would ignore other extensions as prior art. Those haven't been around as long as the rest of the project where we do use "const" more correctly, so should use const correctly (at least on parameters).

(6) The new naming convention sounds confusing and looks weakly typed:
   typedef uint32_t (*MirBufferGbmStride)(MirBuffer* buffer);
What if 'buffer' is not a GBM buffer?

I'm starting to think that the client producing its own gbm_bo might be the right answer. We would have fewer functions, less confusion and stronger typing. Admittedly with some additional linkage and the client would need to specify a gbm_device. But maybe that's a good thing in a multi-GPU world. I also wonder how this plays with RAOF's multi-GPU work in progress...

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote :

(7) I should have picked a different function in (6) because I have another question...
  typedef uint32_t (*MirBufferGbmStride)(MirBuffer* buffer);
Why isn't there a generic mir_buffer_get_stride() function already?

(8) Does it make sense for a buffer's age to be queried without any context?;
  typedef unsigned int (*MirBufferGbmAge)(MirBuffer* buffer)
You may be using a buffer in two or more different rendering contexts so it can have a different age for each. I don't think MirBufferGbmAge() makes sense.

Daniel van Vugt (vanvugt) wrote :

(8) I forgot 'age' is relative to the producer (singular) and not its consumers, so it makes a little bit of sense. Although I'm not 100% convinced that age should ever be an attribute 'of' or internal to a MirBuffer. It's really an external attribute that emerges from the swapping logic, and is only associated with a buffer in the local producer context.

Kevin DuBois (kdub) wrote :

5) If we add const to this extension, we have some MirBuffer-related functions that have const and others that don't. This seems worse (in terms of update cost or mismatched signatures) than leaving as-is.
6) It is a precondition that the MirBuffer is a a GBM buffer. Stronger typing would get us in trouble with how the buffers get submitted.
7) Stride for buffer was rejected. https://code.launchpad.net/~kdub/mir/mir-buffer-get-stride/+merge/303135
8) Sounds like its alright as it is then? The mesa driver makes use of it now, if the mesa driver implements that logic, it can go away.

Daniel van Vugt (vanvugt) wrote :

I'm not convinced (5) and (8) are adequately resolved.

(5) If we make a plan to fix lack of const in 'MirBuffer *' everywhere soon, then OK. If you can change it to "const" in the below diff right now without breaking existing code then please do it before this lands.

(8) Similarly if we can cull that function and existing code still works, please do so. I'm not convinced we have the right design for handling 'age'. It's also not an important feature -- both uses of buffer age in toolkits I've ever seen were buggy and had to avoid it in the end.

Daniel van Vugt (vanvugt) wrote :

(6) is also unconvincing. We shouldn't be designing new APIs that accept MirBuffer* yet implicitly only work on GBM buffers. This would be resolved by forcing the client side to provide gbm_bo. If the extension is for "GBM" then it's perfectly reasonable that users of it will link to GBM and be using gbm.h. If however you can get away with abstracting out gbm.h entirely as you have, then in theory we can get away from calling anything in it "GBM". It should therefore either be a more abstract interface (not mention "GBM" at all) or less abstract interface and use gbm_bo directly.

I feel this GBM v2 extension will soon need to be deprecated and replaced by a v3. However I also don't have sufficient interest in this area of code to stand in the way. Rapid deprecation and redesigns might be the fastest way forward. If any reviewer disagrees with my comments then go ahead and land it...

Kevin DuBois (kdub) wrote :

5) We agree that const* would have been better, but I don't intend to fix it in this MP, as it needs updating elsewhere. I can follow up with an MP that updates everything. (I don't think adding const to the parameter breaks abi or api).
6) Linking to gbm is prohibitive in terms of the existing code. We would not only have to link to the gbm library, but plumb through fds to get the gbm device initialized. Also, the gbm initialization is currently in the mir/rs egl driver, and its not the intention of this MP to change that. The intention is to deprecate MirBufferPackage.
8) It is used to get the egl driver working, so it cannot be culled at this time, unless logic is moved (much the same story as linking to gbm).

I don't mind if we follow up with a v3, and we will have to anyways in the mid-near future when we add gbm buffer import (https://trello.com/c/Me7H4Evr).

The goal here is to get rid of MirBufferPackage before the next upload to our mesa driver.

Alan Griffiths (alan-griffiths) wrote :

OK

review: Approve
Andreas Pokorny (andreas-pokorny) wrote :

ok

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/client/mir_toolkit/extensions/gbm_buffer.h'
2--- include/client/mir_toolkit/extensions/gbm_buffer.h 2017-02-15 07:38:33 +0000
3+++ include/client/mir_toolkit/extensions/gbm_buffer.h 2017-03-10 16:32:19 +0000
4@@ -29,6 +29,8 @@
5
6 /** Allocate a MirBuffer via gbm
7 *
8+ * available in V1 and V2.
9+ *
10 * The callback will be called when the buffer is available for use.
11 * It will be called once when created, and once per every
12 * mir_presentation_chain_submit_buffer.
13@@ -53,6 +55,15 @@
14 unsigned int gbm_bo_flags,
15 MirBufferCallback available_callback, void* available_context);
16
17+/** v2 version of mir_connection_allocate_buffer_gbm, with more accurate types.
18+ */
19+typedef void (*MirConnectionAllocateBufferGbm)(
20+ MirConnection* connection,
21+ uint32_t width, uint32_t height,
22+ uint32_t gbm_pixel_format,
23+ uint32_t gbm_bo_flags,
24+ MirBufferCallback available_callback, void* available_context);
25+
26 typedef struct MirExtensionGbmBufferV1
27 {
28 mir_connection_allocate_buffer_gbm allocate_buffer_gbm;
29@@ -65,6 +76,85 @@
30 connection, "mir_extension_gbm_buffer", 1);
31 }
32
33+/** Allocate a MirBuffer via gbm and wait for the allocation.
34+ * available in V2.
35+ * The buffer can be destroyed via mir_buffer_release().
36+ *
37+ * \param [in] connection The connection
38+ * \param [in] width Requested buffer width
39+ * \param [in] height Requested buffer height
40+ * \param [in] gbm_pixel_format The pixel format, one of the GBM_FORMATs
41+ * \param [in] gbm_bo_flags The gbm_bo_flags for the buffer.
42+ * \return The buffer
43+ **/
44+typedef MirBuffer* (*MirConnectionAllocateBufferGbmSync)(
45+ MirConnection* connection,
46+ uint32_t width, uint32_t height,
47+ uint32_t gbm_pixel_format,
48+ uint32_t gbm_bo_flags);
49+
50+/** Check if a MirBuffer is suitable for import via GBM_BO_IMPORT_FD
51+ *
52+ * \param [in] buffer The buffer
53+ * \return True if suitable, false if unsuitable
54+ */
55+typedef bool (*MirBufferIsGbmImportable)(MirBuffer* buffer);
56+
57+/** Access the fd a MirBuffer suitable for gbm import
58+ * \pre The buffer is suitable for GBM_BO_IMPORT_FD
59+ * \warning The fd is owned by the buffer. Do not close() it.
60+ * \param [in] buffer The buffer
61+ * \return The fd
62+ */
63+typedef int (*MirBufferGbmFd)(MirBuffer* buffer);
64+
65+/** Get the stride of a MirBuffer
66+ * \pre The buffer is suitable for GBM_BO_IMPORT_FD
67+ * \param [in] buffer The buffer
68+ * \return The stride of the buffer
69+ */
70+typedef uint32_t (*MirBufferGbmStride)(MirBuffer* buffer);
71+
72+/** Get the GBM_FORMAT of a MirBuffer
73+ * \pre The buffer is suitable for GBM_BO_IMPORT_FD
74+ * \param [in] buffer The buffer
75+ * \return The GBM_FORMAT of the buffer
76+ */
77+typedef uint32_t (*MirBufferGbmFormat)(MirBuffer* buffer);
78+
79+/** Get the gbm_bo_flags of a MirBuffer
80+ * \pre The buffer is suitable for GBM_BO_IMPORT_FD
81+ * \param [in] buffer The buffer
82+ * \return The gbm_bo_flags of the buffer
83+ */
84+typedef uint32_t (*MirBufferGbmFlags)(MirBuffer* buffer);
85+
86+/** Get the age of a MirBuffer
87+ * \pre The buffer is suitable for GBM_BO_IMPORT_FD
88+ * \param [in] buffer The buffer
89+ * \return The age of the buffer
90+ */
91+typedef unsigned int (*MirBufferGbmAge)(MirBuffer* buffer);
92+
93+typedef struct MirExtensionGbmBufferV2
94+{
95+ MirConnectionAllocateBufferGbm allocate_buffer_gbm;
96+ MirConnectionAllocateBufferGbmSync allocate_buffer_gbm_sync;
97+ MirBufferIsGbmImportable is_gbm_importable;
98+ MirBufferGbmFd fd;
99+ MirBufferGbmStride stride;
100+ MirBufferGbmFormat format;
101+ MirBufferGbmFlags flags;
102+ MirBufferGbmAge age;
103+} MirExtensionGbmBufferV2;
104+
105+static inline MirExtensionGbmBufferV2 const* mir_extension_gbm_buffer_v2(
106+ MirConnection* connection)
107+{
108+ return (MirExtensionGbmBufferV2 const*) mir_connection_request_extension(
109+ connection, "mir_extension_gbm_buffer", 2);
110+}
111+
112 #ifdef __cplusplus
113 }
114 #endif
115
116=== modified file 'src/platforms/mesa/client/client_buffer.cpp'
117--- src/platforms/mesa/client/client_buffer.cpp 2017-02-15 07:38:33 +0000
118+++ src/platforms/mesa/client/client_buffer.cpp 2017-03-10 16:32:19 +0000
119@@ -76,10 +76,14 @@
120 size_t const size_in_bytes;
121 };
122
123-std::shared_ptr<mir::graphics::mesa::NativeBuffer> to_native_buffer(MirBufferPackage const& package)
124+std::shared_ptr<mir::graphics::mesa::NativeBuffer> to_native_buffer(
125+ MirBufferPackage const& package, bool gbm, uint32_t native_format, uint32_t native_flags)
126 {
127 auto buffer = std::make_shared<mir::graphics::mesa::NativeBuffer>();
128 *static_cast<MirBufferPackage*>(buffer.get()) = package;
129+ buffer->is_gbm_buffer = gbm;
130+ buffer->native_format = native_format;
131+ buffer->native_flags = native_flags;
132 return buffer;
133 }
134
135@@ -90,7 +94,7 @@
136 std::shared_ptr<MirBufferPackage> const& package,
137 geom::Size size, MirPixelFormat pf) :
138 buffer_file_ops{buffer_file_ops},
139- creation_package{to_native_buffer(*package)},
140+ creation_package{to_native_buffer(*package, false, 0, 0)},
141 rect({geom::Point{0, 0}, size}),
142 buffer_pf{pf},
143 egl_image_attrs{
144@@ -117,9 +121,9 @@
145 std::shared_ptr<BufferFileOps> const& buffer_file_ops,
146 std::shared_ptr<MirBufferPackage> const& package,
147 geometry::Size size,
148- unsigned int native_pf, unsigned int /*native_flags*/) :
149+ unsigned int native_pf, unsigned int native_flags) :
150 buffer_file_ops{buffer_file_ops},
151- creation_package{to_native_buffer(*package)},
152+ creation_package{to_native_buffer(*package, true, native_pf, native_flags)},
153 rect({geom::Point{0, 0}, size}),
154 buffer_pf{mir::graphics::mesa::gbm_format_to_mir_format(native_pf)},
155 egl_image_attrs{
156
157=== modified file 'src/platforms/mesa/client/client_platform.cpp'
158--- src/platforms/mesa/client/client_platform.cpp 2017-02-15 07:38:33 +0000
159+++ src/platforms/mesa/client/client_platform.cpp 2017-03-10 16:32:19 +0000
160@@ -23,7 +23,9 @@
161 #include "native_surface.h"
162 #include "mir/client_buffer_factory.h"
163 #include "mir/client_context.h"
164+#include "mir/client_buffer.h"
165 #include "mir/mir_render_surface.h"
166+#include "mir/mir_buffer.h"
167 #include "mir/weak_egl.h"
168 #include "mir/platform_message.h"
169 #include "mir_toolkit/mesa/platform_operation.h"
170@@ -33,6 +35,8 @@
171 #include <boost/throw_exception.hpp>
172 #include <stdexcept>
173 #include <cstring>
174+#include <mutex>
175+#include <condition_variable>
176
177 namespace mgm=mir::graphics::mesa;
178 namespace mcl=mir::client;
179@@ -123,9 +127,9 @@
180
181 void allocate_buffer_gbm(
182 MirConnection* connection,
183- int width, int height,
184- unsigned int gbm_pixel_format,
185- unsigned int gbm_bo_flags,
186+ uint32_t width, uint32_t height,
187+ uint32_t gbm_pixel_format,
188+ uint32_t gbm_bo_flags,
189 MirBufferCallback available_callback, void* available_context)
190 {
191 auto context = mcl::to_client_context(connection);
192@@ -134,6 +138,142 @@
193 available_callback, available_context);
194 }
195
196+void allocate_buffer_gbm_legacy(
197+ MirConnection* connection,
198+ int width, int height,
199+ unsigned int gbm_pixel_format,
200+ unsigned int gbm_bo_flags,
201+ MirBufferCallback available_callback, void* available_context)
202+{
203+ allocate_buffer_gbm(
204+ connection, static_cast<uint32_t>(width), static_cast<uint32_t>(height),
205+ static_cast<uint32_t>(gbm_pixel_format), static_cast<uint32_t>(gbm_bo_flags),
206+ available_callback, available_context);
207+}
208+
209+MirBuffer* allocate_buffer_gbm_sync(
210+ MirConnection* connection,
211+ uint32_t width, uint32_t height,
212+ uint32_t gbm_pixel_format,
213+ uint32_t gbm_bo_flags)
214+try
215+{
216+ struct BufferSync
217+ {
218+ void set_buffer(MirBuffer* b)
219+ {
220+ std::unique_lock<decltype(mutex)> lk(mutex);
221+ buffer = b;
222+ cv.notify_all();
223+ }
224+
225+ MirBuffer* wait_for_buffer()
226+ {
227+ std::unique_lock<decltype(mutex)> lk(mutex);
228+ cv.wait(lk, [this]{ return buffer; });
229+ return buffer;
230+ }
231+ private:
232+ std::mutex mutex;
233+ std::condition_variable cv;
234+ MirBuffer* buffer = nullptr;
235+ } sync;
236+
237+ allocate_buffer_gbm(
238+ connection, width, height, gbm_pixel_format, gbm_bo_flags,
239+ [](auto* b, auto* context){ reinterpret_cast<BufferSync*>(context)->set_buffer(b); }, &sync);
240+ return sync.wait_for_buffer();
241+}
242+catch (...)
243+{
244+ return nullptr;
245+}
246+
247+bool is_gbm_importable(MirBuffer* b)
248+try
249+{
250+ if (!b)
251+ return false;
252+ auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
253+ auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
254+ if (!native)
255+ return false;
256+ return native->is_gbm_buffer;
257+}
258+catch (...)
259+{
260+ return false;
261+}
262+
263+int import_fd(MirBuffer* b)
264+try
265+{
266+ if (!is_gbm_importable(b))
267+ return -1;
268+ auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
269+ auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
270+ return native->fd[0];
271+}
272+catch (...)
273+{
274+ return -1;
275+}
276+
277+uint32_t buffer_stride(MirBuffer* b)
278+try
279+{
280+ if (!is_gbm_importable(b))
281+ return 0;
282+ auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
283+ auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
284+ return native->stride;
285+}
286+catch (...)
287+{
288+ return 0;
289+}
290+
291+uint32_t buffer_format(MirBuffer* b)
292+try
293+{
294+ if (!is_gbm_importable(b))
295+ return 0;
296+ auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
297+ auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
298+ return native->native_format;
299+}
300+catch (...)
301+{
302+ return 0;
303+}
304+
305+uint32_t buffer_flags(MirBuffer* b)
306+try
307+{
308+ if (!is_gbm_importable(b))
309+ return 0;
310+ auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
311+ auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
312+ return native->native_flags;
313+}
314+catch (...)
315+{
316+ return 0;
317+}
318+
319+unsigned int buffer_age(MirBuffer* b)
320+try
321+{
322+ if (!is_gbm_importable(b))
323+ return 0;
324+ auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
325+ auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
326+ return native->age;
327+}
328+catch (...)
329+{
330+ return 0;
331+}
332 #pragma GCC diagnostic push
333 #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
334 MirBufferStream* get_hw_stream(
335@@ -164,7 +304,9 @@
336 gbm_dev{nullptr},
337 drm_extensions{auth_fd_ext, auth_magic_ext},
338 mesa_auth{set_device, this},
339- gbm_buffer{allocate_buffer_gbm},
340+ gbm_buffer1{allocate_buffer_gbm_legacy},
341+ gbm_buffer2{allocate_buffer_gbm, allocate_buffer_gbm_sync,
342+ is_gbm_importable, import_fd, buffer_stride, buffer_format, buffer_flags, buffer_age},
343 hw_stream{get_hw_stream}
344 {
345 }
346@@ -289,7 +431,9 @@
347 if (!strcmp(extension_name, "mir_extension_set_gbm_device") && (version == 1))
348 return &mesa_auth;
349 if (!strcmp(extension_name, "mir_extension_gbm_buffer") && (version == 1))
350- return &gbm_buffer;
351+ return &gbm_buffer1;
352+ if (!strcmp(extension_name, "mir_extension_gbm_buffer") && (version == 2))
353+ return &gbm_buffer2;
354 if (!strcmp(extension_name, "mir_extension_hardware_buffer_stream") && (version == 1))
355 return &hw_stream;
356
357
358=== modified file 'src/platforms/mesa/client/client_platform.h'
359--- src/platforms/mesa/client/client_platform.h 2017-02-15 07:38:33 +0000
360+++ src/platforms/mesa/client/client_platform.h 2017-03-10 16:32:19 +0000
361@@ -65,7 +65,8 @@
362 gbm_device* gbm_dev;
363 MirExtensionMesaDRMAuthV1 drm_extensions;
364 MirExtensionSetGbmDeviceV1 mesa_auth;
365- MirExtensionGbmBufferV1 gbm_buffer;
366+ MirExtensionGbmBufferV1 gbm_buffer1;
367+ MirExtensionGbmBufferV2 gbm_buffer2;
368 MirExtensionHardwareBufferStreamV1 hw_stream;
369 };
370
371
372=== modified file 'src/platforms/mesa/include/native_buffer.h'
373--- src/platforms/mesa/include/native_buffer.h 2017-01-18 02:29:37 +0000
374+++ src/platforms/mesa/include/native_buffer.h 2017-03-10 16:32:19 +0000
375@@ -34,6 +34,9 @@
376 struct NativeBuffer : graphics::NativeBuffer, MirBufferPackage
377 {
378 struct gbm_bo *bo;
379+ bool is_gbm_buffer;
380+ uint32_t native_format;
381+ uint32_t native_flags;
382 };
383 }
384 }
385
386=== modified file 'tests/unit-tests/platforms/mesa/client/test_client_platform.cpp'
387--- tests/unit-tests/platforms/mesa/client/test_client_platform.cpp 2017-01-30 08:13:20 +0000
388+++ tests/unit-tests/platforms/mesa/client/test_client_platform.cpp 2017-03-10 16:32:19 +0000
389@@ -19,9 +19,12 @@
390 #include "mir/client_platform.h"
391 #include "mir/shared_library.h"
392 #include "mir/raii.h"
393+#include "src/platforms/mesa/client/buffer_file_ops.h"
394+#include "src/platforms/mesa/client/client_buffer.h"
395 #include "src/platforms/mesa/client/mesa_native_display_container.h"
396 #include "src/client/rpc/mir_basic_rpc_channel.h"
397 #include "src/client/mir_connection.h"
398+#include "src/client/buffer.h"
399 #include "mir_test_framework/client_platform_factory.h"
400 #include "mir/test/doubles/mock_egl.h"
401 #include "mir/test/doubles/mock_egl_native_surface.h"
402@@ -50,6 +53,12 @@
403
404 namespace
405 {
406+struct StubOps : mclm::BufferFileOps
407+{
408+ int close(int) const override { return 0; }
409+ void* map(int, off_t, size_t) const override { return nullptr; }
410+ void unmap(void*, size_t) const override {}
411+};
412
413 struct StubClientContext : mcl::ClientContext
414 {
415@@ -98,10 +107,28 @@
416 return platform->platform_operation(request_msg.get());
417 }
418
419+ std::shared_ptr<MirBufferPackage> make_pkg()
420+ {
421+ auto pkg = std::make_shared<MirBufferPackage>();
422+ pkg->width = width;
423+ pkg->height = height;
424+ pkg->stride = stride;
425+ pkg->fd_items = 1;
426+ pkg->fd[0] = fake_fd;
427+ pkg->data_items = 0;
428+ return pkg;
429+ }
430+
431 StubClientContext client_context;
432 std::shared_ptr<mir::client::ClientPlatform> platform =
433 mtf::create_mesa_client_platform(&client_context);
434 mir::test::doubles::MockEGL mock_egl;
435+ int fake_fd = 11;
436+ int width = 23;
437+ int height = 230;
438+ int stride = 81290;
439+ int flags = GBM_BO_USE_RENDERING;
440+ int pf = GBM_FORMAT_ABGR8888;
441 };
442
443 }
444@@ -203,16 +230,14 @@
445 EXPECT_NE(nullptr, egl_native_window);
446 }
447
448-TEST_F(MesaClientPlatformTest, can_allocate_buffer)
449+TEST_F(MesaClientPlatformTest, can_allocate_buffer_v1)
450 {
451 using namespace std::literals::chrono_literals;
452
453 mtd::StubConnectionConfiguration conf(platform);
454 MirConnection connection(conf);
455-#pragma GCC diagnostic push
456-#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
457- mir_wait_for(connection.connect("", [](MirConnection*, void*){}, nullptr));
458-#pragma GCC diagnostic pop
459+ if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
460+ wh->wait_for_all();
461
462 int width = 32;
463 int height = 90;
464@@ -230,6 +255,73 @@
465 EXPECT_THAT(conf.channel->channel_call_count, Eq(call_count + 1));
466 }
467
468+TEST_F(MesaClientPlatformTest, can_allocate_buffer_v2)
469+{
470+ using namespace std::literals::chrono_literals;
471+
472+ mtd::StubConnectionConfiguration conf(platform);
473+ MirConnection connection(conf);
474+ if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
475+ wh->wait_for_all();
476+
477+ int width = 32;
478+ int height = 90;
479+ auto ext = static_cast<MirExtensionGbmBufferV1*>(
480+ platform->request_interface("mir_extension_gbm_buffer", 2));
481+ ASSERT_THAT(ext, Ne(nullptr));
482+ ASSERT_THAT(ext->allocate_buffer_gbm, Ne(nullptr));
483+
484+ auto call_count = conf.channel->channel_call_count;
485+ ext->allocate_buffer_gbm(
486+ &connection, width, height, pf, flags,
487+ [] (::MirBuffer*, void*) {}, nullptr);
488+ EXPECT_THAT(conf.channel->channel_call_count, Eq(call_count + 1));
489+}
490+
491+TEST_F(MesaClientPlatformTest, shm_buffer_is_not_gbm_compatible)
492+{
493+ mtd::StubConnectionConfiguration conf(platform);
494+ MirConnection connection(conf);
495+ if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
496+ wh->wait_for_all();
497+ auto ext = static_cast<MirExtensionGbmBufferV2*>(
498+ platform->request_interface("mir_extension_gbm_buffer", 2));
499+ ASSERT_THAT(ext, Ne(nullptr));
500+ ASSERT_THAT(ext->is_gbm_importable, Ne(nullptr));
501+
502+ auto client_buffer = std::make_shared<mclm::ClientBuffer>(
503+ std::make_shared<StubOps>(), make_pkg(), geom::Size{width, height}, mir_pixel_format_rgb_888);
504+ mcl::Buffer mirbuffer(nullptr, nullptr, 0, client_buffer, nullptr, mir_buffer_usage_software);
505+ EXPECT_FALSE(ext->is_gbm_importable(reinterpret_cast<MirBuffer*>(&mirbuffer)));
506+}
507+
508+TEST_F(MesaClientPlatformTest, bo_buffer_is_gbm_compatible)
509+{
510+ mtd::StubConnectionConfiguration conf(platform);
511+ MirConnection connection(conf);
512+ if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
513+ wh->wait_for_all();
514+ auto ext = static_cast<MirExtensionGbmBufferV2*>(
515+ platform->request_interface("mir_extension_gbm_buffer", 2));
516+ auto client_buffer = std::make_shared<mclm::ClientBuffer>(
517+ std::make_shared<StubOps>(), make_pkg(), geom::Size{width, height}, pf, flags);
518+ mcl::Buffer mirbuffer(nullptr, nullptr, 0, client_buffer, nullptr, mir_buffer_usage_hardware);
519+
520+ ASSERT_THAT(ext, Ne(nullptr));
521+ ASSERT_THAT(ext->is_gbm_importable, Ne(nullptr));
522+ ASSERT_THAT(ext->fd, Ne(nullptr));
523+ ASSERT_THAT(ext->stride, Ne(nullptr));
524+ ASSERT_THAT(ext->format, Ne(nullptr));
525+ ASSERT_THAT(ext->flags, Ne(nullptr));
526+ ASSERT_THAT(ext->age, Ne(nullptr));
527+ EXPECT_TRUE(ext->is_gbm_importable(reinterpret_cast<MirBuffer*>(&mirbuffer)));
528+ EXPECT_THAT(ext->fd(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(fake_fd));
529+ EXPECT_THAT(ext->stride(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(stride));
530+ EXPECT_THAT(ext->format(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(pf));
531+ EXPECT_THAT(ext->flags(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(flags));
532+ EXPECT_THAT(ext->age(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(0));
533+}
534+
535 TEST_F(MesaClientPlatformTest, converts_gbm_format_correctly)
536 {
537 EXPECT_THAT(platform->native_format_for(mir_pixel_format_argb_8888), Eq(GBM_FORMAT_ARGB8888));

Subscribers

People subscribed via source and target branches