Mir

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

Proposed by Kevin DuBois
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
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
Alan Griffiths Approve
Daniel van Vugt Needs Fixing
Mir CI Bot continuous-integration Approve
Chris Halse Rogers Approve
Cemil Azizoglu (community) Approve
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.
Revision history for this message
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)
Revision history for this message
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
Revision history for this message
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)
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

lgtm

review: Approve
Revision history for this message
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
Revision history for this message
Kevin DuBois (kdub) wrote :

good point, changing code accordingly

Revision history for this message
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)
Revision history for this message
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
Revision history for this message
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
Revision history for this message
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)
Revision history for this message
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
Revision history for this message
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);

Revision history for this message
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?)

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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

Just (1)-(3) then please.

Revision history for this message
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)
Revision history for this message
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.

Revision history for this message
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
Revision history for this message
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)

Revision history for this message
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)
Revision history for this message
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)
Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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...

Revision history for this message
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.

Revision history for this message
Kevin DuBois (kdub) wrote :
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

OK

review: Approve
Revision history for this message
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
=== modified file 'include/client/mir_toolkit/extensions/gbm_buffer.h'
--- include/client/mir_toolkit/extensions/gbm_buffer.h 2017-02-15 07:38:33 +0000
+++ include/client/mir_toolkit/extensions/gbm_buffer.h 2017-03-10 16:32:19 +0000
@@ -29,6 +29,8 @@
2929
30/** Allocate a MirBuffer via gbm30/** Allocate a MirBuffer via gbm
31 *31 *
32 * available in V1 and V2.
33 *
32 * The callback will be called when the buffer is available for use.34 * The callback will be called when the buffer is available for use.
33 * It will be called once when created, and once per every35 * It will be called once when created, and once per every
34 * mir_presentation_chain_submit_buffer.36 * mir_presentation_chain_submit_buffer.
@@ -53,6 +55,15 @@
53 unsigned int gbm_bo_flags,55 unsigned int gbm_bo_flags,
54 MirBufferCallback available_callback, void* available_context);56 MirBufferCallback available_callback, void* available_context);
5557
58/** v2 version of mir_connection_allocate_buffer_gbm, with more accurate types.
59 */
60typedef void (*MirConnectionAllocateBufferGbm)(
61 MirConnection* connection,
62 uint32_t width, uint32_t height,
63 uint32_t gbm_pixel_format,
64 uint32_t gbm_bo_flags,
65 MirBufferCallback available_callback, void* available_context);
66
56typedef struct MirExtensionGbmBufferV167typedef struct MirExtensionGbmBufferV1
57{68{
58 mir_connection_allocate_buffer_gbm allocate_buffer_gbm;69 mir_connection_allocate_buffer_gbm allocate_buffer_gbm;
@@ -65,6 +76,85 @@
65 connection, "mir_extension_gbm_buffer", 1);76 connection, "mir_extension_gbm_buffer", 1);
66}77}
6778
79/** Allocate a MirBuffer via gbm and wait for the allocation.
80 * available in V2.
81 * The buffer can be destroyed via mir_buffer_release().
82 *
83 * \param [in] connection The connection
84 * \param [in] width Requested buffer width
85 * \param [in] height Requested buffer height
86 * \param [in] gbm_pixel_format The pixel format, one of the GBM_FORMATs
87 * \param [in] gbm_bo_flags The gbm_bo_flags for the buffer.
88 * \return The buffer
89 **/
90typedef MirBuffer* (*MirConnectionAllocateBufferGbmSync)(
91 MirConnection* connection,
92 uint32_t width, uint32_t height,
93 uint32_t gbm_pixel_format,
94 uint32_t gbm_bo_flags);
95
96/** Check if a MirBuffer is suitable for import via GBM_BO_IMPORT_FD
97 *
98 * \param [in] buffer The buffer
99 * \return True if suitable, false if unsuitable
100 */
101typedef bool (*MirBufferIsGbmImportable)(MirBuffer* buffer);
102
103/** Access the fd a MirBuffer suitable for gbm import
104 * \pre The buffer is suitable for GBM_BO_IMPORT_FD
105 * \warning The fd is owned by the buffer. Do not close() it.
106 * \param [in] buffer The buffer
107 * \return The fd
108 */
109typedef int (*MirBufferGbmFd)(MirBuffer* buffer);
110
111/** Get the stride of a MirBuffer
112 * \pre The buffer is suitable for GBM_BO_IMPORT_FD
113 * \param [in] buffer The buffer
114 * \return The stride of the buffer
115 */
116typedef uint32_t (*MirBufferGbmStride)(MirBuffer* buffer);
117
118/** Get the GBM_FORMAT of a MirBuffer
119 * \pre The buffer is suitable for GBM_BO_IMPORT_FD
120 * \param [in] buffer The buffer
121 * \return The GBM_FORMAT of the buffer
122 */
123typedef uint32_t (*MirBufferGbmFormat)(MirBuffer* buffer);
124
125/** Get the gbm_bo_flags of a MirBuffer
126 * \pre The buffer is suitable for GBM_BO_IMPORT_FD
127 * \param [in] buffer The buffer
128 * \return The gbm_bo_flags of the buffer
129 */
130typedef uint32_t (*MirBufferGbmFlags)(MirBuffer* buffer);
131
132/** Get the age of a MirBuffer
133 * \pre The buffer is suitable for GBM_BO_IMPORT_FD
134 * \param [in] buffer The buffer
135 * \return The age of the buffer
136 */
137typedef unsigned int (*MirBufferGbmAge)(MirBuffer* buffer);
138
139typedef struct MirExtensionGbmBufferV2
140{
141 MirConnectionAllocateBufferGbm allocate_buffer_gbm;
142 MirConnectionAllocateBufferGbmSync allocate_buffer_gbm_sync;
143 MirBufferIsGbmImportable is_gbm_importable;
144 MirBufferGbmFd fd;
145 MirBufferGbmStride stride;
146 MirBufferGbmFormat format;
147 MirBufferGbmFlags flags;
148 MirBufferGbmAge age;
149} MirExtensionGbmBufferV2;
150
151static inline MirExtensionGbmBufferV2 const* mir_extension_gbm_buffer_v2(
152 MirConnection* connection)
153{
154 return (MirExtensionGbmBufferV2 const*) mir_connection_request_extension(
155 connection, "mir_extension_gbm_buffer", 2);
156}
157
68#ifdef __cplusplus158#ifdef __cplusplus
69}159}
70#endif160#endif
71161
=== modified file 'src/platforms/mesa/client/client_buffer.cpp'
--- src/platforms/mesa/client/client_buffer.cpp 2017-02-15 07:38:33 +0000
+++ src/platforms/mesa/client/client_buffer.cpp 2017-03-10 16:32:19 +0000
@@ -76,10 +76,14 @@
76 size_t const size_in_bytes;76 size_t const size_in_bytes;
77};77};
7878
79std::shared_ptr<mir::graphics::mesa::NativeBuffer> to_native_buffer(MirBufferPackage const& package)79std::shared_ptr<mir::graphics::mesa::NativeBuffer> to_native_buffer(
80 MirBufferPackage const& package, bool gbm, uint32_t native_format, uint32_t native_flags)
80{81{
81 auto buffer = std::make_shared<mir::graphics::mesa::NativeBuffer>();82 auto buffer = std::make_shared<mir::graphics::mesa::NativeBuffer>();
82 *static_cast<MirBufferPackage*>(buffer.get()) = package;83 *static_cast<MirBufferPackage*>(buffer.get()) = package;
84 buffer->is_gbm_buffer = gbm;
85 buffer->native_format = native_format;
86 buffer->native_flags = native_flags;
83 return buffer;87 return buffer;
84}88}
8589
@@ -90,7 +94,7 @@
90 std::shared_ptr<MirBufferPackage> const& package,94 std::shared_ptr<MirBufferPackage> const& package,
91 geom::Size size, MirPixelFormat pf) :95 geom::Size size, MirPixelFormat pf) :
92 buffer_file_ops{buffer_file_ops},96 buffer_file_ops{buffer_file_ops},
93 creation_package{to_native_buffer(*package)},97 creation_package{to_native_buffer(*package, false, 0, 0)},
94 rect({geom::Point{0, 0}, size}),98 rect({geom::Point{0, 0}, size}),
95 buffer_pf{pf},99 buffer_pf{pf},
96 egl_image_attrs{100 egl_image_attrs{
@@ -117,9 +121,9 @@
117 std::shared_ptr<BufferFileOps> const& buffer_file_ops,121 std::shared_ptr<BufferFileOps> const& buffer_file_ops,
118 std::shared_ptr<MirBufferPackage> const& package,122 std::shared_ptr<MirBufferPackage> const& package,
119 geometry::Size size,123 geometry::Size size,
120 unsigned int native_pf, unsigned int /*native_flags*/) :124 unsigned int native_pf, unsigned int native_flags) :
121 buffer_file_ops{buffer_file_ops},125 buffer_file_ops{buffer_file_ops},
122 creation_package{to_native_buffer(*package)},126 creation_package{to_native_buffer(*package, true, native_pf, native_flags)},
123 rect({geom::Point{0, 0}, size}),127 rect({geom::Point{0, 0}, size}),
124 buffer_pf{mir::graphics::mesa::gbm_format_to_mir_format(native_pf)},128 buffer_pf{mir::graphics::mesa::gbm_format_to_mir_format(native_pf)},
125 egl_image_attrs{129 egl_image_attrs{
126130
=== modified file 'src/platforms/mesa/client/client_platform.cpp'
--- src/platforms/mesa/client/client_platform.cpp 2017-02-15 07:38:33 +0000
+++ src/platforms/mesa/client/client_platform.cpp 2017-03-10 16:32:19 +0000
@@ -23,7 +23,9 @@
23#include "native_surface.h"23#include "native_surface.h"
24#include "mir/client_buffer_factory.h"24#include "mir/client_buffer_factory.h"
25#include "mir/client_context.h"25#include "mir/client_context.h"
26#include "mir/client_buffer.h"
26#include "mir/mir_render_surface.h"27#include "mir/mir_render_surface.h"
28#include "mir/mir_buffer.h"
27#include "mir/weak_egl.h"29#include "mir/weak_egl.h"
28#include "mir/platform_message.h"30#include "mir/platform_message.h"
29#include "mir_toolkit/mesa/platform_operation.h"31#include "mir_toolkit/mesa/platform_operation.h"
@@ -33,6 +35,8 @@
33#include <boost/throw_exception.hpp>35#include <boost/throw_exception.hpp>
34#include <stdexcept>36#include <stdexcept>
35#include <cstring>37#include <cstring>
38#include <mutex>
39#include <condition_variable>
3640
37namespace mgm=mir::graphics::mesa;41namespace mgm=mir::graphics::mesa;
38namespace mcl=mir::client;42namespace mcl=mir::client;
@@ -123,9 +127,9 @@
123127
124void allocate_buffer_gbm(128void allocate_buffer_gbm(
125 MirConnection* connection,129 MirConnection* connection,
126 int width, int height,130 uint32_t width, uint32_t height,
127 unsigned int gbm_pixel_format,131 uint32_t gbm_pixel_format,
128 unsigned int gbm_bo_flags,132 uint32_t gbm_bo_flags,
129 MirBufferCallback available_callback, void* available_context)133 MirBufferCallback available_callback, void* available_context)
130{134{
131 auto context = mcl::to_client_context(connection);135 auto context = mcl::to_client_context(connection);
@@ -134,6 +138,142 @@
134 available_callback, available_context); 138 available_callback, available_context);
135}139}
136140
141void allocate_buffer_gbm_legacy(
142 MirConnection* connection,
143 int width, int height,
144 unsigned int gbm_pixel_format,
145 unsigned int gbm_bo_flags,
146 MirBufferCallback available_callback, void* available_context)
147{
148 allocate_buffer_gbm(
149 connection, static_cast<uint32_t>(width), static_cast<uint32_t>(height),
150 static_cast<uint32_t>(gbm_pixel_format), static_cast<uint32_t>(gbm_bo_flags),
151 available_callback, available_context);
152}
153
154MirBuffer* allocate_buffer_gbm_sync(
155 MirConnection* connection,
156 uint32_t width, uint32_t height,
157 uint32_t gbm_pixel_format,
158 uint32_t gbm_bo_flags)
159try
160{
161 struct BufferSync
162 {
163 void set_buffer(MirBuffer* b)
164 {
165 std::unique_lock<decltype(mutex)> lk(mutex);
166 buffer = b;
167 cv.notify_all();
168 }
169
170 MirBuffer* wait_for_buffer()
171 {
172 std::unique_lock<decltype(mutex)> lk(mutex);
173 cv.wait(lk, [this]{ return buffer; });
174 return buffer;
175 }
176 private:
177 std::mutex mutex;
178 std::condition_variable cv;
179 MirBuffer* buffer = nullptr;
180 } sync;
181
182 allocate_buffer_gbm(
183 connection, width, height, gbm_pixel_format, gbm_bo_flags,
184 [](auto* b, auto* context){ reinterpret_cast<BufferSync*>(context)->set_buffer(b); }, &sync);
185 return sync.wait_for_buffer();
186}
187catch (...)
188{
189 return nullptr;
190}
191
192bool is_gbm_importable(MirBuffer* b)
193try
194{
195 if (!b)
196 return false;
197 auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
198 auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
199 if (!native)
200 return false;
201 return native->is_gbm_buffer;
202}
203catch (...)
204{
205 return false;
206}
207
208int import_fd(MirBuffer* b)
209try
210{
211 if (!is_gbm_importable(b))
212 return -1;
213 auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
214 auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
215 return native->fd[0];
216}
217catch (...)
218{
219 return -1;
220}
221
222uint32_t buffer_stride(MirBuffer* b)
223try
224{
225 if (!is_gbm_importable(b))
226 return 0;
227 auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
228 auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
229 return native->stride;
230}
231catch (...)
232{
233 return 0;
234}
235
236uint32_t buffer_format(MirBuffer* b)
237try
238{
239 if (!is_gbm_importable(b))
240 return 0;
241 auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
242 auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
243 return native->native_format;
244}
245catch (...)
246{
247 return 0;
248}
249
250uint32_t buffer_flags(MirBuffer* b)
251try
252{
253 if (!is_gbm_importable(b))
254 return 0;
255 auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
256 auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
257 return native->native_flags;
258}
259catch (...)
260{
261 return 0;
262}
263
264unsigned int buffer_age(MirBuffer* b)
265try
266{
267 if (!is_gbm_importable(b))
268 return 0;
269 auto buffer = reinterpret_cast<mcl::MirBuffer*>(b);
270 auto native = dynamic_cast<mgm::NativeBuffer*>(buffer->client_buffer()->native_buffer_handle().get());
271 return native->age;
272}
273catch (...)
274{
275 return 0;
276}
137#pragma GCC diagnostic push277#pragma GCC diagnostic push
138#pragma GCC diagnostic ignored "-Wdeprecated-declarations"278#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
139MirBufferStream* get_hw_stream(279MirBufferStream* get_hw_stream(
@@ -164,7 +304,9 @@
164 gbm_dev{nullptr},304 gbm_dev{nullptr},
165 drm_extensions{auth_fd_ext, auth_magic_ext},305 drm_extensions{auth_fd_ext, auth_magic_ext},
166 mesa_auth{set_device, this},306 mesa_auth{set_device, this},
167 gbm_buffer{allocate_buffer_gbm},307 gbm_buffer1{allocate_buffer_gbm_legacy},
308 gbm_buffer2{allocate_buffer_gbm, allocate_buffer_gbm_sync,
309 is_gbm_importable, import_fd, buffer_stride, buffer_format, buffer_flags, buffer_age},
168 hw_stream{get_hw_stream}310 hw_stream{get_hw_stream}
169{311{
170}312}
@@ -289,7 +431,9 @@
289 if (!strcmp(extension_name, "mir_extension_set_gbm_device") && (version == 1))431 if (!strcmp(extension_name, "mir_extension_set_gbm_device") && (version == 1))
290 return &mesa_auth;432 return &mesa_auth;
291 if (!strcmp(extension_name, "mir_extension_gbm_buffer") && (version == 1))433 if (!strcmp(extension_name, "mir_extension_gbm_buffer") && (version == 1))
292 return &gbm_buffer;434 return &gbm_buffer1;
435 if (!strcmp(extension_name, "mir_extension_gbm_buffer") && (version == 2))
436 return &gbm_buffer2;
293 if (!strcmp(extension_name, "mir_extension_hardware_buffer_stream") && (version == 1))437 if (!strcmp(extension_name, "mir_extension_hardware_buffer_stream") && (version == 1))
294 return &hw_stream;438 return &hw_stream;
295439
296440
=== modified file 'src/platforms/mesa/client/client_platform.h'
--- src/platforms/mesa/client/client_platform.h 2017-02-15 07:38:33 +0000
+++ src/platforms/mesa/client/client_platform.h 2017-03-10 16:32:19 +0000
@@ -65,7 +65,8 @@
65 gbm_device* gbm_dev;65 gbm_device* gbm_dev;
66 MirExtensionMesaDRMAuthV1 drm_extensions;66 MirExtensionMesaDRMAuthV1 drm_extensions;
67 MirExtensionSetGbmDeviceV1 mesa_auth;67 MirExtensionSetGbmDeviceV1 mesa_auth;
68 MirExtensionGbmBufferV1 gbm_buffer;68 MirExtensionGbmBufferV1 gbm_buffer1;
69 MirExtensionGbmBufferV2 gbm_buffer2;
69 MirExtensionHardwareBufferStreamV1 hw_stream;70 MirExtensionHardwareBufferStreamV1 hw_stream;
70};71};
7172
7273
=== modified file 'src/platforms/mesa/include/native_buffer.h'
--- src/platforms/mesa/include/native_buffer.h 2017-01-18 02:29:37 +0000
+++ src/platforms/mesa/include/native_buffer.h 2017-03-10 16:32:19 +0000
@@ -34,6 +34,9 @@
34struct NativeBuffer : graphics::NativeBuffer, MirBufferPackage34struct NativeBuffer : graphics::NativeBuffer, MirBufferPackage
35{35{
36 struct gbm_bo *bo;36 struct gbm_bo *bo;
37 bool is_gbm_buffer;
38 uint32_t native_format;
39 uint32_t native_flags;
37};40};
38}41}
39}42}
4043
=== modified file 'tests/unit-tests/platforms/mesa/client/test_client_platform.cpp'
--- tests/unit-tests/platforms/mesa/client/test_client_platform.cpp 2017-01-30 08:13:20 +0000
+++ tests/unit-tests/platforms/mesa/client/test_client_platform.cpp 2017-03-10 16:32:19 +0000
@@ -19,9 +19,12 @@
19#include "mir/client_platform.h"19#include "mir/client_platform.h"
20#include "mir/shared_library.h"20#include "mir/shared_library.h"
21#include "mir/raii.h"21#include "mir/raii.h"
22#include "src/platforms/mesa/client/buffer_file_ops.h"
23#include "src/platforms/mesa/client/client_buffer.h"
22#include "src/platforms/mesa/client/mesa_native_display_container.h"24#include "src/platforms/mesa/client/mesa_native_display_container.h"
23#include "src/client/rpc/mir_basic_rpc_channel.h"25#include "src/client/rpc/mir_basic_rpc_channel.h"
24#include "src/client/mir_connection.h"26#include "src/client/mir_connection.h"
27#include "src/client/buffer.h"
25#include "mir_test_framework/client_platform_factory.h"28#include "mir_test_framework/client_platform_factory.h"
26#include "mir/test/doubles/mock_egl.h"29#include "mir/test/doubles/mock_egl.h"
27#include "mir/test/doubles/mock_egl_native_surface.h"30#include "mir/test/doubles/mock_egl_native_surface.h"
@@ -50,6 +53,12 @@
5053
51namespace54namespace
52{55{
56struct StubOps : mclm::BufferFileOps
57{
58 int close(int) const override { return 0; }
59 void* map(int, off_t, size_t) const override { return nullptr; }
60 void unmap(void*, size_t) const override {}
61};
5362
54struct StubClientContext : mcl::ClientContext63struct StubClientContext : mcl::ClientContext
55{64{
@@ -98,10 +107,28 @@
98 return platform->platform_operation(request_msg.get());107 return platform->platform_operation(request_msg.get());
99 }108 }
100109
110 std::shared_ptr<MirBufferPackage> make_pkg()
111 {
112 auto pkg = std::make_shared<MirBufferPackage>();
113 pkg->width = width;
114 pkg->height = height;
115 pkg->stride = stride;
116 pkg->fd_items = 1;
117 pkg->fd[0] = fake_fd;
118 pkg->data_items = 0;
119 return pkg;
120 }
121
101 StubClientContext client_context;122 StubClientContext client_context;
102 std::shared_ptr<mir::client::ClientPlatform> platform =123 std::shared_ptr<mir::client::ClientPlatform> platform =
103 mtf::create_mesa_client_platform(&client_context);124 mtf::create_mesa_client_platform(&client_context);
104 mir::test::doubles::MockEGL mock_egl;125 mir::test::doubles::MockEGL mock_egl;
126 int fake_fd = 11;
127 int width = 23;
128 int height = 230;
129 int stride = 81290;
130 int flags = GBM_BO_USE_RENDERING;
131 int pf = GBM_FORMAT_ABGR8888;
105};132};
106133
107}134}
@@ -203,16 +230,14 @@
203 EXPECT_NE(nullptr, egl_native_window);230 EXPECT_NE(nullptr, egl_native_window);
204}231}
205232
206TEST_F(MesaClientPlatformTest, can_allocate_buffer)233TEST_F(MesaClientPlatformTest, can_allocate_buffer_v1)
207{234{
208 using namespace std::literals::chrono_literals;235 using namespace std::literals::chrono_literals;
209236
210 mtd::StubConnectionConfiguration conf(platform);237 mtd::StubConnectionConfiguration conf(platform);
211 MirConnection connection(conf);238 MirConnection connection(conf);
212#pragma GCC diagnostic push239 if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
213#pragma GCC diagnostic ignored "-Wdeprecated-declarations"240 wh->wait_for_all();
214 mir_wait_for(connection.connect("", [](MirConnection*, void*){}, nullptr));
215#pragma GCC diagnostic pop
216241
217 int width = 32;242 int width = 32;
218 int height = 90;243 int height = 90;
@@ -230,6 +255,73 @@
230 EXPECT_THAT(conf.channel->channel_call_count, Eq(call_count + 1));255 EXPECT_THAT(conf.channel->channel_call_count, Eq(call_count + 1));
231}256}
232257
258TEST_F(MesaClientPlatformTest, can_allocate_buffer_v2)
259{
260 using namespace std::literals::chrono_literals;
261
262 mtd::StubConnectionConfiguration conf(platform);
263 MirConnection connection(conf);
264 if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
265 wh->wait_for_all();
266
267 int width = 32;
268 int height = 90;
269 auto ext = static_cast<MirExtensionGbmBufferV1*>(
270 platform->request_interface("mir_extension_gbm_buffer", 2));
271 ASSERT_THAT(ext, Ne(nullptr));
272 ASSERT_THAT(ext->allocate_buffer_gbm, Ne(nullptr));
273
274 auto call_count = conf.channel->channel_call_count;
275 ext->allocate_buffer_gbm(
276 &connection, width, height, pf, flags,
277 [] (::MirBuffer*, void*) {}, nullptr);
278 EXPECT_THAT(conf.channel->channel_call_count, Eq(call_count + 1));
279}
280
281TEST_F(MesaClientPlatformTest, shm_buffer_is_not_gbm_compatible)
282{
283 mtd::StubConnectionConfiguration conf(platform);
284 MirConnection connection(conf);
285 if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
286 wh->wait_for_all();
287 auto ext = static_cast<MirExtensionGbmBufferV2*>(
288 platform->request_interface("mir_extension_gbm_buffer", 2));
289 ASSERT_THAT(ext, Ne(nullptr));
290 ASSERT_THAT(ext->is_gbm_importable, Ne(nullptr));
291
292 auto client_buffer = std::make_shared<mclm::ClientBuffer>(
293 std::make_shared<StubOps>(), make_pkg(), geom::Size{width, height}, mir_pixel_format_rgb_888);
294 mcl::Buffer mirbuffer(nullptr, nullptr, 0, client_buffer, nullptr, mir_buffer_usage_software);
295 EXPECT_FALSE(ext->is_gbm_importable(reinterpret_cast<MirBuffer*>(&mirbuffer)));
296}
297
298TEST_F(MesaClientPlatformTest, bo_buffer_is_gbm_compatible)
299{
300 mtd::StubConnectionConfiguration conf(platform);
301 MirConnection connection(conf);
302 if (auto wh = connection.connect("", [](MirConnection*, void*){}, nullptr))
303 wh->wait_for_all();
304 auto ext = static_cast<MirExtensionGbmBufferV2*>(
305 platform->request_interface("mir_extension_gbm_buffer", 2));
306 auto client_buffer = std::make_shared<mclm::ClientBuffer>(
307 std::make_shared<StubOps>(), make_pkg(), geom::Size{width, height}, pf, flags);
308 mcl::Buffer mirbuffer(nullptr, nullptr, 0, client_buffer, nullptr, mir_buffer_usage_hardware);
309
310 ASSERT_THAT(ext, Ne(nullptr));
311 ASSERT_THAT(ext->is_gbm_importable, Ne(nullptr));
312 ASSERT_THAT(ext->fd, Ne(nullptr));
313 ASSERT_THAT(ext->stride, Ne(nullptr));
314 ASSERT_THAT(ext->format, Ne(nullptr));
315 ASSERT_THAT(ext->flags, Ne(nullptr));
316 ASSERT_THAT(ext->age, Ne(nullptr));
317 EXPECT_TRUE(ext->is_gbm_importable(reinterpret_cast<MirBuffer*>(&mirbuffer)));
318 EXPECT_THAT(ext->fd(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(fake_fd));
319 EXPECT_THAT(ext->stride(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(stride));
320 EXPECT_THAT(ext->format(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(pf));
321 EXPECT_THAT(ext->flags(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(flags));
322 EXPECT_THAT(ext->age(reinterpret_cast<MirBuffer*>(&mirbuffer)), Eq(0));
323}
324
233TEST_F(MesaClientPlatformTest, converts_gbm_format_correctly)325TEST_F(MesaClientPlatformTest, converts_gbm_format_correctly)
234{326{
235 EXPECT_THAT(platform->native_format_for(mir_pixel_format_argb_8888), Eq(GBM_FORMAT_ARGB8888));327 EXPECT_THAT(platform->native_format_for(mir_pixel_format_argb_8888), Eq(GBM_FORMAT_ARGB8888));

Subscribers

People subscribed via source and target branches