Mir

Merge lp:~afrantzis/mir/different-mesa-display-validation-functions into lp:~mir-team/mir/trunk

  1. different-mesa-display-validation-functions
  2. Merge into trunk
Proposed by Alexandros Frantzis
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 821
Proposed branch: lp:~afrantzis/mir/different-mesa-display-validation-functions
Merge into: lp:~mir-team/mir/trunk
Diff against target: 172 lines (+17/-31)
7 files modified
include/shared/mir_toolkit/mesa/native_display.h (+1/-2)
src/client/gbm/mesa_native_display_container.cpp (+1/-2)
src/client/gbm/mesa_native_display_container.h (+5/-1)
src/server/graphics/gbm/gbm_platform.cpp (+1/-13)
src/server/graphics/gbm/gbm_platform.h (+2/-0)
tests/unit-tests/client/gbm/test_gbm_client_platform.cpp (+4/-6)
tests/unit-tests/graphics/gbm/test_gbm_platform.cpp (+3/-7)
To merge this branch: bzr merge lp:~afrantzis/mir/different-mesa-display-validation-functions
Reviewer Review Type Date Requested Status
Daniel van Vugt Abstain
Kevin DuBois (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Review via email: mp+173215@code.launchpad.net

Commit message

gbm: Provide different functions for validating server and client Mesa EGL native displays

Description of the change

gbm: Provide different functions for validating server and client Mesa EGL native displays

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

I assume the next step is to make use of these different functions?

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:818
http://jenkins.qa.ubuntu.com/job/mir-ci/922/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1186
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1070
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/160
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/160/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> I assume the next step is to make use of these different functions?

The related change in Mesa to use these was merged earlier today: https://github.com/RAOF/mesa/pull/2

People will need to upgrade to the new Mesa after this is merged (but they update Mesa can be used even without this change). I will send an email to the mir-devel to notify about the update.

Revision history for this message
Kevin DuBois (kdub) wrote :

+1!

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

Can we find a way to make the new function names less wordy and more consistent?...

mir_server_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
mir_client_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)

By wordy, I mean they're almost too long. By consistent I mean the words are in a different order in the function name to the parameter: egl_mesa vs MesaEGL

Another inconsistency is use of the word "client". No other client functions do that. So maybe drop "_client_". But admittedly that would be inconsistent too.

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

Following further discussion on IRC, I am not very opinionated on this. Land it when you've made a decision.

review: Abstain

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'include/shared/mir_toolkit/mesa/native_display.h'
--- include/shared/mir_toolkit/mesa/native_display.h 2013-06-13 21:24:29 +0000
+++ include/shared/mir_toolkit/mesa/native_display.h 2013-07-05 14:12:25 +0000
@@ -52,8 +52,7 @@
52 MirSurfaceParameters* surface_parameters);52 MirSurfaceParameters* surface_parameters);
53};53};
5454
55int mir_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display);55typedef int (*MirMesaEGLNativeDisplayIsValidFunc)(MirMesaEGLNativeDisplay* display);
56int mir_server_internal_display_is_valid(MirMesaEGLNativeDisplay* display);
5756
58#ifdef __cplusplus57#ifdef __cplusplus
59} // extern "C"58} // extern "C"
6059
=== modified file 'src/client/gbm/mesa_native_display_container.cpp'
--- src/client/gbm/mesa_native_display_container.cpp 2013-06-13 22:10:04 +0000
+++ src/client/gbm/mesa_native_display_container.cpp 2013-07-05 14:12:25 +0000
@@ -18,7 +18,6 @@
1818
19#include "mesa_native_display_container.h"19#include "mesa_native_display_container.h"
2020
21#include "mir_toolkit/mesa/native_display.h"
22#include "mir_toolkit/mir_client_library.h"21#include "mir_toolkit/mir_client_library.h"
2322
24#include <cstring>23#include <cstring>
@@ -41,7 +40,7 @@
41 return MIR_MESA_TRUE;40 return MIR_MESA_TRUE;
42}41}
4342
44int mir_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)43int mir_client_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
45{44{
46 return mcl::EGLNativeDisplayContainer::instance().validate(display);45 return mcl::EGLNativeDisplayContainer::instance().validate(display);
47}46}
4847
=== modified file 'src/client/gbm/mesa_native_display_container.h'
--- src/client/gbm/mesa_native_display_container.h 2013-06-13 22:10:04 +0000
+++ src/client/gbm/mesa_native_display_container.h 2013-07-05 14:12:25 +0000
@@ -22,6 +22,7 @@
22#include "../egl_native_display_container.h"22#include "../egl_native_display_container.h"
2323
24#include "mir_toolkit/client_types.h"24#include "mir_toolkit/client_types.h"
25#include "mir_toolkit/mesa/native_display.h"
2526
26#include <unordered_set>27#include <unordered_set>
27#include <mutex>28#include <mutex>
@@ -30,9 +31,9 @@
30{31{
31namespace client32namespace client
32{33{
33
34namespace gbm34namespace gbm
35{35{
36
36class MesaNativeDisplayContainer : public EGLNativeDisplayContainer37class MesaNativeDisplayContainer : public EGLNativeDisplayContainer
37{38{
38public:39public:
@@ -52,6 +53,9 @@
52 std::mutex mutable guard;53 std::mutex mutable guard;
53 std::unordered_set<MirEGLNativeDisplayType> valid_displays;54 std::unordered_set<MirEGLNativeDisplayType> valid_displays;
54};55};
56
57extern "C" int mir_client_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display);
58
55}59}
56}60}
57} // namespace mir61} // namespace mir
5862
=== modified file 'src/server/graphics/gbm/gbm_platform.cpp'
--- src/server/graphics/gbm/gbm_platform.cpp 2013-07-03 17:48:30 +0000
+++ src/server/graphics/gbm/gbm_platform.cpp 2013-07-05 14:12:25 +0000
@@ -153,20 +153,8 @@
153 return std::make_shared<mgg::GBMPlatform>(report, vt);153 return std::make_shared<mgg::GBMPlatform>(report, vt);
154}154}
155155
156extern "C"156extern "C" int mir_server_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
157{
158int mir_server_internal_display_is_valid(MirMesaEGLNativeDisplay* display)
159{157{
160 return ((mgg::GBMPlatform::internal_display_clients_present) &&158 return ((mgg::GBMPlatform::internal_display_clients_present) &&
161 (display == mgg::GBMPlatform::internal_native_display.get()));159 (display == mgg::GBMPlatform::internal_native_display.get()));
162}160}
163
164/* TODO: this function is a bit fragile because libmirserver and libmirclient both have very different
165 * implementations and both have symbols for it.
166 * bug filed: lp:1177902
167 */
168int mir_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display)
169{
170 return mir_server_internal_display_is_valid(display);
171}
172}
173161
=== modified file 'src/server/graphics/gbm/gbm_platform.h'
--- src/server/graphics/gbm/gbm_platform.h 2013-06-21 07:47:57 +0000
+++ src/server/graphics/gbm/gbm_platform.h 2013-07-05 14:12:25 +0000
@@ -68,6 +68,8 @@
68 static std::shared_ptr<InternalNativeDisplay> internal_native_display;68 static std::shared_ptr<InternalNativeDisplay> internal_native_display;
69};69};
7070
71extern "C" int mir_server_egl_mesa_display_is_valid(MirMesaEGLNativeDisplay* display);
72
71}73}
72}74}
73}75}
7476
=== modified file 'tests/unit-tests/client/gbm/test_gbm_client_platform.cpp'
--- tests/unit-tests/client/gbm/test_gbm_client_platform.cpp 2013-06-05 22:52:53 +0000
+++ tests/unit-tests/client/gbm/test_gbm_client_platform.cpp 2013-07-05 14:12:25 +0000
@@ -18,6 +18,7 @@
1818
19#include "src/client/client_platform.h"19#include "src/client/client_platform.h"
20#include "src/client/native_client_platform_factory.h"20#include "src/client/native_client_platform_factory.h"
21#include "src/client/gbm/mesa_native_display_container.h"
21#include "mir_test_doubles/mock_client_context.h"22#include "mir_test_doubles/mock_client_context.h"
22#include "mir_test_doubles/mock_client_surface.h"23#include "mir_test_doubles/mock_client_surface.h"
2324
@@ -26,13 +27,10 @@
26#include <gtest/gtest.h>27#include <gtest/gtest.h>
2728
28namespace mcl = mir::client;29namespace mcl = mir::client;
30namespace mclg = mir::client::gbm;
29namespace mt = mir::test;31namespace mt = mir::test;
30namespace mtd = mir::test::doubles;32namespace mtd = mir::test::doubles;
3133
32/* TODO: mir_egl_mesa_display_is_valid is a bit fragile because libmirserver and libmirclient both have very
33 * different implementations and both have symbols for it. If the linking order of the test changes,
34 * specifically, if mir_egl_mesa_display_is_valid resolves into libmirserver, then this test will break.
35 */
36TEST(GBMClientPlatformTest, egl_native_display_is_valid_until_released)34TEST(GBMClientPlatformTest, egl_native_display_is_valid_until_released)
37{35{
38 mtd::MockClientContext context;36 mtd::MockClientContext context;
@@ -44,7 +42,7 @@
44 std::shared_ptr<EGLNativeDisplayType> native_display = platform->create_egl_native_display();42 std::shared_ptr<EGLNativeDisplayType> native_display = platform->create_egl_native_display();
4543
46 nd = reinterpret_cast<MirMesaEGLNativeDisplay*>(*native_display);44 nd = reinterpret_cast<MirMesaEGLNativeDisplay*>(*native_display);
47 EXPECT_TRUE(mir_egl_mesa_display_is_valid(nd));45 EXPECT_TRUE(mclg::mir_client_egl_mesa_display_is_valid(nd));
48 }46 }
49 EXPECT_FALSE(mir_egl_mesa_display_is_valid(nd));47 EXPECT_FALSE(mclg::mir_client_egl_mesa_display_is_valid(nd));
50}48}
5149
=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_platform.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_platform.cpp 2013-07-01 06:53:02 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_platform.cpp 2013-07-05 14:12:25 +0000
@@ -190,21 +190,17 @@
190 }, std::runtime_error);190 }, std::runtime_error);
191}191}
192192
193/* TODO: this function is a bit fragile because libmirserver and libmirclient both have very different
194 * implementations and both have symbols for it. If the linking order of the test changes,
195 * specifically, if mir_egl_mesa_display_is_valid resolves into libmirclient, then this test will break.
196 */
197TEST_F(GBMGraphicsPlatform, platform_provides_validation_of_display_for_internal_clients)193TEST_F(GBMGraphicsPlatform, platform_provides_validation_of_display_for_internal_clients)
198{194{
199 MirMesaEGLNativeDisplay* native_display = nullptr;195 MirMesaEGLNativeDisplay* native_display = nullptr;
200 EXPECT_EQ(0, mir_server_internal_display_is_valid(native_display));196 EXPECT_EQ(0, mgg::mir_server_egl_mesa_display_is_valid(native_display));
201 {197 {
202 auto platform = create_platform();198 auto platform = create_platform();
203 auto client = platform->create_internal_client();199 auto client = platform->create_internal_client();
204 native_display = reinterpret_cast<MirMesaEGLNativeDisplay*>(client->egl_native_display());200 native_display = reinterpret_cast<MirMesaEGLNativeDisplay*>(client->egl_native_display());
205 EXPECT_EQ(1, mir_server_internal_display_is_valid(native_display));201 EXPECT_EQ(1, mgg::mir_server_egl_mesa_display_is_valid(native_display));
206 }202 }
207 EXPECT_EQ(0, mir_server_internal_display_is_valid(native_display));203 EXPECT_EQ(0, mgg::mir_server_egl_mesa_display_is_valid(native_display));
208}204}
209205
210namespace206namespace

Subscribers

People subscribed via source and target branches