Merge lp://staging/~kdub/mir/gbm-ext-v2 into lp://staging/mir
- gbm-ext-v2
- Merge into development-branch
Status: | Merged |
---|---|
Approved by: | Daniel van Vugt |
Approved revision: | no longer in the source branch. |
Merged at revision: | 4088 |
Proposed branch: | lp://staging/~kdub/mir/gbm-ext-v2 |
Merge into: | lp://staging/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://staging/~kdub/mir/gbm-ext-v2 |
Related bugs: |
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.staging.launchpad.net |
Commit message
add MirExtensionGbm
Integrated with the 'rs' platform (in the current naming) Mesa patch on the ML.
Description of the change
add MirExtensionGbm
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.
Mir CI Bot (mir-ci-bot) wrote : | # |
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 (*MirBufferExtF
-------
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 (*MirBufferExtS
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 (*MirBufferExtF
-------
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 (*MirBufferExtA
76
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4041
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Chris Halse Rogers (raof) wrote : | # |
This:
- gbm_buffer{
+ gbm_buffer1{
+ gbm_buffer2{
+ is_gbm_importable, import_fd, buffer_stride, buffer_format, buffer_flags, buffer_age},
is unnecessary; just change the type of gbm_buffer to MirExtensionGbm
Likewise,
if (!strcmp(
- return &gbm_buffer;
+ return &gbm_buffer1;
+ if (!strcmp(
+ return &gbm_buffer2;
Is unnecessary; just check for version <= 2, and return gbm_buffer.
Otherwise looks good!
Kevin DuBois (kdub) wrote : | # |
good point, changing code accordingly
Mir CI Bot (mir-ci-bot) wrote : | # |
FAILED: Continuous integration, rev:4043
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
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?
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".
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4045
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
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.
Daniel van Vugt (vanvugt) wrote : | # |
(3) Shouldn't format and flags be uint32 too?
struct gbm_bo *
gbm_bo_
Daniel van Vugt (vanvugt) wrote : | # |
(4) Does MirConnectionAl
Daniel van Vugt (vanvugt) wrote : | # |
Oh you answered (4) in the description, sorry.
Just (1)-(3) then please.
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4047
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
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 (*MirBufferExtF
(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?
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)
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4049
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
Mir CI Bot (mir-ci-bot) wrote : | # |
PASSED: Continuous integration, rev:4050
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild:
https:/
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 (*MirBufferGbmS
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...
Daniel van Vugt (vanvugt) wrote : | # |
(7) I should have picked a different function in (6) because I have another question...
typedef uint32_t (*MirBufferGbmS
Why isn't there a generic mir_buffer_
(8) Does it make sense for a buffer's age to be queried without any context?;
typedef unsigned int (*MirBufferGbmA
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:/
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:/
The goal here is to get rid of MirBufferPackage before the next upload to our mesa driver.
Kevin DuBois (kdub) wrote : | # |
FAILED: Continuous integration, rev:4039 /mir-jenkins. ubuntu. com/job/ mir-ci/ 3013/ /mir-jenkins. ubuntu. com/job/ build-mir/ 4023/console /mir-jenkins. ubuntu. com/job/ build-0- fetch/4110 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= vivid+overlay/ 4100 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= xenial+ overlay/ 4100 /mir-jenkins. ubuntu. com/job/ build-1- sourcepkg/ release= zesty/4100 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= clang,platform= mesa,release= zesty/4050 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= clang,platform= mesa,release= zesty/4050/ artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= xenial+ overlay/ 4050/console /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= zesty/4050 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= amd64,compiler= gcc,platform= mesa,release= zesty/4050/ artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= cross-armhf, compiler= gcc,platform= android, release= vivid+overlay/ 4050 /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 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= android, release= vivid+overlay/ 4050 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= android, release= vivid+overlay/ 4050/artifact/ output/ *zip*/output. zip /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= mesa,release= xenial+ overlay/ 4050 /mir-jenkins. ubuntu. com/job/ build-2- binpkg- mir/arch= i386,compiler= gcc,platform= mesa,release= xenial+ overlay/ 4050/artifact/ output/ *zip*/output. zip
https:/
Executed test runs:
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
deb: https:/
FAILURE: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
SUCCESS: https:/
deb: https:/
Click here to trigger a rebuild: /mir-jenkins. ubuntu. com/job/ mir-ci/ 3013/rebuild
https:/